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

[yangxingui <yangxingui@huawei.com>]

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