Re: [PATCH v2] scsi: ufs: Fix some problems in task management request implementation
On 1/20/21 7:23 PM, Can Guo wrote: > Current task management request send/compl implementation is broken, the > problems and fixes are listed as below: > > Problem: TMR completion timeout. 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 > started requests, so ufshcd_compl_tm() never gets a chance to run. > Fix: Call blk_mq_start_request() in __ufshcd_issue_tm_cmd(). > > Problem: Race condition in send/compl paths. 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 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, > req->end_io_data is set but the corresponding tag has not been set > in the REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() may > wrongly complete TMRs which have not been sent. > Fix: Protect 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. > > Problem: In __ufshcd_issue_tm_cmd(), it is not right to use hba->nutrs + > req->tag as the Task Tag in one TMR UPIU. > Fix: Directly use req->tag as Task Tag. Please split this patch into three separate patches - one patch per problem that has been described above. Thanks, Bart.
RE: [PATCH v2] scsi: ufs: Fix some problems in task management request implementation
> Current task management request send/compl implementation is broken, the > problems and fixes are listed as below: > > Problem: TMR completion timeout. 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 > started requests, so ufshcd_compl_tm() never gets a chance to run. > Fix: Call blk_mq_start_request() in __ufshcd_issue_tm_cmd(). > > Problem: Race condition in send/compl paths. 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 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, > req->end_io_data is set but the corresponding tag has not been set > in the REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() > may > wrongly complete TMRs which have not been sent. > Fix: Protect 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. > > Problem: In __ufshcd_issue_tm_cmd(), it is not right to use hba->nutrs + > req->tag as the Task Tag in one TMR UPIU. > Fix: Directly use req->tag as Task Tag. > > Cc: Jaegeuk Kim Since you are practically reverting Bart's change (69a6c269c097), maybe cc him as well, And add a fixes tag? Also, even though all those fixes are around the same place, but fixing different issues, You might want to consider to separate those. Whatever you think. Thanks, Avri
Re: [PATCH v2] scsi: ufs: Fix some problems in task management request implementation
On 2021-01-25 19:36, Avri Altman wrote: Current task management request send/compl implementation is broken, the problems and fixes are listed as below: Problem: TMR completion timeout. 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 started requests, so ufshcd_compl_tm() never gets a chance to run. Fix: Call blk_mq_start_request() in __ufshcd_issue_tm_cmd(). Problem: Race condition in send/compl paths. 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 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, req->end_io_data is set but the corresponding tag has not been set in the REG_UTP_TASK_REQ_DOOR_BELL. Thus, ufshcd_tmc_handler() may wrongly complete TMRs which have not been sent. Fix: Protect 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. Problem: In __ufshcd_issue_tm_cmd(), it is not right to use hba->nutrs + req->tag as the Task Tag in one TMR UPIU. Fix: Directly use req->tag as Task Tag. Cc: Jaegeuk Kim Since you are practically reverting Bart's change (69a6c269c097), maybe cc him as well, And add a fixes tag? Hi Avri, It is not reverting Bart's change, but making TMR work properly based on it. I am ok with the Bart's idea of getting a tag for TMR from blk_get_request(), and this patch respects that idea. Also, even though all those fixes are around the same place, but fixing different issues, You might want to consider to separate those. Whatever you think. Thanks for the suggestion. I treat it as a whole because it is convenient for me to get it ported and tested over different platforms. I may revise it in next version after more comments come on it. Thanks, Can Guo. Thanks, Avri