Messages in this thread | | | Date | Fri, 28 Oct 2022 09:38:45 -0500 | Subject | Re: [PATCH 2/5] cxl/pci: Discover and cache pointer to RCD dport's PCIe AER capability | From | Terry Bowman <> |
| |
Hi Bjorn,
On 10/27/22 09:52, Bjorn Helgaas wrote: > On Fri, Oct 21, 2022 at 01:56:12PM -0500, Terry Bowman wrote: >> CXL downport PCIe AER information needs to be logged during error handling. >> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a >> result the CXL PCIe driver is not aware of the AER in 'PCI Express' >> capability located in the RCRB downport/upport. Logic must be introduced to >> use the downport/upport AER information. > > I assume "downport" is the same as "dport" in "cxl_dport" and means > "Downstream Port". Might be nice to reduce the number of variations > if feasible. >
Yes, I'll update the terminology throughout to use cxl_port, cxl_dport, and cxl_uport.
>> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id) >> +{ >> + resource_size_t offset, rcrb; >> + void *rcrb_mapped; >> + u32 cap_hdr; >> + >> + rcrb = cxl_get_rcrb(cxlmd); >> + if (!rcrb) >> + return 0; >> + >> + rcrb_mapped = ioremap(rcrb, SZ_4K); >> + if (!rcrb_mapped) >> + return 0; >> + >> + offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST); >> + cap_hdr = readl(rcrb_mapped + offset); >> + >> + while (PCI_CAP_ID(cap_hdr)) { >> + if (PCI_CAP_ID(cap_hdr) == cap_id) >> + break; >> + >> + offset = PCI_CAP_NEXT(cap_hdr); >> + if (offset == 0) >> + break; >> + >> + cap_hdr = readl(rcrb_mapped + offset); >> + } >> + iounmap((void *)rcrb_mapped); >> + >> + if (PCI_CAP_ID(cap_hdr) != cap_id) >> + return 0; >> + >> + pr_debug("Found capability %X @ %llX (%X)\n", >> + cap_id, rcrb + offset, cap_hdr); > > Would be nice to use dev_dbg() if possible here. > > Is "%X" (upper-case hex) the convention in CXL? Most places in Linux > seem to use "%x". Also consider "%#x" (or "%#X") so it's obvious > these are hex. >
Ok, I will make the change.
>> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd) >> +{ >> + resource_size_t cap_base; >> + >> + /* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */ >> + if (!is_rcd(cxlmd)) >> + return; >> + >> + /* >> + * Read base address of the PCI express cap. Cache the cap's >> + * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status. >> + */ >> + cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP); >> + cxl_setup_dport_aer(cxlmd, cap_base); > > I don't see anything about PCI_EXP_DEVCTL and PCI_EXP_DEVSTA in > cxl_get_dport_cap() or cxl_setup_dport_aer(). And I don't see any > caching, except for setting map->base in cxl_setup_dport_aer(). > > Caching those registers, especially PCI_EXP_DEVSTA, doesn't seem like > it would make much sense anyway since bits there are set by hardware > when things happen.
This comment needs to be moved to later patch enabling AER.
| |