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@xxxxxxxxxxxxxxx
> Signed-off-by: Ranjan Kumar <ranjan.kumar@xxxxxxxxxxxx>
[..]
> 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
