Skip to contentSkip to conversation

PCI/AER: Improve Error Source handling

PATCH Applied Lukas Wunner 4 participants 9 messages Nov 6 – Nov 9

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v2 cover PCI/AER: Improve Error Source handling

While analyzing log output provided by Alex, I noticed that the AER driver fails to find the error-signaling device if the Error Source ID register contains an invalid, non-zero value. Fix it (patch [2/2]).

While at it, fix a potential NULL pointer deref in that portion of the code (patch [1/2]).

Changes versus v1 in patch [2/2]:
Reorder a < b && b < c comparisons (Ilpo, Bjorn) Amend kernel-doc of find_source_device() (Sathya) s/subordinate/secondary/ in commit message (Bjorn) Document behavioral change in commit message (sashiko)

Link to v1:
https://lore.kernel.org/r/[email protected]

Lukas Wunner (2):
PCI/AER: Avoid NULL pointer deref on search for Error Source PCI/AER: Check validity of Error Source ID register

 drivers/pci/pci.h       |  7 ++++++-
 drivers/pci/pcie/aer.c  | 37 +++++++++++++++++++++++++++----------
 drivers/pci/pcie/rcec.c | 14 ++++++++++++++
 3 files changed, 47 insertions(+), 11 deletions(-)
Signature · 1 line
2.53.0
#1 lore raw

PATCH v2 1/2 PCI/AER: Avoid NULL pointer deref on search for Error Source

A Root Port's subordinate bus may be NULL if the PCI core runs out of bus numbers on enumeration. Yet when the AER driver searches for the source of an Error Message, it dereferences the subordinate pointer without checking for NULL. Fix it.

Fixes 28eb27cf0839 ("PCI AER: support invalid error source IDs")
Cc [email protected] # v2.6.31+
---
 drivers/pci/pcie/aer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pcie/aer.c +1 -1
