All USB tools hang when one descriptor read fails and needs to timeout

All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

Hi,

We have a hardware projekt where something is off with power ON
timing. It sometimes gets started in a broken state where the device
is seen by the USB system but does not respond to descriptor reads.

When this happens this causes lsusb and libusb based tools to hang
until the descriptor read in the USB subsystem timeout out after 30
seconds or so. It looks like the tools are trying to read
/sys/bus/usb/devices/.../descriptors and it blocks until the timeout
happens.

We should fix our hardware and have done so in the next revision but
why should one device be able to block the descriptors file that most
user land USB code seem to use.

Would there be any reasoning against just serving the descriptors file
as it looked before inserting the broken USB device instead of
blocking the read?

And if we wanted to create a pull request for a change like that would
it be accepted or would it be considered breaking the API?

Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Greg KH]

On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> Hi,
> 
> We have a hardware projekt where something is off with power ON
> timing. It sometimes gets started in a broken state where the device
> is seen by the USB system but does not respond to descriptor reads.

Ah, a broken USB device, not much the kernel can do about that :(

> When this happens this causes lsusb and libusb based tools to hang
> until the descriptor read in the USB subsystem timeout out after 30
> seconds or so. It looks like the tools are trying to read
> /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> happens.
> 
> We should fix our hardware and have done so in the next revision but
> why should one device be able to block the descriptors file that most
> user land USB code seem to use.

If the device does not respond, what is the kernel or userspace supposed
to do?

> Would there be any reasoning against just serving the descriptors file
> as it looked before inserting the broken USB device instead of
> blocking the read?

So a different device's descriptor file is being blocked by a broken
device?  Are you sure?  They should all be independent.

> And if we wanted to create a pull request for a change like that would
> it be accepted or would it be considered breaking the API?

It depends on what the kernel change looks like.  Have you tried
anything that worked for you that you wish to propose?

thanks,

greg k-h

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

Hi Greg,

On Thu, 26 Jan 2023 at 13:23, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> > Hi,
> >
> > We have a hardware projekt where something is off with power ON
> > timing. It sometimes gets started in a broken state where the device
> > is seen by the USB system but does not respond to descriptor reads.
>
> Ah, a broken USB device, not much the kernel can do about that :(

Would be nice if it could, but I settle for papering over the worst parts  :)

>
> > When this happens this causes lsusb and libusb based tools to hang
> > until the descriptor read in the USB subsystem timeout out after 30
> > seconds or so. It looks like the tools are trying to read
> > /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> > happens.
> >
> > We should fix our hardware and have done so in the next revision but
> > why should one device be able to block the descriptors file that most
> > user land USB code seem to use.
>
> If the device does not respond, what is the kernel or userspace supposed
> to do?

Might not have been clear that the issue is when I "plug in"/connect the
device that it happens. I think the kernel is doing the right thing here and
just timing out the device and at least in the kernel it does not seem to block
other devices from doing their thing. The problem is that part of the userspace
interface used for listing all devices on a hub block until the timeout
happens.

One nice options would be if the timeout was configurable based on
port/devpath, I can understand 30-60 seconds timeout for a spindel USB
disk, but for other devices 5 seconds would be more than enough to
conclude they are dead, but that's nice to have.

>
> > Would there be any reasoning against just serving the descriptors file
> > as it looked before inserting the broken USB device instead of
> > blocking the read?
>
> So a different device's descriptor file is being blocked by a broken
> device?  Are you sure?  They should all be independent.

Not the descriptor file, but the descriptor*s* under the hub folder in sysfs:

From https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb :

What: /sys/bus/usb/devices/usbX/descriptors
Description:
Contains the interface descriptors, in binary.

As I understand it, this is a file that contains all the information for devices
under a hub. Most likely an optimization made for lsusb and libusb so they
don't have to traverse all the individual folders to get the same information.
The problem is that /sys/bus/usb/devices/usbX/descriptors block while the
kernel is trying to do the descriptor read on that one broken device even if
all the other devices on the hub is well behaving the read is blocked until
it times out.

If lsusb and libusb had traversed the folder structure they would not have
blocked as the broken devices never showed in the folder structure:

fx.

tlb@tlb-nuc:/sys/bus/usb/devices/usb3$ ls -1
3-0:1.0 # Device shows up when USB initialization is done
3-10 # Device shows up when USB initialization is done
3-3 # Device shows up when USB initialization is done
...
descriptors # Blocks on read until kernel times out waiting for descriptor read.
...

> > And if we wanted to create a pull request for a change like that would
> > it be accepted or would it be considered breaking the API?
>
> It depends on what the kernel change looks like.  Have you tried
> anything that worked for you that you wish to propose?
>

I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.

Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Greg KH]

On Thu, Jan 26, 2023 at 02:06:39PM +0100, Troels Liebe Bentsen wrote:
> Hi Greg,
> 
> On Thu, 26 Jan 2023 at 13:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> > > Hi,
> > >
> > > We have a hardware projekt where something is off with power ON
> > > timing. It sometimes gets started in a broken state where the device
> > > is seen by the USB system but does not respond to descriptor reads.
> >
> > Ah, a broken USB device, not much the kernel can do about that :(
> 
> Would be nice if it could, but I settle for papering over the worst parts  :)
> 
> >
> > > When this happens this causes lsusb and libusb based tools to hang
> > > until the descriptor read in the USB subsystem timeout out after 30
> > > seconds or so. It looks like the tools are trying to read
> > > /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> > > happens.
> > >
> > > We should fix our hardware and have done so in the next revision but
> > > why should one device be able to block the descriptors file that most
> > > user land USB code seem to use.
> >
> > If the device does not respond, what is the kernel or userspace supposed
> > to do?
> 
> Might not have been clear that the issue is when I "plug in"/connect the
> device that it happens. I think the kernel is doing the right thing here and
> just timing out the device and at least in the kernel it does not seem to block
> other devices from doing their thing. The problem is that part of the userspace
> interface used for listing all devices on a hub block until the timeout
> happens.

