Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods

[Shanker R Donthineni <sdonthineni@nvidia.com>]

On 7/28/21 4:58 PM, Shanker R Donthineni wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Bjorn,
>
> On 7/28/21 3:23 PM, Bjorn Helgaas wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
>>> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
>>>> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
>>>>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
>>>>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>>>>>
>>>>> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
>>>>> is supported by the device which does not match the calling convention
>>>>> followed by reset methods which use second function argument to decide
>>>>> whether to probe or not. Add new function pcie_reset_flr() that follows
>>>>> the calling convention of reset methods.
>>>>>
>>>>> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
>>>>> ---
>>>>>  drivers/crypto/cavium/nitrox/nitrox_main.c |  4 +-
>>>>>  drivers/pci/pci.c                          | 59 +++++++++++-----------
>>>>>  drivers/pci/pcie/aer.c                     | 12 ++---
>>>>>  drivers/pci/probe.c                        |  6 ++-
>>>>>  drivers/pci/quirks.c                       |  9 ++--
>>>>>  include/linux/pci.h                        |  3 +-
>>>>>  6 files changed, 45 insertions(+), 48 deletions(-)
>>>>>
>>>>> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> index facc8e6bc..15d6c8452 100644
>>>>> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>>> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>>>>>               return -ENOMEM;
>>>>>       }
>>>>>
>>>>> -     /* check flr support */
>>>>> -     if (pcie_has_flr(pdev))
>>>>> -             pcie_flr(pdev);
>>>>> +     pcie_reset_flr(pdev, 0);
>>>> I'm not really a fan of exposing the "probe" argument outside
>>>> drivers/pci/.  I think this would be the only occurrence.  Is there a
>>>> way to avoid that?
>>>>
>>>> Can we just make pcie_flr() do the equivalent of pcie_has_flr()
>>>> internally?
>>>>
>>> I like your suggestion don't change the existing definition of
>>> pcie_has_flr()/pcie_flr() and define a new function pcie_reset_flr()
>>> to fit into the reset framework. This way no need to modify
>>> logic/drivers outside of driver/pci/xxx.
>>>
>>> int pcie_reset_flr(struct pci_dev *dev, int probe)
>>> {
>>>         if (!pcie_has_flr(dev))
>>>                 return -ENOTTY;
>>>
>>>         if (probe)
>>>                 return 0;
>>>
>>>         return pcie_flr(dev);
>>> }
>> Can't remember the whole context of this in the series, but I assume
>> this would be static?
> It should be static since it's referenced in driver/pci/qrirk.c and aer.c.
>
Sorry it should not be static since it's referenced in driver/pci/quirk.c and aer.c