Re: [PATCH 1/1] PCI: dwc: Use regular interrupt instead of chained

[Radu Rendec]

On Thu, 2023-06-29 at 17:11 -0500, Bjorn Helgaas wrote:
> [+cc Pali]
> 
> On Thu, Jun 29, 2023 at 05:27:54PM -0400, Radu Rendec wrote:
> > On Thu, 2023-06-29 at 15:58 -0500, Bjorn Helgaas wrote:
> > > On Thu, Jun 29, 2023 at 04:42:07PM -0400, Radu Rendec wrote:
> > > > On Thu, 2023-06-29 at 14:57 -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Jun 29, 2023 at 02:30:19PM -0400, Radu Rendec wrote:
> > > > > > The DesignWare PCIe host driver uses a chained interrupt to demultiplex
> > > > > > the downstream MSI interrupts. On Qualcomm SA8540P Ride, enabling both
> > > > > > pcie2a and pcie3a at the same time can create an interrupt storm where
> > > > > > the parent interrupt fires continuously, even though reading the PCIe
> > > > > > host registers doesn't identify any child MSI interrupt source. This
> > > > > > effectively locks up CPU0, which spends all the time servicing these
> > > > > > interrupts.
> > > > > > 
> > > > > > This is a clear example of how bypassing the interrupt core by using
> > > > > > chained interrupts can be very dangerous if the hardware misbehaves.
> > > > > > 
> > > > > > Convert the driver to use a regular interrupt for the demultiplex
> > > > > > handler. This allows the interrupt storm detector to detect the faulty
> > > > > > interrupt and disable it, allowing the system to run normally.
> 
> > ...
> > > But you do imply that there's some DesignWare hardware issue involved,
> > > too, so I guess it's possible the other drivers don't have an issue
> > > and/or actually require the chained IRQs.  That's why I asked how we
> > > should decide.
> > 
> > That makes sense. I don't know if it's a DesignWare hardware issue or
> > something else down the PCIe bus. Other folks in my team are
> > investigating the root cause. This thread has the details:
> > https://lore.kernel.org/all/pmodcoakbs25z2a7mlo5gpuz63zluh35vbgb5itn6k5aqhjnny@jvphbpvahtse/
> > 
> > I want to point out that this patch doesn't *fix* the root problem (the
> > interrupt storm). It just makes the kernel handle it (much) better if
> > it occurs.
> > 
> > I can't see why any driver would _require_ chained IRQs. As I see it,
> > chained interrupts are just a "shortcut" that circumvents the IRQ core
> > for (debatable) performance reasons. Other than that, they should work
> > the same as regular interrupts.
> > 
> > There are other benefits associated with using regular interrupts. One
> > of them is the ability to control the SMP affinity of the parent
> > interrupt. But that's a different topic.
> 
> Thanks for mentioning IRQ affinity because that reminds me of a
> long-standing related issue.  Pali posted a similar patch for mvebu
> [1], but it's been stalled for a long time because apparently there's
> some potential breakage of the userspace ABI [2].

I'm really glad you brought this up. Ironically, that's exactly where I
started.

> Unfortunately I'm not an IRQ expert and haven't followed all the
> arguments there.  BUT it does seem like Marc's objection to Pali's
> patch would also apply to your patch, so maybe this is an opportune
> time to re-evaluate the situation.

I'm no IRQ expert either, but I've been looking into it quite a lot
lately. For the record, there's an even older thread [3] where Marc
explains the userspace ABI breakage you mentioned and he proposes a new
sysfs based interface to address that problem.

I tried to follow Marc's suggestions and sent an RFC patch series [4]
that attempts to implement the new interface and also some form of
parent-child interrupt tracking. That would be a way of addressing the
affinity issue without having to convert all the drivers. But on the
other hand, it doesn't solve the IRQ storm detection problem.

Anyway, Thomas replied [5] and explained why chained interrupts are a
bad idea altogether. When we (my team and I ) came across the IRQ storm
issue, we experienced first hand the scenario that Thomas describes.

> If converting from chained to normal handlers can be done safely, I
> would definitely be in favor if doing it across all of drivers/pci/ at
> once so they're all consistent.  Otherwise that code just gets copied
> to new drivers, so the issue persists and spreads.

I think the conversion can be done safely, meaning that it won't break
the drivers. And by the way, there are other IRQ drivers (outside the
PCI space) that use chained interrupts.

Unfortunately, it seems we are going in circles. Chained interrupts are
bad because they let IRQ storms go unnoticed and lock up the system,
but converting them to regular interrupts is also bad because it breaks
the userspace ABI.

I am willing to help clean up this mess, but I think first we need to
come up with a solution that's acceptable for everybody. I was hoping
Marc and Thomas would chime in, but unfortunately that hasn't happened
yet - other than each of them pointing out (separately) what is wrong
with each approach.

Radu

> [1] https://lore.kernel.org/r/20220524122817.7199-1-pali@xxxxxxxxxx
> [2] https://lore.kernel.org/r/874k0bf7f7.wl-maz@xxxxxxxxxx
[3] https://lore.kernel.org/all/87fslr7ygl.wl-maz@xxxxxxxxxx/
[4] https://lore.kernel.org/all/20230530214550.864894-1-rrendec@xxxxxxxxxx/
[5] https://lore.kernel.org/all/877csohcll.ffs@tglx/