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

[PATCH v2 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 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 | 114 +++++++++++++++++++++++++++++---------
 1 file changed, 87 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v2 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 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 | 114 +++++++++++++++++++++++++++++---------
 1 file changed, 87 insertions(+), 27 deletions(-)

-- 
2.25.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 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 ee643830a57a..f1e54d719537 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11207,6 +11207,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
@@ -11420,41 +11469,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 v2 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 ee643830a57a..f1e54d719537 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11207,6 +11207,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
@@ -11420,41 +11469,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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 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 a pending error handler and clears the error state
latched by the failed attempt, reconfigures and re-enables MCQ if the
controller reset cleared it, and restores the reset ufshcd_state, so
that no stale error context survives into the retried initialization.

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

---

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 | 46 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f1e54d719537..bd82d6755739 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11468,8 +11468,52 @@ 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 it and start the retry
+		 * from a clean state, mirroring ufshcd_reset_and_restore().
+		 */
+		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);
+		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;
+		}
+
+		/* Reconfigure MCQ upon controller re-enable */
+		if (hba->mcq_enabled) {
+			ufshcd_config_mcq(hba);
+			ufshcd_mcq_enable(hba);
+		}
+
+		msleep(100);
+	}
 
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
-- 
2.25.1

[PATCH v2 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 a pending error handler and clears the error state
latched by the failed attempt, reconfigures and re-enables MCQ if the
controller reset cleared it, and restores the reset ufshcd_state, so
that no stale error context survives into the retried initialization.

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

---

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 | 46 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f1e54d719537..bd82d6755739 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -11468,8 +11468,52 @@ 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 it and start the retry
+		 * from a clean state, mirroring ufshcd_reset_and_restore().
+		 */
+		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);
+		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;
+		}
+
+		/* Reconfigure MCQ upon controller re-enable */
+		if (hba->mcq_enabled) {
+			ufshcd_config_mcq(hba);
+			ufshcd_mcq_enable(hba);
+		}
+
+		msleep(100);
+	}
 
 	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
-- 
2.25.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 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: broken MCQ reconfiguration order in ufshcd_init() retry
- [High] scsi: ufs: error state cleared before ufshcd_hba_stop() in ufshcd_init()
- [High] scsi: ufs: core: missing ufs_hwmon_remove() and EE mask reset on init retry
- [Low] scsi: ufs: core: pass host_lock pointer to spin_lock_irqsave()
--

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

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

This commit retries ufshcd_full_device_init() up to three times in
ufshcd_init() when device initialization fails, resetting the device and
host controller and clearing latched error state between attempts.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index f1e54d7195370..bd82d6755739c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -11468,8 +11468,52 @@ 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 it and start the retry
> +		 * from a clean state, mirroring ufshcd_reset_and_restore().
> +		 */
> +		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: Low]
In include/scsi/scsi_host.h, host_lock in struct Scsi_Host is defined by
value rather than as a pointer:

	spinlock_t		host_lock;

Should spin_lock_irqsave() and spin_unlock_irqrestore() in ufshcd_init()
take &hba->host->host_lock instead of hba->host->host_lock so that
drivers/ufs/core/ufshcd.c compiles?

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

[Severity: High]
What happens if an error or link-loss interrupt fires while the host
controller is still active during ufshcd_device_reset(), before
ufshcd_hba_stop() disables the controller?

Because cancel_work_sync(&hba->eh_work) and the clearing of hba->saved_err,
hba->saved_uic_err, and hba->ufshcd_state happen before ufshcd_hba_stop(),
resetting the attached device while controller interrupts are enabled can
trigger ufshcd_intr() -> ufshcd_check_errors():

	hba->saved_err |= hba->errors;
	hba->saved_uic_err |= hba->uic_error;
	...
	ufshcd_schedule_eh_work(hba);

Since hba->ufs_device_wlun is not set yet and hba->host_sem is not held
during ufshcd_init(), ufshcd_err_handler() can then run concurrently on
hba->eh_wq:

	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;
	}

Could this overwrite hba->ufshcd_state and leave hba->saved_err and
hba->saved_uic_err latched during the retry? Should ufshcd_hba_stop(hba) be
called before cancelling hba->eh_work, clearing the error state, and
resetting the device?

