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

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

On 8/22/24 8:43 PM, Bao D. Nguyen wrote:
> However, the UIC_COMMAND_COMPL extra interrupt 
> belonging to the previous PMC/hibern8_enter/exit() command can come late 
> and causes the ufshcd_uic_cmd_compl() to complete the current uic 
> command incorrectly.

Hi Bao,

If the UIC command completion interrupt could come late we would
already have observed unhandled interrupt errors in device logs, isn't
it?

Anyway, isn't this something that is easy to fix with something like the
(untested) patch below?

Thanks,

Bart.

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d0ae6e50becc..e3a487ea83f9 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2543,13 +2543,11 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, 
struct uic_command *uic_cmd)
   * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
   * @hba: per adapter instance
   * @uic_cmd: UIC command
- * @completion: initialize the completion only if this is set to true
   *
   * Return: 0 only if success.
   */
  static int
-__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
-		      bool completion)
+__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
  {
  	lockdep_assert_held(&hba->uic_cmd_mutex);

@@ -2559,8 +2557,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);
@@ -2586,7 +2583,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);

@@ -4255,7 +4252,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  	unsigned long flags;
  	u8 status;
  	int ret;
-	bool reenable_intr = false;

  	mutex_lock(&hba->uic_cmd_mutex);
  	ufshcd_add_delay_before_dme_cmd(hba);
@@ -4266,17 +4262,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) {
-		ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
-		/*
-		 * Make sure UIC command completion interrupt is disabled before
-		 * issuing UIC command.
-		 */
-		ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
-		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",
@@ -4300,6 +4287,13 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  		goto out;
  	}

+	ret = wait_for_completion_timeout(&cmd->done,
+					  msecs_to_jiffies(uic_cmd_timeout));
+	WARN_ON_ONCE(ret < 0);
+	if (ret == 0)
+		dev_err(hba->dev, "UIC command %#x timed out\n", cmd->command);
+	ret = 0;
+
  check_upmcrs:
  	status = ufshcd_get_upmcrs(hba);
  	if (status != PWR_LOCAL) {
@@ -4318,8 +4312,6 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
*hba, struct uic_command *cmd)
  	spin_lock_irqsave(hba->host->host_lock, flags);
  	hba->active_uic_cmd = NULL;
  	hba->uic_async_done = NULL;
-	if (reenable_intr)
-		ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
  	if (ret) {
  		ufshcd_set_link_broken(hba);
  		ufshcd_schedule_eh_work(hba);