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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `phy_power_on()` is ignored, leading to an inconsistent state if the PHY fails to power on.
- [High] Incomplete state update during error handling in `ufs_mtk_mphy_power_on()` when `ufs_mtk_09v_off()` fails causes MPHY power state desynchronization.
--

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

scsi: ufs: mediatek: Rework 0.9V regulator

This commit reworks how the mediatek UFS driver handles the 0.9V regulator.
It transitions from a downstream va09 check to standard avdd09 supply
naming and splits the regulator operations into separate helpers instead of
handling them directly within the phy power-on function.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> --- 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() leave the driver in an inconsistent state?

If phy_power_on() fails due to underlying clock or i2c communication
timeouts, this function ignores the failure, unconditionally sets
host->mphy_powered_on = true, and returns success. This might cause the
driver to proceed as if the phy is fully operational, leading to hardware
timeouts or system hangs during subsequent controller I/O operations.

>  	} 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 does this early return cause a permanent
state desynchronization when disabling the regulator fails?

If ufs_mtk_09v_off() fails during a power-off transition, the function
returns early and bypasses updating host->mphy_powered_on. Because
phy_power_off() was already called above, the physical hardware is powered
down, but the software state still evaluates host->mphy_powered_on as true.

On a subsequent resume attempt, the driver would evaluate
on == host->mphy_powered_on as true and silently skip powering the phy back
on, which could result in a dead storage controller.

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

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