[PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct

Nicolas Frattaroli posted 23 patches 1 month ago
There is a newer version of this series
[PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Nicolas Frattaroli 1 month ago
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.

Back it up into a member of the host struct instead, as this struct is
per-instance. Rework the function to not use a pointless "ret" local as
well.

Fixes: f5ca8d0c7a63 ("scsi: ufs: host: mediatek: Disable auto-hibern8 during power mode changes")
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/ufs/host/ufs-mediatek.c | 20 ++++++++------------
 drivers/ufs/host/ufs-mediatek.h |  1 +
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index ee677af6c700..046e2f9bb6c7 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1398,28 +1398,24 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
 				const struct ufs_pa_layer_attr *dev_max_params,
 				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->hibernate_idle_timer = ufshcd_readl(
+				hba, REG_AUTO_HIBERNATE_IDLE_TIMER);
 			ufs_mtk_auto_hibern8_disable(hba);
 		}
-		ret = ufs_mtk_pre_pwr_change(hba, dev_max_params,
-					     dev_req_params);
-		break;
+		return ufs_mtk_pre_pwr_change(hba, dev_max_params, 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->hibernate_idle_timer,
+				      REG_AUTO_HIBERNATE_IDLE_TIMER);
+		return 0;
 	}
 
-	return ret;
+	return -EINVAL;
 }
 
 static int ufs_mtk_unipro_set_lpm(struct ufs_hba *hba, bool lpm)
