Skip to contentSkip to conversation

mpt3sas: Additional retries when reading specific registers

PATCH Applied Ranjan Kumar 3 participants 7 messages Aug 29, 2023 – Dec 11, 2023

Subsystems (3)
Stable branch SCSI LSI Logic MPT fusion drivers (FC/SAS/SPI)
lore
Share & tools
lore

Ranjan Kumar

author maintainer
#0 lore raw

v4 cover mpt3sas: Additional retries when reading specific registers

Doorbell and Host diagnostic registers could return 0 even after 3 retries and that leads to occasional resets of the controllers, hence increased the retry count to thirty.

v1->v2:
-added a new patch for volatile as suggested by Greg KH. -renamed macro as suggested by Damien Le Moal.

v2->v3:
-Modified patch description with more details.

v3->v4:
- Code simplification

Ranjan Kumar (2):
mpt3sas: Perform additional retries if Doorbell read returns 0
mpt3sas: Removing volatile qualifier

 drivers/scsi/mpt3sas/mpi/mpi2.h     |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c | 50 ++++++++++++++++++++---------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  3 +-
 3 files changed, 38 insertions(+), 17 deletions(-)
Signature · 1 line
2.31.1
author maintainer
#1 lore raw

PATCH v4 1/2 mpt3sas: Perform additional retries if Doorbell read returns 0

The driver retries certain register reads for 3 times if the returned value is 0, this was done based on hardware specification where the controller could possibly return 0 for certain registers when there is a parallel access to some of other registers from the firmware due to BMC out of band interactions.Recently it is observed that in certain systems with increased BMC interactions, the values returned are 0 even for 3 retries (the proper value is returned between 4 to 6 retries).Hence this patch changes the retry count 3 to 30, which is a revised recommendation, to avoid the out of band conflict.

Fixes b899202901a8 ("mpt3sas: Add separate function for aero doorbell reads")
Cc [email protected]
Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 46 +++++++++++++++++++++--------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
 2 files changed, 34 insertions(+), 13 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +33 -13
index 53f5492579cb..5284584e4cd2 100644
@@ -138,6 +138,9 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc);
 static void
 _base_clear_outstanding_commands(struct MPT3SAS_ADAPTER *ioc);
 
+static u32
+_base_readl_ext_retry(const volatile void __iomem *addr);
+
 /**
  * mpt3sas_base_check_cmd_timeout - Function
  *		to check timeout and command termination due

@@ -213,6 +216,20 @@ _base_readl_aero(const volatile void __iomem *addr)
 	return ret_val;
 }
 
+static u32
+_base_readl_ext_retry(const volatile void __iomem *addr)
+{
+	u32 i, ret_val;
+
+	for (i = 0 ; i < 30 ; i++) {
+		ret_val = readl(addr);
+		if (ret_val == 0)
+			continue;
+	}
+
+	return ret_val;
+}
+
 static inline u32
 _base_readl(const volatile void __iomem *addr)
 {

@@ -940,7 +957,7 @@ mpt3sas_halt_firmware(struct MPT3SAS_ADAPTER *ioc)
 
 	dump_stack();
 
-	doorbell = ioc->base_readl(&ioc->chip->Doorbell);
+	doorbell = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);
 	if ((doorbell & MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_FAULT) {
 		mpt3sas_print_fault_code(ioc, doorbell &
 		    MPI2_DOORBELL_DATA_MASK);

@@ -6686,7 +6703,7 @@ mpt3sas_base_get_iocstate(struct MPT3SAS_ADAPTER *ioc, int cooked)
 {
 	u32 s, sc;
 
-	s = ioc->base_readl(&ioc->chip->Doorbell);
+	s = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);
 	sc = s & MPI2_IOC_STATE_MASK;
 	return cooked ? sc : s;
 }

@@ -6831,7 +6848,7 @@ _base_wait_for_doorbell_ack(struct MPT3SAS_ADAPTER *ioc, int timeout)
 					   __func__, count, timeout));
 			return 0;
 		} else if (int_status & MPI2_HIS_IOC2SYS_DB_STATUS) {
-			doorbell = ioc->base_readl(&ioc->chip->Doorbell);
+			doorbell = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);
 			if ((doorbell & MPI2_IOC_STATE_MASK) ==
 			    MPI2_IOC_STATE_FAULT) {
 				mpt3sas_print_fault_code(ioc, doorbell);

@@ -6871,7 +6888,7 @@ _base_wait_for_doorbell_not_used(struct MPT3SAS_ADAPTER *ioc, int timeout)
 	count = 0;
 	cntdn = 1000 * timeout;
 	do {
-		doorbell_reg = ioc->base_readl(&ioc->chip->Doorbell);
+		doorbell_reg = ioc->base_readl_ext_retry(&ioc->chip->Doorbell);
 		if (!(doorbell_reg & MPI2_DOORBELL_USED)) {
 			dhsprintk(ioc,
 				  ioc_info(ioc, "%s: successful count(%d), timeout(%d)\n",

@@ -7019,7 +7036,7 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 	__le32 *mfp;
 
 	/* make sure doorbell is not in use */
