Re: [PATCH v10 15/26] scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths

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

Pre-existing issues:
- [High] Failures during hardware operations in `_ufs_mtk_clk_scale` are not propagated to callers, causing software state tracking and the UFS core to desynchronize from the actual hardware state.
--

--- Patch [15]: [PATCH v10 15/26] scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths ---
commit fc8d96d1151c17bc40e741378caf17dd49c71248
Author: Nicolas Frattaroli <[email protected]>

scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths

This commit reworks the error paths in _ufs_mtk_clk_scale to print errors
at the correct log level and adds missing gotos in the scale up case.

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

[ ... ]

> @@ -1989,51 +1989,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;

[Severity: High]
This is a pre-existing issue, but since the error paths are being reworked
here, should _ufs_mtk_clk_scale() be changed to return an int so these
errors can be propagated?

Currently, _ufs_mtk_clk_scale() returns void. When hardware operations like
regulator_set_voltage() or clk_set_parent() fail, the function jumps out
but drops the error code.

The caller ufs_mtk_clk_scale() then unconditionally updates the software
tracking state assuming success:

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

Could this desynchronize the driver and devfreq state from the hardware,
potentially causing the UFS core to overdrive the wrong clock parent or
fail to scale frequencies properly?

>  			}
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15