[PATCH 0/3] scsi: hisi_sas: Some Misc fixes

[PATCH 0/3] scsi: hisi_sas: Some Misc fixes

[Xingui Yang]

This series contains three independent fixes for the hisi_sas driver.

Patch 1 fixes incorrect delay values introduced by a previous
magic-number cleanup commit. Three delay sites were changed to use
a single macro (value 100) which did not match their original values,
causing increased boot and shutdown time.

Patch 2 clears stale PHY error counts on phyup so that error detection
is not misled by intermediate errors generated during link
establishment.

Patch 3 fixes spinup failures of directly-attached SAS HDDs that power
up in the Active_Wait state and wait for a NOTIFY(ENABLE SPINUP)
primitive before becoming ready: parse the sense data in the slot
completion path and send the primitive from deferred work.

Xingui Yang (3):
  scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup
  scsi: hisi_sas: Clear PHY error counts on phyup
  scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait
    state

 drivers/scsi/hisi_sas/hisi_sas.h       |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 38 +++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 42 +++++++++++++++++++-------
 5 files changed, 78 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/3] scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

[Xingui Yang]

Commit 4ca7fe99fc84 ("scsi: hisi_sas: Use macro instead of magic
number") replaced several delay constants with a single macro of
value 100. However, three sites originally used different values:

  - reset_hw_v3_hw:     udelay(50) -> udelay(100) [doubled]
  - disable_phy_v3_hw:  mdelay(50) -> mdelay(100) [doubled]
  - disable_host_v3_hw: mdelay(10) -> mdelay(100) [10x longer]

The overly long delays on the PHY disable and host shutdown paths
increase boot and shutdown time for all local PHYs.

Add separate macros for each delay to restore the original values
while still avoiding magic numbers.

Fixes: 4ca7fe99fc84 ("scsi: hisi_sas: Use macro instead of magic number")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 213d5b5dea94..dcb1c65113de 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -547,6 +547,9 @@ struct hisi_sas_err_record_v3 {
 #define IRQ_AXI_INDEX 11
 
 #define DELAY_FOR_RESET_HW 100
+#define STOP_PHY_DELAY_US 50
+#define DISABLE_PHY_DELAY_MS 50
+#define DISABLE_HOST_PHY_DELAY_MS 10
 #define HDR_SG_MOD 0x2
 #define LUN_SIZE 8
 #define ATTR_PRIO_REGION 9
@@ -983,7 +986,7 @@ static int reset_hw_v3_hw(struct hisi_hba *hisi_hba)
 
 	/* Disable all of the PHYs */
 	hisi_sas_stop_phys(hisi_hba);
-	udelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
+	udelay(STOP_PHY_DELAY_US);
 
 	/* Ensure axi bus idle */
 	ret = hisi_sas_read32_poll_timeout(AXI_CFG, val, !val,
@@ -1072,7 +1075,7 @@ static void disable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 	cfg &= ~PHY_CFG_ENA_MSK;
 	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
 
-	mdelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
+	mdelay(DISABLE_PHY_DELAY_MS);
 
 	state = hisi_sas_read32(hisi_hba, PHY_STATE);
 	if (state & BIT(phy_no)) {
@@ -2755,7 +2758,7 @@ static int disable_host_v3_hw(struct hisi_hba *hisi_hba)
 
 	hisi_sas_stop_phys(hisi_hba);
 
-	mdelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
+	mdelay(DISABLE_HOST_PHY_DELAY_MS);
 
 	reg_val = hisi_sas_read32(hisi_hba, AXI_MASTER_CFG_BASE +
 				  AM_CTRL_GLOBAL);
-- 
2.43.0

Re: [PATCH 1/3] scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware callback executes a 50ms busy-wait delay while holding an IRQ-disabled spinlock, severely degrading system latency and risking watchdog timeouts.
--

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

scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

This commit fixes incorrect delay values in the hisi_sas v3 hardware driver
that were introduced during a magic-number cleanup. It adds separate macros
for each delay site to restore their original timings of 50us, 50ms, and
10ms respectively.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 8a2500993e19d..9c363e2373538 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c

[ ... ]

> @@ -1072,7 +1075,7 @@ static void disable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  	cfg &= ~PHY_CFG_ENA_MSK;
>  	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
>  
> -	mdelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
> +	mdelay(DISABLE_PHY_DELAY_MS);

[Severity: High]
This is a pre-existing issue, but is it safe to use a 50ms busy-wait delay
here while holding an IRQ-disabled spinlock?

Looking at hisi_sas_phy_enable() in drivers/scsi/hisi_sas/hisi_sas_main.c,
the hardware callback is invoked with interrupts disabled:

hisi_sas_phy_enable() {
        ...
        spin_lock_irqsave(&phy->lock, flags);
        if (enable) {
                ...
        } else {
                phy->negotiated_linkrate = SAS_PHY_DISABLED;
                hisi_hba->hw->phy_disable(hisi_hba, phy_no);
        }
        ...
}

Since hisi_hba->hw->phy_disable resolves to disable_phy_v3_hw(), this will
block the CPU with interrupts disabled for 50ms. When looping over multiple
PHYs in hisi_sas_stop_phys(), this delay can multiply, potentially causing
severe scheduling latency, dropped interrupts, or triggering softlockup
panics.

Could this delay be moved outside the spinlock, or could we use a sleepable
context here instead?

[ ... ]

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

[PATCH 2/3] scsi: hisi_sas: Clear PHY error counts on phyup

[Xingui Yang]

Some disks may generate link errors during link establishment but still phy
up successfully. These intermediate error counts are not cleared after
phyup, leaving stale data for error detection. Clear them on phyup and only
print non-zero error counts to reduce log noise.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 30 +++++++++++++++++++-------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index dcb1c65113de..0cdf72f3dde9 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -1061,6 +1061,19 @@ static void enable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
 }
 
+static void clear_phy_err_cnt_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
+{
+	static const u32 msk = BIT(CHL_INT2_RX_DISP_ERR_OFF) |
+			       BIT(CHL_INT2_RX_CODE_ERR_OFF) |
+			       BIT(CHL_INT2_RX_INVLD_DW_OFF);
+
+	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
+	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DISP_ERR);
+	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_CODE_ERR);
+
+	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2, msk);
+}
+
 static void disable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 {
 	u32 cfg = hisi_sas_phy_read32(hisi_hba, phy_no, PHY_CFG);
@@ -1085,11 +1098,7 @@ static void disable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 
 	udelay(1);
 
-	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
-	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DISP_ERR);
-	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_CODE_ERR);
-
-	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2, msk);
+	clear_phy_err_cnt_v3_hw(hisi_hba, phy_no);
 	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK, irq_msk);
 }
 
