Re: [PATCH v2] PCI: dwc: Use level-triggered handler for MSI IRQs

[Marc Zyngier]

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.