Re: [PATCH] scsi: ufs: core: Retry the full device init on fatal link loss

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ufs: core: uncollected eh_work and sticky saved_err in ufshcd_init()
- [High] scsi: ufs: core: missing ufshcd_mcq_enable() on retry in ufshcd_init()
--

commit dd93c52494a126db3d5dec49c3f84c857b512392
Author: Shawn Lin <shawn.lin@rock-chips.com>

scsi: ufs: core: Retry the full device init on fatal link loss

This commit adds a retry loop in ufshcd_init() that resets the UFS device
and host controller up to three times with a 100 ms delay if link startup
or device initialization fails during boot.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ee643830a57a2..e75370d1b9501 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11419,12 +11419,14 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  
>  	/* Initialize hba, detect and initialize UFS device */
>  	ktime_t probe_start = ktime_get();
> +	int retries = 3;
>  
> +retry_init:
>  	hba->ufshcd_state = UFSHCD_STATE_RESET;
>  
>  	err = ufshcd_link_startup(hba);
>  	if (err)
> -		goto out_release;
> +		goto retry_device_init;
>  
>  	if (hba->mcq_enabled)
>  		ufshcd_config_mcq(hba);

[Severity: High]
On controllers with MCQ enabled, does retrying after ufshcd_hba_stop() and
ufshcd_hba_enable() leave the hardware in legacy single doorbell mode while
hba->mcq_enabled remains true?

Before the retry_init label in drivers/ufs/core/ufshcd.c:ufshcd_init(),
ufshcd_add_scsi_host() enables MCQ by calling ufshcd_mcq_enable():

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_enable() {
	ufshcd_rmwl(hba, MCQ_MODE_SELECT, MCQ_MODE_SELECT, REG_UFS_MEM_CFG);
	hba->mcq_enabled = true;
}

When initialization fails and enters retry_device_init, calling
ufshcd_hba_stop() and ufshcd_hba_enable() toggles REG_CONTROLLER_ENABLE,
which resets the host controller registers including MCQ_MODE_SELECT in
REG_UFS_MEM_CFG while hba->mcq_enabled stays true.

When jumping back to retry_init, ufshcd_init() calls ufshcd_config_mcq(hba)
without calling ufshcd_mcq_enable(hba), unlike ufshcd_device_init() on
subsequent resets:

drivers/ufs/core/ufshcd.c:ufshcd_device_init() {
	/* Reconfigure MCQ upon reset */
	if (hba->mcq_enabled && !init_dev_params) {
		ufshcd_config_mcq(hba);
		ufshcd_mcq_enable(hba);
	}
}

Without re-enabling MCQ_MODE_SELECT in REG_UFS_MEM_CFG, won't subsequent
device management commands (starting with NOP OUT in
ufshcd_verify_dev_init()) be posted to the MCQ submission queues while the
hardware ignores them and times out?

[ ... ]

> @@ -11468,6 +11470,25 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>  	ufshcd_pm_qos_init(hba);
>  	return 0;
>  
> +retry_device_init:
> +	if (retries--) {
> +		dev_warn(hba->dev, "UFS init failed %d, full retry (%d left)\n",
> +			 err, retries);
> +
> +		ufshcd_device_reset(hba);
> +		ufs_put_device_desc(hba);
> +		ufshcd_hba_stop(hba);
> +		if (ufshcd_hba_enable(hba)) {
> +			dev_err(hba->dev, "Host controller enable failed\n");
> +			ufshcd_print_evt_hist(hba);
> +			ufshcd_print_host_state(hba);
> +			goto out_release;
> +		}
> +
> +		msleep(100);
> +		goto retry_init;
> +	}

[Severity: High]
What happens to the scheduled hba->eh_work and the latched error bits in
hba->saved_err and hba->saved_uic_err when retry_device_init loops back to
retry_init after a fatal error interrupt?

When DEVICE_FATAL_ERROR | UIC_LINK_LOST (0x880) fires during the failed
attempt, ufshcd_check_errors() latches the error into hba->saved_err and
hba->saved_uic_err, sets hba->ufshcd_state to
UFSHCD_STATE_EH_SCHEDULED_FATAL, and queues hba->eh_work:

drivers/ufs/core/ufshcd.c:ufshcd_check_errors() {
		hba->saved_err |= hba->errors;
		hba->saved_uic_err |= hba->uic_error;
		...
		ufshcd_schedule_eh_work(hba);
}

Because hba->ufs_device_wlun is still NULL during ufshcd_init() (until
ufshcd_async_scan() runs later), ufshcd_err_handling_should_stop(hba)
returns true when ufshcd_err_handler() executes:

drivers/ufs/core/ufshcd.c:ufshcd_err_handler() {
	down(&hba->host_sem);
	spin_lock_irqsave(&hba->host->host_lock, flags);
	if (ufshcd_err_handling_should_stop(hba)) {
		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
		spin_unlock_irqrestore(&hba->host->host_lock, flags);
		up(&hba->host_sem);
		return;
	}
}

Since retry_device_init does not flush hba->eh_work or clear hba->saved_err,
hba->saved_uic_err, and hba->force_reset under hba->host->host_lock (as
ufshcd_reset_and_restore() does), two issues appear possible:

1. If hba->eh_work runs concurrently with the retry, ufshcd_err_handler()
can overwrite hba->ufshcd_state with UFSHCD_STATE_OPERATIONAL after
retry_init sets it to UFSHCD_STATE_RESET.

2. Even after a retry succeeds, hba->saved_err retains the sticky
DEVICE_FATAL_ERROR | UIC_LINK_LOST bits. On any later non-fatal UIC event
(such as a line reset during power mode configuration in
ufshcd_post_device_init() or during normal runtime),
ufshcd_check_errors() checks ufshcd_is_saved_err_fatal(hba), misclassifies
the event as a fatal error, and either leaves hba->ufshcd_state in
UFSHCD_STATE_EH_SCHEDULED_FATAL across ufshcd_process_probe_result() or
triggers an unnecessary full reset once hba->ufs_device_wlun is added.

Should retry_device_init flush hba->eh_work and clear hba->saved_err,
hba->saved_uic_err, and hba->force_reset under host_lock before retrying?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028073129.1661829-1-shawn.lin@rock-chips.com?part=1