* [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout [not found] <1617166236-39908-1-git-send-email-cang@codeaurora.org> @ 2021-03-31 4:50 ` Can Guo 2021-03-31 16:45 ` Avri Altman 2021-03-31 4:50 ` [PATCH v4 2/2] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs Can Guo 1 sibling, 1 reply; 7+ messages in thread From: Can Guo @ 2021-03-31 4:50 UTC (permalink / raw) To: asutoshd, nguyenb, hongwus, linux-scsi, kernel-team, cang Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo, Jaegeuk Kim, Bart Van Assche, open list ufshcd_tmc_handler() calls blk_mq_tagset_busy_iter(fn = ufshcd_compl_tm()), but since blk_mq_tagset_busy_iter() only iterates over all reserved tags and requests which are not in IDLE state, ufshcd_compl_tm() never gets a chance to run. Thus, TMR always ends up with completion timeout. Fix it by calling blk_mq_start_request() in __ufshcd_issue_tm_cmd(). Fixes: 69a6c269c097 ("scsi: ufs: Use blk_{get,put}_request() to allocate and free TMFs") Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index b49555fa..d4f8cb2 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6464,6 +6464,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, spin_lock_irqsave(host->host_lock, flags); task_tag = hba->nutrs + free_slot; + blk_mq_start_request(req); treq->req_header.dword_0 |= cpu_to_be32(task_tag); -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout 2021-03-31 4:50 ` [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout Can Guo @ 2021-03-31 16:45 ` Avri Altman 2021-04-01 2:57 ` Can Guo 2021-04-01 3:39 ` Bart Van Assche 0 siblings, 2 replies; 7+ messages in thread From: Avri Altman @ 2021-03-31 16:45 UTC (permalink / raw) To: Can Guo, asutoshd, nguyenb, hongwus, linux-scsi, kernel-team Cc: Alim Akhtar, James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo, Jaegeuk Kim, Bart Van Assche, open list > ufshcd_tmc_handler() calls blk_mq_tagset_busy_iter(fn = > ufshcd_compl_tm()), > but since blk_mq_tagset_busy_iter() only iterates over all reserved tags > and requests which are not in IDLE state, ufshcd_compl_tm() never gets a > chance to run. Thus, TMR always ends up with completion timeout. Fix it by > calling blk_mq_start_request() in __ufshcd_issue_tm_cmd(). > > Fixes: 69a6c269c097 ("scsi: ufs: Use blk_{get,put}_request() to allocate and > free TMFs") > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index b49555fa..d4f8cb2 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6464,6 +6464,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba > *hba, > > spin_lock_irqsave(host->host_lock, flags); > task_tag = hba->nutrs + free_slot; > + blk_mq_start_request(req); Maybe just set req->state to MQ_RQ_IN_FLIGHT Without all other irrelevant initializations such as add timeout etc. Thanks, Avri > > treq->req_header.dword_0 |= cpu_to_be32(task_tag); > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a > Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout 2021-03-31 16:45 ` Avri Altman @ 2021-04-01 2:57 ` Can Guo 2021-04-01 3:39 ` Bart Van Assche 1 sibling, 0 replies; 7+ messages in thread From: Can Guo @ 2021-04-01 2:57 UTC (permalink / raw) To: Avri Altman Cc: asutoshd, nguyenb, hongwus, linux-scsi, kernel-team, Alim Akhtar, James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo, Jaegeuk Kim, Bart Van Assche, open list On 2021-04-01 00:45, Avri Altman wrote: >> ufshcd_tmc_handler() calls blk_mq_tagset_busy_iter(fn = >> ufshcd_compl_tm()), >> but since blk_mq_tagset_busy_iter() only iterates over all reserved >> tags >> and requests which are not in IDLE state, ufshcd_compl_tm() never gets >> a >> chance to run. Thus, TMR always ends up with completion timeout. Fix >> it by >> calling blk_mq_start_request() in __ufshcd_issue_tm_cmd(). >> >> Fixes: 69a6c269c097 ("scsi: ufs: Use blk_{get,put}_request() to >> allocate and >> free TMFs") >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index b49555fa..d4f8cb2 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -6464,6 +6464,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba >> *hba, >> >> spin_lock_irqsave(host->host_lock, flags); >> task_tag = hba->nutrs + free_slot; >> + blk_mq_start_request(req); > Maybe just set req->state to MQ_RQ_IN_FLIGHT > Without all other irrelevant initializations such as add timeout etc. > I don't see any other drivers do that, is it appropriate to call WRITE_ONCE(rq->state, MQ_RQ_IN_FLIGHT) outside block layer? Thanks, Can Guo. > Thanks, > Avri >> >> treq->req_header.dword_0 |= cpu_to_be32(task_tag); >> >> -- >> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a >> Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout 2021-03-31 16:45 ` Avri Altman 2021-04-01 2:57 ` Can Guo @ 2021-04-01 3:39 ` Bart Van Assche 1 sibling, 0 replies; 7+ messages in thread From: Bart Van Assche @ 2021-04-01 3:39 UTC (permalink / raw) To: Avri Altman, Can Guo, asutoshd, nguyenb, hongwus, linux-scsi, kernel-team Cc: Alim Akhtar, James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo, Jaegeuk Kim, open list On 3/31/21 9:45 AM, Avri Altman wrote: >> ufshcd_tmc_handler() calls blk_mq_tagset_busy_iter(fn = >> ufshcd_compl_tm()), >> but since blk_mq_tagset_busy_iter() only iterates over all reserved tags >> and requests which are not in IDLE state, ufshcd_compl_tm() never gets a >> chance to run. Thus, TMR always ends up with completion timeout. Fix it by >> calling blk_mq_start_request() in __ufshcd_issue_tm_cmd(). >> >> Fixes: 69a6c269c097 ("scsi: ufs: Use blk_{get,put}_request() to allocate and >> free TMFs") >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index b49555fa..d4f8cb2 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -6464,6 +6464,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba >> *hba, >> >> spin_lock_irqsave(host->host_lock, flags); >> task_tag = hba->nutrs + free_slot; >> + blk_mq_start_request(req); > Maybe just set req->state to MQ_RQ_IN_FLIGHT > Without all other irrelevant initializations such as add timeout etc. Hmm ... I'm not sure that any of the actions performed by blk_mq_start_request() are irrelevant in this context. Additionally, no other block or SCSI driver sets MQ_RQ_IN_FLIGHT directly. Thanks, Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs [not found] <1617166236-39908-1-git-send-email-cang@codeaurora.org> 2021-03-31 4:50 ` [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout Can Guo @ 2021-03-31 4:50 ` Can Guo 1 sibling, 0 replies; 7+ messages in thread From: Can Guo @ 2021-03-31 4:50 UTC (permalink / raw) To: asutoshd, nguyenb, hongwus, linux-scsi, kernel-team, cang Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley, Martin K. Petersen, Stanley Chu, Bean Huo, Jaegeuk Kim, Sujit Reddy Thumma, Yaniv Gardi, Vinayak Holikatti, open list In __ufshcd_issue_tm_cmd(), it is not right to use hba->nutrs + req->tag as the Task Tag in one TMR UPIU. Directly use req->tag as the Task Tag. Fixes: e293313262d3 ("scsi: ufs: Fix broken task management command implementation") Reviewed-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index d4f8cb2..cdd8c3d 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6446,38 +6446,35 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, DECLARE_COMPLETION_ONSTACK(wait); struct request *req; unsigned long flags; - int free_slot, task_tag, err; + int task_tag, err; /* - * Get free slot, sleep if slots are unavailable. - * Even though we use wait_event() which sleeps indefinitely, - * the maximum wait time is bounded by %TM_CMD_TIMEOUT. + * blk_get_request() is used here only to get a free tag. */ req = blk_get_request(q, REQ_OP_DRV_OUT, 0); if (IS_ERR(req)) return PTR_ERR(req); req->end_io_data = &wait; - free_slot = req->tag; WARN_ON_ONCE(free_slot < 0 || free_slot >= hba->nutmrs); ufshcd_hold(hba, false); spin_lock_irqsave(host->host_lock, flags); - task_tag = hba->nutrs + free_slot; blk_mq_start_request(req); + task_tag = req->tag; treq->req_header.dword_0 |= cpu_to_be32(task_tag); - memcpy(hba->utmrdl_base_addr + free_slot, treq, sizeof(*treq)); - ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function); + memcpy(hba->utmrdl_base_addr + task_tag, treq, sizeof(*treq)); + ufshcd_vops_setup_task_mgmt(hba, task_tag, tm_function); /* send command to the controller */ - __set_bit(free_slot, &hba->outstanding_tasks); + __set_bit(task_tag, &hba->outstanding_tasks); /* Make sure descriptors are ready before ringing the task doorbell */ wmb(); - ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); + ufshcd_writel(hba, 1 << task_tag, REG_UTP_TASK_REQ_DOOR_BELL); /* Make sure that doorbell is committed immediately */ wmb(); @@ -6497,24 +6494,24 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_ERR); dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n", __func__, tm_function); - if (ufshcd_clear_tm_cmd(hba, free_slot)) - dev_WARN(hba->dev, "%s: unable clear tm cmd (slot %d) after timeout\n", - __func__, free_slot); + if (ufshcd_clear_tm_cmd(hba, task_tag)) + dev_WARN(hba->dev, "%s: unable to clear tm cmd (slot %d) after timeout\n", + __func__, task_tag); err = -ETIMEDOUT; } else { err = 0; - memcpy(treq, hba->utmrdl_base_addr + free_slot, sizeof(*treq)); + memcpy(treq, hba->utmrdl_base_addr + task_tag, sizeof(*treq)); ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_COMP); } spin_lock_irqsave(hba->host->host_lock, flags); - __clear_bit(free_slot, &hba->outstanding_tasks); + __clear_bit(task_tag, &hba->outstanding_tasks); spin_unlock_irqrestore(hba->host->host_lock, flags); + ufshcd_release(hba); blk_put_request(req); - ufshcd_release(hba); return err; } -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 7+ messages in thread
[parent not found: <CGME20210401064419epcms2p6b289c9ba573d15883e3e92ddcd233e11@epcms2p6>]
* RE: [PATCH v4 2/2] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs [not found] <CGME20210401064419epcms2p6b289c9ba573d15883e3e92ddcd233e11@epcms2p6> @ 2021-04-01 6:44 ` Daejun Park 2021-04-01 7:17 ` Can Guo 0 siblings, 1 reply; 7+ messages in thread From: Daejun Park @ 2021-04-01 6:44 UTC (permalink / raw) To: cang Cc: ALIM AKHTAR, asutoshd, avri.altman, beanhuo, hongwus, jaegeuk, jejb, kernel-team, linux-kernel, linux-scsi, martin.petersen, nguyenb, stanley.chu, sthumma, vinholikatti, ygardi Hi, Can Guo > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c ... > > req->end_io_data = &wait; > - free_slot = req->tag; > WARN_ON_ONCE(free_slot < 0 || free_slot >= hba->nutmrs); I think this line should be removed. Thanks, Daejun ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs 2021-04-01 6:44 ` Daejun Park @ 2021-04-01 7:17 ` Can Guo 0 siblings, 0 replies; 7+ messages in thread From: Can Guo @ 2021-04-01 7:17 UTC (permalink / raw) To: daejun7.park Cc: ALIM AKHTAR, asutoshd, avri.altman, beanhuo, hongwus, jaegeuk, jejb, kernel-team, linux-kernel, linux-scsi, martin.petersen, nguyenb, stanley.chu, sthumma, vinholikatti, ygardi On 2021-04-01 14:44, Daejun Park wrote: > Hi, Can Guo > >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > ... >> >> req->end_io_data = &wait; >> - free_slot = req->tag; >> WARN_ON_ONCE(free_slot < 0 || free_slot >= hba->nutmrs); > I think this line should be removed. > Oh, yes, will remove it in next version. Thanks, Can Guo. > Thanks, > Daejun ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-01 7:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1617166236-39908-1-git-send-email-cang@codeaurora.org> 2021-03-31 4:50 ` [PATCH v4 1/2] scsi: ufs: Fix task management request completion timeout Can Guo 2021-03-31 16:45 ` Avri Altman 2021-04-01 2:57 ` Can Guo 2021-04-01 3:39 ` Bart Van Assche 2021-03-31 4:50 ` [PATCH v4 2/2] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs Can Guo [not found] <CGME20210401064419epcms2p6b289c9ba573d15883e3e92ddcd233e11@epcms2p6> 2021-04-01 6:44 ` Daejun Park 2021-04-01 7:17 ` 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).