| Message ID | 20230128133815.1796221-1-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: etas_es58x: do not send disable channel command if device is unplugged | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On 28.01.2023 22:38:15, Vincent Mailhol wrote: > When turning the network interface down, es58x_stop() is called and > will send a command to the ES58x device to disable the channel > c.f. es58x_ops::disable_channel(). > > However, if the device gets unplugged while the network interface is > still up, es58x_ops::disable_channel() will obviously fail to send the > URB command and the driver emits below error message: > > es58x_submit_urb: USB send urb failure: -ENODEV > > Check the usb device state before sending the disable channel command > in order to silence above error message. > > Update the documentation of es58x_stop() accordingly. > > The check being added in es58x_stop() is: > > if (es58x_dev->udev->state >= USB_STATE_UNAUTHENTICATED) > > This is just the negation of the check done in usb_submit_urb()[1]. > > [1] usb_submit_urb(), verify usb device's state. > Link: https://elixir.bootlin.com/linux/v6.1/source/drivers/usb/core/urb.c#L384 > > Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces") > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > As far as I know, there doesn't seem to be an helper function to check > udev->state values. If anyone is aware of such helper function, let me > know.. The constant USB_STATE_UNAUTHENTICATED is not used very often in the kernel. Maybe Greg can point out what is best to do here. regards, Marc
On Thu, Feb 02, 2023 at 04:16:22PM +0100, Marc Kleine-Budde wrote: > On 28.01.2023 22:38:15, Vincent Mailhol wrote: > > When turning the network interface down, es58x_stop() is called and > > will send a command to the ES58x device to disable the channel > > c.f. es58x_ops::disable_channel(). > > > > However, if the device gets unplugged while the network interface is > > still up, es58x_ops::disable_channel() will obviously fail to send the > > URB command and the driver emits below error message: > > > > es58x_submit_urb: USB send urb failure: -ENODEV > > > > Check the usb device state before sending the disable channel command > > in order to silence above error message. > > > > Update the documentation of es58x_stop() accordingly. > > > > The check being added in es58x_stop() is: > > > > if (es58x_dev->udev->state >= USB_STATE_UNAUTHENTICATED) > > > > This is just the negation of the check done in usb_submit_urb()[1]. > > > > [1] usb_submit_urb(), verify usb device's state. > > Link: https://elixir.bootlin.com/linux/v6.1/source/drivers/usb/core/urb.c#L384 > > > > Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces") > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > --- > > As far as I know, there doesn't seem to be an helper function to check > > udev->state values. If anyone is aware of such helper function, let me > > know.. > > The constant USB_STATE_UNAUTHENTICATED is not used very often in the > kernel. Maybe Greg can point out what is best to do here. Don't do anything in the driver here, except maybe turn off that useless "error message" in the urb submission function? A USB driver shouldn't be poking around in the state of the device at all, as it's about to go away if it's been physically removed. The urb submission will fail if the device is removed, handle the error and move on, that's a totally normal situation as you say it happens a lot :) Don't spam error logs for common things please. thanks, greg k-h
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c index 3e87f4c1547c..916bd9e2e9ea 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.c +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c @@ -1817,9 +1817,10 @@ static int es58x_open(struct net_device *netdev) * es58x_stop() - Disable the network device. * @netdev: CAN network device. * - * Called when the network transitions to the down state. If all the - * channels of the device are closed, free the URB resources which are - * not needed anymore. + * Called when the network interface transitions to the down + * state. Send a disable command to the device if it is still + * connected. If all the channels of the device are closed, free the + * URB resources which are not needed anymore. * * Return: zero on success, errno when any error occurs. */ @@ -1830,9 +1831,12 @@ static int es58x_stop(struct net_device *netdev) int ret; netif_stop_queue(netdev); - ret = es58x_dev->ops->disable_channel(priv); - if (ret) - return ret; + + if (es58x_dev->udev->state >= USB_STATE_UNAUTHENTICATED) { + ret = es58x_dev->ops->disable_channel(priv); + if (ret) + return ret; + } priv->can.state = CAN_STATE_STOPPED; es58x_can_reset_echo_fifo(netdev);
When turning the network interface down, es58x_stop() is called and will send a command to the ES58x device to disable the channel c.f. es58x_ops::disable_channel(). However, if the device gets unplugged while the network interface is still up, es58x_ops::disable_channel() will obviously fail to send the URB command and the driver emits below error message: es58x_submit_urb: USB send urb failure: -ENODEV Check the usb device state before sending the disable channel command in order to silence above error message. Update the documentation of es58x_stop() accordingly. The check being added in es58x_stop() is: if (es58x_dev->udev->state >= USB_STATE_UNAUTHENTICATED) This is just the negation of the check done in usb_submit_urb()[1]. [1] usb_submit_urb(), verify usb device's state. Link: https://elixir.bootlin.com/linux/v6.1/source/drivers/usb/core/urb.c#L384 Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces") Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- As far as I know, there doesn't seem to be an helper function to check udev->state values. If anyone is aware of such helper function, let me know.. --- drivers/net/can/usb/etas_es58x/es58x_core.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)