[Severity: High]
Can this retry path also leak the hwmon device and struct ufs_hwmon_data if
ufshcd_full_device_init() fails in ufshcd_post_device_init() after
ufshcd_device_params_init() has already succeeded?

During ufshcd_full_device_init(), the call chain is:

	ufshcd_device_params_init()
	  ufs_get_device_desc()
	    ufshcd_temp_notif_probe()
	      ufshcd_enable_ee(hba, mask);
	      ufs_hwmon_probe(hba, mask);
	ufshcd_post_device_init()
	  ufshcd_config_pwr_mode()

If ufshcd_post_device_init() fails, ufs_put_device_desc() only frees
dev_info->model and dev_info->device_id. On the next iteration,
ufs_hwmon_probe() in drivers/ufs/core/ufs-hwmon.c allocates a new
struct ufs_hwmon_data, registers a second hwmon device, and overwrites
hba->hwmon_device:

	data = kzalloc_obj(*data);
	if (!data)
		return;
	...
	hwmon = hwmon_device_register_with_info(dev, "ufs", data, &ufs_hwmon_hba_info, NULL);
	...
	hba->hwmon_device = hwmon;

In addition, hba->ee_ctrl_mask and hba->ee_drv_mask retain the bits set by
ufshcd_enable_ee() before ufshcd_device_reset() reset the device, which
causes ufshcd_update_ee_control() on the next attempt to see an unchanged
mask and skip writing the exception event control attribute to the device.
Should the retry cleanup call ufs_hwmon_remove(hba) and clear the exception
event masks?

> +		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;
> +		}
> +
> +		/* Reconfigure MCQ upon controller re-enable */
> +		if (hba->mcq_enabled) {
> +			ufshcd_config_mcq(hba);
> +			ufshcd_mcq_enable(hba);
> +		}

[Severity: High]
Is the ordering of ufshcd_config_mcq(hba) and ufshcd_mcq_enable(hba) here
intended?

On the initial probe path, ufshcd_add_scsi_host() calls ufshcd_mcq_enable()
first, and ufshcd_full_device_init() calls ufshcd_config_mcq() only after
ufshcd_link_startup() completes:

	err = ufshcd_link_startup(hba);
	if (err)
		return err;

	if (hba->mcq_enabled)
		ufshcd_config_mcq(hba);

In the retry path of ufshcd_init(), calling ufshcd_config_mcq(hba) before
ufshcd_mcq_enable(hba) configures the queues and enables them in
ufshcd_mcq_make_queues_operational() before ufshcd_mcq_enable() selects
multi-circular queue mode in REG_UFS_MEM_CFG and before the next attempt's
ufshcd_link_startup(hba). Then, when ufshcd_full_device_init() runs on the
next iteration, it calls ufshcd_config_mcq(hba) a second time while the
hardware queues are already enabled.

Furthermore, if ufshcd_link_startup() encounters an error on its own retry
and calls ufshcd_hba_enable(hba), the controller reset will clear
REG_UFS_MEM_CFG again without ufshcd_mcq_enable(hba) being re-applied before
ufshcd_config_mcq(hba). Should the retry path only call
ufshcd_mcq_enable(hba) (or should ufshcd_mcq_enable(hba) be called right
before ufshcd_config_mcq(hba) inside ufshcd_full_device_init())?

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

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

[Shawn Lin]

在 2026/10/29 星期四 10:06, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] scsi: ufs: core: broken MCQ reconfiguration order in ufshcd_init() retry
> - [High] scsi: ufs: error state cleared before ufshcd_hba_stop() in ufshcd_init()
> - [High] scsi: ufs: core: missing ufs_hwmon_remove() and EE mask reset on init retry
> - [Low] scsi: ufs: core: pass host_lock pointer to spin_lock_irqsave()
> --

...

> 
> [Severity: High]
> What happens if an error or link-loss interrupt fires while the host
> controller is still active during ufshcd_device_reset(), before
> ufshcd_hba_stop() disables the controller?
> 
> Because cancel_work_sync(&hba->eh_work) and the clearing of hba->saved_err,
> hba->saved_uic_err, and hba->ufshcd_state happen before ufshcd_hba_stop(),
> resetting the attached device while controller interrupts are enabled can
> trigger ufshcd_intr() -> ufshcd_check_errors():
> 
> 	hba->saved_err |= hba->errors;
> 	hba->saved_uic_err |= hba->uic_error;
> 	...
> 	ufshcd_schedule_eh_work(hba);
> 
> Since hba->ufs_device_wlun is not set yet and hba->host_sem is not held
> during ufshcd_init(), ufshcd_err_handler() can then run concurrently on
> hba->eh_wq:
> 
> 	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;
> 	}
> 
> Could this overwrite hba->ufshcd_state and leave hba->saved_err and
> hba->saved_uic_err latched during the retry? Should ufshcd_hba_stop(hba) be
> called before cancelling hba->eh_work, clearing the error state, and
> resetting the device?

