On Mon, 2026-03-02 at 13:59 +0100, Marek Vasut wrote:
> On 3/2/26 5:45 AM, Ronan Dalton wrote:
>
> [...]
>
> > @@ -276,19 +279,22 @@ int usb_control_msg(struct usb_device *dev,
> > unsigned int pipe,
> > int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
> > void *data, int len, int *actual_length,
> > int timeout)
> > {
> > + volatile unsigned long usb_status;
> > +
> > if (len < 0)
> > return -EINVAL;
> >
> > + if (timeout < 0)
> > + return -EINVAL;
> > +
> > dev->status = USB_ST_NOT_PROC; /*not yet processed */
> >
> > if (submit_bulk_msg(dev, pipe, data, len) < 0)
> > return -EIO;
> >
> > - while (timeout--) {
> > - if (!((volatile unsigned long)dev->status &
> > USB_ST_NOT_PROC))
> > - break;
> > - mdelay(1);
> > - }
> > + read_poll_timeout((volatile unsigned long), usb_status,
> > + !(usb_status & USB_ST_NOT_PROC), 1000,
> > + timeout * 1000UL, dev->status);
>
> Maybe this could use an error check here too ?
>
> With that fixed:
>
> Reviewed-by: Marek Vasut <[email protected]>
In patch v2 I made the following note about this:
> usb_bulk_msg could be updated to return -ETIMEDOUT on the timeout
> case. Currently it looks like -EIO is returned if a timeout occurs
> and callers can't tell if a timeout occurred as such. The -EIO
> return code is also inconsistent with usb_control_msg which
> returns -1.
I've made it return -ETIMEDOUT now in the timeout case. However it's
something I'm a bit hesitant about since that will change the error
code returned now (away from -EIO).
The setting of the actual_length output param is also questionable. I
was going to put the timeout check right after the call to
read_poll_timeout, but decided to set the actual_length value first to
minimize the chance of breakage. Callers to usb_bulk_msg really
shouldn't be inpecting the value of that output parameter if the
function returns an error, but some do seem to do that, and they would
be reading garbage in the timeout case.
