Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 16 Mar 2022 09:27:44 -0700
From:   Eric Badger <ebadger@purestorage.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        Russell Currey <ruscur@russell.cc>,
        Oliver OHalloran <oohall@gmail.com>, linux-pci@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        ebadger@purestorage.com
Subject: Re: [PATCH v2] PCI/AER: Handle Multi UnCorrectable/Correctable
 errors properly
Message-ID: <20220316162744.GA1585319@ebps>
References: <20220315050842.120063-1-sathyanarayanan.kuppuswamy@linux.intel.com>
 <20220315171425.GA1521135@ebps>
 <2d4e8811-dce6-c891-e92d-e3746434685e@linux.intel.com>
 <20220315195255.GA1523195@ebps>
 <f794a3fb-4d3a-7e3d-1600-27ee831526fd@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f794a3fb-4d3a-7e3d-1600-27ee831526fd@linux.intel.com>
Precedence: bulk

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