[PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Stanley Chu]

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>
---
 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 577cc0d7487f..9d180da77488 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6493,7 +6493,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",
@@ -6527,6 +6527,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		goto out;
 	}
 
+cleanup:
 	scsi_dma_unmap(cmd);
 
 	spin_lock_irqsave(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

RE: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Avri Altman]

Looks good to me.
But better wait and see if Bart have any further reservations.

Thanks,
Avri 
> 
> 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>
> ---
>  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 577cc0d7487f..9d180da77488 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6493,7 +6493,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",
> @@ -6527,6 +6527,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>                 goto out;
>         }
> 
> +cleanup:
>         scsi_dma_unmap(cmd);
> 
>         spin_lock_irqsave(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

RE: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Stanley Chu]

Hi Bart,

On Mon, 2020-07-27 at 11:18 +0000, Avri Altman wrote:
> Looks good to me.
> But better wait and see if Bart have any further reservations.
> 

Would you have any further suggestions?

Thanks a lot,
Stanley Chu

> Thanks,
> Avri 
> > 
> > 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>
> > ---
> >  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 577cc0d7487f..9d180da77488 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -6493,7 +6493,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",
> > @@ -6527,6 +6527,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
> >                 goto out;
> >         }
> > 
> > +cleanup:
> >         scsi_dma_unmap(cmd);
> > 
> >         spin_lock_irqsave(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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Bart Van Assche]

On 2020-07-30 18:30, Stanley Chu wrote:
> On Mon, 2020-07-27 at 11:18 +0000, Avri Altman wrote:
>> Looks good to me.
>> But better wait and see if Bart have any further reservations.
> 
> Would you have any further suggestions?

Today is the first time that I took a look at ufshcd_abort(). The
approach of that function looks wrong to me. This is how I think that a
SCSI LLD abort handler should work:
(1) Serialize against the completion path
(__ufshcd_transfer_req_compl()) such that it cannot happen that the
abort handler and the regular completion path both call
cmd->scsi_done(cmd) at the same time. I'm not sure whether an existing
synchronization object can be used for this purpose or whether a new
synchronization object has to be introduced to serialize scsi_done()
calls from __ufshcd_transfer_req_compl() and ufshcd_abort().
(2) While holding that synchronization object, check whether the SCSI
command is still outstanding. If so, submit a SCSI abort TMR to the device.
(3) If the command has been aborted, call scsi_done() and return
SUCCESS. If aborting failed and the command is still in progress, return
FAILED.

An example is available in srp_abort() in
drivers/infiniband/ulp/srp/ib_srp.c.

Bart.

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Can Guo]

Hi Bart,

On 2020-07-31 12:06, Bart Van Assche wrote:
> On 2020-07-30 18:30, Stanley Chu wrote:
>> On Mon, 2020-07-27 at 11:18 +0000, Avri Altman wrote:
>>> Looks good to me.
>>> But better wait and see if Bart have any further reservations.
>> 
>> Would you have any further suggestions?
> 
> Today is the first time that I took a look at ufshcd_abort(). The
> approach of that function looks wrong to me. This is how I think that a
> SCSI LLD abort handler should work:
> (1) Serialize against the completion path
> (__ufshcd_transfer_req_compl()) such that it cannot happen that the
> abort handler and the regular completion path both call
> cmd->scsi_done(cmd) at the same time. I'm not sure whether an existing
> synchronization object can be used for this purpose or whether a new
> synchronization object has to be introduced to serialize scsi_done()
> calls from __ufshcd_transfer_req_compl() and ufshcd_abort().
> (2) While holding that synchronization object, check whether the SCSI
> command is still outstanding. If so, submit a SCSI abort TMR to the 
> device.
> (3) If the command has been aborted, call scsi_done() and return
> SUCCESS. If aborting failed and the command is still in progress, 
> return
> FAILED.
> 
> An example is available in srp_abort() in
> drivers/infiniband/ulp/srp/ib_srp.c.
> 
> Bart.


