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.
