lkml.org 
[lkml]   [2021]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling convention of other reset methods
    From
    Date
    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.

    >> And add a new patch to begging of the series for caching 'devcap' in
    >> pci_dev structure.
    >>
    >> --- a/include/linux/pci.h
    >> +++ b/include/linux/pci.h
    >> @@ -333,6 +333,7 @@ struct pci_dev {
    >> struct rcec_ea *rcec_ea; /* RCEC cached endpoint association */
    >> struct pci_dev *rcec; /* Associated RCEC device */
    >> #endif
    >> + u32 devcap; /* Cached PCIe device capabilities */
    >> u8 pcie_cap; /* PCIe capability offset */
    >> u8 msi_cap; /* MSI capability offset */
    >> u8 msix_cap; /* MSI-X capability offset */
    >>
    >>
    >> --- a/drivers/pci/pci.c
    >> +++ b/drivers/pci/pci.c
    >> @@ -31,6 +31,7 @@
    >> #include <linux/vmalloc.h>
    >> #include <asm/dma.h>
    >> #include <linux/aer.h>
    >> +#include <linux/bitfield.h>
    >> #include "pci.h"
    >>
    >> DEFINE_MUTEX(pci_slot_mutex);
    >> @@ -4630,13 +4631,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
    >> */
    >> bool pcie_has_flr(struct pci_dev *dev)
    >> {
    >> - u32 cap;
    >> -
    >> if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
    >> return false;
    >>
    >> - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
    >> - return cap & PCI_EXP_DEVCAP_FLR;
    >> + return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);
    > Nice, thanks for reminding me of FIELD_GET(). I like how that works
    > without having to #define *_SHIFT values. I personally don't care for
    > "!!" and would probably write something like:
    >
    > return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;

    Both are same since FLR is a single bit value.

    >> }
    >>
    >> --- a/drivers/pci/probe.c
    >> +++ b/drivers/pci/probe.c
    >> @@ -19,6 +19,7 @@
    >> #include <linux/hypervisor.h>
    >> #include <linux/irqdomain.h>
    >> #include <linux/pm_runtime.h>
    >> +#include <linux/bitfield.h>
    >> #include "pci.h"
    >>
    >> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
    >> @@ -1498,8 +1499,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
    >> pdev->pcie_cap = pos;
    >> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
    >> pdev->pcie_flags_reg = reg16;
    >> - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, &reg16);
    >> - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
    >> + pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
    >> + pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
    >>

    \
     
     \ /
      Last update: 2021-07-28 23:58    [W:3.961 / U:0.412 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site