Skip to contentSkip to conversation

Two minor fixes for UFS driver

PATCH Applied Can Guo 5 participants 14 messages Nov 3, 2020 – Nov 11, 2020 SCSI

lore
Share & tools
lore

Can Guo

author
#0 lore raw

cover Two minor fixes for UFS driver

This series contains two minor fixes, one for clk gating and one for PMC/UIC cmd completion timeout.

Can Guo (2):
scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()
scsi: ufs: Try to save power mode change and UIC cmd completion
timeout

 drivers/scsi/ufs/ufshcd.c | 32 +++++++++++++++++++++++++++-----
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 29 insertions(+), 5 deletions(-)
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author
#1 lore raw

PATCH 1/2 scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

The scsi_block_reqs_cnt increased in ufshcd_hold() is supposed to be decreased back in ufshcd_ungate_work() in a paired way. However, if specific ufshcd_hold/release sequences are met, it is possible that scsi_block_reqs_cnt is increased twice but only one ungate work is queued. To make sure scsi_block_reqs_cnt is handled by ufshcd_hold() and ufshcd_ungate_work() in a paired way, increase it only if queue_work() returns true.

Signed-off-by Can Guo <[email protected]>
Reviewed-by Hongwu Su <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +3 -3
index 847f355..efa7d86 100644
@@ -1634,12 +1634,12 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 */
 		/* fallthrough */
 	case CLKS_OFF:
-		ufshcd_scsi_block_requests(hba);
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		queue_work(hba->clk_gating.clk_gating_workq,
-			   &hba->clk_gating.ungate_work);
+		if (queue_work(hba->clk_gating.clk_gating_workq,
+			       &hba->clk_gating.ungate_work))
+			ufshcd_scsi_block_requests(hba);
 		/*
 		 * fall through to check if we should wait for this
 		 * work to be done or not.
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
#2 lore raw

Re: [PATCH v1 1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

Hi Can,

Quoted 5 lines · Can Guo

The scsi_block_reqs_cnt increased in ufshcd_hold() is supposed to be decreased back in ufshcd_ungate_work() in a paired way. However, if specific ufshcd_hold/release sequences are met, it is possible that scsi_block_reqs_cnt is increased twice but only one ungate work is queued. To make sure scsi_block_reqs_cnt is handled by ufshcd_hold() and

Just curious that how could this be possible? Would you have some failed examples?

Quoted 19 lines

ufshcd_ungate_work() in a paired way, increase it only if queue_work() returns true.

Signed-off-by Can Guo <[email protected]>
Reviewed-by Hongwu Su <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +0 -1
index 847f355..efa7d86 100644
@@ -1634,12 +1634,12 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 */
 		/* fallthrough */
 	case CLKS_OFF:
-		ufshcd_scsi_block_requests(hba);
 		hba->clk_gating.state = REQ_CLKS_ON;

drivers/scsi/ufs/ufshcd.c int ufshcd_hold(struct ufs_hba *hba, bool async) · line 1645

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +3 -2
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		queue_work(hba->clk_gating.clk_gating_workq,
-			   &hba->clk_gating.ungate_work);
+		if (queue_work(hba->clk_gating.clk_gating_workq,
+			       &hba->clk_gating.ungate_work))
+			ufshcd_scsi_block_requests(hba);
 		/*
 		 * fall through to check if we should wait for this
 		 * work to be done or not.

Thanks,
Stanley Chu

author
#3 lore raw

Re: [PATCH v1 1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

Quoted 12 lines · Stanley Chu

Hi Can,

Quoted 5 lines · Can Guo

The scsi_block_reqs_cnt increased in ufshcd_hold() is supposed to be decreased back in ufshcd_ungate_work() in a paired way. However, if specific ufshcd_hold/release sequences are met, it is possible that scsi_block_reqs_cnt is increased twice but only one ungate work is queued. To make sure scsi_block_reqs_cnt is handled by ufshcd_hold() and

Just curious that how could this be possible? Would you have some failed examples?

[1] One gate_work() is in the workqueue, not yet executed, now clk state == REQ_CLKS_OFF.
[2] ufshcd_queuecommand() calls ufshcd_hold(async == ture) -> active_req++ -> scsi_block_reqs_cnt++ -> REQ_CLKS_ON -> queue ungate work -> active_req-- -> return -EAGAIN.
[3] Now gate_work() starts to run, but since the clk state is REQ_CLKS_ON, gate_work() just sets clk state to CLKS_ON and bail.
[3] Someone calls ufshcd_hold(async == false) -> do something -> ufshcd_release() -> clk state is changed to REQ_CLKS_OFF. Note that, till now, ungate_work() is still in the work queue, not executed yet.
[4] Now, if someone calls ufshcd_hold(), we will hit the issue.

Above sequence is a very common clk gate/ungate sequence. The issue is because ungate_work is queued but cannot be executed in time. In my case, I see the ungate_work is somehow delayed for about 150ms. This change has been tested by customers on multiple platforms. And you can tell from the code that it won't break anything. :)

Thanks,

Can Guo.

Quoted 33 lines
Quoted 29 lines

ufshcd_ungate_work() in a paired way, increase it only if queue_work() returns true.

Signed-off-by Can Guo <[email protected]>
Reviewed-by Hongwu Su <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +3 -3
index 847f355..efa7d86 100644
@@ -1634,12 +1634,12 @@ int ufshcd_hold(struct ufs_hba *hba, bool 
 async)
 		 */
 		/* fallthrough */
 	case CLKS_OFF:
