Re: [PATCH v4 3/5] phy: fsl-imx8mq-usb: add runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Mixing `devm_clk_get_enabled()` with manual `clk_disable_unprepare()` in runtime PM callbacks causes a clock enable count underflow during driver unbind.
--

commit 974b1a50a22e38e08aa5fd0139ae39d578c8f39c
Author: Xu Yang <xu.yang_2@nxp.com>

phy: fsl-imx8mq-usb: add runtime PM support

Add runtime PM to ensure the PHY is properly powered and clocked during
register access, preventing potential system hangs.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 591ddf3460618..27aa696f5dd40 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c

[ ... ]

> @@ -693,13 +681,13 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, imx_phy);
>  
> -	imx_phy->clk = devm_clk_get(dev, "phy");
> +	imx_phy->clk = devm_clk_get_enabled(dev, "phy");
>  	if (IS_ERR(imx_phy->clk)) {
>  		dev_err(dev, "failed to get imx8mq usb phy clock\n");
>  		return PTR_ERR(imx_phy->clk);
>  	}
>  
> -	imx_phy->alt_clk = devm_clk_get_optional(dev, "alt");
> +	imx_phy->alt_clk = devm_clk_get_optional_enabled(dev, "alt");
>  	if (IS_ERR(imx_phy->alt_clk))
>  		return dev_err_probe(dev, PTR_ERR(imx_phy->alt_clk),
>  				    "Failed to get alt clk\n");

[ ... ]

> @@ -735,11 +727,43 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  	return PTR_ERR_OR_ZERO(phy_provider);
>  }
>  
> +static int imx8mq_usb_phy_runtime_suspend(struct device *dev)
> +{
> +	struct imx8mq_usb_phy *imx_phy = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(imx_phy->alt_clk);
> +	clk_disable_unprepare(imx_phy->clk);

[Severity: High]
Does this mix of devm_clk_get_enabled() and manual clk_disable_unprepare()
cause a clock enable count underflow during driver unbind?

During imx8mq_usb_phy_probe(), devm_clk_get_enabled() registers devres actions
that will unconditionally call clk_disable_unprepare() upon driver removal.

When the driver is unbound, the driver core calls pm_runtime_put_sync() before
triggering devres cleanup. This suspends the device, invoking
imx8mq_usb_phy_runtime_suspend() which drops the clock enable counts to 0.

Subsequently, device_unbind_cleanup() executes the devres actions, calling
clk_disable_unprepare() again on the already-disabled clocks. In the clock
framework, checking an enable_count of 0 results in a kernel warning.

To avoid this, the driver could either handle clock initialization manually
without devm_*_enabled variants, or implement a .remove callback that
explicitly wakes the device via pm_runtime_get_sync() and disables runtime PM
to prevent devres from executing on a suspended device.

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-imx8mp-usb-phy-improvement-v4-0-b2ddf2f3862c@nxp.com?part=3

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