[PATCH] can: etas_es58x: do not send disable channel command if device is unplugged

[PATCH] can: etas_es58x: do not send disable channel command if device is unplugged

[Vincent Mailhol]

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(-)

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);
-- 
2.39.1

Re: [PATCH] can: etas_es58x: do not send disable channel command if device is unplugged

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: etas_es58x: do not send disable channel command if device is unplugged

[Greg Kroah-Hartman]

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