[PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe]

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@os.amperecomputing.com>
Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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.

v3: https://lore.kernel.org/all/20250905042751.945616-1-fj1078ii@aa.jp.fujitsu.com/
v2->v3
 - Fix bug in the return value of agdi_probe function.
 - Remove unnecessary curly braces in the agdi_remove function.

v2: https://lore.kernel.org/all/20250829101154.2377800-1-fj1078ii@aa.jp.fujitsu.com/
v1->v2
 - Remove acpica update since there is no need to update define value
   for this patch.
---
 drivers/acpi/arm64/agdi.c | 101 ++++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/arm64/agdi.c b/drivers/acpi/arm64/agdi.c
index e0df3daa4abf..feb4b2cb4618 100644
--- a/drivers/acpi/arm64/agdi.c
+++ b/drivers/acpi/arm64/agdi.c
@@ -16,7 +16,11 @@
 #include "init.h"
 
 struct agdi_data {
+	unsigned char flags; /* AGDI Signaling Mode */
 	int sdei_event;
+	unsigned int gsiv;
+	bool use_nmi;
+	int irq;
 };
 
 static int agdi_sdei_handler(u32 sdei_event, struct pt_regs *regs, void *arg)
@@ -48,6 +52,57 @@ static int agdi_sdei_probe(struct platform_device *pdev,
 	return 0;
 }
 
+static irqreturn_t agdi_interrupt_handler_nmi(int irq, void *dev_id)
+{
+	nmi_panic(NULL, "Arm Generic Diagnostic Dump and Reset NMI Interrupt event issued\n");
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t agdi_interrupt_handler_irq(int irq, void *dev_id)
+{
+	panic("Arm Generic Diagnostic Dump and Reset Interrupt event issued\n");
+	return IRQ_HANDLED;
+}
+
+static int agdi_interrupt_probe(struct platform_device *pdev,
+				struct agdi_data *adata)
+{
+	unsigned long irq_flags;
+	int ret;
+	int irq;
+
+	irq = acpi_register_gsi(NULL, adata->gsiv, ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_HIGH);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "cannot register GSI#%d (%d)\n", adata->gsiv, irq);
+		return irq;
+	}
+
+	irq_flags = IRQF_PERCPU | IRQF_NOBALANCING | IRQF_NO_AUTOEN |
+		    IRQF_NO_THREAD;
+	/* try NMI first */
+	ret = request_nmi(irq, &agdi_interrupt_handler_nmi, irq_flags,
+			  "agdi_interrupt_nmi", NULL);
+	if (!ret) {
+		enable_nmi(irq);
+		adata->irq = irq;
+		adata->use_nmi = true;
+		return 0;
+	}
+
+	/* Then try normal interrupt */
+	ret = request_irq(irq, &agdi_interrupt_handler_irq,
+			  irq_flags, "agdi_interrupt_irq", NULL);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register IRQ %d\n", ret);
+		acpi_unregister_gsi(adata->gsiv);
+		return ret;
+	}
+	enable_irq(irq);
+	adata->irq = irq;
+
+	return 0;
+}
+
 static int agdi_probe(struct platform_device *pdev)
 {
 	struct agdi_data *adata = dev_get_platdata(&pdev->dev);
@@ -55,12 +110,15 @@ static int agdi_probe(struct platform_device *pdev)
 	if (!adata)
 		return -EINVAL;
 
-	return agdi_sdei_probe(pdev, adata);
+	if (adata->flags & ACPI_AGDI_SIGNALING_MODE)
+		return agdi_interrupt_probe(pdev, adata);
+	else
+		return agdi_sdei_probe(pdev, adata);
 }
 
-static void agdi_remove(struct platform_device *pdev)
+static void agdi_sdei_remove(struct platform_device *pdev,
+			     struct agdi_data *adata)
 {
-	struct agdi_data *adata = dev_get_platdata(&pdev->dev);
 	int err, i;
 
 	err = sdei_event_disable(adata->sdei_event);
@@ -83,6 +141,30 @@ static void agdi_remove(struct platform_device *pdev)
 			adata->sdei_event, ERR_PTR(err));
 }
 
