| 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
| ||
|
Message-ID:
<TY3PR01MB998368496A0A51E1C81FA47FD51BA@TY3PR01MB9983.jpnprd01.prod.outlook.com>
Date: Mon, 29 Sep 2025 07:23:04 +0000
From: "Kazuhiro Abe (Fujitsu)" <fj1078ii@...itsu.com>
To: 'Hanjun Guo' <guohanjun@...wei.com>, Lorenzo Pieralisi
<lpieralisi@...nel.org>, Sudeep Holla <sudeep.holla@....com>, "Rafael J.
Wysocki" <rafael@...nel.org>, Len Brown <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>
CC: Ilkka Koskinen <ilkka@...amperecomputing.com>
Subject: RE: [PATCH v3] ACPI: AGDI: Add interrupt signaling mode support
Hi Hanjun,
Thank you for your review.
> Hi Kazuhiro,
>
> Sorry for the late reply, some comments below.
>
> On 2025/9/5 12:27, 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.
> >
> > Signed-off-by: Kazuhiro Abe <fj1078ii@...jp.fujitsu.com>
> > ---
> > I keep normal IRQ code when NMI cannot be used.
> > If there is any concern, please let me know.
> >
> > 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.fu
> > jitsu.com/
> > v1->v2
> > - Remove acpica update since there is no need to update define value
> > for this patch.
> > ---
> > drivers/acpi/arm64/agdi.c | 95
> ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 88 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/acpi/arm64/agdi.c b/drivers/acpi/arm64/agdi.c
> > index e0df3daa4abf..2313a46f01cd 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;
>
> Adding a comment here for what's the falgs used for, multi flags in this file such as
> irq_flags, just make the code easy to understand.
Understood.
I will add the following comment:
unsigned char flags; /* AGDI Signaling Mode (0=SDEI-based, 1=Interrupt-based) */
>
> > 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,55 @@ 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) {
> > + 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;
> > + } else {
> > + enable_nmi(irq);
> > + adata->irq = irq;
> > + adata->use_nmi = true;
> > + }
>
> if (!ret) {
> /* NMI handling code */
> }
>
> /* Then try normal interrupt */
> ret = request_irq();
> ...
>
> This makes code better organized.
Understood. I will fix it for better organization as you suggested.
>
> > +
> > + return 0;
> > +}
> > +
> > static int agdi_probe(struct platform_device *pdev)
> > {
> > struct agdi_data *adata = dev_get_platdata(&pdev->dev); @@ -55,12
> > +108,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 +139,29 @@ 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) {
> > + if (adata->use_nmi)
> > + free_nmi(adata->irq, NULL);
> > + else
> > + free_irq(adata->irq, NULL);
> > +
> > + acpi_unregister_gsi(adata->gsiv);
> > + }
>
> if (adata->irq == -1)
> return;
>
> ...
>
> To save extra { }.
Understood. I will fix it as suggested.
>
> > +}
> > +
> > +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 +173,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 agdi_data pdata = { 0 };
Understood, I'll do that.
Best Regards,
Kazuhiro Abe
>
> Thanks
> Hanjun
Powered by blists - more mailing lists