Message-ID: <36d9e485-26db-4ceb-b57b-47483c3be7b6@amd.com>
Date: Wed, 2 Jul 2025 12:51:34 -0500
From: "Bowman, Terry" <terry.bowman@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: dave@...olabs.net, dave.jiang@...el.com, alison.schofield@...el.com,
 dan.j.williams@...el.com, bhelgaas@...gle.com, shiju.jose@...wei.com,
 ming.li@...omail.com, Smita.KoralahalliChannabasappa@....com,
 rrichter@....com, dan.carpenter@...aro.org,
 PradeepVineshReddy.Kodamati@....com, lukas@...ner.de,
 Benjamin.Cheatham@....com, sathyanarayanan.kuppuswamy@...ux.intel.com,
 linux-cxl@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org
Subject: Re: [PATCH v10 06/17] PCI/AER: Dequeue forwarded CXL error



On 6/27/2025 6:00 AM, Jonathan Cameron wrote:
> On Thu, 26 Jun 2025 17:42:41 -0500
> Terry Bowman <terry.bowman@....com> wrote:
>
>> The AER driver is now designed to forward CXL protocol errors to the CXL
>> driver. Update the CXL driver with functionality to dequeue the forwarded
>> CXL error from the kfifo. Also, update the CXL driver to begin the protocol
>> error handling processing using the work received from the FIFO.
>>
>> Introduce function cxl_proto_err_work_fn() to dequeue work forwarded by the
> After earlier update it already exists, you are just filling it in here.
> So reword this.
Ok.
>> AER service driver. This will begin the CXL protocol error processing with
>> a call to cxl_handle_proto_error().
>>
>> Update cxl/core/native_ras.c by adding cxl_rch_handle_error_iter() that was
>> previously in the AER driver. Add check that Endpoint is bound to a CXL
>> driver.
>>
>> Introduce logic to take the SBDF values from 'struct cxl_proto_error_info'
>> and use in discovering the erring PCI device. The call to pci_get_domain_bus_and_slot()
>> will return a reference counted 'struct pci_dev *'. This will serve as
>> reference count to prevent releasing the CXL Endpoint's mapped RAS while
>> handling the error. Use scope base __free() to put the reference count.
>> This will change when adding support for CXL port devices in the future.
>>
>> Implement cxl_handle_proto_error() to differentiate between Restricted CXL
>> Host (RCH) protocol errors and CXL virtual host (VH) protocol errors. RCH
>> errors will be processed with a call to walk the associated Root Complex
>> Event Collector's (RCEC) secondary bus looking for the Root Complex
>> Integrated Endpoint (RCiEP) to handle the RCH error. Export pcie_walk_rcec()
>> so the CXL driver can walk the RCEC's downstream bus, searching for the
>> RCiEP.
> I'd drop the RCiEP description beyond saying 'handle it as before'
> as I think there is no major change in this.
Ok.
>> VH correctable error (CE) processing will call the CXL CE handler. VH
>> uncorrectable errors (UCE) will call cxl_do_recovery(), implemented as a
>> stub for now and to be updated in future patch. Export pci_aer_clean_fatal_status()
>> and pci_clean_device_status() used to clean up AER status after handling.
>>
>> Maintain the locking logic found in the original AER driver. Replace the
>> existing device_lock() in cxl_rch_handle_error_iter() to use guard(device)
>> lock for maintainability.
> This change is fine, but it is an AND change in a patch doing quite a few other
> things.  So do it in a trivial precursor patch.  Look at the other things in this
> description and see if they can be factored out too so that the guts of this
> patch are much easier to spot.
>

I agree. I'll revisit to try and simplify the amount of changes in this patch.

