Re: [PATCH v11 14/24] scsi: ufs: mediatek: Use the common PHY framework

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unhandled early return on PHY power-off failure in ufs_mtk_runtime_suspend leaves the UFS controller hardware suspended while the PM core believes it is active.
- [High] Unconditional PHY power-off in runtime suspend breaks active UFS links.
- [Medium] Regulator enable count leak in ufs_mtk_09v_on error path.

Pre-existing issues:
- [High] Ignoring the return value of phy_power_on() causes PHY framework power_count corruption.
- [High] Unbalanced PHY power off and PM runtime put due to error handling.
--

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

scsi: ufs: mediatek: Use the common PHY framework
    
The commit converts the MediaTek UFS driver to use the common PHY framework
instead of custom OF parsing for PHYs. It acquires the device's PHY during
initialization to link it to the right device.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index ae053f40aaf46..1df16ce29ea4e 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -2488,9 +2433,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)
> @@ -2501,8 +2445,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) {

[Severity: High]
Does this unconditional power-off break active UFS links?

The UFS subsystem allows runtime PM to keep the link active depending on the
user-configured PM level (hba->rpm_lvl).

The underlying vendor suspend callback (ufs_mtk_suspend), which is called via
ufshcd_runtime_suspend() earlier in this function, explicitly checks
ufshcd_is_link_active(hba) and intentionally skips PHY power-off if the link
must remain active.

By adding an unconditional call to ufs_mtk_mphy_power_on(hba, false) here,
doesn't it override the link-state-aware logic and cut PHY power entirely,
killing the active electrical link?

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
phy_power_on() cause PHY framework power_count corruption?

Inside ufs_mtk_mphy_power_on():

	if (on) {
		ret = ufs_mtk_09v_on(host);
		if (ret)
			return ret;
		phy_power_on(mphy);
	}

If phy_power_on() fails, the PHY framework does not increment
phy->power_count and drops its PM runtime reference.

Because the return value is ignored, ufs_mtk_mphy_power_on() sets
host->mphy_powered_on = true.

Later, during suspend, it calls phy_power_off(), which decrements the zeroed
power_count (causing an underflow) and invokes phy_pm_runtime_put().

Can this cause an unbalanced PM put on the PHY device, leading to a negative
PM usage count and potential system instability?

[Severity: High]
This is a pre-existing issue, but does the error handling in
ufs_mtk_mphy_power_on() cause unbalanced PHY power off and PM runtime put?

Inside ufs_mtk_mphy_power_on():

	} else {
		phy_power_off(mphy);
		ret = ufs_mtk_09v_off(host);
		if (ret)
			return ret;
	}

	host->mphy_powered_on = on;

If ufs_mtk_09v_off() fails, it returns early and skips the assignment
host->mphy_powered_on = false.

The next time the device attempts to suspend, host->mphy_powered_on is still
true, and it executes the power-off branch again.

Doesn't this cause phy_power_off() to be called a second time for a single
power-on, double-decrementing power_count and causing an unbalanced PM runtime
put?

[Severity: Medium]
Does the error path in ufs_mtk_09v_on() leak the regulator enable count?

Inside ufs_mtk_09v_on():

	if (host->reg_avdd09_1) {
		ret = regulator_enable(host->reg_avdd09_1);
		if (ret) {
			dev_err(host->hba->dev, "Failed to enable avdd09-1-supply: %pe\n",
				ERR_PTR(ret));
			return ret;
		}
	}

If the second regulator (reg_avdd09_1) fails to enable, the function returns
the error directly without calling regulator_disable(host->reg_avdd09) to undo
the previous successful enable step.

Doesn't this leak the regulator enable count, keeping the resource active
indefinitely?

> +		dev_err(dev, "Failed to power off PHY: %pe\n", ERR_PTR(ret));
> +		return ret;

[Severity: High]
Can this unhandled early return leave the UFS controller hardware suspended
while the PM core believes it is active?

If ufs_mtk_mphy_power_on(hba, false) fails, returning the error code here
causes the PM core to abort the suspend attempt and treat the device as
remaining RPM_ACTIVE.

However, ufshcd_runtime_suspend() was already executed successfully just
before this, meaning the UFS host controller is physically suspended.

Doesn't this state mismatch mean the PM core will not issue a resume on the
next access, leading to I/O targeting powered-down hardware, which causes
register access timeouts, bus hangs, or crashes?

> +	}
>  
>  	return 0;
>  }

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

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