[PATCH 0/6] can: esd_usb: Fixes and improvements

[PATCH 0/6] can: esd_usb: Fixes and improvements

[Stefan Mätje]

The second patch fixes a condition where the esd_usb CAN driver
may not detect connected CAN-USB devices correctly after a
reboot. This patch was already presented on the list and changes
due to that feedback are integrated.

References:
https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r

The first patch is only a prerequisite for the second one that
simply avoids calling kfree() with NULL pointers.

The third patch fixes situations where the the handling of TX
context objects for each sent CAN frame could go out of sync
with the acknowledged or erroneous TX jobs and then lose free
TX context objects. This could lead to the driver incapable of
sending frames.

The forth patch adds TX FIFO watermark to eliminate occasional
error messages and significantly reduce the number of calls to
netif_start_queue() and netif_stop_queue().

The fifth patch makes some error messages also print the error
code to achieve a higher significance. Removes also a duplicate
message and makes the register / unregister messages symmetric.

The sixth patch avoids confusing error messages when
disconnecting CAN-USB devices.

Stefan Mätje (6):
  can: esd_usb: Fix possible calls to kfree() with NULL
  can: esd_usb: Fix not detecting version reply in probe routine
  can: esd_usb: Fix handling of TX context objects
  can: esd_usb: Add watermark handling for TX jobs
  can: esd_usb: Rework display of error messages
  can: esd_usb: Avoid errors triggered from USB disconnect

 drivers/net/can/usb/esd_usb.c | 213 +++++++++++++++++++++++++---------
 1 file changed, 156 insertions(+), 57 deletions(-)


base-commit: 0e6639c8505d70e821bc27f951a0ff6303f10d4d
-- 
2.34.1

[PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Stefan Mätje]

In esd_usb_start() kfree() is called with the msg variable even if the
allocation of *msg failed.

Move the kfree() call to a line before the allocation error exit label
out: and adjust the exits for other errors to the new free_msg: label
just before kfree().

In esd_usb_probe() add free_dev: label and skip calling kfree() if
allocation of *msg failed.

Fixes: fae37f81fdf3 ( "net: can: esd_usb2: Do not do dma on the stack" )
Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 27a3818885c2..05ed664cf59d 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -3,7 +3,7 @@
  * CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
  *
  * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@esd.eu>
- * Copyright (C) 2022-2024 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
+ * Copyright (C) 2022-2025 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
  */
 
 #include <linux/can.h>
@@ -746,21 +746,22 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
 
 	err = esd_usb_send_msg(dev, msg);
 	if (err)
-		goto out;
+		goto free_msg;
 
 	err = esd_usb_setup_rx_urbs(dev);
 	if (err)
-		goto out;
+		goto free_msg;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
+free_msg:
+	kfree(msg);
 out:
 	if (err == -ENODEV)
 		netif_device_detach(netdev);
 	if (err)
 		netdev_err(netdev, "couldn't start device: %d\n", err);
 
-	kfree(msg);
 	return err;
 }
 
@@ -1279,7 +1280,7 @@ static int esd_usb_probe(struct usb_interface *intf,
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev) {
 		err = -ENOMEM;
-		goto done;
+		goto bail;
 	}
 
 	dev->udev = interface_to_usbdev(intf);
@@ -1291,7 +1292,7 @@ static int esd_usb_probe(struct usb_interface *intf,
 	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
 	if (!msg) {
 		err = -ENOMEM;
-		goto free_msg;
+		goto free_dev;
 	}
 
 	/* query number of CAN interfaces (nets) */
@@ -1334,9 +1335,10 @@ static int esd_usb_probe(struct usb_interface *intf,
 
 free_msg:
 	kfree(msg);
+free_dev:
 	if (err)
 		kfree(dev);
-done:
+bail:
 	return err;
 }
 
-- 
2.34.1

[PATCH 2/6] can: esd_usb: Fix not detecting version reply in probe routine

[Stefan Mätje]

This patch fixes some problems in the esd_usb_probe routine that render
the CAN interface unusable.

The probe routine sends a version request message to the USB device to
receive a version reply with the number of CAN ports and the hard-
& firmware versions. Then for each CAN port a CAN netdev is registered.

The previous code assumed that the version reply would be received
immediately. But if the driver was reloaded without power cycling the
USB device (i. e. on a reboot) there could already be other incoming
messages in the USB buffers. These would be in front of the version
reply and need to be skipped.

In the previous code these problems were present:
- Only one usb_bulk_msg() read was done into a buffer of
  sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
  which could lead to an overflow error from the USB stack.
- The first bytes of the received data were taken without checking for
  the message type. This could lead to zero detected CAN interfaces.

To mitigate these problems:
- Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
- Added a function esd_usb_recv_version() that reads and skips incoming
  "esd_usb_msg" messages until a version reply message is found. This
  is evaluated to return the count of CAN ports and version information.
- The data drain loop is limited by a maximum # of bytes to read from
  the device based on its internal buffer sizes and a timeout
  ESD_USB_DRAIN_TIMEOUT_MS.

This version of the patch incorporates changes recommended on the
linux-can list for a first version.

References:
https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r

Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 125 ++++++++++++++++++++++++++--------
 1 file changed, 97 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 05ed664cf59d..dbdfffe3a4a0 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB_CMD_TS			5 /* also used for TS_REPLY */
 #define ESD_USB_CMD_IDADD		6 /* also used for IDADD_REPLY */
 
+/* esd version message name size */
+#define ESD_USB_FW_NAME_SZ 16
+
 /* esd CAN message flags - dlc field */
 #define ESD_USB_RTR	BIT(4)
 #define ESD_USB_NO_BRS	BIT(4)
@@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB_RX_BUFFER_SIZE		1024
 #define ESD_USB_MAX_RX_URBS		4
 #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
+#define ESD_USB_DRAIN_TIMEOUT_MS	100
 
 /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
 #define ESD_USB_3_BAUDRATE_MODE_DISABLE		0 /* remove from bus */
@@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
 	u8 nets;
 	u8 features;
 	__le32 version;
-	u8 name[16];
+	u8 name[ESD_USB_FW_NAME_SZ];
 	__le32 rsvd;
 	__le32 ts;
 };
@@ -625,17 +629,91 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
 			    1000);
 }
 