AFAIK, sychronization of scsi_done is not a problem here, because scsi 
layer
use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to 
prevent
the concurrency of abort and real completion of it.

Check func scsi_times_out(), hope it helps.

enum blk_eh_timer_return scsi_times_out(struct request *req)
{
...
         if (rtn == BLK_EH_DONE) {
                 /*
                  * Set the command to complete first in order to prevent 
a real
                  * completion from releasing the command while error 
handling
                  * is using it. If the command was already completed, 
then the
                  * lower level driver beat the timeout handler, and it 
is safe
                  * to return without escalating error recovery.
                  *
                  * If timeout handling lost the race to a real 
completion, the
                  * block layer may ignore that due to a fake timeout 
injection,
                  * so return RESET_TIMER to allow error handling another 
shot
                  * at this command.
                  */
                 if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
                         return BLK_EH_RESET_TIMER;
                 if (scsi_abort_command(scmd) != SUCCESS) {
                         set_host_byte(scmd, DID_TIME_OUT);
                         scsi_eh_scmd_add(scmd);
                 }
         }
}

Thanks,

Can Guo.

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Bart Van Assche]

On 2020-07-31 01:00, Can Guo wrote:
> AFAIK, sychronization of scsi_done is not a problem here, because scsi
> layer
> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to prevent
> the concurrency of abort and real completion of it.
> 
> Check func scsi_times_out(), hope it helps.
> 
> enum blk_eh_timer_return scsi_times_out(struct request *req)
> {
> ...
>         if (rtn == BLK_EH_DONE) {
>                 /*
>                  * Set the command to complete first in order to prevent
> a real
>                  * completion from releasing the command while error
> handling
>                  * is using it. If the command was already completed,
> then the
>                  * lower level driver beat the timeout handler, and it
> is safe
>                  * to return without escalating error recovery.
>                  *
>                  * If timeout handling lost the race to a real
> completion, the
>                  * block layer may ignore that due to a fake timeout
> injection,
>                  * so return RESET_TIMER to allow error handling another
> shot
>                  * at this command.
>                  */
>                 if (test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state))
>                         return BLK_EH_RESET_TIMER;
>                 if (scsi_abort_command(scmd) != SUCCESS) {
>                         set_host_byte(scmd, DID_TIME_OUT);
>                         scsi_eh_scmd_add(scmd);
>                 }
>         }
> }

I am familiar with this mechanism. My concern is that both the regular
completion path and the abort handler must call scsi_dma_unmap() before
calling cmd->scsi_done(cmd). I don't see how
test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
the regular completion path and the abort handler call scsi_dma_unmap()
concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
is set?

Thanks,

Bart.

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Can Guo]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]

Hi Bart,

On 2020-08-01 00:51, Bart Van Assche wrote:
> On 2020-07-31 01:00, Can Guo wrote:
>> AFAIK, sychronization of scsi_done is not a problem here, because scsi
>> layer
>> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to 
>> prevent
>> the concurrency of abort and real completion of it.
>> 
>> Check func scsi_times_out(), hope it helps.
>> 
>> enum blk_eh_timer_return scsi_times_out(struct request *req)
>> {
>> ...
>>         if (rtn == BLK_EH_DONE) {
>>                 /*
>>                  * Set the command to complete first in order to 
>> prevent
>> a real
>>                  * completion from releasing the command while error
>> handling
>>                  * is using it. If the command was already completed,
>> then the
>>                  * lower level driver beat the timeout handler, and it
>> is safe
>>                  * to return without escalating error recovery.
>>                  *
>>                  * If timeout handling lost the race to a real
>> completion, the
>>                  * block layer may ignore that due to a fake timeout
>> injection,
>>                  * so return RESET_TIMER to allow error handling 
>> another
>> shot
>>                  * at this command.
>>                  */
>>                 if (test_and_set_bit(SCMD_STATE_COMPLETE, 
>> &scmd->state))
>>                         return BLK_EH_RESET_TIMER;
>>                 if (scsi_abort_command(scmd) != SUCCESS) {
>>                         set_host_byte(scmd, DID_TIME_OUT);
>>                         scsi_eh_scmd_add(scmd);
>>                 }
>>         }
>> }
> 
> I am familiar with this mechanism. My concern is that both the regular
> completion path and the abort handler must call scsi_dma_unmap() before
> calling cmd->scsi_done(cmd). I don't see how
> test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
> the regular completion path and the abort handler call scsi_dma_unmap()
> concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
> is set?
> 
> Thanks,
> 
> Bart.

For scsi_dma_unmap() part, that is true - we should make it serialized 
with
any other completion paths. I've found it during my fault injection 
test, so
I've made a patch to fix it, but it only comes in my next error recovery
enhancement patch series. Please check the attachment.

Thanks,

Can Guo.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0005-scsi-ufs-Properly-release-resources-if-a-task-is-abo.patch --]
[-- Type: text/x-diff; name=0005-scsi-ufs-Properly-release-resources-if-a-task-is-abo.patch, Size: 1473 bytes --]

From ef87832b5f6ff6af29ac9bac7fdea1e245c8162b Mon Sep 17 00:00:00 2001
From: Can Guo <cang@codeaurora.org>
Date: Sun, 7 Jun 2020 12:16:01 +0800
Subject: [PATCH 5/6] scsi: ufs: Properly release resources if a task is
 aborted successfully

In current UFS task abort hook, namely ufshcd_abort(), if a task is
aborted successfully, clock scaling busy time statistics is not updated
and, most important, clk_gating.active_reqs is not decreased, which makes
clk_gating.active_reqs stay above zero forever, meaning clock gating would
never happen. To fix it, instead of releasing resources "mannually", use
the existing func __ufshcd_transfer_req_compl().

Change-Id: Ia8cc496f53bb428eac7cfa784e431a2b37a45375
Signed-off-by: Can Guo <cang@codeaurora.org>

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3c46f74..87b911f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6876,16 +6876,10 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 		goto out;
 	}
 
-	scsi_dma_unmap(cmd);
-
 	spin_lock_irqsave(host->host_lock, flags);
-	ufshcd_outstanding_req_clear(hba, tag);
-	hba->lrb[tag].cmd = NULL;
+	__ufshcd_transfer_req_compl(hba, (1UL << tag));
 	spin_unlock_irqrestore(host->host_lock, flags);
 
-	clear_bit_unlock(tag, &hba->lrb_in_use);
-	wake_up(&hba->dev_cmd.tag_wq);
-
 out:
 	if (!err) {
 		err = SUCCESS;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Stanley Chu]

Hi Can,

On Sat, 2020-08-01 at 07:17 +0800, Can Guo wrote:
> Hi Bart,
> 
> On 2020-08-01 00:51, Bart Van Assche wrote:
> > On 2020-07-31 01:00, Can Guo wrote:
> >> AFAIK, sychronization of scsi_done is not a problem here, because scsi
> >> layer
> >> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to 
> >> prevent
> >> the concurrency of abort and real completion of it.
> >> 
> >> Check func scsi_times_out(), hope it helps.
> >> 
> >> enum blk_eh_timer_return scsi_times_out(struct request *req)
> >> {
> >> ...
> >>         if (rtn == BLK_EH_DONE) {
> >>                 /*
> >>                  * Set the command to complete first in order to 
> >> prevent
> >> a real
> >>                  * completion from releasing the command while error
> >> handling
> >>                  * is using it. If the command was already completed,
> >> then the
> >>                  * lower level driver beat the timeout handler, and it
> >> is safe
> >>                  * to return without escalating error recovery.
> >>                  *
> >>                  * If timeout handling lost the race to a real
> >> completion, the
> >>                  * block layer may ignore that due to a fake timeout
> >> injection,
> >>                  * so return RESET_TIMER to allow error handling 
> >> another
> >> shot
> >>                  * at this command.
> >>                  */
> >>                 if (test_and_set_bit(SCMD_STATE_COMPLETE, 
> >> &scmd->state))
> >>                         return BLK_EH_RESET_TIMER;
> >>                 if (scsi_abort_command(scmd) != SUCCESS) {
> >>                         set_host_byte(scmd, DID_TIME_OUT);
> >>                         scsi_eh_scmd_add(scmd);
> >>                 }
> >>         }
> >> }
> > 
> > I am familiar with this mechanism. My concern is that both the regular
> > completion path and the abort handler must call scsi_dma_unmap() before
> > calling cmd->scsi_done(cmd). I don't see how
> > test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
> > the regular completion path and the abort handler call scsi_dma_unmap()
> > concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
> > is set?
> > 
> > Thanks,
> > 
> > Bart.
> 
> For scsi_dma_unmap() part, that is true - we should make it serialized 
> with
> any other completion paths. I've found it during my fault injection 
> test, so
> I've made a patch to fix it, but it only comes in my next error recovery
> enhancement patch series. Please check the attachment.
> 

Your patch looks good to me.

I have the same idea before but I found that calling scsi_done() (by
__ufshcd_transfer_req_compl()) in ufshcd_abort() in old kernel (e.g.,
4.14) will cause issues but it has been resolved by introduced
SCMD_STATE_COMPLETE flag in newer kernel. So your patch makes sense.

Would you mind sending out this draft patch as a formal patch together
with my patch to fix issues in ufshcd_abort()? Our patches are aimed to
fix cases that host/device reset eventually not being triggered by the
result of ufshcd_abort(), for example, command is aborted successfully
or command is not pending in device with its doorbell also cleared.

Thanks,
Stanley Chu

> Thanks,
> 
> Can Guo.
> 

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Can Guo]

Hi Stanley,

On 2020-08-03 11:00, Stanley Chu wrote:
> Hi Can,
> 
> On Sat, 2020-08-01 at 07:17 +0800, Can Guo wrote:
>> Hi Bart,
>> 
>> On 2020-08-01 00:51, Bart Van Assche wrote:
>> > On 2020-07-31 01:00, Can Guo wrote:
>> >> AFAIK, sychronization of scsi_done is not a problem here, because scsi
>> >> layer
>> >> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to
>> >> prevent
>> >> the concurrency of abort and real completion of it.
>> >>
>> >> Check func scsi_times_out(), hope it helps.
>> >>
>> >> enum blk_eh_timer_return scsi_times_out(struct request *req)
>> >> {
>> >> ...
>> >>         if (rtn == BLK_EH_DONE) {
>> >>                 /*
>> >>                  * Set the command to complete first in order to
>> >> prevent
>> >> a real
>> >>                  * completion from releasing the command while error
>> >> handling
>> >>                  * is using it. If the command was already completed,
>> >> then the
>> >>                  * lower level driver beat the timeout handler, and it
>> >> is safe
>> >>                  * to return without escalating error recovery.
>> >>                  *
>> >>                  * If timeout handling lost the race to a real
>> >> completion, the
>> >>                  * block layer may ignore that due to a fake timeout
>> >> injection,
>> >>                  * so return RESET_TIMER to allow error handling
>> >> another
>> >> shot
>> >>                  * at this command.
>> >>                  */
>> >>                 if (test_and_set_bit(SCMD_STATE_COMPLETE,
>> >> &scmd->state))
>> >>                         return BLK_EH_RESET_TIMER;
>> >>                 if (scsi_abort_command(scmd) != SUCCESS) {
>> >>                         set_host_byte(scmd, DID_TIME_OUT);
>> >>                         scsi_eh_scmd_add(scmd);
>> >>                 }
>> >>         }
>> >> }
>> >
>> > I am familiar with this mechanism. My concern is that both the regular
>> > completion path and the abort handler must call scsi_dma_unmap() before
>> > calling cmd->scsi_done(cmd). I don't see how
>> > test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
>> > the regular completion path and the abort handler call scsi_dma_unmap()
>> > concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
>> > is set?
>> >
>> > Thanks,
>> >
>> > Bart.
>> 
>> For scsi_dma_unmap() part, that is true - we should make it serialized
>> with
>> any other completion paths. I've found it during my fault injection
>> test, so
>> I've made a patch to fix it, but it only comes in my next error 
>> recovery
>> enhancement patch series. Please check the attachment.
>> 
> 
> Your patch looks good to me.
> 
> I have the same idea before but I found that calling scsi_done() (by
> __ufshcd_transfer_req_compl()) in ufshcd_abort() in old kernel (e.g.,
> 4.14) will cause issues but it has been resolved by introduced
> SCMD_STATE_COMPLETE flag in newer kernel. So your patch makes sense.
> 
> Would you mind sending out this draft patch as a formal patch together
> with my patch to fix issues in ufshcd_abort()? Our patches are aimed to
> fix cases that host/device reset eventually not being triggered by the
> result of ufshcd_abort(), for example, command is aborted successfully
> or command is not pending in device with its doorbell also cleared.
> 
> Thanks,
> Stanley Chu
> 

