From c6e07521431ce6d45a0ea2d220df456a7785087c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20M=C3=A4tje?= Date: Thu, 21 Aug 2025 16:34:21 +0200 Subject: [PATCH 1/2] can: esd_usb: Rework display of error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - esd_usb_open(): Get rid of duplicate "couldn't start device: %d\n" message already printed from esd_usb_start(). - Fix duplicate printout of network device name when network device is registered. Add an unregister message for the network device as counterpart to the register message. - Add the printout of error codes together with the error messages in esd_usb_close() and some in esd_usb_probe(). The additional error codes should lead to a better understanding what is really going wrong. - Convert all occurrences of error status prints to use "ERR_PTR(err)" instead of printing the decimal value of "err". - Rename retval to err in esd_usb_read_bulk_callback() to make the naming of error status variables consistent with all other functions. Signed-off-by: Stefan Mätje Link: https://patch.msgid.link/20250821143422.3567029-5-stefan.maetje@esd.eu [mkl: minor change patch description to imperative language] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/usb/esd_usb.c | 36 +++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c index 27a3818885c2..14b5df4d5543 100644 --- a/drivers/net/can/usb/esd_usb.c +++ b/drivers/net/can/usb/esd_usb.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -480,7 +481,7 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv, static void esd_usb_read_bulk_callback(struct urb *urb) { struct esd_usb *dev = urb->context; - int retval; + int err; int pos = 0; int i; @@ -496,7 +497,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb) default: dev_info(dev->udev->dev.parent, - "Rx URB aborted (%d)\n", urb->status); + "Rx URB aborted (%pe)\n", ERR_PTR(urb->status)); goto resubmit_urb; } @@ -539,15 +540,15 @@ resubmit_urb: urb->transfer_buffer, ESD_USB_RX_BUFFER_SIZE, esd_usb_read_bulk_callback, dev); - retval = usb_submit_urb(urb, GFP_ATOMIC); - if (retval == -ENODEV) { + err = usb_submit_urb(urb, GFP_ATOMIC); + if (err == -ENODEV) { for (i = 0; i < dev->net_count; i++) { if (dev->nets[i]) netif_device_detach(dev->nets[i]->netdev); } - } else if (retval) { + } else if (err) { dev_err(dev->udev->dev.parent, - "failed resubmitting read bulk urb: %d\n", retval); + "failed resubmitting read bulk urb: %pe\n", ERR_PTR(err)); } } @@ -572,7 +573,7 @@ static void esd_usb_write_bulk_callback(struct urb *urb) return; if (urb->status) - netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status); + netdev_info(netdev, "Tx URB aborted (%pe)\n", ERR_PTR(urb->status)); netif_trans_update(netdev); } @@ -758,7 +759,7 @@ out: if (err == -ENODEV) netif_device_detach(netdev); if (err) - netdev_err(netdev, "couldn't start device: %d\n", err); + netdev_err(netdev, "couldn't start device: %pe\n", ERR_PTR(err)); kfree(msg); return err; @@ -800,7 +801,6 @@ static int esd_usb_open(struct net_device *netdev) /* finally start device */ err = esd_usb_start(priv); if (err) { - netdev_warn(netdev, "couldn't start device: %d\n", err); close_candev(netdev); return err; } @@ -923,7 +923,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb, if (err == -ENODEV) netif_device_detach(netdev); else - netdev_warn(netdev, "failed tx_urb %d\n", err); + netdev_warn(netdev, "failed tx_urb %pe\n", ERR_PTR(err)); goto releasebuf; } @@ -951,6 +951,7 @@ static int esd_usb_close(struct net_device *netdev) { struct esd_usb_net_priv *priv = netdev_priv(netdev); union esd_usb_msg *msg; + int err; int i; msg = kmalloc(sizeof(*msg), GFP_KERNEL); @@ -964,8 +965,9 @@ static int esd_usb_close(struct net_device *netdev) msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */ for (i = 0; i <= ESD_USB_MAX_ID_SEGMENT; i++) msg->filter.mask[i] = 0; - if (esd_usb_send_msg(priv->usb, msg) < 0) - netdev_err(netdev, "sending idadd message failed\n"); + err = esd_usb_send_msg(priv->usb, msg); + if (err < 0) + netdev_err(netdev, "sending idadd message failed: %pe\n", ERR_PTR(err)); /* set CAN controller to reset mode */ msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */ @@ -973,8 +975,9 @@ static int esd_usb_close(struct net_device *netdev) msg->setbaud.net = priv->index; msg->setbaud.rsvd = 0; msg->setbaud.baud = cpu_to_le32(ESD_USB_NO_BAUDRATE); - if (esd_usb_send_msg(priv->usb, msg) < 0) - netdev_err(netdev, "sending setbaud message failed\n"); + err = esd_usb_send_msg(priv->usb, msg); + if (err < 0) + netdev_err(netdev, "sending setbaud message failed: %pe\n", ERR_PTR(err)); priv->can.state = CAN_STATE_STOPPED; @@ -1251,14 +1254,14 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index) err = register_candev(netdev); if (err) { - dev_err(&intf->dev, "couldn't register CAN device: %d\n", err); + dev_err(&intf->dev, "couldn't register CAN device: %pe\n", ERR_PTR(err)); free_candev(netdev); err = -ENOMEM; goto done; } dev->nets[index] = priv; - netdev_info(netdev, "device %s registered\n", netdev->name); + netdev_info(netdev, "registered\n"); done: return err; @@ -1357,6 +1360,7 @@ static void esd_usb_disconnect(struct usb_interface *intf) for (i = 0; i < dev->net_count; i++) { if (dev->nets[i]) { netdev = dev->nets[i]->netdev; + netdev_info(netdev, "unregister\n"); unregister_netdev(netdev); free_candev(netdev); } From 37dc3ea4d2a21f3b0c474ae3aeebcfada6a29655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20M=C3=A4tje?= Date: Thu, 21 Aug 2025 16:34:22 +0200 Subject: [PATCH 2/2] can: esd_usb: Avoid errors triggered from USB disconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The USB stack calls during disconnect the esd_usb_disconnect() callback. esd_usb_disconnect() calls netdev_unregister() for each network which in turn calls the net_device_ops::ndo_stop callback esd_usb_close() if the net device is up. The esd_usb_close() callback tries to disable all CAN Ids and to reset the CAN controller of the device sending appropriate control messages. Sending these messages in .disconnect() is moot and always fails because either the device is gone or the USB communication is already torn down by the USB stack in the course of a rmmod operation. Move the code that sends these control messages to a new function esd_usb_stop() which is approximately the counterpart of esd_usb_start() to make code structure less convoluted. Then change esd_usb_close() not to send the control messages at all if the ndo_stop() callback is executed from the USB .disconnect() callback. Add a new flag in_usb_disconnect to the struct esd_usb device structure to mark this condition which is checked by esd_usb_close() whether to skip the send operations in esd_usb_start(). Signed-off-by: Stefan Mätje Link: https://patch.msgid.link/20250821143422.3567029-6-stefan.maetje@esd.eu [mkl: minor change patch description to imperative language] Signed-off-by: Marc Kleine-Budde --- drivers/net/can/usb/esd_usb.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c index 14b5df4d5543..9bc1824d7be6 100644 --- a/drivers/net/can/usb/esd_usb.c +++ b/drivers/net/can/usb/esd_usb.c @@ -275,6 +275,7 @@ struct esd_usb { int net_count; u32 version; int rxinitdone; + int in_usb_disconnect; void *rxbuf[ESD_USB_MAX_RX_URBS]; dma_addr_t rxbuf_dma[ESD_USB_MAX_RX_URBS]; }; @@ -947,9 +948,9 @@ nourbmem: return ret; } -static int esd_usb_close(struct net_device *netdev) +/* Stop interface */ +static int esd_usb_stop(struct esd_usb_net_priv *priv) { - struct esd_usb_net_priv *priv = netdev_priv(netdev); union esd_usb_msg *msg; int err; int i; @@ -966,8 +967,10 @@ static int esd_usb_close(struct net_device *netdev) for (i = 0; i <= ESD_USB_MAX_ID_SEGMENT; i++) msg->filter.mask[i] = 0; err = esd_usb_send_msg(priv->usb, msg); - if (err < 0) - netdev_err(netdev, "sending idadd message failed: %pe\n", ERR_PTR(err)); + if (err < 0) { + netdev_err(priv->netdev, "sending idadd message failed: %pe\n", ERR_PTR(err)); + goto bail; + } /* set CAN controller to reset mode */ msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */ @@ -977,7 +980,23 @@ static int esd_usb_close(struct net_device *netdev) msg->setbaud.baud = cpu_to_le32(ESD_USB_NO_BAUDRATE); err = esd_usb_send_msg(priv->usb, msg); if (err < 0) - netdev_err(netdev, "sending setbaud message failed: %pe\n", ERR_PTR(err)); + netdev_err(priv->netdev, "sending setbaud message failed: %pe\n", ERR_PTR(err)); + +bail: + kfree(msg); + + return err; +} + +static int esd_usb_close(struct net_device *netdev) +{ + struct esd_usb_net_priv *priv = netdev_priv(netdev); + int err = 0; + + if (!priv->usb->in_usb_disconnect) { + /* It's moot to try this in usb_disconnect()! */ + err = esd_usb_stop(priv); + } priv->can.state = CAN_STATE_STOPPED; @@ -985,9 +1004,7 @@ static int esd_usb_close(struct net_device *netdev) close_candev(netdev); - kfree(msg); - - return 0; + return err; } static const struct net_device_ops esd_usb_netdev_ops = { @@ -1357,6 +1374,7 @@ static void esd_usb_disconnect(struct usb_interface *intf) usb_set_intfdata(intf, NULL); if (dev) { + dev->in_usb_disconnect = 1; for (i = 0; i < dev->net_count; i++) { if (dev->nets[i]) { netdev = dev->nets[i]->netdev;