Re: [PATCH 07/15] cxl/aer/pci: Add CXL PCIe port uncorrectable error recovery in AER service driver

Bowman, Terry
2024-10-17T16:21:41Z

Hi Jonathan,

On 10/17/2024 8:43 AM, Jonathan Cameron wrote:
> On Wed, 16 Oct 2024 13:07:37 -0500
> Terry Bowman <Terry.Bowman@amd.com> wrote:
>
>> Hi Jonathan,
>>
>> On 10/16/24 11:54, Jonathan Cameron wrote:
>>> On Tue, 8 Oct 2024 17:16:49 -0500
>>> Terry Bowman <terry.bowman@amd.com> wrote:
>>>
>>>> The current pcie_do_recovery() handles device recovery as result of
>>>> uncorrectable errors (UCE). But, CXL port devices require unique
>>>> recovery handling.
>>>>
>>>> Create a cxl_do_recovery() function parallel to pcie_do_recovery(). Add CXL
>>>> specific handling to the new recovery function.
>>>>
>>>> The CXL port UCE recovery must invoke the AER service driver's CXL port
>>>> UCE callback. This is different than the standard pcie_do_recovery()
>>>> recovery that calls the pci_driver::err_handler UCE handler instead.
>>>>
>>>> Treat all CXL PCIe port UCE errors as fatal and call kernel panic to
>>>> "recover" the error. A panic is called instead of attempting recovery
>>>> to avoid potential system corruption.
>>>>
>>>> The uncorrectable support added here will be used to complete CXL PCIe
>>>> port error handling in the future.
>>>>
>>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>>>
>>> Hi Terry,
>>>
>>> I'm a little bothered by the subtle difference in the bus walks
>>> in here vs the existing cases. If we need them, comments needed
>>> to explain why.
>>>
>>
>> Yes, I will add more details in the commit message about "why".
>> I added explanation following your below comment.
>>
>>> If we are going to have separate handling, see if you can share
>>> a lot more of the code by factoring out common functions for
>>> the pci and cxl handling with callbacks to handle the differences.
>>>
>>
>> Dan requested separate paths for the PCIe and CXL recovery. The intent,
>> as I understand, is to isolate the handling of PCIe and CXL protocol
>> errors. This is to create 2 different classes of protocol errors.
> Function call chain wise I'm reasonably convinced that might be a good
> idea. But not code wise if it means we end up with more hard to review
> code.
>
>>
>>> I've managed to get my head around this code a few times in the past
>>> (I think!) and really don't fancy having two subtle variants to
>>> consider next time we get a bug :( The RC_EC additions hurt my head.
>>>
>>> Jonathan
>>
>> Right, the UCE recovery logic is not straightforward. The code can be
>> refactored to take advantage of reuse. I'm interested in your thoughts
>> after I have provided some responses here.
>>
>>>
>>>> static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>>> diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c
>>>> index 31090770fffc..de12f2eb19ef 100644
>>>> --- a/drivers/pci/pcie/err.c
>>>> +++ b/drivers/pci/pcie/err.c
>>>> @@ -86,6 +86,63 @@ static int report_error_detected(struct pci_dev *dev,
>>>> return 0;
>>>> }
>>>>
>>>> +static int cxl_report_error_detected(struct pci_dev *dev,
>>>> + pci_channel_state_t state,
>>>> + enum pci_ers_result *result)
>>>> +{
>>>> + struct cxl_port_err_hndlrs *cxl_port_hndlrs;
>>>> + struct pci_driver *pdrv;
>>>> + pci_ers_result_t vote;
>>>> +
>>>> + device_lock(&dev->dev);
>>>> + cxl_port_hndlrs = find_cxl_port_hndlrs();
>>>
>>> Can we refactor to have a common function under this and report_error_detected()?
>>>
>>
>> Sure, this can be refactored.
>>
>> The difference between cxl_report_error_detected() and report_error_detected() is the
>> handlers that are called.
>>
>> cxl_report_error_detected() calls the CXL driver's registered port error handler.
>>
>> report_error_recovery() calls the pcie_dev::err_handlers.
>>
>> Let me know if I should refactor for common code here?
>
> It certainly makes sense to do that somewhere in here. Just have light
> wrappers that provide callbacks so the bulk of the code is shared.
>

Ok, Ill start on that. I have a v2 ready to-go without the reuse changes.
You want me to wait on sending v2 till it has reuse refactoring?

>>
>>
>>>> + pdrv = dev->driver;
>>>> + if (pci_dev_is_disconnected(dev)) {
>>>> + vote = PCI_ERS_RESULT_DISCONNECT;
>>>> + } else if (!pci_dev_set_io_state(dev, state)) {
>>>> + pci_info(dev, "can't recover (state transition %u -> %u invalid)\n",
>>>> + dev->error_state, state);
>>>> + vote = PCI_ERS_RESULT_NONE;
>>>> + } else if (!cxl_port_hndlrs || !cxl_port_hndlrs->error_detected) {
>>>> + if (dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
>>>> + vote = PCI_ERS_RESULT_NO_AER_DRIVER;
>>>> + pci_info(dev, "can't recover (no error_detected callback)\n");
>>>> + } else {
>>>> + vote = PCI_ERS_RESULT_NONE;
>>>> + }
>>>> + } else {
>>>> + vote = cxl_port_hndlrs->error_detected(dev, state);
>>>> + }
>>>> + pci_uevent_ers(dev, vote);
>>>> + *result = merge_result(*result, vote);
>>>> + device_unlock(&dev->dev);
>>>> + return 0;
>>>> +}
>>>
>>>> static int pci_pm_runtime_get_sync(struct pci_dev *pdev, void *data)
>>>> {
>>>> pm_runtime_get_sync(&pdev->dev);
>>>> @@ -188,6 +245,28 @@ static void pci_walk_bridge(struct pci_dev *bridge,
>>>> cb(bridge, userdata);
>>>> }
>>>>
>>>> +/**
>>>> + * cxl_walk_bridge - walk bridges potentially AER affected
>>>> + * @bridge: bridge which may be a Port, an RCEC, or an RCiEP
>>>> + * @cb: callback to be called for each device found
>>>> + * @userdata: arbitrary pointer to be passed to callback
>>>> + *
>>>> + * If the device provided is a bridge, walk the subordinate bus, including
>>>> + * the device itself and any bridged devices on buses under this bus. Call
>>>> + * the provided callback on each device found.
>>>> + *
>>>> + * If the device provided has no subordinate bus, e.g., an RCEC or RCiEP,
>>>> + * call the callback on the device itself.
>>> only call the callback on the device itself.
>>>
>>> (as you call it as stated above either way).
>>>
>>
>> Thanks. I will update the function header to include "only".
>>
>>>> + */
>>>> +static void cxl_walk_bridge(struct pci_dev *bridge,
>>>> + int (*cb)(struct pci_dev *, void *),
>>>> + void *userdata)
>>>> +{
>>>> + cb(bridge, userdata);
>>>> + if (bridge->subordinate)
>>>> + pci_walk_bus(bridge->subordinate, cb, userdata);
>>> The difference between this and pci_walk_bridge() is subtle and
>>> I'd like to avoid having both if we can.
>>>
>>
>> The cxl_walk_bridge() was added because pci_walk_bridge() does not report
>> CXL errors as needed. If the erroring device is a bridge then pci_walk_bridge()
>> does not call report_error_detected() for the root port itself. If the bridge
>> is a CXL root port then the CXL port error handler is not called. This has 2
>> problems: 1. Error logging is not provided, 2. A result vote is not provided
>> by the root port's CXL port handler.
>
> So what happens for PCIe errors on the root port? How are they reported?
> What I'm failing to understand is why these should be different.
> Maybe there is something missing on the PCIe side though!
> That code plays a game with what bridge and I thought that was there to handle
> this case.
>

PCIe errors (not CXL errors) on a root port will be processed as they are today.

An AER error is treated as a CXL error if *all* of the following are met:
- The AER error is not an internal error
- Check is in AER's is_internal_error(info) function.
- The device is not a CXL device
- Check is in AER's handles_cxl_errors() function.

Root port device PCIe error processing will not call the the pci_dev::err_handlers::error_detected().
because of the walk_bridge() implementation. The result vote to direct handling
is determined by downstream devices. This has probably been Ok until now because ports have been
fairly vanilla and standard until CXL.

>>
>>>> +}
>>>> +
>>>> pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>> pci_channel_state_t state,
>>>> pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
>>>> @@ -276,3 +355,74 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev,
>>>>
>>>> return status;
>>>> }
>>>> +
>>>> +pci_ers_result_t cxl_do_recovery(struct pci_dev *bridge,
>>>> + pci_channel_state_t state,
>>>> + pci_ers_result_t (*reset_subordinates)(struct pci_dev *pdev))
>>>> +{
>>>> + struct pci_host_bridge *host = pci_find_host_bridge(bridge->bus);
>>>> + pci_ers_result_t status = PCI_ERS_RESULT_CAN_RECOVER;
>>>> + int type = pci_pcie_type(bridge);
>>>> +
>>>> + if ((type != PCI_EXP_TYPE_ROOT_PORT) &&
>>>> + (type != PCI_EXP_TYPE_RC_EC) &&
>>>> + (type != PCI_EXP_TYPE_DOWNSTREAM) &&
>>>> + (type != PCI_EXP_TYPE_UPSTREAM)) {
>>>> + pci_dbg(bridge, "Unsupported device type (%x)\n", type);
>>>> + return status;
>>>> + }
>>>> +
>>>
>>> Would similar trick to in pcie_do_recovery work here for the upstream
>>> and downstream ports use pci_upstream_bridge() and for the others pass the dev into
>>> pci_walk_bridge()?
>>>
>>
>> Yes, that would be a good starting point to begin reuse refactoring.
>> I'm interested in getting yours and others feedback on the separation of the
>> PCI and CXL protocol errors and how much separation is or not needed.
>
> Separation may make sense (I'm still thinking about it) for separate passes
> through the topology and separate callbacks / handling when an error is seen.
> What I don't want to see is two horribly complex separate walking codes if
> we can possibly avoid it. Long term to me that just means two sets of bugs
> and problem corners instead of one.
>
> Jonathan
>

I understand. I will look to make changes here for reuse.

Regards,
Terry