Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Thu, 10 Oct 2019, Bastien Nocera wrote: > On Wed, 2019-10-09 at 14:45 -0400, Alan Stern wrote: > > > On Wed, 9 Oct 2019, Bastien Nocera wrote: > > > > > > > + return > > device_driver_attach(usb_generic_driver.drvwrap.driver, dev); > > > + return error; > > > > I think that's right. A little testing wouldn't hurt. > > device_driver_attach() isn't available to this part of the code. > > I think the only way to do things here might be to set status bit for > the usb_device and launch device_reprobe(). The second time around, we > wouldn't match or probe the specific driver. That would mean probing generic_driver twice, right? You probably should call its disconnect routine in between. That sounds pretty awkward, but if there's no other way then go ahead and do it. Ala Stern
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
Hi Bastien, I love your patch! Perhaps something to improve: [auto build test WARNING on usb/usb-testing] [cannot apply to v5.4-rc2 next-20191010] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Bastien-Nocera/Add-Apple-MFi-fastcharge-USB-device-driver/20191010-155853 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file include/linux/reservation.h Error: Cannot open file include/linux/reservation.h include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'locked_down' not described in 'security_list_options' include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found drivers/gpio/gpiolib-of.c:92: warning: Excess function parameter 'dev' description in 'of_gpio_need_valid_mask' include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found >> include/linux/usb.h:1251: warning: Function parameter or member 'match' not >> described in 'usb_device_driver' >> include/linux/usb.h:1251: warning: Function parameter or member 'id_table' >> not described in 'usb_device_driver' include/linux/usb.h:1251: warning: Function parameter or member 'generic_init' not described in 'usb_device_driver' include/linux/w1.h:277: warning: Function parameter or member 'of_match_table' not described in 'w1_family' fs/fs-writeback.c:913: warning: Excess function parameter 'nr_pages' description in 'cgroup_writeback_by_id' fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete' fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end' fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 2019-10-09 at 14:45 -0400, Alan Stern wrote: > On Wed, 9 Oct 2019, Bastien Nocera wrote: > > > > + return > device_driver_attach(usb_generic_driver.drvwrap.driver, dev); > > + return error; > > I think that's right. A little testing wouldn't hurt. device_driver_attach() isn't available to this part of the code. I think the only way to do things here might be to set status bit for the usb_device and launch device_reprobe(). The second time around, we wouldn't match or probe the specific driver.
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 9 Oct 2019, Bastien Nocera wrote:
> On Wed, 2019-10-09 at 13:28 -0400, Alan Stern wrote:
>
> > No, that's not quite it.
> >
> > Here's what should happen when the subclass driver is being probed:
> > First, call the generic_probe routine, and return immediately if that
> > fails. Then call the subclass driver's probe routine. If that gets
> > an
> > error, fail the probe call but tell the device core that the device
> > is
> > now bound to the generic driver, not to the subclass driver.
>
> So, something like that, on top of the existing patches? (I'm not sure
> whether device_driver_attach is the correct call to use here).
>
> - if (udriver->probe)
> - return udriver->probe(udev);
> - return 0;
> + if (!udriver->probe)
> + return 0;
This test is unnecessary; all drivers must have a probe routine.
Otherwise how would they know when they get bound to a device?
> + error = udriver->probe(udev);
> + if (error == -ENODEV &&
> + udrv != &usb_generic_driver)
No need to test for usb_generic_driver; its probe routine always
returns 0. But if you want to include the test anyway, at least don't
split the line -- it will all fit in under 80 columns.
> + return
> device_driver_attach(usb_generic_driver.drvwrap.driver, dev);
> + return error;
I think that's right. A little testing wouldn't hurt.
> Anything else in this patch series? I was concerned about the naming
> for "generic_init" in patch 2 ("subclass").
Yes; see the suggestions in
https://marc.info/?l=linux-usb&m=157063168632242&w=2
Also (I didn't notice this earlier), in patch 1/5 it's not necessary to
EXPORT the usb_generic_* routines. They don't get used in the subclass
driver, only in usbcore.
> If there's nothing, I'll test and respin the patchset with the above
> changes tomorrow.
Okay, good.
Alan Stern
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 2019-10-09 at 13:28 -0400, Alan Stern wrote:
> No, that's not quite it.
>
> Here's what should happen when the subclass driver is being probed:
> First, call the generic_probe routine, and return immediately if that
> fails. Then call the subclass driver's probe routine. If that gets
> an
> error, fail the probe call but tell the device core that the device
> is
> now bound to the generic driver, not to the subclass driver.
So, something like that, on top of the existing patches? (I'm not sure
whether device_driver_attach is the correct call to use here).
- if (udriver->probe)
- return udriver->probe(udev);
- return 0;
+ if (!udriver->probe)
+ return 0;
+ error = udriver->probe(udev);
+ if (error == -ENODEV &&
+ udrv != &usb_generic_driver)
+ return device_driver_attach(usb_generic_driver.drvwrap.driver,
dev);
+ return error;
Anything else in this patch series? I was concerned about the naming
for "generic_init" in patch 2 ("subclass").
If there's nothing, I'll test and respin the patchset with the above
changes tomorrow.
Cheers
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 9 Oct 2019, Bastien Nocera wrote:
> On Wed, 2019-10-09 at 10:43 -0400, Alan Stern wrote:
> > On Wed, 9 Oct 2019, Bastien Nocera wrote:
> >
> > > Now that USB device drivers can reuse code from the generic USB
> > device
> > > driver, we need to make sure that they get selected rather than the
> > > generic driver. Add an id_table and match vfunc to the
> > usb_device_driver
> > > struct, which will get used to select a better matching driver at
> > > ->probe time.
> > >
> > > This is a similar mechanism to that used in the HID drivers, with
> > the
> > > generic driver being selected unless there's a better matching one
> > found
> > > in the registered drivers (see hid_generic_match() in
> > > drivers/hid/hid-generic.c).
> > >
> > > Signed-off-by: Bastien Nocera
> > > ---
> > > drivers/usb/core/driver.c | 15 +--
> > > drivers/usb/core/generic.c | 29 +
> > > include/linux/usb.h| 2 ++
> > > 3 files changed, 44 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > > index 50f92da8afcf..27ce63ed902d 100644
> > > --- a/drivers/usb/core/driver.c
> > > +++ b/drivers/usb/core/driver.c
> > > @@ -819,13 +819,24 @@ static int usb_device_match(struct device
> > *dev, struct device_driver *drv)
> > > {
> > > /* devices and interfaces are handled separately */
> > > if (is_usb_device(dev)) {
> > > + struct usb_device *udev;
> > > + struct usb_device_driver *udrv;
> > >
> > > /* interface drivers never match devices */
> > > if (!is_usb_device_driver(drv))
> > > return 0;
> > >
> > > - /* TODO: Add real matching code */
> > > - return 1;
> > > + udev = to_usb_device(dev);
> > > + udrv = to_usb_device_driver(drv);
> > > +
> > > + if (udrv->id_table &&
> > > + usb_device_match_id(udev, udrv->id_table) !=
> > NULL) {
> > > + return 1;
> > > + }
> > > +
> > > + if (udrv->match)
> > > + return udrv->match(udev);
> > > + return 0;
> >
> > What happens if the subclass driver's probe routine returns an
> > error?
> > Don't you still want the device to be bound to the generic driver?
>
> I don't know whether that's what you'd want to do. But if we did,
> that'd only be for devices which have "generic_init" set.
>
> We'd need to remember the result of the ->probe() call at the end of
> usb_probe_device() (as modified in patch 2), and only call the generic
> driver (not the specific device driver)'s functions in later usage.
>
> Is that what you would expect?
No, that's not quite it.
Here's what should happen when the subclass driver is being probed:
First, call the generic_probe routine, and return immediately if that
fails. Then call the subclass driver's probe routine. If that gets an
error, fail the probe call but tell the device core that the device is
now bound to the generic driver, not to the subclass driver.
Alan Stern
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 2019-10-09 at 10:43 -0400, Alan Stern wrote:
> On Wed, 9 Oct 2019, Bastien Nocera wrote:
>
> > Now that USB device drivers can reuse code from the generic USB
> device
> > driver, we need to make sure that they get selected rather than the
> > generic driver. Add an id_table and match vfunc to the
> usb_device_driver
> > struct, which will get used to select a better matching driver at
> > ->probe time.
> >
> > This is a similar mechanism to that used in the HID drivers, with
> the
> > generic driver being selected unless there's a better matching one
> found
> > in the registered drivers (see hid_generic_match() in
> > drivers/hid/hid-generic.c).
> >
> > Signed-off-by: Bastien Nocera
> > ---
> > drivers/usb/core/driver.c | 15 +--
> > drivers/usb/core/generic.c | 29 +
> > include/linux/usb.h| 2 ++
> > 3 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index 50f92da8afcf..27ce63ed902d 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -819,13 +819,24 @@ static int usb_device_match(struct device
> *dev, struct device_driver *drv)
> > {
> > /* devices and interfaces are handled separately */
> > if (is_usb_device(dev)) {
> > + struct usb_device *udev;
> > + struct usb_device_driver *udrv;
> >
> > /* interface drivers never match devices */
> > if (!is_usb_device_driver(drv))
> > return 0;
> >
> > - /* TODO: Add real matching code */
> > - return 1;
> > + udev = to_usb_device(dev);
> > + udrv = to_usb_device_driver(drv);
> > +
> > + if (udrv->id_table &&
> > + usb_device_match_id(udev, udrv->id_table) !=
> NULL) {
> > + return 1;
> > + }
> > +
> > + if (udrv->match)
> > + return udrv->match(udev);
> > + return 0;
>
> What happens if the subclass driver's probe routine returns an
> error?
> Don't you still want the device to be bound to the generic driver?
I don't know whether that's what you'd want to do. But if we did,
that'd only be for devices which have "generic_init" set.
We'd need to remember the result of the ->probe() call at the end of
usb_probe_device() (as modified in patch 2), and only call the generic
driver (not the specific device driver)'s functions in later usage.
Is that what you would expect?
Re: [PATCH 4/5] USB: Select better matching USB drivers when available
On Wed, 9 Oct 2019, Bastien Nocera wrote:
> Now that USB device drivers can reuse code from the generic USB device
> driver, we need to make sure that they get selected rather than the
> generic driver. Add an id_table and match vfunc to the usb_device_driver
> struct, which will get used to select a better matching driver at
> ->probe time.
>
> This is a similar mechanism to that used in the HID drivers, with the
> generic driver being selected unless there's a better matching one found
> in the registered drivers (see hid_generic_match() in
> drivers/hid/hid-generic.c).
>
> Signed-off-by: Bastien Nocera
> ---
> drivers/usb/core/driver.c | 15 +--
> drivers/usb/core/generic.c | 29 +
> include/linux/usb.h| 2 ++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 50f92da8afcf..27ce63ed902d 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -819,13 +819,24 @@ static int usb_device_match(struct device *dev, struct
> device_driver *drv)
> {
> /* devices and interfaces are handled separately */
> if (is_usb_device(dev)) {
> + struct usb_device *udev;
> + struct usb_device_driver *udrv;
>
> /* interface drivers never match devices */
> if (!is_usb_device_driver(drv))
> return 0;
>
> - /* TODO: Add real matching code */
> - return 1;
> + udev = to_usb_device(dev);
> + udrv = to_usb_device_driver(drv);
> +
> + if (udrv->id_table &&
> + usb_device_match_id(udev, udrv->id_table) != NULL) {
> + return 1;
> + }
> +
> + if (udrv->match)
> + return udrv->match(udev);
> + return 0;
What happens if the subclass driver's probe routine returns an error?
Don't you still want the device to be bound to the generic driver?
Alan Stern
[PATCH 4/5] USB: Select better matching USB drivers when available
Now that USB device drivers can reuse code from the generic USB device
driver, we need to make sure that they get selected rather than the
generic driver. Add an id_table and match vfunc to the usb_device_driver
struct, which will get used to select a better matching driver at
->probe time.
This is a similar mechanism to that used in the HID drivers, with the
generic driver being selected unless there's a better matching one found
in the registered drivers (see hid_generic_match() in
drivers/hid/hid-generic.c).
Signed-off-by: Bastien Nocera
---
drivers/usb/core/driver.c | 15 +--
drivers/usb/core/generic.c | 29 +
include/linux/usb.h| 2 ++
3 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 50f92da8afcf..27ce63ed902d 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -819,13 +819,24 @@ static int usb_device_match(struct device *dev, struct
device_driver *drv)
{
/* devices and interfaces are handled separately */
if (is_usb_device(dev)) {
+ struct usb_device *udev;
+ struct usb_device_driver *udrv;
/* interface drivers never match devices */
if (!is_usb_device_driver(drv))
return 0;
- /* TODO: Add real matching code */
- return 1;
+ udev = to_usb_device(dev);
+ udrv = to_usb_device_driver(drv);
+
+ if (udrv->id_table &&
+ usb_device_match_id(udev, udrv->id_table) != NULL) {
+ return 1;
+ }
+
+ if (udrv->match)
+ return udrv->match(udev);
+ return 0;
} else if (is_usb_interface(dev)) {
struct usb_interface *intf;
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 7454c74d43ee..89f9c026a4d1 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -195,6 +195,34 @@ int usb_choose_configuration(struct usb_device *udev)
}
EXPORT_SYMBOL_GPL(usb_choose_configuration);
+static int __check_usb_generic(struct device_driver *drv, void *data)
+{
+ struct usb_device *udev = data;
+ struct usb_device_driver *udrv;
+
+ if (!is_usb_device_driver(drv))
+ return 0;
+ udrv = to_usb_device_driver(drv);
+ if (udrv == &usb_generic_driver)
+ return 0;
+ if (!udrv->id_table)
+ return 0;
+
+ return usb_device_match_id(udev, udrv->id_table) != NULL;
+}
+
+static bool usb_generic_driver_match(struct usb_device *udev)
+{
+/*
+ * If any other driver wants the device, leave the device to this other
+ * driver.
+ */
+if (bus_for_each_drv(&usb_bus_type, NULL, udev, __check_usb_generic))
+return false;
+
+return true;
+}
+
int usb_generic_driver_probe(struct usb_device *udev)
{
int err, c;
@@ -289,6 +317,7 @@ EXPORT_SYMBOL_GPL(usb_generic_driver_resume);
struct usb_device_driver usb_generic_driver = {
.name = "usb",
+ .match = usb_generic_driver_match,
.probe = usb_generic_driver_probe,
.disconnect = usb_generic_driver_disconnect,
#ifdef CONFIG_PM
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 66bd4344e298..df5604f41118 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1236,6 +1236,7 @@ struct usb_driver {
struct usb_device_driver {
const char *name;
+ bool (*match) (struct usb_device *udev);
int (*probe) (struct usb_device *udev);
void (*disconnect) (struct usb_device *udev);
@@ -1243,6 +1244,7 @@ struct usb_device_driver {
int (*resume) (struct usb_device *udev, pm_message_t message);
const struct attribute_group **dev_groups;
struct usbdrv_wrap drvwrap;
+ const struct usb_device_id *id_table;
unsigned int supports_autosuspend:1;
unsigned int generic_init:1;
};
--
2.21.0
