On 9/18/24 6:29 AM, Peter Wang (王信友) wrote:
Basically, this patch currently only needs to handle requeueing for the error handler abort. The approach for DBR mode and MCQ mode should be consistent. If receive an interrupt response (OCS:ABORTED or INVALID_OCS_VALUE), then set DID_REQUEUE. If there is no interrupt, it will also set SCSI DID_REQUEUE in ufshcd_err_handler through ufshcd_complete_requests with force_compl = true.
Reporting a completion for commands cleared by writing into the legacy UTRLCLR register is not compliant with any version of the UFSHCI standard. Reporting a completion for commands cleared by writing into that register is problematic because it causes ufshcd_release_scsi_cmd() to be called as follows:
ufshcd_sl_intr() ufshcd_transfer_req_compl() ufshcd_poll() __ufshcd_transfer_req_compl() ufshcd_compl_one_cqe() cmd->result = ... ufshcd_release_scsi_cmd() scsi_done()
Calling ufshcd_release_scsi_cmd() if a command has been cleared is problematic because the SCSI core does not expect this. If ufshcd_try_to_abort_task() clears a SCSI command, ufshcd_release_scsi_cmd() must not be called until the SCSI core decides to release the command. This is why I wrote in a previous mail that I think that a quirk should be introduced to suppress the completions generated by clearing a SCSI command.
The more problematic part is with MCQ mode. To imitate the DBR approach, we just need to set DID_REQUEUE upon receiving an interrupt. Everything else remains the same. This would make things simpler.
Moving forward, if we want to simplify things and we have also taken stock of the two or three scenarios where OCS: ABORTED occurs, do we even need a flag? Couldn't we just set DID_REQUEUE directly for OCS: ABORTED? What do you think?
How about making ufshcd_compl_one_cqe() skip entries with status OCS_ABORTED? That would make ufshcd_compl_one_cqe() behave as the SCSI core expects, namely not freeing any command resources if a SCSI command is aborted successfully.
This approach may require further changes to ufshcd_abort_all(). In that function there are separate code paths for legacy and MCQ mode. This is less than ideal. Would it be possible to combine these code paths by removing the ufshcd_complete_requests() call from ufshcd_abort_all() and by handling completions from inside ufshcd_abort_one()?
Thanks,
Bart.