Re: [PATCH v3] scsi: libsas: Fix SMP IO deadlock during HA resume
From: John Garry
Date: Mon Sep 21 2026 - 11:12:04 EST
On 9/21/26 13:21, yangxingui wrote:
Hi, John
On 2026/9/21 19:37, John Garry wrote:
On 9/18/26 08:03, Xingui Yang wrote:
When the controller resumes, sas_resume_ha() -> sas_drain_work()
processes the DISCE_RESUME work, which restores the ATA ports through
the libata error handler (ata_sas_port_resume() requests ATA_EH_RESET)
and waits for it in sas_ata_flush_pm_eh(). For an expander-attached
ATA device the hard reset in that recovery is an SMP PHY CONTROL
command sent to the expander:
ata_eh_recover() -> ata_eh_reset() -> sas_ata_hard_reset()
-> lldd_I_T_nexus_reset() -> sas_phy_reset()
-> sas_smp_phy_control() -> smp_execute_task_sg()
This seems like an obvious issue. How come it was not found earlier? It is apparently fixing a patch which is 5 years old.
The deadlock was not reachable for most of those 5 years - it is a
regression of 3dbbbf656b85 ("scsi: libsas: Fix HA resume deadlock and
hisi_sas disk-wake race"), which restored the draining sas_resume_ha()
in hisi_sas.
0da7ca4c4fd9 was part of the same 2021 series as fbefe22811c3 ("Don't
always drain event workqueue for HA resume"), which switched hisi_sas
to the non-draining sas_resume_ha_no_sync(). The two were designed
together: without the drain, nothing in the resume path waits on the
ATA error handling, so the pm_runtime_get_sync() in
smp_execute_task_sg() could at most delay the EH until the resume
callback returned - no circular wait.
3dbbbf656b85 restored the drain to fix the disk-wake race (the
controller autosuspending while disks were still waking up), which for
the first time made the resume wait on the ATA EH - and with it the
SMP PHY CONTROL for an expander-attached ATA device. So the deadlock
window is really since 3dbbbf656b85, not since 0da7ca4c4fd9.
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?
I 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.
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()?
+ 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?