[PATCH v2 0/3] scsi: ufs: fix broken hba->outstanding_tasks

[PATCH v2 0/3] scsi: ufs: fix broken hba->outstanding_tasks

[Stanley Chu]

Currently bits in hba->outstanding_tasks are cleared only after their
corresponding task management commands are successfully done by
__ufshcd_issue_tm_cmd().

If timeout happens in a task management command, its corresponding
bit in hba->outstanding_tasks will not be cleared until next task
management command with the same tag used successfully finishes.

This is wrong and can lead to some issues, like power issue.
For example, ufshcd_release() and ufshcd_gate_work() will do nothing
if hba->outstanding_tasks is not zero even if both UFS host and devices
are actually idle.

Referring to error handling flow of hba->outstanding_reqs, all timed-out
bits will be cleared by
ufshcd_reset_and_restore() => ufshcd_transfer_req_compl()
after reset is done. Therefore similar handling for hba->outstanding_tasks
could be applied, for example, by
ufshcd_reset_and_restore() => ufshcd_tmc_handler().

This patch tries to "re-factor" cleanup jobs first, and then add fixed
flow to make the whole patch more readable.

Stanley Chu (3):
  scsi: ufs: clean-up task resource immediately only if task is
    responded
  scsi: ufs: introduce ufshcd_tm_cmd_compl() to refactor task cleanup
  scsi: ufs: fix broken hba->outstanding_tasks

 drivers/scsi/ufs/ufshcd.c | 41 ++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

-- 
2.18.0

[PATCH v2 1/3] scsi: ufs: clean-up task resource immediately only if task is responded

[Stanley Chu]

In __ufshcd_issue_tm_cmd(), if host is unable to clear TM command by
setting "clear register", the TM command may be still "outstanding"
in the device. In this case, it may be better to do cleanup after reset
is done. Therefore let __ufshcd_issue_tm_cmd() clean-up task resource
immediately only if task is responded.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3804a704e565..66c8e7402001 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5628,6 +5628,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 	struct Scsi_Host *host = hba->host;
 	unsigned long flags;
 	int free_slot, task_tag, err;
+	bool cleanup = true;
 
 	/*
 	 * Get free slot, sleep if slots are unavailable.
@@ -5667,26 +5668,33 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 		ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete_err");
 		dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
 				__func__, tm_function);
-		if (ufshcd_clear_tm_cmd(hba, free_slot))
+		if (ufshcd_clear_tm_cmd(hba, free_slot)) {
 			dev_WARN(hba->dev, "%s: unable clear tm cmd (slot %d) after timeout\n",
 					__func__, free_slot);
+			/*
+			 * unable to clear task, cleanup shall be done
+			 * during error handling
+			 */
+			cleanup = false;
+		}
 		err = -ETIMEDOUT;
 	} else {
 		err = 0;
 		memcpy(treq, hba->utmrdl_base_addr + free_slot, sizeof(*treq));
 
 		ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete");
+	}
 
+	if (likely(cleanup)) {
 		spin_lock_irqsave(hba->host->host_lock, flags);
 		__clear_bit(free_slot, &hba->outstanding_tasks);
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
 
+		clear_bit(free_slot, &hba->tm_condition);
+		ufshcd_put_tm_slot(hba, free_slot);
+		wake_up(&hba->tm_tag_wq);
 	}
 
-	clear_bit(free_slot, &hba->tm_condition);
-	ufshcd_put_tm_slot(hba, free_slot);
-	wake_up(&hba->tm_tag_wq);
-
 	ufshcd_release(hba);
 	return err;
 }
-- 
2.18.0

[PATCH v2 2/3] scsi: ufs: introduce ufshcd_tm_cmd_compl() to refactor task cleanup

[Stanley Chu]

Introduce ufshcd_tm_cmd_compl() to re-factor taks cleanup jobs
to make code more readable and for future wider usage by task error
handling.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 66c8e7402001..114c15ed75f7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5522,6 +5522,13 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 	 */
 }
 
