[PATCH v3] usb: ohci-omap: handle root hub overcurrent via GPIO

[PATCH v3] usb: ohci-omap: handle root hub overcurrent via GPIO

[Muhammad Mahdi Amirpour]

Implement proper overcurrent handling for the OMAP OHCI root hub using
the optional overcurrent GPIO line that was previously unused.

When an overcurrent condition is detected via GPIO interrupt, the driver
now disables VBUS power through the power GPIO, records the overcurrent
status, and reports it through the root hub port status callbacks so the
USB hub core can react appropriately.

The hub_control override also handles SetPortFeature/ClearPortFeature
for USB_PORT_FEAT_POWER to drive the VBUS power GPIO alongside the OHCI
hardware register, since on boards like the OMAP1 OSK the OHCI PPS bit
does not control the real power switch.

Signed-off-by: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>
---
v3:
  - Fix power GPIO polarity: 1 = VBUS off, 0 = VBUS on
  - Fix wPortChange bit: shift USB_PORT_STAT_C_OVERCURRENT by 16
  - Add clock and PHY cleanup to probe error path
  - Use IRQF_NO_AUTOEN to close race window during probe
  - Only set overcurrent_changed at probe when overcurrent is active
  - Guard hub_status_data and hub_control behind priv->overcurrent
  - Handle SetPortFeature/ClearPortFeature(POWER) for VBUS GPIO
  - Remove shadowed flags variable in hub_control
  - Add comment documenting overcurrent_lock

v2:
  - Use real name in From: and Signed-off-by: lines
---
 drivers/usb/host/ohci-omap.c | 249 ++++++++++++++++++++++++++++++++---
 1 file changed, 228 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index f6e56c4b9914..a3ff50c70d41 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -15,6 +15,7 @@
  * This file is licenced under the GPL.
  */
 
+#include <linux/irqreturn.h>
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
@@ -33,6 +34,9 @@
 #include <linux/signal.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/usb/ch11.h>
 
 #include "ohci.h"
 
@@ -46,6 +50,10 @@ struct ohci_omap_priv {
 	struct clk *usb_dc_ck;
 	struct gpio_desc *power;
 	struct gpio_desc *overcurrent;
+	spinlock_t overcurrent_lock;	/* protects overcurrent_status and overcurrent_changed */
+	bool overcurrent_status;
+	bool overcurrent_changed;
+	int overcurrent_irq;
 };
 
 static const char hcd_name[] = "ohci-omap";
