On Thu, 2024-09-19 at 11:49 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 9/19/24 5:16 AM, Peter Wang (王信友) wrote: > > The four case flows for abort are as follows: > > ---------------------------------------------------------------- > > > > Case1: DBR ufshcd_abort > > Please follow the terminology from the UFSHCI 4.0 standard and use > the > word "legacy" instead of "DBR". > Hi Bart, Okay, but the current code comments all use 'SDB mode'. Should we just stick with that term? > > In this case, you can see that ufshcd_release_scsi_cmd will > > definitely be called. > > > > ufshcd_abort() > > ufshcd_try_to_abort_task()// It should trigger an > > interrupt, but the tensor might not > > get outstanding_lock > > clear outstanding_reqs tag > > ufshcd_release_scsi_cmd() > > release outstanding_lock > > > > ufshcd_intr() > > ufshcd_sl_intr() > > ufshcd_transfer_req_compl() > > ufshcd_poll() > > get outstanding_lock > > clear outstanding_reqs tag > > release outstanding_lock > > __ufshcd_transfer_req_compl() > > ufshcd_compl_one_cqe() > > cmd->result = DID_REQUEUE// mediatek may need quirk > > change DID_ABORT to DID_REQUEUE > > ufshcd_release_scsi_cmd() > > scsi_done(); > > > > In most cases, ufshcd_intr will not reach scsi_done because the > > outstanding_reqs tag is cleared by the original thread. > > Therefore, whether there is an interrupt or not doesn't affect > > the result because the ISR will do nothing in most cases. > > > > In a very low chance, the ISR will reach scsi_done and notify > > SCSI to requeue, and the original thread will not > > call ufshcd_release_scsi_cmd. > > MediaTek may need to change DID_ABORT to DID_REQUEUE in this > > situation, or perhaps not handle this ISR at all. > > Please modify ufshcd_compl_one_cqe() such that it ignores commands > with status OCS_ABORTED. This will make the UFSHCI driver behave in > the same way for all UFSHCI controllers, whether or not clearing a > command triggers a completion interrupt. > Yes, I am considering how to modify the code here. > > ---------------------------------------------------------------- > > > > Case2: MCQ ufshcd_abort > > > > In the case of MCQ ufshcd_abort, you can also see that > > ufshcd_release_scsi_cmd will definitely be called too. > > However, there seems to be a problem here, as > > ufshcd_release_scsi_cmd might be called twice. > > This is because cmd is not null in ufshcd_release_scsi_cmd, > > which the previous version would set cmd to null. > > Skipping OCS: ABORTED in ufshcd_compl_one_cqe indeed > > can avoid this problem. This part needs further > > consideration on how to handle it. > > > > ufshcd_abort() > > ufshcd_mcq_abort() > > ufshcd_try_to_abort_task()// will trigger ISR > > ufshcd_release_scsi_cmd() > > > > ufs_mtk_mcq_intr() > > ufshcd_mcq_poll_cqe_lock() > > ufshcd_mcq_process_cqe() > > ufshcd_compl_one_cqe() > > cmd->result = DID_ABORT > > ufshcd_release_scsi_cmd() // will release twice > > scsi_done() > > Do you agree that this case can be addressed with the > ufshcd_compl_one_cqe() change proposed above? > Agree. > > ---------------------------------------------------------------- > > > > Case3: DBR ufshcd_err_handler > > > > In the case of the DBR mode error handler, it's the same; > > ufshcd_release_scsi_cmd will also be executed, and scsi_done > > will definitely be used to notify SCSI to requeue. > > > > ufshcd_err_handler() > > ufshcd_abort_all() > > ufshcd_abort_one() > > ufshcd_try_to_abort_task()// It should trigger an > > interrupt, but the tensor might not > > ufshcd_complete_requests() > > ufshcd_transfer_req_compl() > > ufshcd_poll() > > get outstanding_lock > > clear outstanding_reqs tag > > release outstanding_lock > > __ufshcd_transfer_req_compl() > > ufshcd_compl_one_cqe() > > cmd->result = DID_REQUEUE // mediatek may need quirk > > change DID_ABORT to DID_REQUEUE > > ufshcd_release_scsi_cmd() > > scsi_done() > > > > ufshcd_intr() > > ufshcd_sl_intr() > > ufshcd_transfer_req_compl() > > ufshcd_poll() > > get outstanding_lock > > clear outstanding_reqs tag > > release outstanding_lock > > __ufshcd_transfer_req_compl() > > ufshcd_compl_one_cqe() > > cmd->result = DID_REQUEUE // mediatek may need quirk > change > > DID_ABORT to DID_REQUEUE > > ufshcd_release_scsi_cmd() > > scsi_done(); > > > > At this time, the same actions are taken regardless of whether > > there is an ISR, and with the protection of outstanding_lock, > > only one thread will execute ufshcd_release_scsi_cmd and scsi_done. > > ---------------------------------------------------------------- > > > > Case4: MCQ ufshcd_err_handler > > > > It's the same with MCQ mode; there is protection from the cqe lock, > > so only one thread will execute. What my patch 2 aims to do is to > > change DID_ABORT to DID_REQUEUE in this situation. > > > > ufshcd_err_handler() > > ufshcd_abort_all() > > ufshcd_abort_one() > > ufshcd_try_to_abort_task()// will trigger irq thread > > ufshcd_complete_requests() > > ufshcd_mcq_compl_pending_transfer() > > ufshcd_mcq_poll_cqe_lock() > > ufshcd_mcq_process_cqe() > > ufshcd_compl_one_cqe() > > cmd->result = DID_ABORT // should change to > DID_REQUEUE > > ufshcd_release_scsi_cmd() > > scsi_done() > > > > ufs_mtk_mcq_intr() > > ufshcd_mcq_poll_cqe_lock() > > ufshcd_mcq_process_cqe() > > ufshcd_compl_one_cqe() > > cmd->result = DID_ABORT // should change to DID_REQUEUE > > ufshcd_release_scsi_cmd() > > scsi_done() > > For legacy and MCQ mode, I prefer the following behavior for > ufshcd_abort_all(): > * ufshcd_compl_one_cqe() ignores commands with status OCS_ABORTED. > * ufshcd_release_scsi_cmd() is called either by ufshcd_abort_one() or > by ufshcd_abort_all(). > > Do you agree with making the changes proposed above? > > Thank you, This might not work, as SDB mode doesn't ignore OCS: INVALID_OCS_VALUE but rather notifies SCSI to requeue. So what we need to correct is to notify SCSI to requeue when MCQ mode receives OCS: ABORTED as well. Furthermore, ufshcd_compl_one_cqe, whether it comes from ufshcd_abort_all or ISR, does the same thing and is protected by a lock. Therefore, there is no need for special handling specifically for ufshcd_abort_all. After discussing with you, I realized that there are indeed many deficiencies and inconsistencies here that need to be addressed. I will submit a new patch for the content discussed above. Thanks. Peter > > Bar
