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

From: Manivannan Sadhasivam <mani@kernel.org>
To: Myeonghun Pak <mhun512@gmail.com>
Cc: Justin Chen <justin.chen@broadcom.com>,
	 Al Cooper <alcooperx@gmail.com>, Vinod Koul <vkoul@kernel.org>,
	 Broadcom internal kernel review list
	<bcm-kernel-feedback-list@broadcom.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	 linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org,
	Ijae Kim <ae878000@gmail.com>
Subject: Re: [PATCH] phy: broadcom: brcm-usb: unwind clocks on probe failure
Date: Mon, 12 Oct 2026 18:58:03 +0530	[thread overview]
Message-ID: <aszekmfBKpAY-4WL@debian> (raw)
In-Reply-To: <20260913012956.16443-1-mhun512@gmail.com>

On Sat, Sep 12, 2026 at 09:29:56PM -0400, Myeonghun Pak wrote:
> 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;

This looks ugly :( Either use two err labels or just club both
clk_disable_unprepare() calls in one err label as clk framework will bail out
safely if the clk pointer is NULL.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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