hid2hci: iterate libusb devices twice

After the previous cleanup submitted to hid2hci, a race condition has been exposed with regard to libusb. Even though we know the bus and devnum are existing, the call to usb_find_devices isn't necessarily finding it on the first time after the device is introduced to the bus by the BT killswitch. I'm not too keen upon iterating something twice as a solution, it does solve the problem until a method is introduced in usb_device_open_from_udev that doesn't require libusb's iteration of the bus in the first place.

Quoted 8 lines

After the previous cleanup submitted to hid2hci, a race condition has been exposed with regard to libusb.  Even though we know the bus and devnum are existing, the call to usb_find_devices isn't necessarily finding it on the first time after the device is introduced to the bus by the BT killswitch.  I'm not too keen upon iterating something twice as a solution, it does solve the problem until a method is introduced in usb_device_open_from_udev that doesn't require libusb's iteration of the bus in the first place.

then the usage of libusb needs to be removed. Read Kay's statement about libusb being utterly stupid.

On Mon, Jul 27, 2009 at 23:32, Marcel Holtmann<[email protected]> wrote:

Now it's getting funny. We need to call the nonsense two times to make it work? We need to fix the real issue here instead of doing guesswork and adding hacks like this. Any idea what's going on with the first scan? The device node is guaranteed to exist when we call stuff from RUN+= instructions.

Well, libusb might be ok for stuff that just searches things which are always there, it's definitely not suitable to be used in conjunction with udev. The entire coldplug with 500 devices takes ~0.5 seconds here, while a single call to libusb takes ~0.15 just to find the device we already have. That alone is not acceptable, and now we should call the useless scan twice? Tsss ... :)

On Mon, Jul 27, 2009 at 23:32, Marcel Holtmann<[email protected]> wrote:

Now it's getting funny. We need to call the nonsense two times to make it work? We need to fix the real issue here instead of doing guesswork and adding hacks like this. Any idea what's going on with the first scan? The device node is guaranteed to exist when we call stuff from RUN+= instructions.

Quoted 7 lines

Now it's getting funny. We need to call the nonsense two times to make it work? We need to fix the real issue here instead of doing guesswork and adding hacks like this. Any idea what's going on with the first scan? The device node is guaranteed to exist when we call stuff from RUN+= instructions.

Are you absolutely positive that the device node will exist and be usable by the time something is called from RUN? I've been enabling/adding debugging code around libusb, and finally got down to the point that when it tries to do > an open( ) on the device node, here's what's happening:

    Jul 29 11:03:25 test-laptop udevd-work[20561]: '/lib/udev/hid2hci'
    (stderr) 'USB error: failed to open /dev/bus/usb/003/061: No such
    file or directory'

This of course causes failures later because of a bad file descriptor:

    Jul 29 11:03:25 test-laptop udevd-work[20561]: '/lib/udev/hid2hci'
    (stderr) 'USB error: could not detach kernel driver from interface
    0: Bad file descriptor'
    Jul 29 11:03:25 test-laptop udevd-work[20561]: '/lib/udev/hid2hci'
    (stderr) 'USB error: could not claim interface 0: Bad file descriptor'

Quoted 8 lines · Marcel Holtmann

Now it's getting funny. We need to call the nonsense two times to make it work? We need to fix the real issue here instead of doing guesswork and adding hacks like this. Any idea what's going on with the first scan? The device node is guaranteed to exist when we call stuff from RUN+= instructions.

Are you absolutely positive that the device node will exist and be usable by the time something is called from RUN?  I've been enabling/adding debugging code around libusb, and finally got down to the point that when it tries to do an open( ) on the device node, here's what's happening:

   Jul 29 11:03:25 test-laptop udevd-work[20561]: '/lib/udev/hid2hci'

   (stderr) 'USB error: failed to open /dev/bus/usb/003/061: No such file or directory'

Yeah, I'm sure. The thing is that the kernel USB code for some weird reason returns -ENOENT instead -ENODEV -- hence the misleading error string.

