Re: [PATCH] phy: broadcom: use devm_clk_get() in brcm_usb_phy_dvr_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: broadcom: unbalanced clk_prepare_enable() in brcm_usb_phy_dvr_init()
--

commit 34ff50179a8d79006737000f5a65391972e3147b
Author: Wentao Liang <vulab@iscas.ac.cn>

phy: broadcom: use devm_clk_get() in brcm_usb_phy_dvr_init()

This commit replaces of_clk_get_by_name() and clk_get() with devm_clk_get()
in brcm_usb_phy_dvr_init() so clock references are automatically released on
probe failure and driver removal, and drops the unused device_node parameter.

> diff --git a/drivers/phy/broadcom/phy-brcm-usb.c b/drivers/phy/broadcom/phy-brcm-usb.c
> index e606db489698a..c0cb701e419ac 100644
> --- a/drivers/phy/broadcom/phy-brcm-usb.c
> +++ b/drivers/phy/broadcom/phy-brcm-usb.c
> @@ -401,14 +401,13 @@ static int brcm_usb_get_regs(struct platform_device *pdev,
>  }
>  
>  static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
> -				 struct brcm_usb_phy_data *priv,
> -				 struct device_node *dn)
> +				 struct brcm_usb_phy_data *priv)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct phy *gphy = NULL;
>  	int err;
>  
> -	priv->usb_20_clk = of_clk_get_by_name(dn, "sw_usb");
> +	priv->usb_20_clk = devm_clk_get(dev, "sw_usb");
>  	if (IS_ERR(priv->usb_20_clk)) {
>  		if (PTR_ERR(priv->usb_20_clk) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
> @@ -440,7 +439,7 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
>  		priv->phys[BRCM_USB_PHY_3_0].id = BRCM_USB_PHY_3_0;
>  		phy_set_drvdata(gphy, &priv->phys[BRCM_USB_PHY_3_0]);
>  
> -		priv->usb_30_clk = of_clk_get_by_name(dn, "sw_usb3");
> +		priv->usb_30_clk = devm_clk_get(dev, "sw_usb3");
>  		if (IS_ERR(priv->usb_30_clk)) {
>  			if (PTR_ERR(priv->usb_30_clk) == -EPROBE_DEFER)
>  				return -EPROBE_DEFER;
> @@ -453,7 +452,7 @@ static int brcm_usb_phy_dvr_init(struct platform_device *pdev,
>  			return err;
>  	}
>  
> -	priv->suspend_clk = clk_get(dev, "usb0_freerun");
> +	priv->suspend_clk = devm_clk_get(dev, "usb0_freerun");
>  	if (IS_ERR(priv->suspend_clk)) {
>  		if (PTR_ERR(priv->suspend_clk) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;

[Severity: Medium]
Does switching to devm_clk_get() in brcm_usb_phy_dvr_init() cause devres to
call clk_put() on priv->usb_20_clk and priv->usb_30_clk while they are still
prepared and enabled if a later step in brcm_usb_phy_dvr_init() fails?

In brcm_usb_phy_dvr_init(), priv->usb_20_clk and priv->usb_30_clk are enabled
with clk_prepare_enable():

    priv->usb_20_clk = devm_clk_get(dev, "sw_usb");
    ...
    err = clk_prepare_enable(priv->usb_20_clk);
    if (err)
        return err;

and for XHCI:

    priv->usb_30_clk = devm_clk_get(dev, "sw_usb3");
    ...
    err = clk_prepare_enable(priv->usb_30_clk);
    if (err)
        return err;

In the normal probe flow, those clocks are only disabled and unprepared near
the end of brcm_usb_phy_probe():

    clk_disable_unprepare(priv->usb_20_clk);
    clk_disable_unprepare(priv->usb_30_clk);

If a subsequent step in brcm_usb_phy_dvr_init() fails (such as
devm_phy_create(), devm_clk_get() returning -EPROBE_DEFER for sw_usb3 or
usb0_freerun, clk_prepare_enable(priv->usb_30_clk), or devm_request_irq()),
brcm_usb_phy_dvr_init() returns the error directly without calling
clk_disable_unprepare() on the already-enabled clocks.

Because devm_clk_get() only registers clk_put() on devres release, unwinding
probe failure will call clk_put() on clocks whose prepare and enable counts
were never balanced.

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

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