| Message ID | 20200724140246.19434-1-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | b10178ee7fa88b68a9e8adc06534d2605cb0ec23 |
| Headers | show |
| Series | [v4] scsi: ufs: Cleanup completed request without interrupt notification | expand |
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
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
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 <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>
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);
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(-)