Alan, I suspect (haven't checked) that at the time we get the event for the usb_device, the device is not ready to be handled by usbfs operations. Is that likely? And if, any idea how, and until which point, to delay the usb_device uevent, in which we hook in to do the usbfs actions from userspace?

Quoted 3 lines · Kay Sievers

Yeah, I'm sure. The thing is that the kernel USB code for some weird reason returns -ENOENT instead -ENODEV -- hence the misleading error string.

That return code can easily be changed.

Quoted 5 lines

Alan, I suspect (haven't checked) that at the time we get the event for the usb_device, the device is not ready to be handled by usbfs operations. Is that likely? And if, any idea how, and until which point, to delay the usb_device uevent, in which we hook in to do the usbfs actions from userspace?

There are two aspects we have to consider: usbfs device nodes (/dev/bus/usb/*/*) and usbfs file nodes (/proc/bus/usb/*/*).  The device nodes are created by udev whereas the file nodes are created by the kernel (if CONFIG_USB_DEVICEFS is enabled).  Nowadays most people use the device nodes, but embedded systems without udev may want to rely on the file nodes.

With the device nodes, the problem is this: usbdev_open() looks up dev_t values by calling bus_find_device().  But devices are added to the bus's list by bus_attach_device(), which isn't called until after the KOBJ_ADD uevent is sent.  Hence there's a window after the uevent is issued and before the device can be looked up on the bus.  That's probably what's happening here.

Interchanging bus_attach_device() and kobject_uevent() in device_add() would solve this problem, but it would introduce other problems.  In particular, if a probe routine registered a child device then the child's uevent would be sent before the parent's!

You know, the real problem may lie in bus_attach_device().  If probing fails then it doesn't add the device to the bus -- and it doesn't even return an error code when this happens.  One can argue that the device should _always_ be added to the bus, regardless of whether a suitable driver can be found.  If you accept that argument then linking the device into the bus's list should be moved up into bus_add_device(), which is called before the uevent.  Indeed, each of those two routines has a comment in the kerneldoc saying "Add the device to its bus's list of devices."  They can't both be right!

There's also a problem with usbfs file nodes.  The file nodes are created by the device's probe routine.  So again, if the uevent comes before probing then it will come before the file nodes exist.  I suppose we could create the file nodes earlier, before the device is registered, but is this really a good idea?

Quoted 9 lines · Alan Stern

You know, the real problem may lie in bus_attach_device().  If probing fails then it doesn't add the device to the bus -- and it doesn't even return an error code when this happens.  One can argue that the device should _always_ be added to the bus, regardless of whether a suitable driver can be found.  If you accept that argument then linking the device into the bus's list should be moved up into bus_add_device(), which is called before the uevent.  Indeed, each of those two routines has a comment in the kerneldoc saying "Add the device to its bus's list of devices."  They can't both be right!

That sounds like the right thing. We create all the links which indicate a membership at the bus, so we should put it to the bus at the same time.

Quoted 7 lines · Alan Stern

Quoted 3 lines · Kay Sievers

Yeah, I'm sure. The thing is that the kernel USB code for some weird reason returns -ENOENT instead -ENODEV -- hence the misleading error string.

That return code can easily be changed.

If we don't confuse anything, I guess that would be better, as it's really about a device not being there, not a file.

Quoted 9 lines

You know, the real problem may lie in bus_attach_device().  If probing fails then it doesn't add the device to the bus -- and it doesn't even return an error code when this happens.  One can argue that the device should _always_ be added to the bus, regardless of whether a suitable driver can be found.  If you accept that argument then linking the device into the bus's list should be moved up into bus_add_device(), which is called before the uevent.  Indeed, each of those two routines has a comment in the kerneldoc saying "Add the device to its bus's list of devices."  They can't both be right!

That sounds like the right thing. We create all the links which indicate a membership at the bus, so we should put it to the bus at the same time.

Quoted 5 lines

There's also a problem with usbfs file nodes.  The file nodes are created by the device's probe routine.  So again, if the uevent comes before probing then it will come before the file nodes exist.  I suppose we could create the file nodes earlier, before the device is registered, but is this really a good idea?

Oh, I don't think we really need to care about the proc files and the uevent timing. Users of the proc files usually don't hook into the events but poll the devices file. The event timing and the proc files have been broken since the first day, and you always needed to loop until the proc file showed up.

Also, we expect to get a devtmpfs which will create the /dev/bus/usb/ nodes from inside the kernel, so the proc things can be ignored for almost all use cases after that.

On Thu, 30 Jul 2009, Kay Sievers wrote:

Try the patch below. It should fix the problem.