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

[Peter Chen <peter.chen@nxp.com>]

On 20-07-23 12:57:18, Bastien Nocera wrote:
> On Thu, 2020-07-23 at 10:35 +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.
> 
> It's a _device_ driver, not an interface driver. It's the only driver
> that has the problem because it's the only non-generic device driver :)
> 

I may clear now, thanks.

So, you mean your device has no interface descriptor, so you can't
create a USB interface driver, and have to create non-generic device
driver for it. I see there is __check_usb_generic function at generic.c
to check if it could use generic driver or not, you may add some
condition (eg, no interface descriptor) to avoid using generic driver
Is it feasible?

-- 

Thanks,
Peter Chen