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

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

[Bastien Nocera]

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.

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);
+	}
+
+	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);
 		pr_info("%s: registered new device driver %s\n",
 			usbcore_name, new_udriver->name);
-	else
+	} else {
 		printk(KERN_ERR "%s: error %d registering device "
 			"	driver %s\n",
 			usbcore_name, retval, new_udriver->name);
+	}
 
 	return retval;
 }
-- 
2.26.2

[PATCH 2/2] USB: Fix source path in header

[Bastien Nocera]

Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
 drivers/usb/core/driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index a6187dd2186c..bd346e683af3 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * drivers/usb/driver.c - most of the driver model stuff for usb
+ * drivers/usb/core/driver.c - most of the driver model stuff for usb
  *
  * (C) Copyright 2005 Greg Kroah-Hartman <gregkh@suse.de>
  *
-- 
2.26.2

Re: [PATCH 2/2] USB: Fix source path in header

[Greg Kroah-Hartman]

On Wed, Jul 22, 2020 at 11:46:28AM +0200, Bastien Nocera wrote:
> Signed-off-by: Bastien Nocera <hadess@hadess.net>

I can't take patches without any changelog text at all, sorry.

> ---
>  drivers/usb/core/driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index a6187dd2186c..bd346e683af3 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * drivers/usb/driver.c - most of the driver model stuff for usb
> + * drivers/usb/core/driver.c - most of the driver model stuff for usb

Just drop the whole file name, it's not needed at all.

thanks,

greg k-h

Re: [PATCH 2/2] USB: Fix source path in header

[Bastien Nocera]

On Wed, 2020-07-22 at 11:51 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 11:46:28AM +0200, Bastien Nocera wrote:
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> 
> I can't take patches without any changelog text at all, sorry.

The subject line isn't clear enough?

> > ---
> >  drivers/usb/core/driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index a6187dd2186c..bd346e683af3 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - * drivers/usb/driver.c - most of the driver model stuff for usb
> > + * drivers/usb/core/driver.c - most of the driver model stuff for
> > usb
> 
> Just drop the whole file name, it's not needed at all.

And the description after or just the filename?

Re: [PATCH 2/2] USB: Fix source path in header

[Greg Kroah-Hartman]

On Wed, Jul 22, 2020 at 11:58:54AM +0200, Bastien Nocera wrote:
> On Wed, 2020-07-22 at 11:51 +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jul 22, 2020 at 11:46:28AM +0200, Bastien Nocera wrote:
> > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > 
> > I can't take patches without any changelog text at all, sorry.
> 
> The subject line isn't clear enough?

Maybe, maybe not, doesn't mean you shouldn't also write some text about
why you did what you did.

> > > ---
> > >  drivers/usb/core/driver.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > > index a6187dd2186c..bd346e683af3 100644
> > > --- a/drivers/usb/core/driver.c
> > > +++ b/drivers/usb/core/driver.c
> > > @@ -1,6 +1,6 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  /*
> > > - * drivers/usb/driver.c - most of the driver model stuff for usb
> > > + * drivers/usb/core/driver.c - most of the driver model stuff for
> > > usb
> > 
> > Just drop the whole file name, it's not needed at all.
> 
> And the description after or just the filename?

The description is fine to keep.

thanks,

greg k-h

Re: [PATCH 2/2] USB: Fix source path in header

[Bastien Nocera]

On Wed, 2020-07-22 at 12:35 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 11:58:54AM +0200, Bastien Nocera wrote:
> > On Wed, 2020-07-22 at 11:51 +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 22, 2020 at 11:46:28AM +0200, Bastien Nocera wrote:
> > > > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > > 
> > > I can't take patches without any changelog text at all, sorry.
> > 
> > The subject line isn't clear enough?
> 
> Maybe, maybe not, doesn't mean you shouldn't also write some text
> about
> why you did what you did.
> 
> > > > ---
> > > >  drivers/usb/core/driver.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/usb/core/driver.c
> > > > b/drivers/usb/core/driver.c
> > > > index a6187dd2186c..bd346e683af3 100644
> > > > --- a/drivers/usb/core/driver.c
> > > > +++ b/drivers/usb/core/driver.c
> > > > @@ -1,6 +1,6 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  /*
> > > > - * drivers/usb/driver.c - most of the driver model stuff for
> > > > usb
> > > > + * drivers/usb/core/driver.c - most of the driver model stuff
> > > > for
> > > > usb
> > > 
> > > Just drop the whole file name, it's not needed at all.
> > 
> > And the description after or just the filename?
> 
> The description is fine to keep.

Looks like there's a lot more cleaning up that would need to be done in
the tree, so feel free to drop this patch.

Cheers

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

[Greg Kroah-Hartman]

On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---

What commit id does this fix?  And if it's in an older kernel, should it
be backported to the stable trees?

thanks,

greg k-h

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

[Bastien Nocera]

On Wed, 2020-07-22 at 13:12 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> > 
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> 
> What commit id does this fix?  And if it's in an older kernel, should
> it
> be backported to the stable trees?

Fixes: 88b7381a939de0fa1f1b1629c56b03dca7077309

AFAIK, the apple-mfi-fastcharge driver wasn't backported to stable
trees, and it's the only driver that's impacted by this bug, so there
shouldn't be any need to backport it there.

Did you want me to respin it with this info?

Cheers

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

[Alan Stern]

On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> 
> 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?

Alan Stern

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

[Bastien Nocera]

On Wed, 2020-07-22 at 11:26 -0400, Alan Stern wrote:
> On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> > 
> > 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.

Thanks

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

[Peter Chen]

On 20-07-22 17:53:25, Bastien Nocera wrote:
> On Wed, 2020-07-22 at 11:26 -0400, Alan Stern wrote:
> > On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> > > 
> > > 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?

-- 

Thanks,
Peter Chen

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

[Bastien Nocera]

On Thu, 2020-07-23 at 02:19 +0000, Peter Chen wrote:
> On 20-07-22 17:53:25, Bastien Nocera wrote:
> > On Wed, 2020-07-22 at 11:26 -0400, Alan Stern wrote:
> > > On Wed, Jul 22, 2020 at 11:46:27AM +0200, Bastien Nocera 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.
> > > > 
> > > > 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.

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

[Peter Chen]

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

Peter

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

[Greg Kroah-Hartman]

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.

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

[Bastien Nocera]

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 :)

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

[Peter Chen]

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

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

[Bastien Nocera]

On Thu, 2020-07-23 at 12:12 +0000, Peter Chen wrote:
> 
<snip>
> 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?

I'm not looking for work-arounds, the driver is meant to be for *the
device*, not for interfaces. It does have interfaces, but they're not
the target of the calls made.

There's a race between USB device (not interface) drivers on the first
plug, which I'm trying to fix.