Re: [PATCH v11 24/24] scsi: ufs: mediatek: Add MT8196 compatible, update copyright

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Inconsistent hardware state on rollback failure in ufs_mtk_09v_off() leads to regulator state mismatch and potential crash.
--

commit 532592d1862d942dd09400d688803111fc2d12cb
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

scsi: ufs: mediatek: Add MT8196 compatible, update copyright
    
THe MT8196's UFS controller has a new compatible. Add the necessary
struct definitions to support it.
    
Also update the copyrights and authors, without tabs following spaces to
avoid checkpatch errors, to list myself as having contributed to this
driver after the preceding rework patches.

> THe MT8196's UFS controller has a new compatible. Add the necessary

This isn't a bug, but there is a typo (THe) in the commit message.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index a894810a83de7..e6a672888d043 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[ ... ]

> @@ -2254,10 +2260,18 @@ static const struct ufs_mtk_soc_data mt8192_8195_data = {
>  	.num_reg_names = ARRAY_SIZE(ufs_mtk_regs_avdd12_ckbuf_avdd18),
>  };
>  
> +static const struct ufs_mtk_soc_data mt8196_data = {
> +	.has_avdd09 = true,
> +	.has_avdd09_1 = true,

[Severity: High]
This is a pre-existing issue, but this commit explicitly exposes a dormant
regulator state mismatch bug because mt8196_data is the first to set
has_avdd09_1 to true.

In ufs_mtk_09v_off(), if host->reg_avdd09 is successfully disabled but
host->reg_avdd09_1 subsequently fails to disable, the function enters an
error rollback path:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_09v_off() {
...
	if (host->reg_avdd09) {
		ret = regulator_disable(host->reg_avdd09);
...
	}

	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;
		}
	}
...
}

It restores SMC power with ufs_mtk_va09_pwr_ctrl(res, 1) and returns an
error, but crucially misses calling regulator_enable(host->reg_avdd09) to
revert the previously disabled host->reg_avdd09 regulator.

This leaves the UFS PHY state marked as powered on, but the PHY is
physically disabled and its essential reg_avdd09 regulator is powered off.

Could subsequent accesses to the UFS controller attempt to operate on
unpowered hardware, likely causing a hardware hang or an asynchronous
external abort that crashes the system?

> +	.reg_names = ufs_mtk_regs_avdd12_ckbuf,
> +	.num_reg_names = ARRAY_SIZE(ufs_mtk_regs_avdd12_ckbuf),
> +};
> +

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