[RFC PATCH] scsi: ufs: core: Allow error recovery before the device W-LUN exists

[RFC PATCH] scsi: ufs: core: Allow error recovery before the device W-LUN exists

[Asdlerto Korry]

ufshcd_err_handling_should_stop() returns true while hba->ufs_device_wlun
is NULL, i.e. until the first probe has added the device W-LUN. If the
link fails during that first probe (for example NOP OUT failing after
link startup), the error handler therefore never resets the host or the
link: it only marks the state operational and returns, and the probe
keeps failing until it gives up.

On a Samsung Galaxy M52 5G (SM7325) the first link startup regularly
ends in PA errors (LINERESET). With the W-LUN check removed, the error
handler performs a real host reset and restore and the device comes up;
without it, it never does.

Removing the check exposes three places that dereference
hba->ufs_device_wlun unconditionally on this path:
ufshcd_rpm_get_sync()/ufshcd_rpm_put(), ufshcd_err_handling_prepare()
and ufshcd_recover_pm_error(). Skip the W-LUN runtime PM handling there
when the W-LUN does not exist yet; there is nothing to resume and no
regular SCSI I/O before it is added.

Note: the error handler then calls ufshcd_verify_dev_init() through
ufshcd_reset_and_restore() while the tag set is quiesced. This needs the
pseudo SCSI device queue to be unquiesced in ufshcd_err_handling_prepare()
(as done upstream), otherwise blk_execute_rq() waits forever.

