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

[Can Guo]

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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>

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.