Re: [PATCH v12 16/24] scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The internal clock scaling function `_ufs_mtk_clk_scale()` returns `void` and does not propagate hardware configuration failures to its caller, causing silent software state desynchronization.
- [High] Clock scaling is silently disabled and triggers clock tree corruption if the optional FDE clock is missing from the device tree.
--

commit 6954740ae6011cf83d515e18a54406e92942ec76
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

    scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths

This commit reworks error paths in _ufs_mtk_clk_scale() to use the correct
logging levels via dev_err(). It also adds missing goto out statements in the
scale up case for consistency with the scale down branch.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 8e2aeb877de83..b5b13444d27cb 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c

[Severity: High]
This isn't a bug introduced by this patch, but I wanted to ask about the
early return just above this first hunk in _ufs_mtk_clk_scale():

	if (!clki || !fde_clki)
		return;

Since ufs_mtk_is_clk_scale_ready() does not require the FDE clock to be
present, does this early return incorrectly skip main clock scaling on
platforms without an FDE clock?

If this returns early, ufs_mtk_clk_scale() will unconditionally update the
clki->curr_freq state, and the UFS core might apply the new frequency to the
unswitched parent clock.

> @@ -2020,16 +2020,16 @@ static void _ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>  
>  	ret = clk_prepare_enable(clki->clk);
>  	if (ret) {
> -		dev_info(hba->dev,
> -			 "clk_prepare_enable() fail, ret: %d\n", ret);
> +		dev_err(hba->dev, "%s: Failed to enable clock: %pe\n", __func__, ERR_PTR(ret));
>  		return;
>  	}

[ ... ]

> @@ -2038,51 +2038,48 @@ static void _ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>  		if (clk_bind_vcore) {
>  			ret = regulator_set_voltage(reg, volt, INT_MAX);
>  			if (ret) {
> -				dev_info(hba->dev,
> -					"Failed to set vcore to %d\n", volt);
> +				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_info(hba->dev, "Failed to set clk mux, ret = %d\n",
> -				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 should _ufs_mtk_clk_scale() propagate
these errors back to the caller instead of returning void?

When this takes the "goto out" path on failure, the caller
ufs_mtk_clk_scale() assumes success and unconditionally applies the target
frequency to the software state:

ufs_mtk_clk_scale() {
    ...
	if (scale_up)
		_ufs_mtk_clk_scale(hba, true);
	else
		_ufs_mtk_clk_scale(hba, false);

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

Because ufs_mtk_clk_scale_notify() then returns 0, the UFS core proceeds to
call clk_set_rate() on the mux clock. If the parent was never switched,
does this apply the rate change to the original parent clock, potentially
corrupting the clock tree?

[ ... ]

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

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