-static int esd_usb_wait_msg(struct esd_usb *dev,
-			    union esd_usb_msg *msg)
+static int esd_usb_req_version(struct esd_usb *dev, void *buf)
 {
-	int actual_length;
+	union esd_usb_msg *msg = buf;
 
-	return usb_bulk_msg(dev->udev,
-			    usb_rcvbulkpipe(dev->udev, 1),
-			    msg,
-			    sizeof(*msg),
-			    &actual_length,
-			    1000);
+	msg->hdr.cmd = ESD_USB_CMD_VERSION;
+	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
+	msg->version.rsvd = 0;
+	msg->version.flags = 0;
+	msg->version.drv_version = 0;
+
+	return esd_usb_send_msg(dev, msg);
+}
+
+static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf)
+{
+	/* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
+	const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
+	unsigned long end_jiffies;
+	int cnt_other = 0;
+	int cnt_ts = 0;
+	int cnt_vs = 0;
+	int len_sum = 0;
+	int attempt = 0;
+	int err;
+
+	end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
+	do {
+		int actual_length;
+		int pos;
+
+		err = usb_bulk_msg(dev->udev,
+				   usb_rcvbulkpipe(dev->udev, 1),
+				   rx_buf,
+				   ESD_USB_RX_BUFFER_SIZE,
+				   &actual_length,
+				   ESD_USB_DRAIN_TIMEOUT_MS);
+		dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
+		if (err)
+			goto bail;
+
+		err = -ENOENT;
+		len_sum += actual_length;
+		pos = 0;
+		while (pos < actual_length) {
+			union esd_usb_msg *p_msg;
+
+			p_msg = (union esd_usb_msg *)(rx_buf + pos);
+
+			switch (p_msg->hdr.cmd) {
+			case ESD_USB_CMD_VERSION:
+				++cnt_vs;
+				dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
+				dev->version = le32_to_cpu(p_msg->version_reply.version);
+				err = 0;
+				dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
+					le32_to_cpu(p_msg->version_reply.ts),
+					le32_to_cpu(p_msg->version_reply.version),
+					p_msg->version_reply.nets,
+					p_msg->version_reply.features,
+					ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
+				break;
+			case ESD_USB_CMD_TS:
+				++cnt_ts;
+				dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
+					le32_to_cpu(p_msg->rx.ts));
+				break;
+			default:
+				++cnt_other;
+				dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
+				break;
+			}
+			pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
+
+			if (pos > actual_length) {
+				dev_err(&dev->udev->dev, "format error\n");
+				err = -EPROTO;
+				goto bail;
+			}
+		}
+		++attempt;
+	} while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
+bail:
+	dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
+		err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
+	return err;
 }
 
 static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
@@ -1274,7 +1352,7 @@ static int esd_usb_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
 	struct esd_usb *dev;
-	union esd_usb_msg *msg;
+	void *buf;
 	int i, err;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
@@ -1289,34 +1367,25 @@ static int esd_usb_probe(struct usb_interface *intf,
 
 	usb_set_intfdata(intf, dev);
 
-	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
-	if (!msg) {
+	buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
+	if (!buf) {
 		err = -ENOMEM;
 		goto free_dev;
 	}
 
 	/* query number of CAN interfaces (nets) */
-	msg->hdr.cmd = ESD_USB_CMD_VERSION;
-	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
-	msg->version.rsvd = 0;
-	msg->version.flags = 0;
-	msg->version.drv_version = 0;
-
-	err = esd_usb_send_msg(dev, msg);
+	err = esd_usb_req_version(dev, buf);
 	if (err < 0) {
 		dev_err(&intf->dev, "sending version message failed\n");
-		goto free_msg;
+		goto free_buf;
 	}
 
-	err = esd_usb_wait_msg(dev, msg);
+	err = esd_usb_recv_version(dev, buf);
 	if (err < 0) {
 		dev_err(&intf->dev, "no version message answer\n");
-		goto free_msg;
+		goto free_buf;
 	}
 
-	dev->net_count = (int)msg->version_reply.nets;
-	dev->version = le32_to_cpu(msg->version_reply.version);
-
 	if (device_create_file(&intf->dev, &dev_attr_firmware))
 		dev_err(&intf->dev,
 			"Couldn't create device file for firmware\n");
@@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
 	for (i = 0; i < dev->net_count; i++)
 		esd_usb_probe_one_net(intf, i);
 
-free_msg:
-	kfree(msg);
+free_buf:
+	kfree(buf);
 free_dev:
 	if (err)
 		kfree(dev);
-- 
2.34.1

[PATCH 3/6] can: esd_usb: Fix handling of TX context objects

[Stefan Mätje]

For each TX CAN frame submitted to the USB device the driver saves the
echo skb index in struct esd_tx_urb_context context objects. If the
driver runs out of free context objects CAN transmission stops.

This patch fixes some spots where such context objects are not freed
correctly.

In esd_usb_tx_done_msg() the check for netif_device_present() is moved
after the identification and release of TX context and the release of
the echo skb. This is allowed even if netif_device_present() would
return false because the mentioned operations don't touch the device
itself but only free local acquired resources. This keeps the context
handling with the acknowledged TX jobs in sync.

In esd_usb_start_xmit() a check is performed to see whether a context
object could be allocated. Added a netif_stop_queue() there before the
function is aborted. This makes sure the network queue is stopped and
avoids getting tons of log messages in a situation without free TX
objects. The adjacent log message now also prints the active jobs
counter making a cross check between active jobs and "no free context"
condition possible.

In esd_usb_start_xmit() the error handling of usb_submit_urb() missed to
free the context object together with the echo skb and decreasing the
job count.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index dbdfffe3a4a0..fc28fb52564c 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -460,9 +460,6 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
 	struct net_device *netdev = priv->netdev;
 	struct esd_tx_urb_context *context;
 
-	if (!netif_device_present(netdev))
-		return;
-
 	context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
 
 	if (!msg->txdone.status) {
@@ -478,6 +475,9 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
 	context->echo_index = ESD_USB_MAX_TX_URBS;
 	atomic_dec(&priv->active_tx_jobs);
 
+	if (!netif_device_present(netdev))
+		return;
+
 	netif_wake_queue(netdev);
 }
 
@@ -961,9 +961,11 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 		}
 	}
 
-	/* This may never happen */
+	/* This should never happen */
 	if (!context) {
-		netdev_warn(netdev, "couldn't find free context\n");
+		netdev_warn(netdev, "No free context. Jobs: %d\n",
+			    atomic_read(&priv->active_tx_jobs));
+		netif_stop_queue(netdev);
 		ret = NETDEV_TX_BUSY;
 		goto releasebuf;
 	}
@@ -994,6 +996,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	if (err) {
 		can_free_echo_skb(netdev, context->echo_index, NULL);
 
+		/* Release used context on error */
+		context->echo_index = ESD_USB_MAX_TX_URBS;
 		atomic_dec(&priv->active_tx_jobs);
 		usb_unanchor_urb(urb);
 
-- 
2.34.1

[PATCH 4/6] can: esd_usb: Add watermark handling for TX jobs

[Stefan Mätje]

The driver tried to keep as much CAN frames as possible submitted to the
USB device (ESD_USB_MAX_TX_URBS). This has led to occasional "No free
context" error messages in high load situations like with
"cangen -g 0 -p 10 canX".

This patch now calls netif_stop_queue() already if the number of active
jobs reaches ESD_USB_TX_URBS_HI_WM which is < ESD_USB_MAX_TX_URBS.
The netif_start_queue() is called in esd_usb_tx_done_msg() only if
the number of active jobs is <= ESD_USB_TX_URBS_LO_WM.

This change eliminates the occasional error messages and significantly
reduces the number of calls to netif_start_queue() and
netif_stop_queue().

The watermark limits have been chosen with the CAN-USB/Micro in mind to
not to compromise its TX throughput. This device is running on USB 1.1
only with its 1ms USB polling cycle where a ESD_USB_TX_URBS_LO_WM
value below 9 decreases the TX throughput.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index fc28fb52564c..41ff453f87b8 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -98,6 +98,8 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB_RX_BUFFER_SIZE		1024
 #define ESD_USB_MAX_RX_URBS		4
 #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
+#define ESD_USB_TX_URBS_HI_WM		((15 * ESD_USB_MAX_TX_URBS) / 16)
+#define ESD_USB_TX_URBS_LO_WM		((9 * ESD_USB_MAX_TX_URBS) / 16)
 #define ESD_USB_DRAIN_TIMEOUT_MS	100
 
 /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
@@ -478,7 +480,8 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
 	if (!netif_device_present(netdev))
 		return;
 
-	netif_wake_queue(netdev);
+	if (atomic_read(&priv->active_tx_jobs) <= ESD_USB_TX_URBS_LO_WM)
+		netif_wake_queue(netdev);
 }
 
 static void esd_usb_read_bulk_callback(struct urb *urb)
@@ -973,7 +976,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	context->priv = priv;
 	context->echo_index = i;
 
-	/* hnd must not be 0 - MSB is stripped in txdone handling */
+	/* hnd must not be 0 - MSB is stripped in TX done handling */
 	msg->tx.hnd = BIT(31) | i; /* returned in TX done message */
 
 	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
@@ -988,8 +991,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 
 	atomic_inc(&priv->active_tx_jobs);
 
-	/* Slow down tx path */
-	if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_MAX_TX_URBS)
+	/* Slow down TX path */
+	if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_TX_URBS_HI_WM)
 		netif_stop_queue(netdev);
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
-- 
2.34.1

[PATCH 5/6] can: esd_usb: Rework display of error messages

[Stefan Mätje]

- esd_usb_open(): Get rid of duplicate "couldn't start device: %d\n"
  message already printed from esd_usb_start().
- Added 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.
- 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.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 41ff453f87b8..3c348af566ec 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -882,7 +882,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;
 	}
@@ -1037,6 +1036,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);
@@ -1050,8 +1050,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: %d\n", err);
 
 	/* set CAN controller to reset mode */
 	msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */
@@ -1059,8 +1060,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: %d\n", err);
 
 	priv->can.state = CAN_STATE_STOPPED;
 
@@ -1344,7 +1346,7 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
 	}
 
 	dev->nets[index] = priv;
-	netdev_info(netdev, "device %s registered\n", netdev->name);
+	netdev_info(netdev, "registered\n");
 
 done:
 	return err;
@@ -1383,13 +1385,13 @@ static int esd_usb_probe(struct usb_interface *intf,
 	/* query number of CAN interfaces (nets) */
 	err = esd_usb_req_version(dev, buf);
 	if (err < 0) {
-		dev_err(&intf->dev, "sending version message failed\n");
+		dev_err(&intf->dev, "sending version message failed: %d\n", err);
 		goto free_buf;
 	}
 
 	err = esd_usb_recv_version(dev, buf);
 	if (err < 0) {
-		dev_err(&intf->dev, "no version message answer\n");
+		dev_err(&intf->dev, "no version message answer: %d\n", err);
 		goto free_buf;
 	}
 
@@ -1435,6 +1437,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);
 			}
-- 
2.34.1

[PATCH 6/6] can: esd_usb: Avoid errors triggered from USB disconnect

[Stefan Mätje]

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.

This patch moves 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.

It then changes esd_usb_close() not to send the control messages at
all if the ndo_stop() callback is executed from the USB .disconnect()
callback. A new flag in_usb_disconnect is added 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 <stefan.maetje@esd.eu>
---
 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 3c348af566ec..70c0e7b96b8c 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -280,6 +280,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];
 };
@@ -1032,9 +1033,9 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	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;
@@ -1051,8 +1052,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: %d\n", err);
+	if (err < 0) {
+		netdev_err(priv->netdev, "sending idadd message failed: %d\n", err);
+		goto bail;
+	}
 
 	/* set CAN controller to reset mode */
 	msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */
