Re: [PATCH v12 23/24] scsi: ufs: mediatek: Remove undocumented "clk-scale-up-vcore-min"

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clock scaling state is updated unconditionally even if clock operations fail, and partial reparenting is not reverted.
--

commit 42aaaa81d59b5f138bb60c0e81306845236f3259
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

scsi: ufs: mediatek: Remove undocumented "clk-scale-up-vcore-min"

The MediaTek UFS driver contains support for an undocumented,
non-vendor-prefixed u32 property named "clk-scale-up-vcore-min".
Since it is not part of any binding, this patch removes it.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 2ebf7a3118dab..ef8924704c902 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -2000,14 +1976,6 @@ static void _ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>  	}
>  
>  	if (scale_up) {
> -		if (clk_bind_vcore) {
> -			ret = regulator_set_voltage(host->reg_vcore, volt, INT_MAX);
> -			if (ret) {
> -				dev_err(hba->dev, "Failed to set vcore to %d\n", volt);
> -				goto out;
> -			}
> -		}
> -
>  		ret = clk_set_parent(clki->clk, mclk->ufs_sel_max_clki->clk);
>  		if (ret) {
>  			dev_err(hba->dev, "%s: Failed to set clock mux: %pe\n",
>  				__func__, ERR_PTR(ret));
>  			goto out;
>  		}

[Severity: High]
This is a pre-existing issue, but I noticed that if clk_set_parent() succeeds
here, but the subsequent reparenting for fde_clki->clk fails, the function
jumps to the cleanup block without reverting this successful parent switch:

		if (clk_fde_scale) {
			ret = clk_set_parent(fde_clki->clk, mclk->ufs_fde_max_clki->clk);
			if (ret) {
				dev_err(hba->dev, "%s: Failed to set fde clock mux: %pe\n",
					__func__, ERR_PTR(ret));
				goto out;
			}
		}

Because _ufs_mtk_clk_scale() returns void, the caller ufs_mtk_clk_scale() has
no way to know that the hardware operations partially failed, and it
unconditionally updates the software state anyway:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_clk_scale() {
    ...
	_ufs_mtk_clk_scale(hba, scale_up);

	host->clk_scale_up = scale_up;

	/* Must always set before clk_set_rate() */
	if (scale_up)
		clki->curr_freq = clki->max_freq;
	else
		clki->curr_freq = clki->min_freq;
    ...
}

Will this permanently desynchronize the hardware and software states if clock
reparenting fails, leaving the UFS controller in a broken mixed-clock state
where future scaling requests are ignored?

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