Re: [PATCH v13 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:
- [High] ufs: PM state mismatch on PHY power off failure in suspend
- [Medium] ufs: MTCMOS power leak on PHY power on failure in resume
--

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

scsi: ufs: mediatek: Use the common PHY framework

This commit switches the Mediatek UFS driver to use the common PHY framework
instead of manually parsing the OF tree for the PHY node. It updates the
initialization, resume, and suspend paths to power the PHY on and off using
the framework's APIs.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index e4b41de4de3a2..6d5687f5e7558 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -2502,9 +2447,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)
> @@ -2515,8 +2459,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;

[Severity: High]
Does returning the error directly here leave the PM state mismatched?

If ufs_mtk_mphy_power_on() fails, this returns early without rolling back the
already suspended UFS hardware (suspended via ufshcd_runtime_suspend()
earlier in this function).

The PM core may mark the device as RPM_ACTIVE because the callback failed,
despite the hardware physically being suspended. Could this cause subsequent
I/O to assume the device is active and hang?

> +	}
>  
>  	return 0;
>  }
> @@ -2524,14 +2471,17 @@ static int ufs_mtk_runtime_suspend(struct device *dev)
>  static int ufs_mtk_runtime_resume(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;
>  
>  	if (ufs_mtk_is_rtff_mtcmos(hba))
>  		ufs_mtk_mtcmos_ctrl(true, res);
>  
> -	if (host->phy_dev)
> -		pm_runtime_get_sync(host->phy_dev);
> +	ret = ufs_mtk_mphy_power_on(hba, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to power on PHY: %pe\n", ERR_PTR(ret));
> +		return ret;

[Severity: Medium]
Can this leak the MTCMOS power state if ufs_mtk_mphy_power_on() fails?

If ufs_mtk_mphy_power_on() fails, the function returns early without rolling
back the MTCMOS power state that was just enabled via
ufs_mtk_mtcmos_ctrl(true, res).

Does this leave the MTCMOS domain powered on indefinitely after a failed
resume attempt?

> +	}
>  
>  	ufs_mtk_dev_vreg_set_lpm(hba, false);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-mt8196-ufs-v13-0-60854adab6d9@collabora.com?part=14