@@ -1062,7 +1065,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: %d\n", err);
+		netdev_err(priv->netdev, "sending setbaud message failed: %d\n", 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;
 
@@ -1070,9 +1089,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 = {
@@ -1434,6 +1451,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;
-- 
2.34.1

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Vadim Fedorenko]

On 11/08/2025 22:06, Stefan Mätje wrote:
> In esd_usb_start() kfree() is called with the msg variable even if the
> allocation of *msg failed.

But kfree() works fine with NULL pointers, have you seen any real issues
with this code?

Re: [PATCH 6/6] can: esd_usb: Avoid errors triggered from USB disconnect

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:11, Stefan Mätje wrote:
> 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.
> 
> This patch moves 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.
> 
> It then changes esd_usb_close() not to send the control messages at
> all if the ndo_stop() callback is executed from the USB .disconnect()
> callback. A new flag in_usb_disconnect is added 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().

I cannot find the reference anymore, but I remember that Greg said, that
USB devices should just be quiet when being unplugged. I removed the
error prints from the gs_usb's close function, see: 5c6c313acdfc ("can:
gs_usb: gs_can_close(): don't complain about failed device reset during
ndo_stop")

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 5/6] can: esd_usb: Rework display of error messages

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:10, Stefan Mätje wrote:
> - esd_usb_open(): Get rid of duplicate "couldn't start device: %d\n"
>   message already printed from esd_usb_start().
> - Added 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.
> - 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.

If you want to print errors, please use '"%pE", ERR_PTR(err)', that will
decode the error code into human readable form.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 4/6] can: esd_usb: Add watermark handling for TX jobs

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:09, Stefan Mätje wrote:
> The driver tried to keep as much CAN frames as possible submitted to the
> USB device (ESD_USB_MAX_TX_URBS). This has led to occasional "No free
> context" error messages in high load situations like with
> "cangen -g 0 -p 10 canX".
> 
> This patch now calls netif_stop_queue() already if the number of active
> jobs reaches ESD_USB_TX_URBS_HI_WM which is < ESD_USB_MAX_TX_URBS.
> The netif_start_queue() is called in esd_usb_tx_done_msg() only if
> the number of active jobs is <= ESD_USB_TX_URBS_LO_WM.
> 
> This change eliminates the occasional error messages and significantly
> reduces the number of calls to netif_start_queue() and
> netif_stop_queue().
> 
> The watermark limits have been chosen with the CAN-USB/Micro in mind to
> not to compromise its TX throughput. This device is running on USB 1.1
> only with its 1ms USB polling cycle where a ESD_USB_TX_URBS_LO_WM
> value below 9 decreases the TX throughput.
> 
> Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>

Please add a Fixes tag.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 3/6] can: esd_usb: Fix handling of TX context objects

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:08, Stefan Mätje wrote:
> For each TX CAN frame submitted to the USB device the driver saves the
> echo skb index in struct esd_tx_urb_context context objects. If the
> driver runs out of free context objects CAN transmission stops.
> 
> This patch fixes some spots where such context objects are not freed
> correctly.
> 
> In esd_usb_tx_done_msg() the check for netif_device_present() is moved
> after the identification and release of TX context and the release of
> the echo skb. This is allowed even if netif_device_present() would
> return false because the mentioned operations don't touch the device
> itself but only free local acquired resources. This keeps the context
> handling with the acknowledged TX jobs in sync.
> 
> In esd_usb_start_xmit() a check is performed to see whether a context
> object could be allocated. Added a netif_stop_queue() there before the
> function is aborted. This makes sure the network queue is stopped and
> avoids getting tons of log messages in a situation without free TX
> objects. The adjacent log message now also prints the active jobs
> counter making a cross check between active jobs and "no free context"
> condition possible.
> 
> In esd_usb_start_xmit() the error handling of usb_submit_urb() missed to
> free the context object together with the echo skb and decreasing the
> job count.
> 
> Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>

Please add a Fixes tag.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in probe routine

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:07, Stefan Mätje wrote:
> This patch fixes some problems in the esd_usb_probe routine that render
> the CAN interface unusable.
> 
> The probe routine sends a version request message to the USB device to
> receive a version reply with the number of CAN ports and the hard-
> & firmware versions. Then for each CAN port a CAN netdev is registered.
> 
> The previous code assumed that the version reply would be received
> immediately. But if the driver was reloaded without power cycling the
> USB device (i. e. on a reboot) there could already be other incoming
> messages in the USB buffers. These would be in front of the version
> reply and need to be skipped.
> 
> In the previous code these problems were present:
> - Only one usb_bulk_msg() read was done into a buffer of
>   sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
>   which could lead to an overflow error from the USB stack.
> - The first bytes of the received data were taken without checking for
>   the message type. This could lead to zero detected CAN interfaces.
> 
> To mitigate these problems:
> - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> - Added a function esd_usb_recv_version() that reads and skips incoming
>   "esd_usb_msg" messages until a version reply message is found. This
>   is evaluated to return the count of CAN ports and version information.
> - The data drain loop is limited by a maximum # of bytes to read from
>   the device based on its internal buffer sizes and a timeout
>   ESD_USB_DRAIN_TIMEOUT_MS.
> 
> This version of the patch incorporates changes recommended on the
> linux-can list for a first version.
> 
> References:
> https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r
> 
> Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
> Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 125 ++++++++++++++++++++++++++--------
>  1 file changed, 97 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 05ed664cf59d..dbdfffe3a4a0 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB_CMD_TS			5 /* also used for TS_REPLY */
>  #define ESD_USB_CMD_IDADD		6 /* also used for IDADD_REPLY */
>  
> +/* esd version message name size */
> +#define ESD_USB_FW_NAME_SZ 16
> +
>  /* esd CAN message flags - dlc field */
>  #define ESD_USB_RTR	BIT(4)
>  #define ESD_USB_NO_BRS	BIT(4)
> @@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB_RX_BUFFER_SIZE		1024
>  #define ESD_USB_MAX_RX_URBS		4
>  #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
> +#define ESD_USB_DRAIN_TIMEOUT_MS	100
>  
>  /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
>  #define ESD_USB_3_BAUDRATE_MODE_DISABLE		0 /* remove from bus */
> @@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
>  	u8 nets;
>  	u8 features;
>  	__le32 version;
> -	u8 name[16];
> +	u8 name[ESD_USB_FW_NAME_SZ];
>  	__le32 rsvd;
>  	__le32 ts;
>  };
> @@ -625,17 +629,91 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
>  			    1000);
>  }
>  
> -static int esd_usb_wait_msg(struct esd_usb *dev,
> -			    union esd_usb_msg *msg)
> +static int esd_usb_req_version(struct esd_usb *dev, void *buf)
>  {
> -	int actual_length;
> +	union esd_usb_msg *msg = buf;
>  
> -	return usb_bulk_msg(dev->udev,
> -			    usb_rcvbulkpipe(dev->udev, 1),
> -			    msg,
> -			    sizeof(*msg),
> -			    &actual_length,
> -			    1000);
> +	msg->hdr.cmd = ESD_USB_CMD_VERSION;
> +	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> +	msg->version.rsvd = 0;
> +	msg->version.flags = 0;
> +	msg->version.drv_version = 0;
> +
> +	return esd_usb_send_msg(dev, msg);
> +}
> +
> +static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf)
> +{
> +	/* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
> +	const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
> +	unsigned long end_jiffies;
> +	int cnt_other = 0;
> +	int cnt_ts = 0;
> +	int cnt_vs = 0;
> +	int len_sum = 0;
> +	int attempt = 0;
> +	int err;
> +
> +	end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
> +	do {
> +		int actual_length;
> +		int pos;
> +
> +		err = usb_bulk_msg(dev->udev,
> +				   usb_rcvbulkpipe(dev->udev, 1),
> +				   rx_buf,
> +				   ESD_USB_RX_BUFFER_SIZE,
> +				   &actual_length,
> +				   ESD_USB_DRAIN_TIMEOUT_MS);
> +		dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> +		if (err)
> +			goto bail;
> +
> +		err = -ENOENT;
> +		len_sum += actual_length;
> +		pos = 0;
> +		while (pos < actual_length) {

You have to check that the actual offset you will access later is within
actual_length.

> +			union esd_usb_msg *p_msg;
> +
> +			p_msg = (union esd_usb_msg *)(rx_buf + pos);
> +
> +			switch (p_msg->hdr.cmd) {
> +			case ESD_USB_CMD_VERSION:
> +				++cnt_vs;
> +				dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
> +				dev->version = le32_to_cpu(p_msg->version_reply.version);
> +				err = 0;
> +				dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
> +					le32_to_cpu(p_msg->version_reply.ts),
> +					le32_to_cpu(p_msg->version_reply.version),
> +					p_msg->version_reply.nets,
> +					p_msg->version_reply.features,
> +					ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
> +				break;
> +			case ESD_USB_CMD_TS:
> +				++cnt_ts;
> +				dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
> +					le32_to_cpu(p_msg->rx.ts));
> +				break;
> +			default:
> +				++cnt_other;
> +				dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
> +				break;
> +			}
> +			pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */

Can pos overflow? hdr.len is a u8, so probably not.

> +			if (pos > actual_length) {
> +				dev_err(&dev->udev->dev, "format error\n");
> +				err = -EPROTO;
> +				goto bail;
> +			}
> +		}
> +		++attempt;
> +	} while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
> +bail:
> +	dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
> +		err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
> +	return err;
>  }
>  
>  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> @@ -1274,7 +1352,7 @@ static int esd_usb_probe(struct usb_interface *intf,
>  			 const struct usb_device_id *id)
>  {
>  	struct esd_usb *dev;
> -	union esd_usb_msg *msg;
> +	void *buf;
>  	int i, err;
>  
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> @@ -1289,34 +1367,25 @@ static int esd_usb_probe(struct usb_interface *intf,
>  
>  	usb_set_intfdata(intf, dev);
>  
> -	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> -	if (!msg) {
> +	buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
> +	if (!buf) {
>  		err = -ENOMEM;
>  		goto free_dev;
>  	}
>  
>  	/* query number of CAN interfaces (nets) */
> -	msg->hdr.cmd = ESD_USB_CMD_VERSION;
> -	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> -	msg->version.rsvd = 0;
> -	msg->version.flags = 0;
> -	msg->version.drv_version = 0;
> -
> -	err = esd_usb_send_msg(dev, msg);
> +	err = esd_usb_req_version(dev, buf);

I'm a bit uneasy with the passing of the buffer as an argument, but not
its size.

>  	if (err < 0) {
>  		dev_err(&intf->dev, "sending version message failed\n");
> -		goto free_msg;
> +		goto free_buf;
>  	}
>  
> -	err = esd_usb_wait_msg(dev, msg);
> +	err = esd_usb_recv_version(dev, buf);
>  	if (err < 0) {
>  		dev_err(&intf->dev, "no version message answer\n");
> -		goto free_msg;
> +		goto free_buf;
>  	}
>  
> -	dev->net_count = (int)msg->version_reply.nets;
> -	dev->version = le32_to_cpu(msg->version_reply.version);
> -
>  	if (device_create_file(&intf->dev, &dev_attr_firmware))
>  		dev_err(&intf->dev,
>  			"Couldn't create device file for firmware\n");
> @@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
>  	for (i = 0; i < dev->net_count; i++)
>  		esd_usb_probe_one_net(intf, i);
>  
> -free_msg:
> -	kfree(msg);
> +free_buf:
> +	kfree(buf);
>  free_dev:
>  	if (err)
>  		kfree(dev);
> -- 
> 2.34.1
> 
> 
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Marc Kleine-Budde]

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

On 11.08.2025 23:06:06, Stefan Mätje wrote:
> In esd_usb_start() kfree() is called with the msg variable even if the
> allocation of *msg failed.
> 
> Move the kfree() call to a line before the allocation error exit label
> out: and adjust the exits for other errors to the new free_msg: label
> just before kfree().
> 
> In esd_usb_probe() add free_dev: label and skip calling kfree() if
> allocation of *msg failed.
> 
> Fixes: fae37f81fdf3 ( "net: can: esd_usb2: Do not do dma on the stack" )
> Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 27a3818885c2..05ed664cf59d 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -3,7 +3,7 @@
>   * CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
>   *
>   * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@esd.eu>
> - * Copyright (C) 2022-2024 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> + * Copyright (C) 2022-2025 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
>   */
>  
>  #include <linux/can.h>
> @@ -746,21 +746,22 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)

	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
	if (!msg) {
		err = -ENOMEM;
		goto out;
	}

Can you adjust the jump label for the kmalloc() fail. There's no need to
check for -ENODEV

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in probe routine

[Vincent Mailhol]

Hi Stefan,

On 8/13/25 5:26 PM, Marc Kleine-Budde wrote:
> On 11.08.2025 23:06:07, Stefan Mätje wrote:
>> This patch fixes some problems in the esd_usb_probe routine that render
>> the CAN interface unusable.
>>
>> The probe routine sends a version request message to the USB device to
>> receive a version reply with the number of CAN ports and the hard-
>> & firmware versions. Then for each CAN port a CAN netdev is registered.
>>
>> The previous code assumed that the version reply would be received
>> immediately. But if the driver was reloaded without power cycling the
>> USB device (i. e. on a reboot) there could already be other incoming
>> messages in the USB buffers. These would be in front of the version
>> reply and need to be skipped.

Wouldn't it be easier to:

  - First empty the incoming USB message queue
  - Then request the version
  - Finally parse the version reply

?

This way, you wouldn't have to do the complex parsing anymore in
esd_usb_recv_version().

>> In the previous code these problems were present:
>> - Only one usb_bulk_msg() read was done into a buffer of
>>   sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
>>   which could lead to an overflow error from the USB stack.
>> - The first bytes of the received data were taken without checking for
>>   the message type. This could lead to zero detected CAN interfaces.
>>
>> To mitigate these problems:
>> - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
>> - Added a function esd_usb_recv_version() that reads and skips incoming
>>   "esd_usb_msg" messages until a version reply message is found. This
>>   is evaluated to return the count of CAN ports and version information.
>> - The data drain loop is limited by a maximum # of bytes to read from
>>   the device based on its internal buffer sizes and a timeout
>>   ESD_USB_DRAIN_TIMEOUT_MS.
>>
>> This version of the patch incorporates changes recommended on the
>> linux-can list for a first version.
>>
>> References:
>> https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r
>>
>> Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
>> Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
>> ---
>>  drivers/net/can/usb/esd_usb.c | 125 ++++++++++++++++++++++++++--------
>>  1 file changed, 97 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
>> index 05ed664cf59d..dbdfffe3a4a0 100644
>> --- a/drivers/net/can/usb/esd_usb.c
>> +++ b/drivers/net/can/usb/esd_usb.c
>> @@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
>>  #define ESD_USB_CMD_TS			5 /* also used for TS_REPLY */
>>  #define ESD_USB_CMD_IDADD		6 /* also used for IDADD_REPLY */
>>  
>> +/* esd version message name size */
>> +#define ESD_USB_FW_NAME_SZ 16
>> +
>>  /* esd CAN message flags - dlc field */
>>  #define ESD_USB_RTR	BIT(4)
>>  #define ESD_USB_NO_BRS	BIT(4)
>> @@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
>>  #define ESD_USB_RX_BUFFER_SIZE		1024
>>  #define ESD_USB_MAX_RX_URBS		4
>>  #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
>> +#define ESD_USB_DRAIN_TIMEOUT_MS	100
>>  
>>  /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
>>  #define ESD_USB_3_BAUDRATE_MODE_DISABLE		0 /* remove from bus */
>> @@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
>>  	u8 nets;
>>  	u8 features;
>>  	__le32 version;
>> -	u8 name[16];
>> +	u8 name[ESD_USB_FW_NAME_SZ];
>>  	__le32 rsvd;
>>  	__le32 ts;
>>  };
>> @@ -625,17 +629,91 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
>>  			    1000);
>>  }
>>  
>> -static int esd_usb_wait_msg(struct esd_usb *dev,
>> -			    union esd_usb_msg *msg)
>> +static int esd_usb_req_version(struct esd_usb *dev, void *buf)
>>  {
>> -	int actual_length;
>> +	union esd_usb_msg *msg = buf;
>>  
>> -	return usb_bulk_msg(dev->udev,
>> -			    usb_rcvbulkpipe(dev->udev, 1),
>> -			    msg,
>> -			    sizeof(*msg),
>> -			    &actual_length,
>> -			    1000);
>> +	msg->hdr.cmd = ESD_USB_CMD_VERSION;
>> +	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
>> +	msg->version.rsvd = 0;
>> +	msg->version.flags = 0;
>> +	msg->version.drv_version = 0;
>> +
>> +	return esd_usb_send_msg(dev, msg);
>> +}
>> +
>> +static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf)
>> +{
>> +	/* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
>> +	const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
>> +	unsigned long end_jiffies;
>> +	int cnt_other = 0;
>> +	int cnt_ts = 0;
>> +	int cnt_vs = 0;
>> +	int len_sum = 0;
>> +	int attempt = 0;
>> +	int err;
>> +
>> +	end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
>> +	do {
>> +		int actual_length;
>> +		int pos;
>> +
>> +		err = usb_bulk_msg(dev->udev,
>> +				   usb_rcvbulkpipe(dev->udev, 1),
>> +				   rx_buf,
>> +				   ESD_USB_RX_BUFFER_SIZE,
>> +				   &actual_length,
>> +				   ESD_USB_DRAIN_TIMEOUT_MS);
>> +		dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
>> +		if (err)
>> +			goto bail;
>> +
>> +		err = -ENOENT;
>> +		len_sum += actual_length;
>> +		pos = 0;
>> +		while (pos < actual_length) {
> 
> You have to check that the actual offset you will access later is within
> actual_length.
> 
>> +			union esd_usb_msg *p_msg;
>> +
>> +			p_msg = (union esd_usb_msg *)(rx_buf + pos);
>> +
>> +			switch (p_msg->hdr.cmd) {
>> +			case ESD_USB_CMD_VERSION:
>> +				++cnt_vs;
>> +				dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
>> +				dev->version = le32_to_cpu(p_msg->version_reply.version);
>> +				err = 0;
>> +				dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
>> +					le32_to_cpu(p_msg->version_reply.ts),
>> +					le32_to_cpu(p_msg->version_reply.version),
>> +					p_msg->version_reply.nets,
>> +					p_msg->version_reply.features,
>> +					ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
>> +				break;
>> +			case ESD_USB_CMD_TS:
>> +				++cnt_ts;
>> +				dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
>> +					le32_to_cpu(p_msg->rx.ts));
>> +				break;
>> +			default:
>> +				++cnt_other;
>> +				dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
>> +				break;
>> +			}
>> +			pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
> 
> Can pos overflow? hdr.len is a u8, so probably not.
> 
>> +			if (pos > actual_length) {
>> +				dev_err(&dev->udev->dev, "format error\n");
>> +				err = -EPROTO;
>> +				goto bail;
>> +			}
>> +		}
>> +		++attempt;
>> +	} while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
>> +bail:
>> +	dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
>> +		err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
>> +	return err;
>>  }
>>  
>>  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
>> @@ -1274,7 +1352,7 @@ static int esd_usb_probe(struct usb_interface *intf,
>>  			 const struct usb_device_id *id)
>>  {
>>  	struct esd_usb *dev;
>> -	union esd_usb_msg *msg;
>> +	void *buf;
>>  	int i, err;
>>  
>>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> @@ -1289,34 +1367,25 @@ static int esd_usb_probe(struct usb_interface *intf,
>>  
>>  	usb_set_intfdata(intf, dev);
>>  
>> -	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
>> -	if (!msg) {
>> +	buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
>> +	if (!buf) {
>>  		err = -ENOMEM;
>>  		goto free_dev;
>>  	}
>>  
>>  	/* query number of CAN interfaces (nets) */
>> -	msg->hdr.cmd = ESD_USB_CMD_VERSION;
>> -	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
>> -	msg->version.rsvd = 0;
>> -	msg->version.flags = 0;
>> -	msg->version.drv_version = 0;
>> -
>> -	err = esd_usb_send_msg(dev, msg);
>> +	err = esd_usb_req_version(dev, buf);
> 
> I'm a bit uneasy with the passing of the buffer as an argument, but not
> its size.

+1

What I also do not like is that buffer of type void *. If I understand
correctly, you are using this buffer for both the request and the reply, thus
the void* type. But is this really a winning trade?

Here we are in the probe function, not something speed critical. So, I would
rather have the esd_usb_req_version() and the esd_usb_recv_version() allocate
their own buffer with the correct size.

Yes, you would be doing two kalloc() instead of one, but you will gain in
readability and the esd_usb_probe() also becomes simpler.

>>  	if (err < 0) {
>>  		dev_err(&intf->dev, "sending version message failed\n");
>> -		goto free_msg;
>> +		goto free_buf;
>>  	}
>>  
>> -	err = esd_usb_wait_msg(dev, msg);
>> +	err = esd_usb_recv_version(dev, buf);
>>  	if (err < 0) {
>>  		dev_err(&intf->dev, "no version message answer\n");
>> -		goto free_msg;
>> +		goto free_buf;
>>  	}
>>  
>> -	dev->net_count = (int)msg->version_reply.nets;
>> -	dev->version = le32_to_cpu(msg->version_reply.version);
>> -
>>  	if (device_create_file(&intf->dev, &dev_attr_firmware))
>>  		dev_err(&intf->dev,
>>  			"Couldn't create device file for firmware\n");
>> @@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
>>  	for (i = 0; i < dev->net_count; i++)
>>  		esd_usb_probe_one_net(intf, i);
>>  
>> -free_msg:
>> -	kfree(msg);
>> +free_buf:
>> +	kfree(buf);
>>  free_dev:
>>  	if (err)
>>  		kfree(dev);
-- 
Yours sincerely,
Vincent Mailhol

Re: [PATCH 5/6] can: esd_usb: Rework display of error messages

[Marc Kleine-Budde]

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

On 13.08.2025 10:12:03, Marc Kleine-Budde wrote:
> On 11.08.2025 23:06:10, Stefan Mätje wrote:
> > - esd_usb_open(): Get rid of duplicate "couldn't start device: %d\n"
> >   message already printed from esd_usb_start().
> > - Added 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.
> > - 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.
> 
> If you want to print errors, please use '"%pE", ERR_PTR(err)', that will
> decode the error code into human readable form.

Sorry, I meant to say "%pe".

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Stefan Mätje]

