Messages in this thread | | | From | "Sean V Kelley" <> | Subject | Re: [PATCH v6 06/10] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs | Date | Mon, 28 Sep 2020 09:20:28 -0700 |
| |
On 25 Sep 2020, at 15:15, Bjorn Helgaas wrote:
> On Tue, Sep 22, 2020 at 02:38:55PM -0700, Sean V Kelley wrote: >> From: Sean V Kelley <sean.v.kelley@intel.com> >> >> 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@intel.com> >> Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> >> Signed-off-by: Sean V Kelley <sean.v.kelley@intel.com> >> --- >> 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); > > Nice solution. One day we'll get rid of pcie_portdrv_probe() and > integrate this stuff better into the PCI core, and we'll have to > figure out a little different solution then. But we'll be smarter > then so it should be possible :)
Indeed!
> >> 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 */ > > Wondering if we can put this pointer inside the struct rcec_ext (or > whatever we call it) so we don't have to pay *two* pointers for every > PCI device? Maybe it should be something like this:
It’s definitely unfortunate to need these two pointers. Here we have the spec implying an association, but leaving it to software to fill in the gaps! The spec provides a bitmap, but the reverse mapping is also needed from the RCiEPs to the RCEC.
> > struct rcec { > u8 ea_ver; > u8 ea_nextbusn; > u8 ea_lastbusn; > u32 ea_bitmap; > struct pci_dev *rcec; > } > > I dunno. Not sure if that would be better or worse, since we'd be > mixing RCEC stuff (the EA capability info) with RCiEP stuff (the > pointer to the related RCEC). Could even be a union, since any given > device only needs one of them, but I'm pretty sure that would be > worse.
Well, I think the pointer savings would be offset by lack of clarity on what is going on here due to this nesting. Then again, *rcec_ea and *rcec are not themselves revealing in terms of the relationship with RCiEPs either. I’ll think on it some more. I do like the ea_ prefix.
> > BTW, 03/10 didn't add a forward declaration, e.g., > > struct rcec_ext; > > to include/linux/pci.h, and the actual definition is in > drivers/pci/pci.h. It seems like you should need that?
Yes, it should be added. Will do.
Thanks,
Sean > >> #endif >> u8 pcie_cap; /* PCIe capability offset */ >> u8 msi_cap; /* MSI capability offset */ >> -- >> 2.28.0 >>
| |