Re: [PATCH v4 2/2] ufs: core: requeue aborted request

[Bart Van Assche <bvanassche@acm.org>]

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.