Re: [PATCH 1/2] USB: Fix device driver race

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jul 23, 2020 at 10:35:30AM +0000, Peter Chen wrote:
>  
> > > > > > 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.
> > > > > >
> > > > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > > > > ---
> > > > > >  drivers/usb/core/driver.c | 31 +++++++++++++++++++++++++++++--
> > > > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/core/driver.c
> > > > > > b/drivers/usb/core/driver.c index f81606c6a35b..a6187dd2186c
> > > > > > 100644
> > > > > > --- a/drivers/usb/core/driver.c
> > > > > > +++ b/drivers/usb/core/driver.c
> > > > > > @@ -905,6 +905,30 @@ static int usb_uevent(struct device *dev,
> > > > > > struct kobj_uevent_env *env)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int __usb_bus_reprobe_drivers(struct device *dev, void
> > > > > > *data)
> > > > > > +{
> > > > > > +	struct usb_device_driver *udriver =
> > > > > > to_usb_device_driver(dev-
> > > > > > > driver);
> > > > > > +	struct usb_device *udev = to_usb_device(dev);
> > > > > > +
> > > > > > +	if (udriver == &usb_generic_driver &&
> > > > > > +	    !udev->use_generic_driver)
> > > > > > +		return device_reprobe(dev);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int __usb_device_driver_added(struct device_driver
> > > > > > *drv,
> > > > > > void *data)
> > > > > > +{
> > > > > > +	struct usb_device_driver *udrv =
> > > > > > to_usb_device_driver(drv);
> > > > > > +
> > > > > > +	if (udrv->match) {
> > > > > > +		bus_for_each_dev(&usb_bus_type, NULL, udrv,
> > > > > > +				 __usb_bus_reprobe_drivers);
> > > > >
> > > > > What does udrv get used for here?
> > > > >
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * usb_register_device_driver - register a USB device (not
> > > > > > interface) driver
> > > > > >   * @new_udriver: USB operations for the device driver @@
> > > > > > -934,13 +958,16 @@ int usb_register_device_driver(struct
> > > > > > usb_device_driver *new_udriver,
> > > > > >
> > > > > >  	retval = driver_register(&new_udriver->drvwrap.driver);
> > > > > >
> > > > > > -	if (!retval)
> > > > > > +	if (!retval) {
> > > > > > +		bus_for_each_drv(&usb_bus_type, NULL, NULL,
> > > > > > +				 __usb_device_driver_added);
> > > > >
> > > > > This looks funny.  You're calling both bus_for_each_drv() and
> > > > > bus_for_each_dev().  Can't you skip this iterator and just call
> > > > > bus_for_each_dev() directly?
> > > >
> > > > You're right, looks like this could be simplified somewhat. I'm
> > > > building and testing a smaller patch.
> > > >
> > >
> > > What do you mean "reprobe" for your device? Do you mean the
> > > mfi_fc_probe is not called? If it is, Would you please check why
> > > usb_device_match at drivers/usb/core/driver.c does not return true for
> > > your device?
> > 
> > mfi_fc_probe() isn't called because the device already has the generic driver
> > attached by the time the apple-mfi-fastcharge driver is loaded.
> 
> Would you please explain more, why only this driver has this issue? Seem you
> create a struct usb_device_driver, but not struct usb_driver, few drivers do like
> that. You may see drivers/usb/misc/ehset.c and drivers/usb/misc/appledisplay.c
> as an example.

This is a different "type" of driver.