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.
This patch also deletes dispensable dev_err() in case of the task
already completed.
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 307622284239..581b4ab52bf4 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6425,19 +6425,16 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
return ufshcd_eh_host_reset_handler(cmd);
ufshcd_hold(hba, false);
- reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
/* If command is already aborted/completed, return SUCCESS */
- if (!(test_bit(tag, &hba->outstanding_reqs))) {
- dev_err(hba->dev,
- "%s: cmd at tag %d already completed, outstanding=0x%lx, doorbell=0x%x\n",
- __func__, tag, hba->outstanding_reqs, reg);
+ if (!(test_bit(tag, &hba->outstanding_reqs)))
goto out;
- }
+ reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
if (!(reg & (1 << tag))) {
dev_err(hba->dev,
"%s: cmd was completed, but without a notifying intr, tag = %d",
__func__, tag);
+ goto out;
}
/* Print Transfer Request of aborted task */
--
2.17.1
Hi Bean,
On 2020-08-04 20:35, 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.
> This patch also deletes dispensable dev_err() in case of the task
> already completed.
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..581b4ab52bf4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6425,19 +6425,16 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> return ufshcd_eh_host_reset_handler(cmd);
>
> ufshcd_hold(hba, false);
> - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> /* If command is already aborted/completed, return SUCCESS */
> - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> - dev_err(hba->dev,
> - "%s: cmd at tag %d already completed, outstanding=0x%lx,
> doorbell=0x%x\n",
> - __func__, tag, hba->outstanding_reqs, reg);
> + if (!(test_bit(tag, &hba->outstanding_reqs)))
> goto out;
> - }
>
> + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> if (!(reg & (1 << tag))) {
> dev_err(hba->dev,
> "%s: cmd was completed, but without a notifying intr, tag = %d",
> __func__, tag);
> + goto out;
Please check Stanley's recent change to ufshcd_abort, you may
want to rebase your change on his and do goto cleanup here.
@Stanley correct me if I am wrong.
But even if you do a goto cleanup here, we still lost the
chances to dump host infos/regs like it does in the old code.
If a cmd was completed but without a notifying intr, this is
kind of a problem that we/host should look into, because it's
pasted at least 30 sec since the cmd was sent, so those dumps
are necessary to debug the problem. How about moving blow prints
in front of this part?
Thanks,
Can Guo.
> }
>
> /* Print Transfer Request of aborted task */
>
> Please check Stanley's recent change to ufshcd_abort, you may
> want to rebase your change on his and do goto cleanup here.
> @Stanley correct me if I am wrong.
>
> But even if you do a goto cleanup here, we still lost the
> chances to dump host infos/regs like it does in the old code.
> If a cmd was completed but without a notifying intr, this is
> kind of a problem that we/host should look into, because it's
> pasted at least 30 sec since the cmd was sent, so those dumps
> are necessary to debug the problem. How about moving blow prints
> in front of this part?
>
> Thanks,
>
> Can Guo.
>
> > }
> >
> > /* Print Transfer Request of aborted task */
Hi Can
Thanks, do you mean that change to like this:
Author: Bean Huo <[email protected]>
Date: Thu Aug 6 11:34:45 2020 +0200
scsi: ufs: no need to send one Abort Task TM in case the task in
was cleared
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.
This patch also deletes dispensable dev_err() in case of the task
already completed.
Signed-off-by: Bean Huo <[email protected]>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 307622284239..f7c91ce9e294 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6425,23 +6425,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
return ufshcd_eh_host_reset_handler(cmd);
ufshcd_hold(hba, false);
- reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
/* If command is already aborted/completed, return SUCCESS */
- if (!(test_bit(tag, &hba->outstanding_reqs))) {
- dev_err(hba->dev,
- "%s: cmd at tag %d already completed,
outstanding=0x%lx, doorbell=0x%x\n",
- __func__, tag, hba->outstanding_reqs, reg);
+ if (!(test_bit(tag, &hba->outstanding_reqs)))
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);
/*
* Print detailed info about aborted request.
@@ -6462,6 +6448,17 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
}
hba->req_abort_count++;
+ reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
+ if (!(reg & (1 << tag))) {
+ dev_err(hba->dev,
+ "%s: cmd was completed, but without a notifying intr,
tag = %d",
+ __func__, tag);
+ goto cleanup;
+ }
+
+ /* Print Transfer Request of aborted task */
+ dev_err(hba->dev, "%s: Device abort task at tag %d\n",
__func__, tag);
+
/* Skip task abort in case previous aborts failed and report
failure */
if (lrbp->req_abort_skip) {
err = -EIO;
@@ -6526,6 +6523,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
goto out;
}
+cleanup:
scsi_dma_unmap(cmd);
spin_lock_irqsave(host->host_lock, flags);
Author: Stanley Chu <[email protected]>
Date: Thu Aug 6 11:48:00 2020 +0200
scsi: ufs: Cleanup completed request without interrupt notification
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]>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f7c91ce9e294..29d5e5e5d0e0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6489,7 +6489,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",
Hi Bean,
On 2020-08-06 17:50, Bean Huo wrote:
>>
>> Please check Stanley's recent change to ufshcd_abort, you may
>> want to rebase your change on his and do goto cleanup here.
>> @Stanley correct me if I am wrong.
>>
>> But even if you do a goto cleanup here, we still lost the
>> chances to dump host infos/regs like it does in the old code.
>> If a cmd was completed but without a notifying intr, this is
>> kind of a problem that we/host should look into, because it's
>> pasted at least 30 sec since the cmd was sent, so those dumps
>> are necessary to debug the problem. How about moving blow prints
>> in front of this part?
>>
>> Thanks,
>>
>> Can Guo.
>>
>> > }
>> >
>> > /* Print Transfer Request of aborted task */
>
> Hi Can
>
> Thanks, do you mean that change to like this:
>
>
> Author: Bean Huo <[email protected]>
> Date: Thu Aug 6 11:34:45 2020 +0200
>
> scsi: ufs: no need to send one Abort Task TM in case the task in
> was cleared
>
> 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.
> This patch also deletes dispensable dev_err() in case of the task
> already completed.
>
> Signed-off-by: Bean Huo <[email protected]>
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 307622284239..f7c91ce9e294 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6425,23 +6425,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> return ufshcd_eh_host_reset_handler(cmd);
>
> ufshcd_hold(hba, false);
> - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> /* If command is already aborted/completed, return SUCCESS */
> - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> - dev_err(hba->dev,
> - "%s: cmd at tag %d already completed,
> outstanding=0x%lx, doorbell=0x%x\n",
> - __func__, tag, hba->outstanding_reqs, reg);
> + if (!(test_bit(tag, &hba->outstanding_reqs)))
> 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);
>
> /*
> * Print detailed info about aborted request.
> @@ -6462,6 +6448,17 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> }
> hba->req_abort_count++;
>
> + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> + if (!(reg & (1 << tag))) {
> + dev_err(hba->dev,
> + "%s: cmd was completed, but without a notifying intr,
> tag = %d",
> + __func__, tag);
> + goto cleanup;
> + }
> +
> + /* Print Transfer Request of aborted task */
> + dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> __func__, tag);
> +
The rest looks good but let below two lines stay where they were.
/* Print Transfer Request of aborted task */
dev_err(hba->dev, "%s: Device abort task at tag %d\n",
__func__, tag);
Thanks,
Can Guo.
> /* Skip task abort in case previous aborts failed and report
> failure */
> if (lrbp->req_abort_skip) {
> err = -EIO;
> @@ -6526,6 +6523,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> goto out;
> }
>
> +cleanup:
> scsi_dma_unmap(cmd);
>
> spin_lock_irqsave(host->host_lock, flags);
>
>
>
>
>
> Author: Stanley Chu <[email protected]>
> Date: Thu Aug 6 11:48:00 2020 +0200
>
> scsi: ufs: Cleanup completed request without interrupt notification
>
> 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]>
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f7c91ce9e294..29d5e5e5d0e0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6489,7 +6489,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",
On Thu, 2020-08-06 at 18:07 +0800, Can Guo wrote:
> Hi Bean,
>
> On 2020-08-06 17:50, Bean Huo wrote:
> > >
> > > Please check Stanley's recent change to ufshcd_abort, you may
> > > want to rebase your change on his and do goto cleanup here.
> > > @Stanley correct me if I am wrong.
> > >
> > > But even if you do a goto cleanup here, we still lost the
> > > chances to dump host infos/regs like it does in the old code.
> > > If a cmd was completed but without a notifying intr, this is
> > > kind of a problem that we/host should look into, because it's
> > > pasted at least 30 sec since the cmd was sent, so those dumps
> > > are necessary to debug the problem. How about moving blow prints
> > > in front of this part?
> > >
> > > Thanks,
> > >
> > > Can Guo.
> > >
> > > > }
> > > >
> > > > /* Print Transfer Request of aborted task */
> >
> > Hi Can
> >
> > Thanks, do you mean that change to like this:
> >
> >
> > Author: Bean Huo <[email protected]>
> > Date: Thu Aug 6 11:34:45 2020 +0200
> >
> > scsi: ufs: no need to send one Abort Task TM in case the task
> > in
> > was cleared
> >
> > 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.
> > This patch also deletes dispensable dev_err() in case of the
> > task
> > already completed.
> >
> > Signed-off-by: Bean Huo <[email protected]>
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 307622284239..f7c91ce9e294 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6425,23 +6425,9 @@ static int ufshcd_abort(struct scsi_cmnd
> > *cmd)
> > return ufshcd_eh_host_reset_handler(cmd);
> >
> > ufshcd_hold(hba, false);
> > - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > /* If command is already aborted/completed, return SUCCESS
> > */
> > - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> > - dev_err(hba->dev,
> > - "%s: cmd at tag %d already completed,
> > outstanding=0x%lx, doorbell=0x%x\n",
> > - __func__, tag, hba->outstanding_reqs, reg);
> > + if (!(test_bit(tag, &hba->outstanding_reqs)))
> > 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);
> >
> > /*
> > * Print detailed info about aborted request.
> > @@ -6462,6 +6448,17 @@ static int ufshcd_abort(struct scsi_cmnd
> > *cmd)
> > }
> > hba->req_abort_count++;
> >
> > + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > + if (!(reg & (1 << tag))) {
> > + dev_err(hba->dev,
> > + "%s: cmd was completed, but without a notifying
> > intr,
> > tag = %d",
> > + __func__, tag);
> > + goto cleanup;
> > + }
> > +
> > + /* Print Transfer Request of aborted task */
> > + dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > __func__, tag);
> > +
>
> The rest looks good but let below two lines stay where they were.
>
> /* Print Transfer Request of aborted task */
> dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> __func__, tag);
>
>
> Thanks,
>
> Can Guo.
>
Hi Can
I will change it in the next version.
Hi Stanly
would you mind I take your patch into my next version patchset? Since
we both will add a new same goto label. I will keep your patch
authorship.
Thanks,
Bean
Hi Bean,
On Mon, 2020-08-10 at 17:41 +0200, Bean Huo wrote:
> On Thu, 2020-08-06 at 18:07 +0800, Can Guo wrote:
> > Hi Bean,
> >
> > On 2020-08-06 17:50, Bean Huo wrote:
> > > >
> > > > Please check Stanley's recent change to ufshcd_abort, you may
> > > > want to rebase your change on his and do goto cleanup here.
> > > > @Stanley correct me if I am wrong.
> > > >
> > > > But even if you do a goto cleanup here, we still lost the
> > > > chances to dump host infos/regs like it does in the old code.
> > > > If a cmd was completed but without a notifying intr, this is
> > > > kind of a problem that we/host should look into, because it's
> > > > pasted at least 30 sec since the cmd was sent, so those dumps
> > > > are necessary to debug the problem. How about moving blow prints
> > > > in front of this part?
> > > >
> > > > Thanks,
> > > >
> > > > Can Guo.
> > > >
> > > > > }
> > > > >
> > > > > /* Print Transfer Request of aborted task */
> > >
> > > Hi Can
> > >
> > > Thanks, do you mean that change to like this:
> > >
> > >
> > > Author: Bean Huo <[email protected]>
> > > Date: Thu Aug 6 11:34:45 2020 +0200
> > >
> > > scsi: ufs: no need to send one Abort Task TM in case the task
> > > in
> > > was cleared
> > >
> > > 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.
> > > This patch also deletes dispensable dev_err() in case of the
> > > task
> > > already completed.
> > >
> > > Signed-off-by: Bean Huo <[email protected]>
> > >
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 307622284239..f7c91ce9e294 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -6425,23 +6425,9 @@ static int ufshcd_abort(struct scsi_cmnd
> > > *cmd)
> > > return ufshcd_eh_host_reset_handler(cmd);
> > >
> > > ufshcd_hold(hba, false);
> > > - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > > /* If command is already aborted/completed, return SUCCESS
> > > */
> > > - if (!(test_bit(tag, &hba->outstanding_reqs))) {
> > > - dev_err(hba->dev,
> > > - "%s: cmd at tag %d already completed,
> > > outstanding=0x%lx, doorbell=0x%x\n",
> > > - __func__, tag, hba->outstanding_reqs, reg);
> > > + if (!(test_bit(tag, &hba->outstanding_reqs)))
> > > 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);
> > >
> > > /*
> > > * Print detailed info about aborted request.
> > > @@ -6462,6 +6448,17 @@ static int ufshcd_abort(struct scsi_cmnd
> > > *cmd)
> > > }
> > > hba->req_abort_count++;
> > >
> > > + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> > > + if (!(reg & (1 << tag))) {
> > > + dev_err(hba->dev,
> > > + "%s: cmd was completed, but without a notifying
> > > intr,
> > > tag = %d",
> > > + __func__, tag);
> > > + goto cleanup;
> > > + }
> > > +
> > > + /* Print Transfer Request of aborted task */
> > > + dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > > __func__, tag);
> > > +
> >
> > The rest looks good but let below two lines stay where they were.
> >
> > /* Print Transfer Request of aborted task */
> > dev_err(hba->dev, "%s: Device abort task at tag %d\n",
> > __func__, tag);
> >
> >
> > Thanks,
> >
> > Can Guo.
> >
> Hi Can
> I will change it in the next version.
>
>
> Hi Stanly
> would you mind I take your patch into my next version patchset? Since
> we both will add a new same goto label. I will keep your patch
> authorship.
Sure, OK to me : )
Thanks,
Stanley Chu