[PATCH v2] scsi: ufs: core: call hibern8 notify when hibern8 cmd failed

[PATCH v2] scsi: ufs: core: call hibern8 notify when hibern8 cmd failed

[Hongjie Fang]

The vendor hibern8 notify callback always can be executed in the
PRE_CHANGE phase of hibern8 enter/exit. But it cannot be executed
in the POST_CHANGE phase if the hibern8 cmd fails.

When the hibern8 cmd fails, the vendor hibern8 notify callback
should still have the opportunity to execute.

Add a new argument to ufshcd_vops_hibern8_notify() that represents
whether or not the hibern8 cmd succeeded for POST_CHANGE callbacks.

Signed-off-by: Hongjie Fang <hongjiefang@asrmicro.com>
---
 drivers/ufs/core/ufshcd-priv.h |  5 +++--
 drivers/ufs/core/ufshcd.c      | 13 ++++++-------
 drivers/ufs/host/cdns-pltfrm.c |  4 ++--
 drivers/ufs/host/ufs-exynos.c  |  6 ++++--
 drivers/ufs/host/ufs-sprd.c    |  5 +++--
 include/ufs/ufshcd.h           |  3 ++-
 6 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 37c32071e754..1d3b90e280a9 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -188,10 +188,11 @@ static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
 
 static inline void ufshcd_vops_hibern8_notify(struct ufs_hba *hba,
 					enum uic_cmd_dme cmd,
-					enum ufs_notify_change_status status)
+					enum ufs_notify_change_status status,
+					int cmd_ret)
 {
 	if (hba->vops && hba->vops->hibern8_notify)
-		return hba->vops->hibern8_notify(hba, cmd, status);
+		return hba->vops->hibern8_notify(hba, cmd, status, cmd_ret);
 }
 
 static inline int ufshcd_vops_apply_dev_quirks(struct ufs_hba *hba)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9ceb6d6d479d..ec636a9b643d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4491,7 +4491,7 @@ int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 	ktime_t start = ktime_get();
 	int ret;
 
-	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE);
+	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, PRE_CHANGE, 0);
 
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(hba, "enter",
@@ -4500,9 +4500,8 @@ int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
 	if (ret)
 		dev_err(hba->dev, "%s: hibern8 enter failed. ret = %d\n",
 			__func__, ret);
-	else
-		ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER,
-								POST_CHANGE);
+
+	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER, POST_CHANGE, ret);
 
 	return ret;
 }
@@ -4516,7 +4515,7 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 	int ret;
 	ktime_t start = ktime_get();
 
-	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE);
+	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, PRE_CHANGE, 0);
 
 	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
 	trace_ufshcd_profile_hibern8(hba, "exit",
@@ -4526,12 +4525,12 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: hibern8 exit failed. ret = %d\n",
 			__func__, ret);
 	} else {
-		ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT,
-								POST_CHANGE);
 		hba->ufs_stats.last_hibern8_exit_tstamp = local_clock();
 		hba->ufs_stats.hibern8_exit_cnt++;
 	}
 
+	ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT, POST_CHANGE, ret);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_exit);
diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
index e793e3538c48..5822f23acd93 100644
--- a/drivers/ufs/host/cdns-pltfrm.c
+++ b/drivers/ufs/host/cdns-pltfrm.c
@@ -164,11 +164,11 @@ static int cdns_ufs_hce_enable_notify(struct ufs_hba *hba,
  * @status: notify stage (pre, post change)
  */
 static void cdns_ufs_hibern8_notify(struct ufs_hba *hba, enum uic_cmd_dme cmd,
-				    enum ufs_notify_change_status status)
+				    enum ufs_notify_change_status status, int cmd_ret)
 {
 	if (status == PRE_CHANGE && cmd == UIC_CMD_DME_HIBER_ENTER)
 		cdns_ufs_get_l4_attr(hba);
-	if (status == POST_CHANGE && cmd == UIC_CMD_DME_HIBER_EXIT)
+	if (status == POST_CHANGE && cmd == UIC_CMD_DME_HIBER_EXIT && !cmd_ret)
 		cdns_ufs_set_l4_attr(hba);
 }
 
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 76fee3a79c77..83f19a3411c5 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1699,14 +1699,16 @@ static int exynos_ufs_pwr_change_notify(struct ufs_hba *hba,
 
 static void exynos_ufs_hibern8_notify(struct ufs_hba *hba,
 				     enum uic_cmd_dme cmd,
-				     enum ufs_notify_change_status notify)
+				     enum ufs_notify_change_status notify,
+				     int cmd_ret)
 {
 	switch ((u8)notify) {
 	case PRE_CHANGE:
 		exynos_ufs_pre_hibern8(hba, cmd);
 		break;
 	case POST_CHANGE:
-		exynos_ufs_post_hibern8(hba, cmd);
+		if (!cmd_ret)
+			exynos_ufs_post_hibern8(hba, cmd);
 		break;
 	}
 }
diff --git a/drivers/ufs/host/ufs-sprd.c b/drivers/ufs/host/ufs-sprd.c
index 65bd8fb96b99..70423a766b32 100644
--- a/drivers/ufs/host/ufs-sprd.c
+++ b/drivers/ufs/host/ufs-sprd.c
@@ -352,7 +352,8 @@ static int sprd_ufs_n6_hce_enable_notify(struct ufs_hba *hba,
 
 static void sprd_ufs_n6_h8_notify(struct ufs_hba *hba,
 				  enum uic_cmd_dme cmd,
-				  enum ufs_notify_change_status status)
+				  enum ufs_notify_change_status status,
+				  int cmd_ret)
 {
 	struct ufs_sprd_priv *priv = ufs_sprd_get_priv_data(hba);
 
@@ -372,7 +373,7 @@ static void sprd_ufs_n6_h8_notify(struct ufs_hba *hba,
 		}
 	}
 
-	if (status == POST_CHANGE) {
+	if (status == POST_CHANGE && !cmd_ret) {
 		if (cmd == UIC_CMD_DME_HIBER_EXIT)
 			ufs_sprd_ctrl_uic_compl(hba, true);
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8563b6648976..c1cb30d8aa4a 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -355,7 +355,8 @@ struct ufs_hba_variant_ops {
 				  bool is_scsi_cmd);
 	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
 	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
-					enum ufs_notify_change_status);
+					enum ufs_notify_change_status,
+					int cmd_ret);
 	int	(*apply_dev_quirks)(struct ufs_hba *hba);
 	void	(*fixup_dev_quirks)(struct ufs_hba *hba);
 	int     (*suspend)(struct ufs_hba *, enum ufs_pm_op,
-- 
2.25.1

Re: [PATCH v2] scsi: ufs: core: call hibern8 notify when hibern8 cmd failed

[Bart Van Assche]

On 4/29/26 4:23 AM, Hongjie Fang wrote:
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 8563b6648976..c1cb30d8aa4a 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -355,7 +355,8 @@ struct ufs_hba_variant_ops {
>   				  bool is_scsi_cmd);
>   	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
>   	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
> -					enum ufs_notify_change_status);
> +					enum ufs_notify_change_status,
> +					int cmd_ret);
>   	int	(*apply_dev_quirks)(struct ufs_hba *hba);
>   	void	(*fixup_dev_quirks)(struct ufs_hba *hba);
>   	int     (*suspend)(struct ufs_hba *, enum ufs_pm_op,

Passing the full 'cmd_ret' value to the hibern8_notify vendor operation 
may make the UFS driver harder to maintain than necessary.
Implementations of this vendor operation may test for specific values of
'cmd_ret'. Hence, when making any change in the code that calls
.hibern8_notify() regarding the return value, all implementations of
.hibern8_notify() would have to be reviewed.

Has it been considered to add a third value in enum 
ufs_notify_change_status, e.g. ROLLBACK_CHANGE? That should be
sufficient for hibern8_notify implementations, isn't it?

Thanks,

Bart.

RE: [PATCH v2] scsi: ufs: core: call hibern8 notify when hibern8 cmd failed

[Fang Hongjie(方洪杰)]

> From: Bart Van Assche [mailto:bvanassche@acm.org]
> Sent: Thursday, April 30, 2026 12:23 AM
> To: Fang Hongjie <hongjiefang@asrmicro.com>;
> alim.akhtar@samsung.com; avri.altman@wdc.com;
> James.Bottomley@HansenPartnership.com; martin.petersen@oracle.com
> Cc: linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] scsi: ufs: core: call hibern8 notify when hibern8 cmd
> failed
> 
> On 4/29/26 4:23 AM, Hongjie Fang wrote:
> > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> > index 8563b6648976..c1cb30d8aa4a 100644
> > --- a/include/ufs/ufshcd.h
> > +++ b/include/ufs/ufshcd.h
> > @@ -355,7 +355,8 @@ struct ufs_hba_variant_ops {
> >   				  bool is_scsi_cmd);
> >   	void	(*setup_task_mgmt)(struct ufs_hba *, int, u8);
> >   	void    (*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
> > -					enum ufs_notify_change_status);
> > +					enum ufs_notify_change_status,
> > +					int cmd_ret);
> >   	int	(*apply_dev_quirks)(struct ufs_hba *hba);
> >   	void	(*fixup_dev_quirks)(struct ufs_hba *hba);
> >   	int     (*suspend)(struct ufs_hba *, enum ufs_pm_op,
> 
> Passing the full 'cmd_ret' value to the hibern8_notify vendor operation
> may make the UFS driver harder to maintain than necessary.
> Implementations of this vendor operation may test for specific values of
> 'cmd_ret'. Hence, when making any change in the code that calls
> .hibern8_notify() regarding the return value, all implementations of
> .hibern8_notify() would have to be reviewed.
> 
> Has it been considered to add a third value in enum
> ufs_notify_change_status, e.g. ROLLBACK_CHANGE? That should be
> sufficient for hibern8_notify implementations, isn't it?
> 

Adding a third enum value to handle this is the more reasonable approach. 
This would not affect the existing vendor implementations, 
and the change would be confined to internal functions within the ufs core.

Pass `ROLLBACK_CHANGE` when the hibern8 command returns a failure, 
and use `POST_CHANGE` otherwise.

> Thanks,
> 
> Bart.

Best.