Re: [PATCH 2/9] scsi: ufs: core: Defer link recovery on reserved PM command timeout

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

On 11/11/26 9:15 PM, Stanley Jhu wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e9dcab35a4cd..6a5f5fa732d9 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -7147,7 +7147,7 @@ static void ufshcd_err_handler(struct work_struct *work)
>   	spin_unlock_irqrestore(&hba->host->host_lock, flags);
>   
>   	/* Complete requests that have door-bell cleared by h/w */
> -	if (!stop)
> +	if (!stop && ufshcd_is_hba_active(hba))
>   		ufshcd_complete_requests(hba, false);
>   	spin_lock_irqsave(&hba->host->host_lock, flags);
>   again:
> @@ -9689,6 +9689,50 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>   		dev_err(hba->dev, "%s failed: %d\n", __func__, ret);
>   }
>   
> +/*
> + * Claim @scmd before stopping the HBA in case normal completion raced with
> + * scsi_timeout(), then complete @scmd so the PM caller releases
> + * hba->dev_cmd.lock and the reserved tag before error recovery runs.
> + */
> +static void ufshcd_eh_pm_cmd_timed_out(struct ufs_hba *hba,
> +				       struct scsi_cmnd *scmd)
> +{
> +	struct request *rq = scsi_cmd_to_rq(scmd);
> +	struct ufs_hw_queue *hwq;
> +
> +	if (!hba->mcq_enabled) {
> +		if (!ufshcd_clear_outstanding_reqs(hba, BIT(rq->tag)))
> +			return;
> +		ufshcd_hba_stop(hba);
> +		ufshcd_force_error_recovery(hba);
> +		set_host_byte(scmd, DID_TIME_OUT);
> +		scsi_done(scmd);
> +		return;
> +	}
> +
> +	hwq = ufshcd_mcq_req_to_hwq(hba, rq);
> +	if (ufshcd_is_hba_active(hba))
> +		ufshcd_mcq_poll_cqe_lock(hba, hwq);
> +	scoped_guard(spinlock_irqsave, &hwq->cq_lock) {
> +		if (test_bit(SCMD_STATE_COMPLETE, &scmd->state))
> +			return;
> +	}
> +	spin_lock_irq(&hba->host->host_lock);
> +	hba->force_reset = true;
> +	if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
> +		hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED_FATAL;
> +	spin_unlock_irq(&hba->host->host_lock);
> +	ufshcd_hba_stop(hba);
> +	ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
> +	ufshcd_force_error_recovery(hba);
> +	scoped_guard(spinlock_irqsave, &hwq->cq_lock) {
> +		if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) {
> +			set_host_byte(scmd, DID_TIME_OUT);
> +			scsi_done(scmd);
> +		}
> +	}
> +}
> +
>   static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>   {
>   	struct ufs_hba *hba = shost_priv(scmd->device->host);
> @@ -9698,6 +9742,11 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>   		return SCSI_EH_NOT_HANDLED;
>   	}
>   
> +	if (!ufshcd_is_scsi_cmd(scmd)) {
> +		ufshcd_eh_pm_cmd_timed_out(hba, scmd);
> +		return SCSI_EH_DONE;
> +	}
> +
>   	/*
>   	 * Handle the timeout directly to prevent a deadlock between
>   	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().

I don't like the approach of this patch series. I think this series 
changes much more code than what is necessary. There is a much simpler
way to fix the issue reported in the cover letter: instead of calling
ufshcd_link_recovery(hba) from ufshcd_eh_timed_out(), remove that call
from ufshcd_eh_timed_out() and instead call ufshcd_link_recovery(hba)
from ufshcd_set_dev_pwr_mode() if the SCSI error code returned by
ufshcd_execute_start_stop() indicates that the SCSI START STOP UNIT
command has timed out.

Thanks,

Bart.