On Thu, 06 Feb 2025 20:06:03 +0000, Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > > Hi Marc, > > First off, thanks for reviewing. I'm a bit unsure about some of this > area, which is one reason I sent this patch. Maybe it could have been > "RFC". RFC means nothing to me. Or rather, RFC is a sure indication that a patch can safely be ignored! ;-) My advise on this front is to either post patches as you have done, or not post it at all. > > (See also v1: https://lore.kernel.org/all/Z3ho7eJMWvAy3HHC@xxxxxxxxxx/ > > I'm dealing with HW bugs that cause me to have to configure the output > signal -- msi_ctrl_int -- as EDGE-triggered on my GIC. This is adjacent > to that problem, but doesn't really solve it.) Configuring a level-triggered signal as edge is another recipe for disaster (a sure way to miss interrupts), but short of a description of your particular issue, I can't help on that. > > On Thu, Feb 06, 2025 at 09:04:00AM +0000, Marc Zyngier wrote: > > On Wed, 05 Feb 2025 23:16:36 +0000, > > Brian Norris <briannorris@xxxxxxxxxxxx> wrote: > > > > > > From: Brian Norris <briannorris@xxxxxxxxxx> > > > > > > Per Synopsis's documentation [1], the msi_ctrl_int signal is > > > level-triggered, not edge-triggered. > > > > > > The use of handle_edge_trigger() was chosen in commit 7c5925afbc58 > > > ("PCI: dwc: Move MSI IRQs allocation to IRQ domains hierarchical API"), > > > which actually dropped preexisting use of handle_level_trigger(). > > > Looking at the patch history, this change was only made by request: > > > > > > Subject: Re: [PATCH v6 1/9] PCI: dwc: Add IRQ chained API support > > > https://lore.kernel.org/all/04d3d5b6-9199-218d-476f-c77d04b8d2e7@xxxxxxx/ > > > > > > "Are you sure about this "handle_level_irq"? MSIs are definitely edge > > > triggered, not level." > > > > > > However, while the underlying MSI protocol is edge-triggered in a sense, > > > the DesignWare IP is actually level-triggered. > > > > You are confusing two things: > > > > - MSIs are edge triggered. No ifs, no buts. That's because you can't > > "unwrite" something. Even the so-called level-triggered MSIs are > > build on a pair of edges (one up, one down). > > > > - The DisgustWare IP multiplexes MSIs onto a single interrupt, and > > *latches* them, presenting a level sensitive signal *for the > > latch*. Not for the MSIs themselves. > > Indeed, I probably was a little confused, and distracted by my > aforementioned HW bug, which can be at least partially mitigated by > masking (which this patch does). I also didn't understand the original > choices in various DW-based PCIe drivers, since their choice of > handle_level_irq vs handle_edge_irq seemed a bit arbitrary. > > ... > > > It also breaks the semantics of > > interrupt being made pending while we were handling them (retrigger > > being one). > > What do you mean here? Are you referring to SW state (a la > IRQS_PENDING), or HW state? For HW state, MSIs are accumulated in the > STATUS register even when we're masked, so they'll "retrigger" when > we're done handling. But I'm less clear about some of the IRQ framework > semantics here. IRQS_PENDING is indeed what indicates the SW-driven retrigger state, by which any part of the kernel can decide to reinject an *edge* interrupt if, for any reason, it needs to. This is actually how lazy masking is implemented, by not masking the interrupt, taking it (which is a "consume" operation), realising we were logically masked, masking it for good and marking it as SW-pending for later processing. Hence the while{} loop in handle_edge_irq(). Switching to level triggered removes most of that processing, since by definition, a level is not consumed when acking the interrupt. You need to talk to the end-point for the level to drop, so simply masking the interrupt is enough to get it back when unmasking it. HTH, M. -- Without deviation from the norm, progress is not possible.
