[PATCH v3 0/4] scsi: hisi_sas: Some misc fixes

[PATCH v3 0/4] scsi: hisi_sas: Some misc fixes

[Xingui Yang]

This series contains three independent fixes for the hisi_sas driver,
plus a libsas fix found during review.

Patch 1 fixes an out-of-bounds memcpy in sas_ssp_task_response(): a
device-provided sense_data_len >= 0x80000000 turns negative in the
min_t(int, ...) clamp and becomes a huge memcpy length.

Patch 2 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 3 clears stale PHY error counts on phyup so that error detection
is not misled by intermediate errors generated during link
establishment.

Patch 4 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.

Changes from v2:
Addresses the Sashiko AI review of v2.
- Clear all five error counter registers.
- Use pm_runtime_get_if_active().

Changes from v1:
Addresses the Sashiko AI review of v1.
- Add Patch 1, found in review of the sense parsing of patch 4.
- Clear the counters under phy->lock.
- Take a runtime PM reference for the deferred work

The remaining review findings are pre-existing issues, left for
separate fixes in the future.

Xingui Yang (4):
  scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()
  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  | 43 ++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 ++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  4 ++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 51 ++++++++++++++++++--------
 drivers/scsi/libsas/sas_task.c         |  2 +-
 6 files changed, 90 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v3 1/4] scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()

[Xingui Yang]

The min_t(int, ...) clamp of the device-provided sense_data_len
turns negative for values >= 0x80000000 and is then used as the
memcpy() length, reading and writing out of bounds. Compare as u32
instead. Well-formed responses are unaffected.

Fixes: 366ca51f30de ("[SCSI] libsas: abstract STP task status into a function")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_task.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c
index e9d291007817..2080e5836179 100644
--- a/drivers/scsi/libsas/sas_task.c
+++ b/drivers/scsi/libsas/sas_task.c
@@ -25,7 +25,7 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task,
 	case SAS_DATAPRES_SENSE_DATA:
 		tstat->stat = SAS_SAM_STAT_CHECK_CONDITION;
 		tstat->buf_valid_size =
