[PATCH 0/1] can: esd_usb: Fix device probe routine

[PATCH 0/1] can: esd_usb: Fix device probe routine

[Stefan Mätje]

The included patch fixes a possible failure to detect the CAN-USB/* device 
after a reboot. The patch has been published as RFC already but I didn't get
any comments. See

https://lore.kernel.org/linux-can/20241204160640.884578-1-stefan.maetje@esd.eu/


The patch carries a Fixes: tag for the commit where the support for the
CAN-USB/3-FD hit the 6.6 mainline kernel even if the erroneous code was
present in previous releases. This is done this way because with the new
hardware the problem became more visible.

Stefan Mätje (1):
  can: esd_usb: Fix not detecting version reply in probe routine

 drivers/net/can/usb/esd_usb.c | 122 +++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 30 deletions(-)


base-commit: c1a6911485b022e093c589c295a953ff744112b9
-- 
2.34.1

[PATCH 1/1] 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.
- On kmalloc() fail for the "union esd_usb_msg msg" (i. e. msg == NULL)
  kfree() would be called with this NULL pointer.

To mitigate these problems:
- Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
- Fix the error exit path taken after allocation failure for the
  transfer buffer.
- 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.

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 | 122 +++++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 30 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 03ad10b01867..a6b3b100f8ac 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -625,17 +625,86 @@ 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)
+{
+	union esd_usb_msg *msg = buf;
+
+	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,
+				u32 *version,
+				int *net_count)
 {
 	int actual_length;
+	int cnt_other = 0;
+	int cnt_ts = 0;
+	int cnt_vs = 0;
+	int attempt;
+	int pos;
+	int err;
 
-	return usb_bulk_msg(dev->udev,
-			    usb_rcvbulkpipe(dev->udev, 1),
-			    msg,
-			    sizeof(*msg),
-			    &actual_length,
-			    1000);
+	for (attempt = 0; attempt < 8 && cnt_vs == 0; ++attempt) {
+		err = usb_bulk_msg(dev->udev,
+				   usb_rcvbulkpipe(dev->udev, 1),
+				   rx_buf,
+				   ESD_USB_RX_BUFFER_SIZE,
+				   &actual_length,
+				   1000);
+		if (err)
+			break;
+
+		err = -ENOENT;
+		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;
+				*net_count = (int)p_msg->version_reply.nets;
+				*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, %.16s\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,
+					(char *)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;
+			}
+		}
+	}
+bail:
+	dev_dbg(&dev->udev->dev, "%s()->%d; ATT=%d, TS=%d, VS=%d, O=%d\n",
+		__func__, err, attempt, cnt_ts, cnt_vs, cnt_other);
+	return err;
 }
 
 static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
@@ -1258,7 +1327,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;
@@ -1273,13 +1342,13 @@ 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);
 	if (!dev) {
 		err = -ENOMEM;
-		goto done;
+		goto bail;
 	}
 
 	dev->udev = interface_to_usbdev(intf);
@@ -1288,34 +1357,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_msg;
+		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, &dev->version, &dev->net_count);
 	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");
@@ -1332,11 +1392,12 @@ 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);
-done:
+bail:
 	return err;
 }
 
@@ -1357,6 +1418,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

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

[Marc Kleine-Budde]

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

On 03.02.2025 15:58:10, 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.
> - On kmalloc() fail for the "union esd_usb_msg msg" (i. e. msg == NULL)
>   kfree() would be called with this NULL pointer.
> 
> To mitigate these problems:
> - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> - Fix the error exit path taken after allocation failure for the
>   transfer buffer.
> - 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.
> 
> 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 | 122 +++++++++++++++++++++++++---------
>  1 file changed, 92 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 03ad10b01867..a6b3b100f8ac 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -625,17 +625,86 @@ 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)
> +{
> +	union esd_usb_msg *msg = buf;
> +
> +	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,
> +				u32 *version,
> +				int *net_count)

static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf,
				u32 *version, int *net_count)

>  {
>  	int actual_length;
> +	int cnt_other = 0;
> +	int cnt_ts = 0;
> +	int cnt_vs = 0;
> +	int attempt;
> +	int pos;
> +	int err;

Try to reduce the scope of the variables and move them into the for-loop.

>  
> -	return usb_bulk_msg(dev->udev,
> -			    usb_rcvbulkpipe(dev->udev, 1),
> -			    msg,
> -			    sizeof(*msg),
> -			    &actual_length,
> -			    1000);
> +	for (attempt = 0; attempt < 8 && cnt_vs == 0; ++attempt) {

Can you create a #define for the "8" to avoid a magic number here?

> +		err = usb_bulk_msg(dev->udev,
> +				   usb_rcvbulkpipe(dev->udev, 1),
> +				   rx_buf,
> +				   ESD_USB_RX_BUFFER_SIZE,
> +				   &actual_length,
> +				   1000);
> +		if (err)
> +			break;

nitpick: I would make it explicit with "goto bail", should be the same?

> +
> +		err = -ENOENT;
> +		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;
> +				*net_count = (int)p_msg->version_reply.nets;

Cast not needed.

What happens if nets is > 2? Please sanitize input from outside against
ESD_USB_MAX_NETS.

> +				*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, %.16s\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,
> +					(char *)p_msg->version_reply.name

Is this cast needed?
What about using '"%.*s", sizeof(p_msg->version_reply.name)'?

> +					);

Please move the closing ")" into the previous line.

> +				break;

Why keep parsing after you've found the version?

> +			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;
> +			}
> +		}
> +	}
> +bail:
> +	dev_dbg(&dev->udev->dev, "%s()->%d; ATT=%d, TS=%d, VS=%d, O=%d\n",
> +		__func__, err, attempt, cnt_ts, cnt_vs, cnt_other);
> +	return err;
>  }
>  
>  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> @@ -1258,7 +1327,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;
> @@ -1273,13 +1342,13 @@ 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);
>  	if (!dev) {
>  		err = -ENOMEM;
> -		goto done;
> +		goto bail;
>  	}
>  
>  	dev->udev = interface_to_usbdev(intf);
> @@ -1288,34 +1357,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_msg;
> +		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, &dev->version, &dev->net_count);

Why pass the "&dev->version, &dev->net_count" pointers, if you already
pass dev?

>  	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");
> @@ -1332,11 +1392,12 @@ static int esd_usb_probe(struct usb_interface *intf,
>  	for (i = 0; i < dev->net_count; i++)
>  		esd_usb_probe_one_net(intf, i);

Return values are not checked here. :/

>  
> -free_msg:
> -	kfree(msg);
> +free_buf:
> +	kfree(buf);
> +free_dev:
>  	if (err)
>  		kfree(dev);
> -done:
> +bail:
>  	return err;
>  }
>  
> @@ -1357,6 +1418,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
> 
> 
> 

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 1/1] can: esd_usb: Fix not detecting version reply in probe routine

[Stefan Mätje]

Am Donnerstag, dem 06.02.2025 um 13:45 +0100 schrieb Marc Kleine-Budde:
> > On 03.02.2025 15:58:10, 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.
> > > > - On kmalloc() fail for the "union esd_usb_msg msg" (i. e. msg == NULL)
> > > >   kfree() would be called with this NULL pointer.
> > > > 
> > > > To mitigate these problems:
> > > > - Use a transfer buffer "buf" with ESD_USB_RX_BUFFER_SIZE.
> > > > - Fix the error exit path taken after allocation failure for the
> > > >   transfer buffer.
> > > > - 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.
> > > > 
> > > > 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 | 122 +++++++++++++++++++++++++---------
> > > >  1 file changed, 92 insertions(+), 30 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > > index 03ad10b01867..a6b3b100f8ac 100644
> > > > --- a/drivers/net/can/usb/esd_usb.c
> > > > +++ b/drivers/net/can/usb/esd_usb.c
> > > > @@ -625,17 +625,86 @@ 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)
> > > > +{
> > > > +	union esd_usb_msg *msg = buf;
> > > > +
> > > > +	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,
> > > > +				u32 *version,
> > > > +				int *net_count)
> > 
> > static int esd_usb_recv_version(struct esd_usb *dev, void *rx_buf,
> > 				u32 *version, int *net_count)
> > 
> > > >  {
> > > >  	int actual_length;
> > > > +	int cnt_other = 0;
> > > > +	int cnt_ts = 0;
> > > > +	int cnt_vs = 0;
> > > > +	int attempt;
> > > > +	int pos;
> > > > +	int err;
> > 
> > Try to reduce the scope of the variables and move them into the for-loop.

Changed this in V2.

> > > >  
> > > > -	return usb_bulk_msg(dev->udev,
> > > > -			    usb_rcvbulkpipe(dev->udev, 1),
> > > > -			    msg,
> > > > -			    sizeof(*msg),
> > > > -			    &actual_length,
> > > > -			    1000);
> > > > +	for (attempt = 0; attempt < 8 && cnt_vs == 0; ++attempt) {
> > 
> > Can you create a #define for the "8" to avoid a magic number here?

This value was found empirically. But I think I have to reconsider
the style of handling here and may need to adapt the solution.


> > > > +		err = usb_bulk_msg(dev->udev,
> > > > +				   usb_rcvbulkpipe(dev->udev, 1),
> > > > +				   rx_buf,
> > > > +				   ESD_USB_RX_BUFFER_SIZE,
> > > > +				   &actual_length,
> > > > +				   1000);
> > > > +		if (err)
> > > > +			break;
> > 
> > nitpick: I would make it explicit with "goto bail", should be the same?

Changed this in V2.


> > > > +
> > > > +		err = -ENOENT;
> > > > +		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;
> > > > +				*net_count = (int)p_msg->version_reply.nets;
> > 
> > Cast not needed.

Changed this in V2.


> > What happens if nets is > 2? Please sanitize input from outside against
> > ESD_USB_MAX_NETS.

Will limit the net_count using min() macro in V2.

> > > > +				*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, %.16s\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,
> > > > +					(char *)p_msg->version_reply.name
> > 
> > Is this cast needed?
> > What about using '"%.*s", sizeof(p_msg->version_reply.name)'?

Added a #define for the name attribute size that is also used in the
structure declaration and here as field precision.


> > > > +					);
> > 
> > Please move the closing ")" into the previous line.

Changed in V2.


> > > > +				break;
> > 
> > Why keep parsing after you've found the version?

The version message is assumed to be the last in the block of messages,
so actual_length should be exhausted and the while (pos < actual_length)
loop should stop anyway. If the unexpected happens that there is more
data left after the version message the loop continues to check at least
that the rest of the data forms valid message blocks and bails out if
not.

> > > > +			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;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +bail:
> > > > +	dev_dbg(&dev->udev->dev, "%s()->%d; ATT=%d, TS=%d, VS=%d, O=%d\n",
> > > > +		__func__, err, attempt, cnt_ts, cnt_vs, cnt_other);
> > > > +	return err;
> > > >  }
> > > >  
> > > >  static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> > > > @@ -1258,7 +1327,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;
> > > > @@ -1273,13 +1342,13 @@ 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);
> > > >  	if (!dev) {
> > > >  		err = -ENOMEM;
> > > > -		goto done;
> > > > +		goto bail;
> > > >  	}
> > > >  
> > > >  	dev->udev = interface_to_usbdev(intf);
> > > > @@ -1288,34 +1357,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_msg;
> > > > +		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, &dev->version, &dev->net_count);
> > 
> > Why pass the "&dev->version, &dev->net_count" pointers, if you already
> > pass dev?

Good point. Changed in V2.

> > > >  	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");
> > > > @@ -1332,11 +1392,12 @@ static int esd_usb_probe(struct usb_interface *intf,
> > > >  	for (i = 0; i < dev->net_count; i++)
> > > >  		esd_usb_probe_one_net(intf, i);
> > 
> > Return values are not checked here. :/

Yes, that is another flaw. This needs to be tackled in another patch.


> > > >  
> > > > -free_msg:
> > > > -	kfree(msg);
> > > > +free_buf:
> > > > +	kfree(buf);
> > > > +free_dev:
> > > >  	if (err)
> > > >  		kfree(dev);
> > > > -done:
> > > > +bail:
> > > >  	return err;
> > > >  }
> > > >  
> > > > @@ -1357,6 +1418,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
> > > > 
> > > > 
> > > > 
> > 
> > regards,
> > Marc
> > 

Thanks for looking at the patch.

Best regards,
   Stefan

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

[Marc Kleine-Budde]

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

On 09.02.2025 18:53:43, Stefan Mätje wrote:
> > > > >  	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");
> > > > > @@ -1332,11 +1392,12 @@ static int esd_usb_probe(struct usb_interface *intf,
> > > > >  	for (i = 0; i < dev->net_count; i++)
> > > > >  		esd_usb_probe_one_net(intf, i);
> > > 
> > > Return values are not checked here. :/
> 
> Yes, that is another flaw. This needs to be tackled in another patch.

ACK

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 1/1] can: esd_usb: Fix not detecting version reply in probe routine

[Marc Kleine-Budde]

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

On 09.02.2025 18:53:43, Stefan Mätje wrote:
> > > > > -	return usb_bulk_msg(dev->udev,
> > > > > -			    usb_rcvbulkpipe(dev->udev, 1),
> > > > > -			    msg,
> > > > > -			    sizeof(*msg),
> > > > > -			    &actual_length,
> > > > > -			    1000);
> > > > > +	for (attempt = 0; attempt < 8 && cnt_vs == 0; ++attempt) {
> > > 
> > > Can you create a #define for the "8" to avoid a magic number here?
> 
> This value was found empirically. But I think I have to reconsider
> the style of handling here and may need to adapt the solution.

You're the expert, change it as needed. I can only comment on the coding
style and its best to avoid numbers in the code. Create a define and if
you think it's worth, document that it's an empirically found value.

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 --]