On Sun, Jan 1, 2012 at 12:01 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Sat, 31 Dec 2011, David Goodenough wrote:
>
>> > It appears that the bug was triggered by the udev rule containing the
>> > new_id attribute, but exactly what went wrong is not clear -- at least,
>> > not to me. You could try commenting out that rule and see if that
>> > helps.
>> >
>> > Is this bug fully repeatable?
>> Yes and no. It only happens when I insert the USB cable from the
>> BeagleBone, but it does not happen every time - sounds like a timing
>> problem to me.
>
> To me too.
>
>> The host I am using is an elderly (and therefore not
>> very fast) laptop, the processor is not SMP.
>>
>> There are 4 rules:-
>>
>> ACTION=="add", SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_interface",
>> ATTRS{idVendor}=="0403", ATTRS{idProduct}=="a6d0", DRIVER=="",
>> RUN+="/sbin/modprobe -b ftdi_sio"
>>
>> ACTION=="add", SUBSYSTEM=="drivers", ENV{DEVPATH}=="/bus/usb-
>> serial/drivers/ftdi_sio", ATTR{new_id}="0403 a6d0"
>>
>> ACTION=="add", KERNEL=="ttyUSB*", ATTRS{interface}=="BeagleBone",
>> ATTRS{bInterfaceNumber}=="00", SYMLINK+="beaglebone-jtag"
>>
>> ACTION=="add", KERNEL=="ttyUSB*", ATTRS{interface}=="BeagleBone",
>> ATTRS{bInterfaceNumber}=="01", SYMLINK+="beaglebone-serial"
>>
>> I have tried it without the last two, assuming it was the symlink
>> that was causing the problem, but removing them does not cure the
>> problem.
>
> As I said above, the problem is triggered by the rule with the new_id
> attribute (the second rule).
>
>> The first two seem rather important to getting it to work. Certainly if I
>> remove the whole file then I do not get the problem, but I also can not
>> communicate with the serial port on the device.
>
> I believe I have figured out the problem. The patch below should take
> care of it. There's still a tiny race, but you're not likely to hit
> it. A fully correct fix will require some more work.
>
> Greg, the issue here is that usb_store_new_id() calls get_driver()
> before the driver in question has been registered. In principle this
> could also happen after the driver has been unregistered, although
> that's less likely.
>
Hi Alan,
Is any chance for user to trigger the window if the driver has been
unregistered? thanks.
> The underlying problem behind all this is in
> usb/serial/bus.c:store_new_id(). That's the store_new_id routine for
> the serial driver, and it calls usb_store_new_id() for the
> corresponding USB driver. However the USB driver isn't registered
> until after the serial driver (and it is unregistered before the serial
> driver). Thus there's a window during which get_driver() can get
> called at a bad time.
>
How about adding more codes into usb_store_new_id() to find out
whether the driver is registered while receiving such requests? and
this could give useful info to user space if the requests fail.
The patch may like this:
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -34,6 +34,20 @@
#ifdef CONFIG_HOTPLUG
+static bool is_drv_ready(struct device_driver *driver)
+{
+ struct device_driver *other = NULL;
+
+ if (driver && driver->name && driver->bus)
+ other = driver_find(driver->name, driver->bus);
+ if (other == NULL)
+ return false;
+
+ put_driver(other);
+
+ return true;
+}
+
/*
* Adds a new dynamic USBdevice ID to this driver,
* and cause the driver to probe for all devices again.
@@ -48,6 +62,9 @@ ssize_t usb_store_new_id(struct usb_dynids *dynids,
int fields = 0;
int retval = 0;
+ if (!is_drv_ready(driver))
+ return -EAGAIN;
+
fields = sscanf(buf, "%x %x", &idVendor, &idProduct);
if (fields < 2)
return -EINVAL;
> In the end, it looks like the serial store_new_id() routine is not
> doing the right thing, because it accesses the USB driver without
> checking whether that driver is registered or even initialized.
> However the best way to fix it isn't obvious. What do you think?
>
> Alan Stern
>
>
> Index: usb-3.2/drivers/base/driver.c
> ===================================================================
> --- usb-3.2.orig/drivers/base/driver.c
> +++ usb-3.2/drivers/base/driver.c
> @@ -159,15 +159,9 @@ EXPORT_SYMBOL_GPL(driver_add_kobj);
> */
> struct device_driver *get_driver(struct device_driver *drv)
> {
> - if (drv) {
> - struct driver_private *priv;
> - struct kobject *kobj;
> -
> - kobj = kobject_get(&drv->p->kobj);
> - priv = to_driver(kobj);
> - return priv->driver;
> - }
> - return NULL;
> + if (!drv || !drv->p || !kobject_get(&drv->p->kobj))
> + drv = NULL;
> + return drv;
> }
> EXPORT_SYMBOL_GPL(get_driver);
>
>
> --
> 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
--
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
