Re: [PATCH] scsi: ufs: mediatek: Handle mPHY power-on failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Failure in `regulator_disable()` during power-off leaves the software state flagged as 'powered on', despite the physical PHY being powered down, which breaks subsequent power-on attempts.
- [High] Unbalanced PHY power management leading to clock setup with an unpowered PHY and VA09 regulator during resume.
--

commit abe799aa942a1ffc27379395adfb1d36f8c84af2
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

scsi: ufs: mediatek: Handle mPHY power-on failures

This patch updates ufs_mtk_mphy_power_on() to handle phy_power_on() failures.
If the PHY fails to power on, the VA09 regulator is turned back off to prevent
it from being left enabled, and the initialization path now checks and
propagates this error.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 865be3ddf448e..92dbd240c7ea5 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -524,7 +525,18 @@ static int ufs_mtk_mphy_power_on(struct ufs_hba *hba, bool on)
>  			usleep_range(200, 210);
>  			ufs_mtk_va09_pwr_ctrl(res, 1);
>  		}
> -		phy_power_on(mphy);
> +		ret = phy_power_on(mphy);
> +		if (ret) {
> +			if (ufs_mtk_is_va09_supported(hba)) {
> +				ufs_mtk_va09_pwr_ctrl(res, 0);
> +				rollback_ret = regulator_disable(host->reg_va09);
> +				if (rollback_ret)
> +					dev_warn(hba->dev, "failed to disable va09 after mphy failure: %d\n",
> +						 rollback_ret);
> +			}
> +			dev_info(hba->dev, "failed to enable mphy: %d\n", ret);
> +			return ret;
> +		}
>  	} else {
>  		phy_power_off(mphy);
>  		if (ufs_mtk_is_va09_supported(hba)) {

[Severity: High]
This isn't a bug introduced by this patch, but if regulator_disable() fails
here during power-off, does it leave the software state permanently out of sync?

In ufs_mtk_mphy_power_on(), we have:

	} 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) {
        ...
	} else {
		host->mphy_powered_on = on;
	}

If regulator_disable() returns an error, host->mphy_powered_on won't be
updated to false, even though phy_power_off() was already executed. Can this
cause future calls to ufs_mtk_mphy_power_on(hba, true) to early-exit, leaving
the PHY physically unpowered?


[Severity: High]
This is also a pre-existing issue, but does the current power management flow
cause unbalanced PHY power off calls?

During suspend, ufs_mtk_suspend() calls ufs_mtk_mphy_power_on(false), which
executes phy_power_off(mphy) and disables the regulator.

Later in the suspend path, ufshcd_suspend() calls ufs_mtk_setup_clocks(false),
which triggers ufs_mtk_pwr_ctrl(false). This redundantly calls
phy_power_off(mphy) again, dropping its internal power_count to -1.

During resume, ufs_mtk_pwr_ctrl(true) calls phy_power_on(mphy). Because
the power_count was -1, it only increments to 0 without actually powering on
the hardware. The VA09 regulator is also left disabled.

ufs_mtk_pwr_ctrl() then calls ufs_mtk_setup_ref_clk(true), which attempts to
poll reference clock registers on physically unpowered hardware.

Does this unconditionally lead to a timeout and broken I/O during resume on
systems binding a PHY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828092311.10774-1-pengpeng@iscas.ac.cn?part=1