| Message ID | ee32a093d43fe6631617c203ea7c86cb700ceac5.camel@hadess.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] USB: Fix device driver race | expand |
Hello! On 24.07.2020 11:59, Bastien Nocera wrote: > When a new device with a specialised device driver is plugged in, the > new driver will be modprobe()'d but the driver core will attach the > "generic" driver to the device. > > After that, nothing will trigger a reprobe when the modprobe()'d device > driver has finished initialising, as the device has the "generic" > driver attached to it. > > Trigger a reprobe ourselves when new specialised drivers get registered. > > Fixes: 88b7381a939d It's strange that nobody has noticed... you need to cite the commit summary here as well, enclosed into (""). > Signed-off-by: Bastien Nocera <hadess@hadess.net> [...] > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index f81606c6a35b..6731a8e104bc 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c [...] MBR, Sergei
On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> wrote: > > When a new device with a specialised device driver is plugged in, the > new driver will be modprobe()'d but the driver core will attach the > "generic" driver to the device. > > After that, nothing will trigger a reprobe when the modprobe()'d device > driver has finished initialising, as the device has the "generic" > driver attached to it. > > Trigger a reprobe ourselves when new specialised drivers get registered. ... > +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > +{ > + struct usb_device_driver *new_udriver = data; > + struct usb_device_driver *udriver; > + struct usb_device *udev; > + > + if (!dev->driver) > + return 0; > + > + udriver = to_usb_device_driver(dev->driver); > + if (udriver != &usb_generic_driver) > + return 0; What about static bool is_dev_usb_generic_driver(dev) { struct usb_device_driver *udd = dev->driver ? to_usb_device_driver(dev->driver) : NULL; return udd == &usb_generic_driver; } if (!is_dev_usb_generic_driver) return 0; ? > + udev = to_usb_device(dev); > + if (usb_device_match_id(udev, new_udriver->id_table) != NULL || > + (new_udriver->match && new_udriver->match(udev) == 0)) > + device_reprobe(dev); > + > + return 0; What about udev = to_usb_device(dev); if (usb_device_match_id(udev, new_udriver->id_table) == NULL) return 0; ? if (new_udriver->match && new_udriver->match(udev) == 0)) device_reprobe(dev); return 0; > +} ... > + /* Check whether any device could be better served with > + * this new driver > + */ Comment style? ... > + if (new_udriver->match || new_udriver->id_table) But match check is incorporated in the loop function. > + bus_for_each_dev(&usb_bus_type, NULL, new_udriver, > + __usb_bus_reprobe_drivers);
On Fri, Jul 24, 2020 at 1:32 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> wrote: ... > > +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > > +{ > > + struct usb_device_driver *new_udriver = data; > > + struct usb_device_driver *udriver; > > + struct usb_device *udev; > > + > > + if (!dev->driver) > > + return 0; > > + > > + udriver = to_usb_device_driver(dev->driver); > > + if (udriver != &usb_generic_driver) > > + return 0; > > What about > > static bool is_dev_usb_generic_driver(dev) > { > struct usb_device_driver *udd = dev->driver ? > to_usb_device_driver(dev->driver) : NULL; > > return udd == &usb_generic_driver; > } > > if (!is_dev_usb_generic_driver) > return 0; > > ? > > > + udev = to_usb_device(dev); > > + if (usb_device_match_id(udev, new_udriver->id_table) != NULL || > > + (new_udriver->match && new_udriver->match(udev) == 0)) > > + device_reprobe(dev); > > + > > + return 0; > > What about > > udev = to_usb_device(dev); > if (usb_device_match_id(udev, new_udriver->id_table) == NULL) > return 0; > ? > > if (new_udriver->match && new_udriver->match(udev) == 0)) > device_reprobe(dev); > return 0; > > > +} It actually sparks a lot of similarities with __check_usb_generic(). Perhaps you may unify them?
On Fri, 2020-07-24 at 13:18 +0300, Sergei Shtylyov wrote: > Hello! > > On 24.07.2020 11:59, Bastien Nocera wrote: > > > When a new device with a specialised device driver is plugged in, > > the > > new driver will be modprobe()'d but the driver core will attach the > > "generic" driver to the device. > > > > After that, nothing will trigger a reprobe when the modprobe()'d > > device > > driver has finished initialising, as the device has the "generic" > > driver attached to it. > > > > Trigger a reprobe ourselves when new specialised drivers get > > registered. > > > > Fixes: 88b7381a939d > > It's strange that nobody has noticed... you need to cite the > commit > summary here as well, enclosed into (""). Changed locally, thanks! > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > [...] > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > > index f81606c6a35b..6731a8e104bc 100644 > > --- a/drivers/usb/core/driver.c > > +++ b/drivers/usb/core/driver.c > [...] > > MBR, Sergei
On Fri, 2020-07-24 at 13:32 +0300, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> > wrote: > > When a new device with a specialised device driver is plugged in, > > the > > new driver will be modprobe()'d but the driver core will attach the > > "generic" driver to the device. > > > > After that, nothing will trigger a reprobe when the modprobe()'d > > device > > driver has finished initialising, as the device has the "generic" > > driver attached to it. > > > > Trigger a reprobe ourselves when new specialised drivers get > > registered. > > ... > > > +static int __usb_bus_reprobe_drivers(struct device *dev, void > > *data) > > +{ > > + struct usb_device_driver *new_udriver = data; > > + struct usb_device_driver *udriver; > > + struct usb_device *udev; > > + > > + if (!dev->driver) > > + return 0; > > + > > + udriver = to_usb_device_driver(dev->driver); > > + if (udriver != &usb_generic_driver) > > + return 0; > > What about > > static bool is_dev_usb_generic_driver(dev) > { > struct usb_device_driver *udd = dev->driver ? > to_usb_device_driver(dev->driver) : NULL; > > return udd == &usb_generic_driver; > } > > if (!is_dev_usb_generic_driver) > return 0; > > ? Great, done locally. > > + udev = to_usb_device(dev); > > + if (usb_device_match_id(udev, new_udriver->id_table) != > > NULL || > > + (new_udriver->match && new_udriver->match(udev) == 0)) > > + device_reprobe(dev); > > + > > + return 0; > > What about > > udev = to_usb_device(dev); > if (usb_device_match_id(udev, new_udriver->id_table) == NULL) > return 0; > ? > > if (new_udriver->match && new_udriver->match(udev) == 0)) > device_reprobe(dev); > return 0; That's not the same conditionals. If there's ->match but no matching ->id_table, you're not going to reprobe, when we'd want to. > > +} > > ... > > > + /* Check whether any device could be better served > > with > > + * this new driver > > + */ > > Comment style? Fixed locally. > ... > > > + if (new_udriver->match || new_udriver->id_table) > > But match check is incorporated in the loop function. It's ->match || ->id_table, not simply ->match. Drivers can have either, and we need to reprobe if either of those are present (and we're also checking in the function to avoid calling a NULL pointer ;) > > + bus_for_each_dev(&usb_bus_type, NULL, > > new_udriver, > > + __usb_bus_reprobe_drivers) > > ;
On Fri, 2020-07-24 at 13:39 +0300, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 1:32 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> > > wrote: > > ... > > > > +static int __usb_bus_reprobe_drivers(struct device *dev, void > > > *data) > > > +{ > > > + struct usb_device_driver *new_udriver = data; > > > + struct usb_device_driver *udriver; > > > + struct usb_device *udev; > > > + > > > + if (!dev->driver) > > > + return 0; > > > + > > > + udriver = to_usb_device_driver(dev->driver); > > > + if (udriver != &usb_generic_driver) > > > + return 0; > > > > What about > > > > static bool is_dev_usb_generic_driver(dev) > > { > > struct usb_device_driver *udd = dev->driver ? > > to_usb_device_driver(dev->driver) : NULL; > > > > return udd == &usb_generic_driver; > > } > > > > if (!is_dev_usb_generic_driver) > > return 0; > > > > ? > > > > > + udev = to_usb_device(dev); > > > + if (usb_device_match_id(udev, new_udriver->id_table) != > > > NULL || > > > + (new_udriver->match && new_udriver->match(udev) == > > > 0)) > > > + device_reprobe(dev); > > > + > > > + return 0; > > > > What about > > > > udev = to_usb_device(dev); > > if (usb_device_match_id(udev, new_udriver->id_table) == NULL) > > return 0; > > ? > > > > if (new_udriver->match && new_udriver->match(udev) == 0)) > > device_reprobe(dev); > > return 0; > > > > > +} > > It actually sparks a lot of similarities with > __check_usb_generic(). Perhaps you may unify them? They might look alike, but apart from that last check (id_table and match), they're really not. It's a different device driver that's getting checked against the generic driver. I tried to merge those, and it ended up being either completely wrong, or just as long as the code that used to be there before.
On Fri, Jul 24, 2020 at 01:32:40PM +0300, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> wrote: > > > > When a new device with a specialised device driver is plugged in, the > > new driver will be modprobe()'d but the driver core will attach the > > "generic" driver to the device. > > > > After that, nothing will trigger a reprobe when the modprobe()'d device > > driver has finished initialising, as the device has the "generic" > > driver attached to it. > > > > Trigger a reprobe ourselves when new specialised drivers get registered. > > ... > > > +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > > +{ > > + struct usb_device_driver *new_udriver = data; > > + struct usb_device_driver *udriver; > > + struct usb_device *udev; > > + > > + if (!dev->driver) > > + return 0; > > + > > + udriver = to_usb_device_driver(dev->driver); > > + if (udriver != &usb_generic_driver) > > + return 0; > > What about > > static bool is_dev_usb_generic_driver(dev) > { > struct usb_device_driver *udd = dev->driver ? > to_usb_device_driver(dev->driver) : NULL; > > return udd == &usb_generic_driver; > } > > if (!is_dev_usb_generic_driver) > return 0; > > ? Why would you want to add more lines of code to do the same thing? Abstraction is fine up to point, but excessive abstraction only makes things more difficult. > > > + udev = to_usb_device(dev); > > + if (usb_device_match_id(udev, new_udriver->id_table) != NULL || > > + (new_udriver->match && new_udriver->match(udev) == 0)) > > + device_reprobe(dev); > > + > > + return 0; > > What about > > udev = to_usb_device(dev); > if (usb_device_match_id(udev, new_udriver->id_table) == NULL) > return 0; > ? The logic is wrong. If usb_device_match_id() returns NULL then we want to go ahead with the second test, not give up right away. But this could be written as: if (usb_device_match_id(udev, new_udriver->id_table) == NULL && (!new_udriver->match || new_udriver->match(udev) != 0)) return 0; device_reprobe(dev); This would make the overall flow of the routine more uniform. > > + /* Check whether any device could be better served with > > + * this new driver > > + */ > > Comment style? Right: /* * Multiline comments * should look like this. */ > ... > > > + if (new_udriver->match || new_udriver->id_table) > > But match check is incorporated in the loop function. Agreed, this test is redundant. However, we should test that new_udriver != &usb_generic_driver. Alan Stern > > > + bus_for_each_dev(&usb_bus_type, NULL, new_udriver, > > + __usb_bus_reprobe_drivers); > > -- > With Best Regards, > Andy Shevchenko
On Fri, 2020-07-24 at 11:27 -0400, Alan Stern wrote: > On Fri, Jul 24, 2020 at 01:32:40PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> > > wrote: > > > When a new device with a specialised device driver is plugged in, > > > the > > > new driver will be modprobe()'d but the driver core will attach > > > the > > > "generic" driver to the device. > > > > > > After that, nothing will trigger a reprobe when the modprobe()'d > > > device > > > driver has finished initialising, as the device has the "generic" > > > driver attached to it. > > > > > > Trigger a reprobe ourselves when new specialised drivers get > > > registered. > > > > ... > > > > > +static int __usb_bus_reprobe_drivers(struct device *dev, void > > > *data) > > > +{ > > > + struct usb_device_driver *new_udriver = data; > > > + struct usb_device_driver *udriver; > > > + struct usb_device *udev; > > > + > > > + if (!dev->driver) > > > + return 0; > > > + > > > + udriver = to_usb_device_driver(dev->driver); > > > + if (udriver != &usb_generic_driver) > > > + return 0; > > > > What about > > > > static bool is_dev_usb_generic_driver(dev) > > { > > struct usb_device_driver *udd = dev->driver ? > > to_usb_device_driver(dev->driver) : NULL; > > > > return udd == &usb_generic_driver; > > } > > > > if (!is_dev_usb_generic_driver) > > return 0; > > > > ? > > Why would you want to add more lines of code to do the same thing? > Abstraction is fine up to point, but excessive abstraction only > makes > things more difficult. I actually like that one, it made the code clearer IMO. > > > + udev = to_usb_device(dev); > > > + if (usb_device_match_id(udev, new_udriver->id_table) != > > > NULL || > > > + (new_udriver->match && new_udriver->match(udev) == > > > 0)) > > > + device_reprobe(dev); > > > + > > > + return 0; > > > > What about > > > > udev = to_usb_device(dev); > > if (usb_device_match_id(udev, new_udriver->id_table) == NULL) > > return 0; > > ? > > The logic is wrong. If usb_device_match_id() returns NULL then we > want > to go ahead with the second test, not give up right away. But this > could be written as: > > if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > (!new_udriver->match || new_udriver- > >match(udev) != 0)) > return 0; > > device_reprobe(dev); > > This would make the overall flow of the routine more uniform. Adopted. > > > + /* Check whether any device could be better > > > served with > > > + * this new driver > > > + */ > > > > Comment style? > > Right: > > /* > * Multiline comments > * should look like this. > */ > > > ... > > > > > + if (new_udriver->match || new_udriver->id_table) > > > > But match check is incorporated in the loop function. > > Agreed, this test is redundant. However, we should test that > new_udriver != &usb_generic_driver. Do you really want to loop over every USB device when you know for a fact that not a single one will match? I guess it's unlikely, the generic driver would be loaded before any device, and the specialised drivers need to be able to selected, so I've done that locally. > Alan Stern > > > > + bus_for_each_dev(&usb_bus_type, NULL, > > > new_udriver, > > > + __usb_bus_reprobe_driver > > > s); > > > > -- > > With Best Regards, > > Andy Shevchenko
On Fri, Jul 24, 2020 at 06:52:31PM +0200, Bastien Nocera wrote: > On Fri, 2020-07-24 at 11:27 -0400, Alan Stern wrote: > > > > + if (new_udriver->match || new_udriver->id_table) > > > > > > But match check is incorporated in the loop function. > > > > Agreed, this test is redundant. However, we should test that > > new_udriver != &usb_generic_driver. > > Do you really want to loop over every USB device when you know for a > fact that not a single one will match? Think of it the other way around: How often will anybody load a specialized USB device driver that doesn't have a match function or ID table? It wouldn't match any devices! > I guess it's unlikely, the generic driver would be loaded before any > device, Since it's built into usbcore, I guess that's true. > and the specialised drivers need to be able to selected, so > I've done that locally. Okay, you're ready to submit the next version? Alan Stern
On Fri, 2020-07-24 at 13:08 -0400, Alan Stern wrote: > On Fri, Jul 24, 2020 at 06:52:31PM +0200, Bastien Nocera wrote: > > On Fri, 2020-07-24 at 11:27 -0400, Alan Stern wrote: > > > > > + if (new_udriver->match || new_udriver- > > > > > >id_table) > > > > > > > > But match check is incorporated in the loop function. > > > > > > Agreed, this test is redundant. However, we should test that > > > new_udriver != &usb_generic_driver. > > > > Do you really want to loop over every USB device when you know for > > a > > fact that not a single one will match? > > Think of it the other way around: How often will anybody load a > specialized USB device driver that doesn't have a match function or > ID > table? It wouldn't match any devices! > > > I guess it's unlikely, the generic driver would be loaded before > > any > > device, > > Since it's built into usbcore, I guess that's true. > > > and the specialised drivers need to be able to selected, so > > I've done that locally. > > Okay, you're ready to submit the next version? It's compiling. Then I need to test it. I've also added a couple of fixes/cleanups I ran into while doing this.
On Fri, Jul 24, 2020 at 6:27 PM Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, Jul 24, 2020 at 01:32:40PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 24, 2020 at 12:03 PM Bastien Nocera <hadess@hadess.net> wrote: ... > > What about > > > > static bool is_dev_usb_generic_driver(dev) > > { > > struct usb_device_driver *udd = dev->driver ? > > to_usb_device_driver(dev->driver) : NULL; > > > > return udd == &usb_generic_driver; > > } > > > > if (!is_dev_usb_generic_driver) > > return 0; > > > > ? > > Why would you want to add more lines of code to do the same thing? > Abstraction is fine up to point, but excessive abstraction only makes > things more difficult. At least to put into ternary will actually reduce LOCs in the original code (if we drop helper). The idea of a helper comes to mind that somebody else might reuse (__check_usb_generic()?). ... > The logic is wrong. If usb_device_match_id() returns NULL then we want > to go ahead with the second test, not give up right away. But this > could be written as: Yes, sorry I didn't think well here. > > if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > (!new_udriver->match || new_udriver->match(udev) != 0)) > return 0; > > device_reprobe(dev); > > This would make the overall flow of the routine more uniform. Yes, my intention was to make all error cases go first.
On Fri, 2020-07-24 at 19:14 +0200, Bastien Nocera wrote: > <snip> > It's compiling. Then I need to test it. I've also added a couple of > fixes/cleanups I ran into while doing this. And sent
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index f81606c6a35b..6731a8e104bc 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -905,6 +905,27 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env) return 0; } +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) +{ + struct usb_device_driver *new_udriver = data; + struct usb_device_driver *udriver; + struct usb_device *udev; + + if (!dev->driver) + return 0; + + udriver = to_usb_device_driver(dev->driver); + if (udriver != &usb_generic_driver) + return 0; + + udev = to_usb_device(dev); + if (usb_device_match_id(udev, new_udriver->id_table) != NULL || + (new_udriver->match && new_udriver->match(udev) == 0)) + device_reprobe(dev); + + return 0; +} + /** * usb_register_device_driver - register a USB device (not interface) driver * @new_udriver: USB operations for the device driver @@ -934,13 +955,20 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver, retval = driver_register(&new_udriver->drvwrap.driver); - if (!retval) + if (!retval) { pr_info("%s: registered new device driver %s\n", usbcore_name, new_udriver->name); - else + /* Check whether any device could be better served with + * this new driver + */ + if (new_udriver->match || new_udriver->id_table) + bus_for_each_dev(&usb_bus_type, NULL, new_udriver, + __usb_bus_reprobe_drivers); + } else { printk(KERN_ERR "%s: error %d registering device " " driver %s\n", usbcore_name, retval, new_udriver->name); + } return retval; }
When a new device with a specialised device driver is plugged in, the new driver will be modprobe()'d but the driver core will attach the "generic" driver to the device. After that, nothing will trigger a reprobe when the modprobe()'d device driver has finished initialising, as the device has the "generic" driver attached to it. Trigger a reprobe ourselves when new specialised drivers get registered. Fixes: 88b7381a939d Signed-off-by: Bastien Nocera <hadess@hadess.net> --- Changes since v3: - Only reprobe devices that could use the new driver - Many code fixes Changes since v2: - Fix formatting Changes since v1: - Simplified after Alan Stern's comments and some clarifications from Benjamin Tissoires. drivers/usb/core/driver.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)