What userspace code exactly is doing this?  Perhaps just work on making
that tool handle timeouts better?

> One nice options would be if the timeout was configurable based on
> port/devpath, I can understand 30-60 seconds timeout for a spindel USB
> disk, but for other devices 5 seconds would be more than enough to
> conclude they are dead, but that's nice to have.

The timeouts the kernel has now have had to be increased over time due
to bad devices.  And we don't know what type of device this is until we
read from it, so you can't pick the timeout based on the type of device
if you can't read the type of device :)

> >
> > > Would there be any reasoning against just serving the descriptors file
> > > as it looked before inserting the broken USB device instead of
> > > blocking the read?
> >
> > So a different device's descriptor file is being blocked by a broken
> > device?  Are you sure?  They should all be independent.
> 
> Not the descriptor file, but the descriptor*s* under the hub folder in sysfs:
> 
> >From https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb :
> 
> What: /sys/bus/usb/devices/usbX/descriptors
> Description:
> Contains the interface descriptors, in binary.
> 
> As I understand it, this is a file that contains all the information for devices
> under a hub.

No, just the single device's descriptor.

> Most likely an optimization made for lsusb and libusb so they
> don't have to traverse all the individual folders to get the same information.

Nope, libusb walks the whole bus if you ask it to.

> The problem is that /sys/bus/usb/devices/usbX/descriptors block while the
> kernel is trying to do the descriptor read on that one broken device even if
> all the other devices on the hub is well behaving the read is blocked until
> it times out.

So if you read one descriptor, it will time out for others if you do so
at the same time?  Do they all share a single kernel lock perhaps?

> If lsusb and libusb had traversed the folder structure they would not have
> blocked as the broken devices never showed in the folder structure:
> 
> fx.
> 
> tlb@tlb-nuc:/sys/bus/usb/devices/usb3$ ls -1
> 3-0:1.0 # Device shows up when USB initialization is done
> 3-10 # Device shows up when USB initialization is done
> 3-3 # Device shows up when USB initialization is done
> ...
> descriptors # Blocks on read until kernel times out waiting for descriptor read.
> ...
> 
> > > And if we wanted to create a pull request for a change like that would
> > > it be accepted or would it be considered breaking the API?
> >
> > It depends on what the kernel change looks like.  Have you tried
> > anything that worked for you that you wish to propose?
> >
> 
> I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.

Patches gladly reviewed to do so :)

thanks,

greg k-h

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

On Thu, 26 Jan 2023 at 14:12, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jan 26, 2023 at 02:06:39PM +0100, Troels Liebe Bentsen wrote:
> > Hi Greg,
> >
> > On Thu, 26 Jan 2023 at 13:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> > > > Hi,
> > > >
> > > > We have a hardware projekt where something is off with power ON
> > > > timing. It sometimes gets started in a broken state where the device
> > > > is seen by the USB system but does not respond to descriptor reads.
> > >
> > > Ah, a broken USB device, not much the kernel can do about that :(
> >
> > Would be nice if it could, but I settle for papering over the worst parts  :)
> >
> > >
> > > > When this happens this causes lsusb and libusb based tools to hang
> > > > until the descriptor read in the USB subsystem timeout out after 30
> > > > seconds or so. It looks like the tools are trying to read
> > > > /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> > > > happens.
> > > >
> > > > We should fix our hardware and have done so in the next revision but
> > > > why should one device be able to block the descriptors file that most
> > > > user land USB code seem to use.
> > >
> > > If the device does not respond, what is the kernel or userspace supposed
> > > to do?
> >
> > Might not have been clear that the issue is when I "plug in"/connect the
> > device that it happens. I think the kernel is doing the right thing here and
> > just timing out the device and at least in the kernel it does not seem to block
> > other devices from doing their thing. The problem is that part of the userspace
> > interface used for listing all devices on a hub block until the timeout
> > happens.
>
> What userspace code exactly is doing this?  Perhaps just work on making
> that tool handle timeouts better?

We are using libusb to drive an automated flashing setup to test our hardware
but libusb_get_device_list blocks as it reads
/sys/bus/usb/devices/usbX/descriptors
for the hub the broken hardware is attached to. The hang happens when we
put the device into flash mode(by setting some flashing pins) where the MCU
has some low level USB function where we can load u-boot over a
proprietary protocol but for some reason does not enter fully into this state.

So we have workaround code to see if the hardware has blocked the descriptors
file of the hub and then power cycle the device until it starts
behaving and set a
global mutex that everything else attached to that hub has to wait as we can't
scan the hub.

But the problem is general so if we try to run lsusb or tools using
libusb they will
block until the kernel times out the device.

>
> > One nice options would be if the timeout was configurable based on
> > port/devpath, I can understand 30-60 seconds timeout for a spindel USB
> > disk, but for other devices 5 seconds would be more than enough to
> > conclude they are dead, but that's nice to have.
>
> The timeouts the kernel has now have had to be increased over time due
> to bad devices.  And we don't know what type of device this is until we
> read from it, so you can't pick the timeout based on the type of device
> if you can't read the type of device :)
>

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.

> > >
> > > > Would there be any reasoning against just serving the descriptors file
> > > > as it looked before inserting the broken USB device instead of
> > > > blocking the read?
> > >
> > > So a different device's descriptor file is being blocked by a broken
> > > device?  Are you sure?  They should all be independent.
> >
> > Not the descriptor file, but the descriptor*s* under the hub folder in sysfs:
> >
> > >From https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb :
> >
> > What: /sys/bus/usb/devices/usbX/descriptors
> > Description:
> > Contains the interface descriptors, in binary.
> >
> > As I understand it, this is a file that contains all the information for devices
> > under a hub.
>
> No, just the single device's descriptor.

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.

The other files in /sys/bus/usb/devices/3-3/ such as idProduct, idVendor, etc.
do not block.

This is what I get when the hardware is in broken state:

strace lsusb
...
openat(AT_FDCWD, "/sys/bus/usb/devices/3-3/descriptors", O_RDONLY|O_CLOEXEC) = 8
read(8,
...

dmesg -wH
[  +2.528009] usb 3-3.6: new high-speed USB device number 20 using xhci_hcd
[  +5.300072] usb 3-3.6: device descriptor read/64, error -110
[ +15.615955] usb 3-3.6: device descriptor read/64, error -110
[  +0.187975] usb 3-3.6: new high-speed USB device number 21 using xhci_hcd
[  +5.188069] usb 3-3.6: device descriptor read/64, error -110
[ +15.615973] usb 3-3.6: device descriptor read/64, error -110
[  +0.112158] usb 3-3-port6: attempt power cycle
[  +0.603789] usb 3-3.6: new high-speed USB device number 22 using xhci_hcd
[Jan26 13:33] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
device command
[  +5.375996] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
device command
[  +0.207876] usb 3-3.6: device not accepting address 22, error -62
[  +0.080030] usb 3-3.6: new high-speed USB device number 23 using xhci_hcd
[  +5.088086] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
device command
[  +5.376001] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
device command
[  +0.207889] usb 3-3.6: device not accepting address 23, error -62
[  +0.002418] usb 3-3-port6: unable to enumerate USB device

I tried the other devices attached to the hub and their descriptors do
not block on
read, only the hubs.

> > Most likely an optimization made for lsusb and libusb so they
> > don't have to traverse all the individual folders to get the same information.
>
> Nope, libusb walks the whole bus if you ask it to.
>
> > The problem is that /sys/bus/usb/devices/usbX/descriptors block while the
> > kernel is trying to do the descriptor read on that one broken device even if
> > all the other devices on the hub is well behaving the read is blocked until
> > it times out.
>
> So if you read one descriptor, it will time out for others if you do so
> at the same time?  Do they all share a single kernel lock perhaps?
>
> > If lsusb and libusb had traversed the folder structure they would not have
> > blocked as the broken devices never showed in the folder structure:
> >
> > fx.
> >
> > tlb@tlb-nuc:/sys/bus/usb/devices/usb3$ ls -1
> > 3-0:1.0 # Device shows up when USB initialization is done
> > 3-10 # Device shows up when USB initialization is done
> > 3-3 # Device shows up when USB initialization is done
> > ...
> > descriptors # Blocks on read until kernel times out waiting for descriptor read.
> > ...
> >
> > > > And if we wanted to create a pull request for a change like that would
> > > > it be accepted or would it be considered breaking the API?
> > >
> > > It depends on what the kernel change looks like.  Have you tried
> > > anything that worked for you that you wish to propose?
> > >
> >
> > I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.
>
> Patches gladly reviewed to do so :)

We will have a look and get back to you.

>
> thanks,
>
> greg k-h

Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Hans Petter Selasky]

On 1/26/23 14:59, Troels Liebe Bentsen wrote:
> On Thu, 26 Jan 2023 at 14:12, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>
>>> I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.
>>
>> Patches gladly reviewed to do so :)

Be careful. Been there done that for FreeBSD. You can cache the 
descriptor in memory - yes, but beware that the values inside the device 
descriptor may change after re-enumerating the device via software, like 
firmware upgrade, and that directly hits on the XHCI controller 
programming, that you need to load and configure the new bMaxPacketSize 
in there!

And the same goes for the other fields in there :-)

> 
> We will have a look and get back to you.

It's probably best to find the undocumented bits of your USB peripheral 
controller first! With USB control transactions I've seen so much 
craziness over the years you won't believe it. The only ones that get it 
right, is the ones that lay out all USB control endpoint jobs in memory 
via DMA descriptors. All the ones that simply use a few registers to 
receive the SETUP packet, DATA and status ZLP, have undocumented races. 
By races I mean, what happens if you get SETUP and DATA interrupt bits 
at the same time, or maybe all three, what is the right order, or what 
about STALL conditions and short control transfers and blah blah blah. 
This thing can really blow your mind, but yeah, many device side 
programmers simply use the example code they get from the vendor and 
give a shit about anything that can later go wrong. That is my simple 
impression so far in the USB world.

--HPS

> 
>>
>> thanks,
>>
>> greg k-h
> 
> Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

On Thu, 26 Jan 2023 at 15:27, Hans Petter Selasky <hps@selasky.org> wrote:
>
> On 1/26/23 14:59, Troels Liebe Bentsen wrote:
> > On Thu, 26 Jan 2023 at 14:12, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>
> >>> I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.
> >>
> >> Patches gladly reviewed to do so :)
>
> Be careful. Been there done that for FreeBSD. You can cache the
> descriptor in memory - yes, but beware that the values inside the device
> descriptor may change after re-enumerating the device via software, like
> firmware upgrade, and that directly hits on the XHCI controller
> programming, that you need to load and configure the new bMaxPacketSize
> in there!
>
> And the same goes for the other fields in there :-)

It actually looks like a lot of the other files in sysfs are cached already, fx.
/sys/bus/usb/devices/x-x.x/manufacturer

We tried changing the manufacturer name after our hardware was booted and using
the Linux usb gadget driver and only libusb seemed to be able to see the change.

But that's another question, how do I get Linux to re-enumerating the
device and update the sysfs files?

>
> >
> > We will have a look and get back to you.
>
> It's probably best to find the undocumented bits of your USB peripheral
> controller first! With USB control transactions I've seen so much
> craziness over the years you won't believe it. The only ones that get it
> right, is the ones that lay out all USB control endpoint jobs in memory
> via DMA descriptors. All the ones that simply use a few registers to
> receive the SETUP packet, DATA and status ZLP, have undocumented races.
> By races I mean, what happens if you get SETUP and DATA interrupt bits
> at the same time, or maybe all three, what is the right order, or what
> about STALL conditions and short control transfers and blah blah blah.
> This thing can really blow your mind, but yeah, many device side
> programmers simply use the example code they get from the vendor and
> give a shit about anything that can later go wrong. That is my simple
> impression so far in the USB world.
>

