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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unchecked return value of `pm_runtime_get_sync()` via `guard(pm_runtime_active)` in `tca_blk_typec_switch_set()` leads to unsafe register access.
- [High] Double disable of clocks (enable count underflow) on driver unbind due to mixing `devm_clk_get_enabled()` with manual clock management in PM runtime suspend.
--

commit 3ca36d3b5ee797e587a615f9df482c7159a29d88
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..b0092c34416e3 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -136,17 +137,13 @@ static int tca_blk_typec_switch_set(struct typec_switch_dev *sw,
>  {
>  	struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
>  	struct tca_blk *tca = imx_phy->tca;
> -	int ret;
>  
>  	if (tca->orientation == orientation)
>  		return 0;
>  
> -	ret = clk_prepare_enable(imx_phy->clk);
> -	if (ret)
> -		return ret;
> +	guard(pm_runtime_active)(&imx_phy->phy->dev);

[Severity: High]
Does this wrapper ignore the return value of pm_runtime_get_sync()?

If the runtime resume fails (for example, if the underlying clock
enablement fails), the error appears to be swallowed here. Execution would
then proceed to tca_blk_orientation_set(), which performs MMIO accesses
on the unclocked TCA hardware registers.

On i.MX/ARM platforms, accessing unclocked register space triggers a
synchronous external abort (SError), resulting in a system hang.

Should this explicitly use pm_runtime_resume_and_get() or similar, and
verify the return value before proceeding?

>  
>  	tca_blk_orientation_set(tca, orientation);
> -	clk_disable_unprepare(imx_phy->clk);
>  
>  	return 0;
>  }
[ ... ]
> @@ -686,6 +671,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct imx8mq_usb_phy *imx_phy;
>  	const struct phy_ops *phy_ops;
> +	int ret;
>  
>  	imx_phy = devm_kzalloc(dev, sizeof(*imx_phy), GFP_KERNEL);
>  	if (!imx_phy)
> @@ -693,13 +679,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");

[Severity: High]
Could mixing devm_clk_get_enabled() with manual clk_disable_unprepare()
in the runtime suspend callback cause a double-disable?

During driver unbind, the driver core may transition the device to the
suspended state, which invokes the runtime suspend callback and drops
the clock enable count to 0.

Immediately following this, devres teardown runs and unconditionally
calls clk_disable_unprepare() again for these devm_clk_get_enabled()
actions.

Would this drop the clock count to -1, triggering an enable count
underflow WARN in the clock core?

>  	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 +725,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);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-imx8mp-usb-phy-improvement-v3-0-7afb8f89abc6@nxp.com?part=3

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