index db4f607f889c..d492c7ce01e1 100644
@@ -1251,7 +1251,7 @@ static bool find_source_device(struct pci_dev *parent,
 
 	if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
 		pcie_walk_rcec(parent, find_device_iter, e_info);
-	else
+	else if (parent->subordinate)
 		pci_walk_bus(parent->subordinate, find_device_iter, e_info);
 
 	if (!e_info->error_dev_num)
Signature · 1 line
2.53.0
#2 lore raw

Re: [PATCH v2 1/2] PCI/AER: Avoid NULL pointer deref on search for Error Source

Sashiko has reviewed this patch and found no issues.
Quoted 7 lines

A Root Port's subordinate bus may be NULL if the PCI core runs out of bus numbers on enumeration. Yet when the AER driver searches for the source of an Error Message, it dereferences the subordinate pointer without checking for NULL. Fix it.

Fixes 28eb27cf0839 ("PCI AER: support invalid error source IDs")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

PATCH v2 2/2 PCI/AER: Check validity of Error Source ID register

In reply to Lukas Wunner

Certain Intel and AMD Root Ports are known to populate the Error Source ID register with an invalid value. The AER driver seeks to recognize such buggy silicon by assuming invalidity if the bus number is zero. It then searches for the error-signaling device in the hierarchy below the Root Port. This was introduced by commit 28eb27cf0839 ("PCI AER: support invalid error source IDs").

Alex has provided dmesg output which shows an invalid Error Source ID with a non-zero bus number. The AER driver's heuristic fails to recognize its invalidity:

pcieport 0000:00:07.1: AER: Correctable Error message received from 0000:01:00.0 (no details found)

The Root Port at 0000:00:07.1 has secondary bus 04, so an error source on bus 01 is obviously invalid. The range of secondary buses is cached and checking against it is cheap.

Do that instead of relying on a heuristic. Emit a message when an invalid Error Source ID is detected to alert silicon validation engineers.

If the Error Source ID is the Root Port itself, assume that it is valid. This changes behavior if the Error Message originated from a device which happens to have the same slot and function number as the Root Port and whose bus number was zeroed in the Error Source ID register. On affected Intel and AMD systems, the invalid Error Source ID always has a zero slot and function number and there is no Root Port at 0000:00:00.0, so it is expected that this change does not lead to false-negatives in practice.

---
Changes versus v1:
Reorder a < b && b < c comparisons (Ilpo, Bjorn) Amend kernel-doc of find_source_device() (Sathya) s/subordinate/secondary/ in commit message (Bjorn) Document behavioral change in commit message (sashiko)

 drivers/pci/pci.h       |  7 ++++++-
 drivers/pci/pcie/aer.c  | 35 ++++++++++++++++++++++++++---------
 drivers/pci/pcie/rcec.c | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 10 deletions(-)
drivers/pci/pci.h +6 -1
index 77efa4c5c960..aa392bb6a482 100644
@@ -871,6 +871,9 @@ static inline bool pci_dev_binding_disallowed(struct pci_dev *dev)
  * @id: Value from register PCI_ERR_ROOT_ERR_SRC
  * @severity: AER severity, 0-UNCOR Non-fatal, 1-UNCOR fatal, 2-COR
  * @root_ratelimit_print: Flag to log or not log the root's error. 0=NotLog/1=Log
+ * @id_valid: Whether @id is a valid value.  Some Root Ports report an invalid
+ *	Error Source ID, necessitating a search for the error-signaling device
+ *	in the hierarchy below the Root Port.
  * @multi_error_valid: If multiple errors are reported
  * @first_error: First reported error
  * @__pad2: Padding for alignment
@@ -893,7 +896,8 @@ struct aer_err_info {
 
 	unsigned int severity:2;
 	unsigned int root_ratelimit_print:1;
-	unsigned int __pad1:4;
+	unsigned int __pad1:3;
+	unsigned int id_valid:1;
 	unsigned int multi_error_valid:1;
 
 	unsigned int first_error:5;
@@ -955,6 +959,7 @@ void pcie_link_rcec(struct pci_dev *rcec);
 void pcie_walk_rcec(struct pci_dev *rcec,
 		    int (*cb)(struct pci_dev *, void *),
 		    void *userdata);
+bool pci_rcec_associated(struct pci_dev *rcec, u16 id);
 #else
 static inline void pci_rcec_init(struct pci_dev *dev) { }
 static inline void pci_rcec_exit(struct pci_dev *dev) { }
drivers/pci/pcie/aer.c +26 -9
index d492c7ce01e1..6f2460231422 100644
@@ -951,12 +951,13 @@ static void aer_print_source(struct pci_dev *dev, struct aer_err_info *info,
 {
 	u16 source = info->id;
 
-	pci_info(dev, "%s%s Error message%s from %04x:%02x:%02x.%d%s\n",
+	pci_info(dev, "%s%s Error message%s from %04x:%02x:%02x.%d%s%s\n",
 		 info->multi_error_valid ? "Multiple " : "",
 		 aer_error_severity_string[info->severity],
 		 info->multi_error_valid ? "s received, first one" : " received",
 		 pci_domain_nr(dev->bus), PCI_BUS_NUM(source),
 		 PCI_SLOT(source), PCI_FUNC(source),
+		 info->id_valid ? "" : " (invalid source)",
 		 found ? "" : " (no details found)");
 }
 
@@ -1146,6 +1147,24 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 	return 0;
 }
 
+static bool source_id_valid(struct pci_dev *root, u16 id)
+{
+	/* The Root Port or RCEC can itself be a valid Error Source */
+	if (pci_dev_id(root) == id)
+		return true;
+
+	/* For RCECs, @id shall be in range of its associated buses/devices */
+	if (pci_pcie_type(root) == PCI_EXP_TYPE_RC_EC)
+		return pci_rcec_associated(root, id);
+
+	/* For Root Ports, @id shall be in range of its subordinate buses */
+	if (!root->subordinate)
+		return false;
+
+	return root->subordinate->busn_res.start <= PCI_BUS_NUM(id) &&
+	       PCI_BUS_NUM(id) <= root->subordinate->busn_res.end;
+}
+
 /**
  * is_error_source - check whether the device is source of reported error
  * @dev: pointer to pci_dev to be checked
@@ -1157,11 +1176,8 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	u32 status, mask;
 	u16 reg16;
 
-	/*
-	 * When bus ID is equal to 0, it might be a bad ID
-	 * reported by Root Port.
-	 */
-	if ((PCI_BUS_NUM(e_info->id) != 0) &&
+	/* Some Root Ports report an invalid Error Source ID */
+	if (e_info->id_valid &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == pci_dev_id(dev))
@@ -1174,8 +1190,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 
 	/*
 	 * When either
-	 *      1) bus ID is equal to 0. Some ports might lose the bus
-	 *              ID of error source id;
+	 *      1) Error Source ID register is invalid;
 	 *      2) bus flag PCI_BUS_FLAGS_NO_AERSID is set
 	 *      3) There are multiple errors and prior ID comparing fails;
 	 * We check AER status registers to find possible reporter.
@@ -1231,7 +1246,7 @@ static int find_device_iter(struct pci_dev *dev, void *data)
  * Return: true if found.
  *
  * Invoked by DPC when error is detected at the Root Port.
- * Caller of this function must set id, severity, and multi_error_valid of
+ * Callers must set id, id_valid, severity, and multi_error_valid fields of
  * struct aer_err_info pointed by @e_info properly.  This function must fill
  * e_info->error_dev_num and e_info->dev[], based on the given information.
  */
@@ -1566,6 +1581,8 @@ static void aer_isr_one_error_type(struct pci_dev *root,
 {
 	bool found;
 
+	info->id_valid = source_id_valid(root, info->id);
+
 	found = find_source_device(root, info);
 
 	/*
drivers/pci/pcie/rcec.c +14 -0
index 2bf6d9d549ba..88070ef0019c 100644
@@ -21,6 +21,20 @@ struct walk_rcec_data {
 	void *user_data;
 };
 
+bool pci_rcec_associated(struct pci_dev *rcec, u16 id)
+{
+	if (!rcec->rcec_ea)
+		return false;
+
+	if (rcec->bus->number == PCI_BUS_NUM(id)) {
+		unsigned long bitmap = rcec->rcec_ea->bitmap;
+		return test_bit(PCI_SLOT(id), &bitmap);
+	}
+
+	return rcec->rcec_ea->nextbusn <= PCI_BUS_NUM(id) &&
+	       PCI_BUS_NUM(id) <= rcec->rcec_ea->lastbusn;
+}
+
 static bool rcec_assoc_rciep(struct pci_dev *rcec, struct pci_dev *rciep)
 {
 	unsigned long bitmap = rcec->rcec_ea->bitmap;
Signature · 1 line
2.53.0
Reviewed-by on v2 2/2
#4 lore raw

Re: [PATCH v2 2/2] PCI/AER: Check validity of Error Source ID register

Quoted 178 lines · Lukas Wunner

Certain Intel and AMD Root Ports are known to populate the Error Source ID register with an invalid value. The AER driver seeks to recognize such buggy silicon by assuming invalidity if the bus number is zero. It then searches for the error-signaling device in the hierarchy below the Root Port. This was introduced by commit 28eb27cf0839 ("PCI AER: support invalid error source IDs").

Alex has provided dmesg output which shows an invalid Error Source ID with a non-zero bus number. The AER driver's heuristic fails to recognize its invalidity:

pcieport 0000:00:07.1: AER: Correctable Error message received from 0000:01:00.0 (no details found)

The Root Port at 0000:00:07.1 has secondary bus 04, so an error source on bus 01 is obviously invalid. The range of secondary buses is cached and checking against it is cheap.

Do that instead of relying on a heuristic. Emit a message when an invalid Error Source ID is detected to alert silicon validation engineers.

If the Error Source ID is the Root Port itself, assume that it is valid. This changes behavior if the Error Message originated from a device which happens to have the same slot and function number as the Root Port and whose bus number was zeroed in the Error Source ID register. On affected Intel and AMD systems, the invalid Error Source ID always has a zero slot and function number and there is no Root Port at 0000:00:00.0, so it is expected that this change does not lead to false-negatives in practice.

---
Changes versus v1:
Reorder a < b && b < c comparisons (Ilpo, Bjorn) Amend kernel-doc of find_source_device() (Sathya) s/subordinate/secondary/ in commit message (Bjorn) Document behavioral change in commit message (sashiko)

 drivers/pci/pci.h       |  7 ++++++-
 drivers/pci/pcie/aer.c  | 35 ++++++++++++++++++++++++++---------
 drivers/pci/pcie/rcec.c | 14 ++++++++++++++
 3 files changed, 46 insertions(+), 10 deletions(-)
drivers/pci/pci.h +6 -1
index 77efa4c5c960..aa392bb6a482 100644
@@ -871,6 +871,9 @@ static inline bool pci_dev_binding_disallowed(struct pci_dev *dev)
  * @id: Value from register PCI_ERR_ROOT_ERR_SRC
  * @severity: AER severity, 0-UNCOR Non-fatal, 1-UNCOR fatal, 2-COR
  * @root_ratelimit_print: Flag to log or not log the root's error. 0=NotLog/1=Log
+ * @id_valid: Whether @id is a valid value.  Some Root Ports report an invalid
+ *	Error Source ID, necessitating a search for the error-signaling device
+ *	in the hierarchy below the Root Port.
  * @multi_error_valid: If multiple errors are reported
  * @first_error: First reported error
  * @__pad2: Padding for alignment
@@ -893,7 +896,8 @@ struct aer_err_info {
 
 	unsigned int severity:2;
 	unsigned int root_ratelimit_print:1;
-	unsigned int __pad1:4;
+	unsigned int __pad1:3;
+	unsigned int id_valid:1;
 	unsigned int multi_error_valid:1;
 
 	unsigned int first_error:5;
@@ -955,6 +959,7 @@ void pcie_link_rcec(struct pci_dev *rcec);
 void pcie_walk_rcec(struct pci_dev *rcec,
 		    int (*cb)(struct pci_dev *, void *),
 		    void *userdata);
+bool pci_rcec_associated(struct pci_dev *rcec, u16 id);
 #else
 static inline void pci_rcec_init(struct pci_dev *dev) { }
 static inline void pci_rcec_exit(struct pci_dev *dev) { }
drivers/pci/pcie/aer.c +26 -9
index d492c7ce01e1..6f2460231422 100644
@@ -951,12 +951,13 @@ static void aer_print_source(struct pci_dev *dev, struct aer_err_info *info,
 {
 	u16 source = info->id;
 
-	pci_info(dev, "%s%s Error message%s from %04x:%02x:%02x.%d%s\n",
+	pci_info(dev, "%s%s Error message%s from %04x:%02x:%02x.%d%s%s\n",
 		 info->multi_error_valid ? "Multiple " : "",
 		 aer_error_severity_string[info->severity],
 		 info->multi_error_valid ? "s received, first one" : " received",
 		 pci_domain_nr(dev->bus), PCI_BUS_NUM(source),
 		 PCI_SLOT(source), PCI_FUNC(source),
+		 info->id_valid ? "" : " (invalid source)",
 		 found ? "" : " (no details found)");
 }
 
@@ -1146,6 +1147,24 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 	return 0;
 }
 
+static bool source_id_valid(struct pci_dev *root, u16 id)
+{
+	/* The Root Port or RCEC can itself be a valid Error Source */
+	if (pci_dev_id(root) == id)
+		return true;
+
+	/* For RCECs, @id shall be in range of its associated buses/devices */
+	if (pci_pcie_type(root) == PCI_EXP_TYPE_RC_EC)
+		return pci_rcec_associated(root, id);
+
+	/* For Root Ports, @id shall be in range of its subordinate buses */
+	if (!root->subordinate)
+		return false;
+
+	return root->subordinate->busn_res.start <= PCI_BUS_NUM(id) &&
+	       PCI_BUS_NUM(id) <= root->subordinate->busn_res.end;
+}
+
 /**
  * is_error_source - check whether the device is source of reported error
  * @dev: pointer to pci_dev to be checked
@@ -1157,11 +1176,8 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	u32 status, mask;
 	u16 reg16;
 
-	/*
-	 * When bus ID is equal to 0, it might be a bad ID
-	 * reported by Root Port.
-	 */
-	if ((PCI_BUS_NUM(e_info->id) != 0) &&
+	/* Some Root Ports report an invalid Error Source ID */
+	if (e_info->id_valid &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == pci_dev_id(dev))
@@ -1174,8 +1190,7 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 
 	/*
 	 * When either
-	 *      1) bus ID is equal to 0. Some ports might lose the bus
-	 *              ID of error source id;
+	 *      1) Error Source ID register is invalid;
 	 *      2) bus flag PCI_BUS_FLAGS_NO_AERSID is set
 	 *      3) There are multiple errors and prior ID comparing fails;
 	 * We check AER status registers to find possible reporter.
@@ -1231,7 +1246,7 @@ static int find_device_iter(struct pci_dev *dev, void *data)
  * Return: true if found.
  *
  * Invoked by DPC when error is detected at the Root Port.
- * Caller of this function must set id, severity, and multi_error_valid of
+ * Callers must set id, id_valid, severity, and multi_error_valid fields of
  * struct aer_err_info pointed by @e_info properly.  This function must fill
  * e_info->error_dev_num and e_info->dev[], based on the given information.
  */
@@ -1566,6 +1581,8 @@ static void aer_isr_one_error_type(struct pci_dev *root,
 {
 	bool found;
 
+	info->id_valid = source_id_valid(root, info->id);
+
 	found = find_source_device(root, info);
 
 	/*
drivers/pci/pcie/rcec.c +7 -0
index 2bf6d9d549ba..88070ef0019c 100644
@@ -21,6 +21,20 @@ struct walk_rcec_data {
 	void *user_data;
 };
 
+bool pci_rcec_associated(struct pci_dev *rcec, u16 id)
+{
+	if (!rcec->rcec_ea)
+		return false;
+
+	if (rcec->bus->number == PCI_BUS_NUM(id)) {
+		unsigned long bitmap = rcec->rcec_ea->bitmap;

drivers/pci/pcie/rcec.c struct walk_rcec_data { · line 40

Quoted 10 lines
drivers/pci/pcie/rcec.c +7 -0
+		return test_bit(PCI_SLOT(id), &bitmap);
+	}
+
+	return rcec->rcec_ea->nextbusn <= PCI_BUS_NUM(id) &&
+	       PCI_BUS_NUM(id) <= rcec->rcec_ea->lastbusn;
+}
+
 static bool rcec_assoc_rciep(struct pci_dev *rcec, struct pci_dev *rciep)
 {
 	unsigned long bitmap = rcec->rcec_ea->bitmap;
Signature · 1 line
 i.
#5 lore raw

Re: [PATCH v2 2/2] PCI/AER: Check validity of Error Source ID register

In reply to Lukas Wunner
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] PCI/AER: false Root Port match in is_error_source() skips downstream search --

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Medium] PCI/AER: false Root Port match in is_error_source() skips downstream search
--

commit 96874e2f4a0f4f66402c752cd7b61e3904f79ff3
Author: Lukas Wunner <[email protected]>

PCI/AER: Check validity of Error Source ID register

This commit replaces the bus-zero heuristic for PCI_ERR_ROOT_ERR_SRC with a check that the Error Source ID matches the Root Port or RCEC itself or falls within its subordinate or associated buses and devices.

Quoted 4 lines
drivers/pci/pcie/aer.c +0 -0
index d492c7ce01e15..6f24602314225 100644

[ ... ]

Quoted 29 lines
drivers/pci/pcie/aer.c +18 -0
@@ -1146,6 +1147,24 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
 	return 0;
 }
 
+static bool source_id_valid(struct pci_dev *root, u16 id)
+{
+	/* The Root Port or RCEC can itself be a valid Error Source */
+	if (pci_dev_id(root) == id)
+		return true;
+
+	/* For RCECs, @id shall be in range of its associated buses/devices */
+	if (pci_pcie_type(root) == PCI_EXP_TYPE_RC_EC)
+		return pci_rcec_associated(root, id);
+
+	/* For Root Ports, @id shall be in range of its subordinate buses */
+	if (!root->subordinate)
+		return false;
+
+	return root->subordinate->busn_res.start <= PCI_BUS_NUM(id) &&
+	       PCI_BUS_NUM(id) <= root->subordinate->busn_res.end;
+}
+
 /**
  * is_error_source - check whether the device is source of reported error
  * @dev: pointer to pci_dev to be checked
@@ -1157,11 +1176,8 @@ static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info)
 	u32 status, mask;
 	u16 reg16;
 

drivers/pci/pcie/aer.c static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info) · line 1183

Quoted 10 lines
drivers/pci/pcie/aer.c +2 -5
-	/*
-	 * When bus ID is equal to 0, it might be a bad ID
-	 * reported by Root Port.
-	 */
-	if ((PCI_BUS_NUM(e_info->id) != 0) &&
+	/* Some Root Ports report an invalid Error Source ID */
+	if (e_info->id_valid &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == pci_dev_id(dev))

[Severity: Medium]
As reported by Ilpo Järvinen on v1 [1], can treating pci_dev_id(root) == id as unconditionally valid cause a false-positive match on the Root Port and skip searching the subordinate hierarchy when a Root Port at 00:00.0 (such as on DesignWare, Cadence, or Aardvark SoC host bridges) reports 0x0000 for a downstream device error, or when a Root Port at 00:xx.y loses the bus number of a downstream error source at bus:xx.y?

Previously, PCI_BUS_NUM(e_info->id) != 0 prevented an Error Source ID on bus 0 from matching the Root Port by ID alone, so is_error_source() fell through to check the Root Port's PCI_ERR_COR_STATUS or PCI_ERR_UNCOR_STATUS registers. With e_info->id_valid set to true, find_source_device() -> find_device_iter() -> is_error_source() matches the Root Port on ID alone and returns before walking parent->subordinate (for single errors).

When aer_process_err_devices() then calls aer_get_device_error_info() on the Root Port, it finds no unmasked error bits set in the Root Port's own AER status register and returns 0:

drivers/pci/pcie/aer.c:aer_get_device_error_info() {
    ...
	if (info->severity == AER_CORRECTABLE) {
		pci_read_config_dword(dev, aer + PCI_ERR_COR_STATUS,
			&info->status);
		pci_read_config_dword(dev, aer + PCI_ERR_COR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;
	} else if (type == PCI_EXP_TYPE_ROOT_PORT ||
		   type == PCI_EXP_TYPE_RC_EC ||
		   type == PCI_EXP_TYPE_DOWNSTREAM ||
		   info->severity == AER_NONFATAL) {

		/* Link is still healthy for IO reads */
		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_STATUS,
			&info->status);
		pci_read_config_dword(dev, aer + PCI_ERR_UNCOR_MASK,
			&info->mask);
		if (!(info->status & ~info->mask))
			return 0;
    ...
}

As a result, the downstream device's AER error goes unlogged, its AER status register is not cleared, and pcie_do_recovery() is skipped for non-fatal and fatal errors. Note that find_source_device() already checks the Root Port first via find_device_iter() and returns before walking parent->subordinate whenever the Root Port has an unmasked error bit set in its own AER status register.

[1] https://lore.kernel.org/r/[email protected]

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#6 lore raw

Re: [PATCH v2 2/2] PCI/AER: Check validity of Error Source ID register

Quoted 6 lines · On Fri, Nov 06, 2026 at 05:15:07PM +0000, [email protected] wrote:
Quoted 6 lines

+++ b/drivers/pci/pcie/aer.c

drivers/pci/pcie/aer.c +0 -5
-	/*
-	 * When bus ID is equal to 0, it might be a bad ID
-	 * reported by Root Port.
-	 */
-	if ((PCI_BUS_NUM(e_info->id) != 0) &&
Quoted 8 lines

drivers/pci/pcie/aer.c static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info) · line 1183

Quoted 5 lines
drivers/pci/pcie/aer.c +2 -0
+	/* Some Root Ports report an invalid Error Source ID */
+	if (e_info->id_valid &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == pci_dev_id(dev))

[Severity: Medium]
As reported by Ilpo Järvinen on v1 [1]

Hallucination, this was reported by sashiko itself and Ilpo merely confirmed sashiko's finding.

Quoted 6 lines

can treating pci_dev_id(root) == id
as unconditionally valid cause a false-positive match on the Root Port and skip searching the subordinate hierarchy when a Root Port at 00:00.0 (such as on DesignWare, Cadence, or Aardvark SoC host bridges) reports 0x0000 for a downstream device error, or when a Root Port at 00:xx.y loses the bus number of a downstream error source at bus:xx.y?

The commit message addresses this concern, it seems sashiko overlooked that paragraph.

Thanks,

Lukas

#7 lore raw

Re: [PATCH v2 2/2] PCI/AER: Check validity of Error Source ID register

Quoted 16 lines · Lukas Wunner
Quoted 6 lines · On Fri, Nov 06, 2026 at 05:15:07PM +0000, [email protected] wrote:
Quoted 6 lines

+++ b/drivers/pci/pcie/aer.c

drivers/pci/pcie/aer.c +0 -5
-	/*
-	 * When bus ID is equal to 0, it might be a bad ID
-	 * reported by Root Port.
-	 */
-	if ((PCI_BUS_NUM(e_info->id) != 0) &&
Quoted 8 lines

drivers/pci/pcie/aer.c static bool is_error_source(struct pci_dev *dev, struct aer_err_info *e_info) · line 1183

Quoted 5 lines
drivers/pci/pcie/aer.c +2 -0
+	/* Some Root Ports report an invalid Error Source ID */
+	if (e_info->id_valid &&
 	    !(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
 		/* Device ID match? */
 		if (e_info->id == pci_dev_id(dev))

[Severity: Medium]
As reported by Ilpo Järvinen on v1 [1]

Quoted 2 lines

Hallucination, this was reported by sashiko itself and Ilpo merely confirmed sashiko's finding.

I agree. I've no problem with this change after the cases we expect this impact are not among the cases sashiko came up so the comment in the changelog is enough for me.

Quoted 9 lines
Quoted 6 lines

can treating pci_dev_id(root) == id
as unconditionally valid cause a false-positive match on the Root Port and skip searching the subordinate hierarchy when a Root Port at 00:00.0 (such as on DesignWare, Cadence, or Aardvark SoC host bridges) reports 0x0000 for a downstream device error, or when a Root Port at 00:xx.y loses the bus number of a downstream error source at bus:xx.y?

The commit message addresses this concern, it seems sashiko overlooked that paragraph.

Signature · 1 line
 i.
#8 lore raw
In reply to Lukas Wunner
Quoted 16 lines · Lukas Wunner

While analyzing log output provided by Alex, I noticed that the AER driver fails to find the error-signaling device if the Error Source ID register contains an invalid, non-zero value. Fix it (patch [2/2]).

While at it, fix a potential NULL pointer deref in that portion of the code (patch [1/2]).

Changes versus v1 in patch [2/2]:
Reorder a < b && b < c comparisons (Ilpo, Bjorn) Amend kernel-doc of find_source_device() (Sathya) s/subordinate/secondary/ in commit message (Bjorn) Document behavioral change in commit message (sashiko)

Link to v1:
https://lore.kernel.org/r/[email protected]

Quoted 8 lines

Lukas Wunner (2):
PCI/AER: Avoid NULL pointer deref on search for Error Source PCI/AER: Check validity of Error Source ID register

 drivers/pci/pci.h       |  7 ++++++-
 drivers/pci/pcie/aer.c  | 37 +++++++++++++++++++++++++++----------
 drivers/pci/pcie/rcec.c | 14 ++++++++++++++
 3 files changed, 47 insertions(+), 11 deletions(-)

Applied to pci/aer for v7.5, thank you!