@@ -1673,6 +1682,8 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
 	phy->phy_attached = 1;
 	spin_unlock(&phy->lock);
 
+	clear_phy_err_cnt_v3_hw(hisi_hba, phy_no);
+
 	/*
 	 * Call pm_runtime_get_noresume() which pairs with
 	 * hisi_sas_phyup_pm_work() -> pm_runtime_put_sync().
@@ -1926,15 +1937,18 @@ static void handle_chl_int2_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 
 		phy_get_events_v3_hw(hisi_hba, phy_no);
 
-		if (irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF))
+		if ((irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF)) &&
+		    sphy->invalid_dword_count > 0)
 			dev_info(dev, "phy%d invalid dword cnt:   %u\n", phy_no,
 				 sphy->invalid_dword_count);
 
-		if (irq_value & BIT(CHL_INT2_RX_CODE_ERR_OFF))
+		if ((irq_value & BIT(CHL_INT2_RX_CODE_ERR_OFF)) &&
+		    phy->code_violation_err_count > 0)
 			dev_info(dev, "phy%d code violation cnt:  %u\n", phy_no,
 				 phy->code_violation_err_count);
 
-		if (irq_value & BIT(CHL_INT2_RX_DISP_ERR_OFF))
+		if ((irq_value & BIT(CHL_INT2_RX_DISP_ERR_OFF)) &&
+		    sphy->running_disparity_error_count > 0)
 			dev_info(dev, "phy%d disparity error cnt: %u\n", phy_no,
 				 sphy->running_disparity_error_count);
 	}
-- 
2.43.0

Re: [PATCH 2/3] scsi: hisi_sas: Clear PHY error counts on phyup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Log noise suppression logic incorrectly uses cumulative software counters, failing to suppress logs after the first error.
- [High] The patch fails to clear intermediate link establishment errors because the interrupt handler processes them before phyup.
- [Medium] Clearing CHL_INT2 interrupt statuses in phy_up_v3_hw races with the channel interrupt handler, causing spurious warnings.
- [Medium] Concurrent lockless reads of read-to-clear hardware registers cause non-deterministic error accounting.
--

commit 7c1564a2ad8d2d14ea2f5907d02dc785640e35d4
Author: Xingui Yang <yangxingui@huawei.com>

scsi: hisi_sas: Clear PHY error counts on phyup

This commit clears PHY intermediate error counts generated during link
establishment upon a successful phyup. It aims to prevent logging of stale
error data by clearing hardware counters and only printing errors when they
are non-zero.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 9c363e2373538..f7e259aff3df1 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -1061,6 +1061,19 @@ static void enable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
>  }
>  
> +static void clear_phy_err_cnt_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
> +{
> +	static const u32 msk = BIT(CHL_INT2_RX_DISP_ERR_OFF) |
> +			       BIT(CHL_INT2_RX_CODE_ERR_OFF) |
> +			       BIT(CHL_INT2_RX_INVLD_DW_OFF);
> +
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DISP_ERR);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_CODE_ERR);

[Severity: Medium]
Could these lockless reads of read-to-clear hardware registers cause
non-deterministic error accounting?

If this runs concurrently with the accounting reads in phy_get_events_v3_hw()
from the interrupt handler or sysfs, it seems one of the readers will consume
the register and get zero, leading to lost or corrupted statistics.

> +
> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2, msk);
> +}

[Severity: Medium]
Will clearing the CHL_INT2 interrupt statuses here race with the channel
interrupt handler?

When phy_up_v3_hw() calls this function, it writes to CHL_INT2 without
synchronization or masking. If handle_chl_int2_v3_hw() is executing
concurrently on another CPU, it might read CHL_INT2 immediately after it was
cleared. This would result in a read of zero and trigger the spurious warning:
"channel int 2 received with status bits cleared".

[ ... ]

> @@ -1673,6 +1682,8 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
>  	phy->phy_attached = 1;
>  	spin_unlock(&phy->lock);
>  
> +	clear_phy_err_cnt_v3_hw(hisi_hba, phy_no);
> +

[Severity: High]
Does this clear the hardware counters too late in the process?

Since the CHL_INT2 error interrupts are unmasked by default during link
establishment, any intermediate error will immediately trigger an interrupt.
The handle_chl_int2_v3_hw() handler will process and log these errors before
phy_up_v3_hw() executes this cleanup, which appears to miss the goal of
suppressing those intermediate logs.

[ ... ]

> @@ -1926,15 +1937,18 @@ static void handle_chl_int2_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  
>  		phy_get_events_v3_hw(hisi_hba, phy_no);
>  
> -		if (irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF))
> +		if ((irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF)) &&
> +		    sphy->invalid_dword_count > 0)

[Severity: High]
Does checking the cumulative software counter (sphy->invalid_dword_count)
break the log suppression logic?

Once a single link error occurs, this lifetime counter remains permanently
greater than zero. Subsequent link events will then evaluate this condition
to true and print the stale lifetime total, even if the new hardware error
count is zero. Should this be checking a newly read hardware count instead?

>  			dev_info(dev, "phy%d invalid dword cnt:   %u\n", phy_no,
>  				 sphy->invalid_dword_count);

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

[PATCH 3/3] scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

[Xingui Yang]

SAS HDDs powered up with RNOT=1 enter Active_Wait/Idle_Wait state and
return NOT_READY with ASC/ASCQ=0x04/0x11, causing indefinite mid-layer
retries and disk spinup failure.

Parse sense data in the driver's slot completion path using
scsi_normalize_sense(). When the spinup-notify sense is detected on a
directly-attached SSP device, queue HISI_PHYE_SPINUP_NOTIFY work to the
ordered workqueue, which defers sl_notify_ssp() (containing msleep) to
process context and serializes SL_CONTROL register access.

Fixes: 60b4a5ee9034 ("scsi: hisi_sas: add v3 cq interrupt handler")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h       |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 38 ++++++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  3 ++
 5 files changed, 50 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 1323ed8aa717..d48ba3747b81 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -163,6 +163,7 @@ enum hisi_sas_phy_event {
 	HISI_PHYE_PHY_UP   = 0U,
 	HISI_PHYE_LINK_RESET,
 	HISI_PHYE_PHY_UP_PM,
+	HISI_PHYE_SPINUP_NOTIFY,
 	HISI_PHYES_NUM,
 };
 
@@ -689,4 +690,6 @@ extern void hisi_sas_sync_cqs(struct hisi_hba *hisi_hba);
 extern void hisi_sas_sync_poll_cqs(struct hisi_hba *hisi_hba);
 extern void hisi_sas_controller_reset_prepare(struct hisi_hba *hisi_hba);
 extern void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba);
+extern void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
+				   struct sas_task *task);
 #endif
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 944ce19ae2fc..1cb578e6ae59 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -996,10 +996,22 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
 	pm_runtime_put_sync(dev);
 }
 
+static void hisi_sas_spinup_notify_work(struct work_struct *work)
+{
+	struct hisi_sas_phy *phy =
+		container_of(work, typeof(*phy), works[HISI_PHYE_SPINUP_NOTIFY]);
+	struct hisi_hba *hisi_hba = phy->hisi_hba;
+	int phy_no = phy->sas_phy.id;
+
+	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
+	dev_info(hisi_hba->dev, "spinup notify primitive on phy%d\n", phy_no);
+}
+
 static const work_func_t hisi_sas_phye_fns[HISI_PHYES_NUM] = {
 	[HISI_PHYE_PHY_UP] = hisi_sas_phyup_work,
 	[HISI_PHYE_LINK_RESET] = hisi_sas_linkreset_work,
 	[HISI_PHYE_PHY_UP_PM] = hisi_sas_phyup_pm_work,
+	[HISI_PHYE_SPINUP_NOTIFY] = hisi_sas_spinup_notify_work,
 };
 
 bool hisi_sas_notify_phy_event(struct hisi_sas_phy *phy,
@@ -1639,6 +1651,32 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
 }
 EXPORT_SYMBOL_GPL(hisi_sas_controller_reset_done);
 
+#define ASC_LUN_NOT_READY		0x04
+#define ASCQ_NOTIFY_SPINUP_REQUIRED	0x11
+void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
+			    struct sas_task *task)
+{
+	struct task_status_struct *ts = &task->task_status;
+	struct domain_device *dev = task->dev;
+	struct scsi_sense_hdr sshdr;
+	struct sas_phy *local_phy;
+	struct hisi_sas_phy *phy;
+
+	if (!scsi_normalize_sense(ts->buf, ts->buf_valid_size, &sshdr))
+		return;
+
+	if (sshdr.sense_key != NOT_READY ||
+	    sshdr.asc != ASC_LUN_NOT_READY ||
+	    sshdr.ascq != ASCQ_NOTIFY_SPINUP_REQUIRED)
+		return;
+
+	local_phy = sas_get_local_phy(dev);
+	phy = &hisi_hba->phy[local_phy->number];
+	hisi_sas_notify_phy_event(phy, HISI_PHYE_SPINUP_NOTIFY);
+	sas_put_local_phy(local_phy);
+}
+EXPORT_SYMBOL_GPL(hisi_sas_spinup_notify);
+
 static int hisi_sas_controller_prereset(struct hisi_hba *hisi_hba)
 {
 	if (!hisi_hba->hw->soft_reset)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index fa94d7110714..9d89db6e8716 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1272,6 +1272,9 @@ static void slot_complete_v1_hw(struct hisi_hba *hisi_hba,
 				&status_buffer->iu[0];
 
 		sas_ssp_task_response(dev, task, iu);
+		if (ts->stat == SAS_SAM_STAT_CHECK_CONDITION &&
+		    !dev_parent_is_expander(device))
+			hisi_sas_spinup_notify(hisi_hba, task);
 		break;
 	}
 	case SAS_PROTOCOL_SMP:
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index f3516a0611dd..dd48626db196 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2427,6 +2427,9 @@ static void slot_complete_v2_hw(struct hisi_hba *hisi_hba,
 				&status_buffer->iu[0];
 
 		sas_ssp_task_response(dev, task, iu);
+		if (ts->stat == SAS_SAM_STAT_CHECK_CONDITION &&
+		    !dev_parent_is_expander(device))
+			hisi_sas_spinup_notify(hisi_hba, task);
 		break;
 	}
 	case SAS_PROTOCOL_SMP:
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 0cdf72f3dde9..97d848885575 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2449,6 +2449,9 @@ static void slot_complete_v3_hw(struct hisi_hba *hisi_hba,
 			sizeof(struct hisi_sas_err_record);
 
 		sas_ssp_task_response(dev, task, iu);
+		if (ts->stat == SAS_SAM_STAT_CHECK_CONDITION &&
+		    !dev_parent_is_expander(device))
+			hisi_sas_spinup_notify(hisi_hba, task);
 		break;
 	}
 	case SAS_PROTOCOL_SMP: {
-- 
2.43.0

Re: [PATCH 3/3] scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

[sashiko-bot]

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

New issues:
- [High] The asynchronous work `hisi_sas_spinup_notify_work` accesses MMIO registers without acquiring a runtime PM reference, potentially accessing unclocked hardware and causing bus faults.

Pre-existing issues:
- [Critical] Integer overflow in `min_t` causes a massive out-of-bounds `memcpy` when processing malicious SAS sense data.
- [High] Read-modify-write race on the `SL_CONTROL` register between process context and hardirq context can lead to lost hardware state updates.
- [High] Race condition in suspend path allows hardware access after device is disabled due to improper `flush_workqueue()` ordering.
--

commit 02edb194a91901ddad40e4d837a58a4131b5ffa4
Author: Xingui Yang <yangxingui@huawei.com>

scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

This commit fixes a spinup failure for SAS SSP devices powered up with RNOT=1.
It parses sense data in the driver's slot completion path and queues a deferred
work item to notify the phy to spin up the device.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..1cb578e6ae593 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -996,10 +996,22 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
>  	pm_runtime_put_sync(dev);
>  }
>  
> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
> +{
> +	struct hisi_sas_phy *phy =
> +		container_of(work, typeof(*phy), works[HISI_PHYE_SPINUP_NOTIFY]);
> +	struct hisi_hba *hisi_hba = phy->hisi_hba;
> +	int phy_no = phy->sas_phy.id;
> +
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);

[Severity: High]
Could this MMIO access happen while the device is in a suspended state?

When hisi_sas_spinup_notify() queues the HISI_PHYE_SPINUP_NOTIFY work, it
does not take a runtime PM reference on the HBA. Once the completed SAS task
finishes, the block layer can release its runtime PM reference on the device.

If the system is under load and the work execution is delayed beyond the
autosuspend timeout, the host controller might enter runtime suspend. When
hisi_sas_spinup_notify_work() eventually executes, it calls
hisi_hba->hw->sl_notify_ssp(), which directly reads and writes hardware
registers.

Would it be safer to wrap this hardware access with pm_runtime_get_sync()
and pm_runtime_put_sync() to prevent synchronous bus faults or invalid
reads on unclocked hardware?

[Severity: High]
This is a pre-existing issue, but does sl_notify_ssp() contain a
read-modify-write race with hardirq contexts?

For example, in sl_notify_ssp_v3_hw() (called here via workqueue in process
context), the code performs a lockless read-modify-write on the SL_CONTROL
register:

sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
sl_control |= SL_CONTROL_NOTIFY_EN_MSK;
hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);

However, phy_down_v3_hw() (called from the int_phy_up_down_bcast_v3_hw
hardirq handler) also performs a lockless read-modify-write on the same
SL_CONTROL register. Since sl_notify_ssp_v3_hw() doesn't disable interrupts
or hold a spinlock, can the interrupt fire exactly between its read and
write, causing the interrupt's modification to be completely overwritten
and lost?

> +	dev_info(hisi_hba->dev, "spinup notify primitive on phy%d\n", phy_no);
> +}
> +
>  static const work_func_t hisi_sas_phye_fns[HISI_PHYES_NUM] = {
>  	[HISI_PHYE_PHY_UP] = hisi_sas_phyup_work,
>  	[HISI_PHYE_LINK_RESET] = hisi_sas_linkreset_work,
>  	[HISI_PHYE_PHY_UP_PM] = hisi_sas_phyup_pm_work,
> +	[HISI_PHYE_SPINUP_NOTIFY] = hisi_sas_spinup_notify_work,

[Severity: High]
This is a pre-existing issue, but could there be a race condition in the
suspend path that executes newly queued work on disabled hardware?

In _suspend_v3_hw(), the workqueue is flushed before threaded IRQs are
silenced:

flush_workqueue(hisi_hba->wq);
interrupt_disable_v3_hw(hisi_hba);

Because the driver uses threaded IRQs, an interrupt could have already woken
the IRQ thread before interrupt_disable_v3_hw() runs. The IRQ thread (calling
slot_complete_v3_hw()) can queue new work items like HISI_PHYE_SPINUP_NOTIFY
to hisi_hba->wq after the flush_workqueue() call has returned.

Will these newly queued works then execute and access hardware registers
concurrently with or after disable_host_v3_hw(), leading to illegal
accesses on disabled or suspended hardware?

[ ... ]

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index fa94d7110714f..9d89db6e8716e 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1272,6 +1272,9 @@ static void slot_complete_v1_hw(struct hisi_hba *hisi_hba,
>  				&status_buffer->iu[0];
>  
>  		sas_ssp_task_response(dev, task, iu);

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a potential integer
overflow leading to a buffer overflow when sas_ssp_task_response() processes
malicious SAS sense data?

In sas_ssp_task_response() (called here), iu->sense_data_len is converted
to CPU byte order and bounded using min_t():

tstat->buf_valid_size =
    min_t(int, SAS_STATUS_BUF_SIZE,
          be32_to_cpu(iu->sense_data_len));

Because both arguments are cast to int, if a malicious or malformed device
provides a sense_data_len like 0xFFFFFFFF, the cast results in -1. Since
-1 < 96, min_t() returns -1.

When this -1 is passed to the subsequent memcpy(tstat->buf, iu->sense_data,
tstat->buf_valid_size), it is interpreted as a massive size_t value. 
Can this cause an immediate out-of-bounds read from the DMA buffer and a 
massive out-of-bounds write to kernel memory?

> +		if (ts->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> +		    !dev_parent_is_expander(device))
> +			hisi_sas_spinup_notify(hisi_hba, task);
>  		break;
>  	}

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