I don't quite actually follow your fix here and I didn't test the 
similar
fault injection scenario like you do here, so I am not sure if I should
just absorb your fix into mine. How about I put my fix in my current 
error
recovery patch series (maybe in next version of it) and you can give 
your
review. So you can still go with your fix as it is. Mine will be picked 
up
later by Martin. What do you think?

Thanks,

Can Guo.

>> Thanks,
>> 
>> Can Guo.
>> 

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Stanley Chu]

Hi Can,

On Mon, 2020-08-03 at 13:14 +0800, Can Guo wrote:
> Hi Stanley,
> 
> On 2020-08-03 11:00, Stanley Chu wrote:
> > Hi Can,
> > 
> > On Sat, 2020-08-01 at 07:17 +0800, Can Guo wrote:
> >> Hi Bart,
> >> 
> >> On 2020-08-01 00:51, Bart Van Assche wrote:
> >> > On 2020-07-31 01:00, Can Guo wrote:
> >> >> AFAIK, sychronization of scsi_done is not a problem here, because scsi
> >> >> layer
> >> >> use the atomic state, namely SCMD_STATE_COMPLETE, of a scsi cmd to
> >> >> prevent
> >> >> the concurrency of abort and real completion of it.
> >> >>
> >> >> Check func scsi_times_out(), hope it helps.
> >> >>
> >> >> enum blk_eh_timer_return scsi_times_out(struct request *req)
> >> >> {
> >> >> ...
> >> >>         if (rtn == BLK_EH_DONE) {
> >> >>                 /*
> >> >>                  * Set the command to complete first in order to
> >> >> prevent
> >> >> a real
> >> >>                  * completion from releasing the command while error
> >> >> handling
> >> >>                  * is using it. If the command was already completed,
> >> >> then the
> >> >>                  * lower level driver beat the timeout handler, and it
> >> >> is safe
> >> >>                  * to return without escalating error recovery.
> >> >>                  *
> >> >>                  * If timeout handling lost the race to a real
> >> >> completion, the
> >> >>                  * block layer may ignore that due to a fake timeout
> >> >> injection,
> >> >>                  * so return RESET_TIMER to allow error handling
> >> >> another
> >> >> shot
> >> >>                  * at this command.
> >> >>                  */
> >> >>                 if (test_and_set_bit(SCMD_STATE_COMPLETE,
> >> >> &scmd->state))
> >> >>                         return BLK_EH_RESET_TIMER;
> >> >>                 if (scsi_abort_command(scmd) != SUCCESS) {
> >> >>                         set_host_byte(scmd, DID_TIME_OUT);
> >> >>                         scsi_eh_scmd_add(scmd);
> >> >>                 }
> >> >>         }
> >> >> }
> >> >
> >> > I am familiar with this mechanism. My concern is that both the regular
> >> > completion path and the abort handler must call scsi_dma_unmap() before
> >> > calling cmd->scsi_done(cmd). I don't see how
> >> > test_and_set_bit(SCMD_STATE_COMPLETE, &scmd->state) could prevent that
> >> > the regular completion path and the abort handler call scsi_dma_unmap()
> >> > concurrently since both calls happen before the SCMD_STATE_COMPLETE bit
> >> > is set?
> >> >
> >> > Thanks,
> >> >
> >> > Bart.
> >> 
> >> For scsi_dma_unmap() part, that is true - we should make it serialized
> >> with
> >> any other completion paths. I've found it during my fault injection
> >> test, so
> >> I've made a patch to fix it, but it only comes in my next error 
> >> recovery
> >> enhancement patch series. Please check the attachment.
> >> 
> > 
> > Your patch looks good to me.
> > 
> > I have the same idea before but I found that calling scsi_done() (by
> > __ufshcd_transfer_req_compl()) in ufshcd_abort() in old kernel (e.g.,
> > 4.14) will cause issues but it has been resolved by introduced
> > SCMD_STATE_COMPLETE flag in newer kernel. So your patch makes sense.
> > 
> > Would you mind sending out this draft patch as a formal patch together
> > with my patch to fix issues in ufshcd_abort()? Our patches are aimed to
> > fix cases that host/device reset eventually not being triggered by the
> > result of ufshcd_abort(), for example, command is aborted successfully
> > or command is not pending in device with its doorbell also cleared.
> > 
> > Thanks,
> > Stanley Chu
> > 
> 
> I don't quite actually follow your fix here and I didn't test the 
> similar
> fault injection scenario like you do here, so I am not sure if I should
> just absorb your fix into mine. How about I put my fix in my current 
> error
> recovery patch series (maybe in next version of it) and you can give 
> your
> review. So you can still go with your fix as it is. Mine will be picked 
> up
> later by Martin. What do you think?
> 

