Re: [PATCH v14 14/23] scsi: ufs: mediatek: Use the common PHY framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ufs/mediatek: PM state machine desync on PHY power-off failure
- [High] scsi: ufs: mediatek: PHY incorrectly powered off during active link runtime suspend
- [Medium] ufs/mediatek: resource leak of PHY initialization on error path
- [Medium] ufs/mediatek: PHY state desynchronization on regulator disable failure
--

commit d74957280789302bafd051c97a8d0add4f2d400a
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

scsi: ufs: mediatek: Use the common PHY framework

This patch transitions the Mediatek UFS driver to use the common PHY framework
instead of custom OF parsing and manual PHY device power management. It updates
ufs_mtk_init() to acquire and initialize the PHY via standard APIs and replaces
pm_runtime_get_sync()/put_sync() calls with direct PHY power control.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 41e2e9f867013..808365b880fd7 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -1319,11 +1289,22 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>  
>  	return 0;
>  
> +out_phy_exit:
> +	phy_exit(host->mphy);
>  out_variant_clear:
>  	ufshcd_set_variant(hba, NULL);
>  	return err;
>  }

[Severity: Medium]
Does this error path bypass phy_exit() if ufs_mtk_mphy_power_on() fails?

Earlier in ufs_mtk_init(), phy_init(host->mphy) is called successfully. Later
in the same function, if ufs_mtk_mphy_power_on(hba, true) fails, the code
jumps directly to out_variant_clear:

    err = ufs_mtk_mphy_power_on(hba, true);
    if (err)
        goto out_variant_clear;

Since out_variant_clear is physically below out_phy_exit, this skips the
newly introduced phy_exit(host->mphy) call, leaking the PHY initialization.

[ ... ]
> @@ -2533,8 +2477,11 @@ static int ufs_mtk_runtime_suspend(struct device *dev)
>  	if (ufs_mtk_is_rtff_mtcmos(hba))
>  		ufs_mtk_mtcmos_ctrl(false, res);
>  
> -	if (host->phy_dev)
> -		pm_runtime_put_sync(host->phy_dev);
> +	ret = ufs_mtk_mphy_power_on(hba, false);
> +	if (ret) {
> +		dev_err(dev, "Failed to power off PHY: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
Does returning an error here break the runtime PM contract?

Before reaching this code, ufshcd_runtime_suspend(dev) has already suspended
the UFS controller, and ufs_mtk_dev_vreg_set_lpm() has put regulators into
low-power mode. Returning an error signals failure to the PM core, which
aborts the suspend transition and leaves the device status as RPM_ACTIVE.

Because the underlying hardware is already powered off, subsequent I/O
requests will bypass runtime_resume() and access unpowered hardware. This
can lead to bus timeouts or kernel crashes.

[Severity: High]
Does this unconditional power down break configurations where the UFS link
is meant to remain active during runtime PM?

If the system is configured to keep the link active during runtime suspend,
the host controller can still be suspended once the device WLUN is suspended.
Previously, pm_runtime_put_sync() would not physically power off the PHY
because the Mediatek PHY driver lacks runtime PM ops. Instead, PHY power was
safely managed by ufs_mtk_pwr_ctrl() which explicitly checks the link state
and skips power-down if the link is active.

By replacing this with an unconditional ufs_mtk_mphy_power_on(hba, false),
this function blindly calls phy_power_off() regardless of the link state.
This forces the PHY off while the MAC/SCSI layers expect it to remain active,
causing UFS access to hang on wake.

[Severity: Medium]
Can this call lead to a state desynchronization if the regulator disable
fails?

Inside the ufs_mtk_mphy_power_on(hba, false) helper function, the code powers
down the hardware and then disables the regulators:

	} else {
		phy_power_off(mphy);
		ret = ufs_mtk_09v_off(host, true);
		if (ret)
			return ret;
	}

	host->mphy_powered_on = on;

If ufs_mtk_09v_off() fails, it returns early. This skips the assignment
of host->mphy_powered_on = on (which would be false). Software will retain
the belief that the PHY is still powered on.

When the device subsequently resumes, ufs_mtk_mphy_power_on(hba, true) checks
if (on == host->mphy_powered_on) which evaluates to true == true, returning 0
without calling phy_power_on(mphy). This leaves the hardware physically off
while the driver expects it to be on, causing the UFS link to fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-mt8196-ufs-v14-0-e8d565e0cd45@collabora.com?part=14