Re: [PATCH v4 2/2] usb: misc: onboard_usb_dev: don't defer probing after the device is configured

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: onboard_usb_dev: -ENODEV fallback leaves device on
  deferred_probe_pending_list
--

commit 8a540d2b1c8688c36c28e760d6d5507f9a82ef03
Author: James Clark <jjc@jclark.com>

usb: misc: onboard_usb_dev: don't defer probing after the device is configured

This commit removes generic_subclass from onboard_dev_usbdev_driver and calls
the generic USB driver operations directly so the USB device is not
configured before probe deferral.  It also returns -ENODEV instead of -EINVAL
when no peer-hub node is found so the device falls back to the generic
driver.

> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index c604062404162..51985e3473ad8 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -591,7 +591,7 @@ static struct onboard_dev *_find_onboard_dev(struct device *dev)
>  		np = of_parse_phandle(dev->of_node, "peer-hub", 0);
>  		if (!np) {
>  			dev_err(dev, "failed to find device node for peer hub\n");
> -			return ERR_PTR(-EINVAL);
> +			return ERR_PTR(-ENODEV);
>  		}
>  
>  		pdev = of_find_device_by_node(np);
> @@ -639,7 +639,11 @@ static int onboard_dev_usbdev_probe(struct usb_device *udev)
>  	if (err)
>  		return err;
>  
> -	return 0;
> +	err = usb_generic_driver_probe(udev);
> +	if (err)
> +		onboard_dev_remove_usbdev(onboard_dev, udev);
> +
> +	return err;
>  }
>  
>  static void onboard_dev_usbdev_disconnect(struct usb_device *udev)
> @@ -647,6 +651,7 @@ static void onboard_dev_usbdev_disconnect(struct usb_device *udev)
>  	struct onboard_dev *onboard_dev = dev_get_drvdata(&udev->dev);
>  
>  	onboard_dev_remove_usbdev(onboard_dev, udev);
> +	usb_generic_driver_disconnect(udev);
>  }
>  
>  static const struct usb_device_id onboard_dev_id_table[] = {
> @@ -697,7 +702,10 @@ static struct usb_device_driver onboard_dev_usbdev_driver = {
>  	.match = onboard_dev_usbdev_match,
>  	.probe = onboard_dev_usbdev_probe,
>  	.disconnect = onboard_dev_usbdev_disconnect,
> -	.generic_subclass = 1,
> +#ifdef CONFIG_PM
> +	.suspend = usb_generic_driver_suspend,
> +	.resume = usb_generic_driver_resume,
> +#endif
>  	.supports_autosuspend =	1,
>  	.id_table = onboard_dev_id_table,
>  };

[Severity: Medium]
Can returning -ENODEV from _find_onboard_dev() leave udev unconfigured on
deferred_probe_pending_list instead of falling back to usb_generic_driver?

During __device_attach(), bus_for_each_drv() visits usb_generic_driver
before onboard_dev_usbdev_driver, where usb_generic_driver_match() returns
false because udev->use_generic_driver is still 0.  When
onboard_dev_usbdev_probe() then fails with -ENODEV, usb_probe_device() sets
udev->use_generic_driver = 1 and converts the error to -EPROBE_DEFER:

drivers/usb/core/driver.c:usb_probe_device() {
    ...
	if (error == -ENODEV && udriver != &usb_generic_driver &&
	    (udriver->id_table || udriver->match)) {
		udev->use_generic_driver = 1;
		return -EPROBE_DEFER;
	}
    ...
}

Previously, generic_subclass caused usb_generic_driver_probe() to run first
and bind child interfaces, which incremented deferred_trigger_count so
driver_probe_device() immediately re-triggered deferred probing.  With
generic_subclass removed, no device binds during probe, so
driver_probe_device() adds udev to deferred_probe_pending_list without
calling driver_deferred_probe_trigger().  Unless another device happens to
bind later, deferred_probe_work is not queued and udev stays unconfigured.

Also, if driver_attach() runs for onboard_dev_usbdev_driver, neither
usb_driver_applicable() nor onboard_dev_usbdev_match() checks
udev->use_generic_driver, so it matches onboard_dev_usbdev_driver again and
repeats the -ENODEV -> -EPROBE_DEFER return without triggering deferred
probing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793941292.git.jjc@jclark.com?part=2