Skip to contentSkip to conversation

mpt3sas: Additional retries when reading specific registers

PATCH Superseded Ranjan Kumar 3 participants 10 messages Jul 26, 2023 – Aug 10, 2023

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

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Ranjan Kumar

author maintainer
#0 lore raw

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

Ranjan Kumar (2):
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 | 53 ++++++++++++++++-------------
 drivers/scsi/mpt3sas/mpt3sas_base.h | 11 +++++-
 3 files changed, 40 insertions(+), 26 deletions(-)
Signature · 1 line
2.31.1
author maintainer
#1 lore raw

PATCH v3 1/2 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 | 53 ++++++++++++++++-------------
 drivers/scsi/mpt3sas/mpt3sas_base.h | 11 +++++-
 2 files changed, 39 insertions(+), 25 deletions(-)
drivers/scsi/mpt3sas/mpt3sas_base.c +29 -24
index 53f5492579cb..efc9bc48db6f 100644
@@ -196,25 +196,27 @@ module_param_call(mpt3sas_fwfault_debug, _scsih_set_fwfault_debug,
 /**
  * _base_readl_aero - retry readl for max three times.
  * @addr: MPT Fusion system interface register address
- *
- * Retry the readl() for max three times if it gets zero value
+ *@retry_count: max no of retry
+
+ * Retry the readl() for max thirty times if it gets zero value
  * while reading the system interface register.
  */
+
 static inline u32
-_base_readl_aero(const volatile void __iomem *addr)
+_base_readl_aero(const volatile void __iomem *addr, u8 retry_count)
 {
 	u32 i = 0, ret_val;
 
 	do {
 		ret_val = readl(addr);
 		i++;
-	} while (ret_val == 0 && i < 3);
+	} while (ret_val == 0 && i < retry_count);
 
 	return ret_val;
 }
 
 static inline u32
-_base_readl(const volatile void __iomem *addr)
+_base_readl(const volatile void __iomem *addr, u8 retry_count)
 {
 	return readl(addr);
 }

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

@@ -1617,10 +1619,10 @@ mpt3sas_base_mask_interrupts(struct MPT3SAS_ADAPTER *ioc)
 	u32 him_register;
 
 	ioc->mask_interrupts = 1;
-	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
+	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask, READL_RETRY_SHORT_COUNT);
 	him_register |= MPI2_HIM_DIM + MPI2_HIM_RIM + MPI2_HIM_RESET_IRQ_MASK;
 	writel(him_register, &ioc->chip->HostInterruptMask);
