Re: [PATCH v2] PCI/AER: Handle Multi UnCorrectable/Correctable errors properly

[Eric Badger <ebadger@purestorage.com>]

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