-			min_t(int, SAS_STATUS_BUF_SIZE,
+			min_t(u32, SAS_STATUS_BUF_SIZE,
 			      be32_to_cpu(iu->sense_data_len));
 		memcpy(tstat->buf, iu->sense_data, tstat->buf_valid_size);
 
-- 
2.43.0

Re: [PATCH v3 1/4] scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: libsas: OOB read in sas_ssp_task_response due to LLDD buffer sizing
- [Medium] scsi: mvsas: info leak in mvs_fill_ssp_resp_iu() due to missing cpu_to_be32
--

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

scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()

This commit changes the min_t clamp of sense_data_len in
sas_ssp_task_response() from int to u32. This prevents negative lengths
from causing out-of-bounds reads and writes during memcpy when the
device provides a value greater than or equal to 0x80000000.

> diff --git a/drivers/scsi/libsas/sas_task.c b/drivers/scsi/libsas/sas_task.c
> index e9d291007817b..2080e5836179b 100644
> --- a/drivers/scsi/libsas/sas_task.c
> +++ b/drivers/scsi/libsas/sas_task.c
> @@ -25,7 +25,7 @@ void sas_ssp_task_response(struct device *dev, struct sas_task *task,
>  	case SAS_DATAPRES_SENSE_DATA:
>  		tstat->stat = SAS_SAM_STAT_CHECK_CONDITION;
>  		tstat->buf_valid_size =
> -			min_t(int, SAS_STATUS_BUF_SIZE,
> +			min_t(u32, SAS_STATUS_BUF_SIZE,
>  			      be32_to_cpu(iu->sense_data_len));

[Severity: Medium]
Does this change inadvertently expose an information leak on little-endian
architectures?

In drivers/scsi/mvsas/mv_sas.c:mvs_fill_ssp_resp_iu(), the driver assigns
the native integer 17 to the __be32 field iu->sense_data_len:

mvs_fill_ssp_resp_iu() {
	...
	iu->sense_data_len = 17;
	...
}

On little-endian architectures, be32_to_cpu() evaluates this as 285212672.
The min_t clamp then evaluates to 96 bytes. Because mvs_set_sense() only
initializes the first 17 bytes, the subsequent memcpy() copies an
additional 79 bytes of uninitialized DMA memory into the task status
buffer.

This uninitialized memory is then copied into the SCSI sense buffer in
drivers/scsi/libsas/sas_scsi_host.c:sas_end_task():

sas_end_task() {
	...
	memcpy(sc->sense_buffer, ts->buf,
	       min(SCSI_SENSE_BUFFERSIZE, ts->buf_valid_size));
	...
}

Could this expose the leaked DMA memory to upper layers and potentially
userspace via the SG_IO ioctl?

>  		memcpy(tstat->buf, iu->sense_data, tstat->buf_valid_size);

[Severity: High]
Can this memcpy read out-of-bounds from the underlying LLDD DMA buffer?

The function clamps the copy length to SAS_STATUS_BUF_SIZE (96 bytes),
but it does not verify if the LLDD actually provided 96 bytes of
physically backed space for iu->sense_data.

For example, in drivers/scsi/pm8001/pm8001_hwi.c:mpi_ssp_completion(),
the ssp_response_iu is embedded in an iomb queue element which is
physically only 64 bytes (or 128 bytes for SPCV). Inside the 64-byte
iomb, the sense_data field starts at offset 44, leaving only 20 bytes
of available space.

If a malicious or malfunctioning SAS target provides a sense_data_len
of 96 or greater, reading 96 bytes here forces an out-of-bounds read
past the end of the iomb queue element.

Could this copy sensitive data from adjacent DMA completion ring
elements (like command tags, statuses, and memory addresses) into the
task status buffer, leading to a kernel memory information leak?

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

[PATCH v3 2/4] 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 8a2500993e19..9c363e237353 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 v3 2/4] 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:
- [High] scsi: hisi_sas: 50ms busy-wait under IRQ-disabled spinlock in phy_disable
--

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

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

Commit 4ca7fe99fc84 ("scsi: hisi_sas: Use macro instead of magic
number") replaced several delay constants with a single macro of
value 100. This commit adds separate macros for each delay to restore
the original values while avoiding magic numbers.

> 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]
Does this execute a 50ms busy-wait under an IRQ-disabled spinlock?

Looking at hisi_sas_phy_enable() in drivers/scsi/hisi_sas/hisi_sas_main.c,
it calls the hardware-specific PHY disable callback while holding the
IRQ-disabled spinlock:

hisi_sas_phy_enable() {
	...
	spin_lock_irqsave(&phy->lock, flags);

	if (enable) {
		...
	} else {
		sphy->negotiated_linkrate = SAS_PHY_DISABLED;
		hisi_hba->hw->phy_disable(hisi_hba, phy_no);
	}
	...
}

Because hisi_hba->hw->phy_disable maps to disable_phy_v3_hw(), this means
the 50ms mdelay() executes with local interrupts disabled.

This will happen on any path that disables a PHY, including
hisi_sas_stop_phys() which iteratively disables all PHYs, compounding the
delay linearly.

Can this be changed to avoid executing a massive busy-wait with interrupts
disabled?

>  
>  	state = hisi_sas_read32(hisi_hba, PHY_STATE);
>  	if (state & BIT(phy_no)) {

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

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

[Xingui Yang]

Some disks 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.

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

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 9c363e237353..030f050731ba 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -280,6 +280,9 @@
 #define CHL_INT2_RX_CODE_ERR_OFF	29
 #define CHL_INT2_RX_INVLD_DW_OFF	30
 #define CHL_INT2_STP_LINK_TIMEOUT_OFF	31
+#define CHL_INT2_RX_ERR_MSK	(BIT(CHL_INT2_RX_DISP_ERR_OFF) | \
+				 BIT(CHL_INT2_RX_CODE_ERR_OFF) | \
+				 BIT(CHL_INT2_RX_INVLD_DW_OFF))
 #define CHL_INT0_MSK			(PORT_BASE + 0x1c0)
 #define CHL_INT1_MSK			(PORT_BASE + 0x1c4)
 #define CHL_INT2_MSK			(PORT_BASE + 0x1c8)
@@ -1061,16 +1064,31 @@ 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)
+{
+	u32 irq_msk = hisi_sas_phy_read32(hisi_hba, phy_no, CHL_INT2_MSK);
+
+	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK,
+			     CHL_INT2_RX_ERR_MSK | irq_msk);
+
+	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DWS_LOST);
+	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_RESET_PROB);
+	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, CHL_INT2_RX_ERR_MSK);
+	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK, irq_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);
 	u32 irq_msk = hisi_sas_phy_read32(hisi_hba, phy_no, CHL_INT2_MSK);