-		ufshcd_scsi_block_requests(hba);
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		queue_work(hba->clk_gating.clk_gating_workq,
-			   &hba->clk_gating.ungate_work);
+		if (queue_work(hba->clk_gating.clk_gating_workq,
+			       &hba->clk_gating.ungate_work))
+			ufshcd_scsi_block_requests(hba);
 		/*
 		 * fall through to check if we should wait for this
Quoted 1 line

* work to be done or not.

Thanks,
Stanley Chu

Reviewed-by
#4 lore raw

Re: [PATCH v1 1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

Hi Can,

Quoted 28 lines · Can Guo
Quoted 10 lines · Stanley Chu

Hi Can,

Quoted 6 lines · Can Guo

The scsi_block_reqs_cnt increased in ufshcd_hold() is supposed to be decreased back in ufshcd_ungate_work() in a paired way. However, if specific ufshcd_hold/release sequences are met, it is possible that scsi_block_reqs_cnt is increased twice but only one ungate work is queued. To make sure scsi_block_reqs_cnt is handled by ufshcd_hold() and

Quoted 4 lines

Just curious that how could this be possible? Would you have some failed
examples?

[1] One gate_work() is in the workqueue, not yet executed, now clk state == REQ_CLKS_OFF.
[2] ufshcd_queuecommand() calls ufshcd_hold(async == ture) -> active_req++ -> scsi_block_reqs_cnt++ -> REQ_CLKS_ON -> queue ungate work -> active_req-- -> return -EAGAIN.
[3] Now gate_work() starts to run, but since the clk state is REQ_CLKS_ON, gate_work() just sets clk state to CLKS_ON and bail.
[3] Someone calls ufshcd_hold(async == false) -> do something -> ufshcd_release() -> clk state is changed to REQ_CLKS_OFF. Note that, till now, ungate_work() is still in the work queue, not executed yet.
[4] Now, if someone calls ufshcd_hold(), we will hit the issue.

Quoted 5 lines

Above sequence is a very common clk gate/ungate sequence. The issue is because ungate_work is queued but cannot be executed in time. In my case, I see the ungate_work is somehow delayed for about 150ms. This change has been tested by customers on multiple platforms. And you can tell from the code that it won't break anything. :)

Thanks so much for the details. Looks good to me.

Reviewed-by Stanley Chu <[email protected]>
Quoted 38 lines

Thanks,

Can Guo.

Quoted 33 lines
Quoted 29 lines

ufshcd_ungate_work() in a paired way, increase it only if queue_work() returns true.

Signed-off-by Can Guo <[email protected]>
Reviewed-by Hongwu Su <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +3 -3
index 847f355..efa7d86 100644
@@ -1634,12 +1634,12 @@ int ufshcd_hold(struct ufs_hba *hba, bool 
 async)
 		 */
 		/* fallthrough */
 	case CLKS_OFF:
-		ufshcd_scsi_block_requests(hba);
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		queue_work(hba->clk_gating.clk_gating_workq,
-			   &hba->clk_gating.ungate_work);
+		if (queue_work(hba->clk_gating.clk_gating_workq,
+			       &hba->clk_gating.ungate_work))
+			ufshcd_scsi_block_requests(hba);
 		/*
 		 * fall through to check if we should wait for this
Quoted 1 line

* work to be done or not.

Thanks,
Stanley Chu

Reviewed-by on v1 1/2
#6 lore raw

Re: [PATCH v1 1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

In reply to Can Guo
Quoted 11 lines · Can Guo

The scsi_block_reqs_cnt increased in ufshcd_hold() is supposed to be decreased back in ufshcd_ungate_work() in a paired way. However, if specific ufshcd_hold/release sequences are met, it is possible that scsi_block_reqs_cnt is increased twice but only one ungate work is queued. To make sure scsi_block_reqs_cnt is handled by ufshcd_hold() and ufshcd_ungate_work() in a paired way, increase it only if queue_work() returns true.

Signed-off-by Can Guo <[email protected]>
Reviewed-by Hongwu Su <[email protected]>

---

Reviewed-by Asutosh Das <[email protected]>
Quoted 24 lines
  drivers/scsi/ufs/ufshcd.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/ufs/ufshcd.c +3 -3
index 847f355..efa7d86 100644
@@ -1634,12 +1634,12 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
  		 */
  		/* fallthrough */
  	case CLKS_OFF:
-		ufshcd_scsi_block_requests(hba);
  		hba->clk_gating.state = REQ_CLKS_ON;
  		trace_ufshcd_clk_gating(dev_name(hba->dev),
  					hba->clk_gating.state);
-		queue_work(hba->clk_gating.clk_gating_workq,
-			   &hba->clk_gating.ungate_work);
+		if (queue_work(hba->clk_gating.clk_gating_workq,
+			       &hba->clk_gating.ungate_work))
+			ufshcd_scsi_block_requests(hba);
  		/*
  		 * fall through to check if we should wait for this
  		 * work to be done or not.
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
author
#7 lore raw

PATCH 2/2 scsi: ufs: Try to save power mode change and UIC cmd completion timeout

In reply to Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd. The flag is set before send the UIC cmd and cleared in IRQ handler. When a PMC or UIC cmd completion timeout happens, if the flag is not set, instead of returning timeout error, we still treat it as a successful operation. This is to deal with the scenario in which completion has been raised but the one waiting for the completion cannot be awaken in time due to kernel scheduling problem.

Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +24 -2
index efa7d86..7f33310 100644
@@ -2122,10 +2122,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2157,6 +2167,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
 	return 0;

@@ -3828,10 +3839,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
 
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

@@ -4923,11 +4942,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
+		if (!hba->uic_async_done)
+			hba->active_uic_cmd->cmd_active = 0;
 		complete(&hba->active_uic_cmd->done);
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
+		hba->active_uic_cmd->cmd_active = 0;
 		complete(hba->uic_async_done);
 		retval = IRQ_HANDLED;
 	}
drivers/scsi/ufs/ufshcd.h +2 -0
index 66e5338..be982ed 100644
@@ -64,6 +64,7 @@ enum dev_cmd_type {
  * @argument1: UIC command argument 1
  * @argument2: UIC command argument 2
  * @argument3: UIC command argument 3
+ * @cmd_active: Indicate if UIC command is outstanding
  * @done: UIC command completion
  */
 struct uic_command {

@@ -71,6 +72,7 @@ struct uic_command {
 	u32 argument1;
 	u32 argument2;
 	u32 argument3;
+	int cmd_active;
 	struct completion done;
 };
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
Reviewed-by on v1 2/2
#8 lore raw

Re: [PATCH v1 2/2] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

Hi Can,

Except for below nit, otherwise looks good to me.

Reviewed-by Stanley Chu <[email protected]>
Quoted 69 lines · Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd. The flag is set before send the UIC cmd and cleared in IRQ handler. When a PMC or UIC cmd completion timeout happens, if the flag is not set, instead of returning timeout error, we still treat it as a successful operation. This is to deal with the scenario in which completion has been raised but the one waiting for the completion cannot be awaken in time due to kernel scheduling problem.

Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +21 -2
index efa7d86..7f33310 100644
@@ -2122,10 +2122,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2157,6 +2167,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
 	return 0;

@@ -3828,10 +3839,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
 
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

drivers/scsi/ufs/ufshcd.c static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status) · line 4945

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +1 -0
@@ -4923,11 +4942,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
+		if (!hba->uic_async_done)

