Re: [PATCH v3] scsi: libsas: Fix SMP IO deadlock during HA resume

[John Garry <john.garry@linux.dev>]

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?

> 
> 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.
> 
I am just wondering why this needs to be fixed so many times...


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?