Re: [PATCH v12 08/24] 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:
- [High] Unchecked NULL pointer dereference of optional regulator `host->reg_avdd09` in error recovery paths.
--

commit c69b1189125cd5106318d877fff7d1bf82dfd012
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 on the MT8196 SoC.
It introduces a second optional 0.9V regulator named "avdd09-1" to the
platform data and manages its enabling and disabling alongside the primary
supply.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index f43bee50adb28..467fc77c90e12 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -513,18 +514,37 @@ static int ufs_mtk_wait_link_state(struct ufs_hba *hba, u32 state,
>  static int ufs_mtk_09v_off(struct ufs_mtk_host *host)
>  {
>  	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));
> -		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));
> +
> +			ret2 = regulator_enable(host->reg_avdd09);

[Severity: High]
Can this cause a NULL pointer dereference?

Since host->reg_avdd09 is fetched using devm_regulator_get_optional(), it
can be NULL if it is absent from the device tree. If a system only has the
avdd09-1 supply configured and disabling it fails, this error path will
unconditionally call regulator_enable() with a NULL pointer, which will
cause a crash when the core API dereferences it.

> +			if (ret2)
> +				dev_err(host->hba->dev, "Failed to re-enable avdd09-supply: %pe\n",
> +					ERR_PTR(ret2));
> +
> +			ufs_mtk_va09_pwr_ctrl(res, 1);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -535,14 +555,26 @@ 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));
> +			regulator_disable(host->reg_avdd09);

[Severity: High]
Could this lead to a kernel panic if avdd09 is missing from the device tree?

Similar to the off path, if host->reg_avdd09 is NULL and enabling
reg_avdd09_1 fails during runtime, calling regulator_disable() on a NULL
pointer will result in an immediate crash.

Should there be a check to ensure host->reg_avdd09 is not NULL before
attempting to disable or enable it in these error paths?

> +			return ret;
> +		}
>  	}
>  
>  	ufs_mtk_va09_pwr_ctrl(res, 1);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-mt8196-ufs-v12-0-9279d7ef814d@collabora.com?part=8