-	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);
 	u32 state;
 
-	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK, msk | irq_msk);
+	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK,
+			     CHL_INT2_RX_ERR_MSK | irq_msk);
 
 	cfg &= ~PHY_CFG_ENA_MSK;
 	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
@@ -1085,11 +1103,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);
 }
 
@@ -1671,6 +1685,7 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
 	/* Delete timer and set phy_attached atomically */
 	timer_delete(&phy->timer);
 	phy->phy_attached = 1;
+	clear_phy_err_cnt_v3_hw(hisi_hba, phy_no);
 	spin_unlock(&phy->lock);
 
 	/*
@@ -1894,9 +1909,6 @@ static void handle_chl_int2_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 	struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];
 	struct pci_dev *pci_dev = hisi_hba->pci_dev;
 	struct device *dev = hisi_hba->dev;
-	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);
 
 	irq_value &= ~irq_msk;
 	if (!irq_value) {
@@ -1920,7 +1932,7 @@ static void handle_chl_int2_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
 			hisi_sas_notify_phy_event(phy, HISI_PHYE_LINK_RESET);
 	}
 
-	if (pci_dev->revision > 0x20 && (irq_value & msk)) {
+	if (pci_dev->revision > 0x20 && (irq_value & CHL_INT2_RX_ERR_MSK)) {
 		struct asd_sas_phy *sas_phy = &phy->sas_phy;
 		struct sas_phy *sphy = sas_phy->phy;
 
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] scsi: hisi_sas: Race condition in clear_phy_err_cnt_v3_hw loses interrupts
--

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

scsi: hisi_sas: Clear PHY error counts on phyup

Some disks generate link errors during link establishment but still
phy up successfully. This commit clears these intermediate error
counts on phyup to prevent leaving stale data for error detection.

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

[ ... ]

> @@ -1061,16 +1064,31 @@ 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)
> +{
> +	u32 irq_msk = hisi_sas_phy_read32(hisi_hba, phy_no, CHL_INT2_MSK);
> +
> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK,
> +			     CHL_INT2_RX_ERR_MSK | irq_msk);
> +
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DWS_LOST);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_RESET_PROB);
> +	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, CHL_INT2_RX_ERR_MSK);

[Severity: Low]
Does this sequence create a race window where an error interrupt can be lost?

Because the error counters are clear-on-read, reading them before clearing the
Write-1-to-Clear interrupt status register CHL_INT2 could lead to a race.
If a new PHY error occurs in the window after the counter is read but before
CHL_INT2 is written, the hardware will increment the error counter and set the
corresponding interrupt status bit.

The subsequent write to CHL_INT2 will then clear this newly set interrupt
status bit, causing the interrupt for the new error to be permanently lost
until another independent error occurs.

Could the CHL_INT2 register be cleared before reading the clear-on-read
counters to prevent this race?

> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK, irq_msk);
> +}

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

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

[Xingui Yang]

SAS HDDs powered up with RNOT=1 wait in Active_Wait state for a
NOTIFY(ENABLE SPINUP) primitive and keep returning NOT_READY with
ASC/ASCQ 0x04/0x11, causing endless mid-layer retries and spinup
failure.

Parse the sense data in the slot completion path and, when
LU_NOT_READY_NOTIFY_REQUIRED is returned by a directly-attached SSP
device, queue work that sends the primitive from process context
(sl_notify_ssp() contains msleep()). The work takes a runtime PM
reference with pm_runtime_get_if_active(), pairing with
pm_runtime_put_sync() in the work, so the controller stays resumed
until the primitive is sent.

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  | 43 ++++++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 +++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  4 +++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  4 +++
 5 files changed, 58 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..cf7aa0a699cd 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -996,10 +996,25 @@ 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;
+	struct device *dev = hisi_hba->dev;
+	int phy_no = phy->sas_phy.id;
+
+	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
+	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
+
+	pm_runtime_put_sync(dev);
+}
+
 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 +1654,34 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
 }
 EXPORT_SYMBOL_GPL(hisi_sas_controller_reset_done);
 
+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.sense_code != LU_NOT_READY_NOTIFY_REQUIRED)
+		return;
+
+	if (!pm_runtime_get_if_active(hisi_hba->dev))
+		return;
+
+	local_phy = sas_get_local_phy(dev);
+	phy = &hisi_hba->phy[local_phy->number];
+	if (!hisi_sas_notify_phy_event(phy, HISI_PHYE_SPINUP_NOTIFY))
+		pm_runtime_put(hisi_hba->dev);
+
+	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..5fcb8d5002f5 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1272,6 +1272,10 @@ static void slot_complete_v1_hw(struct hisi_hba *hisi_hba,
 				&status_buffer->iu[0];
 
 		sas_ssp_task_response(dev, task, iu);
+		if (unlikely(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..f16896e962fd 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2427,6 +2427,10 @@ static void slot_complete_v2_hw(struct hisi_hba *hisi_hba,
 				&status_buffer->iu[0];
 
 		sas_ssp_task_response(dev, task, iu);
+		if (unlikely(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 030f050731ba..9391d031f5df 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2447,6 +2447,10 @@ 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 (unlikely(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 v3 4/4] 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 1 potential issue(s) to consider:
- [Medium] pm: usage count underflow due to incorrect pm_runtime_get_if_active() check in hisi_sas_spinup_notify()
--

commit 255dba6c6286916d717929164fd02a77f83d3168
Author: Xingui Yang <yangxingui@huawei.com>
Subject: scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

This commit addresses spinup failures for directly-attached SSP devices 
waiting in the Active_Wait state. It parses sense data during slot 
completion and queues work to send a NOTIFY(ENABLE SPINUP) primitive while 
managing the runtime PM state of the controller.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..cf7aa0a699cde 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -996,10 +996,25 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
[ ... ]
> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
> +{
[ ... ]
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
> +	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
> +
> +	pm_runtime_put_sync(dev);

[Severity: Medium]
If the earlier pm_runtime_get_if_active() check bypassed a negative error
code, will this cause a runtime PM usage count underflow since the reference
was never successfully incremented?

> +}
[ ... ]
> @@ -1639,6 +1654,34 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
[ ... ]
> +void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
> +			    struct sas_task *task)
> +{
[ ... ]
> +	if (!pm_runtime_get_if_active(hisi_hba->dev))
> +		return;

[Severity: Medium]
Does this check properly handle negative error codes?

pm_runtime_get_if_active() returns 1 if the device is active, 0 if inactive,
and a negative error code (like -EINVAL) if runtime PM is disabled. If it
returns -EINVAL, !(-EINVAL) evaluates to false, causing execution to proceed
without actually acquiring a PM reference.

Should this check be `if (pm_runtime_get_if_active(...) <= 0)` instead?

> +
> +	local_phy = sas_get_local_phy(dev);
> +	phy = &hisi_hba->phy[local_phy->number];
> +	if (!hisi_sas_notify_phy_event(phy, HISI_PHYE_SPINUP_NOTIFY))
> +		pm_runtime_put(hisi_hba->dev);

[Severity: Medium]
Similarly, if queueing fails here, but the initial pm_runtime_get_if_active()
returned a negative error code, does this cause an immediate usage count
underflow?

> +
> +	sas_put_local_phy(local_phy);
> +}

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

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

[yangxingui]

On 2026/10/9 11:58, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] pm: usage count underflow due to incorrect pm_runtime_get_if_active() check in hisi_sas_spinup_notify()
> --
> 
> commit 255dba6c6286916d717929164fd02a77f83d3168
> Author: Xingui Yang <yangxingui@huawei.com>
> Subject: scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state
> 
> This commit addresses spinup failures for directly-attached SSP devices
> waiting in the Active_Wait state. It parses sense data during slot
> completion and queues work to send a NOTIFY(ENABLE SPINUP) primitive while
> managing the runtime PM state of the controller.
> 
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index 944ce19ae2fc1..cf7aa0a699cde 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -996,10 +996,25 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
> [ ... ]
>> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
>> +{
> [ ... ]
>> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
>> +	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
>> +
>> +	pm_runtime_put_sync(dev);
> 
> [Severity: Medium]
> If the earlier pm_runtime_get_if_active() check bypassed a negative error
> code, will this cause a runtime PM usage count underflow since the reference
> was never successfully incremented?
> 
>> +}
> [ ... ]
>> @@ -1639,6 +1654,34 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
> [ ... ]
>> +void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
>> +			    struct sas_task *task)
>> +{
> [ ... ]
>> +	if (!pm_runtime_get_if_active(hisi_hba->dev))
>> +		return;
> 
> [Severity: Medium]
> Does this check properly handle negative error codes?
> 
> pm_runtime_get_if_active() returns 1 if the device is active, 0 if inactive,
> and a negative error code (like -EINVAL) if runtime PM is disabled. If it
> returns -EINVAL, !(-EINVAL) evaluates to false, causing execution to proceed
> without actually acquiring a PM reference.
> 
> Should this check be `if (pm_runtime_get_if_active(...) <= 0)` instead?

-EINVAL only occurs while runtime PM is
disabled, which never overlaps with CQ completions on this driver, so
the check only ever sees 0 or 1 and every put stays paired with a taken
reference. '<= 0' would disable the notify entirely on !CONFIG_PM
builds (the stub always returns -EINVAL), so I'd like to keep the
current check.

Re: [PATCH v3 0/4] scsi: hisi_sas: Some misc fixes

[yangxingui]

Hi Martin, James,

We have gone through all of the Sashiko review comments on v2 and v3
one by one.  Of the six v2 findings, two were fixed in v3 (clearing
all five error counters and taking the runtime PM reference with
pm_runtime_get_if_active()), and one was answered in the v3 commit
message (the software counters are cumulative statistics and are not
reset by design).  The rest are pre-existing issues, summarized below
by trigger condition and impact:

- The delays in the phy disable and host shutdown paths predate this
   series.  The 50 ms wait for the PHY to go down is a hardware
   requirement, and a poll-based rework is planned.
- The pm8001 and mvsas response-IU findings are pre-existing issues
   in those drivers.  The libsas patch leaves these cases unchanged
   and strictly narrows the worst case, and fixing them is outside the
   scope of this series.
- The suspend flush-ordering window from the v2 review also predates
   this series.
- The suggested CHL_INT2 clear order would trade delayed reporting of
   error counters for actual data loss.
- The negative-return concern of pm_runtime_get_if_active() cannot be
   triggered on this driver, and the suggested check would disable the
   fix on !CONFIG_PM builds.

In short, none of the remaining findings is a regression introduced
by this series.

One concern to raise: Sashiko reviews every posting from scratch, and
its findings change from round to round.  Of the six v2 findings, one
was re-raised in v3 and three were not raised again, while six new
ones appeared (7 findings on all 4 patches for v3).  Iterating on its
comments alone will not converge.

Could you let us know whether this series is fine to be merged as is,
and how you would prefer the pre-existing findings to be handled: in
a follow-up series, or folded into this one?

Thanks,
Xingui

On 2026/10/9 11:32, Xingui Yang wrote:
> This series contains three independent fixes for the hisi_sas driver,
> plus a libsas fix found during review.
> 
> Patch 1 fixes an out-of-bounds memcpy in sas_ssp_task_response(): a
> device-provided sense_data_len >= 0x80000000 turns negative in the
> min_t(int, ...) clamp and becomes a huge memcpy length.
> 
> Patch 2 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 3 clears stale PHY error counts on phyup so that error detection
> is not misled by intermediate errors generated during link
> establishment.
> 
> Patch 4 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.
> 
> Changes from v2:
> Addresses the Sashiko AI review of v2.
> - Clear all five error counter registers.
> - Use pm_runtime_get_if_active().
> 
> Changes from v1:
> Addresses the Sashiko AI review of v1.
> - Add Patch 1, found in review of the sense parsing of patch 4.
> - Clear the counters under phy->lock.
> - Take a runtime PM reference for the deferred work
> 
> The remaining review findings are pre-existing issues, left for
> separate fixes in the future.
> 
> Xingui Yang (4):
>    scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()
>    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  | 43 ++++++++++++++++++++++
>   drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 ++
>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  4 ++
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 51 ++++++++++++++++++--------
>   drivers/scsi/libsas/sas_task.c         |  2 +-
>   6 files changed, 90 insertions(+), 17 deletions(-)
>