diff mbox series

[v3,1/2] Perform additional retries if Doorbell read returns 0

Message ID 20230726112527.14987-2-ranjan.kumar@broadcom.com
State New
Headers show
Series mpt3sas: Additional retries when reading specific registers | expand

Commit Message

Ranjan Kumar July 26, 2023, 11:25 a.m. UTC 
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: stable@vger.kernel.org
Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 53 ++++++++++++++++-------------
 drivers/scsi/mpt3sas/mpt3sas_base.h | 11 +++++-
 2 files changed, 39 insertions(+), 25 deletions(-)

Comments

Martin K. Petersen July 31, 2023, 5:54 p.m. UTC | #1 
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.

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

_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?
Ranjan Kumar Aug. 3, 2023, 8:16 a.m. UTC | #2 
Hi Martin,

On Mon, Jul 31, 2023 at 11:24 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> 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.
>
> >  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);
>
> _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
> --
> Martin K. Petersen      Oracle Linux Engineering
Martin K. Petersen Aug. 8, 2023, 2:37 a.m. UTC | #3 
Hi Ranjan!

>> 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?
Ranjan Kumar Aug. 8, 2023, 7:12 a.m. UTC | #4 
Hi Martin,


On Tue, Aug 8, 2023 at 8:07 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Hi Ranjan!
>
> >> 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?
>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
> --
> Martin K. Petersen      Oracle Linux Engineering
Martin K. Petersen Aug. 9, 2023, 1:25 a.m. UTC | #5 
Hi Ranjan,

> 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!
David Laight Aug. 9, 2023, 8:23 a.m. UTC | #6 
From: Martin K. Petersen
> Sent: 09 August 2023 02:26
> 
> 
> Hi Ranjan,
> 
> > 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)
Ranjan Kumar Aug. 10, 2023, 5:44 a.m. UTC | #7 
Hi Martin,


On Wed, Aug 9, 2023 at 6:56 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Hi Ranjan,
>
> > 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
> --
> Martin K. Petersen      Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 53f5492579cb..efc9bc48db6f 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -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,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index 05364aa15ecd..b4e57b89915d 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -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.