| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250521231418.GA1454532@bhelgaas>
Date: Wed, 21 May 2025 18:14:18 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: linux-pci@...r.kernel.org
Cc: Jon Pan-Doh <pandoh@...gle.com>,
Karolina Stolarek <karolina.stolarek@...cle.com>,
Weinan Liu <wnliu@...gle.com>,
Martin Petersen <martin.petersen@...cle.com>,
Ben Fuller <ben.fuller@...cle.com>,
Drew Walton <drewwalton@...rosoft.com>,
Anil Agrawal <anilagrawal@...a.com>,
Tony Luck <tony.luck@...el.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
Lukas Wunner <lukas@...ner.de>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Sargun Dhillon <sargun@...a.com>,
"Paul E . McKenney" <paulmck@...nel.org>,
Mahesh J Salgaonkar <mahesh@...ux.ibm.com>,
Oliver O'Halloran <oohall@...il.com>,
Kai-Heng Feng <kaihengf@...dia.com>,
Keith Busch <kbusch@...nel.org>, Robert Richter <rrichter@....com>,
Terry Bowman <terry.bowman@....com>,
Shiju Jose <shiju.jose@...wei.com>,
Dave Jiang <dave.jiang@...el.com>, linux-kernel@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v7 00/17] Rate limit AER logs
On Tue, May 20, 2025 at 04:50:17PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@...gle.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.
>
> I'm sorry to post a v7 so soon after v6, but I really want to get this in
> v6.16 so it needs to get into pci/next soonish. I pushed this to pci/aer
> at https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=aer
> (head cbde036e5615 ("PCI/AER: Add sysfs attributes for log ratelimits"))
> and appended the interdiff from v6 to v7 below.
Thank you ever so much for all your reviewing time. I addressed most
of the comments (except Sathy's comments about the sysfs
cor/uncor/nonfatal names), but I hate to deluge you all with another
full posting.
So for now I updated the "aer" branch above (current head d41e0decb7d7
("PCI/AER: Add sysfs attributes for log ratelimits")) and attached the
interdiff between v7 and d41e0decb7d7 below.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index a3261e842d6d..eca2812cfd25 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -587,13 +587,14 @@ static inline bool pci_dev_test_and_set_removed(struct pci_dev *dev)
struct aer_err_info {
struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
+ int ratelimit_print[AER_MAX_MULTI_ERR_DEVICES];
int error_dev_num;
const char *level; /* printk level */
unsigned int id:16;
unsigned int severity:2; /* 0:NONFATAL | 1:FATAL | 2:COR */
- unsigned int ratelimit:1; /* 0=skip, 1=print */
+ unsigned int root_ratelimit_print:1; /* 0=skip, 1=print */
unsigned int __pad1:4;
unsigned int multi_error_valid:1;
@@ -606,15 +607,16 @@ struct aer_err_info {
struct pcie_tlp_log tlp; /* TLP Header */
};
-int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info);
-void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
+int aer_get_device_error_info(struct aer_err_info *info, int i);
+void aer_print_error(struct aer_err_info *info, int i);
int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
unsigned int tlp_len, bool flit,
struct pcie_tlp_log *log);
unsigned int aer_tlp_log_len(struct pci_dev *dev, u32 aercc);
void pcie_print_tlp_log(const struct pci_dev *dev,
- const struct pcie_tlp_log *log, const char *pfx);
+ const struct pcie_tlp_log *log, const char *level,
+ const char *pfx);
#endif /* CONFIG_PCIEAER */
#ifdef CONFIG_PCIEPORTBUS
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
index 9b8dea317a79..48014010dc8b 100644
--- a/drivers/pci/pcie/aer.c
+++ b/drivers/pci/pcie/aer.c
@@ -672,31 +672,31 @@ static DEVICE_ATTR_RW(ratelimit_log_enable);
static ssize_t \
name##_show(struct device *dev, struct device_attribute *attr, \
char *buf) \
-{ \
- struct pci_dev *pdev = to_pci_dev(dev); \
+ { \
+ struct pci_dev *pdev = to_pci_dev(dev); \
\
- return sysfs_emit(buf, "%d\n", \
- pdev->aer_info->ratelimit.burst); \
-} \
+ return sysfs_emit(buf, "%d\n", \
+ pdev->aer_info->ratelimit.burst); \
+ } \
\
static ssize_t \
name##_store(struct device *dev, struct device_attribute *attr, \
const char *buf, size_t count) \
-{ \
- struct pci_dev *pdev = to_pci_dev(dev); \
- int burst; \
+ { \
+ struct pci_dev *pdev = to_pci_dev(dev); \
+ int burst; \
\
- if (!capable(CAP_SYS_ADMIN)) \
- return -EPERM; \
+ if (!capable(CAP_SYS_ADMIN)) \
+ return -EPERM; \
\
- if (kstrtoint(buf, 0, &burst) < 0) \
- return -EINVAL; \
+ if (kstrtoint(buf, 0, &burst) < 0) \
+ return -EINVAL; \
\
- pdev->aer_info->ratelimit.burst = burst; \
+ pdev->aer_info->ratelimit.burst = burst; \
\
- return count; \
-} \
-static DEVICE_ATTR_RW(name)
+ return count; \
+ } \
+ static DEVICE_ATTR_RW(name)
aer_ratelimit_burst_attr(ratelimit_burst_cor_log, cor_log_ratelimit);
aer_ratelimit_burst_attr(ratelimit_burst_uncor_log, uncor_log_ratelimit);
@@ -734,9 +734,6 @@ static void pci_dev_aer_stats_incr(struct pci_dev *pdev,
u64 *counter = NULL;
struct aer_info *aer_info = pdev->aer_info;
- trace_aer_event(pci_name(pdev), (info->status & ~info->mask),
- info->severity, info->tlp_header_valid, &info->tlp);
-
if (!aer_info)
return;
@@ -785,6 +782,9 @@ static int aer_ratelimit(struct pci_dev *dev, unsigned int severity)
{
struct ratelimit_state *ratelimit;
+ if (severity == AER_FATAL)
+ return 1; /* AER_FATAL not ratelimited */
+
if (severity == AER_CORRECTABLE)
ratelimit = &dev->aer_info->cor_log_ratelimit;
else
@@ -817,7 +817,7 @@ static void __aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
}
static void aer_print_source(struct pci_dev *dev, struct aer_err_info *info,
- const char *details)
+ bool found)
{
u16 source = info->id;
@@ -825,18 +825,27 @@ static void aer_print_source(struct pci_dev *dev, struct aer_err_info *info,
info->multi_error_valid ? "Multiple " : "",
aer_error_severity_string[info->severity],
pci_domain_nr(dev->bus), PCI_BUS_NUM(source),
- PCI_SLOT(source), PCI_FUNC(source), details);
+ PCI_SLOT(source), PCI_FUNC(source),
+ found ? "" : " (no details found");
}
-void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
+void aer_print_error(struct aer_err_info *info, int i)
{
- int layer, agent;
- int id = pci_dev_id(dev);
+ struct pci_dev *dev;
+ int layer, agent, id;
const char *level = info->level;
- pci_dev_aer_stats_incr(dev, info);
+ if (i >= AER_MAX_MULTI_ERR_DEVICES)
+ return;
- if (!info->ratelimit)
+ dev = info->dev[i];
+ id = pci_dev_id(dev);
+
+ pci_dev_aer_stats_incr(dev, info);
+ trace_aer_event(pci_name(dev), (info->status & ~info->mask),
+ info->severity, info->tlp_header_valid, &info->tlp);
+
+ if (!info->ratelimit_print[i])
return;
if (!info->status) {
@@ -858,7 +867,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
__aer_print_error(dev, info);
if (info->tlp_header_valid)
- pcie_print_tlp_log(dev, &info->tlp, dev_fmt(" "));
+ pcie_print_tlp_log(dev, &info->tlp, level, dev_fmt(" "));
out:
if (info->id && info->error_dev_num > 1 && info->id == id)
@@ -903,11 +912,10 @@ 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)
- info.tlp = aer->header_log;
pci_dev_aer_stats_incr(dev, &info);
+ trace_aer_event(pci_name(dev), (status & ~mask),
+ aer_severity, tlp_header_valid, &aer->header_log);
if (!aer_ratelimit(dev, info.severity))
return;
@@ -925,13 +933,9 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
aer_printk(info.level, dev, "aer_uncor_severity: 0x%08x\n",
aer->uncor_severity);
- /*
- * pcie_print_tlp_log() uses KERN_ERR, but we only call it when
- * tlp_header_valid is set, and info.level is always KERN_ERR in
- * that case.
- */
if (tlp_header_valid)
- pcie_print_tlp_log(dev, &aer->header_log, dev_fmt(" "));
+ pcie_print_tlp_log(dev, &aer->header_log, info.level,
+ dev_fmt(" "));
}
EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
@@ -942,23 +946,27 @@ EXPORT_SYMBOL_NS_GPL(pci_print_aer, "CXL");
*/
static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
{
+ int i = e_info->error_dev_num;
+
+ if (i >= AER_MAX_MULTI_ERR_DEVICES)
+ return -ENOSPC;
+
+ e_info->dev[i] = pci_dev_get(dev);
+ e_info->error_dev_num++;
+
/*
* 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.
+ * error logged in its own AER Capability. Messages are emitted
+ * when "ratelimit_print[i]" is non-zero. If we will print detail
+ * for a downstream device, make sure we print the Error Source ID
+ * from the root as well.
*/
- if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
- e_info->dev[e_info->error_dev_num] = pci_dev_get(dev);
- e_info->ratelimit |= aer_ratelimit(dev, e_info->severity);
- e_info->error_dev_num++;
- return 0;
+ if (aer_ratelimit(dev, e_info->severity)) {
+ e_info->ratelimit_print[i] = 1;
+ e_info->root_ratelimit_print = 1;
}
- return -ENOSPC;
+ return 0;
}
/**
@@ -1337,19 +1345,26 @@ EXPORT_SYMBOL_GPL(aer_recover_queue);
/**
* aer_get_device_error_info - read error status from dev and store it to info
- * @dev: pointer to the device expected to have an error record
* @info: pointer to structure to store the error record
+ * @i: index into info->dev[]
*
* Return: 1 on success, 0 on error.
*
* Note that @info is reused among all error devices. Clear fields properly.
*/
-int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info *info)
+int aer_get_device_error_info(struct aer_err_info *info, int i)
{
- int type = pci_pcie_type(dev);
- int aer = dev->aer_cap;
+ struct pci_dev *dev;
+ int type, aer;
u32 aercc;
+ if (i >= AER_MAX_MULTI_ERR_DEVICES)
+ return 0;
+
+ dev = info->dev[i];
+ aer = dev->aer_cap;
+ type = pci_pcie_type(dev);
+
/* Must reset in this function */
info->status = 0;
info->tlp_header_valid = 0;
@@ -1401,11 +1416,11 @@ static inline void aer_process_err_devices(struct aer_err_info *e_info)
/* Report all before handling them, to not lose records by reset etc. */
for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
- if (aer_get_device_error_info(e_info->dev[i], e_info))
- aer_print_error(e_info->dev[i], e_info);
+ if (aer_get_device_error_info(e_info, i))
+ aer_print_error(e_info, i);
}
for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++) {
- if (aer_get_device_error_info(e_info->dev[i], e_info))
+ if (aer_get_device_error_info(e_info, i))
handle_error_source(e_info->dev[i], e_info);
}
}
@@ -1425,17 +1440,18 @@ static void aer_isr_one_error_type(struct pci_dev *root,
/*
* If we're going to log error messages, we've already set
- * "info->ratelimit" to non-zero (which enables printing) because
- * this is either an ERR_FATAL or we found a device with an error
- * logged in its AER Capability.
+ * "info->root_ratelimit_print" and "info->ratelimit_print[i]" to
+ * non-zero (which enables printing) because this is either an
+ * ERR_FATAL or we found a device with an error logged in its AER
+ * Capability.
*
* If we didn't find the Error Source device, at least log the
* Requester ID from the ERR_* Message received by the Root Port or
* RCEC, ratelimited by the RP or RCEC.
*/
- if (info->ratelimit ||
+ if (info->root_ratelimit_print ||
(!found && aer_ratelimit(root, info->severity)))
- aer_print_source(root, info, found ? "" : " (no details found");
+ aer_print_source(root, info, found);
if (found)
aer_process_err_devices(info);
@@ -1470,14 +1486,12 @@ static void aer_isr_one_error(struct pci_dev *root,
aer_isr_one_error_type(root, &e_info);
}
- /* Note that messages for ERR_FATAL are never ratelimited */
if (status & PCI_ERR_ROOT_UNCOR_RCV) {
int fatal = status & PCI_ERR_ROOT_FATAL_RCV;
int multi = status & PCI_ERR_ROOT_MULTI_UNCOR_RCV;
struct aer_err_info e_info = {
.id = ERR_UNCOR_ID(e_src->id),
.severity = fatal ? AER_FATAL : AER_NONFATAL,
- .ratelimit = fatal ? 1 : 0,
.level = KERN_ERR,
.multi_error_valid = multi ? 1 : 0,
};
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 530c5e2cf7e8..fc18349614d7 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -222,7 +222,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
dpc_tlp_log_len(pdev),
pdev->subordinate->flit_mode,
&tlp_log);
- pcie_print_tlp_log(pdev, &tlp_log, dev_fmt(""));
+ pcie_print_tlp_log(pdev, &tlp_log, KERN_ERR, dev_fmt(""));
if (pdev->dpc_rp_log_size < PCIE_STD_NUM_TLP_HEADERLOG + 1)
goto clear_status;
@@ -253,6 +253,10 @@ static int dpc_get_aer_uncorrect_severity(struct pci_dev *dev,
info->severity = AER_NONFATAL;
info->level = KERN_ERR;
+
+ info->dev[0] = dev;
+ info->error_dev_num = 1;
+
return 1;
}
@@ -270,9 +274,8 @@ void dpc_process_error(struct pci_dev *pdev)
pci_warn(pdev, "containment event, status:%#06x: unmasked uncorrectable error detected\n",
status);
if (dpc_get_aer_uncorrect_severity(pdev, &info) &&
- aer_get_device_error_info(pdev, &info)) {
- info.ratelimit = 1; /* ERR_FATAL; no ratelimit */
- aer_print_error(pdev, &info);
+ aer_get_device_error_info(&info, 0)) {
+ aer_print_error(&info, 0);
pci_aer_clear_nonfatal_status(pdev);
pci_aer_clear_fatal_status(pdev);
}
diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
index 890d5391d7f5..71f8fc9ea2ed 100644
--- a/drivers/pci/pcie/tlp.c
+++ b/drivers/pci/pcie/tlp.c
@@ -98,12 +98,14 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
* pcie_print_tlp_log - Print TLP Header / Prefix Log contents
* @dev: PCIe device
* @log: TLP Log structure
+ * @level: Printk log level
* @pfx: String prefix
*
* Prints TLP Header and Prefix Log information held by @log.
*/
void pcie_print_tlp_log(const struct pci_dev *dev,
- const struct pcie_tlp_log *log, const char *pfx)
+ const struct pcie_tlp_log *log, const char *level,
+ const char *pfx)
{
/* EE_PREFIX_STR fits the extended DW space needed for the Flit mode */
char buf[11 * PCIE_STD_MAX_TLP_HEADERLOG + 1];
@@ -130,6 +132,6 @@ void pcie_print_tlp_log(const struct pci_dev *dev,
}
}
- pci_err(dev, "%sTLP Header%s: %s\n", pfx,
+ dev_printk(level, &dev->dev, "%sTLP Header%s: %s\n", pfx,
log->flit ? " (Flit)" : "", buf);
}
Powered by blists - more mailing lists