[PATCH] phy: broadcom: brcm-usb: unwind late probe failures

[PATCH] phy: broadcom: brcm-usb: unwind late probe failures

[Pengpeng Hou]

brcm_usb_phy_probe() registers a PM notifier and may create a sysfs group
before registering the OF PHY provider. If provider registration fails,
probe returns the error with those resources still active.

Track which optional resources were installed and remove them on the late
provider-registration failure path. Use the same state in remove() so
cleanup matches probe progress.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/phy/broadcom/phy-brcm-usb.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
index 59d756a10d6c..e606db489698 100644
--- a/drivers/phy/broadcom/phy-brcm-usb.c
+++ b/drivers/phy/broadcom/phy-brcm-usb.c
@@ -71,6 +71,8 @@ struct brcm_usb_phy_data {
 	int			wake_irq;
 	struct brcm_usb_phy	phys[BRCM_USB_PHY_ID_MAX];
 	struct notifier_block	pm_notifier;
+	bool			pm_notifier_registered;
+	bool			sysfs_group_created;
 	bool			pm_active;
 };
 
@@ -545,7 +547,11 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
 		return err;
 
 	priv->pm_notifier.notifier_call = brcm_pm_notifier;
-	register_pm_notifier(&priv->pm_notifier);
+	err = register_pm_notifier(&priv->pm_notifier);
+	if (err)
+		dev_warn(dev, "Error registering PM notifier\n");
+	else
+		priv->pm_notifier_registered = true;
 
 	mutex_init(&priv->mutex);
 
@@ -561,6 +567,8 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
 	err = sysfs_create_group(&dev->kobj, &brcm_usb_phy_group);
 	if (err)
 		dev_warn(dev, "Error creating sysfs attributes\n");
+	else
+		priv->sysfs_group_created = true;
 
 	/* Get piarbctl syscon if it exists */
 	rmap = syscon_regmap_lookup_by_phandle(dev->of_node,
@@ -581,16 +589,29 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
 	clk_disable_unprepare(priv->usb_30_clk);
 
 	phy_provider = devm_of_phy_provider_register(dev, brcm_usb_phy_xlate);
+	err = PTR_ERR_OR_ZERO(phy_provider);
+	if (err)
+		goto err_remove_debug;
+
+	return 0;
+
+err_remove_debug:
+	if (priv->sysfs_group_created)
+		sysfs_remove_group(&dev->kobj, &brcm_usb_phy_group);
+	if (priv->pm_notifier_registered)
+		unregister_pm_notifier(&priv->pm_notifier);
 
-	return PTR_ERR_OR_ZERO(phy_provider);
+	return err;
 }
 
 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);
+	if (priv->sysfs_group_created)
+		sysfs_remove_group(&pdev->dev.kobj, &brcm_usb_phy_group);
+	if (priv->pm_notifier_registered)
+		unregister_pm_notifier(&priv->pm_notifier);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.50.1 (Apple Git-155)


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

Re: [PATCH] phy: broadcom: brcm-usb: unwind late probe failures

[sashiko-bot]

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

Pre-existing issues:
- [High] Modifying a static global sysfs attribute array during probe permanently alters the state for subsequent device instances.
- [High] Clock references and clock enable states are permanently leaked during initialization error paths.
--

commit 05dff911114f75fd3f8dc846d904d6dea7ced63e
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

phy: broadcom: brcm-usb: unwind late probe failures

This commit adds tracking for dynamically created resources like the PM
notifier and sysfs group during probe. It ensures these resources are
properly unwound on late provider-registration failure paths and matched
during device removal.

> diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
> index 59d756a10d6cb..e606db489698a 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb.c

[ ... ]

> @@ -545,7 +547,11 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
>  		return err;

[Severity: High]
This is a pre-existing issue, but do we leak clock references and enable
states during initialization error paths?

This error path comes right after brcm_usb_phy_dvr_init(). Inside
brcm_usb_phy_dvr_init(), clocks are obtained using non-managed APIs like
of_clk_get_by_name() but are never released via clk_put(). If an error
occurs after a clock is prepared and enabled:

brcm_usb_phy_dvr_init() {
    ...
    if (IS_ERR(gphy)) {
        dev_err(dev, "failed to create EHCI/OHCI PHY\n");
        return PTR_ERR(gphy);
    }
    ...
    if (IS_ERR(priv->usb_30_clk)) {
        if (PTR_ERR(priv->usb_30_clk) == -EPROBE_DEFER)
            return -EPROBE_DEFER;
    ...
}

The function returns without calling clk_disable_unprepare(). Probe deferral
is particularly problematic as each retry increments the enable count and
clock reference count without bound, permanently leaking resources.

>  	priv->pm_notifier.notifier_call = brcm_pm_notifier;
> -	register_pm_notifier(&priv->pm_notifier);
> +	err = register_pm_notifier(&priv->pm_notifier);
> +	if (err)
> +		dev_warn(dev, "Error registering PM notifier\n");
> +	else
> +		priv->pm_notifier_registered = true;
>  
>  	mutex_init(&priv->mutex);
>  
> @@ -561,6 +567,8 @@ static int brcm_usb_phy_probe(struct platform_device *pdev)
>  	err = sysfs_create_group(&dev->kobj, &brcm_usb_phy_group);

[Severity: High]
This is a pre-existing issue, but does modifying the static global array
brcm_usb_phy_attrs just before this sysfs_create_group() call permanently
alter the state for subsequent device instances?

Just above this hunk, the code mutates a shared static array:

if (priv->ini.supported_port_modes != USB_CTLR_MODE_DRD)
    brcm_usb_phy_attrs[1] = NULL;

Because this array is shared across all driver instances in the module,
modifying it truncates the sysfs attributes for the entire module. Any
subsequently probed devices will be missing the dual_select attribute even
if they do support DRD mode, leading to incorrect functional behavior for
multiple PHY instances.

>  	if (err)
>  		dev_warn(dev, "Error creating sysfs attributes\n");
> +	else
> +		priv->sysfs_group_created = true;
>  
>  	/* Get piarbctl syscon if it exists */
>  	rmap = syscon_regmap_lookup_by_phandle(dev->of_node,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615070123.51155-1-pengpeng@iscas.ac.cn?part=1

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