It's an i.MX6 and built in microcode that runs the USB in flash mode, but
we are quite sure it's a power sequencing issue that hangs the CPU in
I.MX6. So bad hardware design and not NPX this time, we have had
plenty of other issues, but this time it does not seem to be their fault.
Also we have some other designs with i.MX6 and don't have
the issue there.

Regards Troels

> --HPS
>
> >
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> > Regards Troels
>

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Thu, Jan 26, 2023 at 04:26:24PM +0100, Troels Liebe Bentsen wrote:
> But that's another question, how do I get Linux to re-enumerating the
> device and update the sysfs files?

You can force a device reset using libusb or the usb-reset program 
floating around the Internet.  After the kernel resets a device, it 
checks to make sure the device, BOS, configuration, and serial-number 
string descriptors have not changed.  If any of them is different from 
what it used to be, the kernel will re-enumerate the device.

Or, you can get the same effect by unplugging the device and then 
plugging it back in.  But with direct-wired connections, that's not 
possible.

Alan Stern

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Thu, Jan 26, 2023 at 02:59:35PM +0100, Troels Liebe Bentsen wrote:
> On Thu, 26 Jan 2023 at 14:12, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jan 26, 2023 at 02:06:39PM +0100, Troels Liebe Bentsen wrote:
> > > Hi Greg,
> > >
> > > On Thu, 26 Jan 2023 at 13:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> > > > > Hi,
> > > > >
> > > > > We have a hardware projekt where something is off with power ON
> > > > > timing. It sometimes gets started in a broken state where the device
> > > > > is seen by the USB system but does not respond to descriptor reads.
> > > >
> > > > Ah, a broken USB device, not much the kernel can do about that :(
> > >
> > > Would be nice if it could, but I settle for papering over the worst parts  :)
> > >
> > > >
> > > > > When this happens this causes lsusb and libusb based tools to hang
> > > > > until the descriptor read in the USB subsystem timeout out after 30
> > > > > seconds or so. It looks like the tools are trying to read
> > > > > /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> > > > > happens.
> > > > >
> > > > > We should fix our hardware and have done so in the next revision but
> > > > > why should one device be able to block the descriptors file that most
> > > > > user land USB code seem to use.
> > > >
> > > > If the device does not respond, what is the kernel or userspace supposed
> > > > to do?
> > >
> > > Might not have been clear that the issue is when I "plug in"/connect the
> > > device that it happens. I think the kernel is doing the right thing here and
> > > just timing out the device and at least in the kernel it does not seem to block
> > > other devices from doing their thing. The problem is that part of the userspace
> > > interface used for listing all devices on a hub block until the timeout
> > > happens.
> >
> > What userspace code exactly is doing this?  Perhaps just work on making
> > that tool handle timeouts better?
> 
> We are using libusb to drive an automated flashing setup to test our hardware
> but libusb_get_device_list blocks as it reads
> /sys/bus/usb/devices/usbX/descriptors
> for the hub the broken hardware is attached to. The hang happens when we
> put the device into flash mode(by setting some flashing pins) where the MCU
> has some low level USB function where we can load u-boot over a
> proprietary protocol but for some reason does not enter fully into this state.
> 
> So we have workaround code to see if the hardware has blocked the descriptors
> file of the hub and then power cycle the device until it starts
> behaving and set a
> global mutex that everything else attached to that hub has to wait as we can't
> scan the hub.
> 
> But the problem is general so if we try to run lsusb or tools using
> libusb they will
> block until the kernel times out the device.
> 
> >
> > > One nice options would be if the timeout was configurable based on
> > > port/devpath, I can understand 30-60 seconds timeout for a spindel USB
> > > disk, but for other devices 5 seconds would be more than enough to
> > > conclude they are dead, but that's nice to have.
> >
> > The timeouts the kernel has now have had to be increased over time due
> > to bad devices.  And we don't know what type of device this is until we
> > read from it, so you can't pick the timeout based on the type of device
> > if you can't read the type of device :)
> >
> 
> 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.

> > > > > Would there be any reasoning against just serving the descriptors file
> > > > > as it looked before inserting the broken USB device instead of
> > > > > blocking the read?
> > > >
> > > > So a different device's descriptor file is being blocked by a broken
> > > > device?  Are you sure?  They should all be independent.
> > >
> > > Not the descriptor file, but the descriptor*s* under the hub folder in sysfs:
> > >
> > > >From https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb :
> > >
> > > What: /sys/bus/usb/devices/usbX/descriptors
> > > Description:
> > > Contains the interface descriptors, in binary.
> > >
> > > As I understand it, this is a file that contains all the information for devices
> > > under a hub.
> >
> > No, just the single device's descriptor.
> 
> 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.

> The other files in /sys/bus/usb/devices/3-3/ such as idProduct, idVendor, etc.
> do not block.

Because they rely on values stored in the kernel's copy of the device 
descriptor, which does not get reallocated during the lifetime of the 
device.

