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

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

[Xingui Yang]

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()

For a runtime resume ha->dev is still RPM_RESUMING while the callback
runs, so the pm_runtime_get_sync() in smp_execute_task_sg() blocks
waiting for the resume to complete, but the resume is blocked in
sas_drain_work() waiting for that very SMP IO — a deadlock.

Use pm_runtime_get_noresume() to take the reference while
SAS_HA_RESUMING is set, and pm_runtime_put() to drop it. The hardware
is already initialized by the LLDD before sas_resume_ha() runs.
SAS_HA_RESUMING is also set during a system sleep resume, where the
usage counter is still held from the sleep prepare and the put is
harmless.

Outside of the resume window, convert the pm_runtime_get_sync() call
to pm_runtime_resume_and_get() and check the result, so that an SMP IO
is not submitted to a host whose runtime resume failed (the return
value was previously ignored).

Only hisi_sas enables runtime PM among libsas LLDDs, so other drivers
(pm8001, isci, aic94xx, mvsas) are unaffected.

Fixes: 0da7ca4c4fd9 ("scsi: libsas: Resume host while sending SMP I/Os")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
Changes since v2:
- Drop the reference with pm_runtime_put() instead of
  pm_runtime_put_noidle().

Changes since v1:
- Use pm_runtime_get_noresume()/put_noidle() during HA resume instead
  of skipping the PM reference entirely, so an in-flight SMP IO always
  keeps autosuspend away.
- Convert pm_runtime_get_sync() to pm_runtime_resume_and_get() and
  check the result (pre-existing issue flagged by sashiko).

 drivers/scsi/libsas/sas_expander.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

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);
+		if (res)
+			return res;
+	}
 	mutex_lock(&dev->ex_dev.cmd_mutex);
 	for (retry = 0; retry < 3; retry++) {
 		if (test_bit(SAS_DEV_GONE, &dev->state)) {
@@ -135,7 +149,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
 		}
 	}
 	mutex_unlock(&dev->ex_dev.cmd_mutex);
-	pm_runtime_put_sync(ha->dev);
+	pm_runtime_put(ha->dev);
 
 	BUG_ON(retry == 3 && task != NULL);
 	sas_free_task(task);
-- 
2.43.0

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

[John Garry]

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.

> 
> For a runtime resume ha->dev is still RPM_RESUMING while the callback
> runs, so the pm_runtime_get_sync() in smp_execute_task_sg() blocks
> waiting for the resume to complete, but the resume is blocked in
> sas_drain_work() waiting for that very SMP IO — a deadlock.
> 
> Use pm_runtime_get_noresume() to take the reference while
> SAS_HA_RESUMING is set, and pm_runtime_put() to drop it. The hardware
> is already initialized by the LLDD before sas_resume_ha() runs.
> SAS_HA_RESUMING is also set during a system sleep resume, where the
> usage counter is still held from the sleep prepare and the put is
> harmless.
> 
> Outside of the resume window, convert the pm_runtime_get_sync() call
> to pm_runtime_resume_and_get() and check the result, so that an SMP IO
> is not submitted to a host whose runtime resume failed (the return
> value was previously ignored).
> 
> Only hisi_sas enables runtime PM among libsas LLDDs, so other drivers
> (pm8001, isci, aic94xx, mvsas) are unaffected.
> 
> Fixes: 0da7ca4c4fd9 ("scsi: libsas: Resume host while sending SMP I/Os")
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
> Changes since v2:
> - Drop the reference with pm_runtime_put() instead of
>    pm_runtime_put_noidle().
> 
> Changes since v1:
> - Use pm_runtime_get_noresume()/put_noidle() during HA resume instead
>    of skipping the PM reference entirely, so an in-flight SMP IO always
>    keeps autosuspend away.
> - Convert pm_runtime_get_sync() to pm_runtime_resume_and_get() and
>    check the result (pre-existing issue flagged by sashiko).
> 
>   drivers/scsi/libsas/sas_expander.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> 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);
> +		if (res)
> +			return res;
> +	}
>   	mutex_lock(&dev->ex_dev.cmd_mutex);
>   	for (retry = 0; retry < 3; retry++) {
>   		if (test_bit(SAS_DEV_GONE, &dev->state)) {
> @@ -135,7 +149,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
>   		}
>   	}
>   	mutex_unlock(&dev->ex_dev.cmd_mutex);
> -	pm_runtime_put_sync(ha->dev);
> +	pm_runtime_put(ha->dev);
>   
>   	BUG_ON(retry == 3 && task != NULL);
>   	sas_free_task(task);

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

[yangxingui]

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.

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.

Thanks,
Xingui

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

[John Garry]

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?

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

[yangxingui]

On 2026/9/21 22:05, John Garry wrote:
> 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?
Yes，we do have a full test suite, but it runs per machine topology. The
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.

>>
>> 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...
Aha - each respin addressed a review finding on the PM
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: 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;"

> 
> +        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?
Outside the resume window, the caller that needs the resume is BSG
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.

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)


Thanks,
Xingui
.

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

[John Garry]

On 9/22/26 04:10, yangxingui wrote:
>>>
>>> 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?
> Yes，we do have a full test suite, but it runs per machine topology. The
> 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.

>>>
>>> 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...
> Aha - each respin addressed a review finding on the PM
> 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;"
> 
>>
>> +        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?
> Outside the resume window, the caller that needs the resume is BSG
> 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)