Messages in this thread Patch in this message | | | Date | Sat, 01 Aug 2020 07:17:08 +0800 | From | Can Guo <> | Subject | Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification |
| |
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.
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.
| |