| Message ID | 20200722094628.4236-1-hadess@hadess.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] USB: Fix device driver race | expand |
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
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
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
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
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?
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.
> > > > > 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
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.
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 :)
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?
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.
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; }
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(-)