Re: Interrupts in pci-aardvark

[Marc Zyngier <maz@kernel.org>]

On Wed, 31 Mar 2021 12:03:14 +0100,
Pali Rohár <pali@kernel.org> wrote:
> 
> On Wednesday 31 March 2021 11:25:43 Marc Zyngier wrote:
> > On Wed, 31 Mar 2021 10:56:59 +0100,
> > Pali Rohár <pali@kernel.org> wrote:
> > > 
> > > On Tuesday 30 March 2021 14:21:47 Marc Zyngier wrote:
> > > > On Sun, 28 Mar 2021 15:09:12 +0100,
> > > > Pali Rohár <pali@kernel.org> wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > Aardvark HW allows to mask summary TOP, summary CORE, individual CORE
> > > > > (PME, ERR, INTA, INTB, ...), summary MSI and individual MSI bits
> > > > > interrupts, but not final 16 bit MSI interrupt number. MSI bits are low
> > > > > 5 bits of 16 bit interrupt number. So it is not possible to mask or
> > > > > unmask MSI interrupt number X. It is possible to only mask/unmask all
> > > > > MSI interrupts which low 5 bits is specific value.
> > > > 
> > > > If you cannot mask individual MSIs, you have two choices:
> > > > 
> > > > - you only support MSI-X *or* MSI (not multi-MSI) and mask interrupts
> > > >   at the device level
> > > 
> > > There is no information in available documentation how to implement
> > > MSI-X in Root Complex mode, so currently MSI-X is not possible.
> > > 
> > > > - you restrict the number of MSIs to those you can actually control,
> > > >   and that's 2^5 = 32 (which is what the driver currently supports, I
> > > >   believe).
> > > 
> > > Well, 32 MSI interrupts is not enough for new modern wifi cards. E.g.
> > > QCA6390 (ath11k) wifi card requires 32 interrupts and when this card is
> > > connected to 2 port PCIe packet switch then packet switch requires 3
> > > additional interrupts (1 for downstream and 1 for each upstream = 1+2).
> > > So in this setup at least 64 MSI interrupts are required because PCI
> > > functions of packet switch are initialized first (take interrupts 0,1,2)
> > > and then is initialized wifi card which requires 32 aligned interrupts
> > > (so 32,33,...,63). This setup is common on existing A3720 hardware:
> > > Turris Mox with Mox G module.
> > 
> > The fact that it is common doesn't make it less broken. Also, 32
> > interrupts from the same device, all squashed behind a single global
> > IRQ seems pretty unhelpful.
> 
> Well, we cannot do anything with it. Qualcomm has probably decided to
> design their new AX cards in this way and current ath11k kernel driver
> requires 32 interrupts.
> 
> The only way what can be done is to provide 32 interrupts -- what driver
> and hardware of these new AX cards requires.

Which is fine. Using per-queue interrupts is standard procedure if you
want any kind of performance. But it also assume that the HW this is
plugged in is not braindead. Unfortunately, the Marvell stuff ticks
all the boxes for being a terrible implementation.

>
> > > Currently aardvark driver unmask all MSI interrupts and does not
> > > implement masking/unmasking callbacks for individual interrupts.
> > > Currently it has implemented support for 32 individual interrupts.
> > > 
> > > So it is an issue if masking / unmasking stay unimplemented?
> > 
> > A driver is allowed to call disable_irq() at any time. Not
> > implementing this breaks drivers. You may not care for a particular
> > application, but that is still broken in general.
> > 
> > Of course, you could rely on the device only supporting Multi-MSI
> > (which is what you seem to have), in which case you can disable
> > interrupts for the whole device. Goodbye performance.
> 
> In the worst case, interrupt can be masked in software. Yes, it
> decreases performance but now when all MSI and INTx interrupts are
> squashed behind one single interrupt (which is design / limitation of
> Armada 3720) it could not be worse.
> 
> Anyway, hardware allows to mask group of interrupts which same low 5bit
> bits. So these interrupts can be put into equivalence class based on low
> 5 bits.
> 
> So at least something can be done. E.g. when all connected devices
> require maximally 32 interrupts. Or when some devices require Multi-MSI
> without using whole Multi mask and some other devices just individual
> interrupts, then these individual interrupts can be put into different
> equivalence class. Which then allows masking some of them.

You could do that. It will require you to refcount the
enabling/disabling of interrupts, but that's not too hard to get
right. My concern is more if *one* interrupt gets disabled, and that
the driver still expects other interrupts to fire...

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.