@@ -181,11 +189,148 @@ 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);
+	unsigned long flags;
+	int status;
+
+	status = gpiod_get_value_cansleep(priv->overcurrent);
+	if (status < 0)
+		return IRQ_HANDLED;
+
+	if (status && priv->power)
+		gpiod_set_value_cansleep(priv->power, 1);
+
+	spin_lock_irqsave(&priv->overcurrent_lock, flags);
+	if (!!status != priv->overcurrent_status)
+		priv->overcurrent_changed = true;
+	priv->overcurrent_status = !!status;
+	spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
+
+	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);
+	unsigned long flags;
+	bool changed;
+	int length;
+
+	length = ohci_hub_status_data(hcd, buf);
+
+	if (!priv->overcurrent)
+		return length;
+
+	spin_lock_irqsave(&priv->overcurrent_lock, flags);
+	changed = priv->overcurrent_changed;
+	spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
+
+	if (changed) {
+		if (!length) {
+			buf[0] = 0;
+			length = 1;
+		}
+		buf[0] |= BIT(1);
+	}
+
+	return length;
+}
+
+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);
+	unsigned long flags;
+	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) {
+				spin_lock_irqsave(&priv->overcurrent_lock,
+						  flags);
+				if (!priv->overcurrent_status)
+					gpiod_set_value_cansleep(priv->power,
+								 0);
+				spin_unlock_irqrestore(&priv->overcurrent_lock,
+						       flags);
+			}
+			return ret;
+		}
+		if (typeReq == ClearPortFeature &&
+		    wValue == USB_PORT_FEAT_POWER) {
+			ret = ohci_hub_control(hcd, typeReq, wValue,
+					       wIndex, buf, wLength);
+			if (!ret && priv->power)
+				gpiod_set_value_cansleep(priv->power, 1);
+			return ret;
+		}
+	}
+
+	ret = ohci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+	if (ret)
+		return ret;
+
+	if (!priv->overcurrent)
+		return ret;
+
+	if (typeReq == ClearPortFeature && wIndex == 1 &&
+	    wValue == USB_PORT_FEAT_C_OVER_CURRENT) {
+		spin_lock_irqsave(&priv->overcurrent_lock, flags);
+		priv->overcurrent_changed = false;
+		spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
+		return 0;
+	}
+
+	if (typeReq != GetPortStatus || wIndex != 1)
+		return ret;
+
+	{
+		bool status, changed;
+		__le32 *portstatus;
+		u32 value;
+
+		spin_lock_irqsave(&priv->overcurrent_lock, flags);
+		status = priv->overcurrent_status;
+		changed = priv->overcurrent_changed;
+		spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
+
+		portstatus = (__le32 *)buf;
+		value = le32_to_cpu(*portstatus);
+
+		if (status)
+			value |= USB_PORT_STAT_OVERCURRENT;
+		else
+			value &= ~USB_PORT_STAT_OVERCURRENT;
+
+		if (changed)
+			value |= (USB_PORT_STAT_C_OVERCURRENT << 16);
+		else
+			value &= ~(USB_PORT_STAT_C_OVERCURRENT << 16);
+
+		*portstatus = cpu_to_le32(value);
+	}
+
+	return 0;
+}
+
 /*-------------------------------------------------------------------------*/
 
 /**
  * ohci_hcd_omap_probe - initialize OMAP-based HCDs
- * @pdev:	USB controller to probe
+ * @pdev: USB controller to probe
  *
  * Context: task context, might sleep
  *
@@ -196,50 +341,53 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 static int ohci_hcd_omap_probe(struct platform_device *pdev)
 {
 	int retval, irq;
-	struct usb_hcd *hcd = 0;
+	struct usb_hcd *hcd;
 	struct ohci_omap_priv *priv;
 
 	if (pdev->num_resources != 2) {
 		dev_err(&pdev->dev, "invalid num_resources: %i\n",
-		       pdev->num_resources);
+			pdev->num_resources);
 		return -ENODEV;
 	}
 
-	if (pdev->resource[0].flags != IORESOURCE_MEM
-			|| pdev->resource[1].flags != IORESOURCE_IRQ) {
+	if (pdev->resource[0].flags != IORESOURCE_MEM ||
+	    pdev->resource[1].flags != IORESOURCE_IRQ) {
 		dev_err(&pdev->dev, "invalid resource type\n");
 		return -ENODEV;
 	}
 
 	hcd = usb_create_hcd(&ohci_omap_hc_driver, &pdev->dev,
-			dev_name(&pdev->dev));
+			     dev_name(&pdev->dev));
 	if (!hcd)
 		return -ENOMEM;
 
 	hcd->rsrc_start = pdev->resource[0].start;
-	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
+	hcd->rsrc_len = pdev->resource[0].end -
+			pdev->resource[0].start + 1;
 	priv = hcd_to_ohci_omap_priv(hcd);
 
-	/* Obtain two optional GPIO lines */
-	priv->power = devm_gpiod_get_optional(&pdev->dev, "power", GPIOD_ASIS);
+	spin_lock_init(&priv->overcurrent_lock);
+	priv->overcurrent_status = false;
+	priv->overcurrent_changed = false;
+	priv->overcurrent_irq = -1;
+
+	priv->power = devm_gpiod_get_optional(&pdev->dev, "power",
+					      GPIOD_ASIS);
 	if (IS_ERR(priv->power)) {
 		retval = PTR_ERR(priv->power);
 		goto err_put_hcd;
 	}
+
 	if (priv->power)
 		gpiod_set_consumer_name(priv->power, "OHCI power");
 
