RE: [PATCH v2] scsi: ufs: core: handle PM SSU timeout before SCSI EH

[Fang Hongjie(方洪杰)]

> From: Bart Van Assche [mailto:bvanassche@xxxxxxx]
> Sent: Saturday, May 30, 2026 5:46 AM
> To: Fang Hongjie(方洪杰) <hongjiefang@xxxxxxxxxxxx>;
> alim.akhtar@xxxxxxxxxxx; avri.altman@xxxxxxx;
> James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx;
> peter.wang@xxxxxxxxxxxx; beanhuo@xxxxxxxxxx
> Cc: linux-scsi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] scsi: ufs: core: handle PM SSU timeout before SCSI
> EH
> 
> On 5/28/26 4:34 AM, Hongjie Fang wrote:
> > @@ -9465,23 +9498,30 @@ static void ufshcd_async_scan(void *data,
> async_cookie_t cookie)
> >   static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd
> *scmd)
> >   {
> >   	struct ufs_hba *hba = shost_priv(scmd->device->host);
> > +	int ret;
> >
> > -	if (!hba->system_suspending) {
> > +	if (!hba->pm_op_in_progress || scmd->device != hba-
> >ufs_device_wlun ||
> > +	    scmd->cmnd[0] != START_STOP) {
> >   		/* Activate the error handler in the SCSI core. */
> >   		return SCSI_EH_NOT_HANDLED;
> >   	}
> 
> Please don't make the code any more complex than necessary. If a power
> management operation is in progress it is guaranteed that no other SCSI
> commands are in progress. See also blk_pre_runtime_suspend() and
> blk_try_enter_queue(). Hence, for the above if-condition, testing
> hba->pm_op_in_progress is sufficient.
> 

I agree with your comments. It will only check whether a PM operation is
in progress for normal SCSI commands:
    if (!hba->pm_op_in_progress || !ufshcd_is_scsi_cmd(scmd))
            return SCSI_EH_NOT_HANDLED;
			
The reason for keeping ufshcd_is_scsi_cmd() is to avoid applying this
special PM timeout path to UFS internal device-management commands.


> >   	/*
> > -	 * If we get here we know that no TMFs are outstanding and also that
> > -	 * the only pending command is a START STOP UNIT command.
> Handle the
> > -	 * timeout of that command directly to prevent a deadlock between
> > -	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
> > +	 * PM START STOP UNIT commands are issued while a PM operation
> is in
> > +	 * progress. Handle such timeouts directly to avoid entering regular
> > +	 * SCSI EH, which may deadlock with the PM operation and may also
> make
> > +	 * scsi_execute_cmd() retries fail while the host is still in recovery.
> >   	 */
> 
> The original comment is fine, isn't it?
> 
> > -	return scsi_host_busy(hba->host) ? SCSI_EH_RESET_TIMER :
> SCSI_EH_DONE;
> > +	if (ret)
> > +		return SCSI_EH_NOT_HANDLED;
> 
> This is wrong because it may activate the SCSI error handler for a START
> STOP UNIT command. We don't want this - we want the error to be
> propagated to the scsi_execute_cmd() caller.
> 
> > +	WARN_ON_ONCE(!test_bit(SCMD_STATE_COMPLETE, &scmd-
> >state));
> 
> This is also wrong. According to Documentation/process/coding-style.rst,
> WARN*() should only be used for this-should-never-happen situations. If
> the above statement is reached it is almost a certainty that
> SCMD_STATE_COMPLETE is not set.
> 

It will keep the original comment in ufshcd_eh_timed_out(), remove the
SCSI_EH_NOT_HANDLED fallback after link recovery, and remove the WARN_ON
as suggested.


> The rest of this patch looks good to me.
> 
> Thanks,
> 
> Bart.


Best.