[PATCH] phy: broadcom: brcm-usb: unwind clocks on probe failure

[PATCH] phy: broadcom: brcm-usb: unwind clocks on probe failure

[Myeonghun Pak]

brcm_usb_phy_dvr_init() enables the optional USB2 and USB3 clocks before
creating all PHYs and acquiring the remaining probe resources.  Several
later failures return without disabling clocks that were already enabled,
and deferred probe retries can keep increasing their enable counts.

Track each successful clock enable and unwind only those clocks, in reverse
order, when initialization fails.  Keep the existing successful probe path
unchanged.  This is limited to the BCM4908 and Broadcom STB USB PHY driver.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: 49859e55e364 ("phy: usb: phy-brcm-usb: Add Broadcom STB USB phy driver")
Assisted-by: OpenAI:GPT-5.6
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/phy/broadcom/phy-brcm-usb.c | 48 +++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
index 59d756a10..273f7b586 100644
--- a/drivers/phy/broadcom/phy-brcm-usb.c
+++ b/drivers/phy/broadcom/phy-brcm-usb.c
@@ -404,6 +404,8 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 {
 	struct device *dev = &pdev->dev;
 	struct phy *gphy = NULL;
+	bool usb_20_clk_enabled = false;
+	bool usb_30_clk_enabled = false;
 	int err;
 
 	priv->usb_20_clk = of_clk_get_by_name(dn, "sw_usb");
@@ -413,15 +415,19 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 		dev_info(dev, "Clock not found in Device Tree\n");
 		priv->usb_20_clk = NULL;
 	}
-	err = clk_prepare_enable(priv->usb_20_clk);
-	if (err)
-		return err;
+	if (priv->usb_20_clk) {
+		err = clk_prepare_enable(priv->usb_20_clk);
+		if (err)
+			return err;
+		usb_20_clk_enabled = true;
+	}
 
 	if (priv->has_eohci) {
 		gphy = devm_phy_create(dev, NULL, &brcm_usb_phy_ops);
 		if (IS_ERR(gphy)) {
 			dev_err(dev, "failed to create EHCI/OHCI PHY\n");
-			return PTR_ERR(gphy);
+			err = PTR_ERR(gphy);
+			goto err_disable_clks;
 		}
 		priv->phys[BRCM_USB_PHY_2_0].phy = gphy;
 		priv->phys[BRCM_USB_PHY_2_0].id = BRCM_USB_PHY_2_0;
@@ -432,7 +438,8 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 		gphy = devm_phy_create(dev, NULL, &brcm_usb_phy_ops);
 		if (IS_ERR(gphy)) {
 			dev_err(dev, "failed to create XHCI PHY\n");
-			return PTR_ERR(gphy);
+			err = PTR_ERR(gphy);
+			goto err_disable_clks;
 		}
 		priv->phys[BRCM_USB_PHY_3_0].phy = gphy;
 		priv->phys[BRCM_USB_PHY_3_0].id = BRCM_USB_PHY_3_0;
@@ -440,21 +447,28 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 
 		priv->usb_30_clk = of_clk_get_by_name(dn, "sw_usb3");
 		if (IS_ERR(priv->usb_30_clk)) {
-			if (PTR_ERR(priv->usb_30_clk) == -EPROBE_DEFER)
-				return -EPROBE_DEFER;
+			if (PTR_ERR(priv->usb_30_clk) == -EPROBE_DEFER) {
+				err = -EPROBE_DEFER;
+				goto err_disable_clks;
+			}
 			dev_info(dev,
 				 "USB3.0 clock not found in Device Tree\n");
 			priv->usb_30_clk = NULL;
 		}
-		err = clk_prepare_enable(priv->usb_30_clk);
-		if (err)
-			return err;
+		if (priv->usb_30_clk) {
+			err = clk_prepare_enable(priv->usb_30_clk);
+			if (err)
+				goto err_disable_clks;
+			usb_30_clk_enabled = true;
+		}
 	}
 
 	priv->suspend_clk = clk_get(dev, "usb0_freerun");
 	if (IS_ERR(priv->suspend_clk)) {
-		if (PTR_ERR(priv->suspend_clk) == -EPROBE_DEFER)
-			return -EPROBE_DEFER;
+		if (PTR_ERR(priv->suspend_clk) == -EPROBE_DEFER) {
+			err = -EPROBE_DEFER;
+			goto err_disable_clks;
+		}
 		dev_err(dev, "Suspend Clock not found in Device Tree\n");
 		priv->suspend_clk = NULL;
 	}
@@ -467,7 +481,7 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 				       brcm_usb_phy_wake_isr, 0,
 				       dev_name(dev), dev);
 		if (err < 0)
-			return err;
+			goto err_disable_clks;
 		device_set_wakeup_capable(dev, 1);
 	} else {
 		dev_info(dev,
@@ -475,6 +489,14 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
 	}
 
 	return 0;
+
+err_disable_clks:
+	if (usb_30_clk_enabled)
+		clk_disable_unprepare(priv->usb_30_clk);
+	if (usb_20_clk_enabled)
+		clk_disable_unprepare(priv->usb_20_clk);
+
+	return err;
 }
 
 static int brcm_usb_phy_probe(struct platform_device *pdev)
-- 
2.47.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: broadcom: brcm-usb: unwind clocks on probe failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup for PM notifier and sysfs group on probe failure, leading to a use-after-free and zombie sysfs attributes.
- [High] Clock object references acquired via of_clk_get_by_name() and clk_get() are never released with clk_put().
- [High] Unchecked clk_prepare_enable() results in unsafe register accesses.
--

commit 4860feaf09bcf2d19e6bdb2759006de58fb0811e
Author: Myeonghun Pak <mhun512@gmail.com>

    phy: broadcom: brcm-usb: unwind clocks on probe failure
    
    brcm_usb_phy_dvr_init() enables the optional USB2 and USB3 clocks before
    creating all PHYs and acquiring the remaining probe resources. Several
    later failures return without disabling clocks that were already enabled,
    and deferred probe retries can keep increasing their enable counts.

> diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
> index 59d756a10d6cb..273f7b5863018 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb.c
> @@ -404,6 +404,8 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
>  {
>  	struct device *dev = &pdev->dev;
>  	struct phy *gphy = NULL;
> +	bool usb_20_clk_enabled = false;
> +	bool usb_30_clk_enabled = false;
>  	int err;
>  
>  	priv->usb_20_clk = of_clk_get_by_name(dn, "sw_usb");

[Severity: High]
This is a pre-existing issue, but clock object references acquired via
of_clk_get_by_name() and clk_get() are never released with clk_put().

Since these APIs take a reference to the clock, doesn't the lack of clk_put()
leak the clock objects and provider module reference counts on probe deferral
and when the driver is unbound?

[ ... ]

> @@ -475,6 +489,14 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
>  	}
>  
>  	return 0;
> +
> +err_disable_clks:
> +	if (usb_30_clk_enabled)
> +		clk_disable_unprepare(priv->usb_30_clk);
> +	if (usb_20_clk_enabled)
> +		clk_disable_unprepare(priv->usb_20_clk);
> +
> +	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the clock
unwinding, should clk_put() also be called here on the acquired clock
references?

Similarly, in brcm_usb_phy_remove(), the driver fails to call clk_put():

static void brcm_usb_phy_remove(struct platform_device *pdev)
{
	struct brcm_usb_phy_data *priv = dev_get_drvdata(&pdev->dev);

	sysfs_remove_group(&pdev->dev.kobj, &brcm_usb_phy_group);
	unregister_pm_notifier(&priv->pm_notifier);
}

Should clk_put() be added to the remove path as well?


[Severity: High]
This is a pre-existing issue, but in brcm_usb_phy_probe(), the sysfs group
and PM notifier are registered manually but lack removal logic on probe error:

static int brcm_usb_phy_probe(struct platform_device *pdev)
{
...
	priv->pm_notifier.notifier_call = brcm_pm_notifier;
	register_pm_notifier(&priv->pm_notifier);
...
	err = sysfs_create_group(&dev->kobj, &brcm_usb_phy_group);
...
	phy_provider = devm_of_phy_provider_register(dev, brcm_usb_phy_xlate);

	return PTR_ERR_OR_ZERO(phy_provider);
}

If devm_of_phy_provider_register() fails, the probe function returns an error,
which causes the devres framework to free the priv structure.

Does this leave a dangling pointer in the global PM notifier chain and zombie
sysfs attributes, potentially leading to a use-after-free during the next PM
event?


[Severity: High]
This is a pre-existing issue, but I noticed unchecked clk_prepare_enable()
calls in brcm_usb_phy_init():

static void brcm_usb_phy_init(...)
{
...
	clk_prepare_enable(priv->usb_20_clk);
	clk_prepare_enable(priv->usb_30_clk);
	clk_prepare_enable(priv->suspend_clk);
	brcm_usb_init_common(&priv->ini);
...
}

And a similar pattern in brcm_usb_phy_resume():

static int brcm_usb_phy_resume(...)
{
...
	clk_prepare_enable(priv->usb_20_clk);
	clk_prepare_enable(priv->usb_30_clk);
	brcm_usb_init_ipp(&priv->ini);
...
}

If clk_prepare_enable() fails, the functions proceed to access memory-mapped
hardware registers.

Can accessing these unclocked peripheral registers cause an unrecoverable
system panic or bus error?

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy