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

[PATCH v2 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 v1:

- Patch 1 is new, found in review of the sense parsing of patch 4.
- Patch 2 (v1 patch 1) is unchanged.
- Patch 3 (v1 patch 2) now clears the counters under phy->lock with
  the error interrupts masked around the CHL_INT2 status write, drops
  the broken print gate on the cumulative counters, and replaces the
  open-coded RX error bit mask with the CHL_INT2_RX_ERR_MSK macro.
- Patch 4 (v1 patch 3) now takes a runtime PM reference for the
  deferred work, pairing with pm_runtime_put_sync() in the work, and
  uses the 16-bit LU_NOT_READY_NOTIFY_REQUIRED definition from
  scsi_sense.h instead of local ASC/ASCQ macros.

The remaining review findings are pre-existing issues, left for
separate fixes:

  - lockless SL_CONTROL read-modify-write between
    sl_notify_ssp_v3_hw() and phy_down_v3_hw() (hardirq)
  - _suspend_v3_hw() flush/interrupt-disable ordering versus late
    work queued by the threaded CQ handlers

The 50 ms wait in disable_phy_v3_hw() for the PHY to leave the up
state is a hardware requirement and is left as is.

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  | 42 ++++++++++++++++++++++
 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 | 48 +++++++++++++++++---------
 drivers/scsi/libsas/sas_task.c         |  2 +-
 6 files changed, 84 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH v2 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")
Cc: stable@vger.kernel.org
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

[PATCH v2 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 v2 2/4] scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: hisi_sas: 50ms busy-wait with local interrupts disabled in phy_disable
- [Low] scsi: hisi_sas: Unnecessary 10ms busy wait in sleepable contexts
--

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

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

This commit restores original delay values that were incorrectly changed to
100 by replacing a single macro with separate macros for each delay site.
This avoids magic numbers while fixing overly long delays on PHY disable
and host shutdown paths.

> 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 code execute a 50ms busy-wait with local interrupts disabled?

Looking at hisi_sas_phy_enable(), it acquires phy->lock using
spin_lock_irqsave(), disabling local hardware interrupts on the current CPU:

drivers/scsi/hisi_sas/hisi_sas_main.c:hisi_sas_phy_enable() {
    ...
    spin_lock_irqsave(&phy->lock, flags);
    ...
    if (enable)
        ...
    else
        hisi_hba->hw->phy_disable(hisi_hba, phy_no);
    ...
}

For v3 hardware, this callback resolves to disable_phy_v3_hw(), which
executes a 50ms busy loop via mdelay(DISABLE_PHY_DELAY_MS) while remaining
in atomic context.

Furthermore, hisi_sas_stop_phys() iterates over all PHYs and calls
hisi_sas_phy_enable(..., 0) sequentially. For a controller with multiple
PHYs, this multiplies the delay, resulting in hundreds of milliseconds
of CPU stall with interrupts disabled.

Executing massive busy loops with interrupts disabled blocks scheduling,
drops hardware interrupts, and causes massive system latency, risking soft
lockups. While this patch reduces the delay from 100ms back to 50ms, the
underlying architectural violation in the modified delay path remains.

>  
>  	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);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Low]
Is it necessary to use a 10ms busy loop in sleepable contexts?

disable_host_v3_hw() executes mdelay(DISABLE_HOST_PHY_DELAY_MS), which
constitutes a 10ms CPU busy loop.

This function is called from paths like soft_reset_v3_hw(),
hisi_sas_reset_prepare_v3_hw(), and _suspend_v3_hw(). These callers
operate in sleepable contexts such as PM suspend/resume and device reset
paths where local CPU interrupts are not disabled.

Using mdelay() for a delay as long as 10ms on a sleepable code path wastes
CPU cycles and needlessly increases power consumption. Could this be
replaced with msleep() or usleep_range() instead?

>  
>  	reg_val = hisi_sas_read32(hisi_hba, AXI_MASTER_CFG_BASE +
>  				  AM_CTRL_GLOBAL);

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

[PATCH v2 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 | 36 ++++++++++++++++----------
 1 file changed, 23 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..a2011d77915b 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,29 @@ 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_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 +1101,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 +1683,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 +1907,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 +1930,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 v2 3/4] scsi: hisi_sas: Clear PHY error counts on phyup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: hisi_sas: incomplete error clearance on phyup leaves software counters stale
