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

Nicolas Frattaroli posted 23 patches 1 week, 5 days ago
[PATCH v9 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Nicolas Frattaroli 1 week, 5 days 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 c4e70fb99e82..2198271a269a 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->ahit = 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->ahit,
+				      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..2349d9b9375c 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 ahit;
 
 	bool mcq_set_intr;
 	bool is_mcq_intr_enabled;

-- 
2.53.0
Re: [PATCH v9 20/23] scsi: ufs: mediatek: Back up idle timer in per-instance struct
Posted by Chaotian Jing (井朝天) 1 day, 2 hours ago
On Fri, 2026-03-06 at 14:25 +0100, Nicolas Frattaroli wrote:
> 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>
Reviewed-by: Chaotian Jing <chaotian.jing@mediatek.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 c4e70fb99e82..2198271a269a 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->ahit = 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->ahit,
> +				      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..2349d9b9375c 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 ahit;
>  
>  	bool mcq_set_intr;
>  	bool is_mcq_intr_enabled;
>