Message ID | 20230628070511.27774-1-ranjan.kumar@broadcom.com |
---|---|
Headers | show |
Series | mpt3sas: Contains Defect | expand |
On 6/28/23 16:05, Ranjan Kumar wrote: > 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. The magic value "3" for retry count was already that, magic. Why would things work better with 30 ? What is the reasoning ? Isn't a udelay needed to avoid that many retries ? > > Fixes: b899202901a8 ("mpt3sas: Add separate function for aero doorbell reads ") > Cc: stable@vger.kernel.org > Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com> [..] > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h > index 05364aa15ecd..3b8ec4fd2d21 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.h > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h > @@ -160,6 +160,8 @@ > > #define IOC_OPERATIONAL_WAIT_COUNT 10 > > +#define READL_RETRY_COUNT_OF_THIRTY 30 > +#define READL_RETRY_COUNT_OF_THREE 3 Less than ideal naming I think. If the values need to be changed again, a lot of code will need to change. What about soemthing like: #define READL_RETRY_COUNT 30 #define READL_RETRY_SHORT_COUNT 3 > /* > * NVMe defines > */ > @@ -994,7 +996,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.
On 7/5/23 21:37, Ranjan kumar wrote: > Hi Damien, > Regarding delay: > As Sathya already mentioned as this is our hardware specific behavior and > we are confident that the increased retry count > is sufficient from our hardware perspective for any new systems too. So, we > want to go with this change . Fine. Adding a comment above the macro definitions to explain something like that would be nice. > Apart from that, I will change the name as suggested . > > Thanks & Regards, > Ranjan Please avoid top posting. > On Thu, 29 Jun 2023 at 05:24, Damien Le Moal <dlemoal@kernel.org> wrote: > >> On 6/28/23 16:05, Ranjan Kumar wrote: >>> 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. >> >> The magic value "3" for retry count was already that, magic. Why would >> things >> work better with 30 ? What is the reasoning ? Isn't a udelay needed to >> avoid >> that many retries ? >> >>> >>> Fixes: b899202901a8 ("mpt3sas: Add separate function for aero doorbell >> reads ") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Ranjan Kumar <ranjan.kumar@broadcom.com> >> >> [..] >> >>> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h >> b/drivers/scsi/mpt3sas/mpt3sas_base.h >>> index 05364aa15ecd..3b8ec4fd2d21 100644 >>> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h >>> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h >>> @@ -160,6 +160,8 @@ >>> >>> #define IOC_OPERATIONAL_WAIT_COUNT 10 >>> >>> +#define READL_RETRY_COUNT_OF_THIRTY 30 >>> +#define READL_RETRY_COUNT_OF_THREE 3 >> >> Less than ideal naming I think. If the values need to be changed again, a >> lot of >> code will need to change. What about soemthing like: >> >> #define READL_RETRY_COUNT 30 >> #define READL_RETRY_SHORT_COUNT 3 >> >>> /* >>> * NVMe defines >>> */ >>> @@ -994,7 +996,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. >> >> -- >> Damien Le Moal >> Western Digital Research >> >> >