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

[Brian Norris <briannorris@chromium.org>]

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

(See also v1: https://lore.kernel.org/all/Z3ho7eJMWvAy3HHC@google.com/

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

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@chromium.org> wrote:
> > 
> > From: Brian Norris <briannorris@google.com>
> > 
> > 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@arm.com/
> > 
> >   "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.

All in all, I'm OK with dropping this patch, but I'd like to understand
a little more of what you think breaks here.

Thanks again,
Brian