| Message ID | 1611807365-35513-4-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Three fixes for task management request implementation | expand |
On 1/27/21 8:16 PM, Can Guo wrote: > 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. Why is the current code wrong and why is this patch the proper fix? Please explain this in the patch description. > + * blk_get_request() used here is only to get a free tag. Please fix the word order in this comment ("blk_get_request() is used here only to get a free tag"). > + ufshcd_release(hba); > blk_put_request(req); > > - ufshcd_release(hba); An explanation for this change is missing from the patch description. Thanks, Bart.
On 2021-01-29 11:15, Bart Van Assche wrote: > On 1/27/21 8:16 PM, Can Guo wrote: >> 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. > > Why is the current code wrong and why is this patch the proper fix? > Please explain this in the patch description. > req->tag is the tag allocated for one TMR, no? >> + * blk_get_request() used here is only to get a free tag. > > Please fix the word order in this comment ("blk_get_request() is used > here only to get a free tag"). Sure. > >> + ufshcd_release(hba); >> blk_put_request(req); >> >> - ufshcd_release(hba); > > An explanation for this change is missing from the patch description. > This is just for symmetric coding since this change is almost re-writing the whole func - at the entrence it calls blk_get_request() and ufshcd_hold(), so before exit it'd be good to call ufshcd_release() before blk_put_request(). If you think this single line change worths a separate patch, I can split it out in next version. Thanks, Can Guo. > Thanks, > > Bart.
On 1/28/21 9:57 PM, Can Guo wrote: > On 2021-01-29 11:15, Bart Van Assche wrote: >> On 1/27/21 8:16 PM, Can Guo wrote: >>> 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. >> >> Why is the current code wrong and why is this patch the proper fix? >> Please explain this in the patch description. > > req->tag is the tag allocated for one TMR, no? Hi Can, Commit e293313262d3 ("scsi: ufs: Fix broken task management command implementation") includes the following changes: + task_tag = hba->nutrs + free_slot; task_req_upiup->header.dword_0 = UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0, - lrbp->lun, lrbp->task_tag); + lun_id, task_tag); task_req_upiup->header.dword_1 = UPIU_HEADER_DWORD(0, tm_function, 0, 0); As one can see the value written in dword_0 starts at hba->nutrs. Was that code correct? If that code was correct, does your patch perhaps break task management support? Thanks, Bart.
On 2021-02-01 10:39, Bart Van Assche wrote: > On 1/28/21 9:57 PM, Can Guo wrote: >> On 2021-01-29 11:15, Bart Van Assche wrote: >>> On 1/27/21 8:16 PM, Can Guo wrote: >>>> 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. >>> >>> Why is the current code wrong and why is this patch the proper fix? >>> Please explain this in the patch description. >> >> req->tag is the tag allocated for one TMR, no? > > Hi Can, > Commit e293313262d3 ("scsi: ufs: Fix broken task management command > implementation") includes the following changes: > > + task_tag = hba->nutrs + free_slot; > task_req_upiup->header.dword_0 = > UPIU_HEADER_DWORD(UPIU_TRANSACTION_TASK_REQ, 0, > - lrbp->lun, > lrbp->task_tag); > + lun_id, task_tag); > task_req_upiup->header.dword_1 = > UPIU_HEADER_DWORD(0, tm_function, 0, 0); > > As one can see the value written in dword_0 starts at hba->nutrs. Was > that code correct? If that code was correct, does your patch perhaps > break task management support? That code is wrong. The Task Tag in Dword_0 should be the real tag we allocated for TMR. The transfer request Task Tag which we are trying to abort is given in Dword_5, which is the Input Parameter 3 of the TMR UPIU. I am not sure why the author gave hba->nutrs + req->tag as the Task Tag of one TMR, the commit msg abot this part is not quite informative.... Table 10.22 — Task Management Request UPIU TASK MANAGEMENT REQUEST UPIU ---------------------------------- |0 |1 |2 |3 | ---------------------------------- |xx00 0100b| Flags |LUN |Task Tag| ---------------------------------- ... 16 (MSB) |17 |18 |19 (LSB)| ---------------------------------- Input Parameter 2 ---------------------------------- Table 10.24 — Task Management Input Parameters Field Description Input Parameter 2 LSB: Task Tag of the task/command operated by the task management function. Thanks, Can Guo. > > Thanks, > > Bart.
On 2/4/21 10:09 PM, Can Guo wrote: > That code is wrong. The Task Tag in Dword_0 should be the real tag we > allocated for TMR. The transfer request Task Tag which we are trying to > abort is given in Dword_5, which is the Input Parameter 3 of the TMR UPIU. > I am not sure why the author gave hba->nutrs + req->tag as the Task Tag > of one TMR, the commit msg abot this part is not quite informative.... > > Table 10.22 — Task Management Request UPIU > TASK MANAGEMENT REQUEST UPIU > ---------------------------------- > |0 |1 |2 |3 | > ---------------------------------- > |xx00 0100b| Flags |LUN |Task Tag| > ---------------------------------- > ... > 16 (MSB) |17 |18 |19 (LSB)| > ---------------------------------- > Input Parameter 2 > ---------------------------------- > > Table 10.24 — Task Management Input Parameters > Field Description > Input Parameter 2 LSB: Task Tag of the task/command operated by the task > management function. Thanks for the clarification. Feel free to add the following to this patch: Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 43894a3..72fa14b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6384,38 +6384,33 @@ 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() used here is only to get a free tag. */ req = blk_get_request(q, REQ_OP_DRV_OUT, 0); if (IS_ERR(req)) return PTR_ERR(req); - 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); req->end_io_data = &wait; - 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(); @@ -6437,24 +6432,23 @@ 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); + } else { + __clear_bit(task_tag, &hba->outstanding_tasks); + } 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); - spin_unlock_irqrestore(hba->host->host_lock, flags); - + ufshcd_release(hba); blk_put_request(req); - ufshcd_release(hba); return err; }
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") Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)