Re: [PATCH v3] scsi: libsas: fix HA resume deadlock and hisi_sas disk-wake race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing `sas_resume_ha_no_sync()` and restoring `sas_resume_ha()` reintroduces a resume deadlock for hardware events like `PHYE_LOSS_OF_SIGNAL`.
--

--- Patch [1]: [PATCH v3] scsi: libsas: fix HA resume deadlock and hisi_sas disk-wake race ---
commit f62439ea23dd5d0db3e1f9ebd2572dcb77f2890b
Author: Xingui Yang <yangxingui@huawei.com>

scsi: libsas: fix HA resume deadlock and hisi_sas disk-wake race

This patch moves the PHYE_RESUME_TIMEOUT notification after sas_drain_work()
to fix a deadlock in the resume path. It then removes sas_resume_ha_no_sync()
and restores the draining sas_resume_ha() in hisi_sas.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 213d5b5dea94f..8a2500993e19d 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
[ ... ]
> @@ -5261,15 +5261,7 @@ static int _resume_v3_hw(struct device *device)
>  		return rc;
>  	}
>  	phys_init_v3_hw(hisi_hba);
> -
> -	/*
> -	 * If a directly-attached disk is removed during suspend, a deadlock
> -	 * may occur, as the PHYE_RESUME_TIMEOUT processing will require the
> -	 * hisi_hba->device to be active, which can only happen when resume
> -	 * completes. So don't wait for the HA event workqueue to drain upon
> -	 * resume.
> -	 */
> -	sas_resume_ha_no_sync(sha);
> +	sas_resume_ha(sha);
>  	clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);
>  
>  	dev_warn(dev, "end of resuming controller\n");

> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index 0bec236f0fb59..c3f3d05b46dea 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
[ ... ]
> @@ -426,6 +426,23 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
>  		dev_info(ha->dev, "waiting up to 25 seconds for %d phy%s to resume\n",
>  			 i, i > 1 ? "s" : "");
>  	wait_event_timeout(ha->eh_wait_q, phys_suspended(ha) == 0, tmo);
> +
> +	/*
> +	 * All phys are back up or timed out. Turn on I/O and drain
> +	 * pending work.
> +	 */
> +	scsi_unblock_requests(ha->shost);
> +	sas_drain_work(ha);

[Severity: High]
Could this reintroduce a resume deadlock for hardware events like
PHYE_LOSS_OF_SIGNAL?

During host resume in _resume_v3_hw(), hardware is re-enabled via
phys_init_v3_hw() just before calling sas_resume_ha(). If a hardware event
fires during the 25-second wait window, the interrupt handler will queue it.

Looking at sas_defer_event() in sas_event.c, events for suspended phys are
not deferred:

drivers/scsi/libsas/sas_event.c:sas_defer_event() {
    ...
    if (test_bit(SAS_HA_RESUMING, &ha->state) && !phy->suspended) {
        ... deferred = true;
    }
    ...
}

Since it isn't deferred, the event worker executes it immediately:
sas_phye_loss_of_signal() -> sas_deform_port() -> sas_destruct_devices() ->
sas_rphy_delete() -> device_del().

Because hisi_sas uses device links with DL_FLAG_PM_RUNTIME, device_del()
triggers a synchronous resume (pm_runtime_get_sync()), which blocks waiting
for the host to finish resuming.

At the same time, this sas_drain_work() call blocks waiting for the event
worker to finish, resulting in a deadlock. Could a hardware event fire
during this wait and cause this scenario?

> +
> +	/*
> +	 * Send PHYE_RESUME_TIMEOUT after sas_drain_work(). The handler
> +	 * calls sas_deform_port() -> sas_destruct_devices(), which removes
> +	 * SCSI devices and, for LLDDs using device_link() PM sync, waits
> +	 * for the host to be runtime-active. Sending it before the drain
> +	 * would deadlock: the drain waits for the handler, the handler
> +	 * waits for host resume, and host resume waits for the drain to
> +	 * finish.
> +	 */
>  	for (i = 0; i < ha->num_phys; i++) {
>  		struct asd_sas_phy *phy = ha->sas_phy[i];

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