> This is what I get when the hardware is in broken state:
> 
> strace lsusb
> ...
> openat(AT_FDCWD, "/sys/bus/usb/devices/3-3/descriptors", O_RDONLY|O_CLOEXEC) = 8
> read(8,
> ...
> 
> dmesg -wH
> [  +2.528009] usb 3-3.6: new high-speed USB device number 20 using xhci_hcd
> [  +5.300072] usb 3-3.6: device descriptor read/64, error -110
> [ +15.615955] usb 3-3.6: device descriptor read/64, error -110
> [  +0.187975] usb 3-3.6: new high-speed USB device number 21 using xhci_hcd
> [  +5.188069] usb 3-3.6: device descriptor read/64, error -110
> [ +15.615973] usb 3-3.6: device descriptor read/64, error -110
> [  +0.112158] usb 3-3-port6: attempt power cycle
> [  +0.603789] usb 3-3.6: new high-speed USB device number 22 using xhci_hcd
> [Jan26 13:33] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> device command
> [  +5.375996] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> device command
> [  +0.207876] usb 3-3.6: device not accepting address 22, error -62
> [  +0.080030] usb 3-3.6: new high-speed USB device number 23 using xhci_hcd
> [  +5.088086] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> device command
> [  +5.376001] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> device command
> [  +0.207889] usb 3-3.6: device not accepting address 23, error -62
> [  +0.002418] usb 3-3-port6: unable to enumerate USB device
> 
> I tried the other devices attached to the hub and their descriptors do
> not block on
> read, only the hubs.

Because the other children are not locked while the new device is being 
probed.  Only the parent hub and the new device itself are locked.

Alan Stern

> > > Most likely an optimization made for lsusb and libusb so they
> > > don't have to traverse all the individual folders to get the same information.
> >
> > Nope, libusb walks the whole bus if you ask it to.
> >
> > > The problem is that /sys/bus/usb/devices/usbX/descriptors block while the
> > > kernel is trying to do the descriptor read on that one broken device even if
> > > all the other devices on the hub is well behaving the read is blocked until
> > > it times out.
> >
> > So if you read one descriptor, it will time out for others if you do so
> > at the same time?  Do they all share a single kernel lock perhaps?
> >
> > > If lsusb and libusb had traversed the folder structure they would not have
> > > blocked as the broken devices never showed in the folder structure:
> > >
> > > fx.
> > >
> > > tlb@tlb-nuc:/sys/bus/usb/devices/usb3$ ls -1
> > > 3-0:1.0 # Device shows up when USB initialization is done
> > > 3-10 # Device shows up when USB initialization is done
> > > 3-3 # Device shows up when USB initialization is done
> > > ...
> > > descriptors # Blocks on read until kernel times out waiting for descriptor read.
> > > ...
> > >
> > > > > And if we wanted to create a pull request for a change like that would
> > > > > it be accepted or would it be considered breaking the API?
> > > >
> > > > It depends on what the kernel change looks like.  Have you tried
> > > > anything that worked for you that you wish to propose?
> > > >
> > >
> > > I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.
> >
> > Patches gladly reviewed to do so :)
> 
> We will have a look and get back to you.
> 
> >
> > thanks,
> >
> > greg k-h
> 
> Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

On Thu, 26 Jan 2023 at 17:17, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Jan 26, 2023 at 02:59:35PM +0100, Troels Liebe Bentsen wrote:
> > On Thu, 26 Jan 2023 at 14:12, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jan 26, 2023 at 02:06:39PM +0100, Troels Liebe Bentsen wrote:
> > > > Hi Greg,
> > > >
> > > > On Thu, 26 Jan 2023 at 13:23, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, Jan 26, 2023 at 12:49:32PM +0100, Troels Liebe Bentsen wrote:
> > > > > > Hi,
> > > > > >
> > > > > > We have a hardware projekt where something is off with power ON
> > > > > > timing. It sometimes gets started in a broken state where the device
> > > > > > is seen by the USB system but does not respond to descriptor reads.
> > > > >
> > > > > Ah, a broken USB device, not much the kernel can do about that :(
> > > >
> > > > Would be nice if it could, but I settle for papering over the worst parts  :)
> > > >
> > > > >
> > > > > > When this happens this causes lsusb and libusb based tools to hang
> > > > > > until the descriptor read in the USB subsystem timeout out after 30
> > > > > > seconds or so. It looks like the tools are trying to read
> > > > > > /sys/bus/usb/devices/.../descriptors and it blocks until the timeout
> > > > > > happens.
> > > > > >
> > > > > > We should fix our hardware and have done so in the next revision but
> > > > > > why should one device be able to block the descriptors file that most
> > > > > > user land USB code seem to use.
> > > > >
> > > > > If the device does not respond, what is the kernel or userspace supposed
> > > > > to do?
> > > >
> > > > Might not have been clear that the issue is when I "plug in"/connect the
> > > > device that it happens. I think the kernel is doing the right thing here and
> > > > just timing out the device and at least in the kernel it does not seem to block
> > > > other devices from doing their thing. The problem is that part of the userspace
> > > > interface used for listing all devices on a hub block until the timeout
> > > > happens.
> > >
> > > What userspace code exactly is doing this?  Perhaps just work on making
> > > that tool handle timeouts better?
> >
> > We are using libusb to drive an automated flashing setup to test our hardware
> > but libusb_get_device_list blocks as it reads
> > /sys/bus/usb/devices/usbX/descriptors
> > for the hub the broken hardware is attached to. The hang happens when we
> > put the device into flash mode(by setting some flashing pins) where the MCU
> > has some low level USB function where we can load u-boot over a
> > proprietary protocol but for some reason does not enter fully into this state.
> >
> > So we have workaround code to see if the hardware has blocked the descriptors
> > file of the hub and then power cycle the device until it starts
> > behaving and set a
> > global mutex that everything else attached to that hub has to wait as we can't
> > scan the hub.
> >
> > But the problem is general so if we try to run lsusb or tools using
> > libusb they will
> > block until the kernel times out the device.
> >
> > >
> > > > One nice options would be if the timeout was configurable based on
> > > > port/devpath, I can understand 30-60 seconds timeout for a spindel USB
> > > > disk, but for other devices 5 seconds would be more than enough to
> > > > conclude they are dead, but that's nice to have.
> > >
> > > The timeouts the kernel has now have had to be increased over time due
> > > to bad devices.  And we don't know what type of device this is until we
> > > read from it, so you can't pick the timeout based on the type of device
> > > if you can't read the type of device :)
> > >
> >
> > 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?
Using Ubuntu mainline 6.1.4 and it does not seem to have this file in the
ports folder.

>
> > > > > > Would there be any reasoning against just serving the descriptors file
> > > > > > as it looked before inserting the broken USB device instead of
> > > > > > blocking the read?
> > > > >
> > > > > So a different device's descriptor file is being blocked by a broken
> > > > > device?  Are you sure?  They should all be independent.
> > > >
> > > > Not the descriptor file, but the descriptor*s* under the hub folder in sysfs:
> > > >
> > > > >From https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-usb :
> > > >
> > > > What: /sys/bus/usb/devices/usbX/descriptors
> > > > Description:
> > > > Contains the interface descriptors, in binary.
> > > >
> > > > As I understand it, this is a file that contains all the information for devices
> > > > under a hub.
> > >
> > > No, just the single device's descriptor.
> >
> > 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?

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.

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?

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?