+static void ufshcd_tm_cmd_compl(struct ufs_hba *hba, int tag)
+{
+	__clear_bit(tag, &hba->outstanding_tasks);
+	__clear_bit(tag, &hba->tm_condition);
+	ufshcd_put_tm_slot(hba, tag);
+}
+
 /**
  * ufshcd_tmc_handler - handle task management function completion
  * @hba: per adapter instance
@@ -5687,11 +5694,9 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 
 	if (likely(cleanup)) {
 		spin_lock_irqsave(hba->host->host_lock, flags);
-		__clear_bit(free_slot, &hba->outstanding_tasks);
+		ufshcd_tm_cmd_compl(hba, free_slot);
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
 
-		clear_bit(free_slot, &hba->tm_condition);
-		ufshcd_put_tm_slot(hba, free_slot);
 		wake_up(&hba->tm_tag_wq);
 	}
 
-- 
2.18.0

[PATCH v2 3/3] scsi: ufs: fix broken hba->outstanding_tasks

[Stanley Chu]

Currently bits in hba->outstanding_tasks are cleared only after their
corresponding task management commands are successfully done by
__ufshcd_issue_tm_cmd().

If timeout happens in a task management command, its corresponding
bit in hba->outstanding_tasks will not be cleared until next task
management command with the same tag used successfully finishes.

This is wrong and can lead to some issues, like power issue.
For example, ufshcd_release() and ufshcd_gate_work() will do nothing
if hba->outstanding_tasks is not zero even if both UFS host and devices
are actually idle.

Referring to error handling flow of hba->outstanding_reqs, all timed-out
bits will be cleared by
ufshcd_reset_and_restore() => ufshcd_transfer_req_compl()
after reset is done. Therefore similar handling for hba->outstanding_tasks
could be applied, for example, by
ufshcd_reset_and_restore() => ufshcd_tmc_handler().

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 114c15ed75f7..3cb942ef64e2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5535,11 +5535,25 @@ static void ufshcd_tm_cmd_compl(struct ufs_hba *hba, int tag)
  */
 static void ufshcd_tmc_handler(struct ufs_hba *hba)
 {
-	u32 tm_doorbell;
+	u32 tm_doorbell, tag;
 
 	tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
 	hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
+
+	/*
+	 * resource of timed-out tasks shall be cleaned.
+	 * No effect for normal tasks.
+	 */
+	for_each_set_bit(tag, &hba->tm_condition, hba->nutmrs)
+		ufshcd_tm_cmd_compl(hba, tag);
+
 	wake_up(&hba->tm_wq);
+
+	/*
+	 * If a timed-out task is cleaned done above,
+	 * free tag is available now for waiters.
+	 */
+	wake_up(&hba->tm_tag_wq);
 }
 
 /**
-- 
2.18.0

RE: [PATCH v2 0/3] scsi: ufs: fix broken hba->outstanding_tasks

[Avri Altman]

Stanly,

> 
> Currently bits in hba->outstanding_tasks are cleared only after their
> corresponding task management commands are successfully done by
> __ufshcd_issue_tm_cmd().
> 
> If timeout happens in a task management command, its corresponding
> bit in hba->outstanding_tasks will not be cleared until next task
> management command with the same tag used successfully finishes.
I'm sorry - I still don't understand why you just can't release the tag either way,
Just like we do in device management queries tags,
Instead of adding all this unnecessary code.

I will not object to your series -
just step down and let other people review you patches.

Thanks,
Avri

RE: [PATCH v2 0/3] scsi: ufs: fix broken hba->outstanding_tasks

[Stanley Chu]

Hi Avri,

On Thu, 2019-07-25 at 07:54 +0000, Avri Altman wrote:
> Stanly,
> 
> > 
> > Currently bits in hba->outstanding_tasks are cleared only after their
> > corresponding task management commands are successfully done by
> > __ufshcd_issue_tm_cmd().
> > 
> > If timeout happens in a task management command, its corresponding
> > bit in hba->outstanding_tasks will not be cleared until next task
> > management command with the same tag used successfully finishes.
> I'm sorry - I still don't understand why you just can't release the tag either way,
> Just like we do in device management queries tags,
> Instead of adding all this unnecessary code.
> 
> I will not object to your series -
> just step down and let other people review you patches.


Sorry to not describe the failed scenario clearly.

Simpliy focus on outstanding bits cleanup in failed (timeout) case:
- For device command, if timeout happens, its tag can be cleared in
ufshcd_wait_for_dev_cmd() which specifically uses
ufshcd_outstanding_req_clear() to clear failed bit in outstanding_reqs
mask.
- For task management command, if timeout happens, current driver will
not clear failed bit in outstanding_tasks mask:
	- __ufshcd_issue_tm_cmd() will not clear it,
	- ufshcd_tmc_handler() will not clear it either during reset flow.

> Thanks,
> Avri

Thanks,
Stanley

> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

RE: [PATCH v2 0/3] scsi: ufs: fix broken hba->outstanding_tasks

[Stanley Chu]

Hi Avri,

On Thu, 2019-07-25 at 16:52 +0800, Stanley Chu wrote:
> Hi Avri,
> 
> On Thu, 2019-07-25 at 07:54 +0000, Avri Altman wrote:
> > Stanly,
> > 
> > > 
> > > Currently bits in hba->outstanding_tasks are cleared only after their
> > > corresponding task management commands are successfully done by
> > > __ufshcd_issue_tm_cmd().
> > > 
> > > If timeout happens in a task management command, its corresponding
> > > bit in hba->outstanding_tasks will not be cleared until next task
> > > management command with the same tag used successfully finishes.
> > I'm sorry - I still don't understand why you just can't release the tag either way,
> > Just like we do in device management queries tags,
> > Instead of adding all this unnecessary code.
> > 
> > I will not object to your series -
> > just step down and let other people review you patches.

Sorry for late response due to these busy days.

I just got your point and agreed with you: previous proposal may be too
tricky. Simple always wins. So I will provide a short solution in next
version.

Many thanks!
Stanley