Sure, that's good to me.

Thanks,

Stanley Chu

> Thanks,
> 
> Can Guo.
> 
> >> Thanks,
> >> 
> >> Can Guo.
> >> 

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Bart Van Assche]

On 2020-07-31 16:17, Can Guo wrote:
> For scsi_dma_unmap() part, that is true - we should make it serialized with
> any other completion paths. I've found it during my fault injection test, so
> I've made a patch to fix it, but it only comes in my next error recovery
> enhancement patch series. Please check the attachment.

Hi Can,

It is not clear to me how that patch serializes scsi_dma_unmap() against
other completion paths? Doesn't the regular completion path call
__ufshcd_transfer_req_compl() without holding the host lock?

Thanks,

Bart.


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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Can Guo]

Hi Bart,

On 2020-08-03 11:12, Bart Van Assche wrote:
> On 2020-07-31 16:17, Can Guo wrote:
>> For scsi_dma_unmap() part, that is true - we should make it serialized 
>> with
>> any other completion paths. I've found it during my fault injection 
>> test, so
>> I've made a patch to fix it, but it only comes in my next error 
>> recovery
>> enhancement patch series. Please check the attachment.
> 
> Hi Can,
> 
> It is not clear to me how that patch serializes scsi_dma_unmap() 
> against
> other completion paths? Doesn't the regular completion path call
> __ufshcd_transfer_req_compl() without holding the host lock?
> 
> Thanks,
> 
> Bart.

FYI, ufshcd_intr() holds the host spin lock the whole time. So, to your
question, the regular completion path from IRQ handler has the host lock 
held.

Thanks,

Can Guo.

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

Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification

[Can Guo]

On 2020-07-24 22:02, Stanley Chu wrote:
> 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>
> ---
>  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 577cc0d7487f..9d180da77488 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6493,7 +6493,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",
> @@ -6527,6 +6527,7 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
>  		goto out;
>  	}
> 
> +cleanup:
>  	scsi_dma_unmap(cmd);
> 
>  	spin_lock_irqsave(host->host_lock, flags);

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

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