> > The other files in /sys/bus/usb/devices/3-3/ such as idProduct, idVendor, etc.
> > do not block.
>
> Because they rely on values stored in the kernel's copy of the device
> descriptor, which does not get reallocated during the lifetime of the
> device.
>
> > This is what I get when the hardware is in broken state:
> >
> > strace lsusb
> > ...
> > openat(AT_FDCWD, "/sys/bus/usb/devices/3-3/descriptors", O_RDONLY|O_CLOEXEC) = 8
> > read(8,
> > ...
> >
> > dmesg -wH
> > [  +2.528009] usb 3-3.6: new high-speed USB device number 20 using xhci_hcd
> > [  +5.300072] usb 3-3.6: device descriptor read/64, error -110
> > [ +15.615955] usb 3-3.6: device descriptor read/64, error -110
> > [  +0.187975] usb 3-3.6: new high-speed USB device number 21 using xhci_hcd
> > [  +5.188069] usb 3-3.6: device descriptor read/64, error -110
> > [ +15.615973] usb 3-3.6: device descriptor read/64, error -110
> > [  +0.112158] usb 3-3-port6: attempt power cycle
> > [  +0.603789] usb 3-3.6: new high-speed USB device number 22 using xhci_hcd
> > [Jan26 13:33] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> > device command
> > [  +5.375996] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> > device command
> > [  +0.207876] usb 3-3.6: device not accepting address 22, error -62
> > [  +0.080030] usb 3-3.6: new high-speed USB device number 23 using xhci_hcd
> > [  +5.088086] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> > device command
> > [  +5.376001] xhci_hcd 0000:00:14.0: Timeout while waiting for setup
> > device command
> > [  +0.207889] usb 3-3.6: device not accepting address 23, error -62
> > [  +0.002418] usb 3-3-port6: unable to enumerate USB device
> >
> > I tried the other devices attached to the hub and their descriptors do
> > not block on
> > read, only the hubs.
>
> Because the other children are not locked while the new device is being
> probed.  Only the parent hub and the new device itself are locked.
>
> Alan Stern
>
> > > > Most likely an optimization made for lsusb and libusb so they
> > > > don't have to traverse all the individual folders to get the same information.
> > >
> > > Nope, libusb walks the whole bus if you ask it to.
> > >
> > > > The problem is that /sys/bus/usb/devices/usbX/descriptors block while the
> > > > kernel is trying to do the descriptor read on that one broken device even if
> > > > all the other devices on the hub is well behaving the read is blocked until
> > > > it times out.
> > >
> > > So if you read one descriptor, it will time out for others if you do so
> > > at the same time?  Do they all share a single kernel lock perhaps?
> > >
> > > > If lsusb and libusb had traversed the folder structure they would not have
> > > > blocked as the broken devices never showed in the folder structure:
> > > >
> > > > fx.
> > > >
> > > > tlb@tlb-nuc:/sys/bus/usb/devices/usb3$ ls -1
> > > > 3-0:1.0 # Device shows up when USB initialization is done
> > > > 3-10 # Device shows up when USB initialization is done
> > > > 3-3 # Device shows up when USB initialization is done
> > > > ...
> > > > descriptors # Blocks on read until kernel times out waiting for descriptor read.
> > > > ...
> > > >
> > > > > > And if we wanted to create a pull request for a change like that would
> > > > > > it be accepted or would it be considered breaking the API?
> > > > >
> > > > > It depends on what the kernel change looks like.  Have you tried
> > > > > anything that worked for you that you wish to propose?
> > > > >
> > > >
> > > > I would like to change /sys/bus/usb/devices/usbX/descriptors so it never blocks.
> > >
> > > Patches gladly reviewed to do so :)
> >
> > We will have a look and get back to you.
> >
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Regards Troels

Also in general thanks for the quick and detailed responses, it's a
pleasure writing on this mailing list.

Regards Troels

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

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@rowland.harvard.edu> 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.

> 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;
 }
 

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@rowland.harvard.edu> 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@rowland.harvard.edu> 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;
>  }
>
>

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Tue, Jan 31, 2023 at 04:59:36PM +0100, Troels Liebe Bentsen wrote:
> On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@rowland.harvard.edu> wrote:
> > 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?

I don't know.  I will submit it for the -stable kernels, but the 
decision on whether to accept it will be up to Greg KH.

Alan Stern

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Greg KH]

On Tue, Jan 31, 2023 at 03:41:08PM -0500, Alan Stern wrote:
> On Tue, Jan 31, 2023 at 04:59:36PM +0100, Troels Liebe Bentsen wrote:
> > On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > 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?
> 
> I don't know.  I will submit it for the -stable kernels, but the 
> decision on whether to accept it will be up to Greg KH.

I'll backport it, as it can help out with systems as Troels said.  But
will wait until 6.3-rc1 is out as this should get some testing.

thanks,

greg k-h

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

Hi again,

Did a bit more testing and found another lock that would be nice to remove,
the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:

Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub

strace lsusb -v
...
openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...

The openat can not be canceled from userspace and even kill -9 won't stop
the process until the descriptor read times out.

Regards Troels

On Tue, 31 Jan 2023 at 21:56, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 31, 2023 at 03:41:08PM -0500, Alan Stern wrote:
> > On Tue, Jan 31, 2023 at 04:59:36PM +0100, Troels Liebe Bentsen wrote:
> > > On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > 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?
> >
> > I don't know.  I will submit it for the -stable kernels, but the
> > decision on whether to accept it will be up to Greg KH.
>
> I'll backport it, as it can help out with systems as Troels said.  But
> will wait until 6.3-rc1 is out as this should get some testing.
>
> thanks,
>
> greg k-h

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

