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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z3ho7eJMWvAy3HHC@google.com>
Date: Fri, 3 Jan 2025 14:47:09 -0800
From: Brian Norris <briannorris@...omium.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Jingoo Han <jingoohan1@...il.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, Rob Herring <robh@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Marc Zyngier <maz@...nel.org>
Subject: Re: [PATCH] PCI: dwc: Use level-triggered handler for MSI IRQs

Hi Bjorn,

On Fri, Jan 03, 2025 at 11:58:39AM -0600, Bjorn Helgaas wrote:
> On Fri, Jan 03, 2025 at 11:49:57AM -0600, Bjorn Helgaas wrote:
> > On Thu, Jan 02, 2025 at 05:43:26PM -0800, Brian Norris wrote:
> > > On Mon, Dec 30, 2024 at 10:41:45PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Oct 15, 2024 at 02:12:16PM -0700, Brian Norris wrote:
> > > > > From: Brian Norris <briannorris@...gle.com>
> > > > > 
> > > > > Per Synopsis's documentation, the msi_ctrl_int signal is
> > > > > level-triggered, not edge-triggered.
> > > > 
> > > > Could you please quote the spec reference?
> > > 
> > > From the reference manual for msi_ctrl_int:
> > > 
> > >   "Asserted when an MSI interrupt is pending. De-asserted when there is
> > >   no MSI interrupt pending.
> > >   ...
> > >   Active State: High (level)"
> > > 
> > > The reference manual also points at the databook for more info. One
> > > relevant excerpt from the databook:
> > > 
> > >   "When any status bit remains set, then msi_ctrl_int remains asserted.
> > >   The interrupt status register provides a status bit for up to 32
> > >   interrupt vectors per Endpoint. When the decoded interrupt vector is
> > >   enabled but is masked, then the controller sets the corresponding bit
> > >   in interrupt status register but the it does not assert the top-level
> > >   controller output msi_ctrl_int.
> > 
> > "the it" might be a transcription error?

Nope, direct copy/paste quote. I unwisely chose not to include a
"[sic]".

> > > That's essentially a prose description of level-triggering, plus
> > > 32-vector multiplexing and masking.
> > > 
> > > Did you want a v2 with this included, or did you just want it noted
> > > here?
> > 
> > I think a v2 with citations (spec name, revision, section number)
> > would be helpful.  Including these quotes as well would be fine with
> > me.

For the record:

DesignWare Cores PCI Express RP Controller Reference Manual
Version 6.00a, June 2022
Section 2.89 MSI Interface Signals

and

DesignWare Cores PCI Express Controller Databook
Version 6.00a, June 2022
Section 3.9.2.3 iMSI-RX: Integrated MSI Receiver [AXI Bridge]

Sure, I can send v2.

> Oh, and it would be awesome if we can motivate this patch by mentioning
> an actual problem it can avoid.
> 
> It sounds like there really *is* a problem at least in some
> topologies, so I think we should describe that problem before
> explaining why we haven't seen it yet.

Yeah, that's probably a good idea ... I'm still working out the nature
of a problem I'm dealing with here, but it has to do with when (due to
HW bugs) I have to configure the parent interrupt (GIC) as
edge-triggered. It turns out this change alone doesn't resolve all my
problems, but:

(a) I was hoping to get feedback on whether this change is sensible
    regardless of the adjacent HW bug I'm dealing with (I think it is);
    and
(b) I don't think I have a great publishable explanation of my HW bug(s)
    yet.

I understand (b) is not really a great situation for public review and
would understand if that delays/defers any action here. But I'm also not
really an IRQ expert (though I have to dabble quite a lot) and am
interested in (a) still.

(If it helps, I can try to summarize the above in a commit message, even
if it's still a bit vague.)

Brian

> > > (Side note: I think it doesn't really matter that much whether we use
> > > the 'level' or 'edge' variant handlers here, at least if the parent
> > > interrupt is configured correctly as level-triggered. We're not actually
> > > in danger of a level-triggered interrupt flood or similar issue.)