[PATCH v8 1/3] USB: Also match device drivers using the ->match vfunc

[PATCH v8 1/3] USB: Also match device drivers using the ->match vfunc

[Bastien Nocera]

We only ever used the ID table matching before, but we should also support
open-coded match functions.

Fixes: 88b7381a939de ("USB: Select better matching USB drivers when available")
Signed-off-by: Bastien Nocera <hadess@hadess.net>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
Changes since v7:
- Reword commit subject

Changes since v2:
- Added Alan's ack

Changes since v1:
- Fixed typo in commit message


 drivers/usb/core/generic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index b6f2d4b44754..2b2f1ab6e36a 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -205,8 +205,9 @@ static int __check_usb_generic(struct device_driver *drv, void *data)
 	udrv = to_usb_device_driver(drv);
 	if (udrv == &usb_generic_driver)
 		return 0;
-
-	return usb_device_match_id(udev, udrv->id_table) != NULL;
+	if (usb_device_match_id(udev, udrv->id_table) != NULL)
+		return 1;
+	return (udrv->match && udrv->match(udev));
 }
 
 static bool usb_generic_driver_match(struct usb_device *udev)
-- 
2.26.2

[PATCH v8 2/3] USB: Better name for __check_usb_generic()

[Bastien Nocera]

__check_usb_generic() doesn't explain very well what the
function actually does: It checks to see whether the driver is
non-generic and matches the device.

Change it to check_for_non_generic_match()

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
No changes since v7 which was the first version based on
Alan's comment:
https://marc.info/?l=linux-usb&m=159568870400795&w=2

 drivers/usb/core/generic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 2b2f1ab6e36a..22c887f5c497 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -195,7 +195,7 @@ int usb_choose_configuration(struct usb_device *udev)
 }
 EXPORT_SYMBOL_GPL(usb_choose_configuration);
 
-static int __check_usb_generic(struct device_driver *drv, void *data)
+static int __check_for_non_generic_match(struct device_driver *drv, void *data)
 {
 	struct usb_device *udev = data;
 	struct usb_device_driver *udrv;
@@ -219,7 +219,7 @@ static bool usb_generic_driver_match(struct usb_device *udev)
 	 * If any other driver wants the device, leave the device to this other
 	 * driver.
 	 */
-	if (bus_for_each_drv(&usb_bus_type, NULL, udev, __check_usb_generic))
+	if (bus_for_each_drv(&usb_bus_type, NULL, udev, __check_for_non_generic_match))
 		return false;
 
 	return true;
-- 
2.26.2

[PATCH v8 3/3] USB: Fix device driver race

[Bastien Nocera]

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>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
Changes since v7:
- None

Changes since v6:
- Added Alan's ack

Changes since v5:
- Throw error when device_reprobe() fails

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 | 40 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f81606c6a35b..7e73e989645b 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -905,6 +905,35 @@ 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;
+	int ret;
+
+	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;
+
+	ret = device_reprobe(dev);
+	if (ret && ret != -EPROBE_DEFER)
+		dev_err(dev, "Failed to reprobe device (error %d)\n", ret);
+
+	return 0;
+}
+
 /**
  * usb_register_device_driver - register a USB device (not interface) driver
  * @new_udriver: USB operations for the device driver
@@ -934,13 +963,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;
 }
-- 
2.26.2

Re: [PATCH v8 3/3] USB: Fix device driver race

[Sergei Shtylyov]

On 8/18/20 2:04 PM, 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>
[...]
>  drivers/usb/core/driver.c | 40 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f81606c6a35b..7e73e989645b 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
[...]
> @@ -934,13 +963,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",

   Unrelated but... hm, this string literal seems weird. GregKH, would it be OK if we fix it?

>  			usbcore_name, retval, new_udriver->name);
> +	}
>  
>  	return retval;
>  }
> 

MBR, Sergei

Re: [PATCH v8 3/3] USB: Fix device driver race

[Greg Kroah-Hartman]

On Tue, Aug 18, 2020 at 08:06:20PM +0300, Sergei Shtylyov wrote:
> On 8/18/20 2:04 PM, 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>
> [...]
> >  drivers/usb/core/driver.c | 40 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index f81606c6a35b..7e73e989645b 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> [...]
> > @@ -934,13 +963,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",
> 
>    Unrelated but... hm, this string literal seems weird. GregKH, would it be OK if we fix it?

Please do.

Re: [PATCH v8 3/3] USB: Fix device driver race

[Alan Stern]

On Tue, Aug 18, 2020 at 08:06:20PM +0300, Sergei Shtylyov wrote:
> On 8/18/20 2:04 PM, Bastien Nocera wrote:

> > +	} else {
> >  		printk(KERN_ERR "%s: error %d registering device "
> >  			"	driver %s\n",
> 
>    Unrelated but... hm, this string literal seems weird. GregKH, would it be OK if we fix it?
> 
> >  			usbcore_name, retval, new_udriver->name);

Indeed, an extra tab character snuck in there by mistake.  It has been 
present ever since 2006, when the routine was originally added by commit 
8bb54ab573ec ("usbcore: add usb_device_driver definition").

It's perfectly okay with me if someone wants to remove the extra tab.  
In fact, nowadays we'd remove the line break entirely.

Alan Stern

Re: [PATCH v8 3/3] USB: Fix device driver race

[Sergei Shtylyov]

On 8/18/20 8:29 PM, Alan Stern wrote:

>>> +	} else {
>>>  		printk(KERN_ERR "%s: error %d registering device "
>>>  			"	driver %s\n",
>>
>>    Unrelated but... hm, this string literal seems weird. GregKH, would it be OK if we fix it?
>>
>>>  			usbcore_name, retval, new_udriver->name);
> 
> Indeed, an extra tab character snuck in there by mistake.  It has been 
> present ever since 2006, when the routine was originally added by commit 
> 8bb54ab573ec ("usbcore: add usb_device_driver definition").

   And meanwhile it got copied to another function, usb_register_driver().
I guess it's OK to fix both w/one patch?

> It's perfectly okay with me if someone wants to remove the extra tab.  
> In fact, nowadays we'd remove the line break entirely.

    It seems this wasn't needed even in the 80-column era...

> Alan Stern

MBR, Sergei

Re: [PATCH v8 3/3] USB: Fix device driver race

[Greg Kroah-Hartman]

On Tue, Aug 18, 2020 at 11:49:43PM +0300, Sergei Shtylyov wrote:
> On 8/18/20 8:29 PM, Alan Stern wrote:
> 
> >>> +	} else {
> >>>  		printk(KERN_ERR "%s: error %d registering device "
> >>>  			"	driver %s\n",
> >>
> >>    Unrelated but... hm, this string literal seems weird. GregKH, would it be OK if we fix it?
> >>
> >>>  			usbcore_name, retval, new_udriver->name);
> > 
> > Indeed, an extra tab character snuck in there by mistake.  It has been 
> > present ever since 2006, when the routine was originally added by commit 
> > 8bb54ab573ec ("usbcore: add usb_device_driver definition").
> 
>    And meanwhile it got copied to another function, usb_register_driver().
> I guess it's OK to fix both w/one patch?

Yes.