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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect error checking of pm_runtime_get_sync() return value causes spurious warnings when the device is already active.
- [Low] Failure of pm_runtime_get_sync() in remove() leads to an unbalanced clock disable by devres, triggering a kernel WARN.
--

--- Patch [3]: [PATCH v7 3/6] phy: fsl-imx8mq-usb: add runtime PM support ---
commit f054765f7ebd59735a4592c4d68b74584abdbeeb
Author: Xu Yang <[email protected]>

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

Add runtime PM support to ensure the PHY clocks are properly gated
when the PHY is not in use, reducing power consumption. Clock
management is moved from power_on()/power_off() callbacks into the
runtime_resume()/runtime_suspend() callbacks.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 3a5788c609e1b..74e665525ec48 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -708,44 +696,101 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
[ ... ]
>  static void imx8mq_usb_phy_remove(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret)
> +		dev_warn(dev, "failed to resume on remove: %d\n", ret);

[Severity: Medium]
Is it intended to warn when pm_runtime_get_sync() returns 1?

Since pm_runtime_get_sync() returns 1 when the device is already active
(or when CONFIG_PM is disabled), checking "if (ret)" instead of
"if (ret < 0)" will trigger this warning even on successful unbinds
where the device was already active.

> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_put_noidle(dev);
>  }

[Severity: Low]
Could a failure in pm_runtime_get_sync() lead to a clock enable count
underflow?

The clocks are acquired using devm_clk_get_enabled() in probe(), which
registers a devres cleanup action to disable the clocks on teardown. If
the runtime resume fails here in remove(), the clocks will remain disabled
by the runtime PM framework. When devres subsequently attempts to disable
them again during cleanup, it will cause an unbalanced clock disable and
a kernel WARN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3