Is this check necessary?

Quoted 23 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+			hba->active_uic_cmd->cmd_active = 0;
 		complete(&hba->active_uic_cmd->done);
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
+		hba->active_uic_cmd->cmd_active = 0;
 		complete(hba->uic_async_done);
 		retval = IRQ_HANDLED;
 	}
drivers/scsi/ufs/ufshcd.h +1 -0
index 66e5338..be982ed 100644
@@ -64,6 +64,7 @@ enum dev_cmd_type {
  * @argument1: UIC command argument 1
  * @argument2: UIC command argument 2
  * @argument3: UIC command argument 3
+ * @cmd_active: Indicate if UIC command is outstanding
  * @done: UIC command completion
  */
 struct uic_command {

drivers/scsi/ufs/ufshcd.h struct uic_command { · line 78

Quoted 7 lines
drivers/scsi/ufs/ufshcd.h +1 -0
@@ -71,6 +72,7 @@ struct uic_command {
 	u32 argument1;
 	u32 argument2;
 	u32 argument3;
+	int cmd_active;
 	struct completion done;
 };

Thanks,
Stanley Chu

author
#9 lore raw

Re: [PATCH v1 2/2] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

Hi Stanley,

Quoted 76 lines · Stanley Chu

Hi Can,

Except for below nit, otherwise looks good to me.

Reviewed-by Stanley Chu <[email protected]>
Quoted 69 lines · Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd. The flag is set before send the UIC cmd and cleared in IRQ handler. When a PMC or UIC cmd completion timeout happens, if the flag is not set, instead of returning timeout error, we still treat it as a successful operation. This is to deal with the scenario in which completion has been raised but the one waiting for the completion cannot be awaken in time due to kernel scheduling problem.

Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +12 -2
index efa7d86..7f33310 100644
@@ -2122,10 +2122,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, 
 struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the 
 result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
drivers/scsi/ufs/ufshcd.c +1 -0
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2157,6 +2167,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, 
 struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
drivers/scsi/ufs/ufshcd.c +7 -0
 	return 0;

@@ -3828,10 +3839,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
 *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been 
 completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
Quoted 13 lines

drivers/scsi/ufs/ufshcd.c static irqreturn_t ufshcd_uic_cmd_compl(struct · line 4946

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +1 -0
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
drivers/scsi/ufs/ufshcd.c +1 -0
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

@@ -4923,11 +4942,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct 
 ufs_hba *hba, u32 intr_status)
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
+		if (!hba->uic_async_done)

Is this check necessary?

Thanks for your quick response.

In the case of PMC, UIC cmd completion IRQ comes first, then power status change IRQ comes next. In this case, an UIC cmd's lifecyle ends only after the power status change IRQ comes [1].

I guess you may want to say that in current code since we have masked UIC cmd completion IRQ in the case of a PMC operation, so no need to check it here since we won't be here anyways before power status change IRQ comes. So, removing the check here definitely works, and then we won't even need below line as well.

	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
+		hba->active_uic_cmd->cmd_active = 0;
		complete(hba->uic_async_done);
		retval = IRQ_HANDLED;

If my guess is right, my opinion is that the current change may be more readable and comprehensive as it strictly follows my description in [1]. What do you think?

Thanks,

Can Guo.

Quoted 35 lines
Quoted 23 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+			hba->active_uic_cmd->cmd_active = 0;
 		complete(&hba->active_uic_cmd->done);
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
+		hba->active_uic_cmd->cmd_active = 0;
 		complete(hba->uic_async_done);
 		retval = IRQ_HANDLED;
drivers/scsi/ufs/ufshcd.c +0 -0
 	}
drivers/scsi/ufs/ufshcd.h +1 -0
index 66e5338..be982ed 100644
@@ -64,6 +64,7 @@ enum dev_cmd_type {
  * @argument1: UIC command argument 1
  * @argument2: UIC command argument 2
  * @argument3: UIC command argument 3
+ * @cmd_active: Indicate if UIC command is outstanding
  * @done: UIC command completion
  */
 struct uic_command {

drivers/scsi/ufs/ufshcd.h struct uic_command { · line 78

Quoted 7 lines
drivers/scsi/ufs/ufshcd.h +1 -0
@@ -71,6 +72,7 @@ struct uic_command {
 	u32 argument1;
 	u32 argument2;
 	u32 argument3;
+	int cmd_active;
 	struct completion done;
 };

Thanks,
Stanley Chu

#10 lore raw

Re: [PATCH v1 2/2] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

Hi Can,

Quoted 105 lines · Can Guo

Hi Stanley,

Quoted 82 lines · Stanley Chu

Hi Can,

Except for below nit, otherwise looks good to me.

Reviewed-by Stanley Chu <[email protected]>
Quoted 75 lines · Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd.
The flag is set before send the UIC cmd and cleared in IRQ handler. When a
PMC or UIC cmd completion timeout happens, if the flag is not set, instead
of returning timeout error, we still treat it as a successful operation.
This is to deal with the scenario in which completion has been raised but
the one waiting for the completion cannot be awaken in time due to kernel
scheduling problem.

Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +12 -2
index efa7d86..7f33310 100644
@@ -2122,10 +2122,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, 
 struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the 
 result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
drivers/scsi/ufs/ufshcd.c +1 -0
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2157,6 +2167,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, 
 struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
drivers/scsi/ufs/ufshcd.c +7 -0
 	return 0;

@@ -3828,10 +3839,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba 
 *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been 
 completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
Quoted 13 lines

drivers/scsi/ufs/ufshcd.c static irqreturn_t ufshcd_uic_cmd_compl(struct · line 4946

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +1 -0
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
drivers/scsi/ufs/ufshcd.c +1 -0
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

@@ -4923,11 +4942,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct 
 ufs_hba *hba, u32 intr_status)
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
+		if (!hba->uic_async_done)

Is this check necessary?

Thanks for your quick response.

In the case of PMC, UIC cmd completion IRQ comes first, then power status change IRQ comes next. In this case, an UIC cmd's lifecyle ends only after the power status change IRQ comes [1].

Quoted 6 lines

I guess you may want to say that in current code since we have masked UIC cmd completion IRQ in the case of a PMC operation, so no need to check it here since we won't be here anyways before power status change IRQ comes. So, removing the check here definitely works, and then we won't even need below line as well.

You read my mind : )

Quoted 8 lines
	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
(quoted diff) +1 -0
+		hba->active_uic_cmd->cmd_active = 0;

		complete(hba->uic_async_done);
		retval = IRQ_HANDLED;

If my guess is right, my opinion is that the current change may be more readable and comprehensive as it strictly follows my description in [1]. What do you think?

Both looks fine to me.

Thanks for the detailed description.

Stanley Chu

Quoted 40 lines

Thanks,

Can Guo.

Quoted 35 lines
Quoted 23 lines
drivers/scsi/ufs/ufshcd.c +2 -0
+			hba->active_uic_cmd->cmd_active = 0;
 		complete(&hba->active_uic_cmd->done);
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
+		hba->active_uic_cmd->cmd_active = 0;
 		complete(hba->uic_async_done);
 		retval = IRQ_HANDLED;
drivers/scsi/ufs/ufshcd.c +0 -0
 	}
drivers/scsi/ufs/ufshcd.h +1 -0
index 66e5338..be982ed 100644
@@ -64,6 +64,7 @@ enum dev_cmd_type {
  * @argument1: UIC command argument 1
  * @argument2: UIC command argument 2
  * @argument3: UIC command argument 3
+ * @cmd_active: Indicate if UIC command is outstanding
  * @done: UIC command completion
  */
 struct uic_command {

drivers/scsi/ufs/ufshcd.h struct uic_command { · line 78

Quoted 7 lines
drivers/scsi/ufs/ufshcd.h +1 -0
@@ -71,6 +72,7 @@ struct uic_command {
 	u32 argument1;
 	u32 argument2;
 	u32 argument3;
+	int cmd_active;
 	struct completion done;
 };

Thanks,
Stanley Chu

author
#11 lore raw

PATCH 1/1 scsi: ufs: Try to save power mode change and UIC cmd completion timeout

In reply to Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd. The flag is set before send the UIC cmd and cleared after the completion is raised in IRQ handler. For a power mode change operation, including hibern8 enter/exit, the flag is cleared only after hba->uic_async_done completion is raised. When completion timeout happens, if the flag is cleared, instead of returning timeout error, simply ignore it.

Change-Id Ie3cd6ae6221a44619925fb2cf78136a5617fdd5d
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +24 -2
index 252e022..8b291c3 100644
@@ -2131,10 +2131,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2166,6 +2176,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
 
 	return 0;

@@ -3944,10 +3955,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
 
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

@@ -5060,11 +5079,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
 		complete(&hba->active_uic_cmd->done);
+		if (!hba->uic_async_done)
+			hba->active_uic_cmd->cmd_active = 0;
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
 		complete(hba->uic_async_done);
+		hba->active_uic_cmd->cmd_active = 0;
 		retval = IRQ_HANDLED;
 	}
 	return retval;
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author
#12 lore raw

Re: [PATCH] scsi: ufs: Try to save power mode change and UIC cmd completion timeout

Sorry, please ignore this specific change, which is wrongly sent out.

Quoted 88 lines · Can Guo

Use the uic_cmd->cmd_active as a flag to track the lifecycle of an UIC cmd. The flag is set before send the UIC cmd and cleared after the completion is raised in IRQ handler. For a power mode change operation, including hibern8 enter/exit, the flag is cleared only after hba->uic_async_done completion is raised. When completion timeout happens, if the flag is cleared, instead of returning timeout error, simply ignore it.

Change-Id Ie3cd6ae6221a44619925fb2cf78136a5617fdd5d
Signed-off-by Can Guo <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
drivers/scsi/ufs/ufshcd.c +12 -2
index 252e022..8b291c3 100644
@@ -2131,10 +2131,20 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
 struct uic_command *uic_cmd)
 	unsigned long flags;
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT)))
+					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
-	else
+	} else {
 		ret = -ETIMEDOUT;
+		dev_err(hba->dev,
+			"uic cmd 0x%x with arg3 0x%x completion timeout\n",
+			uic_cmd->command, uic_cmd->argument3);
+
+		if (!uic_cmd->cmd_active) {
+			dev_err(hba->dev, "%s: UIC cmd has been completed, return the 
 result\n",
+				__func__);
+			ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+		}
+	}
drivers/scsi/ufs/ufshcd.c +1 -0
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	hba->active_uic_cmd = NULL;

