Hi,
On 07-06-16 17:18, Alan Stern wrote:
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?
That is a good question, right now not so useful, but given how
simple your proposed fix is, I think it is worthwhile to fix
this, so that in the future we can maybe simplify libusb a bit ?
And it might also be useful for a libusb replacement which is
something which some people have been discussing (not me, and
I do not remember the details).
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?
The proposed fix looks good to me and is surprisingly simple :)
Regards,
Hans
--
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