Am Dienstag, dem 12.08.2025 um 13:33 +0100 schrieb Vadim Fedorenko:
> On 11/08/2025 22:06, Stefan Mätje wrote:
> > In esd_usb_start() kfree() is called with the msg variable even if the
> > allocation of *msg failed.
> 
> But kfree() works fine with NULL pointers, have you seen any real issues
> with this code?

Hello Vadim,

I've not seen real problems with this code. And when I posted the patch I
knew that kfree() can cope with NULL pointers. But in any case calling a
*free() function with a NULL pointer sends shivers over my spine and I
want to avoid to stumble over this again and again.

Best regards,
   Stefan

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 11:20 +0200 schrieb Marc Kleine-Budde:
> On 11.08.2025 23:06:06, Stefan Mätje wrote:
> > In esd_usb_start() kfree() is called with the msg variable even if the
> > allocation of *msg failed.
> > 
> > Move the kfree() call to a line before the allocation error exit label
> > out: and adjust the exits for other errors to the new free_msg: label
> > just before kfree().
> > 
> > In esd_usb_probe() add free_dev: label and skip calling kfree() if
> > allocation of *msg failed.
> > 
> > Fixes: fae37f81fdf3 ( "net: can: esd_usb2: Do not do dma on the stack" )
> > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 27a3818885c2..05ed664cf59d 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -3,7 +3,7 @@
> >   * CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
> >   *
> >   * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@esd.eu>
> > - * Copyright (C) 2022-2024 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> > + * Copyright (C) 2022-2025 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> >   */
> >  
> >  #include <linux/can.h>
> > @@ -746,21 +746,22 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
> 
> 	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> 	if (!msg) {
> 		err = -ENOMEM;
> 		goto out;
> 	}
> 
> Can you adjust the jump label for the kmalloc() fail. There's no need to
> check for -ENODEV

Yes, I will move it in the next iteration. Thanks for that hint.

> Marc
> 

Re: [PATCH 3/6] can: esd_usb: Fix handling of TX context objects

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 10:14 +0200 schrieb Marc Kleine-Budde:
> On 11.08.2025 23:06:08, Stefan Mätje wrote:
> > For each TX CAN frame submitted to the USB device the driver saves the
> > echo skb index in struct esd_tx_urb_context context objects. If the
> > driver runs out of free context objects CAN transmission stops.
> > 
> > This patch fixes some spots where such context objects are not freed
> > correctly.
> > 
> > In esd_usb_tx_done_msg() the check for netif_device_present() is moved
> > after the identification and release of TX context and the release of
> > the echo skb. This is allowed even if netif_device_present() would
> > return false because the mentioned operations don't touch the device
> > itself but only free local acquired resources. This keeps the context
> > handling with the acknowledged TX jobs in sync.
> > 
> > In esd_usb_start_xmit() a check is performed to see whether a context
> > object could be allocated. Added a netif_stop_queue() there before the
> > function is aborted. This makes sure the network queue is stopped and
> > avoids getting tons of log messages in a situation without free TX
> > objects. The adjacent log message now also prints the active jobs
> > counter making a cross check between active jobs and "no free context"
> > condition possible.
> > 
> > In esd_usb_start_xmit() the error handling of usb_submit_urb() missed to
> > free the context object together with the echo skb and decreasing the
> > job count.
> > 
> > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> 
> Please add a Fixes tag.
> 
> Marc

I've looked it up. This code was already this way in the initial release.
I'll add then:

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")

Re: [PATCH 6/6] can: esd_usb: Avoid errors triggered from USB disconnect

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 10:09 +0200 schrieb Marc Kleine-Budde:
> On 11.08.2025 23:06:11, Stefan Mätje wrote:
> > 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.
> > 
> > This patch moves 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.
> > 
> > It then changes esd_usb_close() not to send the control messages at
> > all if the ndo_stop() callback is executed from the USB .disconnect()
> > callback. A new flag in_usb_disconnect is added 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().
> 
> I cannot find the reference anymore, but I remember that Greg said, that
> USB devices should just be quiet when being unplugged. I removed the
> error prints from the gs_usb's close function, see: 5c6c313acdfc ("can:
> gs_usb: gs_can_close(): don't complain about failed device reset during
> ndo_stop")

Hello Marc,

I think this patch fulfills the requirement not to print (bogus) error
messages during device disconnnect. This is the purpose of the patch.

It does this by not trying to communicate with the device during USB
disconnect at all. Therefore no error messages can be triggered.

On the other hand I would like to bring the device in an idle state during
"ip link set down" going through ndo_stop(). In this case the USB
communication should still be ok and any errors should be printed in my
opinion.

What do you think?

Stefan

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[David Laight]

On Fri, 15 Aug 2025 14:57:44 +0000
Stefan Mätje <stefan.maetje@esd.eu> wrote:

