From: Bean Huo <[email protected]>
Changelog:
v1 - v2:
1. add patch [1/2], which is from Stanley Chu <[email protected]>
2. change goto command in patch [2/2], let it goto cleanup flow
Bean Huo (1):
scsi: ufs: no need to send one Abort Task TM in case the task in DB
was cleared
Stanley Chu (1):
scsi: ufs: Cleanup completed request without interrupt notification
drivers/scsi/ufs/ufshcd.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
--
2.17.1
From: Stanley Chu <[email protected]>
If somehow no interrupt notification is raised for a completed request
and its doorbell bit is cleared by host, UFS driver needs to cleanup
its outstanding bit in ufshcd_abort(). Otherwise, system may behave
abnormally by below flow:
After ufshcd_abort() returns, this request will be requeued by SCSI
layer with its outstanding bit set. Any future completed request
will trigger ufshcd_transfer_req_compl() to handle all "completed
outstanding bits". In this time, the "abnormal outstanding bit"
will be detected and the "requeued request" will be chosen to execute
request post-processing flow. This is wrong because this request is
still "alive".
Signed-off-by: Stanley Chu <[email protected]>
Reviewed-by: Can Guo <[email protected]>
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 307622284239..66fe814c8725 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6492,7 +6492,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
/* command completed already */
dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n",
__func__, tag);
- goto out;
+ goto cleanup;
} else {
dev_err(hba->dev,
"%s: no response from device. tag = %d, err %d\n",
@@ -6526,6 +6526,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
goto out;
}
+cleanup:
scsi_dma_unmap(cmd);
spin_lock_irqsave(host->host_lock, flags);
--
2.17.1
From: Bean Huo <[email protected]>
If the bit corresponds to a task in the Doorbell register has been
cleared, no need to poll the status of the task on the device side
and to send an Abort Task TM. Instead, let it directly goto cleanup.
Meanwhile, to keep original debug print, move this goto below the debug
print.
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 66fe814c8725..5f09cda7b21c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6434,14 +6434,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
goto out;
}
- if (!(reg & (1 << tag))) {
- dev_err(hba->dev,
- "%s: cmd was completed, but without a notifying intr, tag = %d",
- __func__, tag);
- }
-
/* Print Transfer Request of aborted task */
- dev_err(hba->dev, "%s: Device abort task at tag %d\n", __func__, tag);
+ dev_info(hba->dev, "%s: Device abort task at tag %d\n", __func__, tag);
/*
* Print detailed info about aborted request.
@@ -6462,6 +6456,13 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
}
hba->req_abort_count++;
+ if (!(reg & (1 << tag))) {
+ dev_err(hba->dev,
+ "%s: cmd was completed, but without a notifying intr, tag = %d",
+ __func__, tag);
+ goto cleanup;
+ }
+
/* Skip task abort in case previous aborts failed and report failure */
if (lrbp->req_abort_skip) {
err = -EIO;
--
2.17.1
Hi Avri, Bean,
On Tue, 2020-08-11 at 16:18 +0200, Bean Huo wrote:
> From: Stanley Chu <[email protected]>
>
> If somehow no interrupt notification is raised for a completed request
> and its doorbell bit is cleared by host, UFS driver needs to cleanup
> its outstanding bit in ufshcd_abort(). Otherwise, system may behave
> abnormally by below flow:
>
> After ufshcd_abort() returns, this request will be requeued by SCSI
> layer with its outstanding bit set. Any future completed request
> will trigger ufshcd_transfer_req_compl() to handle all "completed
> outstanding bits". In this time, the "abnormal outstanding bit"
> will be detected and the "requeued request" will be chosen to execute
> request post-processing flow. This is wrong because this request is
> still "alive".
>
> Signed-off-by: Stanley Chu <[email protected]>
> Reviewed-by: Can Guo <[email protected]>
> Signed-off-by: Bean Huo <[email protected]>
Thanks Bean's patch integration.
Like Avri's comment in https://patchwork.kernel.org/patch/11683381/
I think you could add Acked-by tag from Avri.
Hi Avri,
Please correct above description if required.
Thanks for your review! : )
Thanks,
Stanley Chu
> ---
> drivers/scsi/ufs/ufshcd.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..66fe814c8725 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6492,7 +6492,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> /* command completed already */
> dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n",
> __func__, tag);
> - goto out;
> + goto cleanup;
> } else {
> dev_err(hba->dev,
> "%s: no response from device. tag = %d, err %d\n",
> @@ -6526,6 +6526,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> goto out;
> }
>
> +cleanup:
> scsi_dma_unmap(cmd);
>
> spin_lock_irqsave(host->host_lock, flags);
On Wed, 2020-08-12 at 20:47 +0800, Stanley Chu wrote:
> Hi Avri, Bean,
>
> On Tue, 2020-08-11 at 16:18 +0200, Bean Huo wrote:
> > From: Stanley Chu <[email protected]>
> >
> > If somehow no interrupt notification is raised for a completed
> > request
> > and its doorbell bit is cleared by host, UFS driver needs to
> > cleanup
> > its outstanding bit in ufshcd_abort(). Otherwise, system may behave
> > abnormally by below flow:
> >
> > After ufshcd_abort() returns, this request will be requeued by SCSI
> > layer with its outstanding bit set. Any future completed request
> > will trigger ufshcd_transfer_req_compl() to handle all "completed
> > outstanding bits". In this time, the "abnormal outstanding bit"
> > will be detected and the "requeued request" will be chosen to
> > execute
> > request post-processing flow. This is wrong because this request is
> > still "alive".
> >
> > Signed-off-by: Stanley Chu <[email protected]>
> > Reviewed-by: Can Guo <[email protected]>
> > Signed-off-by: Bean Huo <[email protected]>
>
> Thanks Bean's patch integration.
>
> Like Avri's comment in https://patchwork.kernel.org/patch/11683381/
> I think you could add Acked-by tag from Avri.
>
Hi Avri
do I need to re-send the patchset to add your Acked-by tag?
or you will sign your acked-by statement and append this patch?
Thanks,
Bean
>
>
> Hi Avri,
>
> Please correct above description if required.
> Thanks for your review! : )
>
>
> Thanks,
>
> Stanley Chu
>
> >
> > Signed-off-by: Stanley Chu <[email protected]>
> > Reviewed-by: Can Guo <[email protected]>
> > Signed-off-by: Bean Huo <[email protected]>
Acked-by: Avri Altman <[email protected]>
On 2020-08-11 22:18, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> If the bit corresponds to a task in the Doorbell register has been
> cleared, no need to poll the status of the task on the device side
> and to send an Abort Task TM. Instead, let it directly goto cleanup.
>
> Meanwhile, to keep original debug print, move this goto below the debug
> print.
>
> Signed-off-by: Bean Huo <[email protected]>
Reviewed-by: Can Guo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 66fe814c8725..5f09cda7b21c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6434,14 +6434,8 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> goto out;
> }
>
> - if (!(reg & (1 << tag))) {
> - dev_err(hba->dev,
> - "%s: cmd was completed, but without a notifying intr, tag = %d",
> - __func__, tag);
> - }
> -
> /* Print Transfer Request of aborted task */
> - dev_err(hba->dev, "%s: Device abort task at tag %d\n", __func__,
> tag);
> + dev_info(hba->dev, "%s: Device abort task at tag %d\n", __func__,
> tag);
>
> /*
> * Print detailed info about aborted request.
> @@ -6462,6 +6456,13 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> }
> hba->req_abort_count++;
>
> + if (!(reg & (1 << tag))) {
> + dev_err(hba->dev,
> + "%s: cmd was completed, but without a notifying intr, tag = %d",
> + __func__, tag);
> + goto cleanup;
> + }
> +
> /* Skip task abort in case previous aborts failed and report failure
> */
> if (lrbp->req_abort_skip) {
> err = -EIO;
On Tue, 2020-08-11 at 16:18 +0200, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> If the bit corresponds to a task in the Doorbell register has been
> cleared, no need to poll the status of the task on the device side
> and to send an Abort Task TM. Instead, let it directly goto cleanup.
>
> Meanwhile, to keep original debug print, move this goto below the debug
> print.
>
> Signed-off-by: Bean Huo <[email protected]>
Reviewed-by: Stanley Chu <[email protected]>
On Tue, 11 Aug 2020 16:18:57 +0200, Bean Huo wrote:
> Changelog:
>
> v1 - v2:
> 1. add patch [1/2], which is from Stanley Chu <[email protected]>
> 2. change goto command in patch [2/2], let it goto cleanup flow
>
> Bean Huo (1):
> scsi: ufs: no need to send one Abort Task TM in case the task in DB
> was cleared
>
> [...]
Applied to 5.9/scsi-fixes, thanks!
[1/2] scsi: ufs: Clean up completed request without interrupt notification
https://git.kernel.org/mkp/scsi/c/b10178ee7fa8
[2/2] scsi: ufs: No need to send Abort Task if the task in DB was cleared
https://git.kernel.org/mkp/scsi/c/d87a1f6d021f
--
Martin K. Petersen Oracle Linux Engineering