-	/*
-	 * This "overcurrent" GPIO line isn't really used in the code,
-	 * but has a designated hardware function.
-	 * TODO: implement proper overcurrent handling.
-	 */
-	priv->overcurrent = devm_gpiod_get_optional(&pdev->dev, "overcurrent",
-						    GPIOD_IN);
+	priv->overcurrent = devm_gpiod_get_optional(&pdev->dev,
+						    "overcurrent", GPIOD_IN);
 	if (IS_ERR(priv->overcurrent)) {
 		retval = PTR_ERR(priv->overcurrent);
 		goto err_put_hcd;
 	}
+
 	if (priv->overcurrent)
 		gpiod_set_consumer_name(priv->overcurrent, "OHCI overcurrent");
 
@@ -277,23 +425,76 @@ 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;
+		unsigned long flags;
+
+		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;
+		}
+
+		spin_lock_irqsave(&priv->overcurrent_lock, flags);
+		priv->overcurrent_status = !!status;
+		priv->overcurrent_changed = !!status;
+		spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
+
+		if (status && priv->power)
+			gpiod_set_value_cansleep(priv->power, 1);
+
+		enable_irq(priv->overcurrent_irq);
+		if (status)
+			usb_hcd_poll_rh_status(hcd);
+	}
 
 	device_wakeup_enable(hcd->self.controller);
 	return 0;
-err3:
+
+err_remove_hcd:
+	if (priv->overcurrent_irq >= 0) {
+		free_irq(priv->overcurrent_irq, hcd);
+		priv->overcurrent_irq = -1;
+	}
+	usb_remove_hcd(hcd);
+	omap_ohci_clock_power(priv, 0);
+	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {
+		(void)otg_set_host(hcd->usb_phy->otg, NULL);
+		usb_put_phy(hcd->usb_phy);
+	}
+err_iounmap:
 	iounmap(hcd->regs);
-err2:
+err_release_mem:
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 err_unprepare_dc_ck:
 	clk_unprepare(priv->usb_dc_ck);
@@ -327,6 +528,10 @@ static void ohci_hcd_omap_remove(struct platform_device *pdev)
 	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 
 	dev_dbg(hcd->self.controller, "stopping USB Controller\n");
+	if (priv->overcurrent_irq >= 0) {
+		free_irq(priv->overcurrent_irq, hcd);
+		priv->overcurrent_irq = -1;
+	}
 	usb_remove_hcd(hcd);
 	omap_ohci_clock_power(priv, 0);
 	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {
@@ -413,6 +618,8 @@ static int __init ohci_omap_init(void)
 		return -ENODEV;
 
 	ohci_init_driver(&ohci_omap_hc_driver, &omap_overrides);
+	ohci_omap_hc_driver.hub_status_data = ohci_omap_hub_status_data;
+	ohci_omap_hc_driver.hub_control = ohci_omap_hub_control;
 	return platform_driver_register(&ohci_hcd_omap_driver);
 }
 module_init(ohci_omap_init);
-- 
2.55.0

Re: [PATCH v3] usb: ohci-omap: handle root hub overcurrent via GPIO

[Greg Kroah-Hartman]

On Sun, Oct 18, 2026 at 07:23:24PM +0330, Muhammad Mahdi Amirpour wrote:
> Implement proper overcurrent handling for the OMAP OHCI root hub using
> the optional overcurrent GPIO line that was previously unused.
> 
> When an overcurrent condition is detected via GPIO interrupt, the driver
> now disables VBUS power through the power GPIO, records the overcurrent
> status, and reports it through the root hub port status callbacks so the
> USB hub core can react appropriately.
> 
> The hub_control override also handles SetPortFeature/ClearPortFeature
> for USB_PORT_FEAT_POWER to drive the VBUS power GPIO alongside the OHCI
> hardware register, since on boards like the OMAP1 OSK the OHCI PPS bit
> does not control the real power switch.
> 
> Signed-off-by: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>
> ---
> v3:
>   - Fix power GPIO polarity: 1 = VBUS off, 0 = VBUS on
>   - Fix wPortChange bit: shift USB_PORT_STAT_C_OVERCURRENT by 16
>   - Add clock and PHY cleanup to probe error path
>   - Use IRQF_NO_AUTOEN to close race window during probe
>   - Only set overcurrent_changed at probe when overcurrent is active
>   - Guard hub_status_data and hub_control behind priv->overcurrent
>   - Handle SetPortFeature/ClearPortFeature(POWER) for VBUS GPIO
>   - Remove shadowed flags variable in hub_control
>   - Add comment documenting overcurrent_lock
> 
> v2:
>   - Use real name in From: and Signed-off-by: lines
> ---
>  drivers/usb/host/ohci-omap.c | 249 ++++++++++++++++++++++++++++++++---
>  1 file changed, 228 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> index f6e56c4b9914..a3ff50c70d41 100644
> --- a/drivers/usb/host/ohci-omap.c
> +++ b/drivers/usb/host/ohci-omap.c
> @@ -15,6 +15,7 @@
>   * This file is licenced under the GPL.
>   */
>  
> +#include <linux/irqreturn.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
> @@ -33,6 +34,9 @@
>  #include <linux/signal.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <linux/interrupt.h>
> +#include <linux/spinlock.h>
> +#include <linux/usb/ch11.h>

