| Message ID | a3cd9c51f215be37ac9bb44083ab8b3280f9359f.camel@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] USB: Fix device driver race | expand |
On Thu, Jul 23, 2020 at 11:30:39PM +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 > Signed-off-by: Bastien Nocera <hadess@hadess.net> Better than before, but there are still some things to improve. > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c > index f81606c6a35b..44531910637c 100644 > --- a/drivers/usb/core/driver.c > +++ b/drivers/usb/core/driver.c > @@ -905,6 +905,25 @@ 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 *udriver = to_usb_device_driver(dev->driver); This variable isn't used (it gets shadowed below). > + struct usb_device *udev = to_usb_device(dev); And this one doesn't get used unless both tests below succeed, so the declaration should be moved down to the inner block. > + > + if (dev->driver) { > + struct usb_device_driver *udriver = to_usb_device_driver(dev->driver); > + > + if (udriver == NULL || udriver == &usb_generic_driver) { Since dev->driver is not NULL, udriver cannot possibly be NULL. No need to test for it. > + udev->use_generic_driver = false; Are you sure you want to do that? If udev->use_generic_driver was set, it means that a specialized driver has already been probed and failed. We assume only one specialized driver will match a particular device, so there's no point in reprobing -- the same driver will match and it will just fail the probe again. > + device_reprobe(dev); We shouldn't do this unless we have a good reason for believing the new driver will bind to the device. Otherwise you're messing up a perfectly good existing binding for no reason. You should first test whether the device matches the new driver (pass the new driver as the iterator parameter). > + } > + } else { > + 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 +953,19 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver, > > retval = driver_register(&new_udriver->drvwrap.driver); > > - if (!retval) > + if (!retval) { > + /* Check whether any device could be better served with > + * this new driver > + */ > + bus_for_each_dev(&usb_bus_type, NULL, NULL, > + __usb_bus_reprobe_drivers); > pr_info("%s: registered new device driver %s\n", > usbcore_name, new_udriver->name); The new iterator should be added after the pr_info(), not before, because we want the new driver's registration to show up in the kernel log before any notices about it being bound to devices. Alan Stern > - else > + } else { > printk(KERN_ERR "%s: error %d registering device " > " driver %s\n", > usbcore_name, retval, new_udriver->name); > + } > > return retval; > } >
Hi Bastien,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on peter.chen-usb/ci-for-usb-next balbi-usb/testing/next v5.8-rc6 next-20200723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bastien-Nocera/USB-Fix-device-driver-race/20200724-053320
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-rhel-7.6-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/usb/core/driver.c: In function '__usb_bus_reprobe_drivers':
drivers/usb/core/driver.c:910:28: warning: unused variable 'udriver' [-Wunused-variable]
910 | struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
| ^~~~~~~
>> drivers/usb/core/driver.c:918:4: warning: ignoring return value of 'device_reprobe', declared with attribute warn_unused_result [-Wunused-result]
918 | device_reprobe(dev);
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/core/driver.c:921:3: warning: ignoring return value of 'device_reprobe', declared with attribute warn_unused_result [-Wunused-result]
921 | device_reprobe(dev);
| ^~~~~~~~~~~~~~~~~~~
vim +/device_reprobe +918 drivers/usb/core/driver.c
907
908 static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
909 {
910 struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
911 struct usb_device *udev = to_usb_device(dev);
912
913 if (dev->driver) {
914 struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
915
916 if (udriver == NULL || udriver == &usb_generic_driver) {
917 udev->use_generic_driver = false;
> 918 device_reprobe(dev);
919 }
920 } else {
921 device_reprobe(dev);
922 }
923
924 return 0;
925 }
926
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Bastien,
I love your patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on peter.chen-usb/ci-for-usb-next balbi-usb/testing/next v5.8-rc6 next-20200723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Bastien-Nocera/USB-Fix-device-driver-race/20200724-053320
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a004-20200724 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 1d09ecf36175f7910ffedd6d497c07b5c74c22fb)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/usb/core/driver.c:918:4: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
device_reprobe(dev);
^~~~~~~~~~~~~~ ~~~
drivers/usb/core/driver.c:921:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
device_reprobe(dev);
^~~~~~~~~~~~~~ ~~~
drivers/usb/core/driver.c:910:28: warning: unused variable 'udriver' [-Wunused-variable]
struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
^
3 warnings generated.
vim +/warn_unused_result +918 drivers/usb/core/driver.c
907
908 static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
909 {
910 struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
911 struct usb_device *udev = to_usb_device(dev);
912
913 if (dev->driver) {
914 struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
915
916 if (udriver == NULL || udriver == &usb_generic_driver) {
917 udev->use_generic_driver = false;
> 918 device_reprobe(dev);
919 }
920 } else {
921 device_reprobe(dev);
922 }
923
924 return 0;
925 }
926
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, 2020-07-23 at 17:58 -0400, Alan Stern wrote: > On Thu, Jul 23, 2020 at 11:30:39PM +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 > > Signed-off-by: Bastien Nocera <hadess@hadess.net> > > Better than before, but there are still some things to improve. That's kind of you to say, but it was really incredibly sloppy. I've sent a v4 that only runs the reprobe on device that could be attached to the new driver. It was closer to an early version I made locally and didn't work because I forgot that ->id_table could also be used for matching, and because I was only running it when ->match was present, the reprobe was never actually run... This should all be fixed now. Cheers
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c index f81606c6a35b..44531910637c 100644 --- a/drivers/usb/core/driver.c +++ b/drivers/usb/core/driver.c @@ -905,6 +905,25 @@ 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 *udriver = to_usb_device_driver(dev->driver); + struct usb_device *udev = to_usb_device(dev); + + if (dev->driver) { + struct usb_device_driver *udriver = to_usb_device_driver(dev->driver); + + if (udriver == NULL || udriver == &usb_generic_driver) { + udev->use_generic_driver = false; + device_reprobe(dev); + } + } else { + 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 +953,19 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver, retval = driver_register(&new_udriver->drvwrap.driver); - if (!retval) + if (!retval) { + /* Check whether any device could be better served with + * this new driver + */ + bus_for_each_dev(&usb_bus_type, NULL, NULL, + __usb_bus_reprobe_drivers); pr_info("%s: registered new device driver %s\n", usbcore_name, new_udriver->name); - else + } 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 v2: - Fix formatting Changes since v1: - Simplified after Alan Stern's comments and some clarifications from Benjamin Tissoires. drivers/usb/core/driver.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)