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

[Bastien Nocera <hadess@hadess.net>]

On Fri, 2020-07-24 at 11:27 -0400, Alan Stern wrote:
> 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.

I actually like that one, it made the code clearer IMO.

> > > +       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.

Adopted.

> > > +               /* 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.

Do you really want to loop over every USB device when you know for a
fact that not a single one will match?

I guess it's unlikely, the generic driver would be loaded before any
device, and the specialised drivers need to be able to selected, so
I've done that locally.

> Alan Stern
> 
> > > +                       bus_for_each_dev(&usb_bus_type, NULL,
> > > new_udriver,
> > > +                                        __usb_bus_reprobe_driver
> > > s);
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko