From: Can Guo <cang@codeaurora.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: linux-scsi@vger.kernel.org, martin.petersen@oracle.com,
andy.teng@mediatek.com, jejb@linux.ibm.com,
chun-hung.wu@mediatek.com, kuohong.wang@mediatek.com,
linux-kernel@vger.kernel.org, asutoshd@codeaurora.org,
Avri Altman <Avri.Altman@wdc.com>,
linux-mediatek@lists.infradead.org, peter.wang@mediatek.com,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
Stanley Chu <stanley.chu@mediatek.com>,
chaotian.jing@mediatek.com, cc.chou@mediatek.com,
linux-arm-kernel@lists.infradead.org, beanhuo@micron.com
Subject: Re: [PATCH v4] scsi: ufs: Cleanup completed request without interrupt notification
Date: Sat, 01 Aug 2020 07:17:08 +0800 [thread overview]
Message-ID: <548b602daa1e15415625cb8d1f81a208@codeaurora.org> (raw)
In-Reply-To: <f45c6c47-ffc5-3f8e-3234-9e5989dbf996@acm.org>
[-- 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.
[-- Attachment #3: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
next prev parent reply other threads:[~2020-07-31 23:17 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=548b602daa1e15415625cb8d1f81a208@codeaurora.org \
--to=cang@codeaurora.org \
--cc=Avri.Altman@wdc.com \
--cc=alim.akhtar@samsung.com \
--cc=andy.teng@mediatek.com \
--cc=asutoshd@codeaurora.org \
--cc=beanhuo@micron.com \
--cc=bvanassche@acm.org \
--cc=cc.chou@mediatek.com \
--cc=chaotian.jing@mediatek.com \
--cc=chun-hung.wu@mediatek.com \
--cc=jejb@linux.ibm.com \
--cc=kuohong.wang@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=matthias.bgg@gmail.com \
--cc=peter.wang@mediatek.com \
--cc=stanley.chu@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).