Re: [PATCH v4] USB: Fix device driver race

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

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