-	if ((ioc->base_readl(&ioc->chip->Doorbell) & MPI2_DOORBELL_USED)) {
+	if ((ioc->base_readl_ext_retry(&ioc->chip->Doorbell) & MPI2_DOORBELL_USED)) {
 		ioc_err(ioc, "doorbell is in use (line=%d)\n", __LINE__);
 		return -EFAULT;
 	}

@@ -7068,7 +7085,7 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 	}
 
 	/* read the first two 16-bits, it gives the total length of the reply */
-	reply[0] = le16_to_cpu(ioc->base_readl(&ioc->chip->Doorbell)
+	reply[0] = le16_to_cpu(ioc->base_readl_ext_retry(&ioc->chip->Doorbell)
 	    & MPI2_DOORBELL_DATA_MASK);
 	writel(0, &ioc->chip->HostInterruptStatus);
 	if ((_base_wait_for_doorbell_int(ioc, 5))) {

@@ -7076,7 +7093,7 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 			__LINE__);
 		return -EFAULT;
 	}
-	reply[1] = le16_to_cpu(ioc->base_readl(&ioc->chip->Doorbell)
+	reply[1] = le16_to_cpu(ioc->base_readl_ext_retry(&ioc->chip->Doorbell)
 	    & MPI2_DOORBELL_DATA_MASK);
 	writel(0, &ioc->chip->HostInterruptStatus);
 

@@ -7087,10 +7104,10 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 			return -EFAULT;
 		}
 		if (i >=  reply_bytes/2) /* overflow case */
-			ioc->base_readl(&ioc->chip->Doorbell);
+			ioc->base_readl_ext_retry(&ioc->chip->Doorbell);
 		else
 			reply[i] = le16_to_cpu(
-			    ioc->base_readl(&ioc->chip->Doorbell)
+			    ioc->base_readl_ext_retry(&ioc->chip->Doorbell)
 			    & MPI2_DOORBELL_DATA_MASK);
 		writel(0, &ioc->chip->HostInterruptStatus);
 	}

@@ -7949,7 +7966,7 @@ _base_diag_reset(struct MPT3SAS_ADAPTER *ioc)
 			goto out;
 		}
 
-		host_diagnostic = ioc->base_readl(&ioc->chip->HostDiagnostic);
+		host_diagnostic = ioc->base_readl_ext_retry(&ioc->chip->HostDiagnostic);
 		drsprintk(ioc,
 			  ioc_info(ioc, "wrote magic sequence: count(%d), host_diagnostic(0x%08x)\n",
 				   count, host_diagnostic));

