| Message ID | 20220315050842.120063-1-sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly | expand |
On Tue, Mar 15, 2022 at 05:08:42AM +0000, Kuppuswamy Sathyanarayanan wrote: > This error can be reproduced by making following changes to the > aer_irq() function and by executing the given test commands. > > static irqreturn_t aer_irq(int irq, void *context) > struct aer_err_source e_src = {}; > > pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, > &e_src.status); > + pci_dbg(pdev->port, "Root Error Status: %04x\n", > + e_src.status); > if (!(e_src.status & AER_ERR_STATUS_MASK)) > return IRQ_NONE; > > + mdelay(5000); > > # Prep injection data for a correctable error. > $ cd /sys/kernel/debug/apei/einj > $ echo 0x00000040 > error_type > $ echo 0x4 > flags > $ echo 0x891000 > param4 > > # Root Error Status is initially clear > $ setpci -s <Dev ID> ECAP0001+0x30.w > 0000 > > # Inject one error > $ echo 1 > error_inject > > # Interrupt received > pcieport <Dev ID>: AER: Root Error Status 0001 > > # Inject another error (within 5 seconds) > $ echo 1 > error_inject > > # No interrupt received, but "multiple ERR_COR" is now set > $ setpci -s <Dev ID> ECAP0001+0x30.w > 0003 > > # Wait for a while, then clear ERR_COR. A new interrupt immediately > fires. > $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 > pcieport <Dev ID>: AER: Root Error Status 0002 > > Currently, the above issue has been only reproduced in the ICL server > platform. > > [Eric: proposed reproducing steps] Hmm, this differs from the procedure I described on v1, and I don't think will work as described here. Eric
On 3/15/22 10:14 AM, Eric Badger wrote: >> # Prep injection data for a correctable error. >> $ cd /sys/kernel/debug/apei/einj >> $ echo 0x00000040 > error_type >> $ echo 0x4 > flags >> $ echo 0x891000 > param4 >> >> # Root Error Status is initially clear >> $ setpci -s <Dev ID> ECAP0001+0x30.w >> 0000 >> >> # Inject one error >> $ echo 1 > error_inject >> >> # Interrupt received >> pcieport <Dev ID>: AER: Root Error Status 0001 >> >> # Inject another error (within 5 seconds) >> $ echo 1 > error_inject >> >> # No interrupt received, but "multiple ERR_COR" is now set >> $ setpci -s <Dev ID> ECAP0001+0x30.w >> 0003 >> >> # Wait for a while, then clear ERR_COR. A new interrupt immediately >> fires. >> $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 >> pcieport <Dev ID>: AER: Root Error Status 0002 >> >> Currently, the above issue has been only reproduced in the ICL server >> platform. >> >> [Eric: proposed reproducing steps] > Hmm, this differs from the procedure I described on v1, and I don't > think will work as described here. I have attempted to modify the steps to reproduce it without returning IRQ_NONE for all cases (which will break the functionality). But I think I did not correct the last few steps. How about replacing the last 3 steps with following? # Inject another error (within 5 seconds) $ echo 1 > error_inject # You will get a new IRQ with only multiple ERR_COR bit set pcieport <Dev ID>: AER: Root Error Status 0002
On Tue, Mar 15, 2022 at 10:26:46AM -0700, Sathyanarayanan Kuppuswamy wrote: > On 3/15/22 10:14 AM, Eric Badger wrote: > > > # Prep injection data for a correctable error. > > > $ cd /sys/kernel/debug/apei/einj > > > $ echo 0x00000040 > error_type > > > $ echo 0x4 > flags > > > $ echo 0x891000 > param4 > > > > > > # Root Error Status is initially clear > > > $ setpci -s <Dev ID> ECAP0001+0x30.w > > > 0000 > > > > > > # Inject one error > > > $ echo 1 > error_inject > > > > > > # Interrupt received > > > pcieport <Dev ID>: AER: Root Error Status 0001 > > > > > > # Inject another error (within 5 seconds) > > > $ echo 1 > error_inject > > > > > > # No interrupt received, but "multiple ERR_COR" is now set > > > $ setpci -s <Dev ID> ECAP0001+0x30.w > > > 0003 > > > > > > # Wait for a while, then clear ERR_COR. A new interrupt immediately > > > fires. > > > $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 > > > pcieport <Dev ID>: AER: Root Error Status 0002 > > > > > > Currently, the above issue has been only reproduced in the ICL server > > > platform. > > > > > > [Eric: proposed reproducing steps] > > Hmm, this differs from the procedure I described on v1, and I don't > > think will work as described here. > > I have attempted to modify the steps to reproduce it without returning > IRQ_NONE for all cases (which will break the functionality). But I > think I did not correct the last few steps. Well, the thinking in always returning IRQ_NONE was so that only setpci modified the register and we could clearly see how writes to the register affect interrupt generation. > How about replacing the last 3 steps with following? > > # Inject another error (within 5 seconds) > $ echo 1 > error_inject > > # You will get a new IRQ with only multiple ERR_COR bit set > pcieport <Dev ID>: AER: Root Error Status 0002 This seems accurate. Though it does muddy a detail that I think was clearer in the original procedure: was the second interrupt triggered by the second error, or by the write of 0x1 to Root Error Status? Also, in terms of practically running the test, I find the mdelay() can block other interrupts and can make running the test sort of confusing ("is it not printing because the interrupt didn't fire, or because it's spinning and blocking my NIC driver?" :). Cheers, Eric
On 3/15/22 12:52 PM, Eric Badger wrote: > On Tue, Mar 15, 2022 at 10:26:46AM -0700, Sathyanarayanan Kuppuswamy wrote: >> On 3/15/22 10:14 AM, Eric Badger wrote: >>>> # Prep injection data for a correctable error. >>>> $ cd /sys/kernel/debug/apei/einj >>>> $ echo 0x00000040 > error_type >>>> $ echo 0x4 > flags >>>> $ echo 0x891000 > param4 >>>> >>>> # Root Error Status is initially clear >>>> $ setpci -s <Dev ID> ECAP0001+0x30.w >>>> 0000 >>>> >>>> # Inject one error >>>> $ echo 1 > error_inject >>>> >>>> # Interrupt received >>>> pcieport <Dev ID>: AER: Root Error Status 0001 >>>> >>>> # Inject another error (within 5 seconds) >>>> $ echo 1 > error_inject >>>> >>>> # No interrupt received, but "multiple ERR_COR" is now set >>>> $ setpci -s <Dev ID> ECAP0001+0x30.w >>>> 0003 >>>> >>>> # Wait for a while, then clear ERR_COR. A new interrupt immediately >>>> fires. >>>> $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 >>>> pcieport <Dev ID>: AER: Root Error Status 0002 >>>> >>>> Currently, the above issue has been only reproduced in the ICL server >>>> platform. >>>> >>>> [Eric: proposed reproducing steps] >>> Hmm, this differs from the procedure I described on v1, and I don't >>> think will work as described here. >> >> I have attempted to modify the steps to reproduce it without returning >> IRQ_NONE for all cases (which will break the functionality). But I >> think I did not correct the last few steps. > > Well, the thinking in always returning IRQ_NONE was so that only setpci > modified the register and we could clearly see how writes to the > register affect interrupt generation. Got it. Makes sense. > >> How about replacing the last 3 steps with following? >> >> # Inject another error (within 5 seconds) >> $ echo 1 > error_inject >> >> # You will get a new IRQ with only multiple ERR_COR bit set >> pcieport <Dev ID>: AER: Root Error Status 0002 > > This seems accurate. Though it does muddy a detail that I think was > clearer in the original procedure: was the second interrupt triggered by > the second error, or by the write of 0x1 to Root Error Status? I think you are talking about the following command, right? setpci -s <Dev ID> ECAP0001+0x30.w=0x1 If yes, my previously modified instructions already removed it. So no confusion. To summarize, In your case, you have controlled both register read/write of Root error status register to simulate the interrupt with only multi ERR_COR bit set. In my case, I have attempted to simulate it without changing the default behavior of aer_irq() in the kernel. Both seem ok to me. Although my personal preference is to trigger the error without changing the code behavior, if both you and Bjorn prefer to revert to old instructions, I will fix this in the next version. > > Cheers, > Eric
On Tue, Mar 15, 2022 at 02:29:23PM -0700, Sathyanarayanan Kuppuswamy wrote: > On 3/15/22 12:52 PM, Eric Badger wrote: > > On Tue, Mar 15, 2022 at 10:26:46AM -0700, Sathyanarayanan Kuppuswamy wrote: > > > On 3/15/22 10:14 AM, Eric Badger wrote: > > > > > # Prep injection data for a correctable error. > > > > > $ cd /sys/kernel/debug/apei/einj > > > > > $ echo 0x00000040 > error_type > > > > > $ echo 0x4 > flags > > > > > $ echo 0x891000 > param4 > > > > > > > > > > # Root Error Status is initially clear > > > > > $ setpci -s <Dev ID> ECAP0001+0x30.w > > > > > 0000 > > > > > > > > > > # Inject one error > > > > > $ echo 1 > error_inject > > > > > > > > > > # Interrupt received > > > > > pcieport <Dev ID>: AER: Root Error Status 0001 > > > > > > > > > > # Inject another error (within 5 seconds) > > > > > $ echo 1 > error_inject > > > > > > > > > > # No interrupt received, but "multiple ERR_COR" is now set > > > > > $ setpci -s <Dev ID> ECAP0001+0x30.w > > > > > 0003 > > > > > > > > > > # Wait for a while, then clear ERR_COR. A new interrupt immediately > > > > > fires. > > > > > $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 > > > > > pcieport <Dev ID>: AER: Root Error Status 0002 > > > > > > > > > > Currently, the above issue has been only reproduced in the ICL server > > > > > platform. > > > > > > > > > > [Eric: proposed reproducing steps] > > > > Hmm, this differs from the procedure I described on v1, and I don't > > > > think will work as described here. > > > > > > I have attempted to modify the steps to reproduce it without returning > > > IRQ_NONE for all cases (which will break the functionality). But I > > > think I did not correct the last few steps. > > > > Well, the thinking in always returning IRQ_NONE was so that only setpci > > modified the register and we could clearly see how writes to the > > register affect interrupt generation. > > Got it. Makes sense. > > > > > > How about replacing the last 3 steps with following? > > > > > > # Inject another error (within 5 seconds) > > > $ echo 1 > error_inject > > > > > > # You will get a new IRQ with only multiple ERR_COR bit set > > > pcieport <Dev ID>: AER: Root Error Status 0002 > > > > This seems accurate. Though it does muddy a detail that I think was > > clearer in the original procedure: was the second interrupt triggered by > > the second error, or by the write of 0x1 to Root Error Status? > > I think you are talking about the following command, right? > > setpci -s <Dev ID> ECAP0001+0x30.w=0x1 > > If yes, my previously modified instructions already removed it. So > no confusion. The confusion I mention is: "what actually triggers the second interrupt?" Since I can't find a description of the observed behavior in the PCIe spec, I find it interesting to know what's actually happening. Since the procedure we've discussed in this thread stalls in aer_irq(), you can't distinguish clearly which event causes the second interrupt. > > To summarize, > > In your case, you have controlled both register read/write of Root > error status register to simulate the interrupt with only multi > ERR_COR bit set. > > In my case, I have attempted to simulate it without changing the > default behavior of aer_irq() in the kernel. > > Both seem ok to me. Although my personal preference is to trigger > the error without changing the code behavior, if both you and Bjorn > prefer to revert to old instructions, I will fix this in the next version. I think the amended procedure from this thread is fine to demonstrate how to play with the patch. The other procedure is available on the list if anyone has a need for it. Cheers, Eric
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 9fa1f97e5b27..7952e5efd6cf 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -101,6 +101,11 @@ struct aer_stats { #define ERR_COR_ID(d) (d & 0xffff) #define ERR_UNCOR_ID(d) (d >> 16) +#define AER_ERR_STATUS_MASK (PCI_ERR_ROOT_UNCOR_RCV | \ + PCI_ERR_ROOT_COR_RCV | \ + PCI_ERR_ROOT_MULTI_COR_RCV | \ + PCI_ERR_ROOT_MULTI_UNCOR_RCV) + static int pcie_aer_disable; static pci_ers_result_t aer_root_reset(struct pci_dev *dev); @@ -1196,7 +1201,7 @@ static irqreturn_t aer_irq(int irq, void *context) struct aer_err_source e_src = {}; pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status); - if (!(e_src.status & (PCI_ERR_ROOT_UNCOR_RCV|PCI_ERR_ROOT_COR_RCV))) + if (!(e_src.status & AER_ERR_STATUS_MASK)) return IRQ_NONE; pci_read_config_dword(rp, aer + PCI_ERR_ROOT_ERR_SRC, &e_src.id);
Currently the aer_irq() handler returns IRQ_NONE for cases without bits PCI_ERR_ROOT_UNCOR_RCV or PCI_ERR_ROOT_COR_RCV are set. But this assumption is incorrect. Consider a scenario where aer_irq() is triggered for a correctable error, and while we process the error and before we clear the error status in "Root Error Status" register, if the same kind of error is triggered again, since aer_irq() only clears events it saw, the multi-bit error is left in tact. This will cause the interrupt to fire again, resulting in entering aer_irq() with just the multi-bit error logged in the "Root Error Status" register. Repeated AER recovery test has revealed this condition does happen and this prevents any new interrupt from being triggered. Allow to process interrupt even if only multi-correctable (BIT 1) or multi-uncorrectable bit (BIT 3) is set. This error can be reproduced by making following changes to the aer_irq() function and by executing the given test commands. static irqreturn_t aer_irq(int irq, void *context) struct aer_err_source e_src = {}; pci_read_config_dword(rp, aer + PCI_ERR_ROOT_STATUS, &e_src.status); + pci_dbg(pdev->port, "Root Error Status: %04x\n", + e_src.status); if (!(e_src.status & AER_ERR_STATUS_MASK)) return IRQ_NONE; + mdelay(5000); # Prep injection data for a correctable error. $ cd /sys/kernel/debug/apei/einj $ echo 0x00000040 > error_type $ echo 0x4 > flags $ echo 0x891000 > param4 # Root Error Status is initially clear $ setpci -s <Dev ID> ECAP0001+0x30.w 0000 # Inject one error $ echo 1 > error_inject # Interrupt received pcieport <Dev ID>: AER: Root Error Status 0001 # Inject another error (within 5 seconds) $ echo 1 > error_inject # No interrupt received, but "multiple ERR_COR" is now set $ setpci -s <Dev ID> ECAP0001+0x30.w 0003 # Wait for a while, then clear ERR_COR. A new interrupt immediately fires. $ setpci -s <Dev ID> ECAP0001+0x30.w=0x1 pcieport <Dev ID>: AER: Root Error Status 0002 Currently, the above issue has been only reproduced in the ICL server platform. [Eric: proposed reproducing steps] Fixes: 4696b828ca37 ("PCI/AER: Hoist aerdrv.c, aer_inject.c up to drivers/pci/pcie/") Reported-by: Eric Badger <ebadger@purestorage.com> Reviewed-by: Ashok Raj <ashok.raj@intel.com> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- Changes since v1: * Added Fixes tag. * Included reproducing steps proposed by Eric. drivers/pci/pcie/aer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)