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

[yangxingui <yangxingui@huawei.com>]

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
.