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 <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 36 +++++++++++++++---------------------
1 file changed, 15 insertions(+), 21 deletions(-)
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;
}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
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 <[email protected]>