On Wed, 8 Feb 2023 at 17:48, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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__);
>