diff --git a/drivers/ufs/host/ufs-mediatek.h b/drivers/ufs/host/ufs-mediatek.h
index fa27ab4d6d6c..e5a3f70e7024 100644
--- a/drivers/ufs/host/ufs-mediatek.h
+++ b/drivers/ufs/host/ufs-mediatek.h
@@ -187,6 +187,7 @@ struct ufs_mtk_host {
 	u16 ref_clk_gating_wait_us;
 	u32 ip_ver;
 	bool legacy_ip_ver;
+	u32 hibernate_idle_timer;
 
 	bool mcq_set_intr;
 	bool is_mcq_intr_enabled;

-- 
2.53.0
Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Peter Wang (王信友) 3 weeks ago
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> @@ -187,6 +187,7 @@ struct ufs_mtk_host {
>  	u16 ref_clk_gating_wait_us;
>  	u32 ip_ver;
>  	bool legacy_ip_ver;
> +	u32 hibernate_idle_timer;

The name hibernate_idle_timer is somewhat confusing in
terms of its intended use. I would suggest using 
backup_ahit or saved_ahit instead.

Thanks
Peter

Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by AngeloGioacchino Del Regno 3 weeks ago
Il 25/02/26 11:35, Peter Wang (王信友) ha scritto:
> On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
>> @@ -187,6 +187,7 @@ struct ufs_mtk_host {
>>   	u16 ref_clk_gating_wait_us;
>>   	u32 ip_ver;
>>   	bool legacy_ip_ver;
>> +	u32 hibernate_idle_timer;
> 
> The name hibernate_idle_timer is somewhat confusing in
> terms of its intended use. I would suggest using
> backup_ahit or saved_ahit instead.
> 

In my opinion "ahit" is way less readable than "hibernate_idle_timer".

The hibernate_idle_timer member here stores the AUTO HIBERNATE IDLE TIMER, and
there is no other possible hibernation state in this driver.

Not sure why this could ever be confusing in terms of its intended use: its
intended use is to store the (auto) hibern8 idle timer, and the member is called
hibernate_idle_timer.

In my eyes, that matches 1:1 with its usage. Loud and clear.

Regards,
Angelo

> Thanks
> Peter
> 


Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Peter Wang (王信友) 2 weeks, 6 days ago
On Wed, 2026-02-25 at 13:40 +0100, AngeloGioacchino Del Regno wrote:
> In my opinion "ahit" is way less readable than
> "hibernate_idle_timer".
> 
> The hibernate_idle_timer member here stores the AUTO HIBERNATE IDLE
> TIMER, and
> there is no other possible hibernation state in this driver.
> 
> Not sure why this could ever be confusing in terms of its intended
> use: its
> intended use is to store the (auto) hibern8 idle timer, and the
> member is called
> hibernate_idle_timer.
> 
> In my eyes, that matches 1:1 with its usage. Loud and clear.
> 
> Regards,
> Angelo
> 
> 

Hi AngeloGioacchino,

If you want to refer to the AUTO HIBERNATE IDLE TIMER,
then you cannot omit "auto" because the UFS driver also
has a manual hibernate method.
However, "AUTO HIBERNATE IDLE TIMER" is too long, 
and we often use "ahit" instead (which is also used in 
the UFSHCI spec). For example:
https://elixir.bootlin.com/linux/v6.19.3/source/include/ufs/ufshcd.h#L978

So, if you want to distinguish it from hba->ahit, I suggest
using backup_ahit or saved_ahit instead.

Thanks
Peter
Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by AngeloGioacchino Del Regno 2 weeks, 6 days ago
Il 26/02/26 04:46, Peter Wang (王信友) ha scritto:
> On Wed, 2026-02-25 at 13:40 +0100, AngeloGioacchino Del Regno wrote:
>> In my opinion "ahit" is way less readable than
>> "hibernate_idle_timer".
>>
>> The hibernate_idle_timer member here stores the AUTO HIBERNATE IDLE
>> TIMER, and
>> there is no other possible hibernation state in this driver.
>>
>> Not sure why this could ever be confusing in terms of its intended
>> use: its
>> intended use is to store the (auto) hibern8 idle timer, and the
>> member is called
>> hibernate_idle_timer.
>>
>> In my eyes, that matches 1:1 with its usage. Loud and clear.
>>
>> Regards,
>> Angelo
>>
>>
> 
> Hi AngeloGioacchino,
> 
> If you want to refer to the AUTO HIBERNATE IDLE TIMER,
> then you cannot omit "auto" because the UFS driver also
> has a manual hibernate method.
> However, "AUTO HIBERNATE IDLE TIMER" is too long,
> and we often use "ahit" instead (which is also used in
> the UFSHCI spec). For example:
> https://elixir.bootlin.com/linux/v6.19.3/source/include/ufs/ufshcd.h#L978
> 
> So, if you want to distinguish it from hba->ahit, I suggest
> using backup_ahit or saved_ahit instead.
> 

Okay, does "saved_auto_hibern8_idle_tmr" sound good for you instead?

Regards,
Angelo

> Thanks
> Peter

Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Peter Wang (王信友) 2 weeks, 1 day ago
On Thu, 2026-02-26 at 11:36 +0100, AngeloGioacchino Del Regno wrote:
> 
> Okay, does "saved_auto_hibern8_idle_tmr" sound good for you instead?
> 
> Regards,
> Angelo
> 
> 

Hi AngeloGioacchino,

I’m fine with saved_auto_hibern8_idle_tmr, but it is more 
verbose compared to saved_ahit.

Thanks
Peter
Re: [PATCH v7 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Nicolas Frattaroli 2 weeks, 1 day ago
On Tuesday, 3 March 2026 09:01:14 Central European Standard Time Peter Wang (王信友) wrote:
> On Thu, 2026-02-26 at 11:36 +0100, AngeloGioacchino Del Regno wrote:
> > 
> > Okay, does "saved_auto_hibern8_idle_tmr" sound good for you instead?
> > 
> > Regards,
> > Angelo
> > 
> > 
> 
> Hi AngeloGioacchino,
> 
> I’m fine with saved_auto_hibern8_idle_tmr, but it is more 
> verbose compared to saved_ahit.
> 
> Thanks
> Peter
> 

Yeah no I won't change this, this is pointless bikeshedding.