| 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: <20251104184009.00005d27@huawei.com>
Date: Tue, 4 Nov 2025 18:40:09 +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 21/25] PCI/AER: Dequeue forwarded CXL error
On Tue, 4 Nov 2025 11:03:01 -0600
Terry Bowman <terry.bowman@....com> wrote:
> The AER driver now forwards CXL protocol errors to the CXL driver via a
> kfifo. The CXL driver must consume these work items, initiate protocol
> error handling, and ensure RAS mappings remain valid throughout processing.
>
> Implement cxl_proto_err_work_fn() to dequeue work items forwarded by the
> AER service driver and begin protocol error processing by calling
> cxl_handle_proto_error().
>
> Add a PCI device lock on &pdev->dev within cxl_proto_err_work_fn() to
> keep the PCI device structure valid during handling. Locking an Endpoint
> will also defer RAS unmapping until the device is unlocked.
>
> For Endpoints, add a lock on CXL memory device cxlds->dev. The CXL memory
> device structure holds the RAS register reference needed during error
> handling.
>
> Add lock for the parent CXL Port for Root Ports, Downstream Ports, and
> Upstream Ports to prevent destruction of structures holding mapped RAS
> addresses while they are in use.
>
> Invoke cxl_do_recovery() for uncorrectable errors. Treat this as a stub for
> now; implement its functionality in a future patch.
>
> Export pci_clean_device_status() to enable cleanup of AER status following
> error handling.
>
> Signed-off-by: Terry Bowman <terry.bowman@....com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
Various comments inline.
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 142ca8794107..5bc144cde0ee 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -117,17 +117,6 @@ 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);
>
> -int cxl_ras_init(void)
> -{
> - return cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> -}
> -
> -void cxl_ras_exit(void)
> -{
> - cxl_cper_unregister_prot_err_work(&cxl_cper_prot_err_work);
> - 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;
> @@ -178,6 +167,51 @@ static void __iomem *cxl_get_ras_base(struct device *dev)
> return NULL;
> }
>
> +/*
> + * Return 'struct cxl_port *' parent CXL port of dev's
> + *
> + * Reference count increments on success
> + *
> + * dev: Find the parent port of this dev
pdev.
Generally I'd prefer kernel-doc style even for non exported
/ exposed functions. Makes it easy to check for stuff like
this as the script will moan at you.
> + */
> +static struct cxl_port *get_cxl_port(struct pci_dev *pdev)
> +{
> + switch (pci_pcie_type(pdev)) {
> + case PCI_EXP_TYPE_ROOT_PORT:
> + case PCI_EXP_TYPE_DOWNSTREAM:
> + {
> + struct cxl_dport *dport;
> + struct cxl_port *port = find_cxl_port(&pdev->dev, &dport);
> +
> + if (!port) {
> + pci_err(pdev, "Failed to find the CXL device");
> + return NULL;
> + }
> + return port;
> + }
> + case PCI_EXP_TYPE_UPSTREAM:
> + {
> + struct cxl_port *port = find_cxl_port_by_uport(&pdev->dev);
> +
> + if (!port) {
> + pci_err(pdev, "Failed to find the CXL device");
> + return NULL;
> + }
> + return port;
> + }
> + case PCI_EXP_TYPE_ENDPOINT:
> + {
> + struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> + struct cxl_port *port = cxlds->cxlmd->endpoint;
> +
> + get_device(&port->dev);
> + return port;
> + }
> + }
> + pci_warn_once(pdev, "Error: Unsupported device type (%X)", pci_pcie_type(pdev));
> + return NULL;
> +}
> +
> /**
> * cxl_dport_init_ras_reporting - Setup CXL RAS report on this dport
> * @dport: the cxl_dport that needs to be initialized
> @@ -212,6 +246,23 @@ void cxl_uport_init_ras_reporting(struct cxl_port *port,
> }
> EXPORT_SYMBOL_NS_GPL(cxl_uport_init_ras_reporting, "CXL");
>
> +static bool device_lock_if(struct device *dev, bool cond)
> +{
> + if (cond)
> + device_lock(dev);
> + return cond;
> +}
> +
> +static void device_unlock_if(struct device *dev, bool take)
> +{
> + if (take)
> + device_unlock(dev);
> +}
See below. To me these are too weird to wrap up. Open code them inline
where we can see what they are doing.
> +static void cxl_proto_err_work_fn(struct work_struct *work)
> +{
> + struct cxl_proto_err_work_data wd;
> +
> + while (cxl_proto_err_kfifo_get(&wd)) {
> + struct pci_dev *pdev __free(pci_dev_put) = pci_dev_get(wd.pdev);
> + struct device *cxlmd_dev;
> +
> + if (!pdev) {
> + pr_err_ratelimited("NULL PCI device passed in AER-CXL KFIFO\n");
> + continue;
> + }
> +
> + guard(device)(&pdev->dev);
> + if (is_pcie_endpoint(pdev)) {
> + struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> +
> + if (!cxl_pci_drv_bound(pdev))
> + return;
> + cxlmd_dev = &cxlds->cxlmd->dev;
> + device_lock_if(cxlmd_dev, cxlmd_dev);
As below. Too odd. Also needs comments to explain why conditionally locking it
would be useful.
> + } else {
> + cxlmd_dev = NULL;
Set it to NULL at declaration and drop this else leg.
> + }
> +
> + struct cxl_port *port __free(put_cxl_port) = get_cxl_port(pdev);
> + if (!port)
> + return;
> + guard(device)(&port->dev);
> +
> + cxl_handle_proto_error(&wd);
> + device_unlock_if(cxlmd_dev, cxlmd_dev);
This is too odd to wrap up like that. Particularly given the
very generic sounding device_unlock_if() naming.
> + }
> +}
Powered by blists - more mailing lists