Re: [PATCH v2 4/5] cxl/pci: Forward RCH downstream port-detected errors to the CXL.mem dev handler

[Terry Bowman]

Hi Bjorn,

On 3/24/23 17:36, Bjorn Helgaas wrote:
> I'd call this a "PCI/AER: ..." patch since that's where all the
> changes are.
> 
> On Thu, Mar 23, 2023 at 04:38:07PM -0500, Terry Bowman wrote:
>> From: Robert Richter <rrichter@xxxxxxx>
>>
>> In RCD mode a CXL device (RCD) 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.
> 
> "RCD" isn't a common term in drivers/pci; can you expand it once here?
> 
>> Now, RCH downstream port-detected errors are signaled as internal AER
>> errors (UIE/CIE) with the RCEC's source ID. A CXL handler must then
> 
> Similarly, "UIE" and "CIE" are new to drivers/pci; can you expand them
> before using?  I assume Uncorrectable Internal Error (UIE) and
> Corrected Internal Error (CIE)?  (Annoying that the PCIe spec uses
> "Correctable" in general, but "Corrected" for Internal Errors.)
> 
>> 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]
>>
>> This patch connects errors showing up in the RCEC's error handler with
> 
> "Connect errors ..." (we already know this text applies to *this
> patch*).
> 
>> 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.
> 
>> The CXL device driver is then responsible to
>> enable error reporting in the RCEC's AER cap
> 
> I don't know exactly what you mean by "error reporting in the RCEC's
> AER cap", but IIUC, for non-Root Port devices, generation of ERR_COR/
> ERR_NONFATAL/ERR_FATAL messages is controlled by the Device Control
> register and should already be enabled by pci_aer_init().
> 
> Maybe you mean setting AER mask/severity specifically for Internal
> Errors?  I'm hoping to get as much of AER management as we can in the
> PCI core and out of drivers, so maybe we need a new PCI interface to
> do that.
> 
> In any event, I assume this sort of configuration would be an
> enumeration-time thing, while *this* patch is a run-time thing, so
> maybe this information belongs with a different patch?
> 
>> (esp. CIE and UIE) and to
>> inspect the dport's CXL registers in addition (CXL RAS cap and AER ext
>> cap).
>>
>> The reason for choosing this implementation is that a CXL RCEC device
>> is bound to the AER port driver, but the driver does not allow it to
>> register a custom specific handler to support CXL. Connecting the RCEC
>> hard-wired with a CXL handler does not work, as the CXL subsystem
>> might not be present all the time. The alternative to add an
>> implementation to the portdrv to allow the registration of a custom
>> RCEC error handler isn't worth doing it as CXL would be its only user.
>> Instead, just check for an CXL RCEC and pass it down to the connected
>> CXL device's error handler.
>>
>> [1] CXL 3.0 spec, 12.2.1.1 RCH Downstream Port-detected Errors
>> [2] CXL 3.0 spec, 8.1.3 PCIe DVSEC for CXL Devices
>>
>> Co-developed-by: Terry Bowman <terry.bowman@xxxxxxx>
>> Signed-off-by: Terry Bowman <terry.bowman@xxxxxxx>
>> Signed-off-by: Robert Richter <rrichter@xxxxxxx>
> 
> Since you're sending this patch (Terry) your Signed-off-by should be
> last.
> 

I'll move my Signed-off-by to the last.

Regards,
Terry