@@ -7969,7 +7986,7 @@ _base_diag_reset(struct MPT3SAS_ADAPTER *ioc)
 	for (count = 0; count < (300000000 /
 		MPI2_HARD_RESET_PCIE_SECOND_READ_DELAY_MICRO_SEC); count++) {
 
-		host_diagnostic = ioc->base_readl(&ioc->chip->HostDiagnostic);
+		host_diagnostic = ioc->base_readl_ext_retry(&ioc->chip->HostDiagnostic);
 
 		if (host_diagnostic == 0xFFFFFFFF) {
 			ioc_info(ioc,

@@ -8359,10 +8376,13 @@ mpt3sas_base_attach(struct MPT3SAS_ADAPTER *ioc)
 	ioc->rdpq_array_enable_assigned = 0;
 	ioc->use_32bit_dma = false;
 	ioc->dma_mask = 64;
-	if (ioc->is_aero_ioc)
+	if (ioc->is_aero_ioc) {
 		ioc->base_readl = &_base_readl_aero;
-	else
+		ioc->base_readl_ext_retry = &_base_readl_ext_retry;
+	} else {
 		ioc->base_readl = &_base_readl;
+		ioc->base_readl_ext_retry = &_base_readl;
+	}
 	r = mpt3sas_base_map_resources(ioc);
 	if (r)
 		goto out_free_resources;
drivers/scsi/mpt3sas/mpt3sas_base.h +1 -0
index 05364aa15ecd..10055c7e4a9f 100644
@@ -1618,6 +1618,7 @@ struct MPT3SAS_ADAPTER {
 	u8		diag_trigger_active;
 	u8		atomic_desc_capable;
 	BASE_READ_REG	base_readl;
+	BASE_READ_REG	base_readl_ext_retry;
 	struct SL_WH_MASTER_TRIGGER_T diag_trigger_master;
 	struct SL_WH_EVENT_TRIGGERS_T diag_trigger_event;
 	struct SL_WH_SCSI_TRIGGERS_T diag_trigger_scsi;
Signature · 1 line
2.31.1
author maintainer
#2 lore raw

PATCH v4 2/2 mpt3sas: Removing volatile qualifier

In reply to Ranjan Kumar

Removing reduntant volatile qualifier

Signed-off-by Ranjan Kumar <[email protected]>
---
 drivers/scsi/mpt3sas/mpi/mpi2.h     | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c | 8 ++++----
 drivers/scsi/mpt3sas/mpt3sas_base.h | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)
drivers/scsi/mpt3sas/mpi/mpi2.h +1 -1
index ed3923f8db4f..6de35b32223c 100644
@@ -199,7 +199,7 @@
 *
 *****************************************************************************/
 
-typedef volatile struct _MPI2_SYSTEM_INTERFACE_REGS {
+typedef struct _MPI2_SYSTEM_INTERFACE_REGS {
 	U32 Doorbell;		/*0x00 */
 	U32 WriteSequence;	/*0x04 */
 	U32 HostDiagnostic;	/*0x08 */
drivers/scsi/mpt3sas/mpt3sas_base.c +4 -4
index 5284584e4cd2..61a32bf00747 100644
@@ -139,7 +139,7 @@ static void
 _base_clear_outstanding_commands(struct MPT3SAS_ADAPTER *ioc);
 
 static u32
-_base_readl_ext_retry(const volatile void __iomem *addr);
+_base_readl_ext_retry(const void __iomem *addr);
 
 /**
  * mpt3sas_base_check_cmd_timeout - Function

@@ -204,7 +204,7 @@ module_param_call(mpt3sas_fwfault_debug, _scsih_set_fwfault_debug,
  * while reading the system interface register.
  */
 static inline u32
-_base_readl_aero(const volatile void __iomem *addr)
+_base_readl_aero(const void __iomem *addr)
 {
 	u32 i = 0, ret_val;
 

@@ -217,7 +217,7 @@ _base_readl_aero(const volatile void __iomem *addr)
 }
 
 static u32
-_base_readl_ext_retry(const volatile void __iomem *addr)
+_base_readl_ext_retry(const void __iomem *addr)
 {
 	u32 i, ret_val;
 

@@ -231,7 +231,7 @@ _base_readl_ext_retry(const volatile void __iomem *addr)
 }
 
 static inline u32
-_base_readl(const volatile void __iomem *addr)
+_base_readl(const void __iomem *addr)
 {
 	return readl(addr);
 }
drivers/scsi/mpt3sas/mpt3sas_base.h +1 -1
index 10055c7e4a9f..1be0850ca17a 100644
@@ -994,7 +994,7 @@ typedef void (*NVME_BUILD_PRP)(struct MPT3SAS_ADAPTER *ioc, u16 smid,
 typedef void (*PUT_SMID_IO_FP_HIP) (struct MPT3SAS_ADAPTER *ioc, u16 smid,
 	u16 funcdep);
 typedef void (*PUT_SMID_DEFAULT) (struct MPT3SAS_ADAPTER *ioc, u16 smid);
-typedef u32 (*BASE_READ_REG) (const volatile void __iomem *addr);
+typedef u32 (*BASE_READ_REG) (const void __iomem *addr);
 /*
  * To get high iops reply queue's msix index when high iops mode is enabled
  * else get the msix index of general reply queues.
Signature · 1 line
2.31.1
Applied
#3 lore raw
In reply to Ranjan Kumar

Ranjan,

Quoted 3 lines

Doorbell and Host diagnostic registers could return 0 even after 3 retries and that leads to occasional resets of the controllers, hence increased the retry count to thirty.

Applied to 6.6/scsi-staging, thanks!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
#4 lore raw

The loop in _base_readl_ext_retry is wrong, it runs 30 times regardless of the result from readl.

This results in the value being incorrect and the device being reset nearly continuously.

if (ret_val == 0) continue;

should be

if (ret_val != 0) break;

Quoted 7 lines · Martin K. Petersen

Ranjan,

Quoted 3 lines

Doorbell and Host diagnostic registers could return 0 even after 3 retries and that leads to occasional resets of the controllers, hence increased the retry count to thirty.

Applied to 6.6/scsi-staging, thanks!

author maintainer
#5 lore raw

Hi Patrick,

Quoted 7 lines · On Sun, Dec 10, 2023 at 1:51 AM <[email protected]> wrote:

The loop in _base_readl_ext_retry is wrong, it runs 30 times regardless of the result from readl.

This results in the value being incorrect and the device being reset nearly continuously.

Quoted 7 lines

if (ret_val == 0) continue;

should be

if (ret_val != 0) break;

The above code change is already submitted and accepted .

Reference : https://patchwork.kernel.org/project/linux-scsi/patch/[email protected]/

Quoted 8 lines · Martin K. Petersen
Quoted 7 lines

Ranjan,

Quoted 3 lines

Doorbell and Host diagnostic registers could return 0 even after 3 retries and that leads to occasional resets of the controllers, hence increased the retry count to thirty.

Applied to 6.6/scsi-staging, thanks!

Thanks,
Ranjan

Applied
#6 lore raw
In reply to Ranjan Kumar

Review

Quoted 9 lines · Ranjan Kumar

Doorbell and Host diagnostic registers could return 0 even after 3 retries and that leads to occasional resets of the controllers, hence increased the retry count to thirty.

v1->v2:

(quoted diff) +0 -2
-added a new patch for volatile as suggested by Greg KH.
-renamed macro as suggested by Damien Le Moal.
 
[...]

Applied to 6.6/scsi-queue, thanks!

[1/2] mpt3sas: Perform additional retries if Doorbell read returns 0

      https://git.kernel.org/mkp/scsi/c/4ca10f3e3174
[2/2] mpt3sas: Removing volatile qualifier
      https://git.kernel.org/mkp/scsi/c/0854065092a7
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering