* Re: [PATCH v18 05/13] PCI/AER: Introduce AER-CXL protocol error kfifo
From: Jonathan Cameron @ 2026-07-20 22:41 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma, Ashok Raj
In-Reply-To: <20260717222706.3540281-6-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:26:58 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
Hi Terry.
This is a rather length patch description. Might be worth
a parse to see if key details can be covered in something that folk
are more likely to read!
> CXL VH RAS handling requires a path for the AER driver to hand off
> CXL protocol errors to cxl_core for logging and recovery before PCIe
> AER recovery tears down the device. Add
> drivers/pci/pcie/aer_cxl_vh.c to implement this handoff via a kfifo-backed
> work item.
>
> Introduce is_aer_internal_error() to identify CXL protocol errors
> from AER internal error status bits across both correctable and
> uncorrectable severities.
Already existed. Just in a different location.
>
> Introduce is_cxl_error() to gate the VH kfifo path.
Feels like to much detail to me given not really anything to say
about it other than the obvious.
>
> Introduce struct cxl_proto_err_work_data to carry the error source
> PCI device and severity through the kfifo. Encapsulate the kfifo,
> per-producer spinlock, registration rwsem, and work pointer in struct
> cxl_proto_err_kfifo. Initialize the embedded kfifo via INIT_KFIFO()
Can we cut this down a little where we don't use useful info. e.g.
"Initialize the kfifo from a subsys_initcall() to it is ready before
any producer or consumer runs."
> from a subsys_initcall so its metadata is populated before any
> producer or consumer runs.
>
> Introduce cxl_forward_error() to enqueue a CXL protocol error. A
> reference is taken on the PCI device; the consumer releases it via
> for_each_cxl_proto_err(). On enqueue failure the reference is
> released immediately, the error is dropped, and the consumer is
> scheduled to drain existing entries.
We don't need to cover error paths in the patch description unless
they are really complex and need explanation. Even then probably belongs
more in comments.
Anyhow you get the idea..
> A subsequent patch wires
> cxl_forward_error() into handle_error_source() where correctable and
> uncorrectable status clearing is left to pci_aer_handle_error().
>
> Introduce cxl_proto_err_flush() to synchronously wait for the
> consumer worker to drain the kfifo. A subsequent patch wires this
> into handle_error_source() for UCE events so the CXL plane completes
> error handling and panic policy before pci_aer_handle_error() drives
> PCIe recovery.
>
> Introduce cxl_register_proto_err_work() and
> cxl_unregister_proto_err_work() for cxl_core to register and
> deregister its work handler. On unregistration, pending kfifo entries
> are drained and their pdev references released before
> cancel_work_sync() runs. Export these and for_each_cxl_proto_err()
> via EXPORT_SYMBOL_FOR_MODULES restricted to cxl_core.
>
> Protect the work pointer with a rwsem to correctly serialize
> registration, deregistration, enqueue, and dequeue against concurrent
> AER IRQ threads. Serialize concurrent kfifo writers with a spinlock.
>
> Add MAINTAINERS entries for aer_cxl_vh.c and aer_cxl_rch.c under
> the CXL entry so CXL maintainers are CC'd on changes to the AER-CXL
> bridging code.
Various things inline. The one potential thing I'd like to highlight is
the loss of tracking assume panic isn't appropriate. My paranoid hat
says always go the other way. If we don't know we didn't lose an
uncorrectable error panic. Maybe it's worth us considering when this
might happen in a real system?
>
> 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>
>
> ---
>
> Changes in v17->v18:
> - Remove correctable status clear from cxl_forward_error(); the AER core
> clears all status bits via pci_aer_handle_error() info->status writeback
> - Schedule consumer on kfifo overflow so existing entries can be drained
>
> Changes in v16->v17:
> - Reword "kfifo semaphore" to "kfifo spinlock" to match fifo_lock.
> - Defer the handle_error_source() is_cxl_error() switch to the patch that
> registers the kfifo consumer to keep each commit bisect-safe.
> - Rename rwsema to rwsem
> - Change CPER exports to use EXPORT_SYMBOL_FOR_MODULES.
> - Add work cancel function.
> - Replace kfifo_put() with kfifo_in_spinlocked() for multiple producers
> - Add fifo_lock spinlock for concurrent producer serialisation
> - Initialize the embedded kfifo with INIT_KFIFO() in a subsys_initcall so
> kfifo->mask, ->esize and ->data are set before first use.
> - Clear PCI_ERR_COR_STATUS in cxl_forward_error() after enqueue so the
> device is acked for correctable events even when the consumer drops the
> event. Uncorrectable status is left for cxl_do_recovery() to clear after
> recovery completes, mirroring the AER core convention.
> - WARN on double-registration in cxl_register_proto_err_work() to make an
> unintended second consumer visible at runtime.
> - Add direct rwsem.h, cleanup.h and workqueue.h includes for symbols used
> in aer_cxl_vh.c
> - Add MAINTAINERS entries for drivers/pci/pcie/aer_cxl_*.c
> - Update message
> ---
> MAINTAINERS | 2 +
> drivers/pci/pcie/Makefile | 1 +
> drivers/pci/pcie/aer.c | 10 --
> drivers/pci/pcie/aer_cxl_vh.c | 221 ++++++++++++++++++++++++++++++++++
> drivers/pci/pcie/portdrv.h | 6 +
> include/linux/aer.h | 24 ++++
> 6 files changed, 254 insertions(+), 10 deletions(-)
> create mode 100644 drivers/pci/pcie/aer_cxl_vh.c
>
> diff --git a/drivers/pci/pcie/aer_cxl_vh.c b/drivers/pci/pcie/aer_cxl_vh.c
> new file mode 100644
> index 0000000000000..93bed07936100
> --- /dev/null
> +++ b/drivers/pci/pcie/aer_cxl_vh.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2026 AMD Corporation. All rights reserved. */
> +
> +#include <linux/aer.h>
> +#include <linux/atomic.h>
> +#include <linux/cleanup.h>
> +#include <linux/init.h>
> +#include <linux/kfifo.h>
> +#include <linux/rwsem.h>
Check these. I'd expect at least spinlock.h to meet the rough include what
you use aim.
> +#include <linux/wait_bit.h>
> +#include <linux/workqueue.h>
> +#include "../pci.h"
> +#include "portdrv.h"
> +
> +#define CXL_ERROR_SOURCES_MAX 128
> +
> +struct cxl_proto_err_kfifo {
> + struct work_struct *work;
> + void (*flush)(void);
> + struct rw_semaphore rwsem;
> + spinlock_t fifo_lock;
Ideally add a quick comment to every lock to say what data it covers.
Kind of obvious for this one but in general it is good practice and
reduces chance of later scope confusion.
> + atomic_t flush_inflight;
> + DECLARE_KFIFO(fifo, struct cxl_proto_err_work_data,
> + CXL_ERROR_SOURCES_MAX);
> +};
> +/**
> + * cxl_forward_error - Forward a CXL protocol error to the CXL subsystem via kfifo
> + * @pdev: PCI device that reported the AER error
> + * @info: AER error info containing severity and status
> + *
> + * Producer side of the AER-CXL kfifo. Enqueues a CXL protocol error work
> + * item and schedules the consumer workqueue. Takes a reference on @pdev
> + * that the consumer releases after handling.
> + *
> + * Return: true if the caller must flush the kfifo before AER recovery,
> + * false if no CXL error handling was initiated due to early return on
> + * error.
> + */
> +bool cxl_forward_error(struct pci_dev *pdev, struct aer_err_info *info)
> +{
> + struct cxl_proto_err_work_data wd = {
> + .severity = info->severity,
> + .pdev = pdev,
> + };
> +
> + guard(rwsem_read)(&cxl_proto_err_kfifo.rwsem);
> +
> + if (!cxl_proto_err_kfifo.work) {
> + dev_err_ratelimited(&pdev->dev, "AER-CXL kfifo reader not registered\n");
> + return false;
> + }
> +
> + /*
> + * Reference discipline: the AER caller (handle_error_source())
> + * holds a ref on @pdev for the duration of this call and releases
> + * it on return. Take a fresh ref here so the pdev stays live while
> + * queued in the kfifo; the consumer (for_each_cxl_proto_err())
> + * drops that ref after handling. On enqueue failure below, drop
> + * the ref we just took to avoid a leak.
Most of this is useful. The what we do in error handling (given so local)
probably not.
> + */
> + pci_dev_get(pdev);
> +
> + /* Serialize concurrent kfifo writers: multiple AER threaded IRQs */
> + if (!kfifo_in_spinlocked(&cxl_proto_err_kfifo.fifo, &wd, 1,
> + &cxl_proto_err_kfifo.fifo_lock)) {
> + /* Dropped; no panic - UCE unconfirmed without RAS read */
Hmm. That's interesting. To me it fails the normal ras thing of assume
the worst if we lost track. I'd panic. But I'm open to other views on this!
> + dev_err_ratelimited(&pdev->dev, "AER-CXL kfifo add failed\n");
> + pci_dev_put(pdev);
> + schedule_work(cxl_proto_err_kfifo.work);
Shared with below. Maybe just drop out of the if unless this gets more
complex in later patches.
> + return true;
> + }
> +
> + schedule_work(cxl_proto_err_kfifo.work);
> + return true;
> +}
> +/**
> + * for_each_cxl_proto_err - Call a function for each kfifo work item
> + *
> + * Single-consumer invariant: this function is only called from
> + * cxl_proto_err_work_fn() via a single DECLARE_WORK.
> + *
> + * Holds rwsem_read internally; fn() must not call cxl_register_proto_err_work()
> + * or cxl_unregister_proto_err_work().
> + */
> +void for_each_cxl_proto_err(struct cxl_proto_err_work_data *wd,
> + cxl_proto_err_fn_t fn)
> +{
> + guard(rwsem_read)(&cxl_proto_err_kfifo.rwsem);
> + while (kfifo_get(&cxl_proto_err_kfifo.fifo, wd)) {
> + fn(wd);
Like the earlier case I razed, I'd like a comment here to say where
the reference we re releasing was taken.
> + pci_dev_put(wd->pdev);
> + }
> +}
> +EXPORT_SYMBOL_FOR_MODULES(for_each_cxl_proto_err, "cxl_core");
> +
> +/**
> + * cxl_proto_err_flush - drain pending AER-CXL kfifo work synchronously
> + *
> + * Wait for the consumer worker to finish processing all entries
> + * currently in the kfifo. Used by handle_error_source() for UCE so
> + * the CXL plane can read CXL RAS, apply panic policy, and clear CXL
> + * state before pci_aer_handle_error() drives PCIe recovery.
> + *
> + * Snapshots the flush callback under rwsem_read and releases the rwsem
> + * before calling it. This avoids holding rwsem_read across flush_work(),
> + * which would deadlock via the rwsem HANDOFF mechanism when a concurrent
> + * rwsem_write waiter (cxl_unregister_proto_err_work) blocks new readers
> + * including the worker's for_each_cxl_proto_err() rwsem_read acquisition.
> + *
> + * The flush_inflight counter prevents cxl_core module unload while a
> + * flush is in progress outside the rwsem. The counter is incremented
> + * under rwsem_read (mutually exclusive with the rwsem_write in
> + * cancel_cxl_proto_err() that NULLs the flush pointer) and decremented
> + * after the flush completes. cxl_unregister_proto_err_work() waits for
> + * the counter to reach zero before proceeding with cancel_work_sync().
I can't see it from just this patch so it might be useful to say if the
counter is ever not equal to 1 or 0. If it isn't can we make that explicit
in the code?
> + *
> + * For correctable events the consumer can run asynchronously; AER
> + * does not need to call this helper for AER_CORRECTABLE.
For me this is both too much and likely to rot over time. Some stuff
feels like it belongs with the flags it is talking about rather than here.
> + */
> +void cxl_proto_err_flush(void)
> +{
> + void (*flush)(void);
> +
> + scoped_guard(rwsem_read, &cxl_proto_err_kfifo.rwsem) {
> + flush = cxl_proto_err_kfifo.flush;
> + if (flush)
> + atomic_inc(&cxl_proto_err_kfifo.flush_inflight);
> + }
> +
> + if (flush) {
> + flush();
> + if (atomic_dec_and_test(&cxl_proto_err_kfifo.flush_inflight))
> + wake_up_var(&cxl_proto_err_kfifo.flush_inflight);
> + }
> +}
^ permalink raw reply
* Re: [PATCH v7 03/12] PCI: liveupdate: Track incoming preserved PCI devices
From: Pasha Tatashin @ 2026-07-20 22:44 UTC (permalink / raw)
To: David Matlack
Cc: Pasha Tatashin, kexec, linux-doc, linux-kernel, linux-mm,
linux-pci, Adithya Jayachandran, Alexander Graf, Alex Williamson,
Bjorn Helgaas, Chris Li, David Rientjes, Jacob Pan,
Jason Gunthorpe, Jonathan Corbet, Josh Hilke, Leon Romanovsky,
Lukas Wunner, Mike Rapoport, Parav Pandit, Pranjal Shrivastava,
Pratyush Yadav, Saeed Mahameed, Samiullah Khawaja, Shuah Khan,
Vipin Sharma, William Tu, Yi Liu
In-Reply-To: <CALzav=e1Gcu-9TD7F--41cYqKBrStKOE4VqQ4K3PKdNuYnNmcQ@mail.gmail.com>
On 2026-07-20 14:54:51-07:00, David Matlack wrote:
> On Fri, Jul 17, 2026 at 2:47 PM Pasha Tatashin
> <pasha.tatashin@soleen.com> wrote:
>
> > On Fri, 10 Jul 2026 21:26:06 +0000, David Matlack <dmatlack@google.com> wrote:
> >
> > I am confused by this. Each preserved PCI device must have an associated
> > FD preserved with it via LUO. I.e., vfiofd would need to be preserved. If
> > the vfiofd was not reclaimed, and finish is not possible, that vfiofd
> > would still be owned by LUO, and therefore PCI FLB refcount would stay
> > positive.
> >
> > However, if finish is possible, and this is the last vfiofd that is
> > finished, FLB will be freed as soon as the reference count reaches zero,
> > which I would think is the expected behavior.
> >
> > What is the point of holding a reference here, instead of only for the
> > duration of FLB access, i.e. to make sure we are accessing a valid data?
>
> The duration of the access is from here until pci_liveupdate_finish()
> because that it when the pointer (dev->liveupdate.incoming) is
> cleared. So that is why the PCI core holds the reference from here
> until pci_liveupdate_finish().
>
> We could avoid this by deleteing dev->liveupdate.incoming and,
> instead, fetching the incoming FLB and doing the xarray lookup every
> atime the PCI core needs to access the device's incoming ser struct,
> but that would be inefficient.
Thanks for the explanation. As I understand, the most straightforward
way to avoid holding the permanent reference is indeed to delete
dev->liveupdate.incoming entirely and perform an xarray lookup on every
access, like this:
bool pci_liveupdate_is_incoming(struct pci_dev *dev)
{
...
incoming = pci_liveupdate_flb_get_incoming();
...
dev_ser = xa_load(&incoming->xa, key);
...
pci_liveupdate_flb_put_incoming();
return dev_ser && dev_ser->refcount > 0;
}
However, as you note, this is inefficient because it affects every
single device and adds lookup overhead to every access (not sure about
the actual cost though, xarray access is pretty fast!).
We can, however, still avoid tinkering with the lifecycle of the FLB,
and instead treat dev->liveupdate.incoming as a "hint" that we validate
on access with a fast, liveness check:
1. At Setup: In pci_liveupdate_setup_device(), we do the xarray lookup
once, cache the pointer in dev->liveupdate.incoming, and immediately
call pci_liveupdate_flb_put_incoming(). We do not hold a permanent
reference.
2. On Access: When an accessor runs, instead of doing a full xarray
lookup, it just validates the cached pointer's liveness by temporarily
securing the FLB:
static struct pci_flb_incoming *pci_liveupdate_get_incoming(struct pci_dev *dev)
{
struct pci_flb_incoming *incoming;
incoming = pci_liveupdate_flb_get_incoming();
if (!incoming)
return NULL;
if (dev->liveupdate.incoming)
return incoming;
pci_liveupdate_flb_put_incoming();
return NULL;
}
* If get_incoming() returns NULL (the FLB has already finished/freed),
the hint is invalid and the device is no longer incoming.
* If it returns a valid pointer, the FLB is guaranteed to be alive, so
we can immediately use the cached dev->liveupdate.incoming pointer
without xarray lookups. The accessor then puts the transient
reference when done.
3. In Finish/Cleanup: We can simply clear dev->liveupdate.incoming,
and decrement ser->nr_devices.
^ permalink raw reply
* Re: [PATCH v18 10/13] cxl: Add port and dport identifiers to CXL AER trace events
From: Dave Jiang @ 2026-07-20 22:44 UTC (permalink / raw)
To: Terry Bowman, Bjorn Helgaas, Dan Williams, Ira Weiny,
Jonathan Cameron, Len Brown, Rafael J . Wysocki, Robert Richter
Cc: linux-acpi, linux-cxl, linux-doc, linux-kernel, linux-pci,
linuxppc-dev, Alejandro Lucero, Alison Schofield, Ankit Agrawal,
Ard Biesheuvel, Ben Cheatham, Borislav Petkov, Breno Leitao,
Davidlohr Bueso, Fabio M . De Francesco, Gregory Price,
Hanjun Guo, Jonathan Corbet, Kees Cook,
Kuppuswamy Sathyanarayanan, Li Ming, Mahesh J Salgaonkar,
Mauro Carvalho Chehab, Oliver O'Halloran, Shiju Jose,
Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck, Vishal Verma
In-Reply-To: <20260717222706.3540281-11-terry.bowman@amd.com>
On 7/17/26 3:27 PM, Terry Bowman wrote:
> From: Dan Williams <djbw@kernel.org>
>
> Pass struct cxl_port * and struct cxl_dport * to the cxl_aer_*
> trace events instead of a plain struct device * derived at the
> caller. The trace event helpers then derive the right strings for
> endpoints, switch ports, root ports, and RCH downstream ports
> consistently across the CPER and native AER paths.
>
> The unified cxl_aer_* events keep "memdev" as the legacy field
> (endpoint events populate it with the memdev name; non-endpoint
> events emit memdev="") and add new "port" and "dport" string fields
> populated for all CXL device classes. Updated userspace can key
> off "port" and "dport" without a parallel set of events.
>
> Remove the separate cxl_port_aer_uncorrectable_error and
> cxl_port_aer_correctable_error trace events. All CXL AER events now
> use the unified cxl_aer_* events with port and dport fields.
>
> Rework cxl_cper_handle_prot_err() to use find_cxl_port_by_dev() and
> the unified trace helpers, replacing the per-port-type branching and
> bus_find_device() memdev lookup.
>
> The TP_printk format string places "port=%s dport=%s" between
> "memdev=%s" and "host=%s", changing the text-mode field order from
> the pre-patch output. This does not affect consumers such as
> rasdaemon that use libtraceevent to parse fields by name rather than
> by fixed text position.
>
> For non-endpoint events (switch port, root port, RCH dport),
> "memdev" is empty and "port"/"dport" carry the topology information.
>
> The serial number is retrieved via pci_get_dsn() which performs live
> PCI configuration space reads. A following patch ("PCI: Cache PCI
> DSN into pci_dev->dsn during probe") replaces these with a cached
> serial number to avoid config space access in error handlers and panic
> paths.
>
> Below are examples of the different CXL devices' error trace logs
> after this patch:
>
> ---------------------
> | CXL RP - 0C:00.0 |
> ---------------------
> |
> ---------------------
> | CXL USP - 0D:00.0 |
> ---------------------
> |
> --------------------
> | CXL DSP - 0E:00.0 |
> --------------------
> |
> ---------------------
> | CXL EP - 0F:00.0 |
> ---------------------
>
> Root Port:
> cxl_aer_correctable_error: memdev= port=port1 dport=0000:0c:00.0 \
> host=pci0000:0c serial=0: status: 'Memory Data ECC Error'
>
> cxl_aer_uncorrectable_error: memdev= port=port1 dport=0000:0c:00.0 \
> host=pci0000:0c serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> Upstream Switch Port:
> cxl_aer_correctable_error: memdev= port=port2 dport= host=0000:0d:00.0 \
> serial=0: status: 'Memory Data ECC Error'
>
> UCE NA - Upstream Switch Port UCE's are handled in the portdrv driver's
> PCI AER callbacks that are not CXL aware.
>
> Downstream Switch Port:
> cxl_aer_correctable_error: memdev= port=port2 dport=0000:0e:00.0 \
> host=0000:0d:00.0 serial=0: status: 'Memory Data ECC Error'
>
> cxl_aer_uncorrectable_error: memdev= port=port2 dport=0000:0e:00.0 \
> host=0000:0d:00.0 serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> Endpoint:
> cxl_aer_uncorrectable_error: memdev=mem1 port=endpoint4 dport= \
> host=0000:0f:00.0 serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> cxl_aer_correctable_error: memdev=mem1 port=endpoint4 dport= host=0000:0f:00.0 \
> serial=0: status: 'Memory Data ECC Error'
>
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <djbw@kernel.org>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> Changes in v17->v18:
> - Consolidate double find_cxl_port_by_dev() in cxl_cper_handle_prot_err()
> - Add comment noting dport is NULL for Endpoint and Upstream Port devices
> - Add cxl_trace_* helpers
> - Add CPER refactor
>
> Changes in v16->v17:
> - Replace cxlds->serial with pci_get_dsn()
> - Change 'memdev' to 'device' (Dan)
> - Updated Commit message
>
> Changes in v15->v16:
> - Add Dan's review-by
> - Incorporate Dan's comment into commit message:
> "Add the serial number at the end to preserve compatibility with
> libtraceevent parsing of the parameters."
>
> Changes in v14->v15:
> - Update commit message.
> - Moved cxl_handle_ras/cxl_handle_cor_ras() changes to future patch (terry)
>
> Changes in v13->v14:
> - Update commit headline (Bjorn)
>
> Changes in v12->v13:
> - Added Dave Jiang's review-by
>
> Changes in v11 -> v12:
> - Correct parameters to call trace_cxl_aer_correctable_error()
> - Add reviewed-by for Jonathan and Shiju
>
> Changes in v10->v11:
> - Updated CE and UCE trace routines to maintain consistent TP_Struct ABI
> and unchanged TP_printk() logging.
> ---
> drivers/cxl/core/core.h | 8 +--
> drivers/cxl/core/ras.c | 131 +++++++++++--------------------------
> drivers/cxl/core/ras_rch.c | 3 +-
> drivers/cxl/core/trace.c | 35 ++++++++++
> drivers/cxl/core/trace.h | 91 ++++++++------------------
> drivers/cxl/cxlmem.h | 7 ++
> 6 files changed, 113 insertions(+), 162 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 5ca1275fd8f35..a55a4e409feda 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -186,11 +186,11 @@ static inline struct device *dport_to_host(struct cxl_dport *dport)
> void cxl_ras_init(void);
> void cxl_ras_exit(void);
> bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport,
> - void __iomem *ras_base);
> + void __iomem *ras_base, u64 serial);
> void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport);
> void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport,
> - void __iomem *ras_base);
> + void __iomem *ras_base, u64 serial);
> void cxl_dport_map_rch_aer(struct cxl_dport *dport);
> void cxl_disable_rch_root_ints(struct cxl_dport *dport);
> void cxl_handle_rdport_errors(struct pci_dev *pdev);
> @@ -200,14 +200,14 @@ void devm_cxl_dport_ras_setup(struct cxl_dport *dport);
> static inline void cxl_ras_init(void) { }
> static inline void cxl_ras_exit(void) { }
> static inline bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport,
> - void __iomem *ras_base)
> + void __iomem *ras_base, u64 serial)
> {
> return false;
> }
> static inline void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport) { }
> static inline void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport,
> - void __iomem *ras_base) { }
> + void __iomem *ras_base, u64 serial) { }
> static inline void cxl_dport_map_rch_aer(struct cxl_dport *dport) { }
> static inline void cxl_disable_rch_root_ints(struct cxl_dport *dport) { }
> static inline void cxl_handle_rdport_errors(struct pci_dev *pdev) { }
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index d5dc2c22565da..acf40b2396c3b 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -12,69 +12,37 @@
> static_assert(CXL_HEADERLOG_TRACE_SIZE_U32 == 128,
> "rasdaemon ABI requires exactly 128 u32s");
>
> -static void cxl_cper_trace_corr_port_prot_err(struct pci_dev *pdev,
> - struct cxl_ras_capability_regs ras_cap)
> -{
> - u32 status = ras_cap.cor_status & ~ras_cap.cor_mask;
> -
> - trace_cxl_port_aer_correctable_error(&pdev->dev, status);
> -}
> -
> -static void cxl_cper_trace_uncorr_port_prot_err(struct pci_dev *pdev,
> - struct cxl_ras_capability_regs ras_cap)
> +static void cxl_cper_trace_uncorr_prot_err(struct cxl_port *port, struct cxl_dport *dport,
> + u64 serial, struct cxl_ras_capability_regs *ras_cap)
> {
> u32 hl[CXL_HEADERLOG_TRACE_SIZE_U32] = {};
> - u32 status = ras_cap.uncor_status & ~ras_cap.uncor_mask;
> + u32 status = ras_cap->uncor_status & ~ras_cap->uncor_mask;
> u32 fe;
>
> if (hweight32(status) > 1)
> fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
> - ras_cap.cap_control));
> - else
> - fe = status;
> -
> - memcpy(hl, ras_cap.header_log, CXL_HEADERLOG_SIZE);
> - trace_cxl_port_aer_uncorrectable_error(&pdev->dev, status, fe, hl);
> -}
> -
> -static void cxl_cper_trace_corr_prot_err(struct cxl_memdev *cxlmd,
> - struct cxl_ras_capability_regs ras_cap)
> -{
> - u32 status = ras_cap.cor_status & ~ras_cap.cor_mask;
> -
> - trace_cxl_aer_correctable_error(cxlmd, status);
> -}
> -
> -static void
> -cxl_cper_trace_uncorr_prot_err(struct cxl_memdev *cxlmd,
> - struct cxl_ras_capability_regs ras_cap)
> -{
> - u32 hl[CXL_HEADERLOG_TRACE_SIZE_U32] = {};
> - u32 status = ras_cap.uncor_status & ~ras_cap.uncor_mask;
> - u32 fe;
> -
> - if (hweight32(status) > 1)
> - fe = BIT(FIELD_GET(CXL_RAS_CAP_CONTROL_FE_MASK,
> - ras_cap.cap_control));
> + ras_cap->cap_control));
> else
> fe = status;
>
> /*
> - * ras_cap.header_log[] holds CXL_HEADERLOG_SIZE_U32 (16) hardware
> + * ras_cap->header_log[] holds CXL_HEADERLOG_SIZE_U32 (16) hardware
> * dwords. Copy them into the front of a zero-filled
> * CXL_HEADERLOG_TRACE_SIZE_U32 (128) u32 staging buffer so the trace
> * event memcpy sees a full 512-byte source and the userspace ABI
> * (rasdaemon) is preserved.
> */
> - memcpy(hl, ras_cap.header_log, CXL_HEADERLOG_SIZE);
> - trace_cxl_aer_uncorrectable_error(cxlmd, status, fe, hl);
> + memcpy(hl, ras_cap->header_log, CXL_HEADERLOG_SIZE);
> + trace_cxl_aer_uncorrectable_error(port, dport, status, fe,
> + hl, serial);
> }
>
> -static int match_memdev_by_parent(struct device *dev, const void *uport)
> +static void cxl_cper_trace_corr_prot_err(struct cxl_port *port, struct cxl_dport *dport,
> + u64 serial, struct cxl_ras_capability_regs *ras_cap)
> {
> - if (is_cxl_memdev(dev) && dev->parent == uport)
> - return 1;
> - return 0;
> + u32 status = ras_cap->cor_status & ~ras_cap->cor_mask;
> +
> + trace_cxl_aer_correctable_error(port, dport, status, serial);
> }
>
>
> @@ -109,47 +77,34 @@ static struct cxl_port *find_cxl_port_by_dev(struct device *dev, struct cxl_dpor
>
> void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
> {
> + struct cxl_dport *dport;
> unsigned int devfn = PCI_DEVFN(data->prot_err.agent_addr.device,
> data->prot_err.agent_addr.function);
> - struct pci_dev *pdev __free(pci_dev_put) =
> - pci_get_domain_bus_and_slot(data->prot_err.agent_addr.segment,
> - data->prot_err.agent_addr.bus,
> - devfn);
> - struct cxl_memdev *cxlmd;
> - int port_type;
> -
> - if (!pdev)
> - return;
> -
> - port_type = pci_pcie_type(pdev);
> - if (port_type == PCI_EXP_TYPE_ROOT_PORT ||
> - port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> - port_type == PCI_EXP_TYPE_UPSTREAM) {
> - if (data->severity == AER_CORRECTABLE)
> - cxl_cper_trace_corr_port_prot_err(pdev, data->ras_cap);
> - else
> - cxl_cper_trace_uncorr_port_prot_err(pdev, data->ras_cap);
> -
> + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> + data->prot_err.agent_addr.segment, data->prot_err.agent_addr.bus, devfn);
> + if (!pdev) {
> + pr_err_ratelimited("Failed to find CPER device in CXL topology\n");
> return;
> }
>
> - guard(device)(&pdev->dev);
> - if (!pdev->dev.driver) {
> - dev_warn_ratelimited(&pdev->dev,
> - "Device is unbound, abort CPER error handling\n");
> + struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_dev(&pdev->dev, NULL);
> + if (!port) {
> + dev_err_ratelimited(&pdev->dev,
> + "Failed to find parent port device in CXL topology\n");
> return;
> }
>
> - struct device *mem_dev __free(put_device) = bus_find_device(
> - &cxl_bus_type, NULL, pdev, match_memdev_by_parent);
> - if (!mem_dev)
> - return;
> + guard(device)(&port->dev);
> +
> + /* dport is NULL for Endpoint and Upstream Port devices */
> + dport = cxl_find_dport_by_dev(port, &pdev->dev);
>
> - cxlmd = to_cxl_memdev(mem_dev);
> if (data->severity == AER_CORRECTABLE)
> - cxl_cper_trace_corr_prot_err(cxlmd, data->ras_cap);
> + cxl_cper_trace_corr_prot_err(port, dport, pci_get_dsn(pdev),
> + &data->ras_cap);
> else
> - cxl_cper_trace_uncorr_prot_err(cxlmd, data->ras_cap);
> + cxl_cper_trace_uncorr_prot_err(port, dport, pci_get_dsn(pdev),
> + &data->ras_cap);
> }
> EXPORT_SYMBOL_GPL(cxl_cper_handle_prot_err);
>
> @@ -240,14 +195,14 @@ void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port, struct cxl_dpo
> return;
> }
>
> - if (cxl_handle_ras(port, dport, ras_base))
> + if (cxl_handle_ras(port, dport, ras_base, pci_get_dsn(pdev)))
> panic("CXL cachemem error");
>
> dev_dbg(&pdev->dev,
> "CXL UCE signaled but no CXL RAS status bits set\n");
> }
>
> -void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base)
> +void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base, u64 serial)
> {
> u32 status;
> void __iomem *addr;
> @@ -259,12 +214,7 @@ void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __i
> status = readl(addr);
> if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> - if (is_cxl_endpoint(port))
> - trace_cxl_aer_correctable_error(to_cxl_memdev(port->uport_dev), status);
> - else if (dport)
> - trace_cxl_port_aer_correctable_error(dport->dport_dev, status);
> - else
> - trace_cxl_port_aer_correctable_error(port->uport_dev, status);
> + trace_cxl_aer_correctable_error(port, dport, status, serial);
> }
> }
>
> @@ -289,7 +239,8 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
> * Log the state of the RAS status registers and prepare them to log the
> * next error status. Return 1 if reset needed.
> */
> -bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base)
> +bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport,
> + void __iomem *ras_base, u64 serial)
> {
> u32 hl[CXL_HEADERLOG_TRACE_SIZE_U32] = {};
> void __iomem *addr;
> @@ -316,12 +267,7 @@ bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem
> }
>
> header_log_copy(ras_base, hl);
> - if (is_cxl_endpoint(port))
> - trace_cxl_aer_uncorrectable_error(to_cxl_memdev(port->uport_dev), status, fe, hl);
> - else if (dport)
> - trace_cxl_port_aer_uncorrectable_error(dport->dport_dev, status, fe, hl);
> - else
> - trace_cxl_port_aer_uncorrectable_error(port->uport_dev, status, fe, hl);
> + trace_cxl_aer_uncorrectable_error(port, dport, status, fe, hl, serial);
>
> writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>
> @@ -360,7 +306,8 @@ pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> * cases below handle AER recovery for devices without active
> * CXL.mem traffic.
> */
> - ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
> + ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL),
> + pci_get_dsn(pdev));
> }
>
> /*
> @@ -392,7 +339,7 @@ static void cxl_handle_proto_error(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport, int severity)
> {
> if (severity == AER_CORRECTABLE)
> - cxl_handle_cor_ras(port, dport, to_ras_base(port, dport));
> + cxl_handle_cor_ras(port, dport, to_ras_base(port, dport), pci_get_dsn(pdev));
> else
> cxl_do_recovery(pdev, port, dport);
> }
> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index f4b98f2c11a1c..0385d2f4a2f66 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
> @@ -118,7 +118,8 @@ void cxl_handle_rdport_errors(struct pci_dev *pdev)
>
> pci_print_aer(pdev, severity, &aer_regs);
> if (severity == AER_CORRECTABLE)
> - cxl_handle_cor_ras(dport->port, dport, to_ras_base(port, dport));
> + cxl_handle_cor_ras(dport->port, dport, to_ras_base(port, dport),
> + pci_get_dsn(pdev));
> else
> cxl_do_recovery(pdev, dport->port, dport);
> }
> diff --git a/drivers/cxl/core/trace.c b/drivers/cxl/core/trace.c
> index 7f2a9dd0d0e3f..df42d119c53dd 100644
> --- a/drivers/cxl/core/trace.c
> +++ b/drivers/cxl/core/trace.c
> @@ -2,7 +2,42 @@
> /* Copyright(c) 2022 Intel Corporation. All rights reserved. */
>
> #include <cxl.h>
> +#include <cxlmem.h>
> #include "core.h"
>
> +const char *cxl_trace_memdev_name(struct cxl_port *port)
> +{
> + if (is_cxl_endpoint(port)) {
> + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> +
> + return dev_name(&cxlmd->dev);
> + }
> +
> + return "";
> +}
> +
> +const char *cxl_trace_host_name(struct cxl_port *port)
> +{
> + if (is_cxl_endpoint(port)) {
> + struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
> +
> + return dev_name(cxlmd->dev.parent);
> + }
> +
> + return dev_name(port->uport_dev);
> +}
> +
> +const char *cxl_trace_port_name(struct cxl_port *port)
> +{
> + return dev_name(&port->dev);
> +}
> +
> +const char *cxl_trace_dport_name(struct cxl_dport *dport)
> +{
> + if (dport)
> + return dev_name(dport->dport_dev);
> + return "";
> +}
> +
> #define CREATE_TRACE_POINTS
> #include "trace.h"
> diff --git a/drivers/cxl/core/trace.h b/drivers/cxl/core/trace.h
> index d37876096dd7c..910aceb2ca3ab 100644
> --- a/drivers/cxl/core/trace.h
> +++ b/drivers/cxl/core/trace.h
> @@ -48,44 +48,15 @@
> { CXL_RAS_UC_IDE_RX_ERR, "IDE Rx Error" } \
> )
>
> -TRACE_EVENT(cxl_port_aer_uncorrectable_error,
> - TP_PROTO(struct device *dev, u32 status, u32 fe, u32 *hl),
> - TP_ARGS(dev, status, fe, hl),
> - TP_STRUCT__entry(
> - __string(device, dev_name(dev))
> - __string(host, dev_name(dev->parent))
> - __field(u32, status)
> - __field(u32, first_error)
> - __array(u32, header_log, CXL_HEADERLOG_TRACE_SIZE_U32)
> - ),
> - TP_fast_assign(
> - __assign_str(device);
> - __assign_str(host);
> - __entry->status = status;
> - __entry->first_error = fe;
> - /*
> - * Embed headerlog data for user app retrieval and parsing,
> - * but no need to print in the trace buffer. Only
> - * CXL_HEADERLOG_SIZE_U32 (16) dwords are hardware data;
> - * the remaining entries preserve the 512-byte ABI layout
> - * rasdaemon depends on and are zero-filled by the caller.
> - */
> - memcpy(__entry->header_log, hl,
> - CXL_HEADERLOG_TRACE_SIZE_U32 * sizeof(u32));
> - ),
> - TP_printk("device=%s host=%s status: '%s' first_error: '%s'",
> - __get_str(device), __get_str(host),
> - show_uc_errs(__entry->status),
> - show_uc_errs(__entry->first_error)
> - )
> -);
> -
> TRACE_EVENT(cxl_aer_uncorrectable_error,
> - TP_PROTO(const struct cxl_memdev *cxlmd, u32 status, u32 fe, u32 *hl),
> - TP_ARGS(cxlmd, status, fe, hl),
> + TP_PROTO(struct cxl_port *port, struct cxl_dport *dport,
> + u32 status, u32 fe, u32 *hl, u64 serial),
> + TP_ARGS(port, dport, status, fe, hl, serial),
> TP_STRUCT__entry(
> - __string(memdev, dev_name(&cxlmd->dev))
> - __string(host, dev_name(cxlmd->dev.parent))
> + __string(memdev, cxl_trace_memdev_name(port))
> + __string(port, cxl_trace_port_name(port))
> + __string(dport, cxl_trace_dport_name(dport))
> + __string(host, cxl_trace_host_name(port))
> __field(u64, serial)
> __field(u32, status)
> __field(u32, first_error)
> @@ -93,8 +64,10 @@ TRACE_EVENT(cxl_aer_uncorrectable_error,
> ),
> TP_fast_assign(
> __assign_str(memdev);
> + __assign_str(port);
> + __assign_str(dport);
> __assign_str(host);
> - __entry->serial = cxlmd->cxlds->serial;
> + __entry->serial = serial;
> __entry->status = status;
> __entry->first_error = fe;
> /*
> @@ -107,8 +80,9 @@ TRACE_EVENT(cxl_aer_uncorrectable_error,
> memcpy(__entry->header_log, hl,
> CXL_HEADERLOG_TRACE_SIZE_U32 * sizeof(u32));
> ),
> - TP_printk("memdev=%s host=%s serial=%lld: status: '%s' first_error: '%s'",
> - __get_str(memdev), __get_str(host), __entry->serial,
> + TP_printk("memdev=%s port=%s dport=%s host=%s serial=%lld: status: '%s' first_error: '%s'",
> + __get_str(memdev), __get_str(port), __get_str(dport),
> + __get_str(host), __entry->serial,
> show_uc_errs(__entry->status),
> show_uc_errs(__entry->first_error)
> )
> @@ -132,42 +106,29 @@ TRACE_EVENT(cxl_aer_uncorrectable_error,
> { CXL_RAS_CE_PHYS_LAYER_ERR, "Received Error From Physical Layer" } \
> )
>
> -TRACE_EVENT(cxl_port_aer_correctable_error,
> - TP_PROTO(struct device *dev, u32 status),
> - TP_ARGS(dev, status),
> - TP_STRUCT__entry(
> - __string(device, dev_name(dev))
> - __string(host, dev_name(dev->parent))
> - __field(u32, status)
> - ),
> - TP_fast_assign(
> - __assign_str(device);
> - __assign_str(host);
> - __entry->status = status;
> - ),
> - TP_printk("device=%s host=%s status='%s'",
> - __get_str(device), __get_str(host),
> - show_ce_errs(__entry->status)
> - )
> -);
> -
> TRACE_EVENT(cxl_aer_correctable_error,
> - TP_PROTO(const struct cxl_memdev *cxlmd, u32 status),
> - TP_ARGS(cxlmd, status),
> + TP_PROTO(struct cxl_port *port, struct cxl_dport *dport,
> + u32 status, u64 serial),
> + TP_ARGS(port, dport, status, serial),
> TP_STRUCT__entry(
> - __string(memdev, dev_name(&cxlmd->dev))
> - __string(host, dev_name(cxlmd->dev.parent))
> + __string(memdev, cxl_trace_memdev_name(port))
> + __string(port, cxl_trace_port_name(port))
> + __string(dport, cxl_trace_dport_name(dport))
> + __string(host, cxl_trace_host_name(port))
> __field(u64, serial)
> __field(u32, status)
> ),
> TP_fast_assign(
> __assign_str(memdev);
> + __assign_str(port);
> + __assign_str(dport);
> __assign_str(host);
> - __entry->serial = cxlmd->cxlds->serial;
> + __entry->serial = serial;
> __entry->status = status;
> ),
> - TP_printk("memdev=%s host=%s serial=%lld: status: '%s'",
> - __get_str(memdev), __get_str(host), __entry->serial,
> + TP_printk("memdev=%s port=%s dport=%s host=%s serial=%lld: status: '%s'",
> + __get_str(memdev), __get_str(port), __get_str(dport),
> + __get_str(host), __entry->serial,
> show_ce_errs(__entry->status)
> )
> );
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index ed419d0c59f2f..f1ef8b78db18a 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -125,6 +125,13 @@ static inline int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> #endif
>
> struct cxl_memdev *devm_cxl_add_classdev(struct cxl_dev_state *cxlds);
> +
> +/* trace-event helpers */
> +const char *cxl_trace_memdev_name(struct cxl_port *port);
> +const char *cxl_trace_host_name(struct cxl_port *port);
> +const char *cxl_trace_port_name(struct cxl_port *port);
> +const char *cxl_trace_dport_name(struct cxl_dport *dport);
> +
> struct cxl_memdev *__devm_cxl_add_memdev(struct cxl_dev_state *cxlds,
> const struct cxl_memdev_attach *attach);
> int devm_cxl_sanitize_setup_notifier(struct device *host,
^ permalink raw reply
* Re: [PATCH v18 12/13] PCI/CXL: Mask/Unmask CXL protocol errors
From: Dave Jiang @ 2026-07-20 22:52 UTC (permalink / raw)
To: Terry Bowman, Bjorn Helgaas, Dan Williams, Ira Weiny,
Jonathan Cameron, Len Brown, Rafael J . Wysocki, Robert Richter
Cc: linux-acpi, linux-cxl, linux-doc, linux-kernel, linux-pci,
linuxppc-dev, Alejandro Lucero, Alison Schofield, Ankit Agrawal,
Ard Biesheuvel, Ben Cheatham, Borislav Petkov, Breno Leitao,
Davidlohr Bueso, Fabio M . De Francesco, Gregory Price,
Hanjun Guo, Jonathan Corbet, Kees Cook,
Kuppuswamy Sathyanarayanan, Li Ming, Mahesh J Salgaonkar,
Mauro Carvalho Chehab, Oliver O'Halloran, Shiju Jose,
Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck, Vishal Verma
In-Reply-To: <20260717222706.3540281-13-terry.bowman@amd.com>
On 7/17/26 3:27 PM, Terry Bowman wrote:
> CXL protocol errors are not enabled for all CXL devices after boot.
> They must be enabled in order to process CXL protocol errors. Provide
> matching teardown helpers so the masks are restored when a CXL Port
> or dport goes away.
>
> Add pci_aer_mask_internal_errors() as the symmetric counterpart to
> pci_aer_unmask_internal_errors() and export both for the cxl_core
> module.
>
> Introduce cxl_unmask_proto_interrupts() and cxl_mask_proto_interrupts()
> in cxl_core to wrap the PCI helpers with the dev_is_pci() and
> pcie_aer_is_native() gating CXL needs. Both helpers tolerate a NULL
> or non-PCI @dev so callers do not have to special-case it.
>
> Wire cxl_unmask_proto_interrupts() into the success path of
> cxl_dport_map_ras() and devm_cxl_port_ras_setup() so the unmask
> only runs when the RAS register block was actually mapped. Pair each
> unmask with a devm_add_action_or_reset() registration of
> cxl_mask_proto_irqs() scoped to the host device so the mask is
> restored when devres is released. This applies to dports, Endpoints,
> Upstream Switch Ports, Downstream Switch Ports, and Root Ports.
>
> 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
> caused the function to return early on real hardware without mapping
> dport RAS or AER registers. The caller already gates on dport->rch,
> which is sufficient to exclude cxl_test mock devices.
>
> 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>
>
> ---
>
> Changes in v17->v18:
> - Make cxl_unmask_proto_interrupts() and cxl_mask_proto_interrupts() static
> - Remove dev_is_pci() guard from devm_cxl_dport_rch_ras_setup(); the guard
> blocked real RCH hardware because pci_host_bridge is not on pci_bus_type
>
> Changes in v16->v17:
> - Drop redundant cxl_mask_proto_interrupts() calls from unregister_port()
> and cxl_dport_remove(); the devres action registered alongside the unmask
> is the sole mask path.
> - Update title
> - Remove unnecessary check for aer_capabilities
> - Gate cxl_unmask_proto_interrupts() on pcie_aer_is_native()
> - Add pci_aer_mask_internal_errors() and cxl_mask_proto_interrupts()
> - Only unmask on successful cxl_map_component_regs()
> - NULL-check @dev in cxl_{un,}mask_proto_interrupts()
> - Drop static and declare in core/core.h
>
> Change in v15 -> v16:
> - None
>
> Change in v14 -> v15:
> - None
>
> Changes in v13->v14:
> - Update commit title's prefix (Bjorn)
>
> Changes in v12->v13:
> - Add dev and dev_is_pci() NULL checks in cxl_unmask_proto_interrupts() (Terry)
> - Add Dave Jiang's and Ben's review-by
>
> Changes in v11->v12:
> - None
> ---
> drivers/cxl/core/ras.c | 73 +++++++++++++++++++++++++++++++----
> drivers/pci/pcie/aer.c | 28 ++++++++++++--
> include/linux/aer.h | 2 +
> tools/testing/cxl/Kbuild | 1 +
> tools/testing/cxl/test/mock.c | 12 ++++++
> 5 files changed, 105 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 69b320c74469c..d77208af41e03 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -117,16 +117,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;
> + }
> }
>
> /**
> @@ -143,9 +191,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);
> @@ -160,6 +205,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");
> @@ -168,8 +214,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 0bd23a65e7ebc..be6dc2cbd4491 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1143,12 +1143,32 @@ 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)
> +{
> + int aer = dev->aer_cap;
> + u32 mask;
> +
> + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
> + mask |= PCI_ERR_UNC_INTN;
> + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
> +
> + pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
> + mask |= PCI_ERR_COR_INTERNAL;
> + pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
> +}
> +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;
^ permalink raw reply
* Re: [PATCH v18 06/13] PCI: Establish common CXL Port protocol error flow
From: Jonathan Cameron @ 2026-07-20 23:05 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck, Vishal Verma
In-Reply-To: <20260717222706.3540281-7-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:26:59 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> Add CXL protocol error dispatch in handle_error_source() using
> is_cxl_error() and cxl_forward_error() to route errors through the
> AER-CXL kfifo. Expand is_cxl_error() from Endpoint-only to include
> Root Port, Upstream Port, and Downstream Port device types. The
> producer and consumer go live in the same commit to avoid silently
> dropping CXL errors during bisect.
Take a stab at making this description a lot more concise. I'd like
to just be seeing a very brief summary of flow + clearly highlighting
of potentially controversial choices.
>
> For uncorrectable events, call cxl_proto_err_flush() to ensure CXL RAS
> registers are read, panic policy is applied, and CXL state is cleared
> before pci_aer_handle_error() drives PCIe recovery. Without the flush,
> AER recovery can tear down drivers and unmap the CXL RAS iomaps while
> the kfifo consumer is still reading them. Correctable events do not
> need the flush and run asynchronously. RCH kfifo support is added in
> the following patch ("PCI/CXL: Add RCH support to CXL handlers").
>
> Add cxl_handle_proto_error() to dispatch correctable and uncorrectable
> errors through the CXL RAS helpers. Add cxl_do_recovery() to coordinate
> uncorrectable recovery. Panic when a UCE is confirmed by a successful
> CXL RAS status register read. If the RAS registers cannot be read the
> UCE cannot be confirmed and panic is not triggered. Gate error handling
> on the port driver being bound to avoid processing errors on disabled
> devices.
>
> The kfifo consumer holds guard(device)(&port->dev) and checks
> port->dev.driver before accessing RAS registers, serializing against
> driver unbind and devm iomap teardown. For UCE, cxl_proto_err_flush()
> runs the worker synchronously before AER recovery, ensuring the device
> is present during RAS register access.
>
> Add to_ras_base() to centralize RAS base lookup: dport->regs.ras for
> Root/Downstream Ports, port->regs.ras for Upstream Ports and Endpoints.
> Use to_ras_base() to access the CXL devices' RAS registers as it will
> provide an avenue to inject status simulation during testing.
>
> Add CXL RAS logging in cxl_handle_cor_ras() and cxl_handle_ras(). The
> existing cxl_cor_error_detected() and cxl_error_detected() AER
> callbacks remain for all Endpoints and are reworked to use
> find_cxl_port_by_uport() and to_ras_base(), with UCE now triggering
> panic unconditionally. These callbacks are further updated in the
> following patch ("PCI/CXL: Add RCH support to CXL handlers").
>
> Fix a pre-existing race for cxlds between cxl_handle_rdport_errors() and
> cxl_memdev_shutdown() by holding a cxlmd device scoped_guard() around
> the rdport call. Release the lock before taking the Port lock to avoid
> the lock inversion.
>
> 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>
>
A few minor things about the code inline.
Thanks,
Jonathan
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 135f1997e6f4f..b190e69c2d415 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -77,6 +77,36 @@ static int match_memdev_by_parent(struct device *dev, const void *uport)
> return 0;
> }
>
> +
> +/**
> + * find_cxl_port_by_dev - Use @dev as hint to do a _by_dport or _by_uport lookup
> + * @dev: generic device that may either be a companion of port or target dport
> + * @dport: output parameter; set to the matched dport for dport-class
I'd state this is optional.
> + * lookups (Root Port, Downstream Port), NULL otherwise.
> + *
> + * Return a 'struct cxl_port' with an elevated reference if found. Use
> + * __free(put_cxl_port) to release.
> + */
> +static struct cxl_port *find_cxl_port_by_dev(struct device *dev, struct cxl_dport **dport)
> +{
> + if (dport)
> + *dport = NULL;
> + if (!dev_is_pci(dev))
> + return NULL;
> +
> + switch (pci_pcie_type(to_pci_dev(dev))) {
> + case PCI_EXP_TYPE_ROOT_PORT:
> + case PCI_EXP_TYPE_DOWNSTREAM:
> + return find_cxl_port_by_dport(dev, dport);
> + case PCI_EXP_TYPE_UPSTREAM:
> + case PCI_EXP_TYPE_ENDPOINT:
> + case PCI_EXP_TYPE_RC_END:
> + return find_cxl_port_by_uport(dev);
> + }
> +
> + return NULL;
> +}
...
> @@ -270,22 +326,32 @@ bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
>
> void cxl_cor_error_detected(struct pci_dev *pdev)
> {
> - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> - struct cxl_memdev *cxlmd = cxlds->cxlmd;
> - struct device *dev = &cxlds->cxlmd->dev;
> + guard(device)(&pdev->dev);
> + if (!pdev->dev.driver)
> + return;
> +
> + struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_uport(&pdev->dev);
> + if (!port)
> + return;
> +
> + if (is_cxl_restricted(pdev)) {
> + struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> + struct cxl_memdev *cxlmd = cxlds->cxlmd;
>
> - scoped_guard(device, dev) {
> - if (!dev->driver) {
> + scoped_guard(device, &cxlmd->dev) {
> + cxl_handle_rdport_errors(cxlds);
> + }
Unless this gets more complex later you can just use a guard() as exits
scope here anyway.
> + }
> +
> + scoped_guard(device, &port->dev) {
Similar here. I haven't read on though for this one so if it's needed
later just ignore me.
> + if (!port->dev.driver) {
> dev_warn(&pdev->dev,
> - "%s: memdev disabled, abort error handling\n",
> - dev_name(dev));
> + "%s: port disabled, abort error handling\n",
> + dev_name(&port->dev));
> return;
> }
>
> - if (cxlds->rcd)
> - cxl_handle_rdport_errors(cxlds);
> -
> - cxl_handle_cor_ras(&cxlds->cxlmd->dev, cxlmd->endpoint->regs.ras);
> + cxl_handle_cor_ras(port->uport_dev, to_ras_base(port, NULL));
> }
> }
> EXPORT_SYMBOL_NS_GPL(cxl_cor_error_detected, "CXL");
> @@ -293,42 +359,53 @@ 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)
> {
> - struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
...
>
> + /*
> + * CXL.mem UCE means cache coherency is lost. Continuing risks
> + * silent data corruption across interleaved HDM regions.
It is a problem whether interleave or not. So maybe just stop
at corruption.
> + */
> + if (ue)
> + panic("CXL cachemem error");
...
> @@ -338,3 +415,67 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
...
> +static void __cxl_proto_err_work_fn(struct cxl_proto_err_work_data *wd)
> +{
> + struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_dev(&wd->pdev->dev, NULL);
> + if (!port) {
> + dev_err_ratelimited(&wd->pdev->dev,
> + "Failed to find parent port device in CXL topology\n");
> + return;
> + }
I'd put a blank line here.
> + guard(device)(&port->dev);
> + if (!port->dev.driver) {
> + dev_err_ratelimited(&port->dev,
> + "Port device is unbound, abort error handling\n");
> + return;
> + }
> +
> + struct cxl_dport *dport = cxl_find_dport_by_dev(port, &wd->pdev->dev);
> + if (!dport && (pci_pcie_type(wd->pdev) == PCI_EXP_TYPE_ROOT_PORT ||
> + pci_pcie_type(wd->pdev) == PCI_EXP_TYPE_DOWNSTREAM)) {
> + dev_err_ratelimited(&wd->pdev->dev,
> + "Failed to find dport device in CXL topology\n");
> + return;
> + }
> +
> + cxl_handle_proto_error(wd->pdev, port, dport, wd->severity);
> +}
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index c5bce25df51cb..2d9d40528e709 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1185,7 +1185,20 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>
> static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
> {
> + bool cxl_pending = false;
> +
> cxl_rch_handle_error(dev, info);
> +
> + if (is_cxl_error(dev, info))
> + cxl_pending |= cxl_forward_error(dev, info);
> +
> + /*
> + * Wait for UCE CXL work to complete before AER recovery
> + * tears down the device. CE can run asynchronously.
> + */
> + if (cxl_pending && info->severity != AER_CORRECTABLE)
> + cxl_proto_err_flush();
This kind of makes me wonder if the naming of _flush() is clear.
Normally I'd kind of expect that to be dumping any queued up errors.
Perhaps cxl_proto_err_wait_for_empty() or something like that?
I guess flush_work() set the precedent so I'm too later on this one!
Jonathan
^ permalink raw reply
* Re: [PATCH v7 03/12] PCI: liveupdate: Track incoming preserved PCI devices
From: David Matlack @ 2026-07-20 23:07 UTC (permalink / raw)
To: Pasha Tatashin
Cc: kexec, linux-doc, linux-kernel, linux-mm, linux-pci,
Adithya Jayachandran, Alexander Graf, Alex Williamson,
Bjorn Helgaas, Chris Li, David Rientjes, Jacob Pan,
Jason Gunthorpe, Jonathan Corbet, Josh Hilke, Leon Romanovsky,
Lukas Wunner, Mike Rapoport, Parav Pandit, Pranjal Shrivastava,
Pratyush Yadav, Saeed Mahameed, Samiullah Khawaja, Shuah Khan,
Vipin Sharma, William Tu, Yi Liu
In-Reply-To: <178458744511.332171.13770778781241262180.b4-reply@b4>
On Mon, Jul 20, 2026 at 3:44 PM Pasha Tatashin
<pasha.tatashin@soleen.com> wrote:
>
> On 2026-07-20 14:54:51-07:00, David Matlack wrote:
> > On Fri, Jul 17, 2026 at 2:47 PM Pasha Tatashin
> > <pasha.tatashin@soleen.com> wrote:
> >
> > > On Fri, 10 Jul 2026 21:26:06 +0000, David Matlack <dmatlack@google.com> wrote:
> > >
> > > I am confused by this. Each preserved PCI device must have an associated
> > > FD preserved with it via LUO. I.e., vfiofd would need to be preserved. If
> > > the vfiofd was not reclaimed, and finish is not possible, that vfiofd
> > > would still be owned by LUO, and therefore PCI FLB refcount would stay
> > > positive.
> > >
> > > However, if finish is possible, and this is the last vfiofd that is
> > > finished, FLB will be freed as soon as the reference count reaches zero,
> > > which I would think is the expected behavior.
> > >
> > > What is the point of holding a reference here, instead of only for the
> > > duration of FLB access, i.e. to make sure we are accessing a valid data?
> >
> > The duration of the access is from here until pci_liveupdate_finish()
> > because that it when the pointer (dev->liveupdate.incoming) is
> > cleared. So that is why the PCI core holds the reference from here
> > until pci_liveupdate_finish().
> >
> > We could avoid this by deleteing dev->liveupdate.incoming and,
> > instead, fetching the incoming FLB and doing the xarray lookup every
> > atime the PCI core needs to access the device's incoming ser struct,
> > but that would be inefficient.
>
> Thanks for the explanation. As I understand, the most straightforward
> way to avoid holding the permanent reference is indeed to delete
> dev->liveupdate.incoming entirely and perform an xarray lookup on every
> access, like this:
>
> bool pci_liveupdate_is_incoming(struct pci_dev *dev)
> {
> ...
> incoming = pci_liveupdate_flb_get_incoming();
> ...
> dev_ser = xa_load(&incoming->xa, key);
> ...
> pci_liveupdate_flb_put_incoming();
> return dev_ser && dev_ser->refcount > 0;
> }
>
> However, as you note, this is inefficient because it affects every
> single device and adds lookup overhead to every access (not sure about
> the actual cost though, xarray access is pretty fast!).
>
> We can, however, still avoid tinkering with the lifecycle of the FLB,
> and instead treat dev->liveupdate.incoming as a "hint" that we validate
> on access with a fast, liveness check:
Can you tell me more about your concern about FLB lifetime?
The lifetime of the FLB will not be affected by this reference unless
there is a bug in the driver where it fails to call
pci_liveupdate_finish() during it's file handler finish callback.
>
> 1. At Setup: In pci_liveupdate_setup_device(), we do the xarray lookup
> once, cache the pointer in dev->liveupdate.incoming, and immediately
> call pci_liveupdate_flb_put_incoming(). We do not hold a permanent
> reference.
>
> 2. On Access: When an accessor runs, instead of doing a full xarray
> lookup, it just validates the cached pointer's liveness by temporarily
> securing the FLB:
>
> static struct pci_flb_incoming *pci_liveupdate_get_incoming(struct pci_dev *dev)
> {
> struct pci_flb_incoming *incoming;
>
> incoming = pci_liveupdate_flb_get_incoming();
> if (!incoming)
> return NULL;
>
> if (dev->liveupdate.incoming)
> return incoming;
>
> pci_liveupdate_flb_put_incoming();
> return NULL;
> }
>
> * If get_incoming() returns NULL (the FLB has already finished/freed),
> the hint is invalid and the device is no longer incoming.
This avoids the xarray lookup but still requires taking the incoming
FLB mutex twice (once for get and once for put) on every access. And
if there's no incoming PCI FLB, the LUO will iterate over all incoming
FLBs under the mutex to find it.
>
> * If it returns a valid pointer, the FLB is guaranteed to be alive, so
> we can immediately use the cached dev->liveupdate.incoming pointer
> without xarray lookups. The accessor then puts the transient
> reference when done.
>
> 3. In Finish/Cleanup: We can simply clear dev->liveupdate.incoming,
> and decrement ser->nr_devices.
>
^ permalink raw reply
* Re: [PATCH v18 07/13] PCI/CXL: Add RCH support to CXL handlers
From: Jonathan Cameron @ 2026-07-20 23:12 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-8-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:00 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> Restricted CXL Host (RCH) error handling is a separate path from the
> new CXL Port error handling flow. Fold RCH error handling into the
> Port flow so both share a common entry point.
>
> Update cxl_rch_handle_error_iter() to forward RCH protocol errors
> through the AER-CXL kfifo. Change cxl_rch_handle_error() return type
> from void to bool so handle_error_source() can determine whether work
> was enqueued and call cxl_proto_err_flush() before AER recovery
> proceeds.
>
> For RC_END devices, __cxl_proto_err_work_fn() calls
Can we refer to these as RCiEPs to match the spec? I have no idea
where RC_END naming came from but to me it isn't obviously that same
thing. Or talk about the narrow case of RCDs here as well.
> cxl_handle_rdport_errors() to process RCH Downstream Port errors,
> then falls through to the VH path for RC_END Endpoint handling.
>
> An RCD uncorrectable CXL RAS error now panics via cxl_do_recovery().
> Before this patch the RCH Downstream Port UCE path called
> cxl_handle_ras() but ignored its return value - no panic. After this
> patch the same condition calls cxl_do_recovery() which panics on
> confirmed UCE. The Endpoint UCE path already panicked at the parent
> commit. This matches the panic policy added in the common CXL Port
> protocol error flow.
>
> Remove cxl_cor_error_detected() and its .cor_error_detected
> registration in cxl_error_handlers. Correctable Endpoint errors are
> now routed through the AER-CXL kfifo like all other CXL protocol
> errors.
>
> Drop the cxlds->rcd / cxl_handle_rdport_errors(cxlds) branches from
> cxl_error_detected(). RCH downstream port error handling is now
> performed by __cxl_proto_err_work_fn() via the kfifo path, which
> calls cxl_handle_rdport_errors(pdev) before the common dispatch.
>
> Change cxl_handle_rdport_errors() to take a struct pci_dev * instead
> of a struct cxl_dev_state *, matching the new caller context. Re-fetch
> dport under guard() to close the TOCTOU window between
> cxl_pci_find_port()'s lockless xa_load() and the first dereference of
> the returned pointer.
>
> Change find_cxl_port_by_dev() RC_END lookup from
> find_cxl_port_by_dport(dev->parent) to find_cxl_port_by_uport(dev),
> matching the Endpoint lookup path. RC_END Endpoint port resolution
> uses the uport (the RC_END device itself), while the separate RCH
> Downstream Port lookup is handled by cxl_handle_rdport_errors().
>
> The RCH Downstream Port and the RCD Endpoint (RC_END) are separate
> devices with independent RAS register blocks. cxl_handle_rdport_errors()
> handles the RCH Downstream Port RAS. RCD Endpoint (RC_END) is handled in
> cxl_handle_proto_error().
>
> Use to_ras_base() in cxl_handle_rdport_errors() instead of referencing
> dport->regs.ras directly. Make to_ras_base() non-static in ras.c and
> declare it in core.h so ras_rch.c can access it. Route all RAS base address lookups
> through a single helper to prepare for CXL RAS error injection testing
> that follows this series.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>
Other than taking another look at the patch description and seeing
if it can be more concise, this one looks fine to me.
Jonathan
^ permalink raw reply
* Re: [PATCH v18 08/13] cxl/pci: Thread port and dport through RAS handling helpers
From: Jonathan Cameron @ 2026-07-20 23:17 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-9-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:01 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> From: Dan Williams <djbw@kernel.org>
>
> The callers of cxl_handle_ras() and cxl_handle_cor_ras() already hold
> a struct cxl_port * and struct cxl_dport * for the device being
> handled.
It seems unlikely that all have both of them given you have handling
in the code for dport == NULL. Perhaps reword.
> Passing a generic struct device * requires is_cxl_memdev()
> to distinguish Endpoints from ports at trace emission time. Threading
> port and dport directly enables is_cxl_endpoint(port) and explicit
> dport/port branching for cleaner trace dispatch.
>
> Refactor cxl_handle_ras() and cxl_handle_cor_ras() to accept struct
> cxl_port * and struct cxl_dport * directly. The CXL RAS trace event
> emission logic is split into three branches: Endpoint events are
> identified via is_cxl_endpoint(port) and emit with the memdev, dport
> events emit with dport->dport_dev, and Upstream Port events fall back
> to port->uport_dev.
>
> Update cxl_handle_rdport_errors() in ras_rch.c and
> cxl_handle_proto_error() in ras.c to pass port and dport to the
> refactored functions.
Definitely no need to say which files they are in. If we care we can
look at the patch.
>
> RCH Downstream Port correctable trace events now report the dport
> device (dport->dport_dev) as a consequence of threading port and dport
> through the RAS helpers. The following trace event rework ("cxl: Add
> port and dport identifiers to CXL AER trace events") adds explicit
> memdev, port, dport, and host fields that provide full context for
> all device types.
>
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <djbw@kernel.org>
>
> ---
>
> Changes in v17 -> v18:
> - New patch.
> ---
> drivers/cxl/core/core.h | 12 ++++++++----
> drivers/cxl/core/ras.c | 29 +++++++++++++++--------------
> drivers/cxl/core/ras_rch.c | 2 +-
> 3 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 272634ff2615b..5ca1275fd8f35 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -185,10 +185,12 @@ static inline struct device *dport_to_host(struct cxl_dport *dport)
> #ifdef CONFIG_CXL_RAS
> void cxl_ras_init(void);
> void cxl_ras_exit(void);
> -bool cxl_handle_ras(struct device *dev, void __iomem *ras_base);
> +bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport,
> + void __iomem *ras_base);
> void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport);
> -void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base);
> +void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport,
> + void __iomem *ras_base);
> void cxl_dport_map_rch_aer(struct cxl_dport *dport);
> void cxl_disable_rch_root_ints(struct cxl_dport *dport);
> void cxl_handle_rdport_errors(struct pci_dev *pdev);
> @@ -197,13 +199,15 @@ void devm_cxl_dport_ras_setup(struct cxl_dport *dport);
> #else
> static inline void cxl_ras_init(void) { }
> static inline void cxl_ras_exit(void) { }
> -static inline bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
> +static inline bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport,
> + void __iomem *ras_base)
> {
> return false;
> }
> static inline void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport) { }
> -static inline void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base) { }
> +static inline void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport,
> + void __iomem *ras_base) { }
> static inline void cxl_dport_map_rch_aer(struct cxl_dport *dport) { }
> static inline void cxl_disable_rch_root_ints(struct cxl_dport *dport) { }
> static inline void cxl_handle_rdport_errors(struct pci_dev *pdev) { }
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 9a142abcf4f8b..6f4a3c1b0bb85 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -232,7 +232,6 @@ void __iomem *to_ras_base(struct cxl_port *port, struct cxl_dport *dport)
>
> void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port, struct cxl_dport *dport)
> {
> - struct device *dev = dport ? dport->dport_dev : port->uport_dev;
> void __iomem *ras_base = to_ras_base(port, dport);
>
> if (!ras_base) {
> @@ -241,14 +240,14 @@ void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port, struct cxl_dpo
> return;
> }
>
> - if (cxl_handle_ras(dev, ras_base))
> + if (cxl_handle_ras(port, dport, ras_base))
> panic("CXL cachemem error");
>
> dev_dbg(&pdev->dev,
> "CXL UCE signaled but no CXL RAS status bits set\n");
> }
>
> -void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
> +void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base)
> {
> u32 status;
> void __iomem *addr;
> @@ -260,10 +259,12 @@ void cxl_handle_cor_ras(struct device *dev, void __iomem *ras_base)
> status = readl(addr);
> if (status & CXL_RAS_CORRECTABLE_STATUS_MASK) {
> writel(status & CXL_RAS_CORRECTABLE_STATUS_MASK, addr);
> - if (is_cxl_memdev(dev))
> - trace_cxl_aer_correctable_error(to_cxl_memdev(dev), status);
> + if (is_cxl_endpoint(port))
> + trace_cxl_aer_correctable_error(to_cxl_memdev(port->uport_dev), status);
> + else if (dport)
> + trace_cxl_port_aer_correctable_error(dport->dport_dev, status);
> else
> - trace_cxl_port_aer_correctable_error(dev, status);
> + trace_cxl_port_aer_correctable_error(port->uport_dev, status);
> }
> }
>
> @@ -288,7 +289,7 @@ static void header_log_copy(void __iomem *ras_base, u32 *log)
> * Log the state of the RAS status registers and prepare them to log the
> * next error status. Return 1 if reset needed.
> */
> -bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
> +bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base)
> {
> u32 hl[CXL_HEADERLOG_TRACE_SIZE_U32] = {};
> void __iomem *addr;
> @@ -315,10 +316,12 @@ bool cxl_handle_ras(struct device *dev, void __iomem *ras_base)
> }
>
> header_log_copy(ras_base, hl);
> - if (is_cxl_memdev(dev))
> - trace_cxl_aer_uncorrectable_error(to_cxl_memdev(dev), status, fe, hl);
> + if (is_cxl_endpoint(port))
> + trace_cxl_aer_uncorrectable_error(to_cxl_memdev(port->uport_dev), status, fe, hl);
> + else if (dport)
> + trace_cxl_port_aer_uncorrectable_error(dport->dport_dev, status, fe, hl);
> else
> - trace_cxl_port_aer_uncorrectable_error(dev, status, fe, hl);
> + trace_cxl_port_aer_uncorrectable_error(port->uport_dev, status, fe, hl);
>
> writel(status & CXL_RAS_UNCORRECTABLE_STATUS_MASK, addr);
>
> @@ -351,7 +354,7 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> * chance the situation is recoverable dump the status of the RAS
> * capability registers and bounce the active state of the memdev.
> */
> - ue = cxl_handle_ras(port->uport_dev, to_ras_base(port, NULL));
> + ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
> }
>
> /*
> @@ -382,10 +385,8 @@ EXPORT_SYMBOL_NS_GPL(cxl_error_detected, "CXL");
> static void cxl_handle_proto_error(struct pci_dev *pdev, struct cxl_port *port,
> struct cxl_dport *dport, int severity)
> {
> - struct device *dev = dport ? dport->dport_dev : port->uport_dev;
> -
> if (severity == AER_CORRECTABLE)
> - cxl_handle_cor_ras(dev, to_ras_base(port, dport));
> + cxl_handle_cor_ras(port, dport, to_ras_base(port, dport));
> else
> cxl_do_recovery(pdev, port, dport);
> }
> diff --git a/drivers/cxl/core/ras_rch.c b/drivers/cxl/core/ras_rch.c
> index f2d2fb83758b9..f4b98f2c11a1c 100644
> --- a/drivers/cxl/core/ras_rch.c
> +++ b/drivers/cxl/core/ras_rch.c
> @@ -118,7 +118,7 @@ void cxl_handle_rdport_errors(struct pci_dev *pdev)
>
> pci_print_aer(pdev, severity, &aer_regs);
> if (severity == AER_CORRECTABLE)
> - cxl_handle_cor_ras(&pdev->dev, to_ras_base(port, dport));
> + cxl_handle_cor_ras(dport->port, dport, to_ras_base(port, dport));
> else
> cxl_do_recovery(pdev, dport->port, dport);
> }
^ permalink raw reply
* Re: [PATCH v7 09/12] PCI: liveupdate: Inherit ARI Forwarding Enable on preserved bridges
From: David Matlack @ 2026-07-20 23:19 UTC (permalink / raw)
To: Pasha Tatashin
Cc: kexec, linux-doc, linux-kernel, linux-mm, linux-pci,
Adithya Jayachandran, Alexander Graf, Alex Williamson,
Bjorn Helgaas, Chris Li, David Rientjes, Jacob Pan,
Jason Gunthorpe, Jonathan Corbet, Josh Hilke, Leon Romanovsky,
Lukas Wunner, Mike Rapoport, Parav Pandit, Pranjal Shrivastava,
Pratyush Yadav, Saeed Mahameed, Samiullah Khawaja, Shuah Khan,
Vipin Sharma, William Tu, Yi Liu
In-Reply-To: <178433098576.189683.2364970585754668266.b4-review@b4>
On Fri, Jul 17, 2026 at 4:29 PM Pasha Tatashin
<pasha.tatashin@soleen.com> wrote:
>
> On Fri, 10 Jul 2026 21:26:12 +0000, David Matlack <dmatlack@google.com> wrote:
> > diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> > index a95bfe5eff77..74a11e520f0d 100644
> > --- a/drivers/pci/liveupdate.c
> > +++ b/drivers/pci/liveupdate.c
> > @@ -816,6 +820,20 @@ int pci_liveupdate_enable_acs(struct pci_dev *dev)
> > return 0;
> > }
> >
> > +int pci_liveupdate_configure_ari(struct pci_dev *dev)
> > +{
> > + u16 val;
> > +
> > + guard(rwsem_read)(&pci_liveupdate.rwsem);
> > +
> > + if (!dev->liveupdate.incoming)
> > + return -EINVAL;
> > +
> > + pcie_capability_read_word(dev, PCI_EXP_DEVCTL2, &val);
> > + dev->ari_enabled = !!(val & PCI_EXP_DEVCTL2_ARI);
>
> unsigned int ari_enabled:1;
>
> Sashiko asks a valid question, what protects other bits in this word
> during modication? At a very list a comment is needed.
This was my reply to Sashiko, not sure if you saw it:
. pci_liveupdate_configure_ari() is called from pci_configure_ari()
. which also sets dev->ari_enabled=1 and is pre-existing code.
.
. If writing to dev->ari_enabled in this path is indeed unsafe then that
. is a pre-existing bug.
I figured that a comment wouldn't be needed for continuing an
established precedent (it's ok to write to ari_enabled during this
path).
>
> --
> Pasha Tatashin <pasha.tatashin@soleen.com>
^ permalink raw reply
* Re: [PATCH v18 09/13] cxl: Update CXL Endpoint AER handler
From: Jonathan Cameron @ 2026-07-20 23:29 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-10-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:02 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> Rename cxl_error_detected() to cxl_pci_error_detected() and rename
> the struct pci_error_handlers instance to cxl_pci_error_handlers to
> avoid shadowing the struct type tag.
>
> Document the unconditional CXL RAS read policy: on a dead link,
> readl() returns 0xFFFFFFFF which is interpreted as UCE bits set and
> triggers a panic. If RAS registers are not mapped the read is
> skipped and the frozen/perm_failure switch cases defer to AER
> recovery for devices without active CXL.mem traffic.
>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>
A couple of trivial things inline.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> drivers/cxl/core/ras.c | 24 +++++++++++++++---------
> drivers/cxl/cxlpci.h | 8 ++++----
> drivers/cxl/pci.c | 12 ++++++------
> 3 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index 6f4a3c1b0bb85..d5dc2c22565da 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
> @@ -328,10 +328,8 @@ bool cxl_handle_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem
> return true;
> }
>
> -
> -
This white space removal should be in patch 7 I think.
> -pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> - pci_channel_state_t state)
> +pci_ers_result_t cxl_pci_error_detected(struct pci_dev *pdev,
> + pci_channel_state_t state)
> {
> struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_uport(&pdev->dev);
> bool ue = false;
> @@ -349,10 +347,18 @@ pci_ers_result_t cxl_error_detected(struct pci_dev *pdev,
> }
>
> /*
> - * 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.
> + * The CXL RAS read is unconditional regardless of channel
Keep the 80 char wrap for comments. Also local style is single space after .
(I'm terrible at not following style on that stuff but do as I say not as
I do!)
> + * state. Any uncorrectable error bit set in the CXL RAS
> + * status register triggers a panic because CXL.mem cache
> + * coherency is already lost; continuing risks silent data
> + * corruption across interleaved HDM regions.
> + *
> + * On a dead link readl() returns 0xFFFFFFFF which sets all
> + * UCE bits and also triggers the panic - this is intentional.
> + * If RAS registers are not mapped the read is skipped, the
> + * panic is not reached, and the frozen/perm_failure switch
> + * cases below handle AER recovery for devices without active
> + * CXL.mem traffic.
Now this comment I like. Good and clear and explaining the non obvious decision.
> */
> ue = cxl_handle_ras(port, NULL, to_ras_base(port, NULL));
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 5c21db36073fe..6cf1db7b85020 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1000,18 +1000,18 @@ static void cxl_reset_done(struct pci_dev *pdev)
> }
> }
>
> -static const struct pci_error_handlers cxl_error_handlers = {
> - .error_detected = cxl_error_detected,
> - .slot_reset = cxl_slot_reset,
> - .resume = cxl_error_resume,
> - .reset_done = cxl_reset_done,
> +static const struct pci_error_handlers cxl_pci_error_handlers = {
> + .error_detected = cxl_pci_error_detected,
> + .slot_reset = cxl_slot_reset,
> + .resume = cxl_error_resume,
> + .reset_done = cxl_reset_done,
Do we need the re-indent? Nice to make it more obvious what didn't change.
> };
^ permalink raw reply
* Re: [PATCH v18 13/13] Documentation: cxl: Document CXL protocol error handling
From: Dave Jiang @ 2026-07-20 23:40 UTC (permalink / raw)
To: Terry Bowman, Bjorn Helgaas, Dan Williams, Ira Weiny,
Jonathan Cameron, Len Brown, Rafael J . Wysocki, Robert Richter
Cc: linux-acpi, linux-cxl, linux-doc, linux-kernel, linux-pci,
linuxppc-dev, Alejandro Lucero, Alison Schofield, Ankit Agrawal,
Ard Biesheuvel, Ben Cheatham, Borislav Petkov, Breno Leitao,
Davidlohr Bueso, Fabio M . De Francesco, Gregory Price,
Hanjun Guo, Jonathan Corbet, Kees Cook,
Kuppuswamy Sathyanarayanan, Li Ming, Mahesh J Salgaonkar,
Mauro Carvalho Chehab, Oliver O'Halloran, Shiju Jose,
Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck, Vishal Verma
In-Reply-To: <20260717222706.3540281-14-terry.bowman@amd.com>
On 7/17/26 3:27 PM, Terry Bowman wrote:
> Add Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> describing the end-to-end CXL protocol error path: AER ingress, the
> AER-CXL kfifo handoff, the cxl_core consumer worker, RCD/RCH special
> cases, severity policy, trace events, and a source code map.
>
> This documents the architecture introduced by the preceding patches in
> this series.
>
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>
> ---
> Changes in v17->v18:
> - Simplify document for readability (Jonathan)
> - Drop historical context that goes stale (Jonathan)
> - Shorten ASCII flow diagram (Jonathan)
> - Drop manual backtick markup, use automarkup (Jonathan)
> - Clarify USP/DSP as single switch component (Dave)
> - Fix line wrapping to 80 chars (Jonathan)
> ---
> Documentation/driver-api/cxl/index.rst | 1 +
> .../cxl/linux/protocol-error-handling.rst | 222 ++++++++++++++++++
> 2 files changed, 223 insertions(+)
> create mode 100644 Documentation/driver-api/cxl/linux/protocol-error-handling.rst
>
> diff --git a/Documentation/driver-api/cxl/index.rst b/Documentation/driver-api/cxl/index.rst
> index 3dfae1d310ca5..6861b2e5726a3 100644
> --- a/Documentation/driver-api/cxl/index.rst
> +++ b/Documentation/driver-api/cxl/index.rst
> @@ -42,6 +42,7 @@ that have impacts on each other. The docs here break up configurations steps.
> linux/dax-driver
> linux/memory-hotplug
> linux/access-coordinates
> + linux/protocol-error-handling
>
> .. toctree::
> :maxdepth: 2
> diff --git a/Documentation/driver-api/cxl/linux/protocol-error-handling.rst b/Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> new file mode 100644
> index 0000000000000..67f0492e56702
> --- /dev/null
> +++ b/Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> @@ -0,0 +1,222 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================
> +CXL Protocol Error Handling
> +==============================
> +
> +CXL devices report protocol-layer failures (CXL.cachemem RAS) as PCIe
> +AER Internal Errors: PCI_ERR_COR_INTERNAL for correctable events and
> +PCI_ERR_UNC_INTN for uncorrectable events. The actual fault
> +information lives in CXL RAS capability registers, not in the PCIe AER
> +status registers.
> +
> +The kernel routes every CXL Internal Error through a producer/consumer
> +pipeline shared by all CXL device types: Root Ports, Upstream/Downstream
> +Switch Ports, Endpoints, and Restricted CXL Devices (RCDs).
> +
> +
> +Architecture
> +============
> +
> +Two error planes run side by side:
> +
> +* The **PCIe AER plane** handles native PCIe errors (receiver
> + overflows, malformed TLPs, completion timeouts, etc.).
> +* The **CXL protocol error plane** handles CXL Internal Errors.
> + The AER core forwards them to cxl_core via a dedicated kfifo;
> + cxl_core reads the CXL RAS registers, emits trace events, and
> + applies recovery/panic policy.
> +
> +The boundary between the two planes is enforced by is_cxl_error() in
> +aer_cxl_vh.c. It checks info->is_cxl, the PCIe device type
> +(Endpoint, Root Port, Upstream, or Downstream), and whether the AER
> +status word indicates an internal error. RC_END devices are excluded
> +from is_cxl_error() because they reach the kfifo via the separate
> +cxl_rch_handle_error() path instead.
> +
> +The pipeline:
> +
> +1. **Producer** (aer_cxl_vh.c, aer_cxl_rch.c) - AER threaded
> + handler context. Classifies and enqueues a
> + struct cxl_proto_err_work_data into the kfifo.
> +2. **Queue** - the AER-CXL kfifo plus a backing work_struct.
> +3. **Consumer** (cxl_core/ras.c) - workqueue context. Resolves
> + the CXL port topology and dispatches to CE/UE handlers.
> +
> +
> +Topologies
> +==========
> +
> +Virtual Hierarchy (VH)
> +----------------------
> +
> +Standard PCIe topology: Root Port, optional switch (Upstream Port with
> +one or more Downstream Ports), and Endpoints. Each component raises
> +Internal Errors directly via the Root Port's AER interrupt.
> +
> +Producer: cxl_forward_error() in aer_cxl_vh.c.
> +
> +Restricted CXL Host (RCH)
> +--------------------------
> +
> +A Root Complex Event Collector (RCEC) aggregates errors from RCDs
> +attached as Root Complex Integrated Endpoints. The AER driver
> +iterates RCDs beneath the RCEC via pcie_walk_rcec() and forwards
> +each qualifying device through cxl_forward_error() into the same
> +kfifo.
> +
> +Producer: cxl_forward_error() in aer_cxl_vh.c, called from
> +cxl_rch_handle_error_iter() via pcie_walk_rcec().
> +
> +
> +Error flow
> +==========
> +
> +.. code-block:: text
> +
> + CXL device raises AER Internal Error
> + (PCI_ERR_COR_INTERNAL or PCI_ERR_UNC_INTN)
> + |
> + v
> + +--------------------------------------+
> + | AER core (aer.c) |
> + | aer_irq() -> aer_isr() |
> + | -> find_source_device() |
> + | -> handle_error_source(dev, info) |
> + +--------------------------------------+
> + |
> + v
> + +--------------------------------------+
> + | handle_error_source() dispatch |
> + | |
> + | 1. cxl_rch_handle_error() |
> + | [always; filters internally] |
> + | |
> + | 2. if is_cxl_error(): |
> + | cxl_forward_error() |
> + | [enqueue to kfifo] |
> + | |
> + | 3. if cxl_pending && non-CE: |
> + | cxl_proto_err_flush() |
> + | [sync drain before recovery] |
> + | |
> + | 4. pci_aer_handle_error() [always] |
> + +--------------------------------------+
> + |
> + (kfifo -> workqueue)
> + |
> + v
> + +--------------------------------------+
> + | __cxl_proto_err_work_fn() consumer |
> + | |
> + | if is_cxl_restricted(pdev): |
> + | cxl_handle_rdport_errors() |
> + | [RCH dport RAS first] |
> + | |
> + | port = find_cxl_port_by_dev( |
> + | &pdev->dev, NULL) |
> + | dport = cxl_find_dport_by_dev( |
> + | port, &pdev->dev) |
> + | [dport NULL for EP/USP; set RP/DSP] |
> + | |
> + | cxl_handle_proto_error() |
> + +--------------------------------------+
> + | |
> + v v
> + +-----------------+ +--------------------+
> + | CE | | UCE |
> + | cxl_handle_ | | cxl_do_recovery() |
> + | cor_ras() | | read RAS status |
> + | trace + clear | | trace + panic |
> + +-----------------+ +--------------------+
> +
> +cxl_do_recovery() reads the CXL RAS uncorrectable status register.
> +If UE bits are set, it emits the trace event and panics. If no bits
> +are set (e.g. RAS mapped but error already cleared), it logs a
> +diagnostic and defers to AER recovery.
> +
> +
> +Severity policy
> +===============
> +
> +**CE** - cxl_handle_cor_ras() reads the CXL RAS correctable status
> +register, clears set bits, and emits a cxl_aer_correctable_error
> +trace event. No recovery action.
> +
> +**UCE (non-fatal, and fatal on Root Port/Downstream Port)** - cxl_do_recovery() reads the CXL RAS
> +uncorrectable status register. If UE bits are set, the kernel panics.
> +CXL.cachemem traffic cannot be safely recovered once an uncorrectable
> +error is signaled; continuing risks silent data corruption across
> +interleaved HDM regions. This panic policy applies to the native AER
> +path. On firmware-first (CPER/GHES) platforms the CPER handler emits
> +trace events only and does not call cxl_do_recovery().
> +
> +**Fatal UCE on EP/USP** - The AER core driver does not read AER status
> +registers for Endpoint and Upstream Ports with fatal events because the
> +link is down. Without AER status, is_cxl_error() cannot classify
> +the event as a CXL protocol error and it falls through to standard
> +AER recovery.
> +
> +RCH special case
> +================
> +
> +When the consumer sees is_cxl_restricted(pdev), it calls
> +cxl_handle_rdport_errors() first to process the RCH Downstream
> +Port's RAS registers (accessed via RCRB, not standard config space).
> +It then continues to process the RCD Endpoint's own RAS registers
> +via the common path. Both register blocks are checked because
> +errors can appear in either independently.
> +
> +cxl_handle_rdport_errors() acquires the port lock internally.
> +Callers must not hold it.
> +
> +
> +Trace events
> +============
> +
> +Two trace events cover all device types and both the native AER and
> +CPER/GHES firmware-first paths:
> +
> +* cxl_aer_correctable_error
> +* cxl_aer_uncorrectable_error
> +
> +Fields:
> +
> +* ``memdev`` - memdev name for Endpoints; empty for non-Endpoints.
> +* ``port`` - CXL port device name.
> +* ``dport`` - Downstream Port device name; empty when not applicable.
> +* ``host`` - parent host bridge or uport device name.
> +* ``serial`` - PCI Device Serial Number from pdev->dsn (cached at
> + enumeration; no config-space read in the error path).
> +
> +
> +Interrupt masking
> +=================
> +
> +CXL Internal Error bits (PCI_ERR_UNC_INTN and PCI_ERR_COR_INTERNAL)
> +are unmasked in the AER capability only after the CXL RAS register
> +block is successfully mapped. A devm teardown action restores the
> +mask when the port or dport is removed, ensuring clean state after
> +driver removal.
> +
> +
> +Source files
> +============
> +
> +.. list-table::
> + :header-rows: 1
> +
> + * - File
> + - Role
> + * - drivers/pci/pcie/aer.c
> + - AER core; IRQ, dispatch
> + * - drivers/pci/pcie/aer_cxl_vh.c
> + - VH producer; kfifo
> + * - drivers/pci/pcie/aer_cxl_rch.c
> + - RCH dispatch; RCEC walk
> + * - drivers/cxl/core/ras.c
> + - Consumer; CE/UE handlers; CPER
> + * - drivers/cxl/core/ras_rch.c
> + - RCH dport RAS handling
> + * - drivers/acpi/apei/ghes.c
> + - CPER/GHES kfifo producer
^ permalink raw reply
* Re: [PATCH v18 10/13] cxl: Add port and dport identifiers to CXL AER trace events
From: Jonathan Cameron @ 2026-07-21 0:00 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-11-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:03 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> From: Dan Williams <djbw@kernel.org>
>
> Pass struct cxl_port * and struct cxl_dport * to the cxl_aer_*
> trace events instead of a plain struct device * derived at the
> caller. The trace event helpers then derive the right strings for
> endpoints, switch ports, root ports, and RCH downstream ports
> consistently across the CPER and native AER paths.
>
> The unified cxl_aer_* events keep "memdev" as the legacy field
> (endpoint events populate it with the memdev name; non-endpoint
> events emit memdev="") and add new "port" and "dport" string fields
> populated for all CXL device classes. Updated userspace can key
> off "port" and "dport" without a parallel set of events.
>
> Remove the separate cxl_port_aer_uncorrectable_error and
> cxl_port_aer_correctable_error trace events. All CXL AER events now
> use the unified cxl_aer_* events with port and dport fields.
>
> Rework cxl_cper_handle_prot_err() to use find_cxl_port_by_dev() and
> the unified trace helpers, replacing the per-port-type branching and
> bus_find_device() memdev lookup.
>
> The TP_printk format string places "port=%s dport=%s" between
> "memdev=%s" and "host=%s", changing the text-mode field order from
> the pre-patch output. This does not affect consumers such as
> rasdaemon that use libtraceevent to parse fields by name rather than
> by fixed text position.
>
> For non-endpoint events (switch port, root port, RCH dport),
> "memdev" is empty and "port"/"dport" carry the topology information.
>
> The serial number is retrieved via pci_get_dsn() which performs live
> PCI configuration space reads. A following patch ("PCI: Cache PCI
> DSN into pci_dev->dsn during probe") replaces these with a cached
> serial number to avoid config space access in error handlers and panic
> paths.
>
> Below are examples of the different CXL devices' error trace logs
> after this patch:
>
> ---------------------
> | CXL RP - 0C:00.0 |
> ---------------------
> |
> ---------------------
> | CXL USP - 0D:00.0 |
> ---------------------
> |
> --------------------
> | CXL DSP - 0E:00.0 |
> --------------------
> |
> ---------------------
> | CXL EP - 0F:00.0 |
> ---------------------
>
> Root Port:
> cxl_aer_correctable_error: memdev= port=port1 dport=0000:0c:00.0 \
> host=pci0000:0c serial=0: status: 'Memory Data ECC Error'
>
> cxl_aer_uncorrectable_error: memdev= port=port1 dport=0000:0c:00.0 \
> host=pci0000:0c serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> Upstream Switch Port:
> cxl_aer_correctable_error: memdev= port=port2 dport= host=0000:0d:00.0 \
> serial=0: status: 'Memory Data ECC Error'
>
> UCE NA - Upstream Switch Port UCE's are handled in the portdrv driver's
> PCI AER callbacks that are not CXL aware.
>
> Downstream Switch Port:
> cxl_aer_correctable_error: memdev= port=port2 dport=0000:0e:00.0 \
> host=0000:0d:00.0 serial=0: status: 'Memory Data ECC Error'
>
> cxl_aer_uncorrectable_error: memdev= port=port2 dport=0000:0e:00.0 \
> host=0000:0d:00.0 serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> Endpoint:
> cxl_aer_uncorrectable_error: memdev=mem1 port=endpoint4 dport= \
> host=0000:0f:00.0 serial=0: status: 'Cache Address Parity Error' \
> first_error: 'Cache Address Parity Error'
>
> cxl_aer_correctable_error: memdev=mem1 port=endpoint4 dport= host=0000:0f:00.0 \
> serial=0: status: 'Memory Data ECC Error'
>
> Co-developed-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Signed-off-by: Dan Williams <djbw@kernel.org>
One question inline about reference counts for the port.
>
> diff --git a/drivers/cxl/core/ras.c b/drivers/cxl/core/ras.c
> index d5dc2c22565da..acf40b2396c3b 100644
> --- a/drivers/cxl/core/ras.c
> +++ b/drivers/cxl/core/ras.c
...
>
> @@ -109,47 +77,34 @@ static struct cxl_port *find_cxl_port_by_dev(struct device *dev, struct cxl_dpor
>
> void cxl_cper_handle_prot_err(struct cxl_cper_prot_err_work_data *data)
> {
> + struct cxl_dport *dport;
> unsigned int devfn = PCI_DEVFN(data->prot_err.agent_addr.device,
> data->prot_err.agent_addr.function);
> - struct pci_dev *pdev __free(pci_dev_put) =
> - pci_get_domain_bus_and_slot(data->prot_err.agent_addr.segment,
> - data->prot_err.agent_addr.bus,
> - devfn);
> - struct cxl_memdev *cxlmd;
> - int port_type;
> -
> - if (!pdev)
> - return;
> -
> - port_type = pci_pcie_type(pdev);
> - if (port_type == PCI_EXP_TYPE_ROOT_PORT ||
> - port_type == PCI_EXP_TYPE_DOWNSTREAM ||
> - port_type == PCI_EXP_TYPE_UPSTREAM) {
> - if (data->severity == AER_CORRECTABLE)
> - cxl_cper_trace_corr_port_prot_err(pdev, data->ras_cap);
> - else
> - cxl_cper_trace_uncorr_port_prot_err(pdev, data->ras_cap);
> -
> + struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> + data->prot_err.agent_addr.segment, data->prot_err.agent_addr.bus, devfn);
> + if (!pdev) {
> + pr_err_ratelimited("Failed to find CPER device in CXL topology\n");
> return;
> }
>
> - guard(device)(&pdev->dev);
> - if (!pdev->dev.driver) {
> - dev_warn_ratelimited(&pdev->dev,
> - "Device is unbound, abort CPER error handling\n");
> + struct cxl_port *port __free(put_cxl_port) = find_cxl_port_by_dev(&pdev->dev, NULL);
> + if (!port) {
> + dev_err_ratelimited(&pdev->dev,
> + "Failed to find parent port device in CXL topology\n");
> return;
> }
>
> - struct device *mem_dev __free(put_device) = bus_find_device(
> - &cxl_bus_type, NULL, pdev, match_memdev_by_parent);
> - if (!mem_dev)
> - return;
> + guard(device)(&port->dev);
Don't we have a reference for this from find_cxl_port_by_dev()?
Superficially scope looks the same.
> +
> + /* dport is NULL for Endpoint and Upstream Port devices */
> + dport = cxl_find_dport_by_dev(port, &pdev->dev);
>
> - cxlmd = to_cxl_memdev(mem_dev);
> if (data->severity == AER_CORRECTABLE)
> - cxl_cper_trace_corr_prot_err(cxlmd, data->ras_cap);
> + cxl_cper_trace_corr_prot_err(port, dport, pci_get_dsn(pdev),
> + &data->ras_cap);
> else
> - cxl_cper_trace_uncorr_prot_err(cxlmd, data->ras_cap);
> + cxl_cper_trace_uncorr_prot_err(port, dport, pci_get_dsn(pdev),
> + &data->ras_cap);
> }
> EXPORT_SYMBOL_GPL(cxl_cper_handle_prot_err);
>
> @@ -240,14 +195,14 @@ void cxl_do_recovery(struct pci_dev *pdev, struct cxl_port *port, struct cxl_dpo
> return;
> }
>
> - if (cxl_handle_ras(port, dport, ras_base))
> + if (cxl_handle_ras(port, dport, ras_base, pci_get_dsn(pdev)))
> panic("CXL cachemem error");
>
> dev_dbg(&pdev->dev,
> "CXL UCE signaled but no CXL RAS status bits set\n");
> }
>
> -void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base)
> +void cxl_handle_cor_ras(struct cxl_port *port, struct cxl_dport *dport, void __iomem *ras_base, u64 serial)
That's over even the modern 100 char limit. Needs a line break.
> {
^ permalink raw reply
* Re: [PATCH v18 12/13] PCI/CXL: Mask/Unmask CXL protocol errors
From: Jonathan Cameron @ 2026-07-21 0:10 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-13-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:05 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> CXL protocol errors are not enabled for all CXL devices after boot.
> They must be enabled in order to process CXL protocol errors. Provide
> matching teardown helpers so the masks are restored when a CXL Port
> or dport goes away.
>
> Add pci_aer_mask_internal_errors() as the symmetric counterpart to
> pci_aer_unmask_internal_errors() and export both for the cxl_core
> module.
>
> Introduce cxl_unmask_proto_interrupts() and cxl_mask_proto_interrupts()
> in cxl_core to wrap the PCI helpers with the dev_is_pci() and
> pcie_aer_is_native() gating CXL needs. Both helpers tolerate a NULL
> or non-PCI @dev so callers do not have to special-case it.
>
> Wire cxl_unmask_proto_interrupts() into the success path of
> cxl_dport_map_ras() and devm_cxl_port_ras_setup() so the unmask
> only runs when the RAS register block was actually mapped. Pair each
> unmask with a devm_add_action_or_reset() registration of
> cxl_mask_proto_irqs() scoped to the host device so the mask is
> restored when devres is released. This applies to dports, Endpoints,
> Upstream Switch Ports, Downstream Switch Ports, and Root Ports.
>
> 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
> caused the function to return early on real hardware without mapping
> dport RAS or AER registers. The caller already gates on dport->rch,
> which is sufficient to exclude cxl_test mock devices.
>
> 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>
>
One minor thing inline and maybe take another spin at a more concise
patch description
Either way on both of them.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
...
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index 0bd23a65e7ebc..be6dc2cbd4491 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -1143,12 +1143,32 @@ 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)
> +{
> + int aer = dev->aer_cap;
> + u32 mask;
> +
> + pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, &mask);
> + mask |= PCI_ERR_UNC_INTN;
> + pci_write_config_dword(dev, aer + PCI_ERR_UNCOR_MASK, mask);
Could do
pci_clear_and_set_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
0, PCI_ERR_COR_INTERNAL);
or something along those lines.
Maybe it is worth wrapping that in some helpers to make it a bit
more regmap like in that it could have set and clear only variants.
> +
> + pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK, &mask);
> + mask |= PCI_ERR_COR_INTERNAL;
> + pci_write_config_dword(dev, aer + PCI_ERR_COR_MASK, mask);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(pci_aer_mask_internal_errors, "cxl_core");
^ permalink raw reply
* Re: [PATCH v18 13/13] Documentation: cxl: Document CXL protocol error handling
From: Jonathan Cameron @ 2026-07-21 0:19 UTC (permalink / raw)
To: Terry Bowman
Cc: Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
Rafael J . Wysocki, Robert Richter, linux-acpi, linux-cxl,
linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <20260717222706.3540281-14-terry.bowman@amd.com>
On Fri, 17 Jul 2026 17:27:06 -0500
Terry Bowman <terry.bowman@amd.com> wrote:
> Add Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> describing the end-to-end CXL protocol error path: AER ingress, the
> AER-CXL kfifo handoff, the cxl_core consumer worker, RCD/RCH special
> cases, severity policy, trace events, and a source code map.
>
> This documents the architecture introduced by the preceding patches in
> this series.
>
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
A couple of trivial things inline to tidy up. Actual text seems good
to me.
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
>
> ---
> Changes in v17->v18:
> - Simplify document for readability (Jonathan)
> - Drop historical context that goes stale (Jonathan)
> - Shorten ASCII flow diagram (Jonathan)
> - Drop manual backtick markup, use automarkup (Jonathan)
> - Clarify USP/DSP as single switch component (Dave)
> - Fix line wrapping to 80 chars (Jonathan)
> ---
> Documentation/driver-api/cxl/index.rst | 1 +
> .../cxl/linux/protocol-error-handling.rst | 222 ++++++++++++++++++
> 2 files changed, 223 insertions(+)
> create mode 100644 Documentation/driver-api/cxl/linux/protocol-error-handling.rst
>
> diff --git a/Documentation/driver-api/cxl/index.rst b/Documentation/driver-api/cxl/index.rst
> index 3dfae1d310ca5..6861b2e5726a3 100644
> --- a/Documentation/driver-api/cxl/index.rst
> +++ b/Documentation/driver-api/cxl/index.rst
> @@ -42,6 +42,7 @@ that have impacts on each other. The docs here break up configurations steps.
> linux/dax-driver
> linux/memory-hotplug
> linux/access-coordinates
> + linux/protocol-error-handling
>
> .. toctree::
> :maxdepth: 2
> diff --git a/Documentation/driver-api/cxl/linux/protocol-error-handling.rst b/Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> new file mode 100644
> index 0000000000000..67f0492e56702
> --- /dev/null
> +++ b/Documentation/driver-api/cxl/linux/protocol-error-handling.rst
> @@ -0,0 +1,222 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==============================
> +CXL Protocol Error Handling
> +==============================
> +
> +CXL devices report protocol-layer failures (CXL.cachemem RAS) as PCIe
Why short wrap? Docs are 80 chars I think.
> +AER Internal Errors: PCI_ERR_COR_INTERNAL for correctable events and
> +PCI_ERR_UNC_INTN for uncorrectable events. The actual fault
> +information lives in CXL RAS capability registers, not in the PCIe AER
> +status registers.
> +Error flow
> +==========
> +
> +.. code-block:: text
> +
> + CXL device raises AER Internal Error
> + (PCI_ERR_COR_INTERNAL or PCI_ERR_UNC_INTN)
> + |
> + v
> + +--------------------------------------+
> + | AER core (aer.c) |
> + | aer_irq() -> aer_isr() |
> + | -> find_source_device() |
> + | -> handle_error_source(dev, info) |
> + +--------------------------------------+
> + |
> + v
> + +--------------------------------------+
> + | handle_error_source() dispatch |
> + | |
> + | 1. cxl_rch_handle_error() |
> + | [always; filters internally] |
> + | |
> + | 2. if is_cxl_error(): |
Smells like a missing space.
> + | cxl_forward_error() |
> + | [enqueue to kfifo] |
> + | |
> + | 3. if cxl_pending && non-CE: |
> + | cxl_proto_err_flush() |
> + | [sync drain before recovery] |
> + | |
> + | 4. pci_aer_handle_error() [always] |
> + +--------------------------------------+
> + |
> + (kfifo -> workqueue)
> + |
> + v
> + +--------------------------------------+
> + | __cxl_proto_err_work_fn() consumer |
> + | |
> + | if is_cxl_restricted(pdev): |
> + | cxl_handle_rdport_errors() |
> + | [RCH dport RAS first] |
> + | |
> + | port = find_cxl_port_by_dev( |
> + | &pdev->dev, NULL) |
> + | dport = cxl_find_dport_by_dev( |
> + | port, &pdev->dev) |
> + | [dport NULL for EP/USP; set RP/DSP] |
check the alignment here as well. A couple of extra spaces in the
lines above I think.
> + | |
> + | cxl_handle_proto_error() |
> + +--------------------------------------+
> + | |
> + v v
> + +-----------------+ +--------------------+
> + | CE | | UCE |
> + | cxl_handle_ | | cxl_do_recovery() |
> + | cor_ras() | | read RAS status |
> + | trace + clear | | trace + panic |
> + +-----------------+ +--------------------+
> +
> +cxl_do_recovery() reads the CXL RAS uncorrectable status register.
> +If UE bits are set, it emits the trace event and panics. If no bits
> +are set (e.g. RAS mapped but error already cleared), it logs a
> +diagnostic and defers to AER recovery.
> +
> +
> +Severity policy
> +===============
> +
> +**CE** - cxl_handle_cor_ras() reads the CXL RAS correctable status
> +register, clears set bits, and emits a cxl_aer_correctable_error
> +trace event. No recovery action.
> +
> +**UCE (non-fatal, and fatal on Root Port/Downstream Port)** - cxl_do_recovery() reads the CXL RAS
Wrap needs an update here.
> +uncorrectable status register. If UE bits are set, the kernel panics.
> +CXL.cachemem traffic cannot be safely recovered once an uncorrectable
> +error is signaled; continuing risks silent data corruption across
> +interleaved HDM regions. This panic policy applies to the native AER
> +path. On firmware-first (CPER/GHES) platforms the CPER handler emits
> +trace events only and does not call cxl_do_recovery().
^ permalink raw reply
* Re: [RFC] cxl: Device protocol AER injection
From: Jonathan Cameron @ 2026-07-21 0:46 UTC (permalink / raw)
To: Bowman, Terry
Cc: Ashok Raj, Bjorn Helgaas, Dan Williams, Dave Jiang, Ira Weiny,
Len Brown, Rafael J . Wysocki, Robert Richter, linux-acpi,
linux-cxl, linux-doc, linux-kernel, linux-pci, linuxppc-dev,
Alejandro Lucero, Alison Schofield, Ankit Agrawal, Ard Biesheuvel,
Ben Cheatham, Borislav Petkov, Breno Leitao, Davidlohr Bueso,
Fabio M . De Francesco, Gregory Price, Hanjun Guo,
Jonathan Corbet, Kees Cook, Kuppuswamy Sathyanarayanan, Li Ming,
Mahesh J Salgaonkar, Mauro Carvalho Chehab, Oliver O'Halloran,
Shiju Jose, Shuah Khan, Shuai Xue, Smita Koralahalli, Tony Luck,
Vishal Verma
In-Reply-To: <62c40932-0f9c-4a67-8ff6-4bc7ba24f40e@amd.com>
> >
> > Why is this an RFC rather than a final proposal? There should always
> > be something to give the reviewer that info in the patch description.
> > I'd actually be tempted to throw a cover letter in to have somewhere
> > out of the way to put that information.
> >
> > Is it simply because it only makes sense once the other seris lands.
> >
>
> The immediate priority was providing a testing procedure for the v18 series.
> I wasn't sure how the design would be received. For instance, the to_einj_ras_base()
> could be moved into cxl-test as a to_ras_base() mock implemented function or
> it could remain in ras.c (or maybe even ras_einj.c) outside of cxl-mock. I'm
> looking forward to Alison's review and comments.
>
> My preference is to introduce core/ras_einj,c and add these changes. This
> would be to isolate all the changes except for: cxl_ras_einj_init(), cxl_ras_einj_exit(),
> and to_einj_ras_base(). I've started moving forward with making these changes
> knowing the direction could change once this receives more reviews.
>
> Also worth discussing is the commandline takes multiple parameters for
> a single sysfs file which I know isn't acceptable by everyone. I personally
> like the interface as-is because its simpler to use in comparison to multiple
> files that must be set individually.
It is debugfs so rules are much more flexible than sysfs.
> >> ---
> >> drivers/cxl/Kconfig | 13 +++
> >> drivers/cxl/core/core.h | 21 ++++
> >> drivers/cxl/core/port.c | 2 +-
> >> drivers/cxl/core/ras.c | 208 ++++++++++++++++++++++++++++++++++
> >> drivers/cxl/core/ras_rch.c | 12 ++
> >> drivers/pci/pcie/aer_inject.c | 29 ++---
> >> include/linux/aer.h | 15 +++
> >> 7 files changed, 281 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> >> index 80aeb0d556bd7..ef449228b2549 100644
> >> --- a/drivers/cxl/Kconfig
> >> +++ b/drivers/cxl/Kconfig
> >> @@ -238,6 +238,19 @@ config CXL_RAS
> >> def_bool y
> >> depends on ACPI_APEI_GHES && PCIEAER && CXL_BUS
> >>
> >> +config CXL_PROTO_AER_EINJ
> >> + bool "CXL: RAS Protocol Error Injection using AER EINJ"
> >> + depends on CXL_RAS
> >> + depends on PCIEAER_INJECT
> >
> > Do we think anyone who has CXL and PCIEAER_INJECT support will want
> > to carefully not build this? I'm just wondering if we can avoid asking
> > the question and base the built or not on the combination of those.
> >
>
> Good question: How do we incorporate this with the existing CXL EINJ
> functionality making them complementary and consistant? The existing
> EINJ is true ACPI injection but only supports RPs. The ACPI EINJ callouts
> are currrently in core/port.c. We should consider moving it into a common file
> such as core/ras_einj.c. With that move it will help force us merge the interfaces
> where/if possible and at least give central location for error RAS injection.
> I think folding the AER injection into PCIEAER_INJECT kernel config is
> reasonable but looking for others feedback.
>
I hadn't really thought about the injection method. Indeed raises
interesting questions of how it should be done. Add an ABI doc
in Documentation/ABI for next version.
> >
> >> +static DEFINE_MUTEX(cxl_aer_einj_mutex);
> >
> > Needs a comment for what data it is protecting.
> >
> >> +
> >> +struct cxl_aer_einj cxl_aer_einj = {
> >> + .lock = &cxl_aer_einj_mutex,
> >> +};
> >> +
> >> +static const char cxl_aer_einj_usage[] =
> >> + "ssss:bb:dd.f [UCE|CE] AER_STATUS RAS_STATUS [RCH]\n";
> >> +
> >> +static int cxl_aer_inject_error(struct pci_dev *pdev, bool correctable,
> >> + u32 aer_status, u32 ras_status)
> >> +{
> >> + /* RCD errors are signaled as internal errors on the associated RCEC */
> >> + if (pci_pcie_type(pdev) == PCI_EXP_TYPE_RC_END) {
> >> + if (!pdev->rcec)
> >> + return -ENODEV;
> >> + pdev = pdev->rcec;
> >> + }
> >> +
> >> + struct aer_error_inj einj = {
> >> + .bus = pdev->bus->number,
> >> + .dev = PCI_SLOT(pdev->devfn),
> >> + .fn = PCI_FUNC(pdev->devfn),
> >> + .domain = pci_domain_nr(pdev->bus),
> >> + };
> >> + int ret;
> >> + int aer_offset;
> >> + int ras_offset;
> >> +
> >> + if (correctable) {
> >> + einj.cor_status = aer_status | PCI_ERR_COR_INTERNAL;
> >> + aer_offset = PCI_ERR_COR_STATUS / sizeof(u32);
> >> + ras_offset = CXL_RAS_CORRECTABLE_STATUS_OFFSET / sizeof(u32);
> >
> > Given these are offsets into cxl_aer_einj.aer_registers / ras_registers
> > can we use sizeof(*cxl_aer_einj.aer_registers) etc
> >
> We could but that assumes the CEs are book ending the register blocks. And this would
> be inconsistent with UCE case below, right? Tell me if I misunderstaood the question.
I just meant to replace those sizeof(u32) with something that indicates
where the size is coming from. Applies equally below.
> >
> >> +
> >> +static void __iomem *to_einj_ras_base(struct cxl_port *port, struct cxl_dport *dport)
> >> +{
> >> + if (dport) {
> >> + if (cxl_aer_einj.is_rch) {
> >> + if (cxl_aer_einj.dev == dport->dport_dev) {
> >> + cxl_aer_einj.dev = NULL;
> >> + return (__force void __iomem *)cxl_aer_einj.ras_registers;
> >
> > Given the output of this is always force cast, maybe move that force up to the caller?
> >
>
> I think that works if it remains an internal helper and isn't made a mock function to to_ras_base().
> If to_einj_ras_base() is used as a mock than it would require changing the to_ras_base()
> as well.
>
> >> + }
> >> + } else {
> >> + if (cxl_aer_einj.dev == dport->dport_dev) {
> >> + pci_dev_put(to_pci_dev(cxl_aer_einj.dev));
> >
> > Not locally obvious why a thing called to_einj_ras_base should put anything it didn't
> > get. I think this needs a restructure to more obviously be tidying up references
> > that were held over the queue. At very leads needs a comment.
> > /* Reference held from X no longer needed so drop */
> >
>
> The ref was incremented in cxl_aer_einj_write() on invoking injection. The ref is
> decremented here after its usage. RCHs are excluded because they dont have a SBDF.
Just add minimal reference for that.
>
> >> + cxl_aer_einj.dev = NULL;
> >> + return (__force void __iomem *)cxl_aer_einj.ras_registers;
> >> + }
>
> >> + if (cxl_aer_einj.dev) {
> >> + void __iomem *einj = to_einj_ras_base(port, dport);
> >> + if (einj)
> >> + return einj;
> >> + }
> >> +#endif
> >> +
> >> if (dport)
> >> return dport->regs.ras;
> >>
> >> @@ -458,10 +656,20 @@ void cxl_ras_init(void)
> >> cxl_cper_register_prot_err_work(&cxl_cper_prot_err_work);
> >> cxl_register_proto_err_work(&cxl_proto_err_work,
> >> cxl_proto_err_do_flush);
> >> +#if IS_ENABLED(CONFIG_CXL_PROTO_AER_EINJ)
> >> + cxl_ras_create_debugfs(cxl_debugfs);
> > stub that in a header.
> >
>
> Actually, during rework today I moved it to cxl_ras_einj_init(),
> a new function. Thoughts?
Sounds good.
> >> diff --git a/drivers/pci/pcie/aer_inject.c b/drivers/pci/pcie/aer_inject.c
> >> index 09bfc7194ef31..b313adef680ae 100644
> >> --- a/drivers/pci/pcie/aer_inject.c
> >> +++ b/drivers/pci/pcie/aer_inject.c
> >> @@ -14,6 +14,7 @@
> >>
> >> #define dev_fmt(fmt) "aer_inject: " fmt
> >>
> >> +#include <linux/aer.h>
> >> #include <linux/module.h>
> >> #include <linux/init.h>
> >> #include <linux/interrupt.h>
> >> @@ -31,19 +32,6 @@
> >> static bool aer_mask_override;
> >> module_param(aer_mask_override, bool, 0);
> >>
> >> -struct aer_error_inj {
> >> - u8 bus;
> >> - u8 dev;
> >> - u8 fn;
> >> - u32 uncor_status;
> >> - u32 cor_status;
> >> - u32 header_log0;
> >> - u32 header_log1;
> >> - u32 header_log2;
> >> - u32 header_log3;
> >> - u32 domain;
> >> -};
> >> -
> >> struct aer_error {
> >> struct list_head list;
> >> u32 domain;
> >> @@ -316,7 +304,7 @@ static int pci_bus_set_aer_ops(struct pci_bus *bus)
> >> return 0;
> >> }
> >>
> >> -static int aer_inject(struct aer_error_inj *einj)
> >> +int aer_inject(struct aer_error_inj *einj)
> >> {
> >> struct aer_error *err, *rperr;
> >> struct aer_error *err_alloc = NULL, *rperr_alloc = NULL;
> >> @@ -332,10 +320,14 @@ static int aer_inject(struct aer_error_inj *einj)
> >> dev = pci_get_domain_bus_and_slot(einj->domain, einj->bus, devfn);
> >> if (!dev)
> >> return -ENODEV;
> >> - rpdev = pcie_find_root_port(dev);
> >> - /* If Root Port not found, try to find an RCEC */
> >> - if (!rpdev)
> >> - rpdev = dev->rcec;
> >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
> >
> > { }
> > as the else is multiline (see coding standard)
> >
>
> Ok
>
> > Maybe need a comment for why it might be an RCEC for injection.
> > Is this an RCH specific path where there is nothing else to target?
> >
> >> + rpdev = dev;
> >> + else {
> >> + rpdev = pcie_find_root_port(dev);
> >> + /* If Root Port not found, try to find an RCEC */
> >> + if (!rpdev)
> >> + rpdev = dev->rcec;
> >> + }
> >> if (!rpdev) {
> >> pci_err(dev, "Neither Root Port nor RCEC found\n");
> >> ret = -ENODEV;
> >> @@ -482,6 +474,7 @@ static int aer_inject(struct aer_error_inj *einj)
> >> pci_dev_put(dev);
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL_GPL(aer_inject);
> > I wonder if we want to restrict this to specific modules?
> >
> > One for Bjorn probably.
> >
> Because right now it injects AER to any PCI device.
>
> Also, worth discussing is the commandline currently uses multiple parameters for
> a single sysfs file. I mentioned this at the top.
>
>
> Thanks for reviewing Jonathan.
>
> -Terry
>
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] mm/damon: introduce DAMOS_SPLIT action
From: SJ Park @ 2026-07-21 0:47 UTC (permalink / raw)
To: Zi Yan
Cc: SJ Park, Lian Wang, david, damon, linux-mm, akpm, linux-kernel,
ljs, liam, vbabka, rppt, surenb, mhocko, npache, baolin.wang,
ryan.roberts, daichaobing, wangkefeng.wang, gutierrez.asier,
zengheng4, kasong, corbet, skhan, linux-doc, linux-kselftest,
lianux.wang, kunwu.chan
In-Reply-To: <DK3MYJN8KSZ5.1U0STUJGETSJH@nvidia.com>
Hello,
On Mon, 20 Jul 2026 15:12:50 -0400 "Zi Yan" <ziy@nvidia.com> wrote:
> On Mon Jul 20, 2026 at 5:56 AM EDT, Lian Wang wrote:
> > Hi David,
> >
> > On 7/20/2026 10:44 AM, David Hildenbrand (Arm) wrote:
> >> you give no real motivation and evaluation why this is required or
> >> why this gives the user any benefit.
> >> A SPLIT with an explicit order is not really want we want and it
> >> does not fit the existing primitives.
> >
> > Thank you for the direct feedback. Let me explain where this came
> > from -- the cover letter should have included this context.
> >
> > This started from a real problem at Sangfor. The scenario is:
> >
> > KVM-QEMU virtualization on Kunpeng 920, with KVM guest memory
> > backed by tmpfs shared mappings (THP=always on the host). An
> > Oracle database runs inside the VM. DAMON monitors the KVM
> > process on the host to measure the hot-memory ratio.
> >
> > The KVM process allocates and uses a large amount of memory.
> > Under the same workload, DAMON reports a significantly higher
> > hot-memory ratio with THP enabled versus THP disabled. Direct
> > tmpfs write tests inside the VM -- touching at 4K and 2M
> > strides -- show a clear gap between the two cases.
> >
> > DAMON parameters used:
> >
> > operations=vaddr
> > monitoring_attrs/nr_regions/min=500
> > monitoring_attrs/nr_regions/max=2000
> > monitoring_attrs/intervals/sample_us=500000
> > monitoring_attrs/intervals/aggr_us=20000000
> > monitoring_attrs/intervals/update_us=60000000
Thank you for sharing your detailed setup. It is helpful. Btw, have you
considered using intervals auto-tuning [1]?
> > schemes/0/action=stat
> > schemes/0/access_pattern/nr_accesses/min=1
> > schemes/0/access_pattern/nr_accesses/max=max
> >
> > The underlying issue is that under PMD-mapped THP, DAMON's monitoring
> > granularity is coarser than the actual working set -- a single
> > Accessed bit covers 512 base pages. Before SJ's probe infrastructure
> > arrives, there is a gap: DAMON cannot distinguish hot sub-pages from
> > cold ones within a single THP.
> >
> > Split is one possible mechanism to bridge that gap -- by dismantling
> > the PMD mapping, each base page gets its own PTE Accessed bit and
> > DAMON recovers fine-grain monitoring.
Thank you for clarifying the motivation of this series.
To me, it's still unclear what is the real user impact, though. I mean, I can
understand DAMON suddenly reporting more hot memory can surprise some people.
But, why that matters in what extent for your use case? You may not run DAMON
on your system only to read the information. You may run it to do something
beneficial using the information. What is that, and how badly degraded DAMON's
monitoring results affect it?
Overall, unless the real impact is serious, splitting huge pages only for
better DAMON monitoring sounds like not a good tradeoff. You will increase
DAMON overhead and lose THP benefits in some extent.
> > It is not intended to be a
> > permanent API, and certainly not "the opposite of collapse".
Once it is added to the kernel, we have to support it for long term. Let's not
introduce something for only temporal use.
>
> If you just want PTE level access bit information, why not split PMD
> mapping instead of the THP itself?
>
> In addition, the issue is about access monitoring granularity in DAMON,
> why should user care and know about THP split operations? I would expect
> DAMON detects the inability of getting fine grain access information and
> split the PMD mapping itself instead of a user initiated DAMON_SPLIT. If
> that is not possible with DAMON, an alternative is to provide something
> more generic like DAMON_SAMPLE, which does the split under the hood,
> instead of exposing MM internal operations.
Thank you for good opinion, Zi. I agree all the points. That said, I still
want to understand the problem first.
>
> >
> > I did not write this scenario into the cover letter because our test
> > results do not yet show a clear quantitative benefit worth claiming,
> > and I did not want to oversell. Without the context, I understand it
> > looks like I randomly proposed a new primitive -- that was not the
> > intention.
> >
> > SJ acknowledged [1] that the monitoring problem under THP is real.
Yes, the behavior is real and I agree your theory of how it happens. I don't
clearly understand if it is really bad in what situations, though.
> > My RFC is a concrete proposal to start the discussion. If split with
> > an explicit order is not the right primitive, I would appreciate your
> > thoughts on what the correct DAMOS abstraction for this should be.
Only after understanding what is the problem and how bad it is, we will be able
to think of different approaches and assess those. To me, it is still unclear
what is the real problem and how bad it is. I will wait for your further
clarifications of those.
> >
> > [1] https://lore.kernel.org/20260620203915.82947-1-sj@kernel.org/
[1] https://origin.kernel.org/doc/html/latest/mm/damon/design.html#monitoring-intervals-auto-tuning
Thanks,
SJ
[...]
^ permalink raw reply
* Re: [PATCH v9 8/8] drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults
From: Matthew Brost @ 2026-07-21 0:49 UTC (permalink / raw)
To: Stanislav Kinsburskii
Cc: airlied, akhilesh, akpm, corbet, dakr, david, decui, haiyangz,
jgg, kees, kys, leon, liam, lizhi.hou, ljs, longli, lyude,
maarten.lankhorst, mamin506, mhocko, mripard, nouveau, ogabbay,
oleg, rppt, shuah, simona, skhan, surenb, tzimmermann, vbabka,
wei.liu, dri-devel, intel-xe, linux-mm, linux-doc, linux-hyperv,
linux-kernel, linux-kselftest, linux-rdma
In-Reply-To: <178413941236.1155966.14485211391150425997.stgit@skinsburskii>
On Wed, Jul 15, 2026 at 11:16:52AM -0700, Stanislav Kinsburskii wrote:
> Several GPU SVM paths take mmap_read_lock() only to call hmm_range_fault()
> and open-code mmu interval sequence setup before each HMM walk. They also
> retry -EBUSY until HMM_RANGE_DEFAULT_TIMEOUT expires.
>
> Use hmm_range_fault_unlocked_timeout() for those faults. The HMM helper now
> owns mmap_lock acquisition and refreshes range->notifier_seq for its
> internal retries, while GPU SVM keeps its existing driver-lock validation
> with mmu_interval_read_retry() after a successful fault.
>
> Pass HMM_RANGE_DEFAULT_TIMEOUT as the helper retry budget for each HMM
> fault attempt. This scopes the timeout to repeated HMM notifier retries
> while preserving the outer retry loops that restart when the interval is
> invalidated before GPU SVM updates or consumes the mapping state.
>
This part doesn't seem right for get_pages(), see below.
> Leave drm_gpusvm_check_pages() on hmm_range_fault() because that path is
> called with the mmap lock already held by its caller.
>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
> ---
> drivers/gpu/drm/drm_gpusvm.c | 61 +++++-------------------------------------
> 1 file changed, 7 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd..de5bbfe58ee9 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -773,8 +773,7 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct drm_gpusvm_range *range,
> .end = end,
> .dev_private_owner = dev_private_owner,
> };
> - unsigned long timeout =
> - jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> + unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> enum drm_gpusvm_scan_result state = DRM_GPUSVM_SCAN_UNPOPULATED, new_state;
> unsigned long *pfns;
> unsigned long npages = npages_in_range(start, end);
> @@ -788,22 +787,7 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct drm_gpusvm_range *range,
> hmm_range.hmm_pfns = pfns;
>
> retry:
> - hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> - mmap_read_lock(range->gpusvm->mm);
> -
> - while (true) {
> - err = hmm_range_fault(&hmm_range);
> - if (err == -EBUSY) {
> - if (time_after(jiffies, timeout))
> - break;
> -
> - hmm_range.notifier_seq =
> - mmu_interval_read_begin(notifier);
> - continue;
> - }
> - break;
> - }
> - mmap_read_unlock(range->gpusvm->mm);
> + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
> if (err)
> goto err_free;
>
> @@ -1406,8 +1390,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> .dev_private_owner = ctx->device_private_page_owner,
> };
> void *zdd;
> - unsigned long timeout =
> - jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> + unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> unsigned long i, j;
> unsigned long npages = npages_in_range(pages_start, pages_end);
> unsigned long num_dma_mapped;
> @@ -1422,9 +1405,6 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> struct dma_iova_state *state = &svm_pages->state;
>
> retry:
> - if (time_after(jiffies, timeout))
> - return -EBUSY;
> -
I think that by deleting the code above, you have changed this function's
semantics by removing the hard cap of HMM_RANGE_DEFAULT_TIMEOUT. This
code was added because, on some non-production platforms, the timing in
this function could cause it to livelock.
Is there any reason this was remove aside from timeout variable not
being a deadline now? You likely should add the deadline back in.
Matt
> hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
> goto set_seqno;
> @@ -1439,21 +1419,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
> }
>
> hmm_range.hmm_pfns = pfns;
> - while (true) {
> - mmap_read_lock(mm);
> - err = hmm_range_fault(&hmm_range);
> - mmap_read_unlock(mm);
> -
> - if (err == -EBUSY) {
> - if (time_after(jiffies, timeout))
> - break;
> -
> - hmm_range.notifier_seq =
> - mmu_interval_read_begin(notifier);
> - continue;
> - }
> - break;
> - }
> + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
> mmput(mm);
> if (err)
> goto err_free;
> @@ -1720,8 +1686,7 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
> .end = drm_gpusvm_range_end(range),
> .dev_private_owner = NULL,
> };
> - unsigned long timeout =
> - jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> + unsigned long timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> unsigned long *pfns;
> unsigned long npages = npages_in_range(drm_gpusvm_range_start(range),
> drm_gpusvm_range_end(range));
> @@ -1736,24 +1701,12 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
> return -ENOMEM;
>
> hmm_range.hmm_pfns = pfns;
> - while (!time_after(jiffies, timeout)) {
> - hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> - if (time_after(jiffies, timeout)) {
> - err = -ETIME;
> - break;
> - }
> -
> - mmap_read_lock(mm);
> - err = hmm_range_fault(&hmm_range);
> - mmap_read_unlock(mm);
> - if (err != -EBUSY)
> - break;
> - }
> + err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
>
> kvfree(pfns);
> mmput(mm);
>
> - return err;
> + return err == -EBUSY ? -ETIME : err;
> }
> EXPORT_SYMBOL_GPL(drm_gpusvm_range_evict);
>
>
>
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] mm/damon: introduce DAMOS_SPLIT action
From: SJ Park @ 2026-07-21 1:08 UTC (permalink / raw)
To: Lian Wang
Cc: SJ Park, damon, linux-mm, akpm, linux-kernel, david, ljs, liam,
vbabka, rppt, surenb, mhocko, npache, ziy, baolin.wang,
ryan.roberts, daichaobing, wangkefeng.wang, gutierrez.asier,
zengheng4, kasong, corbet, skhan, linux-doc, linux-kselftest,
lianux.wang, kunwu.chan
In-Reply-To: <20260720030327.80153-1-lianux.mm@gmail.com>
You were warned at not Cc-ing THP developers in the previous revision, since it
was modifying THP source code. Since this version is modifying only DAMON
source code, you don't really need to Cc more than DAMON developers. Asking
wider inputs is good practice. And it worked very well. We got great inputs
from Asier, David and Zi. But some people don't really like having too much
mails in their inbox. My personal rule of thumb is just running
get_maintainer.pl via 'hkml patch format' [1]. You must have your own rule,
though :)
On Mon, 20 Jul 2026 11:03:24 +0800 Lian Wang <lianux.mm@gmail.com> wrote:
> DAMOS_SPLIT splits large folios in a target region down to a
> configured target order, using the existing split_folio_to_order().
> No new core-mm code or exported symbols are introduced.
The last sentence may better to go to changelog. If you want to highlight, you
can put changelog at the top of the cover letter.
>
> Based on mm-unstable at 61cccb8363fc ("mm/swap, PM: hibernate:
> atomically replace hibernation pin").
This is also not feasible to be the cover letter main content.
>
> Different addresses within a PMD-mapped folio resolve to the same
> PMD Accessed bit. Accesses to a small part of the folio can
> therefore coarsen DAMON's observed hot set relative to the actual
> working set.
>
> DAMOS already provides promotion actions (HUGEPAGE, COLLAPSE) but
> has no corresponding demotion action. DAMOS_SPLIT fills this gap.
> It is a mechanism, not a policy -- it does not decide which folios
> to split. Selection is left to DAMON's existing access patterns,
> filters, and future probe/PMU signals.
As I commented to the previous version [2], this sounds like you are saying two
very different things. Monitoring quality degradation issue and better THP
handling of DAMOS. This makes understanding the motivation of this series
difficult, as other people also pointed out.
Based on your replies to others, now I understand you are proposing DAMOS_SPLIT
as a way for improving the monitoring results. I'm waiting for your more
clarification of the issue, to better assess if this makes sense or not, as I
commented on the reply to Zi's reply.
>
> target_order selects the split target: 0 for order-0 base pages,
> or a supported smaller mTHP order. Both anonymous and file-backed
> folios are supported. The locking follows split_huge_pages_in_pid()
> in mm/huge_memory.c.
>
> Tests
> =====
>
> damos_split.py (VM + Kunpeng 920):
> anon THP -> order-0 split: PASS
>
> sangfor_exp.py (Kunpeng 920, tmpfs, 4096 MiB):
>
> - Created a 4 GiB PMD-mapped tmpfs workload.
> - Applied DAMOS_SPLIT with target_order=0.
> - ShmemPmdMapped dropped from 4194304 KiB to 0 in every round.
> - Repeated for five rounds without functional failures.
>
> The functional selftest (damos_split.py) is included in this series.
> Additional experiment scripts and raw results are available on
> request. Performance characterization using masim [1] and KMB [2]
> is in progress.
It is completely fine to keep having tests in progress. But, please make the
story complete. What damos_split.py and sangfor_exp.py do? What the results
mean? What the performance tests will do with what expectation?
>
> [1] https://github.com/sjp38/masim
> [2] https://gitee.com/OpenCloudOS/kernel-multi-bench
>
> Open questions
> ==============
>
> - Selection policy: this series keeps folio selection outside the
> action and relies on DAMOS access patterns, filters, and quotas.
> Is this the appropriate layering for future probe-based signals?
You mentioned this series is for monitoring quality improvement. If so,
shouldn't you just apply it to all THPs, regardless of the access pattern? I'm
again being confused. More clarification of the motivation would be useful.
>
> - Hysteresis: khugepaged may re-collapse a just-split folio.
> Should cooldown live in DAMON policy or khugepaged?
Ditto.
>
> - File-backed folios: adjust target_order upward to filesystem
> minimum, or keep current "fail and skip"?
I don't fully understand the question. Could you please elaborate more?
>
> Beyond the action API itself, feedback on real workloads that need
> proactive large-folio demotion is particularly welcome. Follow-up
> work will evaluate candidate selection signals, including DAMON
> probes and hardware-assisted sampling, as well as target-order
> selection and split/collapse hysteresis. Those policies are
> intentionally kept outside this series.
You mentioned this work is for monitoring quality improvement. Now you are
saying somewhat followup. I'm again being confused.
>
> Changes since v2 [3]
> ====================
>
> - Split-only series (collapse deferred).
> - Dropped SPE feedback (mechanism/policy separation).
> - DAMOS_MTHP_SPLIT -> DAMOS_SPLIT.
> - order field in existing union (no struct size increase).
> - Added functional selftest (damos_split.py).
> - checkpatch: 0 errors, 0 warnings.
>
> [3] https://lore.kernel.org/20260701123000.00000-1-lianux.mm@gmail.com/
>
> Lian Wang (Processmission) (3):
> mm/damon: introduce DAMOS_SPLIT action
> mm/damon/vaddr: implement DAMOS_SPLIT handler
> selftests/damon: add functional test for DAMOS_SPLIT
>
> .../ABI/testing/sysfs-kernel-mm-damon | 7 ++
> Documentation/mm/damon/design.rst | 5 +
> include/linux/damon.h | 15 ++-
> mm/damon/core.c | 2 +
> mm/damon/sysfs-schemes.c | 48 +++++++
> mm/damon/vaddr.c | 90 +++++++++++++
> tools/testing/selftests/damon/Makefile | 1 +
> tools/testing/selftests/damon/_damon_sysfs.py | 9 +-
> tools/testing/selftests/damon/damos_split.py | 99 +++++++++++++++
> tools/testing/selftests/damon/sysfs.py | 11 +-
> 10 files changed, 279 insertions(+), 8 deletions(-)
[1] https://github.com/sjp38/hackermail/blob/master/USAGE.md#formatting-patches
[2] https://lore.kernel.org/20260702183551.91007-1-sj@kernel.org
Thanks,
SJ
^ permalink raw reply
* Re: [PATCH] cgroup/cpu: document cpu.stat nice_usec and core_sched.force_idle_usec
From: Tao Cui @ 2026-07-21 1:26 UTC (permalink / raw)
To: Joshua Hahn, Michal Koutný
Cc: cui.tao, cgroups, Tejun Heo, Johannes Weiner, Jonathan Corbet,
Shuah Khan, linux-doc, linux-kernel, cuitao
In-Reply-To: <20260720152741.1226695-1-joshua.hahnjy@gmail.com>
在 2026/7/20 23:27, Joshua Hahn 写道:
>> On Mon, Jul 20, 2026 at 08:02:26AM -0700, Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
>>> I have to ask, which tree is this based on? I can't seem to find a single
>>> tree that contains the "(including those in descendant cgroups)" section
>>> anywhere.
>>
>> It is most likely linux-next which contains a nearby modification from
>> [1] (or it was based on that regardless).
>
> Hello Michal,
>
> Thank you, I must have missed linux-next in my searching... that makes
> a lot of sense! With that, Tao, please feel free to add my review tag:
>
> Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>
>
Thanks Michal for the pointer, and Joshua for the tag.
Based on linux-next (next-20260717).
Thanks，
Tao
> Thanks, have a great day!
> Joshua
>
>> HTH,
>> Michal
>>
>> [1] https://lore.kernel.org/lkml/20260629060636.200118-1-sunshaojie@kylinos.cn/
^ permalink raw reply
* Re: [RFC PATCH v3 0/3] mm/damon: introduce DAMOS_SPLIT action
From: Lian Wang @ 2026-07-21 1:34 UTC (permalink / raw)
To: sj, david, ziy
Cc: damon, linux-mm, akpm, linux-kernel, ljs, liam, vbabka, rppt,
surenb, mhocko, npache, baolin.wang, ryan.roberts, daichaobing,
wangkefeng.wang, gutierrez.asier, zengheng4, kasong, corbet,
skhan, linux-doc, linux-kselftest, lianux.mm, lianux.wang,
kunwu.chan
In-Reply-To: <20260721004755.93824-1-sj@kernel.org>
Hi SJ,
On 7/20/2026 5:47 PM, SJ Park wrote:
> To me, it's still unclear what is the real user impact, though.
> ...
> Only after understanding what is the problem and how bad it is, we
> will be able to think of different approaches and assess those.
You are right. I will work with Sangfor to quantify the real impact
in their production scenario -- what DAMOS action is driven by the
inflated hot-memory readings, and how badly the monitoring error
affects the actual outcome.
Thank you, David, and Zi Yan for the honest and constructive feedback.
It made me realize I should have led with the problem, not the
mechanism.
Let me take a step back, gather concrete data on the problem severity,
and come back with a clearer picture. I will follow up in this thread
once I have something solid to share.
Thanks,
Lian Wang
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: iio: adc: Add AD7768
From: David Lechner @ 2026-07-21 1:39 UTC (permalink / raw)
To: Janani Sunil, Janani Sunil, Nuno Sá, Michael Hennerich,
Jonathan Cameron, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan, Philipp Zabel,
Linus Walleij, Bartosz Golaszewski, Jonathan Corbet, Shuah Khan
Cc: linux, linux-iio, devicetree, linux-kernel, linux-gpio, linux-doc
In-Reply-To: <9dd16bb5-7a30-4024-88a7-4a4bf47c35e8@gmail.com>
On 7/20/26 9:00 AM, Janani Sunil wrote:
>
> On 7/9/26 17:43, David Lechner wrote:
>> On 7/9/26 3:50 AM, Janani Sunil wrote:
>>> Devicetree Bindings for AD7768-4 (4 channel) and AD7768 (8 channel)
>>> simultaneous sampling ADC
>>>
>>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
>>> ---
>>>
>>> +
>>> + adi,power-mode:
>>> + $ref: /schemas/types.yaml#/definitions/string
>>> + enum:
>>> + - low
>>> + - median
>>> + - fast
>>> + description:
>>> + Power mode selection.
>> Unless there are pins that control this, it seems like it should be
>> left up to the driver to decide how to set this.
>>
>> In this case, it looks like the power mode also influences sample rate
>> which is normally something controlled at runtime.
Looking at this again, there is also an MCLK divider that influences
sample rate, so sampling_frequency to power mode is not straight-forward
anyway.
>
> Hi David,
>
> The reason we'd like to retain power mode control is that certain ODRs are supported across all three power modes (low/median/fast), and the RMS noise and power consumption differ significantly between them at the same ODR.
>
> The higher the power mode, the better the noise performance, but power consumption nearly doubles for every ~3 dB improvement in dynamic range. Silently selecting one power mode in the driver would remove a meaningful hardware tradeoff from the user.
>
> We'd like to propose the following instead:
> - Remove adi,power-mode from the DT as suggested.
> - Expose power mode as a per-device sysfs attribute.
> - in_voltage<N>_sampling_frequency_available dynamically reflects only the ODRs valid for the currently selected power mode.
>
> This keeps the DT clean while still giving the user explicit control over the noise versus power trade off. Would this approach be acceptable?
>
> Thanks,
> Jan
>
>
Jonathan usually pushes back against userspace power controls. We do have
this for accelerometers, but not ADCs currently.
If we can't think of anything better, maybe we could use this. It only
has low_noise and low_power options though, so the driver would still
need to chose the best power mode of the 3 based on the other requested
parameters. E.g. always make all sampling_frequency available and just
pick the highest power or lowest power mode that can provide that rate
based on the power_mode attribute.
I wanted to suggest maybe adding some kind of noise attribute instead,
but I'm not sure how we could do that in a way using SI units since the
value would depend on so many things (at least V_REF voltage, filter type,
temperature and even the physical input).
^ permalink raw reply
* Re: [PATCH] cgroup/cpu: document cpu.stat nice_usec and core_sched.force_idle_usec
From: Tao Cui @ 2026-07-21 1:43 UTC (permalink / raw)
To: Michal Koutný
Cc: cui.tao, cgroups, Tejun Heo, Johannes Weiner, Jonathan Corbet,
Shuah Khan, Joshua Hahn, linux-doc, linux-kernel, cuitao
In-Reply-To: <al46ipavC-KCaoHv@localhost.localdomain>
在 2026/7/20 23:26, Michal Koutný 写道:
> On Sat, Jul 18, 2026 at 04:01:00PM +0800, Tao Cui <cui.tao@linux.dev> wrote:
>> From: Tao Cui <cuitao@kylinos.cn>
>>
>> cgroup_base_stat_cputime_show() unconditionally prints a fourth base
>> stat, nice_usec, in addition to usage_usec/user_usec/system_usec, and
>> also prints core_sched.force_idle_usec when CONFIG_SCHED_CORE is enabled.
>> Neither field is currently described in the cpu.stat section of
>> cgroup-v2.rst, which still states it "always reports the following three
>> stats".
>
> Just to better sense popularity of the fields -- have you found this out
> by:
> a) reading cpu.stat and being confused about semantics of the fields or
> b) scanning files/code vs docs looking for discrepancies?
>
(b). I found it while auditing the cgroup cpu controller, comparing
what cgroup_base_stat_cputime_show() prints against the cpu.stat docs.
nice_usec is emitted unconditionally but not listed there, and the
"three stats" count was stale.
The audit is groundwork for a prototype daemon that monitors
per-cgroup resource usage and adjusts limits dynamically. The design
is inspired by DAMON, but DAMON targets a single resource domain,
whereas container workloads need coordinated control across several
(cpu, memory, io) — which is why I'm going through the per-controller
stat interfaces.
Thanks,
Tao
> Thanks,
> Michal
^ permalink raw reply
* [PATCH v7 0/2] hwmon: add Altera SoC FPGA hardware monitoring support
From: tze.yee.ng @ 2026-07-21 2:17 UTC (permalink / raw)
To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet,
Shuah Khan, linux-hwmon, linux-doc
From: Tze Yee Ng <tze.yee.ng@altera.com>
This series adds hardware monitor support for Altera SoC FPGA devices.
Temperature and voltage sensors are accessed through the Stratix 10
service layer and Secure Device Manager (SDM).
Patch 1 adds async HWMON SMC support to stratix10-svc and registers the
socfpga-hwmon platform device.
Patch 2 adds the socfpga-hwmon driver, documentation, Kconfig, and
MAINTAINERS entry.
Changes in v7:
- In Patch 2, address maintainer and Sashiko review feedback on
socfpga-hwmon:
- Break out of the async poll loop on hard errors and return the real
poll status instead of rewriting it to -ETIMEDOUT
- Drop redundant async poll alert/debug messages (SVC already logs)
- Replace msleep(1) with usleep_range() for short retries
- On sync-mode timeout, wait for the outstanding SVC callback before
calling stratix10_svc_done() so a leftover svc_fifo request cannot
desynchronize later sensor reads; still report -ETIMEDOUT
- No functional changes in Patch 1
Changes in v6:
- Rebase on torvalds/master (given “Linux 7.2-rc4”)
- No functional changes in Patch 1 and Patch 2
Changes in v5:
- Rebase on dinguyen/socfpga_svc_fixes_for_v7.2
- Address Sashiko review feedback on socfpga-hwmon (Patch 2):
- Poll async responses until HWMON_TIMEOUT instead of a fixed
3-iteration retry loop (~3 ms), fixing premature timeouts on
silicon
- Add MODULE_ALIAS("platform:socfpga-hwmon")
- No functional changes in Patch 1
Changes in v4:
- Address maintainer and review feedback on socfpga-hwmon (Patch 2):
- Register devm_add_action_or_reset() before
devm_hwmon_device_register_with_info() to fix devres teardown order
- Remove unreferenced completion and pre-poll
wait_for_completion_io_timeout() from async reads; poll directly
with a retry loop after async_send()
- No functional changes in Patch 1
Changes in v3:
- Address review feedback on socfpga-hwmon (Patch 2):
- Fix 16-bit Q8.8 temperature sign extension
- Drop unused async callback; pass NULL to stratix10_svc_async_send()
- Document and retain pre-poll wait (RSU pattern; firmware needs time
before async_poll())
- Align async poll retry behaviour with RSU
- Use uninterruptible wait_for_completion_timeout() for sync reads
- Handle -EINVAL and -EOPNOTSUPP when falling back to sync mode
- Defer SVC channel cleanup via devm_add_action_or_reset()
- No functional changes in Patch 1
Changes in v2:
- Drop altr,stratix10-hwmon DT binding and intel,stratix10-svc hwmon
child property
- Drop Stratix 10 SoCDK DTS hwmon node
- Register socfpga-hwmon from stratix10-svc (RSU-style)
- Replace DT channel parsing with hardcoded Stratix 10 and Agilex tables
- Rename driver/module to socfpga-hwmon
(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)
- Add Agilex channel support
- Fix SDM value conversion (Q8.8 degrees Celsius and Q16 volts to hwmon
millidegrees/millivolts)
- Improve sync-mode error handling via last_err
Previous version:
https://lore.kernel.org/all/cover.1784516953.git.tze.yee.ng@altera.com/
Tze Yee Ng (2):
firmware: stratix10-svc: add async HWMON read commands and register
socfpga-hwmon device
hwmon: add Altera SoC FPGA hardware monitoring driver
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/socfpga-hwmon.rst | 34 ++
MAINTAINERS | 8 +
drivers/firmware/stratix10-svc.c | 46 +-
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/socfpga-hwmon.c | 586 +++++++++++++++++++
include/linux/firmware/intel/stratix10-smc.h | 38 ++
8 files changed, 721 insertions(+), 3 deletions(-)
create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
create mode 100644 drivers/hwmon/socfpga-hwmon.c
--
2.43.7
^ permalink raw reply
* [PATCH v7 2/2] hwmon: add Altera SoC FPGA hardware monitoring driver
From: tze.yee.ng @ 2026-07-21 2:18 UTC (permalink / raw)
To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet,
Shuah Khan, linux-hwmon, linux-doc
In-Reply-To: <cover.1784599864.git.tze.yee.ng@altera.com>
From: Tze Yee Ng <tze.yee.ng@altera.com>
Add a hardware monitor driver for Altera SoC FPGA devices using the
Stratix 10 service layer. Sensor channels are selected based on the
service layer compatible string.
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v7:
- Break out of the async poll loop on hard errors; return the real poll
status instead of -ETIMEDOUT
- Drop redundant async poll alert and -EAGAIN debug messages
- Replace msleep(1) with usleep_range() for short retries
- On sync-mode timeout, wait for the outstanding SVC callback before
stratix10_svc_done() to avoid leftover svc_fifo entries desynchronizing
later reads; still report -ETIMEDOUT to userspace
Changes in v6:
- No functional changes from v5
Changes in v5:
- Poll async responses until HWMON_TIMEOUT (2 s) instead of a fixed
3-iteration retry loop (~3 ms), fixing premature timeouts observed
on silicon
- Add MODULE_ALIAS("platform:socfpga-hwmon")
Changes in v4:
- Register devm_add_action_or_reset() before
devm_hwmon_device_register_with_info(); drop manual channel cleanup
on hwmon registration failure
- Remove unreferenced async completion and pre-poll
wait_for_completion_io_timeout(); poll directly after async_send()
with the existing retry loop
Changes in v3:
- Fix 16-bit signed Q8.8 temperature conversion (cast through s16)
- Remove unused async callback; pass NULL to stratix10_svc_async_send()
- Keep wait_for_completion_io_timeout() before polling with comment
explaining the service layer never invokes the callback but firmware
needs time to complete the transaction (RSU pattern)
- Align async poll loop with RSU (retry on failure instead of aborting)
- Use wait_for_completion_timeout() for synchronous reads
- Handle -EINVAL and -EOPNOTSUPP when async client registration fails
- Defer SVC channel/async cleanup via devm_add_action_or_reset();
drop .remove()
Changes in v2:
- Drop altr,stratix10-hwmon OF compatible and DT channel parsing
- Select channels from hardcoded tables using parent SVC compatible
(intel,stratix10-svc or intel,agilex-svc)
- Rename driver from stratix10-hwmon to socfpga-hwmon
- Rename Kconfig symbol to CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON
- Add Agilex voltage and temperature channel tables
- Convert SDM Q8.8 degrees Celsius to hwmon millidegrees
- Convert SDM Q16 volts to hwmon millivolts
- Use socfpga_hwmon as hwmon sysfs device name
- Add last_err for synchronous SVC read error propagation
- Update Documentation/hwmon and MAINTAINERS accordingly
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/socfpga-hwmon.rst | 34 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/socfpga-hwmon.c | 586 ++++++++++++++++++++++++++
6 files changed, 640 insertions(+)
create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
create mode 100644 drivers/hwmon/socfpga-hwmon.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 29130df44d12..3299417a24b8 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -254,6 +254,7 @@ Hardware Monitoring Kernel Drivers
sparx5-temp
spd5118
stpddc60
+ socfpga-hwmon
surface_fan
sy7636a-hwmon
tc654
diff --git a/Documentation/hwmon/socfpga-hwmon.rst b/Documentation/hwmon/socfpga-hwmon.rst
new file mode 100644
index 000000000000..e5da42556a62
--- /dev/null
+++ b/Documentation/hwmon/socfpga-hwmon.rst
@@ -0,0 +1,34 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver socfpga-hwmon
+=============================
+
+Supported chips:
+
+ * Altera Stratix 10 SoC FPGA
+ * Altera Agilex SoC FPGA
+
+Authors:
+ - Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
+ - Tze Yee Ng <tze.yee.ng@altera.com>
+
+Description
+-----------
+
+This driver supports hardware monitoring for Altera SoC
+FPGA devices through the Secure Device Manager and Stratix 10 service layer.
+
+The following sensor types are supported:
+
+ * temperature
+ * voltage
+
+Usage Notes
+-----------
+
+The stratix10-svc driver registers a socfpga-hwmon platform device when
+hardware monitor support is enabled. Sensor channels are selected in the
+driver based on the service layer compatible string:
+
+ * intel,stratix10-svc
+ * intel,agilex-svc
diff --git a/MAINTAINERS b/MAINTAINERS
index a674e36529f7..ab6cb215a02f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -965,6 +965,14 @@ L: linux-gpio@vger.kernel.org
S: Maintained
F: drivers/gpio/gpio-altera.c
+ALTERA SoC FPGA HWMON DRIVER
+M: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
+M: Tze Yee Ng <tze.yee.ng@altera.com>
+L: linux-hwmon@vger.kernel.org
+S: Maintained
+F: Documentation/hwmon/socfpga-hwmon.rst
+F: drivers/hwmon/socfpga-hwmon.c
+
ALTERA TRIPLE SPEED ETHERNET DRIVER
M: Boon Khai Ng <boon.khai.ng@altera.com>
L: netdev@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2bfbcc033d59..8e768661fec2 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2160,6 +2160,16 @@ config SENSORS_SMSC47M192
This driver can also be built as a module. If so, the module
will be called smsc47m192.
+config SENSORS_ALTERA_SOCFPGA_HWMON
+ tristate "Altera SoC FPGA hardware monitoring features"
+ depends on INTEL_STRATIX10_SERVICE
+ help
+ If you say yes here you get support for the temperature and
+ voltage sensors of Altera SoC FPGA devices.
+
+ This driver can also be built as a module. If so, the module
+ will be called socfpga-hwmon.
+
config SENSORS_SMSC47B397
tristate "SMSC LPC47B397-NC"
depends on HAS_IOPORT
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 63effc0ab8d1..aeedee80e1f0 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -221,6 +221,7 @@ obj-$(CONFIG_SENSORS_SMPRO) += smpro-hwmon.o
obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
+obj-$(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON) += socfpga-hwmon.o
obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
obj-$(CONFIG_SENSORS_SPD5118) += spd5118.o
obj-$(CONFIG_SENSORS_STTS751) += stts751.o
diff --git a/drivers/hwmon/socfpga-hwmon.c b/drivers/hwmon/socfpga-hwmon.c
new file mode 100644
index 000000000000..111435f9da95
--- /dev/null
+++ b/drivers/hwmon/socfpga-hwmon.c
@@ -0,0 +1,586 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Altera SoC FPGA hardware monitoring driver
+ *
+ * Copyright (c) 2026 Altera Corporation
+ *
+ * Authors:
+ * Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
+ * Tze Yee Ng <tze.yee.ng@altera.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/firmware/intel/stratix10-svc-client.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define HWMON_TIMEOUT msecs_to_jiffies(SVC_HWMON_REQUEST_TIMEOUT_MS)
+#define HWMON_RETRY_SLEEP_US 1000U
+#define HWMON_ASYNC_MSG_RETRY 3U
+#define SOCFPGA_HWMON_MAXSENSORS 16
+#define SOCFPGA_HWMON_CHANNEL_MASK GENMASK(15, 0)
+#define SOCFPGA_HWMON_PAGE_SHIFT 16
+#define SOCFPGA_HWMON_CHAN(page, channel) \
+ (((page) << SOCFPGA_HWMON_PAGE_SHIFT) | \
+ ((channel) & SOCFPGA_HWMON_CHANNEL_MASK))
+#define SOCFPGA_HWMON_ATTR_VISIBLE 0444
+/* Temperature from SDM is signed Q8.8 degrees Celsius (8 fractional bits). */
+#define SOCFPGA_HWMON_TEMP_FRAC_BITS 8
+#define SOCFPGA_HWMON_TEMP_FRAC_DIV BIT(SOCFPGA_HWMON_TEMP_FRAC_BITS)
+#define SOCFPGA_HWMON_TEMP_MDEG_SCALE 1000
+/* Voltage from SDM is unsigned Q16 volts (16 fractional bits). */
+#define SOCFPGA_HWMON_VOLT_FRAC_BITS 16
+#define SOCFPGA_HWMON_VOLT_FRAC_DIV BIT(SOCFPGA_HWMON_VOLT_FRAC_BITS)
+#define SOCFPGA_HWMON_VOLT_MV_SCALE 1000
+
+#define ETEMP_INACTIVE 0x80000000U
+#define ETEMP_TOO_OLD 0x80000001U
+#define ETEMP_NOT_PRESENT 0x80000002U
+#define ETEMP_TIMEOUT 0x80000003U
+#define ETEMP_CORRUPT 0x80000004U
+#define ETEMP_BUSY 0x80000005U
+#define ETEMP_NOT_INITIALIZED 0x800000FFU
+
+struct socfpga_hwmon_channel {
+ u32 reg;
+ const char *label;
+};
+
+struct socfpga_hwmon_board_data {
+ const struct socfpga_hwmon_channel *temp;
+ unsigned int num_temp;
+ const struct socfpga_hwmon_channel *volt;
+ unsigned int num_volt;
+};
+
+struct socfpga_hwmon_priv {
+ struct stratix10_svc_chan *chan;
+ struct stratix10_svc_client client;
+ struct completion completion;
+ struct mutex lock; /* protect SVC calls */
+ bool async;
+ int last_err; /* sync-mode SVC result; 0 on success */
+ u32 temperature;
+ u32 voltage;
+ int temperature_channels;
+ int voltage_channels;
+ const char *temp_chan_names[SOCFPGA_HWMON_MAXSENSORS];
+ const char *volt_chan_names[SOCFPGA_HWMON_MAXSENSORS];
+ u32 temp_chan[SOCFPGA_HWMON_MAXSENSORS];
+ u32 volt_chan[SOCFPGA_HWMON_MAXSENSORS];
+};
+
+static umode_t socfpga_hwmon_is_visible(const void *dev,
+ enum hwmon_sensor_types type,
+ u32 attr, int chan)
+{
+ const struct socfpga_hwmon_priv *priv = dev;
+
+ switch (type) {
+ case hwmon_temp:
+ if (chan < priv->temperature_channels)
+ return SOCFPGA_HWMON_ATTR_VISIBLE;
+ return 0;
+ case hwmon_in:
+ if (chan < priv->voltage_channels)
+ return SOCFPGA_HWMON_ATTR_VISIBLE;
+ return 0;
+ default:
+ return 0;
+ }
+}
+
+static void socfpga_hwmon_readtemp_cb(struct stratix10_svc_client *client,
+ struct stratix10_svc_cb_data *data)
+{
+ struct socfpga_hwmon_priv *priv = client->priv;
+
+ priv->last_err = -EIO;
+ if (data->status == BIT(SVC_STATUS_OK)) {
+ priv->last_err = 0;
+ priv->temperature = (u32)*(unsigned long *)data->kaddr1;
+ } else if (data->kaddr1) {
+ dev_err(client->dev, "%s failed with status 0x%x, value 0x%lx\n",
+ __func__, data->status,
+ *(unsigned long *)data->kaddr1);
+ } else {
+ dev_err(client->dev, "%s failed with status 0x%x\n",
+ __func__, data->status);
+ }
+
+ complete(&priv->completion);
+}
+
+static void socfpga_hwmon_readvolt_cb(struct stratix10_svc_client *client,
+ struct stratix10_svc_cb_data *data)
+{
+ struct socfpga_hwmon_priv *priv = client->priv;
+
+ priv->last_err = -EIO;
+ if (data->status == BIT(SVC_STATUS_OK)) {
+ priv->last_err = 0;
+ priv->voltage = (u32)*(unsigned long *)data->kaddr1;
+ } else if (data->kaddr1) {
+ dev_err(client->dev, "%s failed with status 0x%x, value 0x%lx\n",
+ __func__, data->status,
+ *(unsigned long *)data->kaddr1);
+ } else {
+ dev_err(client->dev, "%s failed with status 0x%x\n",
+ __func__, data->status);
+ }
+
+ complete(&priv->completion);
+}
+
+static int socfpga_hwmon_parse_temp(long *val, u32 temperature)
+{
+ switch (temperature) {
+ case ETEMP_INACTIVE:
+ case ETEMP_NOT_PRESENT:
+ case ETEMP_CORRUPT:
+ case ETEMP_NOT_INITIALIZED:
+ return -EOPNOTSUPP;
+ case ETEMP_TIMEOUT:
+ case ETEMP_BUSY:
+ case ETEMP_TOO_OLD:
+ return -EAGAIN;
+ default:
+ /* SDM returns a 16-bit signed Q8.8 value in the low 16 bits. */
+ *val = (long)(s16)(temperature & SOCFPGA_HWMON_CHANNEL_MASK) *
+ SOCFPGA_HWMON_TEMP_MDEG_SCALE / SOCFPGA_HWMON_TEMP_FRAC_DIV;
+ return 0;
+ }
+}
+
+static int socfpga_hwmon_encode_temp_arg(u32 reg, u64 *arg)
+{
+ u32 page = (reg >> SOCFPGA_HWMON_PAGE_SHIFT) & SOCFPGA_HWMON_CHANNEL_MASK;
+ u32 channel = reg & SOCFPGA_HWMON_CHANNEL_MASK;
+
+ if (channel >= SOCFPGA_HWMON_MAXSENSORS)
+ return -EINVAL;
+
+ *arg = (1ULL << channel) | ((u64)page << SOCFPGA_HWMON_PAGE_SHIFT);
+ return 0;
+}
+
+static int socfpga_hwmon_encode_volt_arg(u32 reg, u64 *arg)
+{
+ u32 channel = reg & SOCFPGA_HWMON_CHANNEL_MASK;
+
+ if (channel >= SOCFPGA_HWMON_MAXSENSORS)
+ return -EINVAL;
+
+ *arg = 1ULL << channel;
+ return 0;
+}
+
+static int socfpga_hwmon_async_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ struct stratix10_svc_client_msg *msg)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+ struct stratix10_svc_cb_data data = {};
+ unsigned long deadline = jiffies + HWMON_TIMEOUT;
+ void *handle = NULL;
+ int status, index, ret;
+
+ for (index = 0; index < HWMON_ASYNC_MSG_RETRY; index++) {
+ status = stratix10_svc_async_send(priv->chan, msg, &handle,
+ NULL, NULL);
+ if (status == 0)
+ break;
+ dev_warn(dev, "Failed to send async message: %d\n", status);
+ usleep_range(HWMON_RETRY_SLEEP_US, HWMON_RETRY_SLEEP_US * 2);
+ }
+
+ if (status && !handle) {
+ dev_err(dev, "Failed to send async message after %u retries: %d\n",
+ HWMON_ASYNC_MSG_RETRY, status);
+ return status;
+ }
+
+ ret = -ETIMEDOUT;
+ while (!time_after(jiffies, deadline)) {
+ status = stratix10_svc_async_poll(priv->chan, handle, &data);
+ if (status == -EAGAIN) {
+ /* still in progress */
+ } else if (status < 0) {
+ ret = status;
+ break;
+ } else if (status == 0) {
+ ret = 0;
+ break;
+ }
+ usleep_range(HWMON_RETRY_SLEEP_US, HWMON_RETRY_SLEEP_US * 2);
+ }
+
+ if (ret) {
+ dev_err(dev, "Failed to get async response\n");
+ goto done;
+ }
+
+ if (data.status) {
+ dev_err(dev, "%s returned 0x%x from SDM\n", __func__,
+ data.status);
+ ret = -EFAULT;
+ goto done;
+ }
+
+ if (type == hwmon_temp)
+ priv->temperature = (u32)*(unsigned long *)data.kaddr1;
+ else
+ priv->voltage = (u32)*(unsigned long *)data.kaddr1;
+
+ ret = 0;
+
+done:
+ stratix10_svc_async_done(priv->chan, handle);
+ return ret;
+}
+
+static int socfpga_hwmon_sync_read(struct device *dev,
+ enum hwmon_sensor_types type,
+ struct stratix10_svc_client_msg *msg)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+ int ret;
+
+ reinit_completion(&priv->completion);
+
+ if (type == hwmon_temp)
+ priv->client.receive_cb = socfpga_hwmon_readtemp_cb;
+ else
+ priv->client.receive_cb = socfpga_hwmon_readvolt_cb;
+
+ ret = stratix10_svc_send(priv->chan, msg);
+ if (ret < 0)
+ goto status_done;
+
+ ret = wait_for_completion_timeout(&priv->completion, HWMON_TIMEOUT);
+ if (!ret) {
+ dev_err(priv->client.dev, "timeout waiting for SMC call\n");
+ /*
+ * stratix10_svc_done() stops the worker but does not flush
+ * svc_fifo. Wait for the outstanding callback before stopping
+ * so a leftover request cannot complete a later sensor read.
+ * Discard the late result and still report -ETIMEDOUT.
+ */
+ wait_for_completion(&priv->completion);
+ ret = -ETIMEDOUT;
+ goto status_done;
+ }
+
+ ret = priv->last_err;
+
+status_done:
+ stratix10_svc_done(priv->chan);
+ return ret;
+}
+
+static int socfpga_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int chan, long *val)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+ struct stratix10_svc_client_msg msg = {0};
+ int ret;
+
+ if (chan >= SOCFPGA_HWMON_MAXSENSORS)
+ return -EOPNOTSUPP;
+
+ switch (type) {
+ case hwmon_temp:
+ ret = socfpga_hwmon_encode_temp_arg(priv->temp_chan[chan],
+ &msg.arg[0]);
+ if (ret)
+ return ret;
+ msg.command = COMMAND_HWMON_READTEMP;
+ break;
+ case hwmon_in:
+ ret = socfpga_hwmon_encode_volt_arg(priv->volt_chan[chan],
+ &msg.arg[0]);
+ if (ret)
+ return ret;
+ msg.command = COMMAND_HWMON_READVOLT;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ guard(mutex)(&priv->lock);
+ if (priv->async)
+ ret = socfpga_hwmon_async_read(dev, type, &msg);
+ else
+ ret = socfpga_hwmon_sync_read(dev, type, &msg);
+ if (ret)
+ return ret;
+
+ if (type == hwmon_temp)
+ ret = socfpga_hwmon_parse_temp(val, priv->temperature);
+ else
+ /* SDM returns Q16 volts; convert to hwmon millivolts. */
+ *val = (long)priv->voltage * SOCFPGA_HWMON_VOLT_MV_SCALE /
+ SOCFPGA_HWMON_VOLT_FRAC_DIV;
+ return ret;
+}
+
+static int socfpga_hwmon_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int chan, const char **str)
+{
+ struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_in:
+ *str = priv->volt_chan_names[chan];
+ return 0;
+ case hwmon_temp:
+ *str = priv->temp_chan_names[chan];
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static const struct hwmon_ops socfpga_hwmon_ops = {
+ .is_visible = socfpga_hwmon_is_visible,
+ .read = socfpga_hwmon_read,
+ .read_string = socfpga_hwmon_read_string,
+};
+
+static const struct hwmon_channel_info *socfpga_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_LABEL),
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL),
+ NULL
+};
+
+static const struct hwmon_chip_info socfpga_hwmon_chip_info = {
+ .ops = &socfpga_hwmon_ops,
+ .info = socfpga_hwmon_info,
+};
+
+static const struct socfpga_hwmon_channel s10_hwmon_volt_channels[] = {
+ { SOCFPGA_HWMON_CHAN(0, 2), "0.8V VCC" },
+ { SOCFPGA_HWMON_CHAN(0, 3), "1.8V VCCIO_SDM" },
+ { SOCFPGA_HWMON_CHAN(0, 6), "0.9V VCCERAM" },
+};
+
+static const struct socfpga_hwmon_channel s10_hwmon_temp_channels[] = {
+ { SOCFPGA_HWMON_CHAN(0, 0), "Main Die SDM" },
+};
+
+static const struct socfpga_hwmon_board_data s10_hwmon_board = {
+ .temp = s10_hwmon_temp_channels,
+ .num_temp = ARRAY_SIZE(s10_hwmon_temp_channels),
+ .volt = s10_hwmon_volt_channels,
+ .num_volt = ARRAY_SIZE(s10_hwmon_volt_channels),
+};
+
+static const struct socfpga_hwmon_channel agilex_hwmon_volt_channels[] = {
+ { SOCFPGA_HWMON_CHAN(0, 2), "0.8V VCC" },
+ { SOCFPGA_HWMON_CHAN(0, 3), "1.8V VCCIO_SDM" },
+ { SOCFPGA_HWMON_CHAN(0, 4), "1.8V VCCPT" },
+ { SOCFPGA_HWMON_CHAN(0, 5), "1.2V VCCCRCORE" },
+ { SOCFPGA_HWMON_CHAN(0, 6), "0.9V VCCH" },
+ { SOCFPGA_HWMON_CHAN(0, 7), "0.8V VCCL" },
+};
+
+static const struct socfpga_hwmon_channel agilex_hwmon_temp_channels[] = {
+ { SOCFPGA_HWMON_CHAN(0, 0), "Main Die SDM" },
+ { SOCFPGA_HWMON_CHAN(1, 0), "Main Die corner bottom left max" },
+ { SOCFPGA_HWMON_CHAN(2, 0), "Main Die corner top left max" },
+ { SOCFPGA_HWMON_CHAN(3, 0), "Main Die corner bottom right max" },
+ { SOCFPGA_HWMON_CHAN(4, 0), "Main Die corner top right max" },
+};
+
+static const struct socfpga_hwmon_board_data agilex_hwmon_board = {
+ .temp = agilex_hwmon_temp_channels,
+ .num_temp = ARRAY_SIZE(agilex_hwmon_temp_channels),
+ .volt = agilex_hwmon_volt_channels,
+ .num_volt = ARRAY_SIZE(agilex_hwmon_volt_channels),
+};
+
+static const struct socfpga_hwmon_board_data *
+socfpga_hwmon_get_board(struct device *dev)
+{
+ struct device_node *np = dev->of_node;
+
+ if (!np)
+ return NULL;
+
+ if (of_device_is_compatible(np, "intel,stratix10-svc"))
+ return &s10_hwmon_board;
+ if (of_device_is_compatible(np, "intel,agilex-svc"))
+ return &agilex_hwmon_board;
+
+ return NULL;
+}
+
+static int socfpga_hwmon_init_channels(struct device *dev,
+ const struct socfpga_hwmon_board_data *board,
+ struct socfpga_hwmon_priv *priv)
+{
+ unsigned int i;
+
+ if (board->num_temp > SOCFPGA_HWMON_MAXSENSORS ||
+ board->num_volt > SOCFPGA_HWMON_MAXSENSORS)
+ return -EINVAL;
+
+ for (i = 0; i < board->num_temp; i++) {
+ priv->temp_chan_names[i] = board->temp[i].label;
+ priv->temp_chan[i] = board->temp[i].reg;
+ }
+ priv->temperature_channels = board->num_temp;
+
+ for (i = 0; i < board->num_volt; i++) {
+ priv->volt_chan_names[i] = board->volt[i].label;
+ priv->volt_chan[i] = board->volt[i].reg;
+ }
+ priv->voltage_channels = board->num_volt;
+
+ return 0;
+}
+
+static void socfpga_hwmon_release_svc(void *data)
+{
+ struct socfpga_hwmon_priv *priv = data;
+
+ if (priv->async)
+ stratix10_svc_remove_async_client(priv->chan);
+ stratix10_svc_free_channel(priv->chan);
+}
+
+static int socfpga_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device *parent = dev->parent;
+ const struct socfpga_hwmon_board_data *board;
+ struct socfpga_hwmon_priv *priv;
+ struct device *hwmon_dev;
+ int ret;
+
+ if (!parent || !parent->of_node) {
+ dev_err(dev, "missing parent device node\n");
+ return -ENODEV;
+ }
+
+ board = socfpga_hwmon_get_board(parent);
+ if (!board) {
+ dev_err(dev, "unsupported service layer compatible\n");
+ return -ENODEV;
+ }
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->client.dev = dev;
+ priv->client.priv = priv;
+ init_completion(&priv->completion);
+ mutex_init(&priv->lock);
+
+ ret = socfpga_hwmon_init_channels(dev, board, priv);
+ if (ret)
+ return ret;
+
+ priv->chan = stratix10_svc_request_channel_byname(&priv->client,
+ SVC_CLIENT_HWMON);
+ if (IS_ERR(priv->chan)) {
+ ret = PTR_ERR(priv->chan);
+ if (ret == -EPROBE_DEFER)
+ dev_dbg(dev, "service channel %s not ready, deferring probe\n",
+ SVC_CLIENT_HWMON);
+ else
+ dev_err(dev, "couldn't get service channel %s: %d\n",
+ SVC_CLIENT_HWMON, ret);
+ return ret;
+ }
+
+ ret = stratix10_svc_add_async_client(priv->chan, false);
+ switch (ret) {
+ case 0:
+ priv->async = true;
+ break;
+ case -EINVAL:
+ case -EOPNOTSUPP:
+ /*
+ * stratix10_svc_add_async_client() returns -EINVAL when the
+ * async controller is not initialized; fall back to sync mode.
+ */
+ dev_dbg(dev, "async operations not supported, using sync mode\n");
+ priv->async = false;
+ break;
+ default:
+ dev_err(dev, "failed to add async client: %d\n", ret);
+ stratix10_svc_free_channel(priv->chan);
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(dev, socfpga_hwmon_release_svc, priv);
+ if (ret)
+ return ret;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev, "socfpga_hwmon",
+ priv,
+ &socfpga_hwmon_chip_info,
+ NULL);
+ if (IS_ERR(hwmon_dev))
+ return PTR_ERR(hwmon_dev);
+
+ platform_set_drvdata(pdev, priv);
+ return 0;
+}
+
+static struct platform_driver socfpga_hwmon_driver = {
+ .probe = socfpga_hwmon_probe,
+ .driver = {
+ .name = "socfpga-hwmon",
+ },
+};
+module_platform_driver(socfpga_hwmon_driver);
+
+MODULE_AUTHOR("Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>");
+MODULE_AUTHOR("Tze Yee Ng <tze.yee.ng@altera.com>");
+MODULE_DESCRIPTION("Altera SoC FPGA hardware monitoring driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:socfpga-hwmon");
--
2.43.7
^ permalink raw reply related
* [PATCH v7 1/2] firmware: stratix10-svc: add async HWMON read commands and register socfpga-hwmon device
From: tze.yee.ng @ 2026-07-21 2:18 UTC (permalink / raw)
To: Dinh Nguyen, linux-kernel, Guenter Roeck, Jonathan Corbet,
Shuah Khan, linux-hwmon, linux-doc
In-Reply-To: <cover.1784599864.git.tze.yee.ng@altera.com>
From: Tze Yee Ng <tze.yee.ng@altera.com>
Add asynchronous Stratix 10 service layer support for hardware monitor
temperature and voltage read commands in stratix10_svc_async_send() and
stratix10_svc_async_prepare_response().
Register a socfpga-hwmon platform device from the service layer driver
when hardware monitor support is enabled, similar to the RSU device.
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
Changes in v7:
- No functional changes from v6
Changes in v6:
- No functional changes from v5
Changes in v5:
- No functional changes from v4
Changes in v3:
- No functional changes from v2
Changes in v2:
- Extend patch scope beyond async SMC support: register socfpga-hwmon
platform device from stratix10-svc when CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON
is enabled
- Follow RSU-style registration; RSU probe error handling is unchanged
- Add err_unregister_clients to unregister hwmon and RSU on populate failure
- Unregister hwmon platform device in stratix10-svc remove()
---
drivers/firmware/stratix10-svc.c | 46 ++++++++++++++++++--
include/linux/firmware/intel/stratix10-smc.h | 38 ++++++++++++++++
2 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index c24ca5823078..fc38afed5b7f 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -45,6 +45,7 @@
/* stratix10 service layer clients */
#define STRATIX10_RSU "stratix10-rsu"
+#define SOCFPGA_HWMON "socfpga-hwmon"
/* Maximum number of SDM client IDs. */
#define MAX_SDM_CLIENT_IDS 16
@@ -104,9 +105,11 @@ struct stratix10_svc_chan;
/**
* struct stratix10_svc - svc private data
* @stratix10_svc_rsu: pointer to stratix10 RSU device
+ * @stratix10_svc_hwmon: pointer to stratix10 HWMON device
*/
struct stratix10_svc {
struct platform_device *stratix10_svc_rsu;
+ struct platform_device *stratix10_svc_hwmon;
};
/**
@@ -1329,6 +1332,14 @@ int stratix10_svc_async_send(struct stratix10_svc_chan *chan, void *msg,
args.a0 = INTEL_SIP_SMC_ASYNC_RSU_NOTIFY;
args.a2 = p_msg->arg[0];
break;
+ case COMMAND_HWMON_READTEMP:
+ args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READTEMP;
+ args.a2 = p_msg->arg[0];
+ break;
+ case COMMAND_HWMON_READVOLT:
+ args.a0 = INTEL_SIP_SMC_ASYNC_HWMON_READVOLT;
+ args.a2 = p_msg->arg[0];
+ break;
default:
dev_err(ctrl->dev, "Invalid command ,%d\n", p_msg->command);
ret = -EINVAL;
@@ -1422,6 +1433,10 @@ static int stratix10_svc_async_prepare_response(struct stratix10_svc_chan *chan,
*/
data->kaddr1 = (void *)&handle->res;
break;
+ case COMMAND_HWMON_READTEMP:
+ case COMMAND_HWMON_READVOLT:
+ data->kaddr1 = (void *)&handle->res.a2;
+ break;
default:
dev_alert(ctrl->dev, "Invalid command\n ,%d", p_msg->command);
@@ -2013,16 +2028,38 @@ static int stratix10_svc_drv_probe(struct platform_device *pdev)
if (ret)
goto err_put_device;
+ if (IS_ENABLED(CONFIG_SENSORS_ALTERA_SOCFPGA_HWMON)) {
+ svc->stratix10_svc_hwmon =
+ platform_device_alloc(SOCFPGA_HWMON, 0);
+ if (!svc->stratix10_svc_hwmon) {
+ dev_err(dev, "failed to allocate %s device\n",
+ SOCFPGA_HWMON);
+ } else {
+ svc->stratix10_svc_hwmon->dev.parent = dev;
+
+ ret = platform_device_add(svc->stratix10_svc_hwmon);
+ if (ret) {
+ dev_err(dev, "failed to add %s device: %d\n",
+ SOCFPGA_HWMON, ret);
+ platform_device_put(svc->stratix10_svc_hwmon);
+ svc->stratix10_svc_hwmon = NULL;
+ }
+ }
+ }
+
ret = of_platform_default_populate(dev_of_node(dev), NULL, dev);
if (ret)
- goto err_unregister_rsu_dev;
+ goto err_unregister_clients;
pr_info("Intel Service Layer Driver Initialized\n");
return 0;
-err_unregister_rsu_dev:
- platform_device_unregister(svc->stratix10_svc_rsu);
+err_unregister_clients:
+ if (svc->stratix10_svc_hwmon)
+ platform_device_unregister(svc->stratix10_svc_hwmon);
+ if (svc->stratix10_svc_rsu)
+ platform_device_unregister(svc->stratix10_svc_rsu);
goto err_free_fifos;
err_put_device:
platform_device_put(svc->stratix10_svc_rsu);
@@ -2046,6 +2083,9 @@ static void stratix10_svc_drv_remove(struct platform_device *pdev)
struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
struct stratix10_svc *svc = ctrl->svc;
+ if (svc->stratix10_svc_hwmon)
+ platform_device_unregister(svc->stratix10_svc_hwmon);
+
stratix10_svc_async_exit(ctrl);
of_platform_depopulate(ctrl->dev);
diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/linux/firmware/intel/stratix10-smc.h
index 9116512169dc..18ac6fe96d9d 100644
--- a/include/linux/firmware/intel/stratix10-smc.h
+++ b/include/linux/firmware/intel/stratix10-smc.h
@@ -695,6 +695,44 @@ INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
#define INTEL_SIP_SMC_ASYNC_POLL \
INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_POLL)
+/**
+ * Request INTEL_SIP_SMC_ASYNC_HWMON_READTEMP
+ * Async call to request temperature
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_ASYNC_HWMON_READTEMP
+ * a1 transaction job id
+ * a2 Temperature Channel
+ * a3-a17 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_STATUS_REJECTED
+ * or INTEL_SIP_SMC_STATUS_BUSY
+ * a1-a17 not used
+ */
+#define INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READTEMP 0xE8
+#define INTEL_SIP_SMC_ASYNC_HWMON_READTEMP \
+ INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READTEMP)
+
+/**
+ * Request INTEL_SIP_SMC_ASYNC_HWMON_READVOLT
+ * Async call to request voltage
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_ASYNC_HWMON_READVOLT
+ * a1 transaction job id
+ * a2 Voltage Channel
+ * a3-a17 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_STATUS_REJECTED
+ * or INTEL_SIP_SMC_STATUS_BUSY
+ * a1-a17 not used
+ */
+#define INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READVOLT 0xE9
+#define INTEL_SIP_SMC_ASYNC_HWMON_READVOLT \
+ INTEL_SIP_SMC_ASYNC_VAL(INTEL_SIP_SMC_ASYNC_FUNC_ID_HWMON_READVOLT)
+
/**
* Request INTEL_SIP_SMC_ASYNC_RSU_GET_SPT
* Async call to get RSU SPT from SDM.
--
2.43.7
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox