On 14/10/2021 21:50, Adrian Hunter wrote: > On 14/10/2021 19:47, Bart Van Assche wrote: >> On 10/13/21 11:02 PM, Adrian Hunter wrote: >>> On 14/10/2021 07:14, Bart Van Assche wrote: >>>> Wouldn't it be better to keep the code that clears req->end_io_data >>>> and to change complete(c) into if(c) complete(c) in >>>> ufshcd_tmc_handler()? >>> >>> If that were needed, it would imply the synchronization was broken >>> i.e. why are we referencing a request that has already been through >>> blk_put_request()? >> >> The scenario I'm worried about is as follows: >> * __ufshcd_issue_tm_cmd() issues a task management function. >> * No completion is received before TM_CMD_TIMEOUT has expired (100 ms). >> * ufshcd_clear_tm_cmd() fails. >> * The TMF completes, ufshcd_tmc_handler() is called and that function calls complete(req->end_io_data). >> >> Can this happen? > > No because the tag's bit is cleared from outstanding_tasks before blk_put_request() and > access to outstanding_tasks is protected by host_lock in both __ufshcd_issue_tm_cmd() > and ufshcd_clear_tm_cmd(). Although I just noticed a different issue. In ufshcd_tmc_handler() the task doorbell register needs to be read in conjunction with outstanding_tasks i.e. under the spinlock I will send a V2. > >> >> I agree that this scenario involves completion of a request that has already been through blk_put_request(). >> >> Thanks, >> >> Bart. >
