Re: [PATCH v4] usb: ohci-omap: implement overcurrent handling for GPIO-based power control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: ohci-omap: race and inverted ordering in ohci_omap_overcurrent_irq() leaves VBUS on
- [High] usb: ohci-omap: overcurrent IRQ fails to resume suspended root hub
- [Medium] usb: ohci-omap: race and missed falling edge in probe overcurrent init
--

commit 2a3f441c427f55a1886bf324043fa831b260b3d4
Author: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>

usb: ohci-omap: implement overcurrent handling for GPIO-based power control

This commit implements overcurrent detection and VBUS power control via
GPIOs in the OMAP OHCI driver using a threaded interrupt handler and custom
root hub status and control callbacks.

> diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> index f6e56c4b99148..c717212439053 100644
> --- a/drivers/usb/host/ohci-omap.c
> +++ b/drivers/usb/host/ohci-omap.c

[ ... ]

> @@ -181,6 +195,128 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
>  	return 0;
>  }
>  
> +static irqreturn_t ohci_omap_overcurrent_irq(int irq, void *data)
> +{
> +	struct usb_hcd *hcd = data;
> +	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
> +	int status;
> +	bool was_active;
> +
> +	status = gpiod_get_value_cansleep(priv->overcurrent);
> +	if (status < 0)
> +		return IRQ_HANDLED;
> +
> +	if (status && priv->power)
> +		gpiod_set_value_cansleep(priv->power, 1);
> +
> +	was_active = READ_ONCE(priv->overcurrent_active);
> +	WRITE_ONCE(priv->overcurrent_active, !!status);
> +	if (was_active != !!status)
> +		WRITE_ONCE(priv->overcurrent_change_pending, true);
> +
> +	usb_hcd_poll_rh_status(hcd);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ohci_omap_hub_status_data(struct usb_hcd *hcd, char *buf)
> +{
> +	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
> +	int length;
> +
> +	length = ohci_hub_status_data(hcd, buf);
> +
> +	if (!priv->overcurrent)
> +		return length;
> +
> +	if (READ_ONCE(priv->overcurrent_change_pending)) {
> +		if (!length) {
> +			buf[0] = 0;
> +			length = 1;
> +		}
> +		buf[0] |= BIT(1); /* port 1 */
> +	}
> +
> +	return length;
> +}

[Severity: High]
What happens if an overcurrent interrupt fires while the root hub is
runtime-suspended or autostopped?

When the root hub is suspended, hub_suspend() -> hub_quiesce() kills
hub->urb (leaving hcd->status_urb as NULL) and ohci_bus_suspend() places the
controller into OHCI_USB_SUSPEND.

When ohci_omap_overcurrent_irq() cuts VBUS and calls
usb_hcd_poll_rh_status(), that calls ohci_omap_hub_status_data() ->
ohci_hub_status_data(). Because the overcurrent signal is on an external
GPIO rather than the OHCI hardware pin, ohci_hub_status_data() sees no
hardware port change bits (changed == 0), so ohci_root_hub_state_changes()
does not call ohci_rh_resume() or usb_hcd_resume_root_hub():

drivers/usb/host/ohci-hub.c:ohci_root_hub_state_changes() {
	...
	case OHCI_USB_SUSPEND:
	case OHCI_USB_RESUME:
		/* if there is a port change, autostart or ask to be resumed */
		if (changed) {
			if (ohci->autostop)
				ohci_rh_resume(ohci);
			else
				usb_hcd_resume_root_hub(ohci_to_hcd(ohci));
	...
}

Then, even though ohci_omap_hub_status_data() returns length = 1,
usb_hcd_poll_rh_status() sees that hcd->status_urb is NULL, so it only sets
HCD_FLAG_POLL_PENDING and returns without waking hub_wq:

drivers/usb/core/hcd.c:usb_hcd_poll_rh_status() {
	...
		urb = hcd->status_urb;
		if (urb) {
			...
		} else {
			length = 0;
			set_bit(HCD_FLAG_POLL_PENDING, &hcd->flags);
		}
	...
}

With VBUS turned off and the root hub still suspended, won't the port remain
unpowered indefinitely because hub_wq never runs port_event() to clear the
overcurrent change and restore power?

> +
> +static int ohci_omap_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> +				 u16 wIndex, char *buf, u16 wLength)
> +{
> +	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
> +	int ret;
> +
> +	/*
> +	 * Drive the VBUS power GPIO alongside the OHCI hardware register.
> +	 * On boards like the OSK, the OHCI PPS bit does not control the
> +	 * real power switch — the GPIO does.
> +	 */
> +	if (priv->power && wIndex == 1) {
> +		if (typeReq == SetPortFeature &&
> +		    wValue == USB_PORT_FEAT_POWER) {
> +			ret = ohci_hub_control(hcd, typeReq, wValue, wIndex,
> +					       buf, wLength);
> +			if (!ret && !READ_ONCE(priv->overcurrent_active))
> +				gpiod_set_value_cansleep(priv->power, 0);
> +			/*
> +			 * Re-check: the overcurrent IRQ may have fired
> +			 * between the check above and the GPIO call.
> +			 * A mutex would close this fully, but the window
> +			 * is microseconds and the hardware current limiter
> +			 * provides the final safety net.
> +			 */
> +			if (!ret && READ_ONCE(priv->overcurrent_active))
> +				gpiod_set_value_cansleep(priv->power, 1);
> +			return ret;
> +		}

[Severity: High]
Can a concurrent SetPortFeature(USB_PORT_FEAT_POWER) in
ohci_omap_hub_control() race with ohci_omap_overcurrent_irq() and leave VBUS
switched on during an active overcurrent condition?

In ohci_omap_overcurrent_irq(), gpiod_set_value_cansleep(priv->power, 1) is
called before WRITE_ONCE(priv->overcurrent_active, !!status). Because
priv->power is on an I2C-attached TPS65010 expander on OMAP1 OSK, that GPIO
write sleeps across an I2C transfer.

If ohci_omap_hub_control() runs while ohci_omap_overcurrent_irq() is
sleeping in gpiod_set_value_cansleep(priv->power, 1) (or before the IRQ
thread is scheduled to run WRITE_ONCE(priv->overcurrent_active, true)),
ohci_omap_hub_control() can observe priv->overcurrent_active as false on
both checks and drive priv->power back to 0. When
ohci_omap_overcurrent_irq() finishes and sets priv->overcurrent_active to
true, it has already completed its GPIO write, leaving VBUS enabled while
priv->overcurrent_active is true.

Even if WRITE_ONCE(priv->overcurrent_active, !!status) were moved before the
GPIO write in ohci_omap_overcurrent_irq(), if ohci_omap_hub_control() passes
the initial !READ_ONCE(priv->overcurrent_active) check and is preempted
while an overcurrent event asserts, cuts VBUS, and then deasserts once
current stops flowing, priv->overcurrent_active will return to false and
ohci_omap_hub_control() will turn VBUS back on and pass the re-check without
the hub core authorizing recovery. Would a mutex serializing the GPIO writes
and overcurrent state updates avoid this?

[ ... ]

> @@ -277,23 +412,73 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
>  	if (!hcd->regs) {
>  		dev_err(&pdev->dev, "can't ioremap OHCI HCD\n");
>  		retval = -ENOMEM;
> -		goto err2;
> +		goto err_release_mem;
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
>  		retval = irq;
> -		goto err3;
> +		goto err_iounmap;
>  	}
>  	retval = usb_add_hcd(hcd, irq, 0);
>  	if (retval)
> -		goto err3;
> +		goto err_iounmap;
> +
> +	if (priv->overcurrent) {
> +		int status;
> +
> +		priv->overcurrent_irq = gpiod_to_irq(priv->overcurrent);
> +		if (priv->overcurrent_irq < 0) {
> +			retval = priv->overcurrent_irq;
> +			goto err_remove_hcd;
> +		}
> +
> +		retval = request_threaded_irq(
> +			priv->overcurrent_irq, NULL, ohci_omap_overcurrent_irq,
> +			IRQF_ONESHOT | IRQF_NO_AUTOEN | IRQF_TRIGGER_RISING |
> +				IRQF_TRIGGER_FALLING,
> +			"ohci-omap-overcurrent", hcd);
> +		if (retval) {
> +			priv->overcurrent_irq = -1;
> +			goto err_remove_hcd;
> +		}
> +
> +		status = gpiod_get_value_cansleep(priv->overcurrent);
> +		if (status < 0) {
> +			retval = status;
> +			goto err_remove_hcd;
> +		}
> +
> +		WRITE_ONCE(priv->overcurrent_active, !!status);
> +		WRITE_ONCE(priv->overcurrent_change_pending, !!status);
> +
> +		if (status && priv->power)
> +			gpiod_set_value_cansleep(priv->power, 1);
> +
> +		enable_irq(priv->overcurrent_irq);
> +		if (status)
> +			usb_hcd_poll_rh_status(hcd);
> +	}

[Severity: Medium]
In ohci_hcd_omap_probe(), if status is non-zero at probe time, could cutting
VBUS power before calling enable_irq(priv->overcurrent_irq) cause the
falling edge on priv->overcurrent to be missed?

When gpiod_set_value_cansleep(priv->power, 1) turns off VBUS (which sleeps
on an I2C GPIO expander), the power switch can deassert its overcurrent line
(1 -> 0) before enable_irq(priv->overcurrent_irq) is called. Because
priv->overcurrent_irq was requested with IRQF_NO_AUTOEN, that falling edge
occurs while the interrupt is still disabled, leaving
priv->overcurrent_active stuck at true and preventing subsequent
SetPortFeature(USB_PORT_FEAT_POWER) requests from ever re-enabling VBUS.

Also, since usb_add_hcd() calls ohci_omap_reset() (which enables VBUS) and
registers the root hub before priv->overcurrent_irq is requested, hub_wq can
already be running ohci_omap_hub_control() concurrently with this
initialization block.

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018223312.120913-1-ampr.mhd@gmail.com?part=1