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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d77799f-d551-d7d9-25a5-5a1df4483679@nvidia.com>
Date:   Wed, 28 Jul 2021 17:04:17 -0500
From:   Shanker R Donthineni <sdonthineni@...dia.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     Amey Narkhede <ameynarkhede03@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        <alex.williamson@...hat.com>,
        Raphael Norwitz <raphael.norwitz@...anix.com>,
        <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kw@...ux.com>, Sinan Kaya <okaya@...nel.org>,
        Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling
 convention of other reset methods



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@...il.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