+static void agdi_interrupt_remove(struct platform_device *pdev,
+				  struct agdi_data *adata)
+{
+	if (adata->irq == -1)
+		return;
+
+	if (adata->use_nmi)
+		free_nmi(adata->irq, NULL);
+	else
+		free_irq(adata->irq, NULL);
+
+	acpi_unregister_gsi(adata->gsiv);
+}
+
+static void agdi_remove(struct platform_device *pdev)
+{
+	struct agdi_data *adata = dev_get_platdata(&pdev->dev);
+
+	if (adata->flags & ACPI_AGDI_SIGNALING_MODE)
+		agdi_interrupt_remove(pdev, adata);
+	else
+		agdi_sdei_remove(pdev, adata);
+}
+
 static struct platform_driver agdi_driver = {
 	.driver = {
 		.name = "agdi",
@@ -94,7 +176,7 @@ static struct platform_driver agdi_driver = {
 void __init acpi_agdi_init(void)
 {
 	struct acpi_table_agdi *agdi_table;
-	struct agdi_data pdata;
+	struct agdi_data pdata = { 0 };
 	struct platform_device *pdev;
 	acpi_status status;
 
@@ -103,12 +185,13 @@ void __init acpi_agdi_init(void)
 	if (ACPI_FAILURE(status))
 		return;
 
-	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
-		pr_warn("Interrupt signaling is not supported");
-		goto err_put_table;
-	}
+	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
+		pdata.gsiv = agdi_table->gsiv;
+	else
+		pdata.sdei_event = agdi_table->sdei_event;
 
-	pdata.sdei_event = agdi_table->sdei_event;
+	pdata.irq = -1;
+	pdata.flags = agdi_table->flags;
 
 	pdev = platform_device_register_data(NULL, "agdi", 0, &pdata, sizeof(pdata));
 	if (IS_ERR(pdev))
-- 
2.43.0

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

[Hanjun Guo]

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@os.amperecomputing.com>
> Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.com>

Thanks
Hanjun

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

[Will Deacon]

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@os.amperecomputing.com>
> > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

Will

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

Hi Will,

> [You don't often get email from will@kernel.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@os.amperecomputing.com>
> > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

Best Regards,
Kazuhiro Abe

> 
> Will

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

Hi Will,

> Hi Will,
> 
> > [You don't often get email from will@kernel.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@os.amperecomputing.com>
> > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

Best Regards,
Kazuhiro Abe

> 
> Best Regards,
> Kazuhiro Abe
> 
> >
> > Will

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

[Charles Mirabile]

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@kernel.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@os.amperecomputing.com>
> > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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`.

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

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

Hi Charlie,
Thank you for your comments.

> 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@kernel.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@os.amperecomputing.com>
> > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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`.
> 
> 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.

I noticed that commit (b525903c254da) log says:
>  Interrupt lines allocated for NMI delivery must be enabled/disabled through
>  enable_nmi/disable_nmi_nosync to keep their state consistent.

Similar comments also exist in the function comments of request_nmi().
So, I wonder if disabling auto-enabling for request_nmi() is a design choice.

As far as I can see, there is no fundamental reason to restrict the flag.
If we agree to remove the restriction for request_nmi(),  I'm willing to update this patch.
I'd like to know others' opinions.

Best Regards,
Kazuhiro Abe
> 
> >
> > Best Regards,
> > Kazuhiro Abe
> >
> > >
> > > Best Regards,
> > > Kazuhiro Abe
> > >
> > > >
> > > > Will
> 
> Best—Charlie

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

[Lorenzo Pieralisi]

[+cc Thomas, Marc]

On Tue, Nov 11, 2025 at 11:42:37PM -0500, Charles Mirabile wrote:
> 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@kernel.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@os.amperecomputing.com>
> > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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`.
> 
> 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 don't think that's the right rationale (and I can't claim to understand
it fully either - that's why I added Thomas/Marc in CC).

As per b525903c254da - IIUC NMI must not be autoenable-capable -
enable/disable must be done using enable_nmi/disable_nmi_nosync explicitly.

cbe16f35bee6 ("genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()")
added the logic checking the IRQF_NO_AUTOEN flag to request_nmi().

Now, AFAICS irq_settings_can_autoenable(desc) returns true in this
specific case - the IRQ is a GICv3 SPI.

First question is - before commit cbe16f35bee6, how request_nmi() could
have possibly succeeded - irq_settings_can_autoenable() would return
true and request_nmi() would have failed.

Come cbe16f35bee6:

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;

Now the check passes _if_ you pass in irqflags IRQF_NO_AUTOEN.

I agree with Will that's a bit counterintuitive - maybe we can force
the irqdesc to be NOAUTOEN in request_nmi() directly ?

Thomas and Marc know better so that's where I stop.

Thanks,
Lorenzo

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

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

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@kernel.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@os.amperecomputing.com>
> > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

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

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

[Charles Mirabile]

Hi—

On Fri, Nov 14, 2025 at 3:21 AM Kazuhiro Abe (Fujitsu)
<fj1078ii@fujitsu.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@kernel.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@os.amperecomputing.com>
> > > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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

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

[Will Deacon]

On Fri, Nov 14, 2025 at 10:20:39AM -0500, Charles Mirabile wrote:
> On Fri, Nov 14, 2025 at 3:21 AM Kazuhiro Abe (Fujitsu)
> <fj1078ii@fujitsu.com> wrote:
> > > On Mon, Nov 10, 2025 at 07:38:17AM +0000, Kazuhiro Abe (Fujitsu) wrote:
> > > > > > 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@os.amperecomputing.com>
> > > > > > > > Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

Yes, I wasn't trying to derail this at all. I spotted something that
looked weird, it turns out that's how NMIs are enabled and if the
cleanest thing is to treat normal IRQs the same way then so be it.

Will

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

[Hanjun Guo]

On 2025/11/5 0:06, Will Deacon wrote:
> 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@os.amperecomputing.com>
>>> Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.

Sorry for the late reply, seems this comment was addressed.

But when I go back to read the latest AGDI spec [0], I see
the signaling mode can be SDEI *and* interrupt:

Flags node structures 1 36 Bits [1:0]: Signaling mode:

2: Both SDEI and Interrupt-based signaling is
supported. While an SDEI event handler is
registered, the platform is allowed to not
generate the wired interrupt

So which means we need to support both SDEI and interrupt when the
signaling mode is 2, but for now, the code is

+	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
+		pdata.gsiv = agdi_table->gsiv;
+	else
+		pdata.sdei_event = agdi_table->sdei_event;

Kazuhiro, could you take a look again?

Thanks
Hanjun

[0]: https://developer.arm.com/documentation/den0093/latest/

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

Hi Hanjun,

Thanks for your comments.

> >>> 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@os.amperecomputing.com>
> >>> Signed-off-by: Kazuhiro Abe <fj1078ii@aa.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@huawei.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.
> 
> Sorry for the late reply, seems this comment was addressed.
> 
> But when I go back to read the latest AGDI spec [0], I see the signaling mode
> can be SDEI *and* interrupt:
> 
> Flags node structures 1 36 Bits [1:0]: Signaling mode:
> 
> 2: Both SDEI and Interrupt-based signaling is supported. While an SDEI event
> handler is registered, the platform is allowed to not generate the wired interrupt
> 
> So which means we need to support both SDEI and interrupt when the
> signaling mode is 2, but for now, the code is
> 
> +	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
> +		pdata.gsiv = agdi_table->gsiv;
> +	else
> +		pdata.sdei_event = agdi_table->sdei_event;
> 
> Kazuhiro, could you take a look again?

I don't think enabling both interrupt and SDEI makes sense.
Therefore, we need to choose one but we can't know which one is preferred for each platform.
On the other hand, before this patch the code is:

       if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
               pr_warn("Interrupt signaling is not supported");
               goto err_put_table;
       }

...and SDEI is used when mode is 2. This behavior will not be changed by this patch.
Therefore, It seems for me this patch is reasonable for now.
I would appreciate your thoughts on this.

Best Regards,
Kazuhiro Abe

> 
> Thanks
> Hanjun
> 
> [0]: https://developer.arm.com/documentation/den0093/latest/

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

[Hanjun Guo]

On 2025/11/18 18:09, Kazuhiro Abe (Fujitsu) wrote:
> Hi Hanjun,
> 
> Thanks for your comments.
> 
>>>>> 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@os.amperecomputing.com>
>>>>> Signed-off-by: Kazuhiro Abe<fj1078ii@aa.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@huawei.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.
>> Sorry for the late reply, seems this comment was addressed.
>>
>> But when I go back to read the latest AGDI spec [0], I see the signaling mode
>> can be SDEI*and*  interrupt:
>>
>> Flags node structures 1 36 Bits [1:0]: Signaling mode:
>>
>> 2: Both SDEI and Interrupt-based signaling is supported. While an SDEI event
>> handler is registered, the platform is allowed to not generate the wired interrupt
>>
>> So which means we need to support both SDEI and interrupt when the
>> signaling mode is 2, but for now, the code is
>>
>> +	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
>> +		pdata.gsiv = agdi_table->gsiv;
>> +	else
>> +		pdata.sdei_event = agdi_table->sdei_event;
>>
>> Kazuhiro, could you take a look again?
> I don't think enabling both interrupt and SDEI makes sense.
> Therefore, we need to choose one but we can't know which one is preferred for each platform.
> On the other hand, before this patch the code is:
> 
>         if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
>                 pr_warn("Interrupt signaling is not supported");
>                 goto err_put_table;
>         }
> 
> ...and SDEI is used when mode is 2. This behavior will not be changed by this patch.
> Therefore, It seems for me this patch is reasonable for now.
> I would appreciate your thoughts on this.

I had a talk to firmware guy about this, they say even with both
supported, the platform will use the same interrupt number to
trigger, and the spec says:

While an SDEI event handler is registered, the platform is allowed to
not generate the wired interrupt

I think this patch is ready to go.

Thanks
Hanjun

RE: [PATCH v4] ACPI: AGDI: Add interrupt signaling mode support

[Kazuhiro Abe (Fujitsu)]

Hi Hanjun and Will,

> On 2025/11/18 18:09, Kazuhiro Abe (Fujitsu) wrote:
> > Hi Hanjun,
> >
> > Thanks for your comments.
> >
> >>>>> 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@os.amperecomputing.com>
> >>>>> Signed-off-by: Kazuhiro Abe<fj1078ii@aa.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@huawei.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.
> >> Sorry for the late reply, seems this comment was addressed.
> >>
> >> But when I go back to read the latest AGDI spec [0], I see the
> >> signaling mode can be SDEI*and*  interrupt:
> >>
> >> Flags node structures 1 36 Bits [1:0]: Signaling mode:
> >>
> >> 2: Both SDEI and Interrupt-based signaling is supported. While an
> >> SDEI event handler is registered, the platform is allowed to not
> >> generate the wired interrupt
> >>
> >> So which means we need to support both SDEI and interrupt when the
> >> signaling mode is 2, but for now, the code is
> >>
> >> +	if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE)
> >> +		pdata.gsiv = agdi_table->gsiv;
> >> +	else
> >> +		pdata.sdei_event = agdi_table->sdei_event;
> >>
> >> Kazuhiro, could you take a look again?
> > I don't think enabling both interrupt and SDEI makes sense.
> > Therefore, we need to choose one but we can't know which one is preferred
> for each platform.
> > On the other hand, before this patch the code is:
> >
> >         if (agdi_table->flags & ACPI_AGDI_SIGNALING_MODE) {
> >                 pr_warn("Interrupt signaling is not supported");
> >                 goto err_put_table;
> >         }
> >
> > ...and SDEI is used when mode is 2. This behavior will not be changed by this
> patch.
> > Therefore, It seems for me this patch is reasonable for now.
> > I would appreciate your thoughts on this.
> 
> I had a talk to firmware guy about this, they say even with both supported, the
> platform will use the same interrupt number to trigger, and the spec says:
> 
> While an SDEI event handler is registered, the platform is allowed to not
> generate the wired interrupt
> 
> I think this patch is ready to go.

Hanjun, Thank you for your review.

Will, as all comments have been addressed, please consider applying this patch.

Best Regards,
Kazuhiro Abe

> 
> Thanks
> Hanjun