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@xxxxxxxxxx>
> > 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
