[PATCH v2 0/2] scsi: ufs: two fixups of ufshcd_abort()

[PATCH v2 0/2] scsi: ufs: two fixups of ufshcd_abort()

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

Changelog:

v1 - v2:
    1. add patch [1/2], which is from Stanley Chu <stanley.chu@mediatek.com>
    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

[PATCH v2 1/2] scsi: ufs: Cleanup completed request without interrupt notification

[Bean Huo]

From: Stanley Chu <stanley.chu@mediatek.com>

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 <stanley.chu@mediatek.com>
Reviewed-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 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

Re: [PATCH v2 1/2] scsi: ufs: Cleanup completed request without interrupt notification

[Stanley Chu]

Hi Avri, Bean,

On Tue, 2020-08-11 at 16:18 +0200, Bean Huo wrote:
> From: Stanley Chu <stanley.chu@mediatek.com>
> 
> 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 <stanley.chu@mediatek.com>
> Reviewed-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Bean Huo <beanhuo@micron.com>

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);

Re: [PATCH v2 1/2] scsi: ufs: Cleanup completed request without interrupt notification

[Bean Huo]

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 <stanley.chu@mediatek.com>
> > 
> > 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 <stanley.chu@mediatek.com>
> > Reviewed-by: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> 
> 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
> 

RE: [PATCH v2 1/2] scsi: ufs: Cleanup completed request without interrupt notification

[Avri Altman]

> >
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > Reviewed-by: Can Guo <cang@codeaurora.org>
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
Acked-by: Avri Altman <avri.altman@wdc.com>

[PATCH v2 2/2] scsi: ufs: no need to send one Abort Task TM in case the task in DB was cleared

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

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 <beanhuo@micron.com>
---
 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

Re: [PATCH v2 2/2] scsi: ufs: no need to send one Abort Task TM in case the task in DB was cleared

[Can Guo]

On 2020-08-11 22:18, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> 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 <beanhuo@micron.com>

Reviewed-by: Can Guo <cang@codeaurora.org>

> ---
>  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;

Re: [PATCH v2 2/2] scsi: ufs: no need to send one Abort Task TM in case the task in DB was cleared

[Stanley Chu]

On Tue, 2020-08-11 at 16:18 +0200, Bean Huo wrote:
> From: Bean Huo <beanhuo@micron.com>
> 
> 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 <beanhuo@micron.com>

Reviewed-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [PATCH v2 0/2] scsi: ufs: two fixups of ufshcd_abort()

[Martin K. Petersen]

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 <stanley.chu@mediatek.com>
>     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