[PATCH v3 0/2] Retry the full device init on fatal link loss

[PATCH v3 0/2] Retry the full device init on fatal link loss

[Shawn Lin]

On an RK3572 board the UFS device is sometimes reported dead during the
initialization at boot: DEVICE_FATAL_ERROR | UIC_LINK_LOST is latched and
the following UIC query fails with -EIO, leaving the device unusable.

  [    2.784068][    C0] ufshcd-rockchip 2a2d0000.ufs: ufshcd_check_errors: saved_err 0x880 saved_uic_err 0x0
  [    2.784112][    C0] host_regs: 00000000: 0187011f 00000000 00000200 00000000
  [    2.784125][    C0] host_regs: 00000010: 00000000 000005e6 00000000 00000000
  [    2.784137][    C0] host_regs: 00000020: 00000000 00031ef5 00000000 00000000
  [    2.784148][    C0] host_regs: 00000030: 00000008 00000001 00000000 00000000
  [    2.784159][    C0] host_regs: 00000040: 00000000 00000000 00000000 00000000
  [    2.784170][    C0] host_regs: 00000050: 02dd3000 00000001 00000000 00000000
  [    2.784182][    C0] host_regs: 00000060: 00000000 00000000 00000000 00000000
  [    2.784193][    C0] host_regs: 00000070: 02dd4000 00000001 00000000 00000000
  [    2.784204][    C0] host_regs: 00000080: 00000000 00000000 00000000 00000000
  [    2.784215][    C0] host_regs: 00000090: 00000016 00000000 00000000 00000000
  [    2.784266][   T13] ufshcd-rockchip 2a2d0000.ufs: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 2176; saved_uic_err = 0; force_reset = 0
  [    2.784334][  T168] ufshcd-rockchip 2a2d0000.ufs: ufshcd_query_flag: Sending flag query for idn 1 failed, err = 5

Recover by resetting the device and the controller and redoing the whole
initialization sequence, up to three times with a 100 ms settle delay in
between. Patch 1/2 moves the initialization sequence into a new function
so that it can be retried as a unit, and patch 2/2 adds the retry loop
around its call site in ufshcd_init().


Changes in v3:
- Stop the controller before draining the error handler and clearing
  the error state, so that no interrupt can re-latch errors or
  schedule the error handler during the cleanup(sashiko)
- Remove the hwmon device and reset the exception event masks
  between retries, since the device reset wipes their state on the
  device side; also remove the hwmon device on the init failure
  path(sashiko)
- Only restore the MCQ mode after the controller re-enable and let
  the next attempt configure the queues(sashiko)
- Take the address of the by-value host_lock for
  spin_lock_irqsave()(sashiko)

Changes in v2:
- Move the initialization sequence into a new function and retry by
  looping over its call site instead of adding more gotos to
  ufshcd_init()(Bart)
- Stop a pending error handler and clear the error state latched by the
  failed attempt, so that the retry starts from a clean state(sashiko)
- Reconfigure and re-enable MCQ after the controller re-enable, since
  the HCE toggle clears the MCQ mode setting(sashiko)

Shawn Lin (2):
  scsi: ufs: core: Move the device initialization sequence into a new
    function
  scsi: ufs: core: Retry the full device init on fatal link loss

 drivers/ufs/core/ufshcd.c | 124 ++++++++++++++++++++++++++++++--------
 1 file changed, 99 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] scsi: ufs: core: Move the device initialization sequence into a new function

[Shawn Lin]

Move the link startup and device initialization steps from ufshcd_init()
into a new function ufshcd_full_device_init(), so that the sequence can
be retried as a unit. No functional change intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

(no changes since v1)

 drivers/ufs/core/ufshcd.c | 84 +++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fd52deb72d21..8a8083a8f024 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11215,6 +11215,55 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 	return err;
 }
 
+/**
+ * ufshcd_full_device_init - Link startup and full device initialization
+ * @hba: per-adapter instance
+ *
+ * Execute the whole device initialization sequence used during driver
+ * probe: link startup, MCQ configuration, device verification, device
+ * initialization completion and device parameter setup.
+ *
+ * Return: 0 upon success; < 0 upon failure.
+ */
+static int ufshcd_full_device_init(struct ufs_hba *hba)
+{
+	int err;
+
+	hba->ufshcd_state = UFSHCD_STATE_RESET;
+
+	err = ufshcd_link_startup(hba);
+	if (err)
+		return err;
+
+	if (hba->mcq_enabled)
+		ufshcd_config_mcq(hba);
+
+	if (hba->quirks & UFSHCD_QUIRK_SKIP_PH_CONFIGURATION)
+		return 0;
+
+	/* Debug counters initialization */
+	ufshcd_clear_dbg_ufs_stats(hba);
+
+	/* UniPro link is active now */
+	ufshcd_set_link_active(hba);
+
+	/* Verify device initialization by sending NOP OUT UPIU */
+	err = ufshcd_verify_dev_init(hba);
+	if (err)
+		return err;
+
+	/* Initiate UFS initialization, and waiting until completion */
+	err = ufshcd_complete_dev_init(hba);
+	if (err)
+		return err;
+
+	err = ufshcd_device_params_init(hba);
+	if (err)
+		return err;
+
+	return ufshcd_post_device_init(hba);
+}
+
 /**
  * ufshcd_init - Driver initialization routine
  * @hba: per-adapter instance
@@ -11428,41 +11477,8 @@ 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();
 
-	hba->ufshcd_state = UFSHCD_STATE_RESET;
-
-	err = ufshcd_link_startup(hba);
-	if (err)
-		goto out_release;
-
-	if (hba->mcq_enabled)
-		ufshcd_config_mcq(hba);
-
-	if (hba->quirks & UFSHCD_QUIRK_SKIP_PH_CONFIGURATION)
-		goto initialized;
-
-	/* Debug counters initialization */
-	ufshcd_clear_dbg_ufs_stats(hba);
-
-	/* UniPro link is active now */
-	ufshcd_set_link_active(hba);
-
-	/* Verify device initialization by sending NOP OUT UPIU */
-	err = ufshcd_verify_dev_init(hba);
-	if (err)
-		goto out_release;
-
-	/* Initiate UFS initialization, and waiting until completion */
-	err = ufshcd_complete_dev_init(hba);
-	if (err)
-		goto out_release;
-
-	err = ufshcd_device_params_init(hba);
-	if (err)
-		goto out_release;
-
-	err = ufshcd_post_device_init(hba);
+	err = ufshcd_full_device_init(hba);
 
