Re: [PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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
Re: [PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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.
Re: [PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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.
Re: [PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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.
Re: [PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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.
[PATCH v3 3/3] scsi: ufs: Fix wrong Task Tag used in task management request UPIUs
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
---
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 =
- 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, >outstanding_tasks);
+ __set_bit(task_tag, >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, >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, >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.