-	ioc->base_readl(&ioc->chip->HostInterruptMask);
+	ioc->base_readl(&ioc->chip->HostInterruptMask, READL_RETRY_SHORT_COUNT);
 }
 
 /**

@@ -1634,7 +1636,7 @@ mpt3sas_base_unmask_interrupts(struct MPT3SAS_ADAPTER *ioc)
 {
 	u32 him_register;
 
-	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask);
+	him_register = ioc->base_readl(&ioc->chip->HostInterruptMask, READL_RETRY_SHORT_COUNT);
 	him_register &= ~MPI2_HIM_RIM;
 	writel(him_register, &ioc->chip->HostInterruptMask);
 	ioc->mask_interrupts = 0;

@@ -6686,7 +6688,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(&ioc->chip->Doorbell, READL_RETRY_COUNT);
 	sc = s & MPI2_IOC_STATE_MASK;
 	return cooked ? sc : s;
 }

@@ -6760,7 +6762,8 @@ _base_wait_for_doorbell_int(struct MPT3SAS_ADAPTER *ioc, int timeout)
 	count = 0;
 	cntdn = 1000 * timeout;
 	do {
-		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus);
+		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus,
+						READL_RETRY_SHORT_COUNT);
 		if (int_status & MPI2_HIS_IOC2SYS_DB_STATUS) {
 			dhsprintk(ioc,
 				  ioc_info(ioc, "%s: successful count(%d), timeout(%d)\n",

@@ -6786,7 +6789,8 @@ _base_spin_on_doorbell_int(struct MPT3SAS_ADAPTER *ioc, int timeout)
 	count = 0;
 	cntdn = 2000 * timeout;
 	do {
-		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus);
+		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus,
+						READL_RETRY_SHORT_COUNT);
 		if (int_status & MPI2_HIS_IOC2SYS_DB_STATUS) {
 			dhsprintk(ioc,
 				  ioc_info(ioc, "%s: successful count(%d), timeout(%d)\n",

@@ -6824,14 +6828,15 @@ _base_wait_for_doorbell_ack(struct MPT3SAS_ADAPTER *ioc, int timeout)
 	count = 0;
 	cntdn = 1000 * timeout;
 	do {
-		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus);
+		int_status = ioc->base_readl(&ioc->chip->HostInterruptStatus,
+						READL_RETRY_SHORT_COUNT);
 		if (!(int_status & MPI2_HIS_SYS2IOC_DB_STATUS)) {
 			dhsprintk(ioc,
 				  ioc_info(ioc, "%s: successful count(%d), timeout(%d)\n",
 					   __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(&ioc->chip->Doorbell, READL_RETRY_COUNT);
 			if ((doorbell & MPI2_IOC_STATE_MASK) ==
 			    MPI2_IOC_STATE_FAULT) {
 				mpt3sas_print_fault_code(ioc, doorbell);

@@ -6871,7 +6876,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(&ioc->chip->Doorbell, READL_RETRY_COUNT);
 		if (!(doorbell_reg & MPI2_DOORBELL_USED)) {
 			dhsprintk(ioc,
 				  ioc_info(ioc, "%s: successful count(%d), timeout(%d)\n",

@@ -7019,13 +7024,13 @@ _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(&ioc->chip->Doorbell, READL_RETRY_COUNT) & MPI2_DOORBELL_USED)) {
 		ioc_err(ioc, "doorbell is in use (line=%d)\n", __LINE__);
 		return -EFAULT;
 	}
 
 	/* clear pending doorbell interrupts from previous state changes */
-	if (ioc->base_readl(&ioc->chip->HostInterruptStatus) &
+	if (ioc->base_readl(&ioc->chip->HostInterruptStatus, READL_RETRY_SHORT_COUNT) &
 	    MPI2_HIS_IOC2SYS_DB_STATUS)
 		writel(0, &ioc->chip->HostInterruptStatus);
 

@@ -7068,7 +7073,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(&ioc->chip->Doorbell, READL_RETRY_COUNT)
 	    & MPI2_DOORBELL_DATA_MASK);
 	writel(0, &ioc->chip->HostInterruptStatus);
 	if ((_base_wait_for_doorbell_int(ioc, 5))) {

@@ -7076,7 +7081,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(&ioc->chip->Doorbell, READL_RETRY_COUNT)
 	    & MPI2_DOORBELL_DATA_MASK);
 	writel(0, &ioc->chip->HostInterruptStatus);
 

@@ -7087,10 +7092,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(&ioc->chip->Doorbell, READL_RETRY_COUNT);
 		else
 			reply[i] = le16_to_cpu(
-			    ioc->base_readl(&ioc->chip->Doorbell)
+			    ioc->base_readl(&ioc->chip->Doorbell, READL_RETRY_COUNT)
 			    & MPI2_DOORBELL_DATA_MASK);
 		writel(0, &ioc->chip->HostInterruptStatus);
 	}

@@ -7949,14 +7954,14 @@ _base_diag_reset(struct MPT3SAS_ADAPTER *ioc)
 			goto out;
 		}
 
-		host_diagnostic = ioc->base_readl(&ioc->chip->HostDiagnostic);
+		host_diagnostic = ioc->base_readl(&ioc->chip->HostDiagnostic, READL_RETRY_COUNT);
 		drsprintk(ioc,
 			  ioc_info(ioc, "wrote magic sequence: count(%d), host_diagnostic(0x%08x)\n",
 				   count, host_diagnostic));
 
 	} while ((host_diagnostic & MPI2_DIAG_DIAG_WRITE_ENABLE) == 0);
 
