| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251104182953.00006a16@huawei.com>
Date: Tue, 4 Nov 2025 18:29:53 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Terry Bowman <terry.bowman@....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>, <alucerop@....com>, <ira.weiny@...el.com>,
<linux-kernel@...r.kernel.org>, <linux-pci@...r.kernel.org>
Subject: Re: [RESEND v13 19/25] cxl/pci: Introduce CXL protocol error
handlers for Endpoints
On Tue, 4 Nov 2025 11:02:59 -0600
Terry Bowman <terry.bowman@....com> wrote:
> CXL Endpoint protocol errors are currently handled by generic PCI error
> handlers. However, uncorrectable errors (UCEs) require CXL.mem protocol-
> specific handling logic that the PCI handlers cannot provide.
>
> Add dedicated CXL protocol error handlers for CXL Endpoints. Rename the
> existing cxl_error_handlers to pci_error_handlers to better reflect their
> purpose and maintain naming consistency. Update the PCI error handlers to
> invoke the new CXL protocol handlers when the endpoint is operating in
> CXL.mem mode.
>
> Implement cxl_handle_ras() to return PCI_ERS_RESULT_NONE or
> PCI_ERS_RESULT_PANIC. Remove unnecessary result checks from the previous
> endpoint UCE handler since CXL UCE recovery is not implemented in this
> patch.
>
> Add device lock assertions to protect against concurrent device or RAS
> register removal during error handling. Two devices require locking for
> CXL endpoints:
>
> 1. The PCI device (pdev->dev) - RAS registers are allocated and mapped
> using devm_* functions with this device as the host. Locking prevents
> the RAS registers from being unmapped until after error handling
> completes.
>
> 2. The CXL memory device (cxlmd->dev) - Holds a reference to the RAS
> registers accessed during error handling. Locking prevents the memory
> device and its RAS register references from being removed during error
> handling.
>
> The lock assertions added here will be satisfied by device locks
> introduced in a subsequent patch. A future patch will extend the CXL UCE
> handler to support full UCE recovery.
>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
Hi Terry,
A few comments inline.
Thanks,
Jonathan
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index cb712772de5c..beb142054bda 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -128,6 +128,11 @@ void cxl_ras_exit(void)
> cancel_work_sync(&cxl_cper_prot_err_work);
> }
>
> +static bool is_pcie_endpoint(struct pci_dev *pdev)
> +{
> + return pci_pcie_type(pdev) == PCI_EXP_TYPE_ENDPOINT;
> +}
Not used that I can see. Maybe should be in a different patch?
> }
> EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, "CXL");
>
> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> - pci_channel_state_t state)
> +void pci_cor_error_detected(struct pci_dev *pdev)
> +{
> + struct cxl_dev_state *cxlds;
> +
> + device_lock_assert(&pdev->dev);
> + if (!cxl_pci_drv_bound(pdev))
> + return;
> +
> + cxlds = pci_get_drvdata(pdev);
> + guard(device)(&cxlds->cxlmd->dev);
> +
> + cxl_cor_error_detected(&pdev->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(pci_cor_error_detected, "CXL");
Similarly to below. I'm not keen on exporting such generic PCI
sounding functions even in the CXL namespace.
> +
> +pci_ers_result_t cxl_error_detected(struct device *dev)
> {
> - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> - struct cxl_memdev *cxlmd = cxlds->cxlmd;
> - struct device *dev = &cxlmd->dev;
> - bool ue;
> + struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> + struct cxl_dev_state *cxlds = cxlmd->cxlds;
>
> - guard(device)(dev);
> + device_lock_assert(cxlds->dev);
> + device_lock_assert(&cxlmd->dev);
>
> if (!dev->driver) {
> - dev_warn(&pdev->dev,
> + dev_warn(cxlds->dev,
> "%s: memdev disabled, abort error handling\n",
> dev_name(dev));
> return PCI_ERS_RESULT_DISCONNECT;
> @@ -289,32 +308,34 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
>
> if (cxlds->rcd)
> cxl_handle_rdport_errors(cxlds);
> +
I'd drop this blank line addition as it doesn't matter much and it does
add noise to the patch.
> /*
> * A frozen channel indicates an impending reset which is fatal to
> * CXL.mem operation, and will likely crash the system. On the off
> * chance the situation is recoverable dump the status of the RAS
> * capability registers and bounce the active state of the memdev.
> */
Mind you - I think this comment wants to go away as it's talking about code
that is no longer here.
> - ue = cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
> -
> - switch (state) {
> - case pci_channel_io_normal:
> - if (ue) {
> - device_release_driver(dev);
> - return PCI_ERS_RESULT_NEED_RESET;
> - }
> - return PCI_ERS_RESULT_CAN_RECOVER;
> - case pci_channel_io_frozen:
> - dev_warn(&pdev->dev,
> - "%s: frozen state error detected, disable CXL.mem\n",
> - dev_name(dev));
> - device_release_driver(dev);
> - return PCI_ERS_RESULT_NEED_RESET;
> - case pci_channel_io_perm_failure:
> - dev_warn(&pdev->dev,
> - "failure state error detected, request disconnect\n");
> - return PCI_ERS_RESULT_DISCONNECT;
> - }
> - return PCI_ERS_RESULT_NEED_RESET;
> + return cxl_handle_ras(&cxlds->cxlmd->dev, cxlds->serial, cxlds->regs.ras);
> }
> EXPORT_SYMBOL_NS_GPL(cxl_error_detected, "CXL");
> +
> +pci_ers_result_t pci_error_detected(struct pci_dev *pdev,
> + pci_channel_state_t error)
> +{
> + struct cxl_dev_state *cxlds;
> + pci_ers_result_t rc;
> +
> + device_lock_assert(&pdev->dev);
> + if (!cxl_pci_drv_bound(pdev))
> + return PCI_ERS_RESULT_NONE;
> +
> + cxlds = pci_get_drvdata(pdev);
> + guard(device)(&cxlds->cxlmd->dev);
> +
> + rc = cxl_error_detected(&cxlds->cxlmd->dev);
> + if (rc == PCI_ERS_RESULT_PANIC)
> + panic("CXL cachemem error.");
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(pci_error_detected, "CXL");
Whilst the symbol is namespaced, I'm not sure I want to see
an exported CXL specific function that sounds so generic pci.
Maybe cxl_pci_error_detected() or something like that?
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index a0a491e7b5b9..3526e6d75f79 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -79,21 +79,10 @@ struct cxl_dev_state;
> void read_cdat_data(struct cxl_port *port);
>
> #ifdef CONFIG_CXL_RAS
> -void cxl_cor_error_detected(struct pci_dev *pdev);
> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> - pci_channel_state_t state);
> void cxl_dport_init_ras_reporting(struct cxl_dport *dport, struct device *host);
> void cxl_uport_init_ras_reporting(struct cxl_port *port,
> struct device *host);
> #else
> -static inline void cxl_cor_error_detected(struct pci_dev *pdev) { }
> -
> -static inline pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> - pci_channel_state_t state)
> -{
> - return PCI_ERS_RESULT_NONE;
> -}
> -
> static inline void cxl_dport_init_ras_reporting(struct cxl_dport *dport,
> struct device *host) { }
> static inline void cxl_uport_init_ras_reporting(struct cxl_port *port,
Powered by blists - more mailing lists