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

[Alan Stern <stern@rowland.harvard.edu>]

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;
>  }
>