On Tue, 7 Jun 2016, Hans de Goede wrote: > Hi, > > On 06-06-16 19:03, Alan Stern wrote: > > On Mon, 6 Jun 2016, Hans de Goede wrote: > > > >> Hi, > >> > >> On 06-06-16 16:48, Greg Kroah-Hartman wrote: > >>> On Mon, Jun 06, 2016 at 01:44:23PM +0200, Hans de Goede wrote: > >>>> Hi All, > >>>> > >>>> While looking at libusb today I ended up looking at the > >>>> reap-after-disconnect code. > >>>> > >>>> What stands out is that libusb expects to be able to > >>>> reap all outstanding urbs on a device on receiving > >>>> a POLL_ERR status from poll (on supported kernels). > >>>> > >>>> But the usbfs poll implementation will return POLL_ERR > >>>> as soon as ps->dev->state == USB_STATE_NOTATTACHED, > >>>> and the kernel usb code sets state = USB_STATE_NOTATTACHED > >>>> some time before usbdev_remove() gets called and moves > >>>> all pending urbs to the async_completed list. > > > > This means that URBs can't be reaped immediately. But it will be > > possible to reap them shortly after libusb gets the POLLERR status, > > once the khubd work routine has processed the disconnection. > > Ack. > > >>>> So if libusb ends up calling poll between the > >>>> setting state = USB_STATE_NOTATTACHED and usbdev_remove() > >>>> being called then it ends up not reaping all urbs > >>>> since it will stop monitoring the device after > >>>> the first POLL_ERR. > >>>> > >>>> I'll write a libusb fix for this, but I'm wondering if this > >>>> is something which we also want to fix on the kernel side? > > > > We could. Would it be worthwhile to do so? > > That is exactly the question this mail thread is intended to answer :) > > >>> Would fixing this in the kernel mean that libusb fixes are not needed? > >>> If so, I would recommend doing this. > >> > >> A kernel side fix is non trivial, whereas the libusb fix is trivial > >> since we also need to deal with kernels which do not support > >> reaping urbs after disconnect at all. > >> > >> Basically the question is, do we consider not being able to > >> reap all urbs after the first POLL_ERR a kernel bug, or should > >> userspace keep track of how many urbs it has outstanding and > >> call the reap ioctl (with some msleep after failure) until it > >> has reaped all urbs ? > > > > We already have POLLOUT to indicate when a completed URB is available > > for reaping. Can libusb use that? > > libusb already use that normally, but on a POLLERR it will remove > the fd from the pollfds since POLLERR will stay set once set, so > leaving it in pollfds means we will just set there busy-waiting. > > > Strictly speaking, POLLERR just indicates an error condition, of any > > kind. > > Ack, but the current semantics for how this works with usbfs are > somewhat sub-optimal from a libusb pov. The fix for this is easy > since libusb already has code in place to deal with older kernels > which do not support REAP after disconnect at all and I plan to > use that (rather then wait if we hit the race), but it does > make the whole REAP after disconnect function a whole lot > less useful. You also will have to handle kernels like the current one, that do support reap after disconnect but return POLLERR before the URBs are cancelled. Given that, how useful would changing the kernel be? We could alter usbdev_poll so that it returns POLLERR only after usbdev_remove has been called (say, only when ps->list is empty, ignoring ps->dev->state). That would be easy to do, like in the patch below. What do you think? Alan Stern Index: usb-4.x/drivers/usb/core/devio.c =================================================================== --- usb-4.x.orig/drivers/usb/core/devio.c +++ usb-4.x/drivers/usb/core/devio.c @@ -2580,8 +2580,10 @@ static unsigned int usbdev_poll(struct f poll_wait(file, &ps->wait, wait); if (file->f_mode & FMODE_WRITE && !list_empty(&ps->async_completed)) mask |= POLLOUT | POLLWRNORM; - if (!connected(ps)) - mask |= POLLERR | POLLHUP; + if (list_empty(&ps->list)) + mask |= POLLERR; + else if (!connected(ps)) + mask |= POLLHUP; return mask; } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
