Re: [PATCH v8 4/6] phy: fsl-imx8mq-usb: add control register regmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing PM runtime protection during probe allows mid-probe suspend and double clock disable on error path.
--

--- Patch [4]: [PATCH v8 4/6] phy: fsl-imx8mq-usb: add control register regmap ---
commit 0c8e2a6389c47dd36275e3aecba6491f3f4c70ac
Author: Xu Yang <xu.yang_2@nxp.com>

    phy: fsl-imx8mq-usb: add control register regmap
    
    The CR port is a simple 16-bit data/address parallel port that is
    accessed through 32-bit MMIO registers for on-chip access to the
    control registers inside the USB 3.0 femtoPHY. Add control register
    regmap and export these registers by debugfs to help PHY's diagnostic.

[ ... ]

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 42de2cff4d5f3..e03f9dafd69e0 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -696,6 +708,13 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(imx_phy->base))
>  		return PTR_ERR(imx_phy->base);
>  
> +	imx_phy->cr_regmap = devm_regmap_init_mmio(dev, imx_phy->base + PHY_CRCTL,
> +						   &imx_cr_regmap_config);
> +	if (IS_ERR(imx_phy->cr_regmap)) {
> +		dev_warn(dev, "Fail to init debug register regmap\n");
> +		imx_phy->cr_regmap = NULL;
> +	}
> +
>  	imx_phy->vbus = devm_regulator_get(dev, "vbus");
>  	if (IS_ERR(imx_phy->vbus))
>  		return dev_err_probe(dev, PTR_ERR(imx_phy->vbus), "failed to get vbus\n");

[Severity: High]
Does imx8mq_usb_phy_probe() need a PM runtime usage reference before enabling
runtime PM?

Later in this function, pm_runtime_enable(dev) is called without holding a
usage reference via pm_runtime_get_noresume(). After that, it registers a
Type-C switch.

If the Type-C subsystem (or TCPM) immediately configures this switch via
tca_blk_typec_switch_set(), it acquires and drops a PM reference. Dropping
the reference queues an idle request, causing the device to runtime suspend
and disable its clocks.

If a subsequent probe step like devm_of_phy_provider_register() fails, the
code jumps to disable_rpm:

disable_rpm:
    pm_runtime_disable(dev);
    return ret;

Because pm_runtime_disable() leaves the device in the suspended state if it
suspended mid-probe, the device remains suspended. The probe returns an
error, triggering devres cleanup, which calls clk_disable_unprepare() and
disables the clocks a second time.

For a shared clock, this drops the global enable count unexpectedly, turning
off the hardware clock while other consumers are actively using it. On SoCs
like i.MX8, accessing unclocked registers typically causes an AXI bus lockup.

Can we acquire a PM reference before enabling runtime PM to prevent the
device from suspending during probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-imx8mp-usb-phy-improvement-v8-0-2ec8d6b3854d@nxp.com?part=4

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