Why the odd include order?  Please be more careful.

>  #include "ohci.h"
>  
> @@ -46,6 +50,10 @@ struct ohci_omap_priv {
>  	struct clk *usb_dc_ck;
>  	struct gpio_desc *power;
>  	struct gpio_desc *overcurrent;
> +	spinlock_t overcurrent_lock;	/* protects overcurrent_status and overcurrent_changed */
> +	bool overcurrent_status;

So "status" is true or false?  That doesn't make much sense to me,
sorry.

> +	bool overcurrent_changed;

"changed" from what?  Previous value?  Next value?  Over time?

> +	int overcurrent_irq;

Why are you initializing this to -1?

And what broke that requires this patch?  When the gpio overcurrent
logic was added, why wasn't the irq support added then?  Did it ever
work, or is this to enable new hardware support?

>  };
>  
>  static const char hcd_name[] = "ohci-omap";
> @@ -181,11 +189,148 @@ 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);
> +	unsigned long flags;
> +	int status;
> +
> +	status = gpiod_get_value_cansleep(priv->overcurrent);
> +	if (status < 0)
> +		return IRQ_HANDLED;
> +
> +	if (status && priv->power)
> +		gpiod_set_value_cansleep(priv->power, 1);
> +
> +	spin_lock_irqsave(&priv->overcurrent_lock, flags);
> +	if (!!status != priv->overcurrent_status)
> +		priv->overcurrent_changed = true;
> +	priv->overcurrent_status = !!status;
> +	spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
> +
> +	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);
> +	unsigned long flags;
> +	bool changed;
> +	int length;
> +
> +	length = ohci_hub_status_data(hcd, buf);
> +
> +	if (!priv->overcurrent)
> +		return length;
> +
> +	spin_lock_irqsave(&priv->overcurrent_lock, flags);
> +	changed = priv->overcurrent_changed;
> +	spin_unlock_irqrestore(&priv->overcurrent_lock, flags);

Why do you need a lock to read this?

