On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jan 27, 2023 at 03:12:11PM +0100, Troels Liebe Bentsen wrote:
> > On Thu, 26 Jan 2023 at 17:17, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 02:59:35PM +0100, Troels Liebe Bentsen wrote:
> > > > It might be a special use case, for our automated hardware testing station
> > > > we know what goes into what port/hub so it would be nice to have an option
> > > > to lower the timeout in general or per hub or per port.
> > >
> > > There already is such an option. It's named "early_stop" and it's
> > > described in Documentation/ABI/testing/sysfs-bus-usb. You may not be
> > > aware of it because it was added after the 6.1 version of the kernel was
> > > released.
> >
> > Thanks, is it something that has to be enabled when compiling the kernel?
>
> No, it is always enabled.
>
> > Using Ubuntu mainline 6.1.4 and it does not seem to have this file in the
> > ports folder.
>
> Probably because it is still quite new.
>
> > > > Ahh, sorry I misunderstood but then it makes even less sense that the hub's
> > > > descriptors file blocks when the device hangs or am I missing something.
> > >
> > > Reading the file blocks because it has to be mutually exclusive with
> > > other parts of the kernel which can reallocate the buffers storing the
> > > descriptors. This mutual exclusion is provided by a per-device lock,
> > > and for hubs this device lock is held while a child device is being
> > > probed.
> > >
> > I guess the reasoning for also holding a lock for the hubs is that there is some
> > accounting on the hub for the children. But the hub's descriptors file won't
> > change because a child device got enumerated right?
>
> That's true, and in principle a separate lock could be used. But there
> never seemed to be any need for adding a second lock. Until now.
>
> > Also if I understand it correctly based on your second email both the parsed
> > sysfs files(fx idProduct, idVendor, etc.) and the descriptors file won't change
> > for the lifetime of the hub. It's just that the descriptors file is backed by
> > buffers that need to be locked because they can be reallocated by the kernel.
>
> The files that export data from the device descriptor (idProduct etc.)
> might change during a device's lifetime, but the buffer they read from
> won't get reallocated.
>
> However, now that I look more closely I see that the buffers for the
> config descriptors won't get reallocated either! Evidently this was
> changed back in 2014 by commit e50a322e5177 ("usb: Do not re-read
> descriptors for wired devices in usb_authorize_device()"), but nobody
> realized at the time that the locking for the descriptors sysfs could
> then be relaxed.
>
> > Why not store the descriptors file the same way as the parsed sysfs files,
> > etc. it seems fairly small and I guess it contains the same data that is found
> > in the sysfs folder for the hub?
>
> The buffer holding the device descriptor is fixed size, since all device
> descriptors are always 18 bytes long. The configuration and interface
> descriptors are variable length, so their buffers have to be allocated
> on-the-fly.
>
> > I don't know how much work it would be to try to avoid locking the hub or if
> > it even makes sense. So maybe it would be better if we spent time trying to
> > workaround this in userspace, fx. by getting libusb to open the descriptors
> > file in non-blocking mode and falling back to reading the individual files in
> > sysfs if we get an EAGAIN. Would there be anything wrong with that
> > approach?
>
> Now that I know the config descriptors won't get reallocated after all,
> I can remove the locking from the descriptors file entirely. A patch to
> do this is below. It ought to fix your problem. Try it and see.
Thank you very much, I built a new kernel with the patch and can confirm
that it fixes my issue.
Will the patch make it into any of the LTS kernels as it could seem like a
bugfix depending on how you look at it or is it only destined mainline, fx. 6.2
or 6.3?
>
> > Also in general thanks for the quick and detailed responses, it's a
> > pleasure writing on this mailing list.
>
> You're welcome.
>
> Alan Stern
>
>
>
> Index: usb-devel/drivers/usb/core/sysfs.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/sysfs.c
> +++ usb-devel/drivers/usb/core/sysfs.c
> @@ -869,11 +869,7 @@ read_descriptors(struct file *filp, stru
> size_t srclen, n;
> int cfgno;
> void *src;
> - int retval;
>
> - retval = usb_lock_device_interruptible(udev);
> - if (retval < 0)
> - return -EINTR;
> /* The binary attribute begins with the device descriptor.
> * Following that are the raw descriptor entries for all the
> * configurations (config plus subsidiary descriptors).
> @@ -898,7 +894,6 @@ read_descriptors(struct file *filp, stru
> off -= srclen;
> }
> }
> - usb_unlock_device(udev);
> return count - nleft;
> }
>
>
