Re: [PATCH 1/3] ufs: core: Simplify timeout handling for START STOP UNIT commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing DID_REQUEUE with DID_TIME_OUT for START STOP UNIT commands inadvertently routes them to the SCSI Error Handler, risking deadlock and breaking PM recovery.
--

--- Patch [1]: [PATCH 1/3] ufs: core: Simplify timeout handling for START STOP UNIT commands ---
commit 457c19a7449f41757fe8a8fc3aa57e02dd255499
Author: Bart Van Assche <bvanassche@acm.org>
    
ufs: core: Simplify timeout handling for START STOP UNIT commands
    
This commit simplifies timeout handling for START STOP UNIT commands by failing
them directly instead of requeuing them. It updates the ufshcd_eh_timed_out
handler to use DID_TIME_OUT rather than DID_REQUEUE for SCSI commands.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9572,13 +9572,9 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  			spin_unlock_irqrestore(&hba->outstanding_lock, flags);
>  		}
>  
> -		if (ufshcd_is_scsi_cmd(scmd)) {
> -			set_host_byte(scmd, DID_REQUEUE);
> +		set_host_byte(scmd, DID_TIME_OUT);

[Severity: High]
Will this inadvertently route START STOP UNIT commands to the SCSI Error
Handler and risk deadlocks during PM recovery?

Looking at scsi_decide_disposition(), setting DID_TIME_OUT for commands other
than INQUIRY or TEST_UNIT_READY (such as START STOP UNIT, opcode 0x1B)
evaluates to FAILED. The SCSI core then proceeds to call scsi_eh_scmd_add(),
which flags the host for SHOST_RECOVERY and wakes the SCSI Error Handler
thread.

Waking the EH thread during PM transitions appears to break the UFS
subsystem's custom inline recovery and reintroduces the deadlocks that commit
01d5e237b339 originally resolved.

If the goal is to fail the command directly without retries and without
invoking the EH, would an error status that evaluates to SUCCESS in
scsi_decide_disposition() (such as DID_BAD_TARGET) be more appropriate here?

> +		if (ufshcd_is_scsi_cmd(scmd))
>  			ufshcd_release_scsi_cmd(hba, scmd);
> -		} else {
> -			set_host_byte(scmd, DID_TIME_OUT);
> -		}
> -
>  		scsi_done(scmd);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786558726.git.bvanassche@acm.org?part=1