> +
> +	if (changed) {

Ah, but what if the value "changed" changed right after you read it
(remember, no more lock).

> +		if (!length) {
> +			buf[0] = 0;
> +			length = 1;
> +		}
> +		buf[0] |= BIT(1);

What does bit 1 mean here?

> +	}
> +
> +	return length;
> +}
> +
> +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);
> +	unsigned long flags;
> +	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) {
> +				spin_lock_irqsave(&priv->overcurrent_lock,
> +						  flags);
> +				if (!priv->overcurrent_status)
> +					gpiod_set_value_cansleep(priv->power,
> +								 0);
> +				spin_unlock_irqrestore(&priv->overcurrent_lock,
> +						       flags);
> +			}
> +			return ret;
> +		}
> +		if (typeReq == ClearPortFeature &&
> +		    wValue == USB_PORT_FEAT_POWER) {
> +			ret = ohci_hub_control(hcd, typeReq, wValue,
> +					       wIndex, buf, wLength);
> +			if (!ret && priv->power)
> +				gpiod_set_value_cansleep(priv->power, 1);
> +			return ret;
> +		}
> +	}
> +
> +	ret = ohci_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
> +	if (ret)
> +		return ret;
> +
> +	if (!priv->overcurrent)
> +		return ret;
> +
> +	if (typeReq == ClearPortFeature && wIndex == 1 &&
> +	    wValue == USB_PORT_FEAT_C_OVER_CURRENT) {
> +		spin_lock_irqsave(&priv->overcurrent_lock, flags);
> +		priv->overcurrent_changed = false;
> +		spin_unlock_irqrestore(&priv->overcurrent_lock, flags);

Why do you need a lock to change a single value?  Are you sure these
locks are even needed, they feel really wrong as you are using them
here.

Also, why not use guard()?  Is it because LLMs don't know about them
yet?

thanks,

greg k-h

Re: [PATCH v3] usb: ohci-omap: handle root hub overcurrent via GPIO

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: ohci-omap: sleeping GPIO call under spinlock in ohci_omap_hub_control()
- [High] usb: ohci-omap: VBUS power race in ohci_omap_overcurrent_irq()
- [Medium] usb: ohci-omap: VBUS power left enabled before OC init and on teardown
- [Medium] usb: ohci-omap: overcurrent state and IRQ initialized after usb_add_hcd()
--

commit 5181df0775d84a40d846a175267467ebbcb131d6
Author: Muhammad Mahdi Amirpour <ampr.mhd@gmail.com>

usb: ohci-omap: handle root hub overcurrent via GPIO

This commit adds root hub overcurrent handling for OMAP OHCI using the
optional overcurrent GPIO interrupt and updates hub_status_data and
hub_control to report overcurrent state and control the VBUS power GPIO.

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

[ ... ]

> @@ -181,11 +189,148 @@ 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);
> +	unsigned long flags;
> +	int status;
> +
> +	status = gpiod_get_value_cansleep(priv->overcurrent);
> +	if (status < 0)
> +		return IRQ_HANDLED;
> +
> +	if (status && priv->power)
> +		gpiod_set_value_cansleep(priv->power, 1);
> +
> +	spin_lock_irqsave(&priv->overcurrent_lock, flags);
> +	if (!!status != priv->overcurrent_status)
> +		priv->overcurrent_changed = true;
> +	priv->overcurrent_status = !!status;
> +	spin_unlock_irqrestore(&priv->overcurrent_lock, flags);

[Severity: High]
Can ohci_omap_overcurrent_irq() race with ohci_omap_hub_control() and leave
VBUS power enabled during an active overcurrent condition?

When an overcurrent interrupt fires, ohci_omap_overcurrent_irq() drives
priv->power to 1 before acquiring priv->overcurrent_lock and setting
priv->overcurrent_status = true.

If ohci_omap_hub_control() handles SetPortFeature(USB_PORT_FEAT_POWER) at
the same time (for instance during root hub activation or port power
recovery), it can check priv->overcurrent_status after
ohci_omap_overcurrent_irq() cuts VBUS power, still see
priv->overcurrent_status == false, and call
gpiod_set_value_cansleep(priv->power, 0) to turn VBUS back on just before
ohci_omap_overcurrent_irq() records priv->overcurrent_status = true.

Should the overcurrent_status update and the VBUS power GPIO state changes
be serialized under a sleeping lock such as a mutex?

[ ... ]

> +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);
> +	unsigned long flags;
> +	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) {
> +				spin_lock_irqsave(&priv->overcurrent_lock,
> +						  flags);
> +				if (!priv->overcurrent_status)
> +					gpiod_set_value_cansleep(priv->power,
> +								 0);
> +				spin_unlock_irqrestore(&priv->overcurrent_lock,
> +						       flags);
> +			}
> +			return ret;
> +		}

[Severity: High]
Doesn't calling gpiod_set_value_cansleep() while holding
priv->overcurrent_lock in ohci_omap_hub_control() sleep in atomic context?

During probe (usb_add_hcd() -> register_root_hub() -> hub_probe() ->
hub_activate() -> hub_power_on()) or port power recovery, USB core sends
SetPortFeature(USB_PORT_FEAT_POWER) for port 1.

gpiod_set_value_cansleep() calls might_sleep(), and on OMAP1 OSK the
"power" GPIO resides on the I2C-attached TPS65010 PMIC:

