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

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

[James Clark]

On a BeagleY-AI with BeagleBoard's device tree, kernels since commit
f918e3804498 ("arm64: enable HAVE_SHARED_GPIOS for all ARM64 platforms")
no longer boot to a usable system. The log repeats "hub 1-1:1.0: USB hub
found" for the on-board TUSB8041 hub every few milliseconds, with no USB
disconnect in between. The flood of uevents stalls udev and the boot ends
in emergency mode. Reverting that commit makes the board boot normally
again.

Both peer hub nodes in this device tree carry the same reset-gpios, so
the GPIO is now a shared one, and with CONFIG_GPIO_SHARED_PROXY=m (the
default) the hub's platform probe is legitimately deferred until udev has
loaded that module. The bootloader leaves the hub running, so the USB
core enumerates it long before, and the USB device probe finds the
platform device without driver data.

At that point, onboard_dev_usbdev_probe() returns -EPROBE_DEFER. This is
what causes the problem. The driver has generic_subclass set, so
usb_probe_device() has already run the generic probe, which configured
the device and bound the interface drivers. Each binding is a
deferred-probe trigger, and the driver core re-triggers when a trigger
happened during a probe that then deferred. So the device is re-probed
straight away, reconfigured, its interfaces bound again, and so on for
as long as the platform device remains unbound. A probe must not defer
after it has registered child devices, and with generic_subclass the
driver's probe has no way to defer before that. This is not specific to
shared GPIOs: it happens whenever the platform probe is deferred, and
briefly whenever onboard_usb_dev is a module.

Add a pre_probe callback to struct usb_device_driver, called before the
generic probe, so that a driver that may have to defer can do so before
the device is configured. -EPROBE_DEFER from it ends the probe with
nothing done. Any other error is handled as if probe() had returned it,
once the generic probe has run, so the -ENODEV fallback to the generic
driver works as before. Make onboard_usb_dev's USB device probe a
pre_probe; it does not need the device to be configured.

A device whose driver defers from pre_probe stays unconfigured until the
probe succeeds, and for good if it never does, e.g. an onboard hub whose
platform driver fails to probe. Before, such a device worked while being
re-probed. On the BeagleY-AI the hub is unconfigured for the few seconds
until gpio-shared-proxy has loaded and the platform driver has probed,
which resets it anyway; the board then boots normally with that commit
in place.

Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
Suggested-by: Johan Hovold <johan@kernel.org>
Signed-off-by: James Clark <jjc@jclark.com>
---
I found this by booting commit 85b4bc79dde7 (net/main during the 7.4
merge window) on a BeagleY-AI with BeagleBoard's device tree. A coding
agent identified the problem, explained it to me and proposed an initial
fix, which was revised multiple times on the basis of local Sashiko
reviews. I verified that the patch fixed the problem on top of commit
d89e1d186ec9.

v2: add a pre_probe callback in the USB core, as suggested by Johan
Hovold.
v1: https://lore.kernel.org/all/20261030114705.3159093-1-jjc@jclark.com/

#regzbot introduced: f918e3804498

 drivers/usb/core/driver.c          | 38 ++++++++++++++++++++++++++----
 drivers/usb/misc/onboard_usb_dev.c | 15 ++++++------
 include/linux/usb.h                | 14 +++++++++++
 3 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index d3a61ebc6afb..724cfa9da4e0 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;
 }
 
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index c60406240416..33653d8f76d7 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -606,13 +606,14 @@ static struct onboard_dev *_find_onboard_dev(struct device *dev)
 
 	/*
 	 * The presence of drvdata indicates that the platform driver finished
-	 * probing. This handles the case where (conceivably) we could be
-	 * running at the exact same time as the platform driver's probe. If
-	 * we detect the race we request probe deferral and we'll come back and
-	 * try again.
+	 * probing. Without it, e.g. because the platform driver's probe was
+	 * deferred or is running right now, request probe deferral and we'll
+	 * come back and try again. This is allowed because we run from
+	 * pre_probe(), before the USB core has configured the device.
 	 */
 	if (!onboard_dev)
-		return ERR_PTR(-EPROBE_DEFER);
+		return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
+					     "platform device not bound\n"));
 
 	return onboard_dev;
 }
@@ -623,7 +624,7 @@ static bool onboard_dev_usbdev_match(struct usb_device *udev)
 	return !!udev->dev.of_node;
 }
 
-static int onboard_dev_usbdev_probe(struct usb_device *udev)
+static int onboard_dev_usbdev_pre_probe(struct usb_device *udev)
 {
 	struct device *dev = &udev->dev;
 	struct onboard_dev *onboard_dev;
@@ -695,7 +696,7 @@ MODULE_DEVICE_TABLE(usb, onboard_dev_id_table);
 static struct usb_device_driver onboard_dev_usbdev_driver = {
 	.name = "onboard-usb-dev",
 	.match = onboard_dev_usbdev_match,
-	.probe = onboard_dev_usbdev_probe,
+	.pre_probe = onboard_dev_usbdev_pre_probe,
 	.disconnect = onboard_dev_usbdev_disconnect,
 	.generic_subclass = 1,
 	.supports_autosuspend =	1,
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 825f03b29067..6e07d485d49a 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1280,6 +1280,19 @@ struct usb_driver {
  * @name: The driver name should be unique among USB drivers,
  *	and should normally be the same as the module name.
  * @match: If set, used for better device/driver matching.
+ * @pre_probe: If set, called first, before the device is resumed or
+ *	configured (the generic USB driver's probe runs after it when
+ *	@generic_subclass is set, then @probe), so it must not access the
+ *	device. A driver that may have to defer probing must do so from here
+ *	by returning -EPROBE_DEFER, not from @probe: by then the generic probe
+ *	has bound the interface drivers, and each binding triggers deferred
+ *	probing, so the device would be re-probed in a loop. Returning
+ *	-EPROBE_DEFER ends the probe with the device left as it was, so the
+ *	device stays unconfigured until the probe succeeds. Any other error is
+ *	handled as if @probe had returned it, once the generic probe has run,
+ *	and @probe is then not called. If @pre_probe succeeds and a later step
+ *	of the probe fails, @disconnect is called to undo it, so a driver with
+ *	both callbacks must make @disconnect cope with a failed @probe.
  * @probe: Called to see if the driver is willing to manage a particular
  *	device.  If it is, probe returns zero and uses dev_set_drvdata()
  *	to associate driver-specific data with the device.  If unwilling
@@ -1309,6 +1322,7 @@ struct usb_device_driver {
 	const char *name;
 
 	bool (*match) (struct usb_device *udev);
+	int (*pre_probe) (struct usb_device *udev);
 	int (*probe) (struct usb_device *udev);
 	void (*disconnect) (struct usb_device *udev);
 

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

[sashiko-bot]

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