Re: [PATCH 2/2] scsi: ufs: core: Fix the code for entering hibernation

[Bart Van Assche <bvanassche@acm.org>]

On 8/26/24 6:39 PM, Peter Wang (王信友) wrote:
> It is not a new vendor hook, ufshcd_vops_hibern8_notify is exist
> in current kernel.
Hi Peter,

Is something like the untested patch below perhaps what you have in
mind?

Thanks,

Bart.


diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index ce36154ce963..69ee49a75c04 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -176,12 +176,14 @@ static inline void 
ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
  		return hba->vops->setup_task_mgmt(hba, tag, tm_function);
  }

-static inline void ufshcd_vops_hibern8_notify(struct ufs_hba *hba,
-					enum uic_cmd_dme cmd,
-					enum ufs_notify_change_status status)
+static inline void
+ufshcd_vops_hibern8_notify(struct ufs_hba *hba, enum uic_cmd_dme cmd,
+			   enum ufs_notify_change_status status,
+			   bool *disable_uic_compl_intr)
  {
  	if (hba->vops && hba->vops->hibern8_notify)
-		return hba->vops->hibern8_notify(hba, cmd, status);
+		return hba->vops->hibern8_notify(hba, cmd, status,
+						 disable_uic_compl_intr);
  }

  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 e13b9ac145f6..614b24f2eb7f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2541,9 +2541,8 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, 
struct uic_command *uic_cmd)
   *
   * Return: 0 only if success.
   */
-static int
-__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
-		      bool completion)
+static int __ufshcd_send_uic_cmd(struct ufs_hba *hba,
+				 struct uic_command *uic_cmd)
  {
  	lockdep_assert_held(&hba->uic_cmd_mutex);

@@ -2553,8 +2552,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct 
uic_command *uic_cmd,
  		return -EIO;
  	}

-	if (completion)
-		init_completion(&uic_cmd->done);
+	init_completion(&uic_cmd->done);

  	uic_cmd->cmd_active = 1;
  	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
@@ -2580,7 +2578,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, 
struct uic_command *uic_cmd)
  	mutex_lock(&hba->uic_cmd_mutex);
  	ufshcd_add_delay_before_dme_cmd(hba);

-	ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
+	ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
  	if (!ret)
  		ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);

@@ -4243,7 +4241,8 @@ EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
   *
   * Return: 0 on success, non-zero value on failure.
   */
-static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command 
*cmd)
+static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command 
*cmd,
+			       bool disable_uic_compl_intr)
  {
  	DECLARE_COMPLETION_ONSTACK(uic_async_done);
  	unsigned long flags;
@@ -4260,7 +4259,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  		goto out_unlock;
  	}
  	hba->uic_async_done = &uic_async_done;
-	if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
+	if (disable_uic_compl_intr &&
+	    ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
  		ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
  		/*
  		 * Make sure UIC command completion interrupt is disabled before
@@ -4270,7 +4270,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  		reenable_intr = true;
  	}
  	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
+	ret = __ufshcd_send_uic_cmd(hba, cmd);
  	if (ret) {
  		dev_err(hba->dev,
  			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
@@ -4294,6 +4294,16 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  		goto out;
  	}

+	if (!disable_uic_compl_intr &&
+	    ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
+		ret = wait_for_completion_timeout(
+			&cmd->done, msecs_to_jiffies(uic_cmd_timeout));
+		if (ret == 0)
+			dev_err(hba->dev, "pwr ctrl cmd %#x timed out\n",
+				cmd->command);
+		ret = 0;
+	}
+
  check_upmcrs:
  	status = ufshcd_get_upmcrs(hba);
  	if (status != PWR_LOCAL) {
@@ -4353,7 +4363,7 @@ int ufshcd_uic_change_pwr_mode(struct ufs_hba 
*hba, u8 mode)
  	}

  	ufshcd_hold(hba);
-	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
+	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd, true);
  	ufshcd_release(hba);

  out:
@@ -4396,11 +4406,13 @@ int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
  		.command = UIC_CMD_DME_HIBER_ENTER,
  	};
  	ktime_t start = ktime_get();
+	bool disable_uic_compl_intr = true;
  	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,
+				   &disable_uic_compl_intr);

-	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
+	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd, disable_uic_compl_intr);
  	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "enter",
  			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);

@@ -4409,7 +4421,7 @@ int ufshcd_uic_hibern8_enter(struct ufs_hba *hba)
  			__func__, ret);
  	else
  		ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_ENTER,
-								POST_CHANGE);
+					   POST_CHANGE, NULL);

  	return ret;
  }
@@ -4423,9 +4435,10 @@ 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,
+				   NULL);

-	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd);
+	ret = ufshcd_uic_pwr_ctrl(hba, &uic_cmd, true);
  	trace_ufshcd_profile_hibern8(dev_name(hba->dev), "exit",
  			     ktime_to_us(ktime_sub(ktime_get(), start)), ret);

@@ -4434,7 +4447,7 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
  			__func__, ret);
  	} else {
  		ufshcd_vops_hibern8_notify(hba, UIC_CMD_DME_HIBER_EXIT,
-								POST_CHANGE);
+					   POST_CHANGE, NULL);
  		hba->ufs_stats.last_hibern8_exit_tstamp = local_clock();
  		hba->ufs_stats.hibern8_exit_cnt++;
  	}
diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
index 66811d8d1929..24c05e5c455d 100644
--- a/drivers/ufs/host/cdns-pltfrm.c
+++ b/drivers/ufs/host/cdns-pltfrm.c
@@ -164,7 +164,8 @@ 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,
+				    bool *disable_uic_compl_intr)
  {
  	if (status == PRE_CHANGE && cmd == UIC_CMD_DME_HIBER_ENTER)
  		cdns_ufs_get_l4_attr(hba);
diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 16ad3528d80b..e991d9e5e2e4 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1624,8 +1624,9 @@ 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 enter,
-				     enum ufs_notify_change_status notify)
+				      enum uic_cmd_dme enter,
+				      enum ufs_notify_change_status notify,
+				      bool *disable_uic_compl_intr)
  {
  	switch ((u8)notify) {
  	case PRE_CHANGE:
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a43b14276bc3..59b901d67400 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -355,8 +355,9 @@ struct ufs_hba_variant_ops {
  	void	(*setup_xfer_req)(struct ufs_hba *hba, int tag,
  				  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);
+	void	(*hibern8_notify)(struct ufs_hba *, enum uic_cmd_dme,
+				  enum ufs_notify_change_status,
+				  bool *disable_uic_compl_intr);
  	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,