Re: [PATCH v2 1/2] scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] scsi: ufs: core: START STOP retry on stopped HBA in
  ufshcd_eh_timed_out()
- [Medium] scsi: ufs: core: TOCTOU race on ufshcd_is_hba_active() after
  timeout
--

commit 904c924ede89828a6af258eb088b07dc5e864450
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Recover link in ufshcd_set_dev_pwr_mode() on timeout

This commit moves link recovery on power management command timeouts out of
ufshcd_eh_timed_out() into ufshcd_set_dev_pwr_mode() and
ufshcd_err_handler(), stopping the host controller and failing START STOP
UNIT with DID_TRANSPORT_FAILFAST on timeout.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ddc953b45ce95..0f5365bd90f04 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -9660,17 +9664,20 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  	}
>  
>  	/*
> -	 * Handle the timeout directly to prevent a deadlock between
> -	 * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
> +	 * Stop the host controller so that @scmd can be completed here.
> +	 * ufshcd_set_dev_pwr_mode() recovers the link for START STOP
> +	 * UNIT. ufshcd_err_handler() recovers it for other commands.
>  	 */
> -	ufshcd_link_recovery(hba);
> -	dev_info(hba->dev, "%s() finished; outstanding_tasks = %#lx.\n",
> -		 __func__, hba->outstanding_tasks);
> +	ufshcd_hba_stop(hba);
> +	if (!ufshcd_is_scsi_cmd(scmd))
> +		ufshcd_force_error_recovery(hba);
> +	if (hba->mcq_enabled) {
> +		struct ufs_hw_queue *hwq =
> +			ufshcd_mcq_req_to_hwq(hba, scsi_cmd_to_rq(scmd));
> +
> +		ufshcd_mcq_compl_all_cqes_lock(hba, hwq);
> +	}
>  
> -	/*
> -	 * ufshcd_link_recovery() may already have completed @scmd, e.g. via
> -	 * the existing MCQ force-completion path.
> -	 */
>  	if (!test_bit(SCMD_STATE_COMPLETE, &scmd->state)) {
>  		if (!hba->mcq_enabled) {
>  			struct request *rq = scsi_cmd_to_rq(scmd);
> @@ -9680,7 +9687,7 @@ static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>  		}
>  
>  		if (ufshcd_is_scsi_cmd(scmd)) {
> -			set_host_byte(scmd, DID_REQUEUE);
> +			set_host_byte(scmd, DID_TRANSPORT_FAILFAST);
>  			ufshcd_release_scsi_cmd(hba, scmd);
>  		} else {
>  			set_host_byte(scmd, DID_TIME_OUT);
> @@ -10137,6 +10144,9 @@ static int ufshcd_execute_start_stop(struct scsi_device *sdev,
>  {
>  	const unsigned char cdb[6] = { START_STOP, 0, 0, 0, pwr_mode << 4, 0 };
>  	struct scsi_failure failure_defs[] = {
> +		{
> +			.result = DID_TRANSPORT_FAILFAST << 16,
> +		},
>  		{
>  			.allowed = 2,
>  			.result = SCMD_FAILURE_RESULT_ANY,

[Severity: Medium]
Can scsi_execute_cmd() retry START STOP UNIT while the controller is
already stopped by ufshcd_hba_stop()?

If ufshcd_mcq_compl_all_cqes_lock() (or a completion interrupt just before
ufshcd_hba_stop()) completes scmd with a non-zero status such as
SAM_STAT_CHECK_CONDITION, SCMD_STATE_COMPLETE is set and
ufshcd_eh_timed_out() skips setting DID_TRANSPORT_FAILFAST.
failure_defs[] then matches SCMD_FAILURE_RESULT_ANY, so scsi_execute_cmd()
submits a retry while HCE is 0 and hba->ufshcd_state is still
UFSHCD_STATE_OPERATIONAL, writing to the doorbell or SQTP of a disabled
controller and stalling for another 10-second timeout.

In addition, if scmd completes with result 0 during ufshcd_eh_timed_out()
on a host with UFSHCI_QUIRK_BROKEN_HCE, both fenced and
!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) are false in
ufshcd_set_dev_pwr_mode(), so ufshcd_link_recovery() is skipped even though
ufshcd_hba_stop() disabled the controller.

> @@ -10172,6 +10182,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_device *sdp;
>  	unsigned long flags;
> +	bool fenced;
>  	int ret;
>  
>  	spin_lock_irqsave(&hba->host->host_lock, flags);
> @@ -10199,6 +10210,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  	 * already suspended childs.
>  	 */
>  	ret = ufshcd_execute_start_stop(sdp, pwr_mode, &sshdr);
> +	fenced = ret > 0 && host_byte(ret) == DID_TRANSPORT_FAILFAST;
>  	if (ret) {
>  		sdev_printk(KERN_WARNING, sdp,
>  			    "START_STOP failed for power mode: %d, result %x\n",
> @@ -10212,6 +10224,13 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
>  		hba->curr_dev_pwr_mode = pwr_mode;
>  	}
>  
> +	if (fenced || (!(hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) &&
> +		       !ufshcd_is_hba_active(hba))) {
> +		ret = ufshcd_link_recovery(hba);
> +		if (!ret && hba->curr_dev_pwr_mode != pwr_mode)
> +			ret = -EIO;
> +	}
> +
>  	scsi_device_put(sdp);
>  	WRITE_ONCE(hba->host->eh_noresume, 0);
>  	return ret;

[Severity: Medium]
Can ufshcd_set_dev_pwr_mode() check ufshcd_is_hba_active() before a racing
ufshcd_eh_timed_out() finishes stopping the controller?

CPU0 (IRQ)               CPU1 (timeout)           CPU2 (PM caller)
ufshcd_intr()            ufshcd_eh_timed_out()
  ...                      ufshcd_hba_stop()
    scsi_done(scmd)          ufshcd_disable_irq()
                                                  ufshcd_execute_start_stop()
                                                    returns 0
                                                  ufshcd_is_hba_active(hba)
                                                    --> true (recovery skipped)
                             writel(CONTROLLER_DISABLE,
                                    REG_CONTROLLER_ENABLE)
                               <-- HBA left disabled

When a normal completion interrupt races with request timeout expiration,
scsi_done() wakes scsi_execute_cmd() on CPU2 while CPU1 is still in
ufshcd_disable_irq() inside ufshcd_hba_stop().  CPU2 sees ret == 0 and
ufshcd_is_hba_active(hba) == true, so it skips ufshcd_link_recovery(), and
CPU1 then clears HCE and returns SCSI_EH_DONE because SCMD_STATE_COMPLETE
is already set, leaving the controller disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113111202.3551341-1-stanleyjhu@google.com?part=1