scsi: hisi_sas: Some misc fixes

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

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.

---
 drivers/scsi/libsas/sas_task.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index e9d291007817..2080e5836179 100644
@@ -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

Both findings are pre-existing and unaffected by
this patch: for any positive sense_data_len the int and u32 clamps are identical, so the patch only turns the >= 0x80000000 unbounded read into a bounded one. The mvsas endianness bug and the pm8001 response buffer geometry should be addressed in separate fixes.

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.

---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 8a2500993e19..9c363e237353 100644
@@ -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

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.

---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 38 +++++++++++++++++---------
 1 file changed, 25 insertions(+), 13 deletions(-)

index 9c363e237353..030f050731ba 100644
@@ -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

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.

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

index 1323ed8aa717..d48ba3747b81 100644
@@ -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

index fa94d7110714..5fcb8d5002f5 100644
@@ -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:

index f3516a0611dd..f16896e962fd 100644
@@ -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:

index 030f050731ba..9391d031f5df 100644
@@ -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

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

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

Please send follow-up patches.

Thanks!

Martin K. Petersen

OK.

Thanks,
Xingui

Xingui,

Applied to 7.4/scsi-staging, thanks!

Martin K. Petersen

Applied to 7.4/scsi-queue, thanks!

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

      https://git.kernel.org/mkp/scsi/c/4d7f9c3fdce7
[2/4] scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup
      https://git.kernel.org/mkp/scsi/c/7307109c92f7
[3/4] scsi: hisi_sas: Clear PHY error counts on phyup
      https://git.kernel.org/mkp/scsi/c/7991f33110d2
[4/4] scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state
      https://git.kernel.org/mkp/scsi/c/92821c2358c7

Martin K. Petersen