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]>
---
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
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 <[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 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
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 <[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 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
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.
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.
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.
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.
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.
>
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.
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.
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.
>>
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.
> >>
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 <[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 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 <[email protected]>