Re: [PATCH v3] scsi: libsas: Fix SMP IO deadlock during HA resume
From: yangxingui
Date: Sun Sep 27 2026 - 23:47:44 EST
On 2026/9/22 18:37, John Garry wrote:
On 9/22/26 04:10, yangxingui wrote:
Yes，we do have a full test suite, but it runs per machine topology. The
It is also a narrow trigger: a directly-attached ATA device resets its
phy via lldd_control_phy() (no SMP IO), so it needs an expander-
attached ATA device whose EH lands inside the drain of a runtime
resume - which is why the testing of 3dbbbf656b85, whose scenario was
the disk-wake race, did not catch it.
uh, the expander-attached SATA disk scenario would be a very common scenario - do you test this always when developing this code?
3dbbbf656b85 validation ran on the direct-attach configuration, since
the HA resume race it was fixing was originally reported there. The
expander topology was covered in the next test round, and this
deadlock showed up as soon as we switched to it: on that topology
every ATA port resume goes through the EH reset (that is how
ata_sas_port_resume() is implemented), so the SMP IO against the
ongoing runtime resume happens on essentially every cycle.
So it seems that the expander-attached scenario was not tested for that comment mentioned.
You need to test directly-attached and expander-attached config for any relevant patchset.
Otherwise we have this scenario that alternate configs are continually broken.
Understood, we will cover both configurations going forward.
Aha - each respin addressed a review finding on the PMI am just wondering why this needs to be fixed so many times...
Given that, should Fixes: point at 3dbbbf656b85 instead? I kept
0da7ca4c4fd9 as that is where the pm_runtime_get_sync() being fixed
comes from, but the deadlock itself only exists where 3dbbbf656b85
is.
reference handling, not a re-fix of the deadlock. The core fix is
unchanged since v1.
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/ sas_expander.c
index 811c9eb4fef1..5a8cdd3682fe 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -61,8 +61,22 @@ static int smp_execute_task_sg(struct domain_device *dev,
struct sas_internal *i =
to_sas_internal(dev->port->ha->shost->transportt);
struct sas_ha_struct *ha = dev->port->ha;
-
- pm_runtime_get_sync(ha->dev);
+ bool ha_resuming = test_bit(SAS_HA_RESUMING, &ha->state);
+
+ /*
+ * While the host is resuming, ha->dev may be RPM_RESUMING and
+ * the resume blocked in sas_drain_work() waiting for this very
+ * SMP IO, so waiting for the host to resume here would deadlock.
+ * Hold the reference without resuming, the hardware is already
+ * initialized by the LLDD before sas_resume_ha() runs.
+ */
+ if (ha_resuming) {
+ pm_runtime_get_noresume(ha->dev);
+ } else {
+ res = pm_runtime_resume_and_get(ha->dev);
why change from pm_runtime_get_sync() to pm_runtime_resume_and_get()?
That addresses the pre-existing issue which Sashiko flagged:
Then that would be a separate change.
Ok. With the resume moved to the BSG entry point this is
resolved naturally: smp_execute_task_sg() uses get_noresume() (void
return, nothing to check), and the resume_and_get() at the callsite
checks its result.
the
return value of pm_runtime_get_sync() was ignored, so on a failed
resume the code would blindly proceed to submit the SMP task anyway.
Simply checking the return of pm_runtime_get_sync() would be awkward:
it keeps the usage counter incremented even on failure, so the error
path would also need a manual pm_runtime_put_noidle() to balance it.
pm_runtime_resume_and_get() rolls the counter back internally and
returns 0 or an errno, so the check is just "if (res) return res;"
Outside the resume window, the caller that needs the resume is BSG
+ if (res)
+ return res;
+ }
So when is it required to really do pm_runtime_resume_and_get() (and not pm_runtime_get_noresume())? I mean, when is this called such that we need to resume the ha->dev?
userspace SMP requests (sas_smp_handler): ses/expander tools query
the topology at any time and nothing else holds the host awake, so
the host may have autosuspended; the resume also re-registers the
devices (the PHYE -> dev_found path runs inside the resume), so the
IO can proceed. This is the case 0da7ca4c4fd9 was written for.
The other callers either cannot run while the host is suspended or
already hold their own PM reference, so they never exercise the
resume path.
So then could the RPM resume calls be moved higher up, like at the smp_execute_task_sg() callsite? Would that work?
Yes, that works and is cleaner:
@@ smp_execute_task_sg()
- pm_runtime_get_sync(ha->dev);
+ /*
+ * Non-blocking: a sync resume here would deadlock against
+ * sas_drain_work() during HA resume.
+ */
+ pm_runtime_get_noresume(ha->dev);
...
- pm_runtime_put_sync(ha->dev);
+ pm_runtime_put(ha->dev);
@@ sas_smp_handler()
+ /*
+ * The host may have autosuspended. This is the only
+ * smp_execute_task_sg() caller which can find it suspended,
+ * so resume it here.
+ */
+ ret = pm_runtime_resume_and_get(dev->port->ha->dev);
+ if (ret)
+ goto out;
+
ret = smp_execute_task_sg(dev, job->request_payload.sg_list,
job->reply_payload.sg_list);
+
+ pm_runtime_put(dev->port->ha->dev);
The get_noresume() is kept for the discovery path. Discovery work
normally runs inside an event worker's PM reference, taken at
sas_notify_port_event() notify time and held until the handler has
flushed the disco queue. The exception is sas_rediscover_ex_phy(),
which requeues DISCE_REVALIDATE_DOMAIN from within the revalidation
worker itself: flush_workqueue() does not wait for work items queued
during execution, so that chained revalidation runs with no outer PM
reference and its SMP could race autosuspend.
We confirmed this race by fault injection on expander-attached SATA:
with the usage hold removed, the host autosuspended while the chained
revalidation was issuing its SMP and the command timed out against
the suspending host. With the get_noresume() in place, the same test
shows the suspend attempt being caught by the existing usage check in
_suspend_v3_hw() ("PM suspend: host status cannot be suspended") and
aborted, so the revalidation completes with the host active.
For the original deadlock, the SMP which the resume path itself
issues (sas_ata_hard_reset() -> sas_phy_reset() ->
sas_smp_phy_control()) now completes normally during HA resume, and
BSG SMP queries against an autosuspended host resume it correctly.
The check which you initially proposed for testing SAS_HA_RESUMING looks racy.
Right, the check is gone entirely with this restructure.
Thanks for the guidance. I'll send v4.
Thanks,
Xingui
.