[0/2] mpt3sas: Contains Defect

Defect and removal of non required volatile qualifier of mpt3sas driver.

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 |  4 ++-
 3 files changed, 32 insertions(+), 24 deletions(-)

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