Re: [PATCH v3] scsi: libsas: Fix SMP IO deadlock during HA resume
From: John Garry
Date: Tue Sep 22 2026 - 06:43:06 EST
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.
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.
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?
The check which you initially proposed for testing SAS_HA_RESUMING looks racy.
log as follow:
[root@localhost ~]# smp_discover /dev/bsg/expander-5\:0
phy 11:U:attached:[5446a2eb02349000:00 t(SSP)] 12 Gbps
phy 16:U:attached:[5001882016000001:00 i(SSP+STP+SMP)] 12 Gbps
phy 17:U:attached:[5001882016000001:01 i(SSP+STP+SMP)] 12 Gbps
phy 18:U:attached:[5001882016000001:02 i(SSP+STP+SMP)] 12 Gbps
phy 19:U:attached:[5001882016000001:03 i(SSP+STP+SMP)] 12 Gbps
phy 24:D:attached:[500e004aaaaaaa1e:24 V i(SSP) t(SSP)] 12 Gbps
[47910.943602] jamy pm_runtime_resume_and_get(ha->dev)
[47910.969905] hisi_sas_v3_hw 0000:74:04.0: resuming from operating state [D0]
[47912.205965] hisi_sas_v3_hw 0000:74:04.0: neither _PS0 nor _PR0 is defined
[47912.213852] hisi_sas_v3_hw 0000:74:04.0: waiting up to 25 seconds for 4 phys to resume
[47912.268896] hisi_sas_v3_hw 0000:74:04.0: phyup: phy0 link_rate=11
[47912.275986] hisi_sas_v3_hw 0000:74:04.0: phyup: phy1 link_rate=11
[47912.276017] hisi_sas_v3_hw 0000:74:04.0: dev[7:2] found
[47912.282976] hisi_sas_v3_hw 0000:74:04.0: phyup: phy2 link_rate=11
[47912.282980] hisi_sas_v3_hw 0000:74:04.0: phyup: phy3 link_rate=11
[47912.303834] hisi_sas_v3_hw 0000:74:04.0: dev[8:1] found
[47912.310296] hisi_sas_v3_hw 0000:74:04.0: dev[9:1] found
[47912.316809] hisi_sas_v3_hw 0000:74:04.0: end of resuming controller
[47912.316817] sas: broadcast received: 0
[47912.324219] sas: REVALIDATING DOMAIN on port 0, pid:282487
[47912.329870] jamy pm_runtime_put(ha->dev)