Re: [PATCH v11 20/24] scsi: ufs: mediatek: Rework hardware version reading

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Ignored return values from `ufshcd_dme_get` lead to uninitialized stack variable usage.
- [High] Missing return value checks for `phy_power_on` and `ufs_mtk_mphy_power_on`.
--

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

scsi: ufs: mediatek: Rework hardware version reading

Split assignment to the host struct out from the read function, and
utilise bitfield helpers to simplify the code. Also move the debug print
out of the legacy version helper, which means it no longer has to take a
struct ufs_hba as an input, and can be rewritten as a pure function.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 4e916d5ac2af7..f405f511f41fd 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
[ ... ]
> @@ -1232,7 +1229,11 @@ static int ufs_mtk_init(struct ufs_hba *hba)
>  
>  	ufs_mtk_setup_clocks(hba, true, POST_CHANGE);
>  
> -	ufs_mtk_get_hw_ip_version(hba);
> +	host->ip_ver = ufs_mtk_get_hw_ip_version(hba);
> +	host->legacy_ip_ver = ufs_mtk_is_legacy_chipset(host->ip_ver);
> +
> +	dev_dbg(hba->dev, "IP version 0x%x, legacy = %s", host->ip_ver,
> +		str_true_false(host->legacy_ip_ver));
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does ufs_mtk_init() ignore the return value of
ufs_mtk_mphy_power_on() when it is called earlier in this same function?

If ufs_mtk_mphy_power_on() fails (e.g. because regulators fail), probe
continues and eventually returns 0 (success). Attempting to communicate
over an unpowered PHY or accessing its registers can result in an
asynchronous bus fault, device timeout, or kernel hang.

Looking at ufs_mtk_mphy_power_on(), it also seems to ignore the error code
returned by the generic PHY power-on routine, incorrectly returning 0
(success) when phy_power_on(mphy) is called:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_mphy_power_on() {
    ...
		ret = ufs_mtk_09v_on(host);
		if (ret)
			return ret;
		phy_power_on(mphy);
	} else {
    ...
}

[Severity: High]
This is also a pre-existing issue and was not introduced by the patch under
review, but do we use uninitialized stack variables in ufs_mtk_cfg_unipro_cg()
and ufs_mtk_post_link() if ufshcd_dme_get() fails?

In ufs_mtk_cfg_unipro_cg():

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_cfg_unipro_cg() {
    ...
		ufshcd_dme_get(hba,
			       UIC_ARG_MIB(VS_SAVEPOWERCONTROL), &tmp);
		tmp = tmp |
		      (1 << RX_SYMBOL_CLK_GATE_EN) |
		      (1 << SYS_CLK_GATE_EN) |
		      (1 << TX_CLK_GATE_EN);
		ufshcd_dme_set(hba,
			       UIC_ARG_MIB(VS_SAVEPOWERCONTROL), tmp);
    ...
}

If ufshcd_dme_get() times out or fails due to intermittent link issues, tmp
retains stack garbage. Modifying and writing this garbage back to
VS_SAVEPOWERCONTROL can corrupt the UFS configuration, break the link, or
hang the bus.

A similar issue appears in ufs_mtk_post_link():

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_post_link() {
    ...
		ufshcd_dme_get(hba, UIC_ARG_MIB(VS_DEBUGOMC), &tmp);
		tmp |= 0x100;
		ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), tmp);
    ...
}

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