| Message ID | 20250107143852.3692571-8-terry.bowman@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable CXL PCIe port protocol error handling and logging | expand |
On Tue, 7 Jan 2025 08:38:43 -0600 Terry Bowman <terry.bowman@amd.com> wrote: > Existing recovery procedure for PCIe uncorrectable errors (UCE) does not > apply to CXL devices. Recovery can not be used for CXL devices because of > potential corruption on what can be system memory. Also, current PCIe UCE > recovery, in the case of a Root Port (RP) or Downstream Switch Port (DSP), > does not begin at the RP/DSP but begins at the first downstream device. > This will miss handling CXL Protocol Errors in a CXL RP or DSP. A separate > CXL recovery is needed because of the different handling requirements > > Add a new function, cxl_do_recovery() using the following. > > Add cxl_walk_bridge() to iterate the detected error's sub-topology. > cxl_walk_bridge() is similar to pci_walk_bridge() but the CXL flavor > will begin iteration at the RP or DSP rather than beginning at the > first downstream device. I'm still holding out for making pci_walk_bridge() do the same and seeing what if anything breaks. Other than that I'm fine with this patch. > > Add cxl_report_error_detected() as an analog to report_error_detected(). > It will call pci_driver::cxl_err_handlers for each iterated downstream > device. The pci_driver::cxl_err_handler's UCE handler returns a boolean > indicating if there was a UCE error detected during handling. > > cxl_do_recovery() uses the status from cxl_report_error_detected() to > determine how to proceed. Non-fatal CXL UCE errors will be treated as > fatal. If a UCE was present during handling then cxl_do_recovery() > will kernel panic. > > Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Terry Bowman wrote: > Existing recovery procedure for PCIe uncorrectable errors (UCE) does not > apply to CXL devices. Recovery can not be used for CXL devices because of > potential corruption on what can be system memory. Also, current PCIe UCE > recovery, in the case of a Root Port (RP) or Downstream Switch Port (DSP), > does not begin at the RP/DSP but begins at the first downstream device. > This will miss handling CXL Protocol Errors in a CXL RP or DSP. A separate > CXL recovery is needed because of the different handling requirements > > Add a new function, cxl_do_recovery() using the following. > > Add cxl_walk_bridge() to iterate the detected error's sub-topology. > cxl_walk_bridge() is similar to pci_walk_bridge() but the CXL flavor > will begin iteration at the RP or DSP rather than beginning at the > first downstream device. > > Add cxl_report_error_detected() as an analog to report_error_detected(). > It will call pci_driver::cxl_err_handlers for each iterated downstream > device. The pci_driver::cxl_err_handler's UCE handler returns a boolean > indicating if there was a UCE error detected during handling. > > cxl_do_recovery() uses the status from cxl_report_error_detected() to > determine how to proceed. Non-fatal CXL UCE errors will be treated as > fatal. If a UCE was present during handling then cxl_do_recovery() > will kernel panic. > > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > --- [snip] > + > +static int cxl_report_error_detected(struct pci_dev *dev, void *data) > +{ > + const struct cxl_error_handlers *cxl_err_handler; > + struct pci_driver *pdrv = dev->driver; > + bool *status = data; > + > + device_lock(&dev->dev); > + if (pdrv && pdrv->cxl_err_handler && > + pdrv->cxl_err_handler->error_detected) { > + cxl_err_handler = pdrv->cxl_err_handler; > + *status = cxl_err_handler->error_detected(dev); > + } > + device_unlock(&dev->dev); > + return *status; This is probably just another nit on my part but returning bool here for int may cause issues down the road. Looking at this I wonder if it would be better to add *_PANIC to pci_ers_result_t and return that similar to report_error_detected()? > +} > + > +void cxl_do_recovery(struct pci_dev *dev) > +{ > + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); > + int type = pci_pcie_type(dev); > + struct pci_dev *bridge; > + int status; > + > + if (type == PCI_EXP_TYPE_ROOT_PORT || > + type == PCI_EXP_TYPE_DOWNSTREAM || > + type == PCI_EXP_TYPE_UPSTREAM || > + type == PCI_EXP_TYPE_ENDPOINT) > + bridge = dev; > + else > + bridge = pci_upstream_bridge(dev); > + > + cxl_walk_bridge(bridge, cxl_report_error_detected, &status); > + if (status) > + panic("CXL cachemem error."); > + > + if (host->native_aer || pcie_ports_native) { > + pcie_clear_device_status(dev); > + pci_aer_clear_nonfatal_status(dev); > + } There is a nice informative comment in pcie_do_recovery() about this block. I think we should combine this and that block into a new function which preserves that for both paths. Ira > + > + pci_info(bridge, "CXL uncorrectable error.\n"); > +} > -- > 2.34.1 >
On 1/14/2025 5:33 AM, Jonathan Cameron wrote: > On Tue, 7 Jan 2025 08:38:43 -0600 > Terry Bowman <terry.bowman@amd.com> wrote: > >> Existing recovery procedure for PCIe uncorrectable errors (UCE) does not >> apply to CXL devices. Recovery can not be used for CXL devices because of >> potential corruption on what can be system memory. Also, current PCIe UCE >> recovery, in the case of a Root Port (RP) or Downstream Switch Port (DSP), >> does not begin at the RP/DSP but begins at the first downstream device. >> This will miss handling CXL Protocol Errors in a CXL RP or DSP. A separate >> CXL recovery is needed because of the different handling requirements >> >> Add a new function, cxl_do_recovery() using the following. >> >> Add cxl_walk_bridge() to iterate the detected error's sub-topology. >> cxl_walk_bridge() is similar to pci_walk_bridge() but the CXL flavor >> will begin iteration at the RP or DSP rather than beginning at the >> first downstream device. > I'm still holding out for making pci_walk_bridge() do the same and seeing > what if anything breaks. I can test AER fatal UCE on a PCIe device. Do you have any other ideas for specific testing? A specific device or topology in mind ? Regards, Terry > Other than that I'm fine with this patch. > >> Add cxl_report_error_detected() as an analog to report_error_detected(). >> It will call pci_driver::cxl_err_handlers for each iterated downstream >> device. The pci_driver::cxl_err_handler's UCE handler returns a boolean >> indicating if there was a UCE error detected during handling. >> >> cxl_do_recovery() uses the status from cxl_report_error_detected() to >> determine how to proceed. Non-fatal CXL UCE errors will be treated as >> fatal. If a UCE was present during handling then cxl_do_recovery() >> will kernel panic. >> >> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
On Tue, 14 Jan 2025 14:28:13 -0600 "Bowman, Terry" <terry.bowman@amd.com> wrote: > On 1/14/2025 5:33 AM, Jonathan Cameron wrote: > > On Tue, 7 Jan 2025 08:38:43 -0600 > > Terry Bowman <terry.bowman@amd.com> wrote: > > > >> Existing recovery procedure for PCIe uncorrectable errors (UCE) does not > >> apply to CXL devices. Recovery can not be used for CXL devices because of > >> potential corruption on what can be system memory. Also, current PCIe UCE > >> recovery, in the case of a Root Port (RP) or Downstream Switch Port (DSP), > >> does not begin at the RP/DSP but begins at the first downstream device. > >> This will miss handling CXL Protocol Errors in a CXL RP or DSP. A separate > >> CXL recovery is needed because of the different handling requirements > >> > >> Add a new function, cxl_do_recovery() using the following. > >> > >> Add cxl_walk_bridge() to iterate the detected error's sub-topology. > >> cxl_walk_bridge() is similar to pci_walk_bridge() but the CXL flavor > >> will begin iteration at the RP or DSP rather than beginning at the > >> first downstream device. > > I'm still holding out for making pci_walk_bridge() do the same and seeing > > what if anything breaks. > > I can test AER fatal UCE on a PCIe device. Do you have any other ideas for specific > testing? A specific device or topology in mind ? It's the interaction with runtime power management usage that worries me and might need wider testing. Maybe it is just a case of sending a patch marked RFT. The other paths are no-op where it matters. Jonathan > > Regards, > Terry > > > Other than that I'm fine with this patch. > > > >> Add cxl_report_error_detected() as an analog to report_error_detected(). > >> It will call pci_driver::cxl_err_handlers for each iterated downstream > >> device. The pci_driver::cxl_err_handler's UCE handler returns a boolean > >> indicating if there was a UCE error detected during handling. > >> > >> cxl_do_recovery() uses the status from cxl_report_error_detected() to > >> determine how to proceed. Non-fatal CXL UCE errors will be treated as > >> fatal. If a UCE was present during handling then cxl_do_recovery() > >> will kernel panic. > >> > >> Signed-off-by: Terry Bowman <terry.bowman@amd.com> >
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 2e40fc63ba31..566ad527e61f 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -711,6 +711,9 @@ 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)); +/* CXL error reporting and handling */ +void cxl_do_recovery(struct pci_dev *dev); + bool pcie_wait_for_link(struct pci_dev *pdev, bool active); int pcie_retrain_link(struct pci_dev *pdev, bool use_lt); diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 79c828bdcb6d..68e957459008 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1025,6 +1025,8 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data) err_handler->error_detected(dev, pci_channel_io_normal); else if (info->severity == AER_FATAL) err_handler->error_detected(dev, pci_channel_io_frozen); + + cxl_do_recovery(dev); } out: device_unlock(&dev->dev); @@ -1049,6 +1051,8 @@ static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info) pdrv->cxl_err_handler->cor_error_detected(dev); pcie_clear_device_status(dev); + } else { + cxl_do_recovery(dev); } } diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index 31090770fffc..bfa5dbbc0e1a 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -276,3 +276,57 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, return status; } + +static void cxl_walk_bridge(struct pci_dev *bridge, + int (*cb)(struct pci_dev *, void *), + void *userdata) +{ + if (cb(bridge, userdata)) + return; + + if (bridge->subordinate) + pci_walk_bus(bridge->subordinate, cb, userdata); +} + +static int cxl_report_error_detected(struct pci_dev *dev, void *data) +{ + const struct cxl_error_handlers *cxl_err_handler; + struct pci_driver *pdrv = dev->driver; + bool *status = data; + + device_lock(&dev->dev); + if (pdrv && pdrv->cxl_err_handler && + pdrv->cxl_err_handler->error_detected) { + cxl_err_handler = pdrv->cxl_err_handler; + *status = cxl_err_handler->error_detected(dev); + } + device_unlock(&dev->dev); + return *status; +} + +void cxl_do_recovery(struct pci_dev *dev) +{ + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus); + int type = pci_pcie_type(dev); + struct pci_dev *bridge; + int status; + + if (type == PCI_EXP_TYPE_ROOT_PORT || + type == PCI_EXP_TYPE_DOWNSTREAM || + type == PCI_EXP_TYPE_UPSTREAM || + type == PCI_EXP_TYPE_ENDPOINT) + bridge = dev; + else + bridge = pci_upstream_bridge(dev); + + cxl_walk_bridge(bridge, cxl_report_error_detected, &status); + if (status) + panic("CXL cachemem error."); + + if (host->native_aer || pcie_ports_native) { + pcie_clear_device_status(dev); + pci_aer_clear_nonfatal_status(dev); + } + + pci_info(bridge, "CXL uncorrectable error.\n"); +}
Existing recovery procedure for PCIe uncorrectable errors (UCE) does not apply to CXL devices. Recovery can not be used for CXL devices because of potential corruption on what can be system memory. Also, current PCIe UCE recovery, in the case of a Root Port (RP) or Downstream Switch Port (DSP), does not begin at the RP/DSP but begins at the first downstream device. This will miss handling CXL Protocol Errors in a CXL RP or DSP. A separate CXL recovery is needed because of the different handling requirements Add a new function, cxl_do_recovery() using the following. Add cxl_walk_bridge() to iterate the detected error's sub-topology. cxl_walk_bridge() is similar to pci_walk_bridge() but the CXL flavor will begin iteration at the RP or DSP rather than beginning at the first downstream device. Add cxl_report_error_detected() as an analog to report_error_detected(). It will call pci_driver::cxl_err_handlers for each iterated downstream device. The pci_driver::cxl_err_handler's UCE handler returns a boolean indicating if there was a UCE error detected during handling. cxl_do_recovery() uses the status from cxl_report_error_detected() to determine how to proceed. Non-fatal CXL UCE errors will be treated as fatal. If a UCE was present during handling then cxl_do_recovery() will kernel panic. Signed-off-by: Terry Bowman <terry.bowman@amd.com> --- drivers/pci/pci.h | 3 +++ drivers/pci/pcie/aer.c | 4 ++++ drivers/pci/pcie/err.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+)