> Am Dienstag, dem 12.08.2025 um 13:33 +0100 schrieb Vadim Fedorenko:
> > On 11/08/2025 22:06, Stefan Mätje wrote:  
> > > In esd_usb_start() kfree() is called with the msg variable even if the
> > > allocation of *msg failed.  
> > 
> > But kfree() works fine with NULL pointers, have you seen any real issues
> > with this code?  
> 
> Hello Vadim,
> 
> I've not seen real problems with this code. And when I posted the patch I
> knew that kfree() can cope with NULL pointers. But in any case calling a
> *free() function with a NULL pointer sends shivers over my spine and I
> want to avoid to stumble over this again and again.

The only time it is worth the check in the caller is the case where it
is normal for the pointer to be NULL.

But for an error exit it is safer to have one exit path that tidies everything
up rather than lots of them where there is the opportunity to exit via the
wrong one.

	David

Re: [PATCH 4/6] can: esd_usb: Add watermark handling for TX jobs

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 10:13 +0200 schrieb Marc Kleine-Budde:
> On 11.08.2025 23:06:09, Stefan Mätje wrote:
> > The driver tried to keep as much CAN frames as possible submitted to the
> > USB device (ESD_USB_MAX_TX_URBS). This has led to occasional "No free
> > context" error messages in high load situations like with
> > "cangen -g 0 -p 10 canX".
> > 
> > This patch now calls netif_stop_queue() already if the number of active
> > jobs reaches ESD_USB_TX_URBS_HI_WM which is < ESD_USB_MAX_TX_URBS.
> > The netif_start_queue() is called in esd_usb_tx_done_msg() only if
> > the number of active jobs is <= ESD_USB_TX_URBS_LO_WM.
> > 
> > This change eliminates the occasional error messages and significantly
> > reduces the number of calls to netif_start_queue() and
> > netif_stop_queue().
> > 
> > The watermark limits have been chosen with the CAN-USB/Micro in mind to
> > not to compromise its TX throughput. This device is running on USB 1.1
> > only with its 1ms USB polling cycle where a ESD_USB_TX_URBS_LO_WM
> > value below 9 decreases the TX throughput.
> > 
> > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> 
> Please add a Fixes tag.

I did not add a fixes tag because I had the impression that the code was working
fine with older kernels and the occasional messages began showing up only with
newer kernels after ~ 6.11. So I was not able to pinpoint the error to the patch 
that exposed the problem. 

But I retested the stuff now with older kernels. Somewhere between 5.4 and the 5.15
the problem starts. But on older kernels it can only be exposed transmitting zero
byte frames with "cangen -I i -g 0 -p 10 -L 0 canX" and additionally load the system
with other heavy tasks.

The code was unchanged since the initial implementation. I will then add a fixes 
tag that denotes the initial implementation. That would be:

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")

Re: [PATCH 5/6] can: esd_usb: Rework display of error messages

[Stefan Mätje]

Am Donnerstag, dem 14.08.2025 um 14:45 +0200 schrieb Marc Kleine-Budde:
> On 13.08.2025 10:12:03, Marc Kleine-Budde wrote:
> > On 11.08.2025 23:06:10, Stefan Mätje wrote:
> > > - esd_usb_open(): Get rid of duplicate "couldn't start device: %d\n"
> > >   message already printed from esd_usb_start().
> > > - Added 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.
> > > - 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.
> > 
> > If you want to print errors, please use '"%pE", ERR_PTR(err)', that will
> > decode the error code into human readable form.
> 
> Sorry, I meant to say "%pe".

Thanks for that hint. I will rework all occurrences of '"...: %d\n", err'
with a version based on "ERR_PTR(err)" and send a V2.

Stefan

Re: [PATCH 1/6] can: esd_usb: Fix possible calls to kfree() with NULL

[Stefan Mätje]

Am Freitag, dem 15.08.2025 um 15:02 +0000 schrieb Stefan Mätje:
> Am Mittwoch, dem 13.08.2025 um 11:20 +0200 schrieb Marc Kleine-Budde:
> > On 11.08.2025 23:06:06, Stefan Mätje wrote:
> > > In esd_usb_start() kfree() is called with the msg variable even if the
> > > allocation of *msg failed.
> > > 
> > > Move the kfree() call to a line before the allocation error exit label
> > > out: and adjust the exits for other errors to the new free_msg: label
> > > just before kfree().
> > > 
> > > In esd_usb_probe() add free_dev: label and skip calling kfree() if
> > > allocation of *msg failed.
> > > 
> > > Fixes: fae37f81fdf3 ( "net: can: esd_usb2: Do not do dma on the stack" )
> > > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> > > ---
> > >  drivers/net/can/usb/esd_usb.c | 16 +++++++++-------
> > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > index 27a3818885c2..05ed664cf59d 100644
> > > --- a/drivers/net/can/usb/esd_usb.c
> > > +++ b/drivers/net/can/usb/esd_usb.c
> > > @@ -3,7 +3,7 @@
> > >   * CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
> > >   *
> > >   * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@esd.eu>
> > > - * Copyright (C) 2022-2024 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> > > + * Copyright (C) 2022-2025 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> > >   */
> > >  
> > >  #include <linux/can.h>
> > > @@ -746,21 +746,22 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
> > 
> > 	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> > 	if (!msg) {
> > 		err = -ENOMEM;
> > 		goto out;
> > 	}
> > 
> > Can you adjust the jump label for the kmalloc() fail. There's no need to
> > check for -ENODEV
> 
> Yes, I will move it in the next iteration. Thanks for that hint.
> 
Hi Marc,

since there have been some rather rejecting emails, I will withdraw patch 1. 
Therefore, the function esd_usb_start() will remain unchanged in the next
iteration.

