Re: [PATCH v3 2/2] 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 4 potential issue(s) to consider:
- [High] scsi: ufs: core: unpropagated link loss in ufshcd_device_params_init()
- [High] scsi: ufs: core: stale outstanding_reqs and req_abort_skip across init retries
- [Medium] scsi: ufs: core: ufshcd_wb_probe() clears UFSHCD_CAP_WB_EN across retries
- [Medium] scsi: ufs: core: stale max_pwr_info.is_valid retained across init retries
--

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

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

This commit retries the full UFS device initialization sequence up to three
times in ufshcd_init() when initialization fails due to fatal link loss at
boot. Between retries, it stops the controller, cancels the error handler,
resets the device and controller state, and waits 100 ms before trying
again.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8a8083a8f024e..12ada46d2b7e1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11476,8 +11476,65 @@ 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;
>  
> -	err = ufshcd_full_device_init(hba);
> +	while (retries--) {
> +		unsigned long flags;
> +
> +		err = ufshcd_full_device_init(hba);
> +		if (!err)
> +			break;

[Severity: High]
Can ufshcd_full_device_init() return 0 even when DEVICE_FATAL_ERROR |
UIC_LINK_LOST is latched and the following UIC query fails with -EIO,
causing this loop to break out on the first attempt without retrying?

In ufshcd_full_device_init() -> ufshcd_device_params_init(), once
ufshcd_device_geo_params_init() and the initial descriptor reads in
ufs_get_device_desc() succeed, the remaining queries in ufs_get_device_desc()
(such as ufshcd_wb_probe(), ufshcd_temp_notif_probe(), ufshcd_enable_ee(),
ufs_init_rtc(), and ufshcd_device_lvl_exception_probe()) and in
ufshcd_device_params_init() (ufshcd_set_rtt(),
ufshcd_get_ref_clk_gating_wait(), and QUERY_FLAG_IDN_PWR_ON_WPE) ignore errors.

If DEVICE_FATAL_ERROR | UIC_LINK_LOST is latched during that window,
ufshcd_check_errors() latches hba->saved_err and hba->saved_uic_err, and the
first UIC DME_GET query in ufshcd_get_max_pwr_mode() fails with -EIO.
However, ufshcd_device_params_init() ignores the failure of
ufshcd_get_max_pwr_mode() and still returns 0:

drivers/ufs/core/ufshcd.c:ufshcd_device_params_init() {
	...
	/* Probe maximum power mode co-supported by both UFS host and device */
	if (ufshcd_get_max_pwr_mode(hba))
		dev_err(hba->dev,
			"%s: Failed getting max supported power mode\n",
			__func__);

	return 0;
}

Then ufshcd_post_device_init() sees that hba->max_pwr_info.is_valid is false
and also returns 0 without calling ufshcd_config_pwr_mode():

drivers/ufs/core/ufshcd.c:ufshcd_post_device_init() {
	...
	if (!hba->max_pwr_info.is_valid)
		return 0;
	...
}

Should ufshcd_device_params_init() propagate the error from
ufshcd_get_max_pwr_mode(), or should the retry check also inspect
hba->saved_err, hba->saved_uic_err, or ufshcd_is_link_broken(hba)?

> +
> +		if (!retries)
> +			break;
> +
> +		dev_warn(hba->dev, "UFS init failed %d, full retry (%d left)\n",
> +			 err, retries);
> +
> +		/*
> +		 * The failed attempt may have latched fatal errors and
> +		 * scheduled the error handler. Stop the controller first
> +		 * so that no interrupt can interfere with the cleanup,
> +		 * then drain the error handler and start the retry from a
> +		 * clean state.
> +		 */
> +		ufshcd_hba_stop(hba);
> +		cancel_work_sync(&hba->eh_work);
> +		spin_lock_irqsave(&hba->host->host_lock, flags);
> +		hba->saved_err = 0;
> +		hba->saved_uic_err = 0;
> +		hba->force_reset = false;
> +		hba->ufshcd_state = UFSHCD_STATE_RESET;
> +		spin_unlock_irqrestore(&hba->host->host_lock, flags);

[Severity: High]
If a device management command (NOP OUT or Query) times out on a wedged
controller during ufshcd_full_device_init(), can stale state in
hba->outstanding_reqs and lrbp->req_abort_skip survive into the next retry?

When blk_execute_rq() times out in ufshcd_exec_dev_cmd() ->
ufshcd_issue_dev_cmd(), scsi_timeout() calls ufshcd_eh_timed_out() ->
ufshcd_abort(). If ufshcd_clear_cmd() times out because the controller is
wedged, ufshcd_abort() sets lrbp->req_abort_skip on the reserved command slot
and leaves BIT(tag) set in hba->outstanding_reqs:

drivers/ufs/core/ufshcd.c:ufshcd_abort() {
	...
	if (blk_mq_is_reserved_rq(rq))
		err = ufshcd_clear_cmd(hba, tag);
	else
		err = ufshcd_try_to_abort_task(hba, tag);
	if (err) {
		dev_err(hba->dev, "%s: failed with err %d\n", __func__, err);
		ufshcd_set_req_abort_skip(hba, hba->outstanding_reqs);
		err = FAILED;
		goto release;
	}
	...
}

SCSI EH then invokes ufshcd_eh_host_reset_handler(), which schedules and
flushes hba->eh_work. However, because hba->ufs_device_wlun is still NULL
during ufshcd_init(), ufshcd_err_handling_should_stop() returns true:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_should_stop() {
	return (!hba->is_powered || hba->shutting_down ||
		!hba->ufs_device_wlun ||
		...
}

As a result, ufshcd_err_handler() exits early without calling
ufshcd_complete_requests() or clearing hba->outstanding_reqs, while
ufshcd_eh_host_reset_handler() sees UFSHCD_STATE_OPERATIONAL and completes the
timed-out request.

When the retry loop then stops and re-enables the controller (clearing the
hardware doorbell register) without clearing hba->outstanding_reqs or
resetting lrbp->req_abort_skip (which ufshcd_setup_dev_cmd() does not clear),
won't ufshcd_poll() on the next retry observe the cleared doorbell bit against
the stale bit in hba->outstanding_reqs and trigger a spurious or premature
completion on the reserved slot?

> +
> +		ufshcd_device_reset(hba);
> +		ufs_put_device_desc(hba);

[Severity: Medium]
Does the retry cleanup also need to reset hba->max_pwr_info.is_valid and
restore UFSHCD_CAP_WB_EN in hba->caps?

First, if a failed attempt reaches ufshcd_get_max_pwr_mode(), it sets
hba->max_pwr_info.is_valid = true before ufshcd_post_device_init() ->
ufshcd_config_pwr_mode() fails (or if a link error mid-way through
ufshcd_get_max_pwr_mode() causes a partial fallback in hba->max_pwr_info.info).
On the next retry, ufshcd_get_max_pwr_mode() will see:

drivers/ufs/core/ufshcd.c:ufshcd_get_max_pwr_mode() {
	...
	if (hba->max_pwr_info.is_valid)
		return 0;
	...
}

and skip probing the negotiated power mode parameters after the reset.

Second, if a query fails inside ufshcd_wb_probe() during a failed attempt
(such as ufshcd_read_unit_desc_param() or
ufshcd_is_wb_buf_lifetime_available()), it jumps to wb_disabled and clears
UFSHCD_CAP_WB_EN:

drivers/ufs/core/ufshcd.c:ufshcd_wb_probe() {
	...
wb_disabled:
	hba->caps &= ~UFSHCD_CAP_WB_EN;
}

On subsequent retries, ufshcd_is_wb_allowed(hba) will be false and WriteBooster
will remain permanently disabled even if the retry succeeds.

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