arch/arm/mach-omap1/board-osk.c:osk_usb_gpio_table:
	/* Power GPIO on the I2C-attached TPS65010 */
	GPIO_LOOKUP("tps65010", OSK_TPS_GPIO_USB_PWR_EN, "power",
		    GPIO_ACTIVE_HIGH),

drivers/mfd/tps65010.c:tps65010_set_gpio_out_value():
	mutex_lock(&the_tps->lock);

	defgpio = i2c_smbus_read_byte_data(the_tps->client, TPS_DEFGPIO);

Calling this inside spin_lock_irqsave(&priv->overcurrent_lock, flags)
acquires a mutex and performs blocking I2C SMBus transfers with preemption
and local interrupts disabled.

[ ... ]

> @@ -277,23 +425,76 @@ 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;
> +		unsigned long flags;
> +
> +		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;
> +		}
> +
> +		spin_lock_irqsave(&priv->overcurrent_lock, flags);
> +		priv->overcurrent_status = !!status;
> +		priv->overcurrent_changed = !!status;
> +		spin_unlock_irqrestore(&priv->overcurrent_lock, flags);
> +
> +		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]
Should the initial overcurrent GPIO state and IRQ setup happen before
usb_add_hcd() enables VBUS and registers the root hub?

Inside usb_add_hcd():
1. hcd->driver->reset() calls ohci_omap_reset(), which unconditionally
   enables VBUS via gpiod_set_value_cansleep(priv->power, 0) before
   priv->overcurrent is sampled.
2. hcd->driver->start() calls ohci_run(), which sleeps with VBUS enabled
   and no overcurrent IRQ registered.
3. register_root_hub() runs hub_probe() -> hub_configure() ->
   hub_activate() -> hub_power_on(), issuing
   SetPortFeature(USB_PORT_FEAT_POWER) to ohci_omap_hub_control() while
   priv->overcurrent_status is still false, so priv->power is driven to 0
   even if an overcurrent condition is already present.

Furthermore, because the root hub is already registered and exposed to
hub_wq before usb_add_hcd() returns, unconditionally assigning
priv->overcurrent_status and priv->overcurrent_changed here can overwrite
concurrent root hub status queries or ClearPortFeature requests.

>  
>  	device_wakeup_enable(hcd->self.controller);
>  	return 0;
> -err3:
> +
> +err_remove_hcd:
> +	if (priv->overcurrent_irq >= 0) {
> +		free_irq(priv->overcurrent_irq, hcd);
> +		priv->overcurrent_irq = -1;
> +	}
> +	usb_remove_hcd(hcd);
> +	omap_ohci_clock_power(priv, 0);
> +	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {
> +		(void)otg_set_host(hcd->usb_phy->otg, NULL);
> +		usb_put_phy(hcd->usb_phy);
> +	}
> +err_iounmap:
>  	iounmap(hcd->regs);
> -err2:
> +err_release_mem:
>  	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
>  err_unprepare_dc_ck:
>  	clk_unprepare(priv->usb_dc_ck);
> @@ -327,6 +528,10 @@ static void ohci_hcd_omap_remove(struct platform_device *pdev)
>  	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
>  
>  	dev_dbg(hcd->self.controller, "stopping USB Controller\n");
> +	if (priv->overcurrent_irq >= 0) {
> +		free_irq(priv->overcurrent_irq, hcd);
> +		priv->overcurrent_irq = -1;
> +	}
>  	usb_remove_hcd(hcd);
>  	omap_ohci_clock_power(priv, 0);
>  	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {

[Severity: Medium]
Does VBUS power remain enabled via priv->power if probe fails at
err_remove_hcd or when ohci_hcd_omap_remove() unbinds the driver?

During usb_add_hcd(), ohci_omap_reset() and hub_power_on() drive
priv->power to 0 to turn on VBUS. When usb_remove_hcd() runs in
err_remove_hcd or ohci_hcd_omap_remove(), neither hub_disconnect() nor
ohci_stop() issues ClearPortFeature(USB_PORT_FEAT_POWER), and resetting the
OHCI registers does not turn off the external GPIO power switch.

Should err_remove_hcd and ohci_hcd_omap_remove() drive priv->power to 1
after usb_remove_hcd() so VBUS is not left powered on without an active HCD
or overcurrent interrupt handler?

[ ... ]

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