Message-ID: <788ba8655a966909a78a40c7ac475999448ae4c1.camel@esd.eu>
Date: Tue, 19 Aug 2025 16:41:39 +0000
From: Stefan Mätje <stefan.maetje@....eu>
To: "mailhol@...nel.org" <mailhol@...nel.org>
CC: "socketcan@...tkopp.net" <socketcan@...tkopp.net>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>, socketcan
	<socketcan@....eu>, "mkl@...gutronix.de" <mkl@...gutronix.de>, Frank
 Jungclaus <frank.jungclaus@....eu>, "horms@...nel.org" <horms@...nel.org>,
	"olivier@...rie.be" <olivier@...rie.be>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: [PATCH 2/6] can: esd_usb: Fix not detecting version reply in
 probe routine

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@...nel.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@....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
> 

Powered by blists - more mailing lists