Stefan

Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in probe routine

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 10:26 +0200 schrieb Marc Kleine-Budde:
> On 11.08.2025 23:06:07, Stefan Mätje wrote:
> > This patch fixes some problems in the esd_usb_probe routine that render
> > the CAN interface unusable.
> > 
> > The probe routine sends a version request message to the USB device to
> > receive a version reply with the number of CAN ports and the hard-
> > & firmware versions. Then for each CAN port a CAN netdev is registered.
> > 
> > The previous code assumed that the version reply would be received
> > immediately. But if the driver was reloaded without power cycling the
> > USB device (i. e. on a reboot) there could already be other incoming
> > messages in the USB buffers. These would be in front of the version
> > reply and need to be skipped.
> > 
> > In the previous code these problems were present:
> > - Only one usb_bulk_msg() read was done into a buffer of
> >   sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
> >   which could lead to an overflow error from the USB stack.
> > - The first bytes of the received data were taken without checking for
> >   the message type. This could lead to zero detected CAN interfaces.
> > 
> > To mitigate these problems:
> > - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> > - Added a function esd_usb_recv_version() that reads and skips incoming
> >   "esd_usb_msg" messages until a version reply message is found. This
> >   is evaluated to return the count of CAN ports and version information.
> > - The data drain loop is limited by a maximum # of bytes to read from
> >   the device based on its internal buffer sizes and a timeout
> >   ESD_USB_DRAIN_TIMEOUT_MS.
> > 
> > This version of the patch incorporates changes recommended on the
> > linux-can list for a first version.
> > 
> > References:
> > https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r
> > 
> > Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
> > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 125 ++++++++++++++++++++++++++--------
> >  1 file changed, 97 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 05ed664cf59d..dbdfffe3a4a0 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
> >  #define ESD_USB_CMD_TS			5 /* also used for TS_REPLY */
> >  #define ESD_USB_CMD_IDADD		6 /* also used for IDADD_REPLY */
> >  
> > +/* esd version message name size */
> > +#define ESD_USB_FW_NAME_SZ 16
> > +
> >  /* esd CAN message flags - dlc field */
> >  #define ESD_USB_RTR	BIT(4)
> >  #define ESD_USB_NO_BRS	BIT(4)
> > @@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
> >  #define ESD_USB_RX_BUFFER_SIZE		1024
> >  #define ESD_USB_MAX_RX_URBS		4
> >  #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
> > +#define ESD_USB_DRAIN_TIMEOUT_MS	100
> >  
> >  /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
> >  #define ESD_USB_3_BAUDRATE_MODE_DISABLE		0 /* remove from bus */
> > @@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
> >  	u8 nets;
> >  	u8 features;
> >  	__le32 version;
> > -	u8 name[16];
> > +	u8 name[ESD_USB_FW_NAME_SZ];
> >  	__le32 rsvd;
> >  	__le32 ts;
> >  };
> > @@ -625,17 +629,91 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
> >  			    1000);
> >  }
> >  
> > -static int esd_usb_wait_msg(struct esd_usb *dev,
> > -			    union esd_usb_msg *msg)
> > +static int esd_usb_req_version(struct esd_usb *dev, void *buf)
> >  {
> > -	int actual_length;
> > +	union esd_usb_msg *msg = buf;
> >  
> > -	return usb_bulk_msg(dev->udev,
> > -			    usb_rcvbulkpipe(dev->udev, 1),
> > -			    msg,
> > -			    sizeof(*msg),
> > -			    &actual_length,
> > -			    1000);
> > +	msg->hdr.cmd = ESD_USB_CMD_VERSION;
> > +	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> > +	msg->version.rsvd = 0;
> > +	msg->version.flags = 0;
> > +	msg->version.drv_version = 0;
> > +
> > +	return esd_usb_send_msg(dev, msg);
> > +}
> > +
> > +static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf)
> > +{
> > +	/* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
> > +	const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
> > +	unsigned long end_jiffies;
> > +	int cnt_other = 0;
> > +	int cnt_ts = 0;
> > +	int cnt_vs = 0;
> > +	int len_sum = 0;
> > +	int attempt = 0;
> > +	int err;
> > +
> > +	end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
> > +	do {
> > +		int actual_length;
> > +		int pos;
> > +
> > +		err = usb_bulk_msg(dev->udev,
> > +				   usb_rcvbulkpipe(dev->udev, 1),
> > +				   rx_buf,
> > +				   ESD_USB_RX_BUFFER_SIZE,
> > +				   &actual_length,
> > +				   ESD_USB_DRAIN_TIMEOUT_MS);
> > +		dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> > +		if (err)
> > +			goto bail;
> > +
> > +		err = -ENOENT;
> > +		len_sum += actual_length;
> > +		pos = 0;
> > +		while (pos < actual_length) {
> 
> You have to check that the actual offset you will access later is within
> actual_length.

+			union esd_usb_msg *p_msg;
> > +
> > +			p_msg = (union esd_usb_msg *)(rx_buf + pos);
> > +

I have moved the calculation of the next position and the check if it fits into
actual_length of the buffer to this point. This will do the check for access is
in actual length.

> > 
> > +			switch (p_msg->hdr.cmd) {
> > +			case ESD_USB_CMD_VERSION:
> > +				++cnt_vs;
> > +				dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
> > +				dev->version = le32_to_cpu(p_msg->version_reply.version);
> > +				err = 0;
> > +				dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
> > +					le32_to_cpu(p_msg->version_reply.ts),
> > +					le32_to_cpu(p_msg->version_reply.version),
> > +					p_msg->version_reply.nets,
> > +					p_msg->version_reply.features,
> > +					ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
> > +				break;
> > +			case ESD_USB_CMD_TS:
> > +				++cnt_ts;
> > +				dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
> > +					le32_to_cpu(p_msg->rx.ts));
> > +				break;
> > +			default:
> > +				++cnt_other;
> > +				dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
> > +				break;
> > +			}
> > +			pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
> 
> Can pos overflow? hdr.len is a u8, so probably not.

pos is limited in the while loop to be < actual_length which can't be bigger than ESD_USB_RX_BUFFER_SIZE (1kB).
And "p_msg->hdr.len * sizeof(u32)" is limited to 1024 being hdr.len an u8 as you mentioned. So overflow
is not possible here.

> 
> > +			if (pos > actual_length) {
> > +				dev_err(&dev->udev->dev, "format error\n");
> > +				err = -EPROTO;
> > +				goto bail;
> > +			}
> > +		}
> > +		++attempt;
> > +	} while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
> > +bail:
> > +	dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
> > +		err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
> > +	return err;
> >  }
> >  
> >  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> > @@ -1274,7 +1352,7 @@ static int esd_usb_probe(struct usb_interface *intf,
> >  			 const struct usb_device_id *id)
> >  {
> >  	struct esd_usb *dev;
> > -	union esd_usb_msg *msg;
> > +	void *buf;
> >  	int i, err;
> >  
> >  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > @@ -1289,34 +1367,25 @@ static int esd_usb_probe(struct usb_interface *intf,
> >  
> >  	usb_set_intfdata(intf, dev);
> >  
> > -	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> > -	if (!msg) {
> > +	buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
> > +	if (!buf) {
> >  		err = -ENOMEM;
> >  		goto free_dev;
> >  	}
> >  
> >  	/* query number of CAN interfaces (nets) */
> > -	msg->hdr.cmd = ESD_USB_CMD_VERSION;
> > -	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> > -	msg->version.rsvd = 0;
> > -	msg->version.flags = 0;
> > -	msg->version.drv_version = 0;
> > -
> > -	err = esd_usb_send_msg(dev, msg);
> > +	err = esd_usb_req_version(dev, buf);
> 
> I'm a bit uneasy with the passing of the buffer as an argument, but not
> its size.
> 
> >  	if (err < 0) {
> >  		dev_err(&intf->dev, "sending version message failed\n");
> > -		goto free_msg;
> > +		goto free_buf;
> >  	}
> >  
> > -	err = esd_usb_wait_msg(dev, msg);
> > +	err = esd_usb_recv_version(dev, buf);
> >  	if (err < 0) {
> >  		dev_err(&intf->dev, "no version message answer\n");
> > -		goto free_msg;
> > +		goto free_buf;
> >  	}
> >  
> > -	dev->net_count = (int)msg->version_reply.nets;
> > -	dev->version = le32_to_cpu(msg->version_reply.version);
> > -
> >  	if (device_create_file(&intf->dev, &dev_attr_firmware))
> >  		dev_err(&intf->dev,
> >  			"Couldn't create device file for firmware\n");
> > @@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
> >  	for (i = 0; i < dev->net_count; i++)
> >  		esd_usb_probe_one_net(intf, i);
> >  
> > -free_msg:
> > -	kfree(msg);
> > +free_buf:
> > +	kfree(buf);
> >  free_dev:
> >  	if (err)
> >  		kfree(dev);
> > -- 
> > 2.34.1
> > 
> > 
> > 
> 
> Marc
> 

Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in probe routine

[Stefan Mätje]

Am Mittwoch, dem 13.08.2025 um 20:44 +0900 schrieb Vincent Mailhol:
> [Some people who received this message don't often get email from mailhol@kernel.org. Learn why this is important at 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hi Stefan,
> 
> On 8/13/25 5:26 PM, Marc Kleine-Budde wrote:
> > On 11.08.2025 23:06:07, Stefan Mätje wrote:
> > > This patch fixes some problems in the esd_usb_probe routine that render
> > > the CAN interface unusable.
> > > 
> > > The probe routine sends a version request message to the USB device to
> > > receive a version reply with the number of CAN ports and the hard-
> > > & firmware versions. Then for each CAN port a CAN netdev is registered.
> > > 
> > > The previous code assumed that the version reply would be received
> > > immediately. But if the driver was reloaded without power cycling the
> > > USB device (i. e. on a reboot) there could already be other incoming
> > > messages in the USB buffers. These would be in front of the version
> > > reply and need to be skipped.
> 
> Wouldn't it be easier to:
> 
>   - First empty the incoming USB message queue
>   - Then request the version
>   - Finally parse the version reply
> 
> ?
> 
> This way, you wouldn't have to do the complex parsing anymore in
> esd_usb_recv_version().

The complex parsing must be done anyway. If I do like you propose then

a) Empty the message queue without looking at the data.
b) Then send the version request
c) The device can create a timestamp or CAN RX message at any time here!
d) Read input data and hope for the version response message, then I'm
  again at the point that I need to parse and check the incoming messages
  because I still have no guarantee that the version reply message is
  the first one.

So I can leave out step a). That's what the patch does.



