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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing rollback of avdd09 regulator state on avdd09-1 error paths in ufs_mtk_09v_on() and ufs_mtk_09v_off().
- [Medium] Incorrect variable passed to %pe format specifier in dev_err_probe() for avdd09-1 regulator error.
--

commit 233271811903bef688f03076564e8ae57965014c
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

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

The MT8196 SoC has an additional pin compared to the MT8183 SoC to feed
the host controller with a 0.9V supply. This patch adds management for a
second optional 0.9V regulator named "avdd09-1" in preparation for this
SoC support.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index eb7a0cc464f43..f298ea6f6ecb9 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -515,16 +516,29 @@ static int ufs_mtk_09v_off(struct ufs_mtk_host *host)
>  	struct arm_smccc_res res;
>  	int ret;
>  
> -	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));
> -		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));
> +			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));
> +			ufs_mtk_va09_pwr_ctrl(res, 1);
> +			return ret;
> +		}

[Severity: High]
Does this error path leave host->reg_avdd09 disabled if disabling
host->reg_avdd09_1 fails?

If the second disable fails, returning here means the power state becomes
desynchronized. Should host->reg_avdd09 be re-enabled here to match the
driver's expected state before returning the error?

>  	}
>  
>  	return 0;
> @@ -535,14 +549,25 @@ static int ufs_mtk_09v_on(struct ufs_mtk_host *host)
>  	struct arm_smccc_res res;
>  	int ret;
>  
> -	if (!host->reg_avdd09)
> +	if (!host->reg_avdd09 && !host->reg_avdd09_1)
>  		return 0;
>  
> -	ret = regulator_enable(host->reg_avdd09);
> -	if (ret) {
> -		dev_err(host->hba->dev, "Failed to enable avdd09-supply: %pe\n",
> -			ERR_PTR(ret));
> -		return ret;
> +	if (host->reg_avdd09) {
> +		ret = regulator_enable(host->reg_avdd09);
> +		if (ret) {
> +			dev_err(host->hba->dev, "Failed to enable avdd09-supply: %pe\n",
> +				ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
> +	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;
> +		}

[Severity: High]
Does this path leak a reference count for host->reg_avdd09?

If enabling host->reg_avdd09_1 fails after host->reg_avdd09 was successfully
enabled, the function returns immediately without disabling host->reg_avdd09.
Should host->reg_avdd09 be disabled before returning?

>  	}
>  
>  	ufs_mtk_va09_pwr_ctrl(res, 1);
> @@ -1232,20 +1257,33 @@ static int ufs_mtk_get_supplies(struct ufs_mtk_host *host)
>  	struct device *dev = host->hba->dev;
>  	const struct ufs_mtk_soc_data *data = of_device_get_match_data(dev);
>  
> -	if (!data || !data->has_avdd09)
> +	if (!data)
>  		return 0;

[ ... ]

> +	if (data->has_avdd09_1) {
> +		host->reg_avdd09_1 = devm_regulator_get_optional(dev, "avdd09-1");
> +		if (IS_ERR(host->reg_avdd09_1)) {
> +			if (PTR_ERR(host->reg_avdd09_1) != -ENODEV) {
> +				return dev_err_probe(dev,
> +						     PTR_ERR(host->reg_avdd09_1),
> +						     "Failed to get avdd09-1 regulator: %pe\n",
> +						     host->reg_avdd09);

[Severity: Medium]
Are we passing the correct variable to the %pe format specifier here?

It looks like host->reg_avdd09 is passed instead of host->reg_avdd09_1, which
would cause dev_err_probe() to print the pointer value or hash of the wrong
regulator instead of the actual error string.

> +			}
> +			host->reg_avdd09_1 = NULL;
> +		}
>  	}
>  
>  	return 0;

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

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