Re: [PATCH v6 06/10] PCI/RCEC: Add pcie_link_rcec() to associate RCiEPs

["Sean V Kelley" <sean.v.kelley@intel.com>]

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