| Message ID | 20260902133933.2992457-9-terry.bowman@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable CXL PCIe Port Protocol Error handling and logging | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Devres memory and action leak on CXL endpoint port unbind/rebind for RCH topologies. -- commit 6742f1a341d88bb57310da7468fadf1a3f3b4556 Author: Terry Bowman <terry.bowman@amd.com> PCI/CXL: Mask/Unmask CXL protocol errors This commit introduces helpers to mask and unmask CXL protocol errors. It unmasks internal errors in cxl_dport_map_ras() and devm_cxl_port_ras_setup() after the RAS register block is successfully mapped, and adds a devm action to restore the mask on teardown. > diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c > index fac37b6fd882f..f1e05d240059b 100644 > --- a/drivers/cxl/core/ras.c > +++ b/drivers/cxl/core/ras.c [ ... ] > @@ -124,16 +124,64 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work) > } > static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn); [ ... ] > static void cxl_dport_map_ras(struct cxl_dport *dport) > { > struct cxl_register_map *map = &dport->reg_map; > struct device *dev = dport->dport_dev; > > - if (!map->component_map.ras.valid) > + if (!map->component_map.ras.valid) { > dev_dbg(dev, "RAS registers not found\n"); > - else if (cxl_map_component_regs(map, &dport->regs.component, > - BIT(CXL_CM_CAP_CAP_ID_RAS))) > + return; > + } > + > + if (cxl_map_component_regs(map, &dport->regs.component, > + BIT(CXL_CM_CAP_CAP_ID_RAS))) { > dev_dbg(dev, "Failed to map RAS capability.\n"); > + return; > + } > + > + if (!dev_is_pci(dev)) > + return; > + > + cxl_unmask_proto_interrupts(dev); > + if (devm_add_action_or_reset(dport_to_host(dport), > + cxl_mask_proto_irqs, dev)) { [Severity: High] Does this devres action cause an unbounded memory and action leak when a CXL endpoint port is unbound and rebound in RCH topologies? In cxl_endpoint_port_probe() in drivers/cxl/port.c, when dport->rch is true, it calls devm_cxl_dport_rch_ras_setup(dport). This executes repeatedly across unbind/bind cycles of the endpoint port. That setup path eventually calls cxl_dport_map_ras(dport), which registers the cxl_mask_proto_irqs action on the device returned by dport_to_host(dport). For RCH topologies, dport_to_host(dport) evaluates to the parent root port's host device (the ACPI host bridge), not the endpoint port being probed. Because the action is tied to the parent host device instead of the endpoint port, unbinding and rebinding the endpoint port does not free the devres action. This leaves a leaked action on the parent host device each time the endpoint port is probed. > + dev_warn(dev, "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n"); > + dport->regs.component.ras = NULL; > + } > }
On 9/2/2026 9:03 AM, sashiko-bot@kernel.org wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [High] Devres memory and action leak on CXL endpoint port unbind/rebind for RCH topologies. > -- > > commit 6742f1a341d88bb57310da7468fadf1a3f3b4556 > Author: Terry Bowman <terry.bowman@amd.com> > > PCI/CXL: Mask/Unmask CXL protocol errors > > This commit introduces helpers to mask and unmask CXL protocol errors. It > unmasks internal errors in cxl_dport_map_ras() and devm_cxl_port_ras_setup() > after the RAS register block is successfully mapped, and adds a devm action > to restore the mask on teardown. > >> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c >> index fac37b6fd882f..f1e05d240059b 100644 >> --- a/drivers/cxl/core/ras.c >> +++ b/drivers/cxl/core/ras.c > > [ ... ] > >> @@ -124,16 +124,64 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work) >> } >> static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn); > > [ ... ] > >> static void cxl_dport_map_ras(struct cxl_dport *dport) >> { >> struct cxl_register_map *map = &dport->reg_map; >> struct device *dev = dport->dport_dev; >> >> - if (!map->component_map.ras.valid) >> + if (!map->component_map.ras.valid) { >> dev_dbg(dev, "RAS registers not found\n"); >> - else if (cxl_map_component_regs(map, &dport->regs.component, >> - BIT(CXL_CM_CAP_CAP_ID_RAS))) >> + return; >> + } >> + >> + if (cxl_map_component_regs(map, &dport->regs.component, >> + BIT(CXL_CM_CAP_CAP_ID_RAS))) { >> dev_dbg(dev, "Failed to map RAS capability.\n"); >> + return; >> + } >> + >> + if (!dev_is_pci(dev)) >> + return; >> + >> + cxl_unmask_proto_interrupts(dev); >> + if (devm_add_action_or_reset(dport_to_host(dport), >> + cxl_mask_proto_irqs, dev)) { > > [Severity: High] > Does this devres action cause an unbounded memory and action leak when a CXL > endpoint port is unbound and rebound in RCH topologies? > No. CXL 1.1 RCD/RCH is not hotpluggable — the RCH Downstream Port and RCD are enumerated once by platform firmware and are never hot-removed or rebound. The unbind/rebind cycle this accumulation depends on does not occur, so the action is registered once, not repeatedly. CXL4.0 - 9.9 Hot-Plug "By definition, RCDs and RCHs do not support Hot-Plug."* Terry > In cxl_endpoint_port_probe() in drivers/cxl/port.c, when dport->rch is true, > it calls devm_cxl_dport_rch_ras_setup(dport). This executes repeatedly across > unbind/bind cycles of the endpoint port. > > That setup path eventually calls cxl_dport_map_ras(dport), which registers > the cxl_mask_proto_irqs action on the device returned by > dport_to_host(dport). > > For RCH topologies, dport_to_host(dport) evaluates to the parent root port's > host device (the ACPI host bridge), not the endpoint port being probed. > > Because the action is tied to the parent host device instead of the endpoint > port, unbinding and rebinding the endpoint port does not free the devres > action. This leaves a leaked action on the parent host device each time the > endpoint port is probed. > >> + dev_warn(dev, "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n"); >> + dport->regs.component.ras = NULL; >> + } >> } >
On 9/2/2026 8:39 AM, Terry Bowman wrote: > CXL protocol errors must be unmasked to be reported. Add > pci_aer_mask_internal_errors() as the symmetric counterpart to > pci_aer_unmask_internal_errors() and export both for cxl_core. > > Unmask CXL internal errors in cxl_dport_map_ras() and > devm_cxl_port_ras_setup() after the RAS register block is > successfully mapped. Register a devm action to restore the mask > on teardown. The unmask/mask helpers gate on dev_is_pci() and > pcie_aer_is_native() internally so callers need no special-casing. > > Remove the dev_is_pci(dport->dport_dev) guard in > devm_cxl_dport_rch_ras_setup(). On RCH systems dport->dport_dev > is the pci_host_bridge device which is not on pci_bus_type, so > this guard blocked real hardware. The caller already gates on > dport->rch. > > Co-developed-by: Dan Williams <djbw@kernel.org> > Signed-off-by: Dan Williams <djbw@kernel.org> > Signed-off-by: Terry Bowman <terry.bowman@amd.com> > Reviewed-by: Dave Jiang <dave.jiang@intel.com> > Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com> > Reviewed-by: Alison Schofield <alison.schofield@intel.com> > > --- Reviewed-by: Ben Cheatham <benjamin.cheatham@amd.com> Thanks, Ben
diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c index fac37b6fd882f..f1e05d240059b 100644 --- a/drivers/cxl/core/ras.c +++ b/drivers/cxl/core/ras.c @@ -124,16 +124,64 @@ static void cxl_cper_prot_err_work_fn(struct work_struct *work) } static DECLARE_WORK(cxl_cper_prot_err_work, cxl_cper_prot_err_work_fn); +static void cxl_unmask_proto_interrupts(struct device *dev) +{ + struct pci_dev *pdev; + + if (!dev || !dev_is_pci(dev)) + return; + + pdev = to_pci_dev(dev); + if (!pcie_aer_is_native(pdev)) + return; + + pci_aer_unmask_internal_errors(pdev); +} + +static void cxl_mask_proto_interrupts(struct device *dev) +{ + struct pci_dev *pdev; + + if (!dev || !dev_is_pci(dev)) + return; + + pdev = to_pci_dev(dev); + if (!pcie_aer_is_native(pdev)) + return; + + pci_aer_mask_internal_errors(pdev); +} + +static void cxl_mask_proto_irqs(void *dev) +{ + cxl_mask_proto_interrupts(dev); +} + static void cxl_dport_map_ras(struct cxl_dport *dport) { struct cxl_register_map *map = &dport->reg_map; struct device *dev = dport->dport_dev; - if (!map->component_map.ras.valid) + if (!map->component_map.ras.valid) { dev_dbg(dev, "RAS registers not found\n"); - else if (cxl_map_component_regs(map, &dport->regs.component, - BIT(CXL_CM_CAP_CAP_ID_RAS))) + return; + } + + if (cxl_map_component_regs(map, &dport->regs.component, + BIT(CXL_CM_CAP_CAP_ID_RAS))) { dev_dbg(dev, "Failed to map RAS capability.\n"); + return; + } + + if (!dev_is_pci(dev)) + return; + + cxl_unmask_proto_interrupts(dev); + if (devm_add_action_or_reset(dport_to_host(dport), + cxl_mask_proto_irqs, dev)) { + dev_warn(dev, "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n"); + dport->regs.component.ras = NULL; + } } /** @@ -150,9 +198,6 @@ void devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport) { struct pci_host_bridge *host_bridge; - if (!dev_is_pci(dport->dport_dev)) - return; - devm_cxl_dport_ras_setup(dport); host_bridge = to_pci_host_bridge(dport->dport_dev); @@ -167,6 +212,7 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_dport_rch_ras_setup, "CXL"); void devm_cxl_port_ras_setup(struct cxl_port *port) { struct cxl_register_map *map = &port->reg_map; + struct device *dev; if (!map->component_map.ras.valid) { dev_dbg(&port->dev, "RAS registers not found\n"); @@ -175,8 +221,21 @@ void devm_cxl_port_ras_setup(struct cxl_port *port) map->host = &port->dev; if (cxl_map_component_regs(map, &port->regs, - BIT(CXL_CM_CAP_CAP_ID_RAS))) + BIT(CXL_CM_CAP_CAP_ID_RAS))) { dev_dbg(&port->dev, "Failed to map RAS capability\n"); + return; + } + + dev = is_cxl_endpoint(port) ? port->uport_dev->parent : port->uport_dev; + if (!dev_is_pci(dev)) + return; + + cxl_unmask_proto_interrupts(dev); + if (devm_add_action_or_reset(&port->dev, cxl_mask_proto_irqs, dev)) { + dev_warn(&port->dev, + "failed to defer CXL proto-irq mask; CXL protocol error reporting disabled\n"); + port->regs.ras = NULL; + } } EXPORT_SYMBOL_NS_GPL(devm_cxl_port_ras_setup, "CXL"); diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 8c998cffa89e4..1b182b9cc9553 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -1281,12 +1281,26 @@ void pci_aer_unmask_internal_errors(struct pci_dev *dev) mask &= ~PCI_ERR_COR_INTERNAL; pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask); } +EXPORT_SYMBOL_FOR_MODULES(pci_aer_unmask_internal_errors, "cxl_core"); -/* - * Internal errors are too device-specific to enable generally, however for CXL - * their behavior is standardized for conveying CXL protocol errors. +/** + * pci_aer_mask_internal_errors - mask internal errors + * @dev: pointer to the pci_dev data structure + * + * Mask internal errors in the Uncorrectable and Correctable Error + * Mask registers. + * + * Note: AER must be enabled and supported by the device which must be + * checked in advance, e.g. with pcie_aer_is_native(). */ -EXPORT_SYMBOL_FOR_MODULES(pci_aer_unmask_internal_errors, "cxl_core"); +void pci_aer_mask_internal_errors(struct pci_dev *dev) +{ + pci_clear_and_set_config_dword(dev, dev->aer_cap + PCI_ERR_UNCOR_MASK, + 0, PCI_ERR_UNC_INTN); + pci_clear_and_set_config_dword(dev, dev->aer_cap + PCI_ERR_COR_MASK, + 0, PCI_ERR_COR_INTERNAL); +} +EXPORT_SYMBOL_FOR_MODULES(pci_aer_mask_internal_errors, "cxl_core"); /** * pci_aer_handle_error - handle logging error into an event log diff --git a/include/linux/aer.h b/include/linux/aer.h index 8eba3192e2d15..b3657b80564b9 100644 --- a/include/linux/aer.h +++ b/include/linux/aer.h @@ -58,6 +58,7 @@ struct aer_capability_regs { int pci_aer_clear_nonfatal_status(struct pci_dev *dev); int pcie_aer_is_native(struct pci_dev *dev); void pci_aer_unmask_internal_errors(struct pci_dev *dev); +void pci_aer_mask_internal_errors(struct pci_dev *dev); #else static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) { @@ -65,6 +66,7 @@ static inline int pci_aer_clear_nonfatal_status(struct pci_dev *dev) } static inline int pcie_aer_is_native(struct pci_dev *dev) { return 0; } static inline void pci_aer_unmask_internal_errors(struct pci_dev *dev) { } +static inline void pci_aer_mask_internal_errors(struct pci_dev *dev) { } #endif #ifdef CONFIG_CXL_RAS diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild index 2be1df80fcc93..957945201f04d 100644 --- a/tools/testing/cxl/Kbuild +++ b/tools/testing/cxl/Kbuild @@ -6,6 +6,7 @@ ldflags-y += --wrap=acpi_pci_find_root ldflags-y += --wrap=nvdimm_bus_register ldflags-y += --wrap=cxl_await_media_ready ldflags-y += --wrap=devm_cxl_add_rch_dport +ldflags-y += --wrap=devm_cxl_dport_rch_ras_setup ldflags-y += --wrap=cxl_endpoint_parse_cdat ldflags-y += --wrap=devm_cxl_endpoint_decoders_setup ldflags-y += --wrap=hmat_get_extended_linear_cache_size diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c index 6454b868b122c..5ad3243da8d29 100644 --- a/tools/testing/cxl/test/mock.c +++ b/tools/testing/cxl/test/mock.c @@ -220,6 +220,18 @@ struct cxl_dport *__wrap_devm_cxl_add_rch_dport(struct cxl_port *port, } EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_add_rch_dport, "CXL"); +void __wrap_devm_cxl_dport_rch_ras_setup(struct cxl_dport *dport) +{ + int index; + struct cxl_mock_ops *ops = get_cxl_mock_ops(&index); + + if (!ops || !ops->is_mock_port(dport->dport_dev)) + devm_cxl_dport_rch_ras_setup(dport); + + put_cxl_mock_ops(index); +} +EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_dport_rch_ras_setup, "CXL"); + void __wrap_cxl_endpoint_parse_cdat(struct cxl_port *port) { int index;