lkml.org 
[lkml]   [2017]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation
    On Mon, Oct 16, 2017 at 04:23:20PM -0500, Bjorn Helgaas wrote:
    Hi Bjorn,

    > [+cc David, Manish]
    >
    > Please use a subject line that tells more about what's going on.
    > "Update quirk" doesn't really convey any useful information.
    > Something like "Apply Cavium ThunderX ACS quirk only to Root Ports".
    >
    > On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote:
    > > This commit makes Cavium PCI ACS quirk applicable only to Cavium
    > > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities
    > > in terms of no ACS support advertisement. However, the RTL internally
    > > implements similar protection as if ACS had completion/request redirection,
    > > upstream forwarding and validation features enabled.
    > >
    > > Current quirk implementation doesn't take into account PCIERCs which
    > > also needs to be quirked. So the pci device id check mask is updated
    > > and check of device ID moved into separate function.
    >
    > s/PCIE/PCIe/ above
    >
    > s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex
    > itself doesn't appear as a pci_dev)
    >
    > > Signed-off-by: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
    > > ---
    > > v5 -> v6: comment typo fix: change 0xa00 to 0xa000
    > >
    > > drivers/pci/quirks.c | 29 +++++++++++++++++++++--------
    > > 1 file changed, 21 insertions(+), 8 deletions(-)
    > >
    > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
    > > index a4d3361..ed6c76d 100644
    > > --- a/drivers/pci/quirks.c
    > > +++ b/drivers/pci/quirks.c
    > > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags)
    > > #endif
    > > }
    > >
    > > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
    > > +/*
    > > + * The Cavium downstream ports doesn't advertise their ACS capability registers.
    > > + * However, the RTL internally implements similar protection as if
    > > + * ACS had completion redirection, forwarding and validation features enabled.
    > > + * So by this flags we're asserting that the hardware implements and
    > > + * enables equivalent ACS functionality for these flags.
    > > + */
    > > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF)
    >
    > You are changing the set of flags, which isn't mentioned in the changelog.
    > I think the best thing would be to have two patches: one that changes the
    > set of flags and a second that changes the set of affected devices.
    >
    > This set of flags was used twice in the original patch, so a #define made
    > sense. But now you only use it once, so there's no benefit in adding the
    > #define, and adding it makes the change in the set of flags harder to see.
    >
    > > +static __inline__ bool pci_quirk_cavium_acs_match(struct pci_dev *dev)
    >
    > No need to use "__inline__" here. This isn't performance critical,
    > and the compiler will probably inline it anyway because there's only
    > one use.
    >
    > > {
    > > /*
    > > - * Cavium devices matching this quirk do not perform peer-to-peer
    > > - * with other functions, allowing masking out these bits as if they
    > > - * were unimplemented in the ACS capability.
    > > + * Effectively selects all downstream ports for whole ThunderX 1 family
    > > + * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID
    > > + * are used to indicate which subdevice is used within the SoC.
    >
    > Please wrap your comments so they fit in 80 columns.
    >
    > > */
    > > - acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
    > > - PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
    > > + return (pci_is_pcie(dev) &&
    > > + (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) &&
    > > + ((dev->device & 0xf800) == 0xa000));
    >
    > I'm just a little confused by the 0xa000 mask you refer to in the
    > comment, because the mask in the code is 0xf800.

    Sorry for confusion, will correct it to 0xf800 for comment.

    >
    > Previously the quirk applied to all Cavium device IDs in the range
    > 0xa000-0xa0ff. Now it applies to device IDs in the range
    > 0xa000-0xa7ff, but only if they are PCIe Root Ports. Right?

    Correct. Since this quirk is necessary for Root Ports only.

    Thanks for reply, I'll re-work this one accordingly to your comments and re-send v7.

    WBR,
    Vadim

    >
    > > +}
    > >
    > > - if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff)))
    > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
    > > +{
    > > + if (!pci_quirk_cavium_acs_match(dev))
    > > return -ENOTTY;
    > >
    > > - return acs_flags ? 0 : 1;
    > > + return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1;
    > > }
    > >
    > > static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
    > > --
    > > 2.4.11
    > >

    \
     
     \ /
      Last update: 2017-10-17 13:30    [W:4.209 / U:0.548 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site