-initialized:
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
 		goto out_release;
-- 
2.25.1

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

[Shawn Lin]

On an RK3572 board the UFS device is sometimes reported dead during the
initialization at boot: DEVICE_FATAL_ERROR | UIC_LINK_LOST is latched and
the following UIC query fails with -EIO, leaving the device unusable.

Recover by resetting the device and the controller and redoing the whole
initialization sequence, up to three times with a 100 ms settle delay in
between. Doing this in ufshcd_init() rather than in the platform driver's
probe keeps the interrupt, clock and regulator resources acquired only
once, and the UFS device is already reachable through hba->priv at that
point.

Each retry stops the controller first, so that no interrupt can interfere
with the cleanup, then stops a pending error handler and clears the error
state latched by the failed attempt. The controller reset clears the MCQ
mode, so it is restored after the controller re-enable. The hwmon device
and the exception event masks of the failed attempt are dropped as well,
since the device reset wiped their state on the device side.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v3:
- Stop the controller before draining the error handler and clearing
  the error state, so that no interrupt can re-latch errors or
  schedule the error handler during the cleanup(sashiko)
- Remove the hwmon device and reset the exception event masks
  between retries, since the device reset wipes their state on the
  device side; also remove the hwmon device on the init failure
  path(sashiko)
- Only restore the MCQ mode after the controller re-enable and let
  the next attempt configure the queues(sashiko)
- Take the address of the by-value host_lock for
  spin_lock_irqsave()(sashiko)

Changes in v2:
- Move the initialization sequence into a new function and retry by
  looping over its call site instead of adding more gotos to
  ufshcd_init()(Bart)
- Stop a pending error handler and clear the error state latched by the
  failed attempt, so that the retry starts from a clean state(sashiko)
- Reconfigure and re-enable MCQ after the controller re-enable, since
  the HCE toggle clears the MCQ mode setting(sashiko)

 drivers/ufs/core/ufshcd.c | 60 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8a8083a8f024..12ada46d2b7e 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;
+
+		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);
+
+		ufshcd_device_reset(hba);
+		ufs_put_device_desc(hba);
+
+		/*
+		 * The device reset cleared the exception event control of
+		 * the device: drop the hwmon device of the previous attempt
+		 * and reset the masks, so that the next attempt registers a
+		 * fresh hwmon device and re-enables the exception events.
+		 */
+		mutex_lock(&hba->ee_ctrl_mutex);
+		hba->ee_ctrl_mask = 0;
+		hba->ee_drv_mask = 0;
+		mutex_unlock(&hba->ee_ctrl_mutex);
+		ufs_hwmon_remove(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;
+		}
+
+		/* The controller reset cleared the MCQ mode: restore it. */
+		if (hba->mcq_enabled)
+			ufshcd_mcq_enable(hba);
+
+		msleep(100);
+	}
 
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
@@ -11493,6 +11550,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	return 0;
 
 out_release:
+	ufs_hwmon_remove(hba);
 	ufshcd_release(hba);
 out_disable:
 	hba->is_irq_enabled = false;
-- 
2.25.1

Re: [PATCH v3 1/2] scsi: ufs: core: Move the device initialization sequence into a new function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] scsi: ufs: core: inaccurate return doc on ufshcd_full_device_init()
--

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

scsi: ufs: core: Move the device initialization sequence into a new function

This commit factors the link startup and device initialization steps out of
ufshcd_init() into a new helper function, ufshcd_full_device_init(), so that
the sequence can be retried as a unit.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index fd52deb72d21c..8a8083a8f024e 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11215,6 +11215,55 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
>  	return err;
>  }
>  
> +/**
> + * ufshcd_full_device_init - Link startup and full device initialization
> + * @hba: per-adapter instance
> + *
> + * Execute the whole device initialization sequence used during driver
> + * probe: link startup, MCQ configuration, device verification, device
> + * initialization completion and device parameter setup.
> + *
> + * Return: 0 upon success; < 0 upon failure.
> + */
> +static int ufshcd_full_device_init(struct ufs_hba *hba)

[Severity: Low]
This isn't a bug, but is the Return description accurate?

ufshcd_full_device_init() directly returns the status of
ufshcd_link_startup(), ufshcd_verify_dev_init(), ufshcd_complete_dev_init(),
ufshcd_device_params_init(), and ufshcd_post_device_init(). Several of these
callees can return positive (> 0) UFS OCS or UIC hardware status codes on
failure, which is why ufshcd_init() normalizes positive error codes at
out_error:

drivers/ufs/core/ufshcd.c:ufshcd_init() {
	...
out_error:
	return err > 0 ? -EIO : err;
}

Should the kernel-doc comment note that non-zero (or < 0 and > 0) values are
returned upon failure?

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

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

[sashiko-bot]

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