Re: [PATCH v2] phy: fsl-imx8mq-usb: set usb phy to be wakeup capable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing error handling in the probe function leads to a Type-C switch leak and a critical Use-After-Free.
- [High] Using device_set_wakeup_capable() fails to enable wakeup by default, causing the PHY power domain to still be powered down during suspend.
- [High] Failing to enable the clocks in imx8mq_phy_power_on() leaves the vbus regulator permanently enabled.
--

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

phy: fsl-imx8mq-usb: set usb phy to be wakeup capable

The patch intends to keep the PHY power domain active during suspend
by setting the PHY to be wakeup capable.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index b05d80e849a14..b2e1e3749fc19 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -733,6 +733,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  					"failed to get tca\n");
>  
>  	imx8m_get_phy_tuning_data(imx_phy);
> +	device_set_wakeup_capable(dev, true);

Will this successfully enable wakeup by default?

The PM domain framework (genpd) checks genpd_is_active_wakeup(genpd) and
device_awake_path(dev) (which depends on device_may_wakeup(dev)) to determine
if the domain should remain powered on.

device_may_wakeup(dev) evaluates to dev->power.can_wakeup &&
dev->power.should_wakeup. While device_set_wakeup_capable(dev, true) sets
can_wakeup = true, it leaves should_wakeup = false. As a result,
device_may_wakeup() returns false by default, and genpd will still power off
the PHY domain.

Should this use device_init_wakeup(dev, true) to enable it by default,
or should the USB subsystem dynamically propagate the controller's wakeup state
to the PHY instead?

>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);

This isn't a bug introduced by this patch, but does this code leave a Type-C
switch registered if devm_of_phy_provider_register() fails?

If devm_of_phy_provider_register() returns an error, the probe function exits
without unregistering the Type-C switch (e.g., via imx95_usb_phy_put_tca()).
The imx_phy structure allocated with devm_kzalloc() would then be freed by the
driver core, but the switch remains active.

If the Type-C subsystem triggers the switch's set callback
(tca_blk_typec_switch_set()), it will access the freed imx_phy,
potentially resulting in a use-after-free.

>  
>  	return PTR_ERR_OR_ZERO(phy_provider);
>  }

This isn't a bug introduced by this patch, but does imx8mq_phy_power_on() leak
a regulator reference on error paths?

Looking at imx8mq_phy_power_on():

drivers/phy/freescale/phy-fsl-imx8mq-usb.c:imx8mq_phy_power_on() {
    ...
    ret = regulator_enable(imx_phy->vbus);
    if (ret)
        return ret;

    ret = clk_prepare_enable(imx_phy->clk);
    if (ret)
        return ret;
    ...
}

If clk_prepare_enable(imx_phy->clk) or clk_prepare_enable(imx_phy->alt_clk)
fails, the function returns directly without calling
regulator_disable(imx_phy->vbus).

Since the function returns an error, the PHY framework will not call
imx8mq_phy_power_off(). Could this leave the vbus regulator permanently
enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512101046.1498096-1-xu.yang_2@nxp.com?part=1

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