Signed-off-by: Asdlerto Korry <korryasdlerto@gmail.com>
---
 drivers/ufs/core/ufshcd-priv.h |  4 ++++
 drivers/ufs/core/ufshcd.c      | 13 +++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f5..2cb0d542d114 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -385,6 +385,8 @@ static inline int ufshcd_update_ee_usr_mask(struct ufs_hba *hba,
 
 static inline int ufshcd_rpm_get_sync(struct ufs_hba *hba)
 {
+	if (!hba->ufs_device_wlun)
+		return 0;
 	return pm_runtime_get_sync(&hba->ufs_device_wlun->sdev_gendev);
 }
 
@@ -410,6 +412,8 @@ static inline int ufshcd_rpm_resume(struct ufs_hba *hba)
 
 static inline void ufshcd_rpm_put(struct ufs_hba *hba)
 {
+	if (!hba->ufs_device_wlun)
+		return;
 	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
 }
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index fd52deb72d21..9767fbc7b870 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6876,8 +6876,9 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 	 */
 	pm_runtime_get_sync(hba->dev);
 	ufshcd_rpm_get_sync(hba);
-	if (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
-	    hba->is_sys_suspended) {
+	if (hba->ufs_device_wlun &&
+	    (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
+	     hba->is_sys_suspended)) {
 		enum ufs_pm_op pm_op;
 
 		/*
@@ -6926,7 +6927,6 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
 {
 	return (!hba->is_powered || hba->shutting_down ||
-		!hba->ufs_device_wlun ||
 		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
 		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
 		   ufshcd_is_link_broken(hba))));
@@ -6935,12 +6935,17 @@ static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
 #ifdef CONFIG_PM
 static void ufshcd_recover_pm_error(struct ufs_hba *hba)
 {
-	struct scsi_target *starget = hba->ufs_device_wlun->sdev_target;
+	struct scsi_target *starget;
 	struct Scsi_Host *shost = hba->host;
 	struct scsi_device *sdev;
 	struct request_queue *q;
 	bool resume_sdev_queues = false;
 
+	/* Nothing to recover before the device W-LUN has been added */
+	if (!hba->ufs_device_wlun)
+		return;
+	starget = hba->ufs_device_wlun->sdev_target;
+
 	hba->is_sys_suspended = false;
 
 	/*
-- 
2.25.1

Re: [RFC PATCH] scsi: ufs: core: Allow error recovery before the device W-LUN exists

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] scsi: ufs: core: unserialized ufshcd_err_handler() race with ufshcd_init()
- [High] scsi: ufs: core: use-after-free in ufshcd_err_handler() during teardown
- [High] scsi: ufs: core: ufs_device_wlun race in ufshcd_err_handling_prepare()
- [High] scsi: ufs: core: missing ufs_device_wlun check in ufshcd_rpm_put_sync()
--

commit c2d444d6fca4852f29bbcb2d985d743a2e3be799
Author: Asdlerto Korry <korryasdlerto@gmail.com>

scsi: ufs: core: Allow error recovery before the device W-LUN exists

This commit removes the hba->ufs_device_wlun check from
ufshcd_err_handling_should_stop() so that the UFS error handler can reset and
restore the host and link if an error occurs before the device W-LUN is
added. It also adds hba->ufs_device_wlun checks to ufshcd_rpm_get_sync(),
ufshcd_rpm_put(), ufshcd_err_handling_prepare(), and
ufshcd_recover_pm_error().

> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index e55c2a02c1f50..2cb0d542d1147 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -385,6 +385,8 @@ static inline int ufshcd_update_ee_usr_mask(struct ufs_hba *hba,
>  
>  static inline int ufshcd_rpm_get_sync(struct ufs_hba *hba)
>  {
> +	if (!hba->ufs_device_wlun)
> +		return 0;
>  	return pm_runtime_get_sync(&hba->ufs_device_wlun->sdev_gendev);
>  }

[Severity: High]
Should ufshcd_rpm_put_sync() and the other runtime power management helpers in
drivers/ufs/core/ufshcd-priv.h also check hba->ufs_device_wlun?

Right below ufshcd_rpm_get_sync(), ufshcd_rpm_put_sync() still dereferences
hba->ufs_device_wlun unconditionally:

drivers/ufs/core/ufshcd-priv.h:ufshcd_rpm_put_sync() {
	return pm_runtime_put_sync(&hba->ufs_device_wlun->sdev_gendev);
}

Both ufs_debugfs_hba_init() (called in ufshcd_hba_init()) and
ufs_sysfs_add_nodes() (called in ufshcd_init()) register attributes before
ufshcd_async_scan()->ufshcd_add_lus()->ufshcd_scsi_add_wlus() creates
hba->ufs_device_wlun, and those attributes remain registered if probing or
adding the W-LUN fails.

Callers such as ufshcd_read_hci_reg() in drivers/ufs/core/ufs-sysfs.c and
ufs_debugfs_get_user_access() / ufs_debugfs_put_user_access() in
drivers/ufs/core/ufs-debugfs.c pair ufshcd_rpm_get_sync() with
ufshcd_rpm_put_sync():

drivers/ufs/core/ufs-sysfs.c:ufshcd_read_hci_reg() {
	ufshcd_rpm_get_sync(hba);
	ufshcd_hold(hba);
	*val = ufshcd_readl(hba, reg);
	ufshcd_release(hba);
	ufshcd_rpm_put_sync(hba);
	return 0;
}

When hba->ufs_device_wlun is not set yet, ufshcd_rpm_get_sync() now succeeds
and returns 0, after which ufshcd_rpm_put_sync() dereferences a null
hba->ufs_device_wlun pointer.

> @@ -410,6 +412,8 @@ static inline int ufshcd_rpm_resume(struct ufs_hba *hba)
>  
>  static inline void ufshcd_rpm_put(struct ufs_hba *hba)
>  {
> +	if (!hba->ufs_device_wlun)
> +		return;
>  	pm_runtime_put(&hba->ufs_device_wlun->sdev_gendev);
>  }
>  
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index fd52deb72d21c..9767fbc7b870b 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6876,8 +6876,9 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>  	 */
>  	pm_runtime_get_sync(hba->dev);
>  	ufshcd_rpm_get_sync(hba);
> -	if (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
> -	    hba->is_sys_suspended) {
> +	if (hba->ufs_device_wlun &&
> +	    (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
> +	     hba->is_sys_suspended)) {

[Severity: High]
Can ufshcd_err_handler() race with ufshcd_scsi_add_wlus() while
hba->ufs_device_wlun is being populated?

In ufshcd_async_scan(), hba->host_sem is released before calling
ufshcd_add_lus()->ufshcd_scsi_add_wlus():

drivers/ufs/core/ufshcd.c:ufshcd_async_scan() {
	down(&hba->host_sem);
	/* Initialize hba, detect and initialize UFS device */
	probe_start = ktime_get();
	ret = ufshcd_probe_hba(hba, true);
	ufshcd_process_probe_result(hba, probe_start, ret);
	up(&hba->host_sem);
	if (ret)
		goto out;

	/* Probe and add UFS logical units  */
	ret = ufshcd_add_lus(hba);
}

And ufshcd_scsi_add_wlus() assigns the result of __scsi_add_device() directly
to hba->ufs_device_wlun before checking for an error pointer:

drivers/ufs/core/ufshcd.c:ufshcd_scsi_add_wlus() {
	hba->ufs_device_wlun = __scsi_add_device(hba->host, 0, 0,
		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN), NULL);
	if (IS_ERR(hba->ufs_device_wlun)) {
		ret = PTR_ERR(hba->ufs_device_wlun);
		hba->ufs_device_wlun = NULL;
		goto out;
	}
	...
}

Because ufshcd_err_handler() can now run before the W-LUN exists, it can run
concurrently with ufshcd_scsi_add_wlus(). Does this open two races?

First, if ufshcd_err_handling_prepare() runs while hba->ufs_device_wlun is
still null, ufshcd_rpm_get_sync() returns 0 without incrementing the W-LUN
runtime power management usage count. If __scsi_add_device() finishes and
sets hba->ufs_device_wlun before ufshcd_err_handling_unprepare() runs,
ufshcd_rpm_put() will see a non-null hba->ufs_device_wlun and call
pm_runtime_put(), dropping a reference that ufshcd_err_handling_prepare()
never acquired (and conversely leaking a reference if adding sdev_rpmb fails
and removes ufs_device_wlun while error handling is running).

Second, if __scsi_add_device() fails, hba->ufs_device_wlun transiently holds
an error pointer before being reset to null, which the non-null checks in
ufshcd_rpm_get_sync(), ufshcd_err_handling_prepare(),
ufshcd_recover_pm_error(), and ufshcd_rpm_put() will dereference.

[ ... ]

> @@ -6926,7 +6927,6 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
>  static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
>  {
>  	return (!hba->is_powered || hba->shutting_down ||
> -		!hba->ufs_device_wlun ||
>  		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
>  		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
>  		   ufshcd_is_link_broken(hba))));

[Severity: High]
What serializes ufshcd_err_handler() against ufshcd_init() once this check is
removed?

In ufshcd_init(), ufshcd_link_startup()->ufshcd_make_hba_operational() enables
controller interrupts, and ufshcd_init() then runs ufshcd_verify_dev_init(),
ufshcd_complete_dev_init(), ufshcd_device_params_init(), and
ufshcd_post_device_init() without holding hba->host_sem:

drivers/ufs/core/ufshcd.c:ufshcd_init() {
	...
	hba->ufshcd_state = UFSHCD_STATE_RESET;

	err = ufshcd_link_startup(hba);
	if (err)
		goto out_release;
	...
	/* Verify device initialization by sending NOP OUT UPIU */
	err = ufshcd_verify_dev_init(hba);
	if (err)
		goto out_release;

	err = ufshcd_complete_dev_init(hba);
	if (err)
		goto out_release;

	/* Init check for device descriptor sizes */
	err = ufshcd_device_params_init(hba);
	if (err)
		goto out_release;

	/* Let the variant know that the UFS device has been initialized */
	err = ufshcd_post_device_init(hba);
	if (err)
		goto out_put_Desc;
	...
}

When an error interrupt schedules hba->eh_work during this initial bring-up,
ufshcd_err_handler() acquires hba->host_sem on hba->eh_wq and runs in parallel
with ufshcd_init():

1. In ufshcd_verify_dev_init(), the probe thread holds ufshcd_dev_man_lock()
(which acquires mutex_lock(&hba->dev_cmd.lock) and
down_read(&hba->clk_scaling_lock)) across all retry iterations:

drivers/ufs/core/ufshcd.c:ufshcd_verify_dev_init() {
	ufshcd_dev_man_lock(hba);

	for (retries = NOP_OUT_RETRIES; retries > 0; retries--) {
		err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_NOP,
					  hba->nop_out_timeout);

		if (!err || err == -ETIMEDOUT)
			break;

		dev_dbg(hba->dev, "%s: error %d retrying\n", __func__, err);
	}
	ufshcd_dev_man_unlock(hba);
	...
}

Concurrently, ufshcd_err_handler()->ufshcd_err_handling_prepare() calls
ufshcd_clk_scaling_allow(hba, false), which blocks on
down_write(&hba->clk_scaling_lock). If an in-flight command times out and
ufshcd_abort()->ufshcd_clear_cmd() fails to clear the doorbell, the error
handler thread invokes ufshcd_eh_host_reset_handler()->flush_work(&hba->eh_work),
deadlocking because ufshcd_verify_dev_init() holds clk_scaling_lock while
waiting in blk_execute_rq() for error handling, error handling waits in
flush_work(&hba->eh_work), and ufshcd_err_handler() waits for clk_scaling_lock.
Even without a timeout, ufshcd_verify_dev_init() only releases clk_scaling_lock
after failing and returning to ufshcd_init(), which immediately aborts via
goto out_release before ufshcd_err_handler() can reset and restore the link.

2. If ufshcd_err_handling_prepare() acquires clk_scaling_lock before
ufshcd_init() (or between queries in ufshcd_complete_dev_init() and
ufshcd_device_params_init()), ufshcd_err_handler() runs
ufshcd_reset_and_restore()->ufshcd_host_reset_and_restore() concurrently with
ufshcd_init():

drivers/ufs/core/ufshcd.c:ufshcd_host_reset_and_restore() {
	...
	err = ufshcd_hba_enable(hba);

	/* Establish the link again and restore the device */
	if (!err) {
		err = ufshcd_device_init(hba, /*init_dev_params=*/false);
		if (!err)
			err = ufshcd_probe_hba(hba, false);
	}
	...
}

Because init_dev_params is false and ufshcd_device_params_init() in
ufshcd_init() has not completed yet, ufshcd_probe_hba(hba, false) runs with
uninitialized hba->dev_info, hba->dev_quirks, and hba->max_pwr_info, and on
controllers with UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH it calls
ufs_put_device_desc(hba) and frees hba->dev_info.model while ufshcd_init()
is still running.

[Severity: High]
Can removing !hba->ufs_device_wlun from ufshcd_err_handling_should_stop() also
cause a use-after-free during driver removal or probe failure teardown?

In ufshcd_remove(), scsi_remove_host() is called before interrupts are
disabled and before ufshcd_hba_exit() destroys hba->eh_wq, while
hba->is_powered is still true and hba->shutting_down is false:

drivers/ufs/core/ufshcd.c:ufshcd_remove() {
	if (hba->bsg_queue)
		ufshcd_bsg_remove(hba);
	ufs_hwmon_remove(hba);
	ufs_sysfs_remove_nodes(hba->dev);
	cancel_delayed_work_sync(&hba->ufs_rtc_update_work);
	blk_mq_destroy_queue(hba->tmf_queue);
	blk_put_queue(hba->tmf_queue);
	blk_mq_free_tag_set(&hba->tmf_tag_set);
	if (hba->scsi_host_added)
		scsi_remove_host(hba->host);
	/* disable interrupts */
	ufshcd_disable_intr(hba, hba->intr_mask);
	ufshcd_hba_stop(hba);
	ufshcd_hba_exit(hba);
}

And in ufshcd_hba_exit() (which is also called on the out_release failure path
of ufshcd_init()), clock scaling and clock gating are torn down before
destroy_workqueue(hba->eh_wq) flushes pending eh_work, while hba->is_powered
is still true:

drivers/ufs/core/ufshcd.c:ufshcd_hba_exit() {
	if (hba->is_powered) {
		ufshcd_pm_qos_exit(hba);
		ufshcd_exit_clk_scaling(hba);
		ufshcd_exit_clk_gating(hba);
		if (hba->eh_wq)
			destroy_workqueue(hba->eh_wq);
		...
		hba->is_powered = false;
	}
}

Previously, scsi_remove_host() destroyed the W-LUN device and cleared
hba->ufs_device_wlun to null (and on the ufshcd_init() failure path
hba->ufs_device_wlun was already null), so ufshcd_err_handling_should_stop()
returned true and any pending eh_work exited immediately.

Without that check, if eh_work runs after scsi_remove_host() (which frees
hba->host->pseudo_sdev and hba->host->tag_set) or when flushed by
destroy_workqueue(hba->eh_wq) in ufshcd_hba_exit(),
ufshcd_err_handling_should_stop() returns false and
ufshcd_err_handling_prepare() accesses the torn-down clock-gating/scaling
state, freed hba->host->tag_set, and freed
hba->host->pseudo_sdev->request_queue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102003607.341651-1-korryasdlerto@gmail.com?part=1

Re: [RFC PATCH] scsi: ufs: core: Allow error recovery before the device W-LUN exists

[Peter Wang]

On Mon, 2026-11-02 at 02:36 +0200, Asdlerto Korry wrote:
> ufshcd_err_handling_should_stop() returns true while hba-
> >ufs_device_wlun
> is NULL, i.e. until the first probe has added the device W-LUN. If
> the
> link fails during that first probe (for example NOP OUT failing after
> link startup), the error handler therefore never resets the host or
> the
> link: it only marks the state operational and returns, and the probe
> keeps failing until it gives up.
> 
> On a Samsung Galaxy M52 5G (SM7325) the first link startup regularly
> ends in PA errors (LINERESET). With the W-LUN check removed, the
> error
> handler performs a real host reset and restore and the device comes
> up;
> without it, it never does.

Hi Asdlerto,

I’m a bit curious why the first link startup fails, while 
the link startup after a reset and restore doesn’t. 
Shouldn’t these two flows be identical? 
I think the driver is designed on the assumption that the 
basic init must succeed, and that only a re-init can make 
recovery possible.

Thanks.
Peter

Re: [RFC PATCH] scsi: ufs: core: Allow error recovery before the device W-LUN exists

[Asdlerto Korry]

> I'm a bit curious why the first link startup fails, while
> the link startup after a reset and restore doesn't.
> Shouldn't these two flows be identical?
> I think the driver is designed on the assumption that the
> basic init must succeed, and that only a re-init can make
> recovery possible.

Hi Peter,

Thanks for the question, you are right. I went back to my notes:
the first init failure was caused by the board, not by the core.

The vendor device tree of this phone has the UFS VCC and VCCQ
supplies (PM7325 LDO7 and LDO9) without regulator-allow-set-load,
so regulator_set_load() from the UFS driver is a no-op and both LDOs
stay in low-power mode. Under the load of the first transfers the
device does not answer the NOP OUT, the host sees
TCx_REPLAY_TIMER_EXPIRED, and the PA error / LINERESET storm I
described only starts after that. With regulator-allow-set-load and
an HPM initial mode on those regulators, the first init succeeds
and the error handler is never needed.

Also, my claim that the device always comes up after the reset was
too strong: in my tests the second link startup succeeded, but
ufshcd_device_init() did not always complete.

So please drop this RFC. The races Sashiko pointed out are another
reason not to remove that check. Sorry for the noise, and thanks
for the review.

Asdlerto