On Wed, 31 Mar 2021 10:56:59 +0100, Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx> 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. > > 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. > > > > Also aardvark HW allows to globally enable / disable processing of MSI > > > interrupts. Masking summary MSI interrupt just cause that GIC does not > > > trigger it but from registers I can read it (e.g. when GIC calls > > > aardvark interrupt handler for other non-MSI interrupt). > > > > > > And I would like to ask, what is in this hierarchy from kernel point of > > > view "bottom part of MSI" and what is the "upper part of MSI"? As in > > > above diagram there are 3 MSI layers. > > > > The upper part is the bus-specific part, PCI in your case. You don't > > need to implement it. > > Ok! > > > The bottom part controls the HW, and deals with all the masking, > > acknoledgement, allocation and demuxing. > > So it is basically everything related to MSI interrupts in that diagram. > > > > And which irq enable/disable/mask/unmask/ack callbacks I need to > > > implement for legacy irq, bottom MSI and upper MSI domains? > > > > You need to provide what makes sense for your HW. I would guess that > > you need at least mask/unmask and most probably ack at both levels, > > and of course a compose_msg callback at the bottom level. > > > > > And where should I add code which globally enable/disable receiving of > > > aardvark MSI interrupts? Currently it is part of aardvark driver probe > > > function. > > > > Seems like the logical place to put it. The kernel deals with > > individual interrupts, and not with global switches. > > Global enable is needed to call also when HW resume from suspend state. > Suspend is not currently implemented, but I would like to know what is > the preferred way, where to put "MSI initialization code" which needs > to be called for "global initialization & enable" and also after wakeup > from suspend. Has struct irq_chip callback for this action? Or such code > really should be called in driver probe function and then also in driver > resume function? > The irq_chip only deals with individual interrupts, and has no callbacks for global settings. You will have to keep your global settings for things that are global, such as probe and resume. M. -- Without deviation from the norm, progress is not possible.
