| Message ID | ab1fcd9c7e8f4aecd1f709a74a763bcc239fe6c4.camel@hadess.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sat, Jul 25, 2020 at 11:16:47AM +0200, 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 ("USB: Select better matching USB drivers when available") > Signed-off-by: Bastien Nocera <hadess@hadess.net> > --- > Changes since v4: > - Add commit subject to "fixes" section > - Clarify conditional that checks for generic driver > - Remove check duplicated inside the loop > > 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 | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index f81606c6a35b..7d3878aa8090 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -905,6 +905,32 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env) > return 0; > } > > +static bool is_dev_usb_generic_driver(struct device *dev) > +{ > + struct usb_device_driver *udd = dev->driver ? > + to_usb_device_driver(dev->driver) : NULL; > + > + return udd == &usb_generic_driver; > +} Heh... I don't recommend this optimization because it's a little unclear, but the function can be shortened to: return dev->driver == &usb_generic_driver.drvwrap.driver; > + > +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) > +{ > + struct usb_device_driver *new_udriver = data; > + struct usb_device *udev; > + > + if (!is_dev_usb_generic_driver(dev)) > + 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)) > + return 0; > + > + (void)!device_reprobe(dev); What's that '!' doing hiding in there? It doesn't affect the final outcome, but it sure looks weird -- if people notice it at all. Aside from that, Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern > + > + return 0; > +} > + > /** > * usb_register_device_driver - register a USB device (not interface) driver > * @new_udriver: USB operations for the device driver > @@ -934,13 +960,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 > + */ > + 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; > } >
On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote: <snip> > > + udev = to_usb_device(dev); > > + if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > > + (!new_udriver->match || new_udriver->match(udev) != 0)) > > + return 0; > > + > > + (void)!device_reprobe(dev); > > What's that '!' doing hiding in there? It doesn't affect the final > outcome, but it sure looks weird -- if people notice it at all. It's how we stop gcc from complaining about the warn_unused_result attribute on device_reprobe()... (void) is enough with clang, but not with gcc. > Aside from that, > > Acked-by: Alan Stern <stern@rowland.harvard.edu> Thanks!
On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote: > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote: > <snip> > > > + udev = to_usb_device(dev); > > > + if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > > > + (!new_udriver->match || new_udriver->match(udev) != 0)) > > > + return 0; > > > + > > > + (void)!device_reprobe(dev); > > > > What's that '!' doing hiding in there? It doesn't affect the final > > outcome, but it sure looks weird -- if people notice it at all. > > It's how we stop gcc from complaining about the warn_unused_result > attribute on device_reprobe()... (void) is enough with clang, but not > with gcc. Hmmm. Maybe this is an indication that device_reprobe() doesn't really need to be __must_check. Greg, do you know why it's annotated this way? Alan Stern
On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote: > On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote: > > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote: > > <snip> > > > > + udev = to_usb_device(dev); > > > > + if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > > > > + (!new_udriver->match || new_udriver->match(udev) != 0)) > > > > + return 0; > > > > + > > > > + (void)!device_reprobe(dev); > > > > > > What's that '!' doing hiding in there? It doesn't affect the final > > > outcome, but it sure looks weird -- if people notice it at all. > > > > It's how we stop gcc from complaining about the warn_unused_result > > attribute on device_reprobe()... (void) is enough with clang, but not > > with gcc. > > Hmmm. Maybe this is an indication that device_reprobe() doesn't really > need to be __must_check. > > Greg, do you know why it's annotated this way? Because you really should pass up the return value if an error happens here. Why do we think it is safe to ignore? And that "(void)!" is not ok, if the annotation is safe to ignore, then we need to remove the annotation, don't work around stuff like this without at the very least, a comment saying why it is ok. thanks, greg k-h
On Sun, Jul 26, 2020 at 10:36:55AM +0200, Greg Kroah-Hartman wrote: > On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote: > > On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote: > > > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote: > > > <snip> > > > > > + udev = to_usb_device(dev); > > > > > + if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > > > > > + (!new_udriver->match || new_udriver->match(udev) != 0)) > > > > > + return 0; > > > > > + > > > > > + (void)!device_reprobe(dev); > > > > > > > > What's that '!' doing hiding in there? It doesn't affect the final > > > > outcome, but it sure looks weird -- if people notice it at all. > > > > > > It's how we stop gcc from complaining about the warn_unused_result > > > attribute on device_reprobe()... (void) is enough with clang, but not > > > with gcc. > > > > Hmmm. Maybe this is an indication that device_reprobe() doesn't really > > need to be __must_check. > > > > Greg, do you know why it's annotated this way? > > Because you really should pass up the return value if an error happens > here. Why do we think it is safe to ignore? > > And that "(void)!" is not ok, if the annotation is safe to ignore, then > we need to remove the annotation, don't work around stuff like this > without at the very least, a comment saying why it is ok. I suppose Bastien could log an error message at that point. There isn't much else to do. Alan Stern
On Sun, Jul 26, 2020 at 10:17:08AM -0400, Alan Stern wrote: > On Sun, Jul 26, 2020 at 10:36:55AM +0200, Greg Kroah-Hartman wrote: > > On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote: > > > On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote: > > > > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote: > > > > <snip> > > > > > > + udev = to_usb_device(dev); > > > > > > + if (usb_device_match_id(udev, new_udriver->id_table) == NULL && > > > > > > + (!new_udriver->match || new_udriver->match(udev) != 0)) > > > > > > + return 0; > > > > > > + > > > > > > + (void)!device_reprobe(dev); > > > > > > > > > > What's that '!' doing hiding in there? It doesn't affect the final > > > > > outcome, but it sure looks weird -- if people notice it at all. > > > > > > > > It's how we stop gcc from complaining about the warn_unused_result > > > > attribute on device_reprobe()... (void) is enough with clang, but not > > > > with gcc. > > > > > > Hmmm. Maybe this is an indication that device_reprobe() doesn't really > > > need to be __must_check. > > > > > > Greg, do you know why it's annotated this way? > > > > Because you really should pass up the return value if an error happens > > here. Why do we think it is safe to ignore? > > > > And that "(void)!" is not ok, if the annotation is safe to ignore, then > > we need to remove the annotation, don't work around stuff like this > > without at the very least, a comment saying why it is ok. > > I suppose Bastien could log an error message at that point. There isn't > much else to do. It looks like no one does anything with the return value of that function, so we should just change it to void and stop checking it entirely :( greg k-h
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index f81606c6a35b..7d3878aa8090 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -905,6 +905,32 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env) return 0; } +static bool is_dev_usb_generic_driver(struct device *dev) +{ + struct usb_device_driver *udd = dev->driver ? + to_usb_device_driver(dev->driver) : NULL; + + return udd == &usb_generic_driver; +} + +static int __usb_bus_reprobe_drivers(struct device *dev, void *data) +{ + struct usb_device_driver *new_udriver = data; + struct usb_device *udev; + + if (!is_dev_usb_generic_driver(dev)) + 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)) + return 0; + + (void)!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 +960,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 + */ + 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 ("USB: Select better matching USB drivers when available") Signed-off-by: Bastien Nocera <hadess@hadess.net> --- Changes since v4: - Add commit subject to "fixes" section - Clarify conditional that checks for generic driver - Remove check duplicated inside the loop 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 | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)