Re: [PATCH v14 08/23] scsi: ufs: mediatek: Add dual 0.9V supply support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: mediatek: bypass of allow_pwr_on leaves regulator enabled on error
--

commit 169f13062deb73e7db2227752ba9ee6ad0ec3257
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

scsi: ufs: mediatek: Add dual 0.9V supply support

This commit adds support for an additional 0.9V supply pin on the MT8196
SoC compared to the MT8183 SoC. It introduces management for a second
optional 0.9V regulator named "avdd09-1".

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index bcb7f350984c0..c8c5f85507af6 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -513,20 +514,43 @@ static int ufs_mtk_wait_link_state(struct ufs_hba *hba, u32 state,
>  static int ufs_mtk_09v_off(struct ufs_mtk_host *host, bool allow_pwr_on)
>  {
>  	struct arm_smccc_res res;
> -	int ret;
> +	int ret, ret2;
>  
> -	if (!host->reg_avdd09)
> +	if (!host->reg_avdd09 && !host->reg_avdd09_1)
>  		return 0;
>  
>  	ufs_mtk_va09_pwr_ctrl(res, 0);
> -	ret = regulator_disable(host->reg_avdd09);
> -	if (ret) {
> -		dev_err(host->hba->dev, "Failed to disable avdd09-supply: %pe\n",
> -			ERR_PTR(ret));
> -		if (allow_pwr_on)
> -			ufs_mtk_va09_pwr_ctrl(res, 1);
>  
> -		return ret;
> +	if (host->reg_avdd09) {
> +		ret = regulator_disable(host->reg_avdd09);
> +		if (ret) {
> +			dev_err(host->hba->dev, "Failed to disable avdd09-supply: %pe\n",
> +				ERR_PTR(ret));
> +			if (allow_pwr_on)
> +				ufs_mtk_va09_pwr_ctrl(res, 1);
> +
> +			return ret;
> +		}
> +	}
> +
> +	if (host->reg_avdd09_1) {
> +		ret = regulator_disable(host->reg_avdd09_1);
> +		if (ret) {
> +			dev_err(host->hba->dev, "Failed to disable avdd09-1-supply: %pe\n",
> +				ERR_PTR(ret));
> +
> +			if (host->reg_avdd09) {
> +				ret2 = regulator_enable(host->reg_avdd09);

[Severity: Medium]
When regulator_disable() fails for reg_avdd09_1, does this unintentionally
re-enable the avdd09 regulator even if allow_pwr_on is false?

For example, ufs_mtk_mphy_power_on() calls ufs_mtk_09v_off(host, false) on
its error path to force regulators off.

If the allow_pwr_on parameter is false to prevent rolling back to a powered-on
state, unconditionally calling regulator_enable() here seems to defeat the
purpose of the flag. This could leave the regulator actively powered on after
a forced shutdown.

Should this regulator_enable() be conditioned on allow_pwr_on as well?

> +				if (ret2)
> +					dev_err(host->hba->dev,
> +						"Failed to re-enable avdd09-supply: %pe\n",
> +						ERR_PTR(ret2));
> +			}
> +			if (allow_pwr_on)
> +				ufs_mtk_va09_pwr_ctrl(res, 1);
> +
> +			return ret;
> +		}
>  	}
>  
>  	return 0;

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