It's a valid concern, will fix it in v3.

> 
> [Severity: High]
> Can this retry path also leak the hwmon device and struct ufs_hwmon_data if
> ufshcd_full_device_init() fails in ufshcd_post_device_init() after
> ufshcd_device_params_init() has already succeeded?
> 
> During ufshcd_full_device_init(), the call chain is:
> 
> 	ufshcd_device_params_init()
> 	  ufs_get_device_desc()
> 	    ufshcd_temp_notif_probe()
> 	      ufshcd_enable_ee(hba, mask);
> 	      ufs_hwmon_probe(hba, mask);
> 	ufshcd_post_device_init()
> 	  ufshcd_config_pwr_mode()
> 
> If ufshcd_post_device_init() fails, ufs_put_device_desc() only frees
> dev_info->model and dev_info->device_id. On the next iteration,
> ufs_hwmon_probe() in drivers/ufs/core/ufs-hwmon.c allocates a new
> struct ufs_hwmon_data, registers a second hwmon device, and overwrites
> hba->hwmon_device:
> 
> 	data = kzalloc_obj(*data);
> 	if (!data)
> 		return;
> 	...
> 	hwmon = hwmon_device_register_with_info(dev, "ufs", data, &ufs_hwmon_hba_info, NULL);
> 	...
> 	hba->hwmon_device = hwmon;
> 
> In addition, hba->ee_ctrl_mask and hba->ee_drv_mask retain the bits set by
> ufshcd_enable_ee() before ufshcd_device_reset() reset the device, which
> causes ufshcd_update_ee_control() on the next attempt to see an unchanged
> mask and skip writing the exception event control attribute to the device.
> Should the retry cleanup call ufs_hwmon_remove(hba) and clear the exception
> event masks?

Ack.

> 
>> +		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;
>> +		}
>> +
>> +		/* Reconfigure MCQ upon controller re-enable */
>> +		if (hba->mcq_enabled) {
>> +			ufshcd_config_mcq(hba);
>> +			ufshcd_mcq_enable(hba);
>> +		}
> 
> [Severity: High]
> Is the ordering of ufshcd_config_mcq(hba) and ufshcd_mcq_enable(hba) here
> intended?
> 
> On the initial probe path, ufshcd_add_scsi_host() calls ufshcd_mcq_enable()
> first, and ufshcd_full_device_init() calls ufshcd_config_mcq() only after
> ufshcd_link_startup() completes:
> 
> 	err = ufshcd_link_startup(hba);
> 	if (err)
> 		return err;
> 
> 	if (hba->mcq_enabled)
> 		ufshcd_config_mcq(hba);
> 
> In the retry path of ufshcd_init(), calling ufshcd_config_mcq(hba) before
> ufshcd_mcq_enable(hba) configures the queues and enables them in
> ufshcd_mcq_make_queues_operational() before ufshcd_mcq_enable() selects
> multi-circular queue mode in REG_UFS_MEM_CFG and before the next attempt's
> ufshcd_link_startup(hba). Then, when ufshcd_full_device_init() runs on the
> next iteration, it calls ufshcd_config_mcq(hba) a second time while the
> hardware queues are already enabled.
> 
> Furthermore, if ufshcd_link_startup() encounters an error on its own retry
> and calls ufshcd_hba_enable(hba), the controller reset will clear
> REG_UFS_MEM_CFG again without ufshcd_mcq_enable(hba) being re-applied before
> ufshcd_config_mcq(hba). Should the retry path only call
> ufshcd_mcq_enable(hba) (or should ufshcd_mcq_enable(hba) be called right
> before ufshcd_config_mcq(hba) inside ufshcd_full_device_init())?

My platform doesn't support MCQ, so actually I cannot test
this path. Will look into it.

>