> > > In the previous code these problems were present:
> > > - Only one usb_bulk_msg() read was done into a buffer of
> > >   sizeof(union esd_usb_msg) which is smaller than ESD_USB_RX_BUFFER_SIZE
> > >   which could lead to an overflow error from the USB stack.
> > > - The first bytes of the received data were taken without checking for
> > >   the message type. This could lead to zero detected CAN interfaces.
> > > 
> > > To mitigate these problems:
> > > - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> > > - Added a function esd_usb_recv_version() that reads and skips incoming
> > >   "esd_usb_msg" messages until a version reply message is found. This
> > >   is evaluated to return the count of CAN ports and version information.
> > > - The data drain loop is limited by a maximum # of bytes to read from
> > >   the device based on its internal buffer sizes and a timeout
> > >   ESD_USB_DRAIN_TIMEOUT_MS.
> > > 
> > > This version of the patch incorporates changes recommended on the
> > > linux-can list for a first version.
> > > 
> > > References:
> > > https://lore.kernel.org/linux-can/d7fd564775351ea8a60a6ada83a0368a99ea6b19.camel@esd.eu/#r
> > > 
> > > Fixes: 80662d943075 ("can: esd_usb: Add support for esd CAN-USB/3")
> > > Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
> > > ---
> > >  drivers/net/can/usb/esd_usb.c | 125 ++++++++++++++++++++++++++--------
> > >  1 file changed, 97 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > index 05ed664cf59d..dbdfffe3a4a0 100644
> > > --- a/drivers/net/can/usb/esd_usb.c
> > > +++ b/drivers/net/can/usb/esd_usb.c
> > > @@ -44,6 +44,9 @@ MODULE_LICENSE("GPL v2");
> > >  #define ESD_USB_CMD_TS                      5 /* also used for TS_REPLY */
> > >  #define ESD_USB_CMD_IDADD           6 /* also used for IDADD_REPLY */
> > > 
> > > +/* esd version message name size */
> > > +#define ESD_USB_FW_NAME_SZ 16
> > > +
> > >  /* esd CAN message flags - dlc field */
> > >  #define ESD_USB_RTR BIT(4)
> > >  #define ESD_USB_NO_BRS      BIT(4)
> > > @@ -95,6 +98,7 @@ MODULE_LICENSE("GPL v2");
> > >  #define ESD_USB_RX_BUFFER_SIZE              1024
> > >  #define ESD_USB_MAX_RX_URBS         4
> > >  #define ESD_USB_MAX_TX_URBS         16 /* must be power of 2 */
> > > +#define ESD_USB_DRAIN_TIMEOUT_MS    100
> > > 
> > >  /* Modes for CAN-USB/3, to be used for esd_usb_3_set_baudrate_msg_x.mode */
> > >  #define ESD_USB_3_BAUDRATE_MODE_DISABLE             0 /* remove from bus */
> > > @@ -131,7 +135,7 @@ struct esd_usb_version_reply_msg {
> > >      u8 nets;
> > >      u8 features;
> > >      __le32 version;
> > > -    u8 name[16];
> > > +    u8 name[ESD_USB_FW_NAME_SZ];
> > >      __le32 rsvd;
> > >      __le32 ts;
> > >  };
> > > @@ -625,17 +629,91 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
> > >                          1000);
> > >  }
> > > 
> > > -static int esd_usb_wait_msg(struct esd_usb *dev,
> > > -                        union esd_usb_msg *msg)
> > > +static int esd_usb_req_version(struct esd_usb *dev, void *buf)
> > >  {
> > > -    int actual_length;
> > > +    union esd_usb_msg *msg = buf;
> > > 
> > > -    return usb_bulk_msg(dev->udev,
> > > -                        usb_rcvbulkpipe(dev->udev, 1),
> > > -                        msg,
> > > -                        sizeof(*msg),
> > > -                        &actual_length,
> > > -                        1000);
> > > +    msg->hdr.cmd = ESD_USB_CMD_VERSION;
> > > +    msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> > > +    msg->version.rsvd = 0;
> > > +    msg->version.flags = 0;
> > > +    msg->version.drv_version = 0;
> > > +
> > > +    return esd_usb_send_msg(dev, msg);
> > > +}
> > > +
> > > +static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf)
> > > +{
> > > +    /* Device hardware has 2 RX buffers with ESD_USB_RX_BUFFER_SIZE, * 4 to give some slack. */
> > > +    const int max_drain_bytes = (4 * ESD_USB_RX_BUFFER_SIZE);
> > > +    unsigned long end_jiffies;
> > > +    int cnt_other = 0;
> > > +    int cnt_ts = 0;
> > > +    int cnt_vs = 0;
> > > +    int len_sum = 0;
> > > +    int attempt = 0;
> > > +    int err;
> > > +
> > > +    end_jiffies = jiffies + msecs_to_jiffies(ESD_USB_DRAIN_TIMEOUT_MS);
> > > +    do {
> > > +            int actual_length;
> > > +            int pos;
> > > +
> > > +            err = usb_bulk_msg(dev->udev,
> > > +                               usb_rcvbulkpipe(dev->udev, 1),
> > > +                               rx_buf,
> > > +                               ESD_USB_RX_BUFFER_SIZE,
> > > +                               &actual_length,
> > > +                               ESD_USB_DRAIN_TIMEOUT_MS);
> > > +            dev_dbg(&dev->udev->dev, "AT %d, LEN %d, ERR %d\n", attempt, actual_length, err);
> > > +            if (err)
> > > +                    goto bail;
> > > +
> > > +            err = -ENOENT;
> > > +            len_sum += actual_length;
> > > +            pos = 0;
> > > +            while (pos < actual_length) {
> > 
> > You have to check that the actual offset you will access later is within
> > actual_length.
> > 
> > > +                    union esd_usb_msg *p_msg;
> > > +
> > > +                    p_msg = (union esd_usb_msg *)(rx_buf + pos);
> > > +
> > > +                    switch (p_msg->hdr.cmd) {
> > > +                    case ESD_USB_CMD_VERSION:
> > > +                            ++cnt_vs;
> > > +                            dev->net_count = min(p_msg->version_reply.nets, ESD_USB_MAX_NETS);
> > > +                            dev->version = le32_to_cpu(p_msg->version_reply.version);
> > > +                            err = 0;
> > > +                            dev_dbg(&dev->udev->dev, "TS 0x%08x, V 0x%08x, N %u, F 0x%02x, %.*s\n",
> > > +                                    le32_to_cpu(p_msg->version_reply.ts),
> > > +                                    le32_to_cpu(p_msg->version_reply.version),
> > > +                                    p_msg->version_reply.nets,
> > > +                                    p_msg->version_reply.features,
> > > +                                    ESD_USB_FW_NAME_SZ, p_msg->version_reply.name);
> > > +                            break;
> > > +                    case ESD_USB_CMD_TS:
> > > +                            ++cnt_ts;
> > > +                            dev_dbg(&dev->udev->dev, "TS 0x%08x\n",
> > > +                                    le32_to_cpu(p_msg->rx.ts));
> > > +                            break;
> > > +                    default:
> > > +                            ++cnt_other;
> > > +                            dev_dbg(&dev->udev->dev, "HDR %d\n", p_msg->hdr.cmd);
> > > +                            break;
> > > +                    }
> > > +                    pos += p_msg->hdr.len * sizeof(u32); /* convert to # of bytes */
> > 
> > Can pos overflow? hdr.len is a u8, so probably not.
> > 
> > > +                    if (pos > actual_length) {
> > > +                            dev_err(&dev->udev->dev, "format error\n");
> > > +                            err = -EPROTO;
> > > +                            goto bail;
> > > +                    }
> > > +            }
> > > +            ++attempt;
> > > +    } while (cnt_vs == 0 && len_sum < max_drain_bytes && time_before(jiffies, end_jiffies));
> > > +bail:
> > > +    dev_dbg(&dev->udev->dev, "RC=%d; ATT=%d, TS=%d, VS=%d, O=%d, B=%d\n",
> > > +            err, attempt, cnt_ts, cnt_vs, cnt_other, len_sum);
> > > +    return err;
> > >  }
> > > 
> > >  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> > > @@ -1274,7 +1352,7 @@ static int esd_usb_probe(struct usb_interface *intf,
> > >                       const struct usb_device_id *id)
> > >  {
> > >      struct esd_usb *dev;
> > > -    union esd_usb_msg *msg;
> > > +    void *buf;
> > >      int i, err;
> > > 
> > >      dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > > @@ -1289,34 +1367,25 @@ static int esd_usb_probe(struct usb_interface *intf,
> > > 
> > >      usb_set_intfdata(intf, dev);
> > > 
> > > -    msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> > > -    if (!msg) {
> > > +    buf = kmalloc(ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL);
> > > +    if (!buf) {
> > >              err = -ENOMEM;
> > >              goto free_dev;
> > >      }
> > > 
> > >      /* query number of CAN interfaces (nets) */
> > > -    msg->hdr.cmd = ESD_USB_CMD_VERSION;
> > > -    msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
> > > -    msg->version.rsvd = 0;
> > > -    msg->version.flags = 0;
> > > -    msg->version.drv_version = 0;
> > > -
> > > -    err = esd_usb_send_msg(dev, msg);
> > > +    err = esd_usb_req_version(dev, buf);
> > 
> > I'm a bit uneasy with the passing of the buffer as an argument, but not
> > its size.
> 
> +1
> 
> What I also do not like is that buffer of type void *. If I understand
> correctly, you are using this buffer for both the request and the reply, thus
> the void* type. But is this really a winning trade?
> 
> Here we are in the probe function, not something speed critical. So, I would
> rather have the esd_usb_req_version() and the esd_usb_recv_version() allocate
> their own buffer with the correct size.
> 
> Yes, you would be doing two kalloc() instead of one, but you will gain in
> readability and the esd_usb_probe() also becomes simpler.

In a V2 now I'm allocating the used buffers for transmit in esd_usb_req_version()
and for receive in esd_usb_recv_version(). 

The receive buffer is of ESD_USB_RX_BUFFER_SIZE because that's the maximum size
the device sends en bloc with messages. The firmware makes sure that a
message won't cross this HW buffer block size boundary.


> > >      if (err < 0) {
> > >              dev_err(&intf->dev, "sending version message failed\n");
> > > -            goto free_msg;
> > > +            goto free_buf;
> > >      }
> > > 
> > > -    err = esd_usb_wait_msg(dev, msg);
> > > +    err = esd_usb_recv_version(dev, buf);
> > >      if (err < 0) {
> > >              dev_err(&intf->dev, "no version message answer\n");
> > > -            goto free_msg;
> > > +            goto free_buf;
> > >      }
> > > 
> > > -    dev->net_count = (int)msg->version_reply.nets;
> > > -    dev->version = le32_to_cpu(msg->version_reply.version);
> > > -
> > >      if (device_create_file(&intf->dev, &dev_attr_firmware))
> > >              dev_err(&intf->dev,
> > >                      "Couldn't create device file for firmware\n");
> > > @@ -1333,8 +1402,8 @@ static int esd_usb_probe(struct usb_interface *intf,
> > >      for (i = 0; i < dev->net_count; i++)
> > >              esd_usb_probe_one_net(intf, i);
> > > 
> > > -free_msg:
> > > -    kfree(msg);
> > > +free_buf:
> > > +    kfree(buf);
> > >  free_dev:
> > >      if (err)
> > >              kfree(dev);
> --
> Yours sincerely,
> Vincent Mailhol
>