Three fixes for task management request implementation

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 <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/scsi/ufs/ufshcd.c +0 -0

index 8da75e6..c0c5925 100644

Quoted 24 lines · Can Guo

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 <[email protected]>

---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

drivers/scsi/ufs/ufshcd.c +1 -0

index 8da75e6..c0c5925 100644
@@ -6395,6 +6395,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);
 

drivers/scsi/ufs/ufshcd.c static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, · line 6400

Quoted 1 line

drivers/scsi/ufs/ufshcd.c +0 -0

 	treq->req_header.dword_0 |= cpu_to_be32(task_tag);

blk_mq_start_request() not only marks a request as in-flight but also starts a timer. However, no timeout handler has been defined in ufshcd_tmf_ops. Should a timeout handler be defined in that data structure?

Thanks,

Bart.

ufshcd_compl_tm() looks for all 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL and call complete() for each req who has the req->end_io_data set. There can be a race condition btw tmc send/compl, because the req->end_io_data is set, in __ufshcd_issue_tm_cmd(), without host lock protection, so it is possible that when ufshcd_compl_tm() checks the req->end_io_data, it is set but the corresponding tag has not been set in REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() may wrongly complete TMRs which have not been sent out. Fix it by protecting req->end_io_data with host lock, and let ufshcd_compl_tm() only handle those tm cmds which have been completed instead of looking for 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL.

Quoted 10 lines · Can Guo

ufshcd_compl_tm() looks for all 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL and call complete() for each req who has the req->end_io_data set. There can be a race condition btw tmc send/compl, because the req->end_io_data is set, in __ufshcd_issue_tm_cmd(), without host lock protection, so it is possible that when ufshcd_compl_tm() checks the req->end_io_data, it is set but the corresponding tag has not been set in REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() may wrongly complete TMRs which have not been sent out. Fix it by protecting req->end_io_data with host lock, and let ufshcd_compl_tm() only handle those tm cmds which have been completed instead of looking for 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL.

I don't know any other block driver that needs locking to protect races between submission and completion context. Can the block layer timeout mechanism be used instead of the mechanism introduced by this patch, e.g. by using blk_execute_rq_nowait() to submit requests? That would allow to reuse the existing mechanism in the block layer core to handle races between request completion and timeout handling.

Thanks,

Bart.

Quoted 12 lines · Bart Van Assche

Quoted 10 lines · Can Guo

ufshcd_compl_tm() looks for all 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL and call complete() for each req who has the req->end_io_data set. There can be a race condition btw tmc send/compl, because the req->end_io_data is set, in __ufshcd_issue_tm_cmd(), without host lock protection, so it is possible that when ufshcd_compl_tm() checks the req->end_io_data, it is set but the corresponding tag has not been set in REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() may wrongly complete TMRs which have not been sent out. Fix it by protecting req->end_io_data with host lock, and let ufshcd_compl_tm() only handle those tm cmds which have been completed instead of looking for 0 bits in the REG_UTP_TASK_REQ_DOOR_BELL.

Quoted 6 lines

I don't know any other block driver that needs locking to protect races between submission and completion context. Can the block layer timeout mechanism be used instead of the mechanism introduced by this patch, e.g. by using blk_execute_rq_nowait() to submit requests? That would allow to reuse the existing mechanism in the block layer core to handle races between request completion and timeout handling.

This patch is not introducing any new mechanism, it is fixing the usage of completion (req->end_io_data = c) introduced by commit 69a6c269c097 ("scsi: ufs: Use blk_{get,put}_request() to allocate and free TMFs"). If you have better idea to get it fixed once for all, we are glad to take your change to get it fixed asap.

Regards,

Can Guo.

Quoted 4 lines

Thanks,

Bart.

On second thought, actually the 1st fix alone is enough to eliminate the race condition. Because blk_mq_tagset_busy_iter() only iterates over all requests which are not in IDLE state, if blk_mq_start_request() is called within the protection of host spin lock, ufshcd_compl_tm() shall not run into the scenario where req->end_io_data is set but REG_UTP_TASK_REQ_DOOR_BELL
has not been set. What do you think?

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.

Quoted 2 lines · Can Guo

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.

drivers/scsi/ufs/ufshcd.c static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, · line 6390

Quoted 1 line

drivers/scsi/ufs/ufshcd.c +1 -0

+	 * 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").

drivers/scsi/ufs/ufshcd.c static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba, · line 6457

Quoted 5 lines

drivers/scsi/ufs/ufshcd.c +1 -1

+	ufshcd_release(hba);
 	blk_put_request(req);
 
-	ufshcd_release(hba);

An explanation for this change is missing from the patch description.

Thanks,

Bart.

Quoted 7 lines · Bart Van Assche

Quoted 3 lines · Can Guo

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?

Quoted 10 lines · Can Guo

Quoted 7 lines · Bart Van Assche

Quoted 3 lines · Can Guo

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:

(quoted diff) +1 -1

+       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.

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.