Also managed to get "lsusb -v" to hang in an unkillable way even after
I unplugged the device and hub:

) = 1 ([{fd=5, revents=POLLIN}])
ioctl(8, USBDEVFS_DISCARDURB

Regards Troels

On Tue, 7 Feb 2023 at 09:25, Troels Liebe Bentsen
<troels@connectedcars.dk> wrote:
>
> Hi again,
>
> Did a bit more testing and found another lock that would be nice to remove,
> the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:
>
> Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub
>
> strace lsusb -v
> ...
> openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...
>
> The openat can not be canceled from userspace and even kill -9 won't stop
> the process until the descriptor read times out.
>
> Regards Troels
>
> On Tue, 31 Jan 2023 at 21:56, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jan 31, 2023 at 03:41:08PM -0500, Alan Stern wrote:
> > > On Tue, Jan 31, 2023 at 04:59:36PM +0100, Troels Liebe Bentsen wrote:
> > > > On Fri, 27 Jan 2023 at 17:07, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > 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?
> > >
> > > I don't know.  I will submit it for the -stable kernels, but the
> > > decision on whether to accept it will be up to Greg KH.
> >
> > I'll backport it, as it can help out with systems as Troels said.  But
> > will wait until 6.3-rc1 is out as this should get some testing.
> >
> > thanks,
> >
> > greg k-h

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Tue, Feb 07, 2023 at 09:25:55AM +0100, Troels Liebe Bentsen wrote:
> Hi again,
> 
> Did a bit more testing and found another lock that would be nice to remove,
> the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:
> 
> Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub
> 
> strace lsusb -v
> ...
> openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...
> 
> The openat can not be canceled from userspace and even kill -9 won't stop
> the process until the descriptor read times out.

Yes, that really should be an interruptible lock.  In fact, all the 
locks connected with usbfs should be interruptible.

However, it can't be eliminated entirely.  This is a case where two 
things end up being mutually exclusive with each other because they both 
need to be mutually exclusive with a third thing.  In other words, 
opening a hub's usbfs file and probing a hub's children both have to be 
mutually exclusive with disconnecting the hub.  The three pathways all 
use the device lock for this purpose, so they are all mutually exclusive 
with each other.

> Also managed to get "lsusb -v" to hang in an unkillable way even after
> I unplugged the device and hub:
>
> ) = 1 ([{fd=5, revents=POLLIN}])
> ioctl(8, USBDEVFS_DISCARDURB

Making these lock calls interruptible should fix this problem, right?

Alan Stern

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Tue, Feb 07, 2023 at 12:52:16PM -0500, Alan Stern wrote:
> On Tue, Feb 07, 2023 at 09:25:55AM +0100, Troels Liebe Bentsen wrote:
> > Hi again,
> > 
> > Did a bit more testing and found another lock that would be nice to remove,
> > the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:
> > 
> > Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub
> > 
> > strace lsusb -v
> > ...
> > openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...
> > 
> > The openat can not be canceled from userspace and even kill -9 won't stop
> > the process until the descriptor read times out.
> 
> Yes, that really should be an interruptible lock.  In fact, all the 
> locks connected with usbfs should be interruptible.
> 
> However, it can't be eliminated entirely.  This is a case where two 
> things end up being mutually exclusive with each other because they both 
> need to be mutually exclusive with a third thing.  In other words, 
> opening a hub's usbfs file and probing a hub's children both have to be 
> mutually exclusive with disconnecting the hub.  The three pathways all 
> use the device lock for this purpose, so they are all mutually exclusive 
> with each other.
> 
> > Also managed to get "lsusb -v" to hang in an unkillable way even after
> > I unplugged the device and hub:
> >
> > ) = 1 ([{fd=5, revents=POLLIN}])
> > ioctl(8, USBDEVFS_DISCARDURB
> 
> Making these lock calls interruptible should fix this problem, right?

Here's a patch.  It should fix most of these problems.

Alan Stern



Index: usb-devel/drivers/usb/core/devio.c
===================================================================
--- usb-devel.orig/drivers/usb/core/devio.c
+++ usb-devel/drivers/usb/core/devio.c
@@ -303,13 +303,15 @@ static ssize_t usbdev_read(struct file *
 {
 	struct usb_dev_state *ps = file->private_data;
 	struct usb_device *dev = ps->dev;
-	ssize_t ret = 0;
+	ssize_t ret;
 	unsigned len;
 	loff_t pos;
 	int i;
 
 	pos = *ppos;
-	usb_lock_device(dev);
+	ret = usb_lock_device_interruptible(dev);
+	if (ret < 0)
+		return ret;
 	if (!connected(ps)) {
 		ret = -ENODEV;
 		goto err;
@@ -1039,7 +1041,9 @@ static int usbdev_open(struct inode *ino
 	if (!dev)
 		goto out_free_ps;
 
-	usb_lock_device(dev);
+	ret = usb_lock_device_interruptible(dev);
+	if (ret < 0)
+		goto out_put_device;
 	if (dev->state == USB_STATE_NOTATTACHED)
 		goto out_unlock_device;
 
@@ -1071,6 +1075,7 @@ static int usbdev_open(struct inode *ino
 
  out_unlock_device:
 	usb_unlock_device(dev);
+ out_put_device:
 	usb_put_dev(dev);
  out_free_ps:
 	kfree(ps);
@@ -2591,14 +2596,17 @@ static long usbdev_do_ioctl(struct file
 	struct usb_dev_state *ps = file->private_data;
 	struct inode *inode = file_inode(file);
 	struct usb_device *dev = ps->dev;
-	int ret = -ENOTTY;
+	int ret;
 
 	if (!(file->f_mode & FMODE_WRITE))
 		return -EPERM;
 
-	usb_lock_device(dev);
+	ret = usb_lock_device_interruptible(dev);
+	if (ret < 0)
+		return ret;
 
 	/* Reap operations are allowed even after disconnection */
+	ret = -ENOTTY;
 	switch (cmd) {
 	case USBDEVFS_REAPURB:
 		snoop(&dev->dev, "%s: REAPURB\n", __func__);

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Troels Liebe Bentsen]

On Wed, 8 Feb 2023 at 17:48, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Feb 07, 2023 at 12:52:16PM -0500, Alan Stern wrote:
> > On Tue, Feb 07, 2023 at 09:25:55AM +0100, Troels Liebe Bentsen wrote:
> > > Hi again,
> > >
> > > Did a bit more testing and found another lock that would be nice to remove,
> > > the /dev/bus/usb/$BUS/$DEV file for the hub is also locked:
> > >
> > > Bus 003 Device 016: ID 1a40:0201 Terminus Technology Inc. FE 2.1 7-port Hub
> > >
> > > strace lsusb -v
> > > ...
> > > openat(AT_FDCWD, "/dev/bus/usb/003/016", O_RDWR|O_CLOEXEC...
> > >
> > > The openat can not be canceled from userspace and even kill -9 won't stop
> > > the process until the descriptor read times out.
> >
> > Yes, that really should be an interruptible lock.  In fact, all the
> > locks connected with usbfs should be interruptible.
> >
> > However, it can't be eliminated entirely.  This is a case where two
> > things end up being mutually exclusive with each other because they both
> > need to be mutually exclusive with a third thing.  In other words,
> > opening a hub's usbfs file and probing a hub's children both have to be
> > mutually exclusive with disconnecting the hub.  The three pathways all
> > use the device lock for this purpose, so they are all mutually exclusive
> > with each other.
> >
> > > Also managed to get "lsusb -v" to hang in an unkillable way even after
> > > I unplugged the device and hub:
> > >
> > > ) = 1 ([{fd=5, revents=POLLIN}])
> > > ioctl(8, USBDEVFS_DISCARDURB
> >
> > Making these lock calls interruptible should fix this problem, right?
>
> Here's a patch.  It should fix most of these problems.
>
> Alan Stern
>
Thanks, it will give the patches a try.

But I guess it still means that "lsusb -v" or other tools that try to
read the hub's usbfs file will block until the child device's descriptor read
has timed out as something in that code path takes the hub's device lock.

I tried following the code path up from the read descriptors error and it
looks like locking is done on port level with usb_lock_port until we hit
hub_event where usb_lock_device(hdev) is taken, being new to this
code base I'm not sure it's the same mutex we locked on in
read_descriptors, but do we need to hold it until the end of the
function or is the hub device lock taken somewhere else?

As a little side note we found a rather ugly workaround where we cp
the sysfs read by libusb to /tmp and bind mount them back to sysfs
and remove the read permission from the hubs usbfs file, this
effectively means our testing software won't get disturbed by
descriptor read timeouts happening on other ports.

The way we get the device out of the broken state is by power
cycling it rapidly until it boots up in flashing mode, it normally takes a
couple of seconds and after that we can remove the flashing
pins that cause it to sometimes go into the broken state.

Regards Troels
>
> Index: usb-devel/drivers/usb/core/devio.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/devio.c
> +++ usb-devel/drivers/usb/core/devio.c
> @@ -303,13 +303,15 @@ static ssize_t usbdev_read(struct file *
>  {
>         struct usb_dev_state *ps = file->private_data;
>         struct usb_device *dev = ps->dev;
> -       ssize_t ret = 0;
> +       ssize_t ret;
>         unsigned len;
>         loff_t pos;
>         int i;
>
>         pos = *ppos;
> -       usb_lock_device(dev);
> +       ret = usb_lock_device_interruptible(dev);
> +       if (ret < 0)
> +               return ret;
>         if (!connected(ps)) {
>                 ret = -ENODEV;
>                 goto err;
> @@ -1039,7 +1041,9 @@ static int usbdev_open(struct inode *ino
>         if (!dev)
>                 goto out_free_ps;
>
> -       usb_lock_device(dev);
> +       ret = usb_lock_device_interruptible(dev);
> +       if (ret < 0)
> +               goto out_put_device;
>         if (dev->state == USB_STATE_NOTATTACHED)
>                 goto out_unlock_device;
>
> @@ -1071,6 +1075,7 @@ static int usbdev_open(struct inode *ino
>
>   out_unlock_device:
>         usb_unlock_device(dev);
> + out_put_device:
>         usb_put_dev(dev);
>   out_free_ps:
>         kfree(ps);
> @@ -2591,14 +2596,17 @@ static long usbdev_do_ioctl(struct file
>         struct usb_dev_state *ps = file->private_data;
>         struct inode *inode = file_inode(file);
>         struct usb_device *dev = ps->dev;
> -       int ret = -ENOTTY;
> +       int ret;
>
>         if (!(file->f_mode & FMODE_WRITE))
>                 return -EPERM;
>
> -       usb_lock_device(dev);
> +       ret = usb_lock_device_interruptible(dev);
> +       if (ret < 0)
> +               return ret;
>
>         /* Reap operations are allowed even after disconnection */
> +       ret = -ENOTTY;
>         switch (cmd) {
>         case USBDEVFS_REAPURB:
>                 snoop(&dev->dev, "%s: REAPURB\n", __func__);
>

Re: All USB tools hang when one descriptor read fails and needs to timeout

[Alan Stern]

On Wed, Feb 08, 2023 at 09:46:01PM +0100, Troels Liebe Bentsen wrote:
> On Wed, 8 Feb 2023 at 17:48, Alan Stern <stern@rowland.harvard.edu> wrote:
> > Here's a patch.  It should fix most of these problems.
> >
> > Alan Stern
> >
> Thanks, it will give the patches a try.
> 
> But I guess it still means that "lsusb -v" or other tools that try to
> read the hub's usbfs file will block until the child device's descriptor read
> has timed out as something in that code path takes the hub's device lock.

That's right.  However, at least you'll be able to kill the process if 
you have to.

> I tried following the code path up from the read descriptors error and it
> looks like locking is done on port level with usb_lock_port until we hit
> hub_event where usb_lock_device(hdev) is taken, being new to this
> code base I'm not sure it's the same mutex we locked on in
> read_descriptors, but do we need to hold it until the end of the
> function or is the hub device lock taken somewhere else?

It is the same mutex.  And yes, it needs to be held until the end of the 
function.

Alan Stern