Hi Jonathan, On 17.04.23 17:54:31, Jonathan Cameron wrote: > On Fri, 14 Apr 2023 16:35:05 +0200 > Robert Richter <rrich...@amd.com> wrote: > > > On 14.04.23 13:19:50, Jonathan Cameron wrote: > > > On Tue, 11 Apr 2023 13:03:01 -0500 > > > Terry Bowman <terry.bow...@amd.com> wrote: > > > > > > > From: Robert Richter <rrich...@amd.com> > > > > > > > > In Restricted CXL Device (RCD) mode a CXL device is exposed as an > > > > RCiEP, but CXL downstream and upstream ports are not enumerated and > > > > not visible in the PCIe hierarchy. Protocol and link errors are sent > > > > to an RCEC. > > > > > > > > Restricted CXL host (RCH) downstream port-detected errors are signaled > > > > as internal AER errors, either Uncorrectable Internal Error (UIE) or > > > > Corrected Internal Errors (CIE). The error source is the id of the > > > > RCEC. A CXL handler must then inspect the error status in various CXL > > > > registers residing in the dport's component register space (CXL RAS > > > > cap) or the dport's RCRB (AER ext cap). [1] > > > > > > > > Errors showing up in the RCEC's error handler must be handled and > > > > connected to the CXL subsystem. Implement this by forwarding the error > > > > to all CXL devices below the RCEC. Since the entire CXL device is > > > > controlled only using PCIe Configuration Space of device 0, Function > > > > 0, only pass it there [2]. These devices have the Memory Device class > > > > code set (PCI_CLASS_MEMORY_CXL, 502h) and the existing cxl_pci driver > > > > can implement the handler. > > > > > > This comment implies only class code compliant drivers. Sure we don't > > > have drivers for anything else yet, but we should try to avoid saying > > > there won't be any (which I think above implies). > > > > > > You have a comment in the code, but maybe relaxing the description above > > > to "currently support devices have..." > > > > It is used here to identify CXL memory devices and limit the > > enablement to those. The spec requires this to be set for CXL mem devs > > (see cxl 3.0, 8.1.12.2). > > > > There could be other CXL devices (e.g. cache), but other drivers are > > not yet implemented. That is what I am referring to. The check makes > > sure there is actually a driver with a handler for it (cxl_pci). > > Understood on intent. My worry is that the above can be read as a > statement on hardware restrictions, rathe than on what software currently > implements. Meh. Minor point so I don't care that much! > Unlikely anyone will read the patch description after it merges anyway ;)
I have updated the description ... > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > > > index 7a25b62d9e01..171a08fd8ebd 100644 > > > > --- a/drivers/pci/pcie/aer.c > > > > +++ b/drivers/pci/pcie/aer.c > > > > @@ -946,6 +946,65 @@ static bool find_source_device(struct pci_dev > > > > *parent, > > > > return true; > > > > } > > > > > > > > +#ifdef CONFIG_PCIEAER_CXL > > > > + > > > > +static bool is_cxl_mem_dev(struct pci_dev *dev) > > > > +{ > > > > + /* > > > > + * A CXL device is controlled only using PCIe Configuration > > > > + * Space of device 0, Function 0. > > > > > > That's not true in general. Definitely true that CXL protocol > > > error reporting is controlled only using this Devfn, but > > > more generally there could be other stuff in later functions. > > > So perhaps make the comment more specific. > > > > I actually mean CXL device in RCD mode here (seen as RCiEP in the PCI > > hierarchy). > > > > The spec says (cxl 3.0, 8.1.3): > > > > """ > > In either case [(RCD and non-RCD)], the capability, status, and > > control fields in Device 0, Function 0 DVSEC control the CXL > > functionality of the entire device. > > > """ > > > > So dev 0, func 0 must contain a CXL PCIe DVSEC. Thus it is a CXL > > device and able to handle CXL AER errors. The limitation to the first > > device prevents the handler from being run multiple times for the same > > event. > > Fine with limitation. Text says "device is controlled only using". > That is true for what you are controlling here, but other aspects of the > device are controlled via whatever interface they like. > > Perhaps just quote the specification as you have done in your reply. Then it > is clear that we mean just these registers. ... and comments. Thanks, -Robert