[PATCH v11 21/24] scsi: ufs: mediatek: Back up idle timer in per-instance struct

[sashiko-bot at kernel.org]

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

Pre-existing issues:
- [High] A failed power mode change will permanently disable the auto-hibern8 timer due to brittle backup/restore logic.
- [High] Concurrent sysfs write to `auto_hibern8` races with power mode changes, circumventing hardware constraints and losing user configuration.
--

commit dec217a616e37a6d6c6074f353ace1fc743596e2
Author: Nicolas Frattaroli <nicolas.frattaroli at collabora.com>

scsi: ufs: mediatek: Back up idle timer in per-instance struct

The MediaTek UFS driver uses a function-scope static variable to back up
a hardware register across a power change in the
ufs_mtk_pwr_change_notify function. This is dangerous, as it's only
correct if only ever one instance of the driver is loaded, which isn't
true if there's more than one device on a SoC that needs it, or it
otherwise gets loaded a second time.

> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index f405f511f41fd..4782fdf54866b 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1441,27 +1441,24 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
>  				enum ufs_notify_change_status stage,
>  				struct ufs_pa_layer_attr *dev_req_params)
>  {
> -	int ret = 0;
> -	static u32 reg;
> +	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
>  
>  	switch (stage) {
>  	case PRE_CHANGE:
>  		if (ufshcd_is_auto_hibern8_supported(hba)) {
> -			reg = ufshcd_readl(hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
> +			host->ahit = ufshcd_readl(
> +				hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
>  			ufs_mtk_auto_hibern8_disable(hba);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this brittle caching logic
risk permanently disabling the auto-hibernate feature?

In ufshcd_change_power_mode(), PRE_CHANGE is called first, which reads the
hardware timer value and disables it. If the subsequent
ufshcd_dme_change_power_mode() fails (e.g., due to a UIC command timeout),
POST_CHANGE is bypassed.

This leaves the hardware timer physically disabled (0).

On the next power mode change attempt, PRE_CHANGE reads the now-disabled
value (0) and overwrites host->ahit with 0. When POST_CHANGE eventually
succeeds, it restores 0, permanently disabling the feature until a reboot.

Could we instead restore from the core driver's hba->ahit, which reliably
tracks the configured value, rather than caching the physical hardware state
here?

> -		ret = ufs_mtk_pre_pwr_change(hba, dev_req_params);
> -		break;
> +		return ufs_mtk_pre_pwr_change(hba, dev_req_params);
>  	case POST_CHANGE:
>  		if (ufshcd_is_auto_hibern8_supported(hba))
> -			ufshcd_writel(hba, reg, REG_AUTO_HIBERNATE_IDLE_TIMER);
> -		break;
> -	default:
> -		ret = -EINVAL;
> -		break;
> +			ufshcd_writel(hba, host->ahit,
> +				      REG_AUTO_HIBERNATE_IDLE_TIMER);

[Severity: High]
This is a pre-existing issue, but does blindly restoring this cached value risk
losing user configuration and violating hardware constraints during a race
with sysfs?

When ufshcd_devfreq_scale() changes power modes, it holds clk_scaling_lock and
scan_mutex, but does not acquire hba->host_sem. Concurrently, a sysfs write to
the auto_hibern8 node invokes auto_hibern8_store(), which updates the hardware
register under hba->host_sem but without holding clk_scaling_lock.

If a sysfs write occurs immediately after PRE_CHANGE disabled the
auto-hibernate timer, the timer is re-enabled during
ufshcd_dme_change_power_mode(), violating the constraint this function is
trying to enforce.

Furthermore, when POST_CHANGE executes, it overwrites the register with the
stale host->ahit value, completely undoing the user's sysfs configuration.

> +		return 0;
>  	}

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