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

[sashiko-bot@kernel.org]

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