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

[sashiko-bot@kernel.org]

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