- [Low] scsi: hisi_sas: missing clear of DWS_LOST and RESET_PROB error counts
--

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

scsi: hisi_sas: Clear PHY error counts on phyup

This patch attempts to clear intermediate link error counts generated during
link establishment in phyup. This is done to prevent stale error data from
polluting runtime statistics.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 9c363e2373538..a2011d77915bb 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
[ ... ]
> @@ -1061,16 +1064,29 @@ 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_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]
Will this leave the software counters stale? While the hardware registers are
cleared here, intermediate errors might have already been accumulated into
software counters during link establishment via phy_get_events_v3_hw():

handle_chl_int2_v3_hw()
  phy_get_events_v3_hw()
    reg_value = hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
    sphy->invalid_dword_count += reg_value;

Because clear_phy_err_cnt_v3_hw() only reads and clears the hardware
registers, the corresponding software counters like sphy->invalid_dword_count
and phy->code_violation_err_count remain untouched. Should the software
counters also be reset here to completely clear the stale data?

[Severity: Low]
Does this function also need to clear ERR_CNT_DWS_LOST and ERR_CNT_RESET_PROB?

Since these registers are clear-on-read, and phy_get_events_v3_hw() reads all
five of them, any subsequent RX error could cause phy_get_events_v3_hw() to
read the uncleared, stale DWS_LOST and RESET_PROB counts, incorrectly adding
them to the running statistics.

> +
> +	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);
> +}

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

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

[yangxingui]

On 2026/10/8 11:41, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] scsi: hisi_sas: incomplete error clearance on phyup leaves software counters stale
> - [Low] scsi: hisi_sas: missing clear of DWS_LOST and RESET_PROB error counts

The suggestion from Sashiko is appropriate, update it in the next version.

[PATCH v2 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, 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  | 42 ++++++++++++++++++++++++++
 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, 54 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..f3b6bb510a71 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,33 @@ 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;
+
+	local_phy = sas_get_local_phy(dev);
+	phy = &hisi_hba->phy[local_phy->number];
+
+	pm_runtime_get_noresume(hisi_hba->dev);
+	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..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 a2011d77915b..95f79bac3dbe 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2445,6 +2445,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 v2 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 2 potential issue(s) to consider:
- [High] scsi: hisi_sas: hardware access on suspended device via late CQ interrupt
- [Medium] pm: pm_runtime_get_noresume() used in IRQ handler instead of pm_runtime_get_if_active()
--

commit 8d6af7e7596b04b1c535c9ae957073a4a940e9cc
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 by parsing the sense
data in the slot completion path. When LU_NOT_READY_NOTIFY_REQUIRED is
returned, it queues a work item to send a spinup notify primitive while
taking a runtime PM reference.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..f3b6bb510a71e 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);

[Severity: High]
Could this work item execute after the hardware is suspended? 

If a completion queue interrupt fires during suspend, it could happen in the
window between the workqueue flush and disabling interrupts:

_suspend_v3_hw()
    flush_workqueue(hisi_hba->wq);
    // interrupt fires -> hisi_sas_spinup_notify() queues this work
    interrupt_disable_v3_hw(hisi_hba);

Because the interrupt handler queues this work to a non-freezable workqueue, 
it can execute concurrently with or after the host is disabled. If the 
controller's bus is shut down by the time sl_notify_ssp() accesses MMIO 
registers, will this cause a system freeze or panic?

> +	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
> +
> +	pm_runtime_put_sync(dev);
> +}

[ ... ]

> @@ -1639,6 +1654,33 @@ 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;
> +
> +	local_phy = sas_get_local_phy(dev);
> +	phy = &hisi_hba->phy[local_phy->number];
> +
> +	pm_runtime_get_noresume(hisi_hba->dev);

[Severity: Medium]
The PM subsystem guidelines state that IRQ handlers should use 
pm_runtime_get_if_active() rather than pm_runtime_get_noresume(). 

Since hisi_sas_spinup_notify() is called directly from hardware completion 
interrupt handlers, could this blindly increment the usage counter on a 
device that is already suspended or transitioning to suspend? 

Using pm_runtime_get_if_active() would verify the device is actually active 
and prevent queuing the spinup notify work when the controller is powering
down.

> +	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);

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