[PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[PATCH v1 0/2] 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 consumpton 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.

Because error handling flow, i.e., ufshcd_reset_and_restore(), will be
triggered after any task management command times out, we fix this by
clearing corresponding hba->outstanding_tasks bits during this flow.
To achieve this, we need a mask to track timed-out commands and thus
error handling flow can clear their tags specifically.

Stanley Chu (2):
  scsi: ufs: Make new function for clearing outstanding task bits
  scsi: ufs: Fix broken hba->outstanding_tasks

 drivers/scsi/ufs/ufshcd.c | 49 +++++++++++++++++++++++++++++++++------
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 43 insertions(+), 7 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/2] scsi: ufs: Make new function for clearing outstanding task bits

[Stanley Chu]

Make a new function "ufshcd_outstanding_task_clear()" used to
clear bits in hba->outstanding_tasks for future wider usage.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8f0426a36b0b..a667dbb547f2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -723,6 +723,17 @@ static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
 	__clear_bit(tag, &hba->outstanding_reqs);
 }
 
+/**
+ * ufshcd_outstanding_task_clear - Clear a bit in outstanding task field
+ * @hba: per adapter instance
+ * @tag: position of the bit to be cleared
+ */
+static inline void ufshcd_outstanding_task_clear(struct ufs_hba *hba, int tag)
+{
+	__clear_bit(tag, &hba->outstanding_tasks);
+	dev_info(hba->dev, "clear outstanding_tasks: %d\n", tag);
+}
+
 /**
  * ufshcd_get_lists_status - Check UCRDY, UTRLRDY and UTMRLRDY
  * @reg: Register value of host controller status
@@ -5679,7 +5690,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 		ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete");
 
 		spin_lock_irqsave(hba->host->host_lock, flags);
-		__clear_bit(free_slot, &hba->outstanding_tasks);
+		ufshcd_outstanding_task_clear(hba, free_slot);
 		spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	}
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/2] 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 consumpton 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.

Because error handling flow, i.e., ufshcd_reset_and_restore(), will be
triggered after any task management command times out, we fix this by
clearing corresponding hba->outstanding_tasks bits during this flow.
To achieve this, we need a mask to track timed-out commands and thus
error handling flow can clear their tags specifically.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 38 +++++++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a667dbb547f2..f780066edf26 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -731,7 +731,6 @@ static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
 static inline void ufshcd_outstanding_task_clear(struct ufs_hba *hba, int tag)
 {
 	__clear_bit(tag, &hba->outstanding_tasks);
-	dev_info(hba->dev, "clear outstanding_tasks: %d\n", tag);
 }
 
 /**
@@ -5540,11 +5539,34 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
  */
 static void ufshcd_tmc_handler(struct ufs_hba *hba)
 {
-	u32 tm_doorbell;
+	u32 tm_doorbell, tag;
+	unsigned long tm_err_handled = 0;
+	unsigned long tm_done;
 
 	tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
 	hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
-	wake_up(&hba->tm_wq);
+	tm_done = hba->tm_condition;
+
+	/* clean resources for timed-out tasks */
+	for_each_set_bit(tag, &hba->tm_condition, hba->nutmrs) {
+		if (test_and_clear_bit(tag, &hba->tm_slots_err)) {
+			clear_bit(tag, &hba->tm_condition);
+			ufshcd_put_tm_slot(hba, tag);
+			ufshcd_outstanding_task_clear(hba, tag);
+			__set_bit(tag, &tm_err_handled);
+		}
+	}
+
+	/*
+	 * Now tag waiters can get free tags if tags were occupied
+	 * by timed-out tasks
+	 */
+	if (tm_err_handled)
+		wake_up(&hba->tm_tag_wq);
+
+	/* if we have normal tasks, they shall have post-processing */
+	if (tm_err_handled != tm_done)
+		wake_up(&hba->tm_wq);
 }
 
 /**
@@ -5682,6 +5704,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 		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);
+		set_bit(free_slot, &hba->tm_slots_err);
 		err = -ETIMEDOUT;
 	} else {
 		err = 0;
@@ -5692,12 +5715,13 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
 		spin_lock_irqsave(hba->host->host_lock, flags);
 		ufshcd_outstanding_task_clear(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);
+	if (!(test_bit(free_slot, &hba->tm_slots_err))) {
+		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;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index a43c7135f33d..4e4dfa6e233c 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -645,6 +645,7 @@ struct ufs_hba {
 	wait_queue_head_t tm_tag_wq;
 	unsigned long tm_condition;
 	unsigned long tm_slots_in_use;
+	unsigned long tm_slots_err;
 
 	struct uic_command *active_uic_cmd;
 	struct mutex uic_cmd_mutex;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Stanley Chu]

Hi Avri, Alim and Pedrom,

Gentle ping for this fix.

On Fri, 2019-07-12 at 12:44 +0800, Stanley Chu wrote:
> 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 consumpton 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.
> 
> Because error handling flow, i.e., ufshcd_reset_and_restore(), will be
> triggered after any task management command times out, we fix this by
> clearing corresponding hba->outstanding_tasks bits during this flow.
> To achieve this, we need a mask to track timed-out commands and thus
> error handling flow can clear their tags specifically.
> 
> Stanley Chu (2):
>   scsi: ufs: Make new function for clearing outstanding task bits
>   scsi: ufs: Fix broken hba->outstanding_tasks
> 
>  drivers/scsi/ufs/ufshcd.c | 49 +++++++++++++++++++++++++++++++++------
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 43 insertions(+), 7 deletions(-)
> 

Thanks,
Stanley


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Avri Altman]

Hi,

> 
> 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.‧
ufshcd_clear_tm_cmd is also called as part of ufshcd_err_handler.
Does this change something in your assumptions?

Thanks,
Avri

> 
> This is wrong and can lead to some issues, like power consumpton 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.
> 
> Because error handling flow, i.e., ufshcd_reset_and_restore(), will be
> triggered after any task management command times out, we fix this by
> clearing corresponding hba->outstanding_tasks bits during this flow.
> To achieve this, we need a mask to track timed-out commands and thus
> error handling flow can clear their tags specifically.
> 
> Stanley Chu (2):
>   scsi: ufs: Make new function for clearing outstanding task bits
>   scsi: ufs: Fix broken hba->outstanding_tasks
> 
>  drivers/scsi/ufs/ufshcd.c | 49 +++++++++++++++++++++++++++++++++------
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 43 insertions(+), 7 deletions(-)
> 
> --
> 2.18.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Avri Altman]

> 
> Hi,
> 
> >
> > 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.‧
> ufshcd_clear_tm_cmd is also called as part of ufshcd_err_handler.
> Does this change something in your assumptions?
And BTW there is a specific __clear_bit in __ufshcd_issue_tm_cmd() in case of a TO.

> 
> Thanks,
> Avri
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Avri Altman]

> 
> >
> > Hi,
> >
> > >
> > > 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.‧
> > ufshcd_clear_tm_cmd is also called as part of ufshcd_err_handler.
> > Does this change something in your assumptions?
> And BTW there is a specific __clear_bit in __ufshcd_issue_tm_cmd() in case
> of a TO.

Gave it another look - 
If indeed this bit isn't cleared as part of the error flow that the timeout triggers,
I think you should relate to ufshcd_clear_tm_cmd specifically in your commit log - 
Because this is the obvious place where the bit cleanup should take place.

Also the fix should be much more intuitive IMO - 
Today we do __clear_bit() on success, ufshcd_clear_tm_cmd() on error,
And also ufshcd_put_tm_slot() either way?

Maybe you can choose a single place to clear it, without any additional code?

Thanks,
Avri
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Stanley Chu]

Hi Avri,

On Mon, 2019-07-22 at 06:10 +0000, Avri Altman wrote:
> > 
> > >
> > > Hi,
> > >
> > > >
> > > > 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.‧
> > > ufshcd_clear_tm_cmd is also called as part of ufshcd_err_handler.
> > > Does this change something in your assumptions?
> > And BTW there is a specific __clear_bit in __ufshcd_issue_tm_cmd() in case
> > of a TO.
> 
> Gave it another look - 
> If indeed this bit isn't cleared as part of the error flow that the timeout triggers,
> I think you should relate to ufshcd_clear_tm_cmd specifically in your commit log - 
> Because this is the obvious place where the bit cleanup should take place.
> 
> Also the fix should be much more intuitive IMO - 
> Today we do __clear_bit() on success, ufshcd_clear_tm_cmd() on error,
> And also ufshcd_put_tm_slot() either way?
> 
> Maybe you can choose a single place to clear it, without any additional code?

ufshcd_clear_tm_cmd() is similar to ufshcd_clear_cmd() which tries to
write timed-out bit in "clear register". They do not clean bits in both
outstanding masks.

Another reason to not to do outstanding bits cleanup in
ufshcd_clear_tm_cmd() is: 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. Cleanup includes bits in hba->outstanding_tasks and
hba->tm_slots_in_use which is possibly cleaned too early by
ufshcd_put_tm_slot() if command is timed-out now.

Referring to error handling flow of hba->outstanding_reqs, all timed-out
bits will be cleaned by ufshcd_reset_and_restore() =>
ufshcd_transfer_req_compl() after reset is done. Similar handling for
hba->outstanding_tasks could be applied, i.e., do cleanup by
ufshcd_reset_and_restore() => ufshcd_tmc_handler().

The next thing is what you suggested: How to make the fix more
intuitive. Patchset v2 will be uploaded for review: It tries to
"re-factor" cleanup jobs first, and then add fixed flow to make the
whole patch more readable.

One more thing, above description and flow is done through UFSHCD SCSI
error handling routines registered with SCSI Midlayer. For TM command
timeout happening in bsg path without error handling triggered by SCSI
layer, we may need to consider to handle those tasks in future patches.

> 
> Thanks,
> Avri

Thanks,
Stanley

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1 0/2] scsi: ufs: Fix broken hba->outstanding_tasks

[Avri Altman]

Stanley,

> 
> Hi Avri,
> 
> On Mon, 2019-07-22 at 06:10 +0000, Avri Altman wrote:
> > >
> > > >
> > > > Hi,
> > > >
> > > > >
> > > > > 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.‧
> > > > ufshcd_clear_tm_cmd is also called as part of ufshcd_err_handler.
> > > > Does this change something in your assumptions?
> > > And BTW there is a specific __clear_bit in __ufshcd_issue_tm_cmd() in
> case
> > > of a TO.
> >
> > Gave it another look -
> > If indeed this bit isn't cleared as part of the error flow that the timeout
> triggers,
> > I think you should relate to ufshcd_clear_tm_cmd specifically in your
> commit log -
> > Because this is the obvious place where the bit cleanup should take
> place.
> >
> > Also the fix should be much more intuitive IMO -
> > Today we do __clear_bit() on success, ufshcd_clear_tm_cmd() on error,
> > And also ufshcd_put_tm_slot() either way?
> >
> > Maybe you can choose a single place to clear it, without any additional
> code?
> 
> ufshcd_clear_tm_cmd() is similar to ufshcd_clear_cmd() which tries to
> write timed-out bit in "clear register". They do not clean bits in both
> outstanding masks.
> 
> Another reason to not to do outstanding bits cleanup in
> ufshcd_clear_tm_cmd() is: 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. Cleanup includes bits in hba->outstanding_tasks and
> hba->tm_slots_in_use which is possibly cleaned too early by
> ufshcd_put_tm_slot() if command is timed-out now.
> 
> Referring to error handling flow of hba->outstanding_reqs, all timed-out
> bits will be cleaned by ufshcd_reset_and_restore() =>
> ufshcd_transfer_req_compl() after reset is done. Similar handling for
> hba->outstanding_tasks could be applied, i.e., do cleanup by
> ufshcd_reset_and_restore() => ufshcd_tmc_handler().
Fair enough.  Thank you for the detailed explanation.

> 
> The next thing is what you suggested: How to make the fix more
> intuitive. Patchset v2 will be uploaded for review: It tries to
> "re-factor" cleanup jobs first, and then add fixed flow to make the
> whole patch more readable.
> 
> One more thing, above description and flow is done through UFSHCD SCSI
> error handling routines registered with SCSI Midlayer. For TM command
> timeout happening in bsg path without error handling triggered by SCSI
> layer, we may need to consider to handle those tasks in future patches.
Please do.

Thanks,
Avri

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel