PCI/AER: Improve Error Source handling
Subsystems (2)
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. Patch [2/2] fixes that.
Also, a potential NULL pointer deref caught my eye in that portion of the code which is fixed by patch [1/2]. I believe this is hard to trigger, but the kernel should still handle it gracefully.
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 | 35 ++++++++++++++++++++++++++--------- drivers/pci/pcie/rcec.c | 14 ++++++++++++++ 3 files changed, 46 insertions(+), 10 deletions(-)
Signature · 1 line
2.53.0
PATCH 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.
--- drivers/pci/pcie/aer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/pcie/aer.c +1 -1
@@ -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
[email protected]
AI reviewRe: [PATCH 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")Signed-off-by Lukas Wunner <[email protected]>
Sashiko has reviewed this patch and found no issues.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH 1/2] PCI/AER: Avoid NULL pointer deref on search for Error Source
In reply to Lukas WunnerQuoted 18 lines · Lukas Wunner
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")Signed-off-by Lukas Wunner <[email protected]>Cc [email protected] # v2.6.31+--- drivers/pci/pcie/aer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/pcie/aer.c +0
-0
drivers/pci/pcie/aer.c static bool find_source_device(struct pci_dev *parent, · line 1257
Quoted 8 lines
drivers/pci/pcie/aer.c +1
-1@@ -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
i.
Re: [PATCH 1/2] PCI/AER: Avoid NULL pointer deref on search for Error Source
In reply to Lukas WunnerQuoted 9 lines · Lukas Wunner
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")Signed-off-by Lukas Wunner <[email protected]>Cc [email protected] # v2.6.31+---
Quoted 16 lines
drivers/pci/pcie/aer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/pci/pcie/aer.c +1
-1@@ -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 · 2 lines
Sathyanarayanan Kuppuswamy Linux Kernel Developer
PATCH 2/2 PCI/AER: Check validity of Error Source ID register
In reply to Lukas WunnerCertain 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 subordinate bus 04, so an error source on bus 01 is obviously invalid. The range of subordinate 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.
--- drivers/pci/pci.h | 7 ++++++- drivers/pci/pcie/aer.c | 33 +++++++++++++++++++++++++-------- drivers/pci/pcie/rcec.c | 14 ++++++++++++++ 3 files changed, 45 insertions(+), 9 deletions(-)
drivers/pci/pci.h +6 -1
@@ -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 +25 -8
@@ -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) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id); +} +/** * 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.
@@ -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
@@ -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) && + rcec->rcec_ea->lastbusn >=PCI_BUS_NUM (id); +} +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
[email protected]
AI reviewRe: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] 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(s) to consider:
- [High] PCI/AER: false Root Port match in is_error_source() skips downstream search --
commit 4a4627301e6c414f9fd5fc732ea651b5b76f3b16
Author: Lukas Wunner <[email protected]>
PCI/AER: Check validity of Error Source ID register
This commit replaces the bus-zero heuristic for detecting invalid AER Error Source IDs with a check against the Root Port's subordinate bus range or an RCEC's associated buses and devices, and logs when an invalid source ID is detected.
Quoted 4 lines
drivers/pci/pcie/aer.c +0
-0
[ ... ]
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) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id); +} +/** * 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: High]
Can this cause a false positive match on the Root Port itself and skip searching the subordinate hierarchy when a buggy Root Port reports 0x0000 (or loses the bus number of a downstream error source)?
Previously, in drivers/pci/pcie/aer.c, when find_source_device() checked a Root Port on bus 0 via find_device_iter() -> is_error_source(), the check PCI_BUS_NUM(e_info->id) != 0 was false whenever e_info->id was on bus 0. As a result, is_error_source() never matched the Root Port on bus 0 by ID alone, and instead fell through to check whether the Root Port actually had an unmasked error bit set in PCI_ERR_COR_STATUS or PCI_ERR_UNCOR_STATUS.
With this change, aer_isr_one_error_type() sets info->id_valid via source_id_valid(), which returns true whenever pci_dev_id(root) == id. On platforms where the Root Port itself is at 00:00.0 (such as many SoC PCIe controllers or VMD domains), pci_dev_id(root) is 0x0000. The same condition also occurs if a Root Port at 00:xx.y loses the bus number of a downstream endpoint at bus:xx.y (the failure mode described in commit 28eb27cf0839), or
similarly for an RCEC on bus 0 with slot 0 set in rcec->rcec_ea->bitmap.
When a downstream device signals an error and PCI_ERR_ROOT_ERR_SRC reads as pci_dev_id(root), source_id_valid() sets info->id_valid = 1. Then in find_source_device():
find_source_device()
find_device_iter(root, e_info)
is_error_source(root, e_info)
if (e_info->id_valid &&
!(dev->bus->bus_flags & PCI_BUS_FLAGS_NO_AERSID)) {
if (e_info->id == pci_dev_id(dev))
return true;is_error_source() immediately returns true for the Root Port without checking its AER status registers, and when e_info->multi_error_valid is 0, find_source_device() returns early without walking parent->subordinate:
drivers/pci/pcie/aer.c:find_source_device() {
...
/* Is Root Port an agent that sends error message? */
result = find_device_iter(dev, e_info);
if (result)
return true; if (pci_pcie_type(parent) == PCI_EXP_TYPE_RC_EC)
pcie_walk_rcec(parent, find_device_iter, e_info);
else if (parent->subordinate)
pci_walk_bus(parent->subordinate, find_device_iter, e_info);}
Subsequently, aer_process_err_devices() calls aer_get_device_error_info() on the Root Port, sees !(info->status & ~info->mask), and returns 0, leaving the downstream device's error unlogged, uncleared, and unrecovered.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
Quoted 14 lines · On Tue, Nov 03, 2026 at 10:11:52AM +0000, [email protected] wrote:
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))Can this cause a false positive match on the Root Port itself and skip searching the subordinate hierarchy when a buggy Root Port reports
0x0000(or loses the bus number of a downstream error source)?
[...]
Quoted 4 lines
With this change,
aer_isr_one_error_type()setsinfo->id_validviasource_id_valid(), which returns true wheneverpci_dev_id(root)== id. On platforms where the Root Port itself is at 00:00.0 (such as many SoC PCIe controllers or VMD domains),pci_dev_id(root)is0x0000.
I am only aware of Intel and AMD silicon reporting an invalid Error Source ID, not of any ARM or other SoCs misbehaving in this way. On Intel and AMD silicon, bus 0 is generally the root bus and never the subordinate bus of a Root Port, so an Error Source ID with bus 0 will continue to be considered invalid after this patch, unless it matches the Root Port's ID.
In the log output I've seen so far, the slot and function number of the invalid ID is always 0 and there's generally no Root Port at 0000:00:00.0 on Intel and AMD platforms, so the Root Port's ID isn't matched.
It would be possible of course to always search the hierarchy if the Root Port's ID is found in the Error Source ID register, but I'm inclined not to do that unless there's a need. If anybody knows that this can actually occur or feels strongly that it should be handled, please let me know.
Thanks,
Lukas
Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
In reply to Lukas WunnerQuoted 90 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 subordinate bus 04, so an error source on bus 01 is obviously invalid. The range of subordinate 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.
Tested-by Alex Deucher <[email protected]>Signed-off-by Lukas Wunner <[email protected]>--- drivers/pci/pci.h | 7 ++++++- drivers/pci/pcie/aer.c | 33 +++++++++++++++++++++++++-------- drivers/pci/pcie/rcec.c | 14 ++++++++++++++ 3 files changed, 45 insertions(+), 9 deletions(-)drivers/pci/pci.h +6
-1@@ -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 +8
-1@@ -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; +
drivers/pci/pcie/aer.c static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) · line 1165
Quoted 10 lines
drivers/pci/pcie/aer.c +10
-0+/* 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) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id);
I kind of hate the order of this logic. I understand you're probably doing it for the alignment which has some value but...
...For me, the more logical order would be to keep these in increasing order even if doing it results in losing the alignment:
start <= val &&
val <= end
Maybe that's just me and it's not an end of the world either way.
Quoted 48 lines
drivers/pci/pcie/aer.c +7
-7+} +/** * 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. @@ -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 +2
-0@@ -21,6 +21,20 @@ struct walk_rcec_data {void *user_data; }; +bool pci_rcec_associated (struct pci_dev *rcec,u16 id) +{
drivers/pci/pcie/rcec.c struct walk_rcec_data { · line 35
Quoted 10 lines
drivers/pci/pcie/rcec.c +10
-0+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) && + rcec->rcec_ea->lastbusn >=PCI_BUS_NUM (id);
Same thing here.
Sashiko's comment about the behavior change on bad 0 report matching to root port at 0 looked valid concern to me.
Signature · 1 line
i.
Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
Quoted 9 lines · Ilpo Järvinen
Quoted 7 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").
Quoted 7 lines
Quoted 7 lines
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 subordinate bus 04, so an error source
I always think "subordinate" is just slightly ambiguous because it's used for both "anything below this bridge" and the more specific "Subordinate Bus Number" (the highest numbered bus below the bridge).
It's perfectly accurate as-is especially because in this case it looks like Secondary Bus Number and Subordinate Bus Number are *both* 04 for 00:07.1. But "secondary bus 04" seems less ambiguous to me.
Quoted 10 lines
Quoted 2 lines
on bus 01 is obviously invalid. The range of subordinate buses is cached and checking against it is cheap.
...
drivers/pci/pcie/aer.c static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) · line 1165
Quoted 2 lines
drivers/pci/pcie/aer.c +2
-0+return root->subordinate->busn_res.start <=PCI_BUS_NUM (id) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id);I kind of hate the order of this logic. I understand you're probably doing it for the alignment which has some value but...
Quoted 7 lines
...For me, the more logical order would be to keep these in increasing order even if doing it results in losing the alignment:
start <= val &&
val <= endMaybe that's just me and it's not an end of the world either way.
I agree, I think "start <= val && val <= end" is more readable, though not quite as pretty.
Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
Quoted 39 lines · Bjorn Helgaas
Quoted 9 lines · Ilpo Järvinen
Quoted 7 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").Quoted 7 lines
Quoted 7 lines
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 subordinate bus 04, so an error source
I always think "subordinate" is just slightly ambiguous because it's used for both "anything below this bridge" and the more specific "Subordinate Bus Number" (the highest numbered bus below the bridge).
It's perfectly accurate as-is especially because in this case it looks like Secondary Bus Number and Subordinate Bus Number are *both* 04 for 00:07.1. But "secondary bus 04" seems less ambiguous to me.
Quoted 13 lines
Quoted 2 lines
on bus 01 is obviously invalid. The range of subordinate buses is cached and checking against it is cheap.
...
drivers/pci/pcie/aer.c static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) · line 1165
Quoted 2 lines
drivers/pci/pcie/aer.c +2
-0+return root->subordinate->busn_res.start <=PCI_BUS_NUM (id) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id);I kind of hate the order of this logic. I understand you're probably doing it for the alignment which has some value but...
...For me, the more logical order would be to keep these in increasing order even if doing it results in losing the alignment:
Quoted 7 lines
Quoted 4 lines
start <= val &&
val <= endMaybe that's just me and it's not an end of the world either way.
I agree, I think "start <= val && val <= end" is more readable, though not quite as pretty.
I suppose a local var could be used to shorten the derefs and it would even fit to a single line then.
Signature · 1 line
i.
Re: [PATCH 2/2] PCI/AER: Check validity of Error Source ID register
In reply to Lukas WunnerQuoted 130 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 subordinate bus 04, so an error source on bus 01 is obviously invalid. The range of subordinate 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.
Tested-by Alex Deucher <[email protected]>Signed-off-by Lukas Wunner <[email protected]>--- drivers/pci/pci.h | 7 ++++++- drivers/pci/pcie/aer.c | 33 +++++++++++++++++++++++++-------- drivers/pci/pcie/rcec.c | 14 ++++++++++++++ 3 files changed, 45 insertions(+), 9 deletions(-)drivers/pci/pci.h +6
-1@@ -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 +23
-8@@ -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) && + root->subordinate->busn_res.end >=PCI_BUS_NUM (id); +} +/** * 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.
drivers/pci/pcie/aer.c static void aer_isr_one_error_type(struct pci_dev *root, · line 1586
Quoted 6 lines
drivers/pci/pcie/aer.c +2
-0@@ -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);
nit: Updated the kernel-doc help of find_source_device() to include id_valid.
Quoted 17 lines
drivers/pci/pcie/aer.c +0
-0/* drivers/pci/pcie/rcec.c +7
-0@@ -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) && + rcec->rcec_ea->lastbusn >=PCI_BUS_NUM (id); +} +static bool rcec_assoc_rciep (struct pci_dev *rcec,struct pci_dev *rciep) {unsigned long bitmap = rcec->rcec_ea->bitmap;
Signature · 2 lines
Sathyanarayanan Kuppuswamy Linux Kernel Developer