2020-09-18 20:52:53

by Kelley, Sean V

[permalink] [raw]
Subject: [PATCH v5 06/10] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs

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 <[email protected]>
Signed-off-by: Qiuxu Zhuo <[email protected]>
Signed-off-by: Sean V Kelley <[email protected]>
---
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 */
+ 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)
+{
+ 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;
+
+ 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.
+ *
+ */
+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 */
--
2.28.0


2020-09-21 11:28:04

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v5 06/10] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs

On Fri, 18 Sep 2020 13:45:59 -0700
Sean V Kelley <[email protected]> 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 <[email protected]>
> Signed-off-by: Qiuxu Zhuo <[email protected]>
> Signed-off-by: Sean V Kelley <[email protected]>
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 <[email protected]>

> ---
> 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 */


2020-09-23 03:08:22

by Sean V Kelley

[permalink] [raw]
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
<[email protected]> wrote:
>
> On Fri, 18 Sep 2020 13:45:59 -0700
> Sean V Kelley <[email protected]> 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 <[email protected]>
> > Signed-off-by: Qiuxu Zhuo <[email protected]>
> > Signed-off-by: Sean V Kelley <[email protected]>
> 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 <[email protected]>

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 */
>
>