Re: [PATCH v11 07/24] scsi: ufs: mediatek: Rework 0.9V regulator

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] State desynchronization and missing error handling in `ufs_mtk_mphy_power_on` can permanently break PHY power state.
--

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

scsi: ufs: mediatek: Rework 0.9V regulator
    
The mediatek UFS host driver does some pretty bad stuff with regards to
the 0.9V regulator. Instead of just checking for the presence of the
regulator, it adds a cap if it's there, and then checks for the cap.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 80c6ba3d026d2..eb7a0cc464f43 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -508,44 +510,70 @@ static int ufs_mtk_wait_link_state(struct ufs_hba *hba, u32 state,
>  static int ufs_mtk_mphy_power_on(struct ufs_hba *hba, bool on)
>  {
>  	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  	struct phy *mphy = host->mphy;
> -	struct arm_smccc_res res;
> -	int ret = 0;
> +	int ret;
>  
> -	if (!mphy || !(on ^ host->mphy_powered_on))
> +	if (!mphy || on == host->mphy_powered_on)
>  		return 0;
>  
>  	if (on) {
> -		if (ufs_mtk_is_va09_supported(hba)) {
> -			ret = regulator_enable(host->reg_va09);
> -			if (ret < 0)
> -				goto out;
> -			/* wait 200 us to stablize VA09 */
> -			usleep_range(200, 210);
> -			ufs_mtk_va09_pwr_ctrl(res, 1);
> -		}
> +		ret = ufs_mtk_09v_on(host);
> +		if (ret)
> +			return ret;
>  		phy_power_on(mphy);

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of phy_power_on() risk desynchronizing the power state?

If phy_power_on() fails (for example, if a clock enable fails in the
underlying implementation), this function will still return success and update
host->mphy_powered_on = true. This could cause subsequent UFS operations to
hang or trigger a synchronous external abort when accessing unclocked PHY
registers.

>  	} else {
>  		phy_power_off(mphy);
> -		if (ufs_mtk_is_va09_supported(hba)) {
> -			ufs_mtk_va09_pwr_ctrl(res, 0);
> -			ret = regulator_disable(host->reg_va09);
> -		}
> -	}
> -out:
> -	if (ret) {
> -		dev_info(hba->dev,
> -			 "failed to %s va09: %d\n",
> -			 on ? "enable" : "disable",
> -			 ret);
> -	} else {
> -		host->mphy_powered_on = on;
> +		ret = ufs_mtk_09v_off(host);
> +		if (ret)
> +			return ret;

[Severity: High]
This is a pre-existing issue, but if ufs_mtk_09v_off() fails here, the
function exits early and leaves host->mphy_powered_on incorrectly set to true.

Because phy_power_off() was already called, the PHY is physically shut down.
When attempting to power the PHY on again, the check
(on == host->mphy_powered_on) at the start of ufs_mtk_mphy_power_on() will
evaluate to true and bypass the power-on sequence entirely, leaving the PHY
permanently uninitialized.

Could we make sure host->mphy_powered_on remains consistent even on early
exits?

>  	}
>  
> -	return ret;
> +	host->mphy_powered_on = on;
> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-mt8196-ufs-v11-0-dbd007704490@collabora.com?part=7

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