Re: [PATCH] PCI/AER: Use IRQF_NO_THREAD on aer_irq

[Crystal Wood <crwood@redhat.com>]

On Thu, 2025-09-04 at 14:48 +0200, Lukas Wunner wrote:
> On Thu, Sep 04, 2025 at 09:30:24AM +0200, Sebastian Andrzej Siewior wrote:
> > On 2025-09-02 17:44:41 [-0500], Crystal Wood wrote:
> > > On PREEMPT_RT, currently both aer_irq and aer_isr run in separate threads,
> > > at the same FIFO priority.  This can lead to the aer_isr thread starving
> > > the aer_irq thread, particularly if multi_error_valid causes a scan of
> > > all devices, and multiple errors are raised during the scan.
> > > 
> > > On !PREEMPT_RT, or if aer_irq runs at a higher priority than aer_isr, these
> > > errors can be queued as single-error events as they happen.  But if aer_irq
> > > can't run until aer_isr finishes, by that time the multi event bit will be
> > > set again, causing a new scan and an infinite loop.
> > 
> > So if aer_irq is too slow we get new "work" pilled up? Is it because
> > there is a timing constrains how long until the error needs to be
> > acknowledged?

The error needs to be cleared before the next error happens, or else
the hardware will set the "Multiple ERR_COR Received" bit.  If that bit
is set, then aer_isr can't rely on the error source ID register, so it
scans through all devices looking for errors -- and for some reason, on
this system, accessing the error registers (or any config space above
0x400, even though there are capabilities located there) generates an
Unsupported Request Error (but returns valid data).

Since this happens more than once, without aer_irq preempting, it
causes another multi error and we get stuck in a loop.

> Since v6.16, AER supports rate limiting.  It's unclear which
> kernel version Crystal is using, but if it's older than v6.16,
> it may be worth retrying with a newer release to see if that
> solves the problem.

The problem shows in top-of-tree.  The messages are ratelimited, but
the problem isn't from the messages.  It still does the scan.

> > Another way would be to let the secondary handler run at a slightly lower
> > priority than the primary handler. In this case making the primary
> > non-threaded should not cause any harm.
> 
> Why isn't the secondary handler always assigned a lower priority
> by default?  I think a lot of drivers are built on the assumption
> that the primary handler is scheduled sooner than the secondary
> handler.

That also works, and I agree it's more intuitive.

> > > +++ b/drivers/pci/pcie/aer.c
> > > @@ -1671,7 +1671,8 @@ static int aer_probe(struct pcie_device *dev)
> > >  	set_service_data(dev, rpc);
> > >  
> > >  	status = devm_request_threaded_irq(device, dev->irq, aer_irq, aer_isr,
> > > -					   IRQF_SHARED, "aerdrv", dev);
> > > +					   IRQF_NO_THREAD | IRQF_SHARED,
> > > +					   "aerdrv", dev);
> > 
> > I'm not sure if this works with IRQF_SHARED. Your primary handler is
> > IRQF_SHARED + IRQF_NO_THREAD and another shared handler which is
> > forced-threaded will have IRQF_SHARED + IRQF_ONESHOT. 
> > If the core does not complain, all good. Worst case might be the shared
> > ONESHOT lets your primary handler starve. It would be nice if you could
> > check if you have shared handler here (I have no aer I three boxes I
> > checked).
> 
> Yes, interrupt sharing can happen if the Root Port uses legacy INTx
> interrupts.  In that case other port services such as hotplug,
> bandwidth control, PME or DPC may use the same interrupt.

It's shared, but with another explicitly threaded interrupt.
This is with the patch applied:
root         778  0.0  0.0      0     0 ?        S    Sep02   0:00 [irq/87-aerdrv]
root         779  0.0  0.0      0     0 ?        S    Sep02   0:00 [irq/87-pciehp]
root         780  0.0  0.0      0     0 ?        S    Sep02   0:00 [irq/87-s-pciehp]

If it were shared with a oneshot irq (forced or otherwise) wouldn't
that have already been a mismatch?

-Crystal