[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: libsas: concurrent SMP IO to uninitialized hardware during PM resume
--

commit 2357f5bc7ff2aaf7e480b366cb3ce830c9f21616
Author: Xingui Yang <yangxingui@huawei.com>

scsi: libsas: Fix SMP IO deadlock during HA resume
    
This commit replaces pm_runtime_get_sync() with a conditional
pm_runtime_get_noresume() during HA resume to prevent an SMP IO deadlock.
Outside the resume window, it converts to pm_runtime_resume_and_get() and
checks the result.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 811c9eb4fef11..5a8cdd3682fe7 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);

[Severity: High]
Does this disjoint check of the global state flag followed by
pm_runtime_get_noresume() bypass power management synchronization,
potentially enabling concurrent I/Os to race with host hardware
initialization?

For instance, if userspace submits concurrent SMP I/Os via bsg, could
they race with the driver actively resetting the hardware? In
hisi_sas_v3_resume(), scsi_unblock_requests() is called before
sas_prep_resume_ha(). Could this allow concurrent requests to observe
ha_resuming == true and bypass the pm_runtime_resume_and_get() sync,
resulting in I/O submission to uninitialized hardware?

Also, is there a time-of-check to time-of-use race here? If a thread
reads ha_resuming == true, gets preempted while the device resumes and
autosuspends back to RPM_SUSPENDED, could it wake up and use
pm_runtime_get_noresume() to submit I/O to a powered-off device?

> +	} else {
> +		res = pm_runtime_resume_and_get(ha->dev);
> +		if (res)
> +			return res;
> +	}
>  	mutex_lock(&dev->ex_dev.cmd_mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918070307.381207-1-yangxingui@huawei.com?part=1

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

[yangxingui]

On 2026/9/18 15:15, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] scsi: libsas: concurrent SMP IO to uninitialized hardware during PM resume

No SMP command can reach the hardware before it is initialised. During
suspend, sas_suspend_devices() -> sas_notify_lldd_dev_gone() marks
every device gone (hisi_sas_dev_gone() sets SAS_PHY_UNUSED, or NULLs
lldd_dev for non-PM removal), and lldd rejects such commands via 
DEV_IS_GONE() with -ECOMM before any hardware access.
Devices are only re-registered by sas_resume_port() ->
sas_notify_lldd_dev_found(), which runs from the PHY UP events - i.e.
after hw_init_v3_hw() and phys_init_v3_hw() have completed.

log as follow:
[48370.881905] hisi_sas_v3_hw 0000:74:04.0: resuming from operating 
state [D0]
[48371.864031] jamy pm_runtime_get_noresume(ha->dev)
[48371.869751] hisi_sas_v3_hw 0000:74:04.0: task prep: device 19 not ready
[48371.877442] hisi_sas_v3_hw 0000:74:04.0: task exec: failed[-70]!
[48371.884342] sas: executing SMP task failed:-70
[48371.889667] jamy pm_runtime_put(ha->dev)
[48371.903331] jamy pm_runtime_get_noresume(ha->dev)
[48371.908931] hisi_sas_v3_hw 0000:74:04.0: task prep: device 19 not ready
[48371.916628] hisi_sas_v3_hw 0000:74:04.0: task exec: failed[-70]!
[48371.923535] sas: executing SMP task failed:-70
[48371.928877] jamy pm_runtime_put(ha->dev)
[48372.117963] hisi_sas_v3_hw 0000:74:04.0: neither _PS0 nor _PR0 is defined
[48372.126124] hisi_sas_v3_hw 0000:74:04.0: waiting up to 25 seconds for 
4 phys to resume
[48372.180880] hisi_sas_v3_hw 0000:74:04.0: phyup: phy0 link_rate=11
[48372.187994] hisi_sas_v3_hw 0000:74:04.0: phyup: phy1 link_rate=11
[48372.188003] hisi_sas_v3_hw 0000:74:04.0: dev[22:2] found
[48372.195016] hisi_sas_v3_hw 0000:74:04.0: phyup: phy2 link_rate=11
[48372.195019] hisi_sas_v3_hw 0000:74:04.0: phyup: phy3 link_rate=11
[48372.201352] hisi_sas_v3_hw 0000:74:04.0: dev[23:1] found
[48372.221700] hisi_sas_v3_hw 0000:74:04.0: dev[24:1] found
[48372.228037] hisi_sas_v3_hw 0000:74:04.0: end of resuming controller

Thanks,
Xingui

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)

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

[yangxingui]

On 2026/9/22 18:37, John Garry wrote:
> 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.

Understood, we will cover both configurations going forward.

> 
>>>>
>>>> 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.

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;"
>>
>>>
>>> +        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?

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
.