-	hcb_size = ioc->base_readl(&ioc->chip->HCBSize);
+	hcb_size = ioc->base_readl(&ioc->chip->HCBSize, READL_RETRY_SHORT_COUNT);
 
 	drsprintk(ioc, ioc_info(ioc, "diag reset: issued\n"));
 	writel(host_diagnostic | MPI2_DIAG_RESET_ADAPTER,

@@ -7969,7 +7974,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(&ioc->chip->HostDiagnostic, READL_RETRY_COUNT);
 
 		if (host_diagnostic == 0xFFFFFFFF) {
 			ioc_info(ioc,
drivers/scsi/mpt3sas/mpt3sas_base.h +10 -1
index 05364aa15ecd..b4e57b89915d 100644
@@ -160,6 +160,15 @@
 
 #define IOC_OPERATIONAL_WAIT_COUNT	10
 
+/*
+ *Due to hardware specific behaviour
+ *retry count is increased from 3 to 30.
+ *Delay is not needed as retry count fulfill
+ *the desired requirement
+ */
+#define READL_RETRY_COUNT              30
+#define READL_RETRY_SHORT_COUNT		3
+
 /*
  * NVMe defines
  */

@@ -994,7 +1003,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 volatile void __iomem *addr, u8 retry_count);
 /*
  * 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
#2 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

Hi Ranjan!

Since this patch is a candidate for stable it should be as simple as possible. I don't understand all the complexity introduced to accommodate the new retry_count argument.

drivers/scsi/mpt3sas/mpt3sas_base.c module_param_call(mpt3sas_fwfault_debug, _scsih_set_fwfault_debug, · line 213

Quoted 11 lines

static inline u32

drivers/scsi/mpt3sas/mpt3sas_base.c +2 -2
-_base_readl_aero(const volatile void __iomem *addr)
+_base_readl_aero(const volatile void __iomem *addr, u8 retry_count)
 {
 	u32 i = 0, ret_val;
 
 	do {
 		ret_val = readl(addr);
 		i++;
-	} while (ret_val == 0 && i < 3);
+	} while (ret_val == 0 && i < retry_count);

_base_readl_aero() is going to return as soon as the register is non-zero. Why is it important that some register reads are only retried 3 times instead of 30? Why can't you just bump the "3" above to "30" and make it a one line change?

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author maintainer
#3 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

Hi Martin,

Quoted 14 lines · Martin K. Petersen

Hi Ranjan!

Since this patch is a candidate for stable it should be as simple as possible. I don't understand all the complexity introduced to accommodate the new retry_count argument.

Quoted 6 lines

static inline u32

drivers/scsi/mpt3sas/mpt3sas_base.c +1 -1
-_base_readl_aero(const volatile void __iomem *addr)
+_base_readl_aero(const volatile void __iomem *addr, u8 retry_count)
 {
      u32 i = 0, ret_val;
Quoted 11 lines

drivers/scsi/mpt3sas/mpt3sas_base.c module_param_call(mpt3sas_fwfault_debug, _scsih_set_fwfault_debug, · line 213

Quoted 5 lines
      do {
              ret_val = readl(addr);
              i++;
drivers/scsi/mpt3sas/mpt3sas_base.c +1 -1
-     } while (ret_val == 0 && i < 3);
+     } while (ret_val == 0 && i < retry_count);

_base_readl_aero() is going to return as soon as the register is
non-zero. Why is it important that some register reads are only retried
3 times instead of 30? Why can't you just bump the "3" above to "30" and
make it a one line change?
This HW bug impacts only a few registers. However, for code simplicity, we added retry logic(of 3 retries count) for all registers exposed to the driver.

Now the recommendation is:
- Increase retry count from 3 to 30 for only impacted registers.
- We also do not want to reduce the retries from 3 to 1 for non-impacted registers to avoid any regression.

Hence, this patch retries upto 30 times for impacted registers and 3 times for non-impacted registers.

Thanks,
Ranjan

Quoted 2 lines

--
Martin K. Petersen Oracle Linux Engineering

#4 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

Hi Ranjan!

Quoted 11 lines
Quoted 3 lines

This HW bug impacts only a few registers. However, for code simplicity, we added retry logic(of 3 retries count) for all registers exposed to the driver.

Now the recommendation is:
- Increase retry count from 3 to 30 for only impacted registers.
- We also do not want to reduce the retries from 3 to 1 for non-impacted registers to avoid any regression.

Hence, this patch retries upto 30 times for impacted registers and 3 times for non-impacted registers.

What is the adverse affect is of having a loop iterator of 30 if the non-impacted registers always return a non-zero value after 1 to 3 attempts?

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author maintainer
#5 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

Hi Martin,

Quoted 13 lines · Martin K. Petersen

Hi Ranjan!

Quoted 9 lines
Quoted 3 lines

This HW bug impacts only a few registers. However, for code simplicity, we added retry logic(of 3 retries count) for all registers exposed to the driver.

Now the recommendation is:
- Increase retry count from 3 to 30 for only impacted registers.
- We also do not want to reduce the retries from 3 to 1 for non-impacted registers to avoid any regression.

Quoted 7 lines
Quoted 2 lines

Hence, this patch retries upto 30 times for impacted registers and 3 times for non-impacted registers.

What is the adverse affect is of having a loop iterator of 30 if the
non-impacted registers always return a non-zero value after 1 to 3
attempts?
For non-impacted registers where zero is invalid, the driver will always get non-zero value in 1/3 attempts. So no concerns with 30 retries with those registers.

But for few registers zero may be a valid value and we don’t want
those registers to get penalized with 30 read retries where 1/3 reads would have sufficed.

Thanks & Regards,
Ranjan

Quoted 2 lines

--
Martin K. Petersen Oracle Linux Engineering

#6 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

Hi Ranjan,

Quoted 3 lines

But for few registers zero may be a valid value and we don’t want those registers to get penalized with 30 read retries where 1/3 reads would have sufficed.

If 0 is a valid register value you'll end up always doing 3 retries before returning. Even if the first register reads were "successful". Peculiar!

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

RE: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

From: Martin K. Petersen

Quoted 5 lines

Sent: 09 August 2023 02:26

Hi Ranjan,

Quoted 7 lines
Quoted 3 lines

But for few registers zero may be a valid value and we don’t want those registers to get penalized with 30 read retries where 1/3 reads would have sufficed.

If 0 is a valid register value you'll end up always doing 3 retries before returning. Even if the first register reads were "successful". Peculiar!

Looks like the correct solution is to completely disable the BMC. It clearly isn't compatible with using the driver as well.

If that isn't possible is seems it needs to be marked BROKEN :-)

Much like the main ethernet interface on my Ivy bridge i7.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

author maintainer
#8 lore raw

Re: [PATCH v3 1/2] Perform additional retries if Doorbell read returns 0

In reply to Martin K. Petersen

Hi Martin,

Quoted 4 lines · Martin K. Petersen

Hi Ranjan,

Quoted 8 lines
Quoted 3 lines

But for few registers zero may be a valid value and we don’t want those registers to get penalized with 30 read retries where 1/3 reads would have sufficed.

If 0 is a valid register value you'll end up always doing 3 retries
before returning. Even if the first register reads were "successful".
Peculiar!
Yes, the registers that has “0” valid value will be read 3 times. Since “3” retries logic for all

registers is already there since years, we don’t want to touch it to avoid any regression.

Now when the retry count needs to be increased further to 30, we want to limit it to impacted registers only.

Thanks,
Ranjan

Quoted 2 lines

--
Martin K. Petersen Oracle Linux Engineering

author maintainer
#9 lore raw

PATCH v3 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 | 4 ++--
 drivers/scsi/mpt3sas/mpt3sas_base.h | 2 +-
 3 files changed, 4 insertions(+), 4 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 +2 -2
index efc9bc48db6f..01ebbd8b21de 100644
@@ -203,7 +203,7 @@ module_param_call(mpt3sas_fwfault_debug, _scsih_set_fwfault_debug,
  */
 
 static inline u32
-_base_readl_aero(const volatile void __iomem *addr, u8 retry_count)
+_base_readl_aero(const void __iomem *addr, u8 retry_count)
 {
 	u32 i = 0, ret_val;
 

@@ -216,7 +216,7 @@ _base_readl_aero(const volatile void __iomem *addr, u8 retry_count)
 }
 
 static inline u32
-_base_readl(const volatile void __iomem *addr, u8 retry_count)
+_base_readl(const void __iomem *addr, u8 retry_count)
 {
 	return readl(addr);
 }
drivers/scsi/mpt3sas/mpt3sas_base.h +1 -1
index b4e57b89915d..9b0c338df3a7 100644
@@ -1003,7 +1003,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, u8 retry_count);
+typedef u32 (*BASE_READ_REG) (const void __iomem *addr, u8 retry_count);
 /*
  * 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