>> CE errors did not include locking in previous driver
>> implementation. Leave the updated CE handling path as-is.
>>
>> Signed-off-by: Terry Bowman <terry.bowman@....com>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> A few comments inline.
>
> Jonathan
>
>> ---
>>  drivers/cxl/core/native_ras.c | 87 +++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxlpci.h          |  1 +
>>  drivers/cxl/pci.c             |  6 +++
>>  drivers/pci/pci.c             |  1 +
>>  drivers/pci/pci.h             |  7 ---
>>  drivers/pci/pcie/aer.c        |  1 +
>>  drivers/pci/pcie/cxl_aer.c    | 41 -----------------
>>  drivers/pci/pcie/rcec.c       |  1 +
>>  include/linux/aer.h           |  2 +
>>  include/linux/pci.h           | 10 ++++
>>  10 files changed, 109 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/cxl/core/native_ras.c b/drivers/cxl/core/native_ras.c
>> index 011815ddaae3..5bd79d5019e7 100644
>> --- a/drivers/cxl/core/native_ras.c
>> +++ b/drivers/cxl/core/native_ras.c
>> @@ -6,9 +6,96 @@
>>  #include <cxl/event.h>
>>  #include <cxlmem.h>
>>  #include <core/core.h>
>> +#include <cxlpci.h>
>> +
>> +static void cxl_do_recovery(struct pci_dev *pdev)
>> +{
>> +}
>> +
>> +static bool is_cxl_rcd(struct pci_dev *pdev)
>> +{
>> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
>> +		return false;
>> +
>> +	/*
>> +	 * The capability, status, and control fields in Device 0,
>> +	 * Function 0 DVSEC control the CXL functionality of the
>> +	 * entire device (CXL 3.2, 8.1.3).
>> +	 */
>> +	if (pdev->devfn != PCI_DEVFN(0, 0))
>> +		return false;
>> +
>> +	/*
>> +	 * CXL Memory Devices must have the 502h class code set (CXL
> Short wrap.
Ok.
>> +	 * 3.2, 8.1.12.1).
>> +	 */
>> +	if (FIELD_GET(PCI_CLASS_CODE_MASK, pdev->class) != PCI_CLASS_MEMORY_CXL)
>> +		return false;
>> +
>> +	return true;
>
> If this isn't going to get more complex
>
> 	return FIELD_GET(...)
Good idea. I overlooked this.

>> +}
>> +
>> +static int cxl_rch_handle_error_iter(struct pci_dev *pdev, void *data)
>> +{
>> +	struct cxl_proto_error_info *err_info = data;
>> +
>> +	guard(device)(&pdev->dev);
>> +
>> +	if (!is_cxl_rcd(pdev) || !cxl_pci_drv_bound(pdev))
>> +		return 0;
>> +
>> +	if (err_info->severity == AER_CORRECTABLE)
>> +		cxl_cor_error_detected(pdev);
>> +	else
>> +		cxl_error_detected(pdev, pci_channel_io_frozen);
>> +
>> +	return 1;
>> +}
>> +
>> +static void cxl_handle_proto_error(struct cxl_proto_error_info *err_info)
>> +{
>> +	struct pci_dev *pdev __free(pci_dev_put) =
>> +		pci_get_domain_bus_and_slot(err_info->segment,
>> +					    err_info->bus,
>> +					    err_info->devfn);
>> +
>> +	if (!pdev) {
>> +		pr_err("Failed to find the CXL device (SBDF=%x:%x:%x:%x)\n",
>> +		       err_info->segment, err_info->bus, PCI_SLOT(err_info->devfn),
>> +		       PCI_FUNC(err_info->devfn));
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Internal errors of an RCEC indicate an AER error in an
>> +	 * RCH's downstream port. Check and handle them in the CXL.mem
>> +	 * device driver.
> I don't think the reference here to the CXL.mem driver is that helpful
> given the code is immediate above. Maybe move the comment?
>
Ok.

- Terry

>> +	 */
>> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_EC)
>> +		return pcie_walk_rcec(pdev, cxl_rch_handle_error_iter, err_info);
>> +
>> +	if (err_info->severity == AER_CORRECTABLE) {
>> +		int aer = pdev->aer_cap;
>> +
>> +		if (aer)
>> +			pci_clear_and_set_config_dword(pdev,
>> +						       aer + PCI_ERR_COR_STATUS,
>> +						       0, PCI_ERR_COR_INTERNAL);
>> +
>> +		cxl_cor_error_detected(pdev);
>> +
>> +		pcie_clear_device_status(pdev);
>> +	} else {
>> +		cxl_do_recovery(pdev);
>> +	}
>> +}
>

Powered by blists - more mailing lists