Re: [PATCH 2/2] scsi: ufs: core: Wake consumer queues on suspended W-LUN recovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ufs: core: premature HBA runtime suspend in ufshcd_recover_pm_error()
- [High] scsi: ufs: core: missed queue wake in ufshcd_recover_pm_error() after prepare
--

commit ea7f240ee0c25a5e5c4a46de9b52655deaa5da4e
Author: Stanley Jhu <stanleyjhu@google.com>

scsi: ufs: core: Wake consumer queues on suspended W-LUN recovery

This commit updates ufshcd_recover_pm_error() to set resume_sdev_queues
when ufs_device_wlun is runtime-suspended, so consumer queues blocked in
__bio_queue_enter() after a failed W-LUN runtime resume are woken via
pm_request_resume().

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index fd52deb72d21c..0701ec408736d 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6953,7 +6953,9 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba)
>  		resume_sdev_queues = true;
>  	}
>  
> -	if (hba->ufs_device_wlun->sdev_gendev.power.runtime_error) {
> +	if (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev)) {
> +		resume_sdev_queues = true;

[Severity: High]
Can leaving ufs_device_wlun in RPM_SUSPENDED here cause hba->dev to
prematurely runtime-suspend before the asynchronous consumer resume work runs?

When W-LUN runtime resume fails, rpm_resume() leaves ufs_device_wlun in
RPM_SUSPENDED with power.runtime_error == 0 and drops the reference on its
parent starget->dev, allowing starget->dev and shost->shost_gendev to remain
RPM_SUSPENDED with hba->dev->power.child_count == 0.

In ufshcd_err_handler(), ufshcd_err_handling_prepare() takes
pm_runtime_get_sync(hba->dev), powers on regulators/clocks/IRQs, and calls
ufshcd_vops_resume(), and then ufshcd_reset_and_restore() resets the host and
restores hba->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE and
hba->uic_link_state = UIC_LINK_ACTIVE_STATE.

Unlike the power.runtime_error branch below:

drivers/ufs/core/ufshcd.c:ufshcd_recover_pm_error() {
	...
	} else if (hba->ufs_device_wlun->sdev_gendev.power.runtime_error) {
		pm_runtime_get_sync(&starget->dev);
		pm_runtime_set_active(&hba->ufs_device_wlun->sdev_gendev);
		pm_runtime_put_sync(&starget->dev);
		resume_sdev_queues = true;
	}
}

which synchronously resumes starget->dev and sets ufs_device_wlun active (so
starget->dev, shost->shost_gendev, and hba->dev all have power.child_count ==
1), this new branch only sets resume_sdev_queues = true and queues
asynchronous pm_request_resume(q->dev) work on pm_wq (or queues nothing if no
consumer LUN has q->rpm_status in RPM_SUSPENDED or RPM_SUSPENDING).

Immediately after ufshcd_recover_pm_error() returns,
ufshcd_err_handling_unprepare() runs:

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_unprepare() {
	...
	if (ufshcd_is_clkscaling_supported(hba))
		ufshcd_clk_scaling_suspend(hba, false);
	ufshcd_rpm_put(hba);
	pm_runtime_put(hba->dev);
}

Because ufs_device_wlun, starget->dev, and shost->shost_gendev are still
RPM_SUSPENDED, pm_runtime_put(hba->dev) drops hba->dev->power.usage_count to 0
while hba->dev->power.child_count is 0. Wouldn't that allow
ufshcd_runtime_suspend() -> ufshcd_suspend() to disable host IRQs, gate clocks
to CLKS_OFF, and put vreg and vdd_hba into LPM while the UFS device and UniPro
link are still in UFS_ACTIVE_PWR_MODE and UIC_LINK_ACTIVE_STATE (without
__ufshcd_wl_suspend() or ufshcd_vops_suspend() having run), and then cause a
subsequent __ufshcd_wl_resume() to skip controller/link reinitialization
because both ufshcd_is_link_hibern8(hba) and ufshcd_is_link_off(hba) are
false?

> +	} else if (hba->ufs_device_wlun->sdev_gendev.power.runtime_error) {

[Severity: High]
What happens if ufshcd_rpm_get_sync(hba) in ufshcd_err_handling_prepare()
already transitioned ufs_device_wlun to RPM_ACTIVE before
ufshcd_recover_pm_error() is reached?

When a consumer-initiated runtime resume of ufs_device_wlun fails, commit
eef1d74bf88f leaves ufs_device_wlun in RPM_SUSPENDED with
power.runtime_error == 0, while the consumer queue remains blocked in
__bio_queue_enter() with q->pm_only set and q->rpm_status == RPM_SUSPENDED.

When ufshcd_err_handler() subsequently runs, it calls
ufshcd_err_handling_prepare() before ufshcd_set_eh_in_progress():

drivers/ufs/core/ufshcd.c:ufshcd_err_handling_prepare() {
	pm_runtime_get_sync(hba->dev);
	ufshcd_rpm_get_sync(hba);
	if (pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) ||
	...
}

Because power.runtime_error is 0, ufshcd_rpm_get_sync(hba) invokes
__ufshcd_wl_resume() a second time instead of failing immediately with
-EINVAL. That second __ufshcd_wl_resume() call can succeed and transition
ufs_device_wlun to RPM_ACTIVE: for example, when hba->ufshcd_state is
UFSHCD_STATE_EH_SCHEDULED_NON_FATAL (which ufshcd_queuecommand() allows before
ufshcd_set_eh_in_progress() and which later triggers needs_reset = true in
ufshcd_err_handler()), or when __ufshcd_wl_resume() recovers the link via
ufshcd_link_recovery() or ufshcd_reset_and_restore():

drivers/ufs/core/ufshcd.c:__ufshcd_wl_resume() {
	...
			ret = ufshcd_link_recovery(hba);
			if (ret)
				goto vendor_suspend;
		}
	} else if (ufshcd_is_link_off(hba)) {
		ret = ufshcd_reset_and_restore(hba);
	...
}

Once ufshcd_rpm_get_sync(hba) in ufshcd_err_handling_prepare() sets
ufs_device_wlun to RPM_ACTIVE, both
pm_runtime_status_suspended(&hba->ufs_device_wlun->sdev_gendev) and
hba->ufs_device_wlun->sdev_gendev.power.runtime_error will be false in
ufshcd_recover_pm_error() (or ufshcd_err_handling_should_stop() will skip the
do_reset path altogether if __ufshcd_wl_resume() already ran
ufshcd_reset_and_restore()). Wouldn't that leave resume_sdev_queues false and
leave the consumer queue stuck in __bio_queue_enter()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105071518.1458428-1-stanleyjhu@google.com?part=2