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

[Brian Norris]

Hi Marc,

On Fri, Feb 07, 2025 at 09:13:32AM +0000, Marc Zyngier wrote:
> On Thu, 06 Feb 2025 20:06:03 +0000,
> Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> > 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.

Haha, OK, I guess you're a Cunningham's Law proponent :)

> > (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),

I'm very well aware of that, but I'm not aware of great alternatives.

> but short of a description
> of your particular issue, I can't help on that.

Sure, I'm not really asking for that, at least not in this forum. I'm
just trying to color the background a bit, that I'm not trying to flip
level/edge settings just for fun.

> > On Thu, Feb 06, 2025 at 09:04:00AM +0000, Marc Zyngier wrote:
> > > 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.

Are you referring to check_irq_resend() and related code? Notably, the
current patch doesn't actually change the result of
irq_settings_is_level() -- the nested descriptor still retains its
"edge" status -- so the "We do not resend level type interrupts" comment
doesn't actually apply.

But anyway, that still suggests my patch is probably the wrong way to go
about things, as it further mixes up "edge" and "level" concepts.

> 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().

Sure, I've familiarized myself with lazy masking. I don't think it
causes any problem here; handle_level_irq simply non-lazily masks it,
and we'll pick up the latched result (if any) again when we eventually
unmask.

> 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.

Ack, thanks.

Brian