* [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification @ 2020-07-24 14:02 Stanley Chu 2020-07-27 11:18 ` Avri Altman 2020-08-04 10:01 ` Can Guo 0 siblings, 2 replies; 13+ messages in thread From: Stanley Chu @ 2020-07-24 14:02 UTC (permalink / raw) To: linux-scsi, martin.petersen, avri.altman, alim.akhtar, jejb, bvanassche Cc: beanhuo, asutoshd, cang, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou, 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* RE: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-24 14:02 [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification Stanley Chu @ 2020-07-27 11:18 ` Avri Altman 2020-07-31 1:30 ` Stanley Chu 2020-08-04 10:01 ` Can Guo 1 sibling, 1 reply; 13+ messages in thread From: Avri Altman @ 2020-07-27 11:18 UTC (permalink / raw) To: Stanley Chu, linux-scsi, martin.petersen, alim.akhtar, jejb, bvanassche Cc: beanhuo, asutoshd, cang, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-27 11:18 ` Avri Altman @ 2020-07-31 1:30 ` Stanley Chu 2020-07-31 4:06 ` Bart Van Assche 0 siblings, 1 reply; 13+ messages in thread From: Stanley Chu @ 2020-07-31 1:30 UTC (permalink / raw) To: Avri Altman, Bart Van Assche Cc: linux-scsi, martin.petersen, alim.akhtar, jejb, bvanassche, beanhuo, asutoshd, cang, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 1:30 ` Stanley Chu @ 2020-07-31 4:06 ` Bart Van Assche 2020-07-31 8:00 ` Can Guo 0 siblings, 1 reply; 13+ messages in thread From: Bart Van Assche @ 2020-07-31 4:06 UTC (permalink / raw) To: Stanley Chu, Avri Altman Cc: linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, cang, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 4:06 ` Bart Van Assche @ 2020-07-31 8:00 ` Can Guo 2020-07-31 16:51 ` Bart Van Assche 0 siblings, 1 reply; 13+ messages in thread From: Can Guo @ 2020-07-31 8:00 UTC (permalink / raw) To: Bart Van Assche Cc: Stanley Chu, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 8:00 ` Can Guo @ 2020-07-31 16:51 ` Bart Van Assche 2020-07-31 23:17 ` Can Guo 0 siblings, 1 reply; 13+ messages in thread From: Bart Van Assche @ 2020-07-31 16:51 UTC (permalink / raw) To: Can Guo Cc: Stanley Chu, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 16:51 ` Bart Van Assche @ 2020-07-31 23:17 ` Can Guo 2020-08-03 3:00 ` Stanley Chu 2020-08-03 3:12 ` Bart Van Assche 0 siblings, 2 replies; 13+ messages in thread From: Can Guo @ 2020-07-31 23:17 UTC (permalink / raw) To: Bart Van Assche Cc: Stanley Chu, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou [-- 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. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 23:17 ` Can Guo @ 2020-08-03 3:00 ` Stanley Chu 2020-08-03 5:14 ` Can Guo 2020-08-03 3:12 ` Bart Van Assche 1 sibling, 1 reply; 13+ messages in thread From: Stanley Chu @ 2020-08-03 3:00 UTC (permalink / raw) To: Can Guo Cc: Bart Van Assche, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-08-03 3:00 ` Stanley Chu @ 2020-08-03 5:14 ` Can Guo 2020-08-03 5:27 ` Stanley Chu 0 siblings, 1 reply; 13+ messages in thread From: Can Guo @ 2020-08-03 5:14 UTC (permalink / raw) To: Stanley Chu Cc: Bart Van Assche, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-08-03 5:14 ` Can Guo @ 2020-08-03 5:27 ` Stanley Chu 0 siblings, 0 replies; 13+ messages in thread From: Stanley Chu @ 2020-08-03 5:27 UTC (permalink / raw) To: Can Guo Cc: Bart Van Assche, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. > >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-31 23:17 ` Can Guo 2020-08-03 3:00 ` Stanley Chu @ 2020-08-03 3:12 ` Bart Van Assche 2020-08-03 5:07 ` Can Guo 1 sibling, 1 reply; 13+ messages in thread From: Bart Van Assche @ 2020-08-03 3:12 UTC (permalink / raw) To: Can Guo Cc: Stanley Chu, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-08-03 3:12 ` Bart Van Assche @ 2020-08-03 5:07 ` Can Guo 0 siblings, 0 replies; 13+ messages in thread From: Can Guo @ 2020-08-03 5:07 UTC (permalink / raw) To: Bart Van Assche Cc: Stanley Chu, Avri Altman, linux-scsi, martin.petersen, alim.akhtar, jejb, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification 2020-07-24 14:02 [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification Stanley Chu 2020-07-27 11:18 ` Avri Altman @ 2020-08-04 10:01 ` Can Guo 1 sibling, 0 replies; 13+ messages in thread From: Can Guo @ 2020-08-04 10:01 UTC (permalink / raw) To: Stanley Chu Cc: linux-scsi, martin.petersen, avri.altman, alim.akhtar, jejb, bvanassche, beanhuo, asutoshd, matthias.bgg, linux-mediatek, linux-arm-kernel, linux-kernel, kuohong.wang, peter.wang, chun-hung.wu, andy.teng, chaotian.jing, cc.chou 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-04 10:02 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-24 14:02 [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification Stanley Chu 2020-07-27 11:18 ` Avri Altman 2020-07-31 1:30 ` Stanley Chu 2020-07-31 4:06 ` Bart Van Assche 2020-07-31 8:00 ` Can Guo 2020-07-31 16:51 ` Bart Van Assche 2020-07-31 23:17 ` Can Guo 2020-08-03 3:00 ` Stanley Chu 2020-08-03 5:14 ` Can Guo 2020-08-03 5:27 ` Stanley Chu 2020-08-03 3:12 ` Bart Van Assche 2020-08-03 5:07 ` Can Guo 2020-08-04 10:01 ` Can Guo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).