Re: [PATCH v11 22/24] scsi: ufs: mediatek: Remove ret local from link_startup_notify

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of ufshcd_dme_set() is overwritten on MT6989 platforms without being checked, losing the error state.
- [High] The return value of ufs_mtk_auto_hibern8_disable() is completely ignored in ufs_mtk_pwr_change_notify().
--

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

scsi: ufs: mediatek: Remove ret local from link_startup_notify

This commit removes the unused local ret variable from
ufs_mtk_link_startup_notify. It also refactors the function to return
directly from the switch statement.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 4782fdf54866b..bcde58e3aee86 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1543,21 +1543,15 @@ static void ufs_mtk_post_link(struct ufs_hba *hba)
>  static int ufs_mtk_link_startup_notify(struct ufs_hba *hba,
>  				       enum ufs_notify_change_status stage)
>  {
> -	int ret = 0;
> -
>  	switch (stage) {
>  	case PRE_CHANGE:
> -		ret = ufs_mtk_pre_link(hba);
> -		break;
> +		return ufs_mtk_pre_link(hba);

[Severity: High]
This isn't a bug introduced by this patch, but looking at ufs_mtk_pre_link()
which is called here:

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_pre_link() {
	...
	ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_SAVEPOWERCONTROL), tmp);

	/* Enable the 1144 functions setting */
	if (host->ip_ver == IP_VER_MT6989) {
		ret = ufshcd_dme_get(hba, UIC_ARG_MIB(VS_DEBUGOMC), &tmp);
	...
}

Does this code unconditionally overwrite the return value of ufshcd_dme_set()
when the IP version matches MT6989 without being checked?

If ufshcd_dme_set() fails while disabling deep stall, the failure appears to
be silently ignored on these platforms.

Leaving deep stall inappropriately configured might lead to UFS bus lockups
and command timeouts.

>  	case POST_CHANGE:
>  		ufs_mtk_post_link(hba);
> -		break;
> -	default:
> -		ret = -EINVAL;
> -		break;
> +		return 0;
>  	}
>  
> -	return ret;
> +	return -EINVAL;
>  }

[ ... ]

[Severity: High]
This is also a pre-existing issue in this file, but while reviewing the notify
functions, I noticed this in ufs_mtk_pwr_change_notify():

drivers/ufs/host/ufs-mediatek.c:ufs_mtk_pwr_change_notify() {
	...
	case PRE_CHANGE:
		if (ufshcd_is_auto_hibern8_supported(hba)) {
			host->ahit = ufshcd_readl(
				hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
			ufs_mtk_auto_hibern8_disable(hba);
		}
		return ufs_mtk_pre_pwr_change(hba, dev_req_params);
	...
}

Is the return value of ufs_mtk_auto_hibern8_disable() intentionally ignored
here?

If it fails and returns an error like -EBUSY, the code proceeds to execute
ufs_mtk_pre_pwr_change().

Executing power mode changes while the link is stuck in hibern8 and error
recovery is pending could result in hardware lockups and unresponsiveness in
the storage subsystem.

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