Re: [PATCH v2] usb: core: add a pre_probe callback for device drivers that may defer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: core: missing generic and runtime PM cleanup in usb_probe_device()
--

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

usb: core: add a pre_probe callback for device drivers that may defer

This commit adds a pre_probe callback to struct usb_device_driver so that
device drivers with generic_subclass set can defer probing before
usb_generic_driver_probe() configures the device and binds its interfaces.
It also converts onboard_usb_dev to use pre_probe to avoid a re-probe loop
when the companion platform device has not finished probing.

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index d3a61ebc6afb0..724cfa9da4e01 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -249,24 +249,41 @@ static int usb_probe_device(struct device *dev)
>  {
>  	struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
>  	struct usb_device *udev = to_usb_device(dev);
> -	int error = 0;
> +	int error = 0, pre_error = 0;
>  
>  	dev_dbg(dev, "%s\n", __func__);
>  
>  	/* TODO: Add real matching code */
>  
> +	/*
> +	 * Give the driver a chance to defer before anything is done to the
> +	 * device. The generic probe below binds the interface drivers, and
> +	 * each binding triggers deferred probing, so a probe that deferred
> +	 * after it would be retried straight away and the device re-probed
> +	 * in a loop.
> +	 *
> +	 * Any other error from pre_probe() is not acted on until the generic
> +	 * probe has run, so that the -ENODEV fallback below works as it does
> +	 * for probe().
> +	 */
> +	if (udriver->pre_probe) {
> +		pre_error = udriver->pre_probe(udev);
> +		if (pre_error == -EPROBE_DEFER)
> +			return pre_error;
> +	}
> +
>  	/* The device should always appear to be in use
>  	 * unless the driver supports autosuspend.
>  	 */
>  	if (!udriver->supports_autosuspend)
>  		error = usb_autoresume_device(udev);
>  	if (error)
> -		return error;
> +		goto err_pre_probe;
>  
>  	if (udriver->generic_subclass)
>  		error = usb_generic_driver_probe(udev);
>  	if (error)
> -		return error;
> +		goto err_pre_probe;
>  
>  	/* Probe the USB device with the driver in hand, but only
>  	 * defer to a generic driver in case the current USB
> @@ -289,15 +306,26 @@ static int usb_probe_device(struct device *dev)
>  	 * specialised device drivers prior to setting the
>  	 * use_generic_driver bit.
>  	 */
> -	if (udriver->probe)
> +	if (pre_error)
> +		error = pre_error;
> +	else if (udriver->probe)
>  		error = udriver->probe(udev);
>  	else if (!udriver->generic_subclass)
>  		error = -EINVAL;
>  	if (error == -ENODEV && udriver != &usb_generic_driver &&
>  	    (udriver->id_table || udriver->match)) {
>  		udev->use_generic_driver = 1;
> -		return -EPROBE_DEFER;
> +		error = -EPROBE_DEFER;
>  	}
> +	if (error)
> +		goto err_pre_probe;
> +
> +	return 0;
> +
> +err_pre_probe:
> +	/* The driver core doesn't call disconnect() for a failed probe. */
> +	if (udriver->pre_probe && !pre_error && udriver->disconnect)
> +		udriver->disconnect(udev);
>  	return error;
>  }

[Severity: Medium]
When udriver->pre_probe(udev) returns a non-deferral error (for example,
-EINVAL or -ENOMEM from onboard_dev_usbdev_pre_probe() when
_find_onboard_dev() or onboard_dev_add_usbdev() fails), or when pre_probe()
succeeds and udriver->probe(udev) fails, usb_probe_device() runs
usb_autoresume_device(udev) and usb_generic_driver_probe(udev) before jumping
to err_pre_probe.

However, err_pre_probe only invokes udriver->disconnect(udev) and does not
undo usb_generic_driver_probe(udev) or usb_autoresume_device(udev) the way
usb_unbind_device() does:

drivers/usb/core/driver.c:usb_unbind_device() {
	if (udriver->disconnect)
		udriver->disconnect(udev);
	if (udriver->generic_subclass)
		usb_generic_driver_disconnect(udev);
	if (!udriver->supports_autosuspend)
		usb_autosuspend_device(udev);
}

Because the driver core does not call usb_unbind_device() when
usb_probe_device() fails, doesn't a non-deferral failure leave udev unbound
(udev->dev.driver == NULL) while still configured with bound child
interfaces, an unbalanced runtime PM usage count when !supports_autosuspend,
and a sent USB_DEVICE_ADD notification that will never be paired with
USB_DEVICE_REMOVE on unplug (leaking struct usbport_trig_port and sysfs
attributes in usbport_trig_notify())?

Similarly, on the -ENODEV fallback path where udev->use_generic_driver is set
to 1 and usb_probe_device() returns -EPROBE_DEFER, won't leaving
usb_generic_driver_probe(udev) and usb_autoresume_device(udev) in place cause
the subsequent usb_generic_driver probe to call usb_generic_driver_probe() a
second time and emit a duplicate USB_DEVICE_ADD notification without an
intervening USB_DEVICE_REMOVE?

Should pre_probe() handle the -ENODEV fallback and return immediately on any
error before touching the device, and should the error path after
usb_generic_driver_probe() and usb_autoresume_device() also unwind them via
usb_generic_driver_disconnect() and usb_autosuspend_device()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030151919.3511106-1-jjc@jclark.com?part=1