| 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
| ||
|
Message-ID: <CAAbOPF3=CC2gNjPvNk48=1JpM6v5APF-ksDYbhYua_qA1Ukxjg@mail.gmail.com>
Date: Tue, 22 Sep 2020 19:55:39 -0700
From: Sean V Kelley <seanvk.dev@...gontracks.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Sean V Kelley <sean.v.kelley@...el.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
rafael.j.wysocki@...el.com, ashok.raj@...el.com,
tony.luck@...el.com, sathyanarayanan.kuppuswamy@...el.com,
qiuxu.zhuo@...el.com, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 06/10] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs
On Mon, Sep 21, 2020 at 4:26 AM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
> On Fri, 18 Sep 2020 13:45:59 -0700
> Sean V Kelley <sean.v.kelley@...el.com> wrote:
>
> > A Root Complex Event Collector provides support for
> > terminating error and PME messages from associated RCiEPs.
> >
> > Make use of the RCEC Endpoint Association Extended Capability
> > to identify associated RCiEPs. Link the associated RCiEPs as
> > the RCECs are enumerated.
> >
> > Co-developed-by: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@...el.com>
> > Signed-off-by: Sean V Kelley <sean.v.kelley@...el.com>
> A couple of minor things inline plus follow through on not
> special casing the older versions of the capability.
>
> Otherwise looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Thanks again for your feedback on v5. I will be sure to add in v7.
Apologies again for the email server trouble resulting in partial
patch series landing on the list.
Sean
>
> > ---
> > drivers/pci/pci.h | 2 +
> > drivers/pci/pcie/portdrv_pci.c | 3 ++
> > drivers/pci/pcie/rcec.c | 96 ++++++++++++++++++++++++++++++++++
> > include/linux/pci.h | 1 +
> > 4 files changed, 102 insertions(+)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 7b547fc3679a..ddb5872466fb 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -474,9 +474,11 @@ static inline void pci_dpc_init(struct pci_dev *pdev) {}
> > #ifdef CONFIG_PCIEPORTBUS
> > void pci_rcec_init(struct pci_dev *dev);
> > void pci_rcec_exit(struct pci_dev *dev);
> > +void pcie_link_rcec(struct pci_dev *rcec);
> > #else
> > static inline void pci_rcec_init(struct pci_dev *dev) {}
> > static inline void pci_rcec_exit(struct pci_dev *dev) {}
> > +static inline void pcie_link_rcec(struct pci_dev *rcec) {}
> > #endif
> >
> > #ifdef CONFIG_PCI_ATS
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index 4d880679b9b1..dbeb0155c2c3 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -110,6 +110,9 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > (pci_pcie_type(dev) != PCI_EXP_TYPE_RC_EC)))
> > return -ENODEV;
> >
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> > + pcie_link_rcec(dev);
> > +
> > status = pcie_port_device_register(dev);
> > if (status)
> > return status;
> > diff --git a/drivers/pci/pcie/rcec.c b/drivers/pci/pcie/rcec.c
> > index 519ae086ff41..5630480a6659 100644
> > --- a/drivers/pci/pcie/rcec.c
> > +++ b/drivers/pci/pcie/rcec.c
> > @@ -17,6 +17,102 @@
> >
> > #include "../pci.h"
> >
> > +struct walk_rcec_data {
> > + struct pci_dev *rcec;
> > + int (*user_callback)(struct pci_dev *dev, void *data);
> > + void *user_data;
> > +};
> > +
> > +static bool rcec_assoc_rciep(struct pci_dev *rcec, struct pci_dev *rciep)
> > +{
> > + unsigned long bitmap = rcec->rcec_ext->bitmap;
> > + unsigned int devn;
> > +
> > + /* An RCiEP found on bus in range */
> Perhaps adjust the comment to say:
> /* An RCiEP found on a different bus in range */
>
> as the actual rcec bus can be in the range as I understand it.
>
> > + if (rcec->bus->number != rciep->bus->number)
> > + return true;
> > +
> > + /* Same bus, so check bitmap */
> > + for_each_set_bit(devn, &bitmap, 32)
> > + if (devn == rciep->devfn)
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static int link_rcec_helper(struct pci_dev *dev, void *data)
> > +{
> > + struct walk_rcec_data *rcec_data = data;
> > + struct pci_dev *rcec = rcec_data->rcec;
> > +
> > + if ((pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END) && rcec_assoc_rciep(rcec, dev)) {
> > + dev->rcec = rcec;
> > + pci_dbg(dev, "PME & error events reported via %s\n", pci_name(rcec));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +void walk_rcec(int (*cb)(struct pci_dev *dev, void *data), void *userdata)
>
> static, or declare it in a header if we are going to need it elsewhere
> later in the series.
>
> > +{
> > + struct walk_rcec_data *rcec_data = userdata;
> > + struct pci_dev *rcec = rcec_data->rcec;
> > + u8 nextbusn, lastbusn;
> > + struct pci_bus *bus;
> > + unsigned int bnr;
> > +
> > + if (!rcec->rcec_cap)
> > + return;
> > +
> > + /* Walk own bus for bitmap based association */
> > + pci_walk_bus(rcec->bus, cb, rcec_data);
> > +
> > + /* Check whether RCEC BUSN register is present */
> > + if (rcec->rcec_ext->ver < PCI_RCEC_BUSN_REG_VER)
> > + return;
>
> If you make earlier suggested change go fill in nextbusn = 0xFF
> for the earlier versions of the capability can avoid special casing
> here.
>
> > +
> > + nextbusn = rcec->rcec_ext->nextbusn;
> > + lastbusn = rcec->rcec_ext->lastbusn;
> > +
> > + /* All RCiEP devices are on the same bus as the RCEC */
> > + if (nextbusn == 0xff && lastbusn == 0x00)
> > + return;
> > +
> > + for (bnr = nextbusn; bnr <= lastbusn; bnr++) {
> > + /* No association indicated (PCIe 5.0-1, 7.9.10.3) */
> > + if (bnr == rcec->bus->number)
> > + continue;
> > +
> > + bus = pci_find_bus(pci_domain_nr(rcec->bus), bnr);
> > + if (!bus)
> > + continue;
> > +
> > + /* Find RCiEP devices on the given bus ranges */
> > + pci_walk_bus(bus, cb, rcec_data);
> > + }
> > +}
> > +
> > +/**
> > + * pcie_link_rcec - Link RCiEP devices associating with RCEC.
> > + * @rcec RCEC whose RCiEP devices should be linked.
> > + *
> > + * Link the given RCEC to each RCiEP device found.
>
> I'm a fusspot on blank lines. The one here doesn't add anything!
>
> > + *
> > + */
> > +void pcie_link_rcec(struct pci_dev *rcec)
> > +{
> > + struct walk_rcec_data rcec_data;
> > +
> > + if (!rcec->rcec_cap)
> > + return;
> > +
> > + rcec_data.rcec = rcec;
> > + rcec_data.user_callback = NULL;
> > + rcec_data.user_data = NULL;
> > +
> > + walk_rcec(link_rcec_helper, &rcec_data);
> > +}
> > +
> > void pci_rcec_init(struct pci_dev *dev)
> > {
> > u32 rcec, hdr, busn;
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 5c5c4eb642b6..ad382a9484ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -330,6 +330,7 @@ struct pci_dev {
> > #ifdef CONFIG_PCIEPORTBUS
> > u16 rcec_cap; /* RCEC capability offset */
> > struct rcec_ext *rcec_ext; /* RCEC cached assoc. endpoint extended capabilities */
> > + struct pci_dev *rcec; /* Associated RCEC device */
> > #endif
> > u8 pcie_cap; /* PCIe capability offset */
> > u8 msi_cap; /* MSI capability offset */
>
>
Powered by blists - more mailing lists