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

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

[Xingui Yang]

Commit fbefe22811c3140 ("scsi: libsas: Don't always drain event workqueue
for HA resume") introduced sas_resume_ha_no_sync() to avoid a deadlock: the
PHYE_RESUME_TIMEOUT handler, running on the HA event workqueue, calls
sas_deform_port() -> sas_destruct_devices(), which removes SCSI devices and
waits for the host to become runtime-active. But the host cannot resume
until sas_resume_ha() -> sas_drain_work() returns, and the drain is blocked
on that very handler.

However skipping the drain reintroduces a race: hisi_sas returns from
resume before all PHY UP work and libsas discovery work finish. The
controller may then autosuspend while disks are still waking up. The disks
issue IO to a suspended controller, the IO fails, and the disks get
disabled.

Fix the deadlock at its source by moving the PHYE_RESUME_TIMEOUT
notification to after sas_drain_work(). By then the host resume is about to
complete, so device removal through device_link no longer blocks on the
resume and the cycle is broken.

With the deadlock gone, restore sas_resume_ha() (the draining variant) in
hisi_sas and remove sas_resume_ha_no_sync().

Fixes: fbefe22811c3140 ("scsi: libsas: Don't always drain event workqueue for HA resume")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  9 +-------
 drivers/scsi/libsas/sas_init.c         | 32 ++++++++++++++------------
 include/scsi/libsas.h                  |  1 -
 3 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 0687bdefcd63..c8673ae4e472 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -5263,14 +5263,7 @@ static int _resume_v3_hw(struct device *device)
 	}
 	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 0bec236f0fb5..624850f1483d 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -410,7 +410,7 @@ static void sas_resume_insert_broadcast_ha(struct sas_ha_struct *ha)
 	}
 }
 
-static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
+static void _sas_resume_ha(struct sas_ha_struct *ha)
 {
 	const unsigned long tmo = msecs_to_jiffies(25000);
 	int i;
@@ -426,6 +426,21 @@ 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 so we can
+	 * flush out disks that did not return
+	 */
+	scsi_unblock_requests(ha->shost);
+	sas_drain_work(ha);
+
+	/* 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];
 
@@ -436,12 +451,6 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 		}
 	}
 
-	/* all phys are back up or timed out, turn on i/o so we can
-	 * flush out disks that did not return
-	 */
-	scsi_unblock_requests(ha->shost);
-	if (drain)
-		sas_drain_work(ha);
 	clear_bit(SAS_HA_RESUMING, &ha->state);
 
 	sas_queue_deferred_work(ha);
@@ -453,17 +462,10 @@ static void _sas_resume_ha(struct sas_ha_struct *ha, bool drain)
 
 void sas_resume_ha(struct sas_ha_struct *ha)
 {
-	_sas_resume_ha(ha, true);
+	_sas_resume_ha(ha);
 }
 EXPORT_SYMBOL(sas_resume_ha);
 
-/* A no-sync variant, which does not call sas_drain_ha(). */
-void sas_resume_ha_no_sync(struct sas_ha_struct *ha)
-{
-	_sas_resume_ha(ha, false);
-}
-EXPORT_SYMBOL(sas_resume_ha_no_sync);
-
 void sas_suspend_ha(struct sas_ha_struct *ha)
 {
 	int i;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 163f23c92b41..36d4cb567837 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -680,7 +680,6 @@ extern int sas_register_ha(struct sas_ha_struct *);
 extern int sas_unregister_ha(struct sas_ha_struct *);
 extern void sas_prep_resume_ha(struct sas_ha_struct *sas_ha);
 extern void sas_resume_ha(struct sas_ha_struct *sas_ha);
-extern void sas_resume_ha_no_sync(struct sas_ha_struct *sas_ha);
 extern void sas_suspend_ha(struct sas_ha_struct *sas_ha);
 
 int sas_phy_reset(struct sas_phy *phy, int hard_reset);
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware initialization race: scsi_unblock_requests() allows I/O dispatch to uninitialized hardware in _resume_v3_hw().
--

commit 9e820865310a614d1e3c143d85daf43eae7329e2
Author: Xingui Yang <yangxingui@huawei.com>

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

This commit fixes a resume deadlock by moving the PHYE_RESUME_TIMEOUT
notification to occur after sas_drain_work(). It restores the standard
sas_resume_ha() behavior in hisi_sas and removes the no-sync variant.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -5262,14 +5262,7 @@ static int _resume_v3_hw(struct device *device)
>  
> [ ... ]
>  
>  	scsi_unblock_requests(shost);
>  	clear_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags);
>  
>  	sas_prep_resume_ha(sha);
>  	rc = hw_init_v3_hw(hisi_hba);

[Severity: High]
This is a pre-existing issue, but I noticed a potential hardware
initialization race here.

Does unblocking requests before hw_init_v3_hw() completes allow pending I/O
to be dispatched to uninitialized hardware?

During PM resume, _resume_v3_hw() calls scsi_unblock_requests() and clears
HISI_SAS_REJECT_CMD_BIT. Any blocked block-layer requests could immediately
be dispatched to hisi_sas_queue_command().

These requests would be written to the controller hardware before it has
been reset or initialized by hw_init_v3_hw().

Could this lead to hardware corruption, SErrors, or system lockups on
standard suspend/resume cycles if there is pending I/O?

>  	if (rc) {
>  		scsi_remove_host(shost);
>  		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);

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