@@ -2166,6 +2176,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba,
 struct uic_command *uic_cmd,
 	if (completion)
 		init_completion(&uic_cmd->done);
 
+	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
drivers/scsi/ufs/ufshcd.c +8 -0
 	return 0;

@@ -3944,10 +3955,18 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
 *hba, struct uic_command *cmd)
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
+
+		if (!cmd->cmd_active) {
+			dev_err(hba->dev, "%s: Power Mode Change operation has been
 completed, go check UPMCRS\n",
+				__func__);
+			goto check_upmcrs;
+		}
+
 		ret = -ETIMEDOUT;
 		goto out;
 	}
 
+check_upmcrs:
 	status = ufshcd_get_upmcrs(hba);
drivers/scsi/ufs/ufshcd.c +3 -0
 	if (status != PWR_LOCAL) {
 		dev_err(hba->dev,

@@ -5060,11 +5079,14 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct
 ufs_hba *hba, u32 intr_status)
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
 		complete(&hba->active_uic_cmd->done);
+		if (!hba->uic_async_done)
+			hba->active_uic_cmd->cmd_active = 0;
 		retval = IRQ_HANDLED;
 	}
 
 	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
 		complete(hba->uic_async_done);
+		hba->active_uic_cmd->cmd_active = 0;
 		retval = IRQ_HANDLED;
 	}

return retval;

Applied
#13 lore raw
In reply to Can Guo
Quoted 9 lines · Can Guo

This series contains two minor fixes, one for clk gating and one for PMC/UIC cmd completion timeout.

Can Guo (2):
scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()
scsi: ufs: Try to save power mode change and UIC cmd completion
timeout

[...]

Applied to 5.10/scsi-fixes, thanks!

[1/2] scsi: ufs: Fix unbalanced scsi_block_reqs_cnt caused by ufshcd_hold()

      https://git.kernel.org/mkp/scsi/c/da3fecb00403
[2/2] scsi: ufs: Try to save power mode change and UIC cmd completion timeout
      https://git.kernel.org/mkp/scsi/c/0f52fcb99ea2
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering