Re: [PATCH v9 2/3] ufs: core: fix error handler process for MCQ abort

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

On Mon, 2024-09-30 at 10:25 -0700, Bart Van Assche wrote:
> 

> Hi Peter,
> 
> The purpose of the SCMD_STATE_COMPLETE bit is to prevent that the
> SCSI
> core tries to abort a SCSI command concurrently with the SCSI LLD
> (UFS
> driver in this case) calling scsi_done(). Making scsi_done() calls
> no-
> ops while an .eh_abort_handler() implementation is in progress is an
> undocumented side effect of this mechanism. But since it is unlikely
> that this behavior will change in the SCSI core, I'm OK with relying
> on
> this behavior.

Hi Bart,

Yes, the SCMD_STATE_COMPLETE bit is there to protect against
a scsi command timeout and finish occurring simultaneously. 
Regardless of whether the timeout or finish happens first, 
the other will not proceed with its subsequent actions. 
Therefore, it is within expectations that scsi_done will 
not perform any actions after SCSI notified that the command 
has already been timeout aborted.


> 
> ufshcd_abort_one() does not set the SCMD_STATE_COMPLETE bit before it
> tries to abort a SCSI command. I think this is wrong because plenty
> of
> code in ufshcd_try_to_abort_task() relies on the assumption that
> scsi_done() is not called while that function is in progress. Do you
> agree that SCMD_STATE_COMPLETE should be set by ufshcd_abort_one()
> before it calls ufshcd_try_to_abort_task()? If this change is made, a
> consequence is that the completion handler won't inform the SCSI core
> anymore that abortion of a command by ufshcd_abort_one() has
> completed.
> Hence, the cmd->result value won't matter anymore for commands
> aborted
> by ufshcd_abort_one() and how ufshcd_transfer_rsp_status() translates
> OCS_ABORTED won't matter anymore either.
> 
> Thanks,
> 

SCMD_STATE_COMPLETE should not be set outside of the SCSI 
core. It is reasonable to set it through scsi_done(), as 
setting this flag also requires notifying the block layer 
on how to handle the completed command. If the UFS driver 
sets this flag, the driver would have to bypass the SCSI 
layer and directly notify the block layer, which is clearly 
not a reasonable situation. Additionally, when a command 
completes, the SCSI layer needs to determine how to handle 
it based on the result. This is obviously not something 
the UFS driver layer can handle on its own. Notifying 
the SCSI layer through the result on how to proceed 
is what the UFS driver should do.


Furthermore, ufshcd_try_to_abort_task() is responsible for 
comunication between the UFS host controller and the UFS 
device, unrelated to the SCSI layer. Notifying SCSI layter 
based on the outcome of ufshcd_try_to_abort_task() should 
be the correct and reasonable approach, as has always been 
the case in SDB mode. The current difference is only that 
the OCS and result in SDB mode and MCQ is differ. Aligning 
both to the same result would likely be a more reasonable 
approach, wouldn't it?"

Thanks.
Peter



> Bart.