Re: [PATCH v14 07/23] 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:
- [High] scsi: ufs: unbalanced phy power refcount on regulator disable failure
--

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

scsi: ufs: mediatek: Rework 0.9V regulator

The mediatek UFS host driver is refactored to manage the 0.9V regulator
by relying on the regulator framework instead of adding explicit sleeps
and checking downstream names. It renames the regulator to avdd09 and
updates the devicetree matching appropriately.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 2aa13b5954800..bcb7f350984c0 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -508,56 +510,77 @@ 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 err;
> -	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;
>  		ret = phy_power_on(mphy);
>  		if (ret) {
> -			if (ufs_mtk_is_va09_supported(hba)) {
> -				ufs_mtk_va09_pwr_ctrl(res, 0);
> -				err = regulator_disable(host->reg_va09);
> -				if (err)
> -					dev_err(hba->dev,
> -						"failed to unwind va09: %d\n", err);
> -			}
>  			dev_err(hba->dev, "failed to enable mphy: %d\n", ret);
> +			ufs_mtk_09v_off(host, false);
>  			return ret;
>  		}
>  	} 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, true);
> +		if (ret)
> +			return ret;

[Severity: High]
Does returning early here leave the mphy_powered_on boolean out of sync with
the physical state?

If ufs_mtk_09v_off() fails, the early return skips updating
host->mphy_powered_on = on (where on is false). This leaves the state as
true even though phy_power_off(mphy) has already been called.

Because of the early return, a subsequent request to power off would evaluate
on == host->mphy_powered_on (false == true) to false, causing the function
to execute phy_power_off(mphy) again and underflow the PHY power count.
Conversely, a request to power on would evaluate true == true, causing an
early success return without actually powering on the PHY, potentially breaking
UFS I/O permanently.

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

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