From: Bjorn Helgaas <bhelgaas@google.com>
This work is mostly due to Jon Pan-Doh and Karolina Stolarek. I rebased
this to v6.15-rc1, factored out some of the trace and statistics updates,
and added some minor cleanups.
Proposal
========
When using native AER, spammy devices can flood kernel logs with AER errors
and slow/stall execution. Add per-device per-error-severity ratelimits for
more robust error logging. Allow userspace to configure ratelimits via
sysfs knobs.
Motivation
==========
Inconsistent PCIe error handling, exacerbated at datacenter scale (myriad
of devices), affects repairabilitiy flows for fleet operators.
Exposing PCIe errors/debug info in-band for a userspace daemon (e.g.
rasdaemon) to collect/pass on to repairability services will allow for more
predictable repair flows and decrease machine downtime.
Background
==========
AER error spam has been observed many times, both publicly (e.g. [1], [2],
[3]) and privately. While it usually occurs with correctable errors, it can
happen with uncorrectable errors (e.g. during new HW bringup).
There have been previous attempts to add ratelimits to AER logs ([4], [5]).
The most recent attempt[5] has many similarities with the proposed
approach.
v6:
- Rebase to v6.15-rc1
- Initialize struct aer_err_info completely before using it
- Log DPC Error Source ID only when it's valid
- Consolidate AER Error Source ID logging to one place
- Tidy Error Source ID bus/dev/fn decoding using macros
- Rename aer_print_port_info() to aer_print_source()
- Consolidate trace events and statistic updates to one non-ratelimited place
- Save log level in struct aer_err_info instead of passing as parameter
v5: https://lore.kernel.org/r/20250321015806.954866-1-pandoh@google.com
- Handle multi-error AER by evaluating ratelimits once and storing result
- Reword/rename commit messages/functions/variable
v4: https://lore.kernel.org/r/20250320082057.622983-1-pandoh@google.com
- Fix bug where trace not emitted with malformed aer_err_info
- Extend ratelimit to malformed aer_err_info
- Update commit messages with patch motivation
- Squash AER sysfs filename change (Patch 8)
v3: https://lore.kernel.org/r/20250319084050.366718-1-pandoh@google.com
- Ratelimit aer_print_port_info() (drop Patch 1)
- Add ratelimit enable toggle
- Move trace outside of ratelimit
- Split log level (Patch 2) into two
- More descriptive documentation/sysfs naming
v2: https://lore.kernel.org/r/20250214023543.992372-1-pandoh@google.com
- Rebased on top of pci/aer (6.14.rc-1)
- Split series into log and IRQ ratelimits (defer patch 5)
- Dropped patch 8 (Move AER sysfs)
- Added log level cleanup patch[7] from Karolina's series
- Fixed bug where dpc errors didn't increment counters
- "X callbacks suppressed" message on ratelimit release -> immediately
- Separate documentation into own patch
v1: https://lore.kernel.org/r/20250115074301.3514927-1-pandoh@google.com
[1] https://bugzilla.kernel.org/show_bug.cgi?id=215027
[2] https://bugzilla.kernel.org/show_bug.cgi?id=201517
[3] https://bugzilla.kernel.org/show_bug.cgi?id=196183
[4] https://lore.kernel.org/linux-pci/20230606035442.2886343-2-grundler@chromium.org/
[5] https://lore.kernel.org/linux-pci/cover.1736341506.git.karolina.stolarek@oracle.com/
[6]
https://lore.kernel.org/linux-pci/8bcb8c9a7b38ce3bdaca5a64fe76f08b0b337511.1742202797.git.k
arolina.stolarek@oracle.com/
[7]
https://lore.kernel.org/linux-pci/edd77011aafad4c0654358a26b4e538d0c5a321d.1736341506.git.k
arolina.stolarek@oracle.com/
Bjorn Helgaas (9):
PCI/DPC: Initialize aer_err_info before using it
PCI/DPC: Log Error Source ID only when valid
PCI/AER: Consolidate Error Source ID logging in aer_print_port_info()
PCI/AER: Extract bus/dev/fn in aer_print_port_info() with
PCI_BUS_NUM(), etc
PCI/AER: Move aer_print_source() earlier in file
PCI/AER: Initialize aer_err_info before using it
PCI/AER: Simplify pci_print_aer()
PCI/AER: Update statistics early in logging
PCI/AER: Combine trace_aer_event() with statistics updates
Jon Pan-Doh (4):
PCI/AER: Rename aer_print_port_info() to aer_print_source()
PCI/AER: Introduce ratelimit for error logs
PCI/AER: Add ratelimits to PCI AER Documentation
PCI/AER: Add sysfs attributes for log ratelimits
Karolina Stolarek (3):
PCI/AER: Check log level once and remember it
PCI/AER: Make all pci_print_aer() log levels depend on error type
PCI/AER: Rename struct aer_stats to aer_report
...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 ++
Documentation/PCI/pcieaer-howto.rst | 16 +-
drivers/pci/pci-sysfs.c | 1 +
drivers/pci/pci.h | 5 +-
drivers/pci/pcie/aer.c | 346 ++++++++++++------
drivers/pci/pcie/dpc.c | 49 ++-
include/linux/pci.h | 2 +-
7 files changed, 329 insertions(+), 124 deletions(-)
rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)
--
2.43.0
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "info" at declaration so it starts as all zeroes.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Bjorn Helgaas <bhelgaas@google.com>
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 45 ++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 17 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Previously we decoded the AER Error Source ID in two places. Consolidate
them so both places use aer_print_port_info(). Add a "details" parameter
so we can add a note when we didn't find any downstream devices with errors
logged in their AER Capability.
When we didn't read any error details from the source device, we logged two
messages: one in aer_isr_one_error() and another in find_source_device().
Since they both contain the same information, only log the first one when
when find_source_device() has found error details.
This changes the dmesg logging when we found no devices with errors logged:
- pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0
- pci 0000:00:01.0: AER: found no error details for 0000:02:00.0
+ pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0 (no details found)
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Use PCI_BUS_NUM(), PCI_SLOT(), PCI_FUNC() to extract the bus number,
device, and function number directly from the Error Source ID. There's no
need to shift and mask it explicitly.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Jon Pan-Doh <redacted>
Rename aer_print_port_info() to aer_print_source() to be more descriptive.
This logs the Error Source ID logged by a Root Port or Root Complex Event
Collector when it receives an ERR_COR, ERR_NONFATAL, or ERR_FATAL Message.
[bhelgaas: aer_print_rp_info() -> aer_print_source()]
Link: https://lore.kernel.org/r/20250321015806.954866-5-pandoh@google.com
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Move aer_print_source() earlier in the file so a future change can use it
from aer_print_error(), where it's easier to rate limit it.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "e_info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "e_info" at declaration with a designated initializer list,
which initializes the other members to zero.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
Simplify pci_print_aer() by initializing the struct aer_err_info "info"
with a designated initializer list (it was previously initialized with
memset()) and using pci_name().
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: Bjorn Helgaas <bhelgaas@google.com>
There are two AER logging entry points:
- aer_print_error() is used by DPC (dpc_process_error()) and native AER
handling (aer_process_err_devices()).
- pci_print_aer() is used by GHES (aer_recover_work_func()) and CXL
(cxl_handle_rdport_errors())
Both use __aer_print_error() to print the AER error bits. Previously
__aer_print_error() also incremented the AER statistics via
pci_dev_aer_stats_incr().
Call pci_dev_aer_stats_incr() early in the entry points instead of in
__aer_print_error() so we update the statistics even if the actual printing
of error bits is rate limited by a future change.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Bjorn Helgaas <bhelgaas@google.com>
As with the AER statistics, we always want to emit trace events, even if
the actual dmesg logging is rate limited.
Call trace_aer_event() directly from pci_dev_aer_stats_incr(), where we
update the statistics.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Karolina Stolarek <redacted>
When reporting an AER error, we check its type multiple times to determine
the log level for each message. Do this check only in the top-level
functions (aer_isr_one_error(), pci_print_aer()) and save the level in
struct aer_err_info.
[bhelgaas: save log level in struct aer_err_info instead of passing it
as a parameter]
Link: https://lore.kernel.org/r/20250321015806.954866-2-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.h | 1 +
drivers/pci/pcie/aer.c | 21 ++++++++++-----------
drivers/pci/pcie/dpc.c | 1 +
3 files changed, 12 insertions(+), 11 deletions(-)
From: Karolina Stolarek <redacted>
Some existing logs in pci_print_aer() log with error severity by default.
Convert them to depend on error type (consistent with rest of AER logging).
Link: https://lore.kernel.org/r/20250321015806.954866-3-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
@@ -54,11 +54,11 @@ struct aer_rpc {DECLARE_KFIFO(aer_fifo,structaer_err_source,AER_ERROR_SOURCES_MAX);};-/* AER stats for the device */-structaer_stats{+/* AER report for the device */+structaer_report{/*-*FieldsforallAERcapabledevices.Theyindicatetheerrors+*StatsforallAERcapabledevices.Theyindicatetheerrors*"as seen by this device".Notethatthismaymeanthatifan*Endpointiscausingproblems,theAERcountersmayincrement*atitslinkpartner(e.g.RootPort)becausetheerrorswillbe
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
The GHES and CXL pci_print_aer() cases are ratelimited by the Error Source
device.
Sargun at Meta reported internally that a flood of AER errors causes RCU
CPU stall warnings and CSD-lock warnings.
Tested using aer-inject[1]. Sent 11 AER errors. Observed 10 errors logged
while AER stats (cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) show
true count of 11.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
[bhelgaas: commit log, factor out trace_aer_event() and aer_print_rp_info()
changes to previous patches, collect single aer_err_info.ratelimit as union
of ratelimits of all error source devices]
Link: https://lore.kernel.org/r/20250321015806.954866-7-pandoh@google.com
Reported-by: Sargun Dhillon <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.h | 3 ++-
drivers/pci/pcie/aer.c | 49 ++++++++++++++++++++++++++++++++++++------
drivers/pci/pcie/dpc.c | 1 +
3 files changed, 46 insertions(+), 7 deletions(-)
@@ -85,6 +85,17 @@ In the example, 'Requester ID' means the ID of the device that sent the error message to the Root Port. Please refer to PCIe specs for other fields.+AER Ratelimits+--------------++Since error messages can be generated for each transaction, we may see+large volumes of errors reported. To prevent spammy devices from flooding+the console/stalling execution, messages are throttled by device and error+type (correctable vs. uncorrectable).++AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over+DEFAULT_RATELIMIT_INTERVAL (5 seconds).+ AER Statistics / Counters -------------------------
From: Jon Pan-Doh <redacted>
Allow userspace to read/write log ratelimits per device (including
enable/disable). Create aer/ sysfs directory to store them and any
future aer configs.
Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
attributes (e.g. stats and ratelimits).
Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
sysfs-bus-pci-devices-aer
Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
Sent 6 AER errors. Observed 5 errors logged while AER stats
(cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.
Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
logged and accounted in AER stats (12 total errors).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 +++++++
Documentation/PCI/pcieaer-howto.rst | 5 +-
drivers/pci/pci-sysfs.c | 1 +
drivers/pci/pci.h | 1 +
drivers/pci/pcie/aer.c | 99 +++++++++++++++++++
5 files changed, 139 insertions(+), 1 deletion(-)
rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats b/Documentation/ABI/testing/sysfs-bus-pci-devices-aersimilarity index 77%rename from Documentation/ABI/testing/sysfs-bus-pci-devices-aer_statsrename to Documentation/ABI/testing/sysfs-bus-pci-devices-aerindex d1f67bb81d5d..771204197b71 100644--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
@@ -117,3 +117,37 @@ Date: July 2018 KernelVersion: 4.19.0 Contact: linux-pci@vger.kernel.org, rajatja@google.com Description: Total number of ERR_NONFATAL messages reported to rootport.++PCIe AER ratelimits+-------------------++These attributes show up under all the devices that are AER capable.+They represent configurable ratelimits of logs per error type.++See Documentation/PCI/pcieaer-howto.rst for more info on ratelimits.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_log_enable+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Writing 1/0 enables/disables AER log ratelimiting. Reading+ gets whether or not AER is currently enabled. Enabled by+ default.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_cor_log+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Ratelimit burst for correctable error logs. Writing a value+ changes the number of errors (burst) allowed per interval+ (5 second window) before ratelimiting. Reading gets the+ current ratelimit burst.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_uncor_log+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Ratelimit burst for uncorrectable error logs. Writing a+ value changes the number of errors (burst) allowed per+ interval (5 second window) before ratelimiting. Reading+ gets the current ratelimit burst.
@@ -96,12 +96,15 @@ type (correctable vs. uncorrectable). AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over DEFAULT_RATELIMIT_INTERVAL (5 seconds).+Ratelimits are exposed in the form of sysfs attributes and configurable.+See Documentation/ABI/testing/sysfs-bus-pci-devices-aer.+ AER Statistics / Counters ------------------------- When PCIe AER errors are captured, the counters / statistics are also exposed in the form of sysfs attributes which are documented at-Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+Documentation/ABI/testing/sysfs-bus-pci-devices-aer. Developer Guide ===============
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "info" was allocated on the stack
/s/Previously/Currently ?
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "info" at declaration so it starts as all zeroes.
From: Bjorn Helgaas <bhelgaas@google.com>
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Since we are handling Uncorrectable errors, why not use pci_err?
From: Bjorn Helgaas <bhelgaas@google.com>
Previously we decoded the AER Error Source ID in two places. Consolidate
them so both places use aer_print_port_info(). Add a "details" parameter
so we can add a note when we didn't find any downstream devices with errors
logged in their AER Capability.
When we didn't read any error details from the source device, we logged two
messages: one in aer_isr_one_error() and another in find_source_device().
Since they both contain the same information, only log the first one when
when find_source_device() has found error details.
/s/when//
quoted hunk
This changes the dmesg logging when we found no devices with errors logged:
- pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0
- pci 0000:00:01.0: AER: found no error details for 0000:02:00.0
+ pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0 (no details found)
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
@@ -926,13 +927,13 @@ static bool find_source_device(struct pci_dev *parent, else pci_walk_bus(parent->subordinate, find_device_iter, e_info);+ /*+ * If we didn't find any devices with errors logged in the AER+ * Capability, just print the Error Source ID from the Root Port or+ * RCEC that received an ERR_* Message.+ */ if (!e_info->error_dev_num) {- u8 bus = e_info->id >> 8;- u8 devfn = e_info->id & 0xff;-- pci_info(parent, "found no error details for %04x:%02x:%02x.%d\n",- pci_domain_nr(parent->bus), bus, PCI_SLOT(devfn),- PCI_FUNC(devfn));+ aer_print_port_info(parent, e_info, " (no details found)"); return false; } return true;
Instead of printing the error information in find_source_device() (a helper function), I think it be better to print it here (the error handler). source_found = find_source_device(pdev, &e_info); aer_print_port_info(pdev, &e_info, source_found? "" : "(no details found) " );
if (source_found) aer_process_err_devices(&e_info)
quoted hunk
- if (find_source_device(pdev, &e_info))
+ if (find_source_device(pdev, &e_info)) {
+ aer_print_port_info(pdev, &e_info, "");
aer_process_err_devices(&e_info);
+ }
}
if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
From: Bjorn Helgaas <bhelgaas@google.com>
Use PCI_BUS_NUM(), PCI_SLOT(), PCI_FUNC() to extract the bus number,
device, and function number directly from the Error Source ID. There's no
need to shift and mask it explicitly.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Jon Pan-Doh <redacted>
Rename aer_print_port_info() to aer_print_source() to be more descriptive.
This logs the Error Source ID logged by a Root Port or Root Complex Event
Collector when it receives an ERR_COR, ERR_NONFATAL, or ERR_FATAL Message.
[bhelgaas: aer_print_rp_info() -> aer_print_source()]
Link: https://lore.kernel.org/r/20250321015806.954866-5-pandoh@google.com
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Bjorn Helgaas <bhelgaas@google.com>
Move aer_print_source() earlier in the file so a future change can use it
from aer_print_error(), where it's easier to rate limit it.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "e_info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "e_info" at declaration with a designated initializer list,
which initializes the other members to zero.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Bjorn Helgaas <bhelgaas@google.com>
Simplify pci_print_aer() by initializing the struct aer_err_info "info"
with a designated initializer list (it was previously initialized with
memset()) and using pci_name().
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -765,7 +765,10 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,{intlayer,agent,tlp_header_valid=0;u32status,mask;-structaer_err_infoinfo;
You have cleaned up other stack allocations of struct aer_err_info to zero
initialization in your previous patches. Why not follow the same format
here? I don't think this function resets all fields of aer_err_info, right?
quoted hunk
+ struct aer_err_info info = {
+ .severity = aer_severity,
+ .first_error = PCI_ERR_CAP_FEP(aer->cap_control),
+ };
if (aer_severity == AER_CORRECTABLE) {
status = aer->cor_status;
From: Bjorn Helgaas <bhelgaas@google.com>
There are two AER logging entry points:
- aer_print_error() is used by DPC (dpc_process_error()) and native AER
handling (aer_process_err_devices()).
- pci_print_aer() is used by GHES (aer_recover_work_func()) and CXL
(cxl_handle_rdport_errors())
Both use __aer_print_error() to print the AER error bits. Previously
__aer_print_error() also incremented the AER statistics via
pci_dev_aer_stats_incr().
Call pci_dev_aer_stats_incr() early in the entry points instead of in
__aer_print_error() so we update the statistics even if the actual printing
of error bits is rate limited by a future change.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Bjorn Helgaas <bhelgaas@google.com>
As with the AER statistics, we always want to emit trace events, even if
the actual dmesg logging is rate limited.
Call trace_aer_event() directly from pci_dev_aer_stats_incr(), where we
update the statistics.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
@@ -741,9 +744,6 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)out:if(info->id&&info->error_dev_num>1&&info->id==id)pci_err(dev," Error of this Agent is reported first\n");--trace_aer_event(dev_name(&dev->dev),(info->status&~info->mask),-info->severity,info->tlp_header_valid,&info->tlp);}#ifdef CONFIG_ACPI_APEI_PCIEAER
@@ -782,6 +782,9 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,info.status=status;info.mask=mask;+info.tlp_header_valid=tlp_header_valid;+if(tlp_header_valid)
I think you can skip this check. The trace call checks for valid flag before accessing
the tlp buffer. If you want to keep it, try to set it to NULL for !tlp_header_valid case.
From: Karolina Stolarek <redacted>
When reporting an AER error, we check its type multiple times to determine
the log level for each message. Do this check only in the top-level
functions (aer_isr_one_error(), pci_print_aer()) and save the level in
struct aer_err_info.
[bhelgaas: save log level in struct aer_err_info instead of passing it
as a parameter]
Link: https://lore.kernel.org/r/20250321015806.954866-2-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Karolina Stolarek <redacted>
Some existing logs in pci_print_aer() log with error severity by default.
Convert them to depend on error type (consistent with rest of AER logging).
Link: https://lore.kernel.org/r/20250321015806.954866-3-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
From: Karolina Stolarek <redacted>
Update name to reflect the broader definition of structs/variables that are
stored (e.g. ratelimits). This is a preparatory patch for adding rate limit
support.
Link: https://lore.kernel.org/r/20250321015806.954866-6-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
@@ -54,11 +54,11 @@ struct aer_rpc {DECLARE_KFIFO(aer_fifo,structaer_err_source,AER_ERROR_SOURCES_MAX);};-/* AER stats for the device */-structaer_stats{+/* AER report for the device */+structaer_report{
For me aer_report also sounds like stats like struct. I prefer aer_info, but
it is up to you.
quoted hunk
/*
- * Fields for all AER capable devices. They indicate the errors
+ * Stats for all AER capable devices. They indicate the errors
* "as seen by this device". Note that this may mean that if an
* Endpoint is causing problems, the AER counters may increment
* at its link partner (e.g. Root Port) because the errors will be
@@ -80,7 +80,7 @@ struct aer_stats { u64 dev_total_nonfatal_errs; /*- * Fields for Root Ports & Root Complex Event Collectors only; these+ * Stats for Root Ports & Root Complex Event Collectors only; these * indicate the total number of ERR_COR, ERR_FATAL, and ERR_NONFATAL * messages received by the Root Port / Event Collector, INCLUDING the * ones that are generated internally (by the Root Port itself)
Hi Bjorn,
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
Can we also not rate limit fatal errors in AER driver?
quoted hunk
The GHES and CXL pci_print_aer() cases are ratelimited by the Error Source
device.
Sargun at Meta reported internally that a flood of AER errors causes RCU
CPU stall warnings and CSD-lock warnings.
Tested using aer-inject[1]. Sent 11 AER errors. Observed 10 errors logged
while AER stats (cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) show
true count of 11.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
[bhelgaas: commit log, factor out trace_aer_event() and aer_print_rp_info()
changes to previous patches, collect single aer_err_info.ratelimit as union
of ratelimits of all error source devices]
Link: https://lore.kernel.org/r/20250321015806.954866-7-pandoh@google.com
Reported-by: Sargun Dhillon <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.h | 3 ++-
drivers/pci/pcie/aer.c | 49 ++++++++++++++++++++++++++++++++++++------
drivers/pci/pcie/dpc.c | 1 +
3 files changed, 46 insertions(+), 7 deletions(-)
@@ -85,6 +85,17 @@ In the example, 'Requester ID' means the ID of the device that sent the error message to the Root Port. Please refer to PCIe specs for other fields.+AER Ratelimits+--------------++Since error messages can be generated for each transaction, we may see+large volumes of errors reported. To prevent spammy devices from flooding+the console/stalling execution, messages are throttled by device and error+type (correctable vs. uncorrectable).
Can we list exceptions like DPC and FATAL errors (if added) ?
+
+AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over
+DEFAULT_RATELIMIT_INTERVAL (5 seconds).
+
AER Statistics / Counters
-------------------------
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
From: Jon Pan-Doh <redacted>
Allow userspace to read/write log ratelimits per device (including
enable/disable). Create aer/ sysfs directory to store them and any
future aer configs.
Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
attributes (e.g. stats and ratelimits).
Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
sysfs-bus-pci-devices-aer
Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
Sent 6 AER errors. Observed 5 errors logged while AER stats
(cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.
Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
logged and accounted in AER stats (12 total errors).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats b/Documentation/ABI/testing/sysfs-bus-pci-devices-aersimilarity index 77%rename from Documentation/ABI/testing/sysfs-bus-pci-devices-aer_statsrename to Documentation/ABI/testing/sysfs-bus-pci-devices-aerindex d1f67bb81d5d..771204197b71 100644--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
@@ -117,3 +117,37 @@ Date: July 2018 KernelVersion: 4.19.0 Contact: linux-pci@vger.kernel.org, rajatja@google.com Description: Total number of ERR_NONFATAL messages reported to rootport.++PCIe AER ratelimits+-------------------++These attributes show up under all the devices that are AER capable.+They represent configurable ratelimits of logs per error type.++See Documentation/PCI/pcieaer-howto.rst for more info on ratelimits.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_log_enable+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Writing 1/0 enables/disables AER log ratelimiting. Reading+ gets whether or not AER is currently enabled. Enabled by+ default.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_cor_log+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Ratelimit burst for correctable error logs. Writing a value+ changes the number of errors (burst) allowed per interval+ (5 second window) before ratelimiting. Reading gets the+ current ratelimit burst.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_uncor_log+Date: March 2025+KernelVersion: 6.15.0+Contact: linux-pci@vger.kernel.org, pandoh@google.com+Description: Ratelimit burst for uncorrectable error logs. Writing a+ value changes the number of errors (burst) allowed per+ interval (5 second window) before ratelimiting. Reading+ gets the current ratelimit burst.
@@ -96,12 +96,15 @@ type (correctable vs. uncorrectable). AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over DEFAULT_RATELIMIT_INTERVAL (5 seconds).+Ratelimits are exposed in the form of sysfs attributes and configurable.+See Documentation/ABI/testing/sysfs-bus-pci-devices-aer.+ AER Statistics / Counters ------------------------- When PCIe AER errors are captured, the counters / statistics are also exposed in the form of sysfs attributes which are documented at-Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+Documentation/ABI/testing/sysfs-bus-pci-devices-aer. Developer Guide ===============
From: Krzysztof Wilczyński <hidden> Date: 2025-05-20 09:05:14
Hello,
This work is mostly due to Jon Pan-Doh and Karolina Stolarek. I rebased
this to v6.15-rc1, factored out some of the trace and statistics updates,
and added some minor cleanups.
Proposal
========
When using native AER, spammy devices can flood kernel logs with AER errors
and slow/stall execution. Add per-device per-error-severity ratelimits for
more robust error logging. Allow userspace to configure ratelimits via
sysfs knobs.
Motivation
==========
Inconsistent PCIe error handling, exacerbated at datacenter scale (myriad
of devices), affects repairabilitiy flows for fleet operators.
Exposing PCIe errors/debug info in-band for a userspace daemon (e.g.
rasdaemon) to collect/pass on to repairability services will allow for more
predictable repair flows and decrease machine downtime.
Background
==========
AER error spam has been observed many times, both publicly (e.g. [1], [2],
[3]) and privately. While it usually occurs with correctable errors, it can
happen with uncorrectable errors (e.g. during new HW bringup).
There have been previous attempts to add ratelimits to AER logs ([4], [5]).
The most recent attempt[5] has many similarities with the proposed
approach.
I have been testing this series locally with and without faults triggered
using the AER error injection facility. No issues thus far.
And, as such...
Tested-by: Krzysztof Wilczyński <kwilczynski@kernel.org>
Thank you!
Krzysztof
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 09:39:47
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "info" at declaration so it starts as all zeroes.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:28:13
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 45 ++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 17 deletions(-)
@@ -261,25 +261,36 @@ void dpc_process_error(struct pci_dev *pdev)structaer_err_infoinfo={0};pci_read_config_word(pdev,cap+PCI_EXP_DPC_STATUS,&status);-pci_read_config_word(pdev,cap+PCI_EXP_DPC_SOURCE_ID,&source);--pci_info(pdev,"containment event, status:%#06x source:%#06x\n",-status,source);reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN;-ext_reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;-pci_warn(pdev,"%s detected\n",-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR)?-"unmasked uncorrectable error":-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)?-"ERR_NONFATAL":-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE)?-"ERR_FATAL":-(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)?-"RP PIO error":-(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER)?-"software trigger":-"reserved error");++switch(reason){+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR:+pci_warn(pdev,"containment event, status:%#06x: unmasked uncorrectable error detected\n",+status);+break;+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE:+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_FE:+pci_read_config_word(pdev,cap+PCI_EXP_DPC_SOURCE_ID,+&source);+pci_warn(pdev,"containment event, status:%#06x, %s received from %04x:%02x:%02x.%d\n",+status,+(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE)?+"ERR_FATAL":"ERR_NONFATAL",+pci_domain_nr(pdev->bus),PCI_BUS_NUM(source),+PCI_SLOT(source),PCI_FUNC(source));+return;+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT:+ext_reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;+pci_warn(pdev,"containment event, status:%#06x: %s detected\n",+status,+(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)?+"RP PIO error":+(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER)?+"software trigger":+"reserved error");+break;+}/* show RP PIO error detail information */if(pdev->dpc_rp_extensions&&
After adding that switch (reason) there, wouldn't it make sense to move
also the code from the if blocks into the case blocks? That if
conditions check for reason anyway so those if branches would naturally
belong under one of the cases each.
--
i.
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:32:04
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Previously we decoded the AER Error Source ID in two places. Consolidate
them so both places use aer_print_port_info(). Add a "details" parameter
so we can add a note when we didn't find any downstream devices with errors
logged in their AER Capability.
When we didn't read any error details from the source device, we logged two
messages: one in aer_isr_one_error() and another in find_source_device().
Since they both contain the same information, only log the first one when
when find_source_device() has found error details.
This changes the dmesg logging when we found no devices with errors logged:
- pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0
- pci 0000:00:01.0: AER: found no error details for 0000:02:00.0
+ pci 0000:00:01.0: AER: Correctable error message received from 0000:02:00.0 (no details found)
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:32:44
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Use PCI_BUS_NUM(), PCI_SLOT(), PCI_FUNC() to extract the bus number,
device, and function number directly from the Error Source ID. There's no
need to shift and mask it explicitly.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:33:33
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Jon Pan-Doh <redacted>
Rename aer_print_port_info() to aer_print_source() to be more descriptive.
This logs the Error Source ID logged by a Root Port or Root Complex Event
Collector when it receives an ERR_COR, ERR_NONFATAL, or ERR_FATAL Message.
[bhelgaas: aer_print_rp_info() -> aer_print_source()]
Link: https://lore.kernel.org/r/20250321015806.954866-5-pandoh@google.com
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:35:08
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Move aer_print_source() earlier in the file so a future change can use it
from aer_print_error(), where it's easier to rate limit it.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:39:16
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "e_info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "e_info" at declaration with a designated initializer list,
which initializes the other members to zero.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 10:42:46
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
Simplify pci_print_aer() by initializing the struct aer_err_info "info"
with a designated initializer list (it was previously initialized with
memset()) and using pci_name().
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 11:04:49
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
There are two AER logging entry points:
- aer_print_error() is used by DPC (dpc_process_error()) and native AER
handling (aer_process_err_devices()).
- pci_print_aer() is used by GHES (aer_recover_work_func()) and CXL
(cxl_handle_rdport_errors())
Both use __aer_print_error() to print the AER error bits. Previously
__aer_print_error() also incremented the AER statistics via
pci_dev_aer_stats_incr().
Call pci_dev_aer_stats_incr() early in the entry points instead of in
__aer_print_error() so we update the statistics even if the actual printing
of error bits is rate limited by a future change.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 11:08:30
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Bjorn Helgaas <bhelgaas@google.com>
As with the AER statistics, we always want to emit trace events, even if
the actual dmesg logging is rate limited.
Call trace_aer_event() directly from pci_dev_aer_stats_incr(), where we
update the statistics.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 11:26:16
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Karolina Stolarek <redacted>
When reporting an AER error, we check its type multiple times to determine
the log level for each message. Do this check only in the top-level
functions (aer_isr_one_error(), pci_print_aer()) and save the level in
struct aer_err_info.
[bhelgaas: save log level in struct aer_err_info instead of passing it
as a parameter]
Link: https://lore.kernel.org/r/20250321015806.954866-2-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.h | 1 +
drivers/pci/pcie/aer.c | 21 ++++++++++-----------
drivers/pci/pcie/dpc.c | 1 +
3 files changed, 12 insertions(+), 11 deletions(-)
As a general direction, wouldn't it be better to start adding these
comments in the kerneldoc compatible format (even if not yet enabling the
kerneldoc with /**)?
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 11:37:45
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Karolina Stolarek <redacted>
Some existing logs in pci_print_aer() log with error severity by default.
Convert them to depend on error type (consistent with rest of AER logging).
Link: https://lore.kernel.org/r/20250321015806.954866-3-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
There's another similar callsite but only this has the comment added. I
was thinking if this call could be made from __aer_print_error(). There
would be small change in order of messages but I can't seem to decide if
it would be bad/good.
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
@@ -54,11 +54,11 @@ struct aer_rpc {DECLARE_KFIFO(aer_fifo,structaer_err_source,AER_ERROR_SOURCES_MAX);};-/* AER stats for the device */-structaer_stats{+/* AER report for the device */+structaer_report{/*-*FieldsforallAERcapabledevices.Theyindicatetheerrors+*StatsforallAERcapabledevices.Theyindicatetheerrors*"as seen by this device".Notethatthismaymeanthatifan*Endpointiscausingproblems,theAERcountersmayincrement*atitslinkpartner(e.g.RootPort)becausetheerrorswillbe
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 11:55:43
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
The GHES and CXL pci_print_aer() cases are ratelimited by the Error Source
device.
Sargun at Meta reported internally that a flood of AER errors causes RCU
CPU stall warnings and CSD-lock warnings.
Tested using aer-inject[1]. Sent 11 AER errors. Observed 10 errors logged
while AER stats (cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) show
true count of 11.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
[bhelgaas: commit log, factor out trace_aer_event() and aer_print_rp_info()
changes to previous patches, collect single aer_err_info.ratelimit as union
of ratelimits of all error source devices]
Link: https://lore.kernel.org/r/20250321015806.954866-7-pandoh@google.com
Reported-by: Sargun Dhillon <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.h | 3 ++-
drivers/pci/pcie/aer.c | 49 ++++++++++++++++++++++++++++++++++++------
drivers/pci/pcie/dpc.c | 1 +
3 files changed, 46 insertions(+), 7 deletions(-)
I can see the code uses OR ;-) but I wasn't helpful because this comment
didn't explain why at all. As this ratelimit thing is using reverse logic
to begin with, this is a very tricky bit.
Perhaps something less vague like:
... we ratelimit if all devices have reached their ratelimit.
Assuming that was the intention here? (I'm not sure.)
@@ -914,7 +948,7 @@ static int find_device_iter(struct pci_dev *dev, void *data) * e_info->error_dev_num and e_info->dev[], based on the given information. */ static bool find_source_device(struct pci_dev *parent,- struct aer_err_info *e_info)+ struct aer_err_info *e_info) { struct pci_dev *dev = parent; int result;
@@ -935,10 +969,12 @@ static bool find_source_device(struct pci_dev *parent, /* * If we didn't find any devices with errors logged in the AER * Capability, just print the Error Source ID from the Root Port or- * RCEC that received an ERR_* Message.+ * RCEC that received an ERR_* Message, ratelimited by the RP or+ * RCEC. */ if (!e_info->error_dev_num) {- aer_print_source(parent, e_info, " (no details found)");+ if (aer_ratelimit(parent, e_info->severity))+ aer_print_source(parent, e_info, " (no details found)"); return false; } return true;
@@ -1147,9 +1183,10 @@ static void aer_recover_work_func(struct work_struct *work) pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus, entry.devfn); if (!pdev) {- pr_err("no pci_dev for %04x:%02x:%02x.%x\n",- entry.domain, entry.bus,- PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));+ pr_err_ratelimited("%04x:%02x:%02x.%x: no pci_dev found\n",
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 12:02:24
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted hunk
From: Jon Pan-Doh <redacted>
Allow userspace to read/write log ratelimits per device (including
enable/disable). Create aer/ sysfs directory to store them and any
future aer configs.
Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
attributes (e.g. stats and ratelimits).
Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
sysfs-bus-pci-devices-aer
Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
Sent 6 AER errors. Observed 5 errors logged while AER stats
(cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.
Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
logged and accounted in AER stats (12 total errors).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 +++++++
Documentation/PCI/pcieaer-howto.rst | 5 +-
drivers/pci/pci-sysfs.c | 1 +
drivers/pci/pci.h | 1 +
drivers/pci/pcie/aer.c | 99 +++++++++++++++++++
5 files changed, 139 insertions(+), 1 deletion(-)
rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats b/Documentation/ABI/testing/sysfs-bus-pci-devices-aersimilarity index 77%rename from Documentation/ABI/testing/sysfs-bus-pci-devices-aer_statsrename to Documentation/ABI/testing/sysfs-bus-pci-devices-aerindex d1f67bb81d5d..771204197b71 100644--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
@@ -117,3 +117,37 @@ Date: July 2018 KernelVersion: 4.19.0 Contact: linux-pci@vger.kernel.org, rajatja@google.com Description: Total number of ERR_NONFATAL messages reported to rootport.++PCIe AER ratelimits+-------------------++These attributes show up under all the devices that are AER capable.+They represent configurable ratelimits of logs per error type.++See Documentation/PCI/pcieaer-howto.rst for more info on ratelimits.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_log_enable+Date: March 2025+KernelVersion: 6.15.0
This ship has sailed.
+Contact: linux-pci@vger.kernel.org, pandoh@google.com
+Description: Writing 1/0 enables/disables AER log ratelimiting. Reading
+ gets whether or not AER is currently enabled.
AER or AER ratelimiting is enabled?
quoted hunk
+ Enabled by
+ default.
+
+What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_cor_log
+Date: March 2025
+KernelVersion: 6.15.0
+Contact: linux-pci@vger.kernel.org, pandoh@google.com
+Description: Ratelimit burst for correctable error logs. Writing a value
+ changes the number of errors (burst) allowed per interval
+ (5 second window) before ratelimiting. Reading gets the
+ current ratelimit burst.
+
+What: /sys/bus/pci/devices/<dev>/aer/ratelimit_burst_uncor_log
+Date: March 2025
+KernelVersion: 6.15.0
+Contact: linux-pci@vger.kernel.org, pandoh@google.com
+Description: Ratelimit burst for uncorrectable error logs. Writing a
+ value changes the number of errors (burst) allowed per
+ interval (5 second window) before ratelimiting. Reading
+ gets the current ratelimit burst.
@@ -96,12 +96,15 @@ type (correctable vs. uncorrectable). AER uses the default ratelimit of DEFAULT_RATELIMIT_BURST (10 events) over DEFAULT_RATELIMIT_INTERVAL (5 seconds).+Ratelimits are exposed in the form of sysfs attributes and configurable.+See Documentation/ABI/testing/sysfs-bus-pci-devices-aer.+ AER Statistics / Counters ------------------------- When PCIe AER errors are captured, the counters / statistics are also exposed in the form of sysfs attributes which are documented at-Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+Documentation/ABI/testing/sysfs-bus-pci-devices-aer. Developer Guide ===============
On Mon, May 19, 2025 at 03:41:50PM -0700, Sathyanarayanan Kuppuswamy wrote:
Hi,
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "info" was allocated on the stack
/s/Previously/Currently ?
I prefer "previously" here because it clearly refers to the situation
*before* this patch (allocated on stack without initialization), and
it also gives a hint that this situation is what the patch changes.
If I used "currently," I could be mentioning something relevant that
isn't being changed by the patch, e.g., "currently the struct is
allocated on the stack so it's important to keep it small."
quoted
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "info" at declaration so it starts as all zeroes.
On Tue, May 20, 2025 at 12:39:18PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "info" at declaration so it starts as all zeroes.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Mon, May 19, 2025 at 04:15:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Since we are handling Uncorrectable errors, why not use pci_err?
Sounds reasonable to me. I would do it in a separate patch because
the point of this one is to avoid logging junk when Error Source ID is
not valid.
quoted
+ pci_warn(pdev, "containment event, status:%#06x, %s received from %04x:%02x:%02x.%d\n",
+ status,
I see the BDF extraction and format code in many places in the PCI
drivers. May be a common macro will make it more readable.
Good idea. Not sure how to implement it, so I put that on my TODO
list for now.
On Tue, May 20, 2025 at 01:28:02PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/dpc.c | 45 ++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 17 deletions(-)
@@ -261,25 +261,36 @@ void dpc_process_error(struct pci_dev *pdev)structaer_err_infoinfo={0};pci_read_config_word(pdev,cap+PCI_EXP_DPC_STATUS,&status);-pci_read_config_word(pdev,cap+PCI_EXP_DPC_SOURCE_ID,&source);--pci_info(pdev,"containment event, status:%#06x source:%#06x\n",-status,source);reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN;-ext_reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;-pci_warn(pdev,"%s detected\n",-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR)?-"unmasked uncorrectable error":-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)?-"ERR_NONFATAL":-(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE)?-"ERR_FATAL":-(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)?-"RP PIO error":-(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER)?-"software trigger":-"reserved error");++switch(reason){+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_UNCOR:+pci_warn(pdev,"containment event, status:%#06x: unmasked uncorrectable error detected\n",+status);+break;+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE:+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_FE:+pci_read_config_word(pdev,cap+PCI_EXP_DPC_SOURCE_ID,+&source);+pci_warn(pdev,"containment event, status:%#06x, %s received from %04x:%02x:%02x.%d\n",+status,+(reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE)?+"ERR_FATAL":"ERR_NONFATAL",+pci_domain_nr(pdev->bus),PCI_BUS_NUM(source),+PCI_SLOT(source),PCI_FUNC(source));+return;+casePCI_EXP_DPC_STATUS_TRIGGER_RSN_IN_EXT:+ext_reason=status&PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT;+pci_warn(pdev,"containment event, status:%#06x: %s detected\n",+status,+(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_RP_PIO)?+"RP PIO error":+(ext_reason==PCI_EXP_DPC_STATUS_TRIGGER_RSN_SW_TRIGGER)?+"software trigger":+"reserved error");+break;+}/* show RP PIO error detail information */if(pdev->dpc_rp_extensions&&
After adding that switch (reason) there, wouldn't it make sense to move
also the code from the if blocks into the case blocks? That if
conditions check for reason anyway so those if branches would naturally
belong under one of the cases each.
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-20 14:20:46
On Tue, 20 May 2025, Bjorn Helgaas wrote:
On Mon, May 19, 2025 at 04:15:56PM -0700, Sathyanarayanan Kuppuswamy wrote:
quoted
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
DPC Error Source ID is only valid when the DPC Trigger Reason indicates
that DPC was triggered due to reception of an ERR_NONFATAL or ERR_FATAL
Message (PCIe r6.0, sec 7.9.14.5).
When DPC was triggered by ERR_NONFATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_NFE)
or ERR_FATAL (PCI_EXP_DPC_STATUS_TRIGGER_RSN_FE) from a downstream device,
log the Error Source ID (decoded into domain/bus/device/function). Don't
print the source otherwise, since it's not valid.
For DPC trigger due to reception of ERR_NONFATAL or ERR_FATAL, the dmesg
logging changes:
- pci 0000:00:01.0: DPC: containment event, status:0x000d source:0x0200
- pci 0000:00:01.0: DPC: ERR_FATAL detected
+ pci 0000:00:01.0: DPC: containment event, status:0x000d, ERR_FATAL received from 0000:02:00.0
and when DPC triggered for other reasons, where DPC Error Source ID is
undefined, e.g., unmasked uncorrectable error:
- pci 0000:00:01.0: DPC: containment event, status:0x0009 source:0x0200
- pci 0000:00:01.0: DPC: unmasked uncorrectable error detected
+ pci 0000:00:01.0: DPC: containment event, status:0x0009: unmasked uncorrectable error detected
Previously the "containment event" message was at KERN_INFO and the
"%s detected" message was at KERN_WARNING. Now the single message is at
KERN_WARNING.
Since we are handling Uncorrectable errors, why not use pci_err?
Sounds reasonable to me. I would do it in a separate patch because
the point of this one is to avoid logging junk when Error Source ID is
not valid.
quoted
quoted
+ pci_warn(pdev, "containment event, status:%#06x, %s received from %04x:%02x:%02x.%d\n",
+ status,
I see the BDF extraction and format code in many places in the PCI
drivers. May be a common macro will make it more readable.
Good idea. Not sure how to implement it, so I put that on my TODO
list for now.
Instead of macros, it might be worth adding a printf specifier for this.
Together with some flags, it should be possible to cover also the
variations that print less than the full BDF format.
On Mon, May 19, 2025 at 04:39:19PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
Previously we decoded the AER Error Source ID in two places. Consolidate
them so both places use aer_print_port_info(). Add a "details" parameter
so we can add a note when we didn't find any downstream devices with errors
logged in their AER Capability.
When we didn't read any error details from the source device, we logged two
messages: one in aer_isr_one_error() and another in find_source_device().
Since they both contain the same information, only log the first one when
when find_source_device() has found error details.
/s/when//
Fixed, thanks!
quoted
- pci_info(dev, "%s%s error message received from %04x:%02x:%02x.%d\n",
+ pci_info(dev, "%s%s error message received from %04x:%02x:%02x.%d%s\n",
Instead of relying on the callers, why not add a space before details here?
Could, but I don't like adding an extra space at the end of the line
when the caller passes "". The extra space could make the line wrap
unnecessarily.
@@ -926,13 +927,13 @@ static bool find_source_device(struct pci_dev *parent, else pci_walk_bus(parent->subordinate, find_device_iter, e_info);+ /*+ * If we didn't find any devices with errors logged in the AER+ * Capability, just print the Error Source ID from the Root Port or+ * RCEC that received an ERR_* Message.+ */ if (!e_info->error_dev_num) {- u8 bus = e_info->id >> 8;- u8 devfn = e_info->id & 0xff;-- pci_info(parent, "found no error details for %04x:%02x:%02x.%d\n",- pci_domain_nr(parent->bus), bus, PCI_SLOT(devfn),- PCI_FUNC(devfn));+ aer_print_port_info(parent, e_info, " (no details found)"); return false; } return true;
Instead of printing the error information in find_source_device() (a helper function), I think it be better to print it here (the error handler). source_found = find_source_device(pdev, &e_info); aer_print_port_info(pdev, &e_info, source_found? "" : "(no details found) " );
if (source_found) aer_process_err_devices(&e_info)
Great idea, thanks! That looks much nicer.
quoted
- if (find_source_device(pdev, &e_info))
+ if (find_source_device(pdev, &e_info)) {
+ aer_print_port_info(pdev, &e_info, "");
aer_process_err_devices(&e_info);
+ }
}
if (e_src->status & PCI_ERR_ROOT_UNCOR_RCV) {
On Tue, May 20, 2025 at 01:39:06PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
Previously the struct aer_err_info "e_info" was allocated on the stack
without being initialized, so it contained junk except for the fields we
explicitly set later.
Initialize "e_info" at declaration with a designated initializer list,
which initializes the other members to zero.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)
On Mon, May 19, 2025 at 05:02:28PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Bjorn Helgaas <bhelgaas@google.com>
Simplify pci_print_aer() by initializing the struct aer_err_info "info"
with a designated initializer list (it was previously initialized with
memset()) and using pci_name().
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -765,7 +765,10 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,{intlayer,agent,tlp_header_valid=0;u32status,mask;-structaer_err_infoinfo;
You have cleaned up other stack allocations of struct aer_err_info to zero
initialization in your previous patches. Why not follow the same format
here? I don't think this function resets all fields of aer_err_info, right?
This is new to me, but IIUC this does initialize all the fields.
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html says "Omitted
fields are implicitly initialized the same as for objects that have
static storage duration."
quoted
+ struct aer_err_info info = {
+ .severity = aer_severity,
+ .first_error = PCI_ERR_CAP_FEP(aer->cap_control),
+ };
if (aer_severity == AER_CORRECTABLE) {
status = aer->cor_status;
@@ -252,6 +252,7 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,elseinfo->severity=AER_NONFATAL;+info->level=KERN_WARNING;return1;
}
I think the print level should be KERN_ERR for uncorrectable errors.
Yes, thank you, fixed! dpc_get_aer_uncorrect_severity() always sets
info->severity to AER_FATAL or AER_NONFATAL, and aer_print_error()
only uses KERN_WARNING for AER_CORRECTABLE.
On Tue, May 20, 2025 at 02:37:33PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Karolina Stolarek <redacted>
Some existing logs in pci_print_aer() log with error severity by default.
Convert them to depend on error type (consistent with rest of AER logging).
Link: https://lore.kernel.org/r/20250321015806.954866-3-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pcie/aer.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
There's another similar callsite but only this has the comment added. I
was thinking if this call could be made from __aer_print_error(). There
would be small change in order of messages but I can't seem to decide if
it would be bad/good.
I guess the other caller is dpc_process_rp_pio_error(), which uses
pci_err() for other logging, so at least it matches the level used by
pcie_print_tlp_log().
This patch uses info.level to control the message level, and
pcie_print_tlp_log() doesn't look at info.level. I added this comment
to explain why that's OK and the message level happens to match
already. Maybe not super ideal long term.
On Tue, May 20, 2025 at 03:02:06PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Allow userspace to read/write log ratelimits per device (including
enable/disable). Create aer/ sysfs directory to store them and any
future aer configs.
Update AER sysfs ABI filename to reflect the broader scope of AER sysfs
attributes (e.g. stats and ratelimits).
Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats ->
sysfs-bus-pci-devices-aer
Tested using aer-inject[1]. Configured correctable log ratelimit to 5.
Sent 6 AER errors. Observed 5 errors logged while AER stats
(cat /sys/bus/pci/devices/<dev>/aer_dev_correctable) shows 6.
Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors
logged and accounted in AER stats (12 total errors).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-inject.git
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Jon Pan-Doh <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Acked-by: Paul E. McKenney <paulmck@kernel.org>
---
...es-aer_stats => sysfs-bus-pci-devices-aer} | 34 +++++++
Documentation/PCI/pcieaer-howto.rst | 5 +-
drivers/pci/pci-sysfs.c | 1 +
drivers/pci/pci.h | 1 +
drivers/pci/pcie/aer.c | 99 +++++++++++++++++++
5 files changed, 139 insertions(+), 1 deletion(-)
rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats => sysfs-bus-pci-devices-aer} (77%)
diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats b/Documentation/ABI/testing/sysfs-bus-pci-devices-aersimilarity index 77%rename from Documentation/ABI/testing/sysfs-bus-pci-devices-aer_statsrename to Documentation/ABI/testing/sysfs-bus-pci-devices-aerindex d1f67bb81d5d..771204197b71 100644--- a/Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-aer
@@ -117,3 +117,37 @@ Date: July 2018 KernelVersion: 4.19.0 Contact: linux-pci@vger.kernel.org, rajatja@google.com Description: Total number of ERR_NONFATAL messages reported to rootport.++PCIe AER ratelimits+-------------------++These attributes show up under all the devices that are AER capable.+They represent configurable ratelimits of logs per error type.++See Documentation/PCI/pcieaer-howto.rst for more info on ratelimits.++What: /sys/bus/pci/devices/<dev>/aer/ratelimit_log_enable+Date: March 2025+KernelVersion: 6.15.0
This ship has sailed.
Updated to May 2025 and 6.16.0 (I hope :)).
quoted
+Contact: linux-pci@vger.kernel.org, pandoh@google.com
+Description: Writing 1/0 enables/disables AER log ratelimiting. Reading
+ gets whether or not AER is currently enabled.
On Mon, May 19, 2025 at 09:59:29PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
Can we also not rate limit fatal errors in AER driver?
In other words, only rate limit AER_CORRECTABLE and AER_NONFATAL for
AER? Seems plausible to me.
On Mon, May 19, 2025 at 09:59:29PM -0700, Sathyanarayanan Kuppuswamy wrote:
quoted
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
Can we also not rate limit fatal errors in AER driver?
In other words, only rate limit AER_CORRECTABLE and AER_NONFATAL for
AER? Seems plausible to me.
Yes, we might lose important information by rate-limiting FATAL errors. I
believe FATAL errors should be infrequent, so it's reasonable to allow them
through without rate limiting. Once you make this change, please also
update the related SysFS documentation and update code accordingly.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, May 20, 2025 at 02:55:32PM +0300, Ilpo Järvinen wrote:
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
The GHES and CXL pci_print_aer() cases are ratelimited by the Error Source
device.
quoted
static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
{
+ /*
+ * Ratelimit AER log messages. Generally we add the Error Source
+ * device, but there are is_error_source() cases that can result in
+ * multiple devices being added here, so we OR them all together.
I can see the code uses OR ;-) but I wasn't helpful because this comment
didn't explain why at all. As this ratelimit thing is using reverse logic
to begin with, this is a very tricky bit.
Perhaps something less vague like:
... we ratelimit if all devices have reached their ratelimit.
Assuming that was the intention here? (I'm not sure.)
My intention was that if there's any downstream device that has an
unmasked error logged and it has not reached its ratelimit, we should
log messages for all devices with errors logged. Does something like
this help?
/*
* Ratelimit AER log messages. "dev" is either the source
* identified by the root's Error Source ID or it has an unmasked
* error logged in its own AER Capability. If any of these devices
* has not reached its ratelimit, log messages for all of them.
* Messages are emitted when e_info->ratelimit is non-zero.
*
* Note that e_info->ratelimit was already initialized to 1 for the
* ERR_FATAL case.
*/
The ERR_FATAL case is from this post-v6 change that I haven't posted
yet:
aer_isr_one_error(...)
{
...
if (status & PCI_ERR_ROOT_UNCOR_RCV) {
int fatal = status & PCI_ERR_ROOT_FATAL_RCV;
struct aer_err_info e_info = {
...
+ .ratelimit = fatal ? 1 : 0;
@@ -1147,9 +1183,10 @@ static void aer_recover_work_func(struct work_struct *work) pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus, entry.devfn); if (!pdev) {- pr_err("no pci_dev for %04x:%02x:%02x.%x\n",- entry.domain, entry.bus,- PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));+ pr_err_ratelimited("%04x:%02x:%02x.%x: no pci_dev found\n",
This case was not mentioned in the changelog.
Sharp eyes! What do you think of this commit log text?
The CXL pci_print_aer() case is ratelimited by the Error Source device.
The GHES pci_print_aer() case is via aer_recover_work_func(), which
searches for the Error Source device. If the device is not found, there's
no per-device ratelimit, so we use a system-wide ratelimit that covers all
error types (correctable, non-fatal, and fatal).
This isn't really ideal because in pci_print_aer(), the struct
aer_capability_regs has already been filled by firmware and the
logging doesn't read any registers from the device at all.
However, pci_print_aer() *does* want the pci_dev for statistics and
tracing (pci_dev_aer_stats_incr()) and, of course, for the aer_printks
themselves.
We could leave this pr_err() completely alone; hopefully it's a rare
case. I think the CXL path just silently skips pci_print_aer() if
this happens.
Eventually I would really like the native AER path to start by doing
whatever firmware is doing, e.g., fill in struct aer_capability_regs,
so the core of the AER handling could be identical between native AER
and GHES/CXL. If we could do that, maybe we could figure out a
cleaner way to handle this corner case.
On Mon, May 19, 2025 at 10:01:09PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Add ratelimits section for rationale and defaults.
quoted
+AER Ratelimits
+--------------
+
+Since error messages can be generated for each transaction, we may see
+large volumes of errors reported. To prevent spammy devices from flooding
+the console/stalling execution, messages are throttled by device and error
+type (correctable vs. uncorrectable).
Can we list exceptions like DPC and FATAL errors (if added) ?
Like this?
+... messages are throttled by device and error
+type (correctable vs. non-fatal uncorrectable). Fatal errors, including
+DPC errors, are not ratelimited.
DPC is currently only triggered for fatal errors.
On Mon, May 19, 2025 at 10:01:09PM -0700, Sathyanarayanan Kuppuswamy wrote:
quoted
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Add ratelimits section for rationale and defaults.
+AER Ratelimits
+--------------
+
+Since error messages can be generated for each transaction, we may see
+large volumes of errors reported. To prevent spammy devices from flooding
+the console/stalling execution, messages are throttled by device and error
+type (correctable vs. uncorrectable).
Can we list exceptions like DPC and FATAL errors (if added) ?
Like this?
+... messages are throttled by device and error
+type (correctable vs. non-fatal uncorrectable). Fatal errors, including
+DPC errors, are not ratelimited.
DPC is currently only triggered for fatal errors.
Yes. I think it is good enough.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Mon, May 19, 2025 at 08:30:09PM -0700, Sathyanarayanan Kuppuswamy wrote:
On 5/19/25 2:35 PM, Bjorn Helgaas wrote:
quoted
From: Karolina Stolarek <redacted>
Update name to reflect the broader definition of structs/variables that are
stored (e.g. ratelimits). This is a preparatory patch for adding rate limit
support.
Link: https://lore.kernel.org/r/20250321015806.954866-6-pandoh@google.com
Signed-off-by: Karolina Stolarek <redacted>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
@@ -54,11 +54,11 @@ struct aer_rpc {DECLARE_KFIFO(aer_fifo,structaer_err_source,AER_ERROR_SOURCES_MAX);};-/* AER stats for the device */-structaer_stats{+/* AER report for the device */+structaer_report{
For me aer_report also sounds like stats like struct. I prefer
aer_info, but it is up to you.
I tend to agree and can imagine a future where we might collect the
stats, ratelimits, and maybe aer_capability_regs into a per-device AER
structure. "aer_info" seems like a decent generic name, so I did
s/\<aer_stats\>/aer_info/
From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: 2025-05-21 10:04:57
On Tue, 20 May 2025, Bjorn Helgaas wrote:
On Tue, May 20, 2025 at 02:55:32PM +0300, Ilpo Järvinen wrote:
quoted
On Mon, 19 May 2025, Bjorn Helgaas wrote:
quoted
From: Jon Pan-Doh <redacted>
Spammy devices can flood kernel logs with AER errors and slow/stall
execution. Add per-device ratelimits for AER correctable and uncorrectable
errors that use the kernel defaults (10 per 5s).
There are two AER logging entry points:
- aer_print_error() is used by DPC and native AER
- pci_print_aer() is used by GHES and CXL
The native AER aer_print_error() case includes a loop that may log details
from multiple devices. This is ratelimited by the union of ratelimits for
these devices, set by add_error_device(), which collects the devices. If
no such device is found, the Error Source message is ratelimited by the
Root Port or RCEC that received the ERR_* message.
The DPC aer_print_error() case is currently not ratelimited.
The GHES and CXL pci_print_aer() cases are ratelimited by the Error Source
device.
quoted
quoted
static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
{
+ /*
+ * Ratelimit AER log messages. Generally we add the Error Source
+ * device, but there are is_error_source() cases that can result in
+ * multiple devices being added here, so we OR them all together.
I can see the code uses OR ;-) but I wasn't helpful because this comment
didn't explain why at all. As this ratelimit thing is using reverse logic
to begin with, this is a very tricky bit.
Perhaps something less vague like:
... we ratelimit if all devices have reached their ratelimit.
Assuming that was the intention here? (I'm not sure.)
My intention was that if there's any downstream device that has an
unmasked error logged and it has not reached its ratelimit, we should
log messages for all devices with errors logged. Does something like
this help?
/*
* Ratelimit AER log messages. "dev" is either the source
* identified by the root's Error Source ID or it has an unmasked
* error logged in its own AER Capability. If any of these devices
* has not reached its ratelimit, log messages for all of them.
* Messages are emitted when e_info->ratelimit is non-zero.
*
* Note that e_info->ratelimit was already initialized to 1 for the
* ERR_FATAL case.
*/
Yes, this is much clearer of intent, thanks.
The ERR_FATAL case is from this post-v6 change that I haven't posted
yet:
aer_isr_one_error(...)
{
...
if (status & PCI_ERR_ROOT_UNCOR_RCV) {
int fatal = status & PCI_ERR_ROOT_FATAL_RCV;
struct aer_err_info e_info = {
...
+ .ratelimit = fatal ? 1 : 0;
@@ -1147,9 +1183,10 @@ static void aer_recover_work_func(struct work_struct *work) pdev = pci_get_domain_bus_and_slot(entry.domain, entry.bus, entry.devfn); if (!pdev) {- pr_err("no pci_dev for %04x:%02x:%02x.%x\n",- entry.domain, entry.bus,- PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn));+ pr_err_ratelimited("%04x:%02x:%02x.%x: no pci_dev found\n",
This case was not mentioned in the changelog.
Sharp eyes! What do you think of this commit log text?
The CXL pci_print_aer() case is ratelimited by the Error Source device.
The GHES pci_print_aer() case is via aer_recover_work_func(), which
searches for the Error Source device. If the device is not found, there's
no per-device ratelimit, so we use a system-wide ratelimit that covers all
error types (correctable, non-fatal, and fatal).
Works for me as long as it is mentioned.
This isn't really ideal because in pci_print_aer(), the struct
aer_capability_regs has already been filled by firmware and the
logging doesn't read any registers from the device at all.
However, pci_print_aer() *does* want the pci_dev for statistics and
tracing (pci_dev_aer_stats_incr()) and, of course, for the aer_printks
themselves.
While not a perfect solution, this looks yet another case where it would
help to create a dummy pci_dev struct with minimal setup which allows
calling functions that input a pci_dev.
That solution is not perfect because it arms a trap. Downstream
functions could get changed and if the developer assumes they have a full
pci_dev at hand, it could cause issues with the dummy pci_dev. How likely
it happens is debatable but for many cases where the call-chain isn't
overly complex such as here, dummy pci_dev seems helpful.
We could leave this pr_err() completely alone; hopefully it's a rare
case. I think the CXL path just silently skips pci_print_aer() if
this happens.
Eventually I would really like the native AER path to start by doing
whatever firmware is doing, e.g., fill in struct aer_capability_regs,
so the core of the AER handling could be identical between native AER
and GHES/CXL. If we could do that, maybe we could figure out a
cleaner way to handle this corner case.