linux-kernel - Re: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

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: <CABe3_aE7EjKX1uJbA_r9W_Em3Eu8F4xzdcKh0vfjjJ4YQe7C4Q@mail.gmail.com>
Date: Fri, 14 Nov 2025 10:20:39 -0500
From: Charles Mirabile <cmirabil@...hat.com>
To: "Kazuhiro Abe (Fujitsu)" <fj1078ii@...itsu.com>
Cc: "catalin.marinas@....com" <catalin.marinas@....com>, 
	"Koichi Okuno (Fujitsu)" <fj2767dz@...itsu.com>, "guohanjun@...wei.com" <guohanjun@...wei.com>, 
	"ilkka@...amperecomputing.com" <ilkka@...amperecomputing.com>, "lenb@...nel.org" <lenb@...nel.org>, 
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"lpieralisi@...nel.org" <lpieralisi@...nel.org>, "rafael@...nel.org" <rafael@...nel.org>, 
	"sudeep.holla@....com" <sudeep.holla@....com>, "will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

Hi—

On Fri, Nov 14, 2025 at 3:21 AM Kazuhiro Abe (Fujitsu)
<fj1078ii@...itsu.com> wrote:
>
> Hi Charlie,
>
> > Hi All—
> >
> > On Mon, Nov 10, 2025 at 07:38:17AM +0000, Kazuhiro Abe (Fujitsu) wrote:
> > > Hi Will,
> > >
> > > > Hi Will,
> > > >
> > > > > [You don't often get email from will@...nel.org. Learn why this is
> > > > > important at https://aka.ms/LearnAboutSenderIdentification ]
> > > > >
> > > > > On Mon, Oct 20, 2025 at 09:23:05PM +0800, Hanjun Guo wrote:
> > > > > > On 2025/10/17 15:39, Kazuhiro Abe wrote:
> > > > > > > AGDI has two types of signaling modes: SDEI and interrupt.
> > > > > > > Currently, the AGDI driver only supports SDEI.
> > > > > > > Therefore, add support for interrupt signaling mode The
> > > > > > > interrupt vector is retrieved from the AGDI table, and call
> > > > > > > panic function when an interrupt occurs.
> > > > > > >
> > > > > > > Reviewed-by: Ilkka Koskinen <ilkka@...amperecomputing.com>
> > > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@...jp.fujitsu.com>
> > > > > > > ---
> > > > > > > Hanjun, I have addressed all your comments.
> > > > > > > Please review them.
> > > > > > >
> > > > > > > v3->v4
> > > > > > >   - Add a comment to the flags member.
> > > > > > >   - Fix agdi_interrupt_probe.
> > > > > > >   - Fix agdi_interrupt_remove.
> > > > > > >   - Add space in struct initializsation.
> > > > > > >   - Delete curly braces.
> > > > > >
> > > > > > Looks good to me,
> > > > > >
> > > > > > Acked-by: Hanjun Guo <guohanjun@...wei.com>
> > > > >
> > > > > I wasn't cc'd on the original patch but I couldn't figure out why
> > > > > it uses IRQF_NO_AUTOEN when requesting the irq given that the
> > > > > first thing it does is enable it.
> > > >
> > > > I misunderstood the usage of request_irq and enable_irq.
> > > > Since there's no need to separate them, I will remove IRQF_NO_AUTOEN
> > > > and the enable_irq call, and send v5.
> > >
> > > I found out when calling request_nmi, removing IRQF_NO_AUTOEN results in an
> > error (-EINVAL).
> > > Therefore, I would like to keep IRQF_NO_AUTOEN specified.
> > > If you have any comments on this version, please let me know.
> >
> > Could it be that this is just a bug in `request_nmi`? I see the following:
> >
> > if (!desc || (irq_settings_can_autoenable(desc) &&
> >     !(irqflags & IRQF_NO_AUTOEN)) ||
> >     !irq_settings_can_request(desc) ||
> >     WARN_ON(irq_settings_is_per_cpu_devid(desc)) ||
> >     !irq_supports_nmi(desc))
> >         return -EINVAL;
> >
> > Perhaps there is just a missing `!` before `irq_settings_can_autoenable`.
>
> I tried this change without specifying NO_AUTOEN, but it resulted in an error.
> __setup_irq succeeded, but irq_nmi_setup failed with -EINVAL.
> I haven't investigated further beyond that point yet.

Thank you for trying, from reading the other commentary in the thread,
I recognize now that that was a naive suggestion.

I think that this patch should go in as is then unless there are other
concerns, because it seems like it is not possible and should not be
possible to autoenable NMIs and that explains why the code does what
it does to answer Will's original question.

Whether or not that behavior can or should be changed is a different
concern, but given that all the other callers use IRQ_NOAUTOEN and
later enable the NMI explicitly, I think that should be acceptable
here and a subsequent patch could be made to change that if it becomes
possible to use IRQ_NOAUTOEN later.

>
> Best Regards,
> Kazuhiro Abe
>
> >
> > As far as I can tell it has always been wrong - git blame points me to the original
> > commit where that code was introduced:
> >
> > b525903c254da ("genirq: Provide basic NMI management for interrupt lines")
> >
> > I looked and the only two callers are using `IRQF_NO_AUTOEN` so I guess it just
> > hasn't been noticed yet.
> >
> > Happy to send a patch to fix it.
> >
> > >
> > > Best Regards,
> > > Kazuhiro Abe
> > >
> > > >
> > > > Best Regards,
> > > > Kazuhiro Abe
> > > >
> > > > >
> > > > > Will
> >
> > Best—Charlie
>

Best—Charlie