Re: [PATCH v12 14/24] scsi: ufs: mediatek: Use the common PHY framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning an error in `ufs_mtk_runtime_suspend` without rolling back the hardware state causes a fatal PM state desync.

Pre-existing issues:
- [High] `ufs_mtk_mphy_power_on` fails to update `mphy_powered_on` when returning an error, permanently desynchronizing the PHY state.
--

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

scsi: ufs: mediatek: Use the common PHY framework

This commit changes the driver to use the common PHY framework to acquire the
device's PHY during initialization. This simplifies the driver by removing
reinvented PHY framework code and OF parsing.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 3c10a9fd09dfc..538009e3b1f17 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -2497,9 +2442,8 @@ static int ufs_mtk_system_resume(struct device *dev)
>  static int ufs_mtk_runtime_suspend(struct device *dev)
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> -	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  	struct arm_smccc_res res;
> -	int ret = 0;
> +	int ret;
>  
>  	ret = ufshcd_runtime_suspend(dev);
>  	if (ret)
> @@ -2510,8 +2454,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);

[Severity: High]
This is a pre-existing issue, but ufs_mtk_mphy_power_on() fails to update
mphy_powered_on when returning an error, which can permanently desynchronize
the PHY state.

Looking at ufs_mtk_mphy_power_on():

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_mphy_power_on() {
    ...
	} else {
		phy_power_off(mphy);
		ret = ufs_mtk_09v_off(host);
		if (ret)
			return ret;
	}

	host->mphy_powered_on = on;

	return 0;
}

If ufs_mtk_09v_off() fails during power off, it returns the error immediately,
leaving host->mphy_powered_on as true.

On the next resume, ufs_mtk_mphy_power_on(hba, true) will return early because
on == host->mphy_powered_on, skipping phy_power_on(mphy).

Can this permanently leave the PHY powered off and make the storage
inaccessible?

> +	if (ret) {
> +		dev_err(dev, "Failed to power off PHY: %pe\n", ERR_PTR(ret));
> +		return ret;

[Severity: High]
If ufs_mtk_mphy_power_on() fails, this returns an error immediately without
rolling back the earlier suspend operations.

Since the UFS hardware is already left physically suspended (clocks gated,
VREGs and MTCMOS powered down via ufshcd_runtime_suspend and 
ufs_mtk_dev_vreg_set_lpm), returning an error causes the PM core to mismatch
the physical hardware state and consider the device active.

Can this cause a fatal PM state desync, where subsequent I/O requests crash or
hang the bus because the PM core will skip calling runtime_resume?

> +	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-mt8196-ufs-v12-0-9279d7ef814d@collabora.com?part=14