diff mbox series

ACPI: GED: Support GpioInt event resources

Message ID 20260930111505.1108-1-theflyingrahul@gmail.com
State New
Headers show
Series ACPI: GED: Support GpioInt event resources | expand

Commit Message

Rahul Pon Sept. 30, 2026, 11:15 a.m. UTC 
The Generic Event Device accepts only Interrupt and ExtendedInterrupt
resources in _CRS. Some ACPI firmware signals GED events through GPIO
controllers instead: the HP OmniBook 5 16-bf1xxx (Snapdragon X2) describes
its lid (LIGE) and embedded-controller event device (ECGE) as ACPI0013 with
GpioInt resources, and both currently fail to probe with "unable to parse
IRQ resource".

Handle GpioInt resources: get the Linux IRQ with acpi_dev_gpio_irq_get(),
which applies the trigger and polarity and returns -EPROBE_DEFER until the
GPIO controller is registered, and evaluate _EVT with the GPIO pin number,
as done for GSIs above 255. GpioIo resources are skipped. On a failed walk,
free the events already requested and propagate the GPIO error (including
-EPROBE_DEFER) instead of -EINVAL.

Tested on the HP OmniBook 5 16-bf1xxx: lid close/open run _EVT and reach
the ACPI button driver and logind.

Assisted-by: LLM
Signed-off-by: Rahul Pon <theflyingrahul@gmail.com>
---
 drivers/acpi/evged.c | 46 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)


base-commit: 551c722f40809618230001baccf219193e22fc5a

Comments

Linus Walleij Sept. 30, 2026, 8:22 p.m. UTC | #1 
On Wed, Sep 30, 2026 at 1:16 PM Rahul Pon <theflyingrahul@gmail.com> wrote:

> The Generic Event Device accepts only Interrupt and ExtendedInterrupt
> resources in _CRS. Some ACPI firmware signals GED events through GPIO
> controllers instead: the HP OmniBook 5 16-bf1xxx (Snapdragon X2) describes
> its lid (LIGE) and embedded-controller event device (ECGE) as ACPI0013 with
> GpioInt resources, and both currently fail to probe with "unable to parse
> IRQ resource".
>
> Handle GpioInt resources: get the Linux IRQ with acpi_dev_gpio_irq_get(),
> which applies the trigger and polarity and returns -EPROBE_DEFER until the
> GPIO controller is registered, and evaluate _EVT with the GPIO pin number,
> as done for GSIs above 255. GpioIo resources are skipped. On a failed walk,
> free the events already requested and propagate the GPIO error (including
> -EPROBE_DEFER) instead of -EINVAL.
>
> Tested on the HP OmniBook 5 16-bf1xxx: lid close/open run _EVT and reach
> the ACPI button driver and logind.
>
> Assisted-by: LLM
> Signed-off-by: Rahul Pon <theflyingrahul@gmail.com>

I don't understand the patch at all, but I understand that Andy or Mika must
review it otherwise it's not going anywhere, so looping them in.

Yours,
Linus Walleij
Mika Westerberg Oct. 1, 2026, 4:04 a.m. UTC | #2 
Hi,

On Wed, Sep 30, 2026 at 10:22:22PM +0200, Linus Walleij wrote:
> On Wed, Sep 30, 2026 at 1:16 PM Rahul Pon <theflyingrahul@gmail.com> wrote:
> 
> > The Generic Event Device accepts only Interrupt and ExtendedInterrupt
> > resources in _CRS. Some ACPI firmware signals GED events through GPIO
> > controllers instead: the HP OmniBook 5 16-bf1xxx (Snapdragon X2) describes
> > its lid (LIGE) and embedded-controller event device (ECGE) as ACPI0013 with
> > GpioInt resources, and both currently fail to probe with "unable to parse
> > IRQ resource".
> >
> > Handle GpioInt resources: get the Linux IRQ with acpi_dev_gpio_irq_get(),
> > which applies the trigger and polarity and returns -EPROBE_DEFER until the
> > GPIO controller is registered, and evaluate _EVT with the GPIO pin number,
> > as done for GSIs above 255. GpioIo resources are skipped. On a failed walk,
> > free the events already requested and propagate the GPIO error (including
> > -EPROBE_DEFER) instead of -EINVAL.
> >
> > Tested on the HP OmniBook 5 16-bf1xxx: lid close/open run _EVT and reach
> > the ACPI button driver and logind.
> >
> > Assisted-by: LLM
> > Signed-off-by: Rahul Pon <theflyingrahul@gmail.com>
> 
> I don't understand the patch at all, but I understand that Andy or Mika must
> review it otherwise it's not going anywhere, so looping them in.

ACPI GED specifically is supposed to use interrupts only (hence the name,
Interrupt -signaled ACPI events). I think this one should really use the
GPIO signaled ACPI events instead and probably just has the ACPI0013 there
by accident. Can you share the ASL around this device?
Rahul Pon Oct. 1, 2026, 4:18 a.m. UTC | #3 
On Thu, Oct 1, 2026 at 1:52 AM Linus Walleij <linusw@kernel.org> wrote:
> I don't understand the patch at all, but I understand that Andy or Mika must
> review it otherwise it's not going anywhere, so looping them in.

Thanks, Linus.

Andy, Mika: some context that should have been in the commit message.

ACPI 6.6 section 5.6.9.2 says a GED _CRS contains only Interrupt
resource descriptors, so this firmware is outside the spec. It is still
the only source of lid events here: LIGE's _CRS is a single GpioInt on
the GPIO controller (pin 960), and the controller's _AEI lists one
other, unrelated pin. ECGE is the same, with pin 768. On Windows, the
in-box ACPI device driver (acpidev.inf) binds both GED devices, and the
lid works there.

For _EVT, section 5.6.9.3 passes the GSI number, which a GpioInt does
not have. The patch passes the pin number from the GpioInt descriptor,
as section 5.6.5.3 does for _EVT under _AEI. Both _EVT methods on this
laptop ignore Arg0.

If the approach is acceptable, v2 will state the spec deviation in the
commit message.

Thanks,
Rahul
Rahul Pon Oct. 1, 2026, 4:27 a.m. UTC | #4 
Hi Mika,

On Thu, Oct 01, 2026 at 06:04:20AM +0200, Mika Westerberg wrote:
> ACPI GED specifically is supposed to use interrupts only (hence the name,
> Interrupt -signaled ACPI events). I think this one should really use the
> GPIO signaled ACPI events instead and probably just has the ACPI0013 there
> by accident. Can you share the ASL around this device?

Sure, below. iasl shows the _CRS buffers as raw bytes; the GpioInt
lines are decoded from them and recompile to the same bytes.

The lid device:

    Device (LIGE)
    {
        Name (_HID, "ACPI0013")
        Name (_UID, One)
        Name (_DEP, Package (0x03)
        {
            \_SB.GIO0, ,
            \_SB.LID0, ,
            \_SB.IC10,
        })
        Name (EEDP, Buffer (0x03)
        {
             0xFF
        })
        CreateByteField (EEDP, Zero, STAT)
        CreateField (EEDP, 0x10, 0x08, DBUF)
        Name (LIDG, Zero)
        Method (_CRS, 0, NotSerialized)
        {
            Name (RBUF, Buffer (0x25)
            {
                /* 0000 */  0x8C, 0x20, 0x00, 0x01, 0x00, 0x01, 0x00, 0x1D,
                /* 0008 */  0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x17, 0x00,
                /* 0010 */  0x00, 0x19, 0x00, 0x23, 0x00, 0x00, 0x00, 0xC0,
                /* 0018 */  0x03, 0x5C, 0x5F, 0x53, 0x42, 0x2E, 0x47, 0x49,
                /* 0020 */  0x4F, 0x30, 0x00, 0x79, 0x00
            })
            /* RBUF decodes to:
             * GpioInt (Edge, ActiveBoth, SharedAndWake, PullNone, 0x0000,
             *          "\\_SB.GIO0", 0x00, ResourceConsumer, ,) { 960 }
             */
            Return (RBUF)
        }

        Method (_EVT, 1, NotSerialized)
        {
            If ((\_SB.IC10.AVBL == One))
            {
                Sleep (0x05)
                Acquire (\_SB.ECMX, 0xFFFF)
                EEDP = \_SB.IC10.CMB2
                Release (\_SB.ECMX)
                LIDG = DBUF
                If ((LIDG & 0x04))
                {
                    \_SB.LID0.LIDB = One
                    \_SB.GPU0.LIDB = One
                    Notify (\_SB.LID0, 0x80)
                }
                Else
                {
                    \_SB.LID0.LIDB = \_SB.GIO0.LIDR
                    \_SB.GPU0.LIDB = \_SB.GIO0.LIDR
                    Notify (\_SB.LID0, 0x80)
                    If ((\_SB.LID0.LIDB == Zero))
                    {
                        \_SB.WMID.GWMT (0x08, One)
                    }

                    If ((\_SB.LID0.LIDB == One))
                    {
                        \_SB.WMID.GWMT (0x08, 0x02)
                    }
                }
            }
            Else
            {
                \_SB.LID0.LIDB = \_SB.GIO0.LIDR
                \_SB.GPU0.LIDB = \_SB.GIO0.LIDR
                Notify (\_SB.LID0, 0x80)
            }
        }
    }

The EC event device, ECGE, is another ACPI0013 with one GpioInt:

    GpioInt (Edge, ActiveLow, Exclusive, PullUp, 0x0000,
             "\\_SB.GIO0", 0x00, ResourceConsumer, ,) { 768 }

The GPIO controller (GIO0, QCOM0F0C) does have GPIO-signaled events.
Its _AEI lists one pin:

    GpioInt (Edge, ActiveHigh, Exclusive, PullNone, 0x01F4,
             "\\_SB.GIO0", 0x00, ResourceConsumer, ,) { 126 }

Its _EVT handles that pin (0x7E, a Notify to the GPU) and also pin 0x5C
with the same EC read and lid Notify as LIGE's _EVT, but _AEI does not
list 0x5C. So on this firmware the lid is signaled only through LIGE.

Either way, please drop this patch, and disregard the v2 plan in my
other reply. Looking at it again, it has no upstream user: GIO0 has no
ACPI driver in mainline, and my lid test used an out-of-tree bring-up
driver for it. Mainline supports these laptops through the device tree,
where the lid is a gpio-keys switch and no GED is involved.

Sorry for the noise.

Thanks,
Rahul
Andy Shevchenko Oct. 1, 2026, 6:15 p.m. UTC | #5 
On Thu, Oct 01, 2026 at 09:48:56AM +0530, Rahul Pon wrote:
> On Thu, Oct 1, 2026 at 1:52 AM Linus Walleij <linusw@kernel.org> wrote:
> > I don't understand the patch at all, but I understand that Andy or Mika must
> > review it otherwise it's not going anywhere, so looping them in.
> 
> Thanks, Linus.
> 
> Andy, Mika: some context that should have been in the commit message.
> 
> ACPI 6.6 section 5.6.9.2 says a GED _CRS contains only Interrupt
> resource descriptors, so this firmware is outside the spec. It is still
> the only source of lid events here: LIGE's _CRS is a single GpioInt on
> the GPIO controller (pin 960), and the controller's _AEI lists one
> other, unrelated pin. ECGE is the same, with pin 768. On Windows, the
> in-box ACPI device driver (acpidev.inf) binds both GED devices, and the
> lid works there.
> 
> For _EVT, section 5.6.9.3 passes the GSI number, which a GpioInt does
> not have. The patch passes the pin number from the GpioInt descriptor,
> as section 5.6.5.3 does for _EVT under _AEI. Both _EVT methods on this
> laptop ignore Arg0.
> 
> If the approach is acceptable, v2 will state the spec deviation in the
> commit message.

Is vendor of the firmware / platform being informed? Do they accept their
mistake? Are they going to fix their crap?
diff mbox series

Patch

diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
index 5c35cbc7f6..478172ede7 100644
--- a/drivers/acpi/evged.c
+++ b/drivers/acpi/evged.c
@@ -43,6 +43,8 @@ 
 struct acpi_ged_device {
 	struct device *dev;
 	struct list_head event_list;
+	unsigned int gpio_index;
+	int error;
 };
 
 struct acpi_ged_event {
@@ -76,7 +78,7 @@  static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
 	struct device *dev = geddev->dev;
 	acpi_handle handle = ACPI_HANDLE(dev);
 	acpi_handle evt_handle;
-	struct resource r;
+	struct resource r = {};
 	struct acpi_resource_irq *p = &ares->data.irq;
 	struct acpi_resource_extended_irq *pext = &ares->data.extended_irq;
 	char ev_name[5];
@@ -85,6 +87,33 @@  static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
 	if (ares->type == ACPI_RESOURCE_TYPE_END_TAG)
 		return AE_OK;
 
+	if (ares->type == ACPI_RESOURCE_TYPE_GPIO) {
+		struct acpi_resource_gpio *agpio;
+		int ret;
+
+		/* GpioIo resources are not event sources. */
+		if (!acpi_gpio_get_irq_resource(ares, &agpio))
+			return AE_OK;
+
+		/* The GPIO core applies the GpioInt trigger and polarity. */
+		ret = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), geddev->gpio_index++);
+		if (ret < 0) {
+			geddev->error = ret;
+			return AE_ERROR;
+		}
+		irq = ret;
+		gsi = agpio->pin_table[0];
+		if (agpio->shareable == ACPI_SHARED)
+			r.flags |= IORESOURCE_IRQ_SHAREABLE;
+
+		/* As for GSIs above 255, GPIO-signaled events use _EVT. */
+		if (ACPI_FAILURE(acpi_get_handle(handle, "_EVT", &evt_handle))) {
+			dev_err(dev, "cannot locate _EVT method\n");
+			return AE_ERROR;
+		}
+		goto request;
+	}
+
 	if (!acpi_dev_resource_interrupt(ares, 0, &r)) {
 		dev_err(dev, "unable to parse IRQ resource\n");
 		return AE_ERROR;
@@ -115,6 +144,7 @@  static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
 		return AE_ERROR;
 	}
 
+request:
 	event = devm_kzalloc(dev, sizeof(*event), GFP_KERNEL);
 	if (!event)
 		return AE_ERROR;
@@ -138,6 +168,8 @@  static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
 	return AE_OK;
 }
 
+static void acpi_ged_free_events(struct acpi_ged_device *geddev);
+
 static int ged_probe(struct platform_device *pdev)
 {
 	struct acpi_ged_device *geddev;
@@ -152,6 +184,10 @@  static int ged_probe(struct platform_device *pdev)
 	acpi_ret = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), "_CRS",
 				       acpi_ged_request_interrupt, geddev);
 	if (ACPI_FAILURE(acpi_ret)) {
+		acpi_ged_free_events(geddev);
+		if (geddev->error)
+			return dev_err_probe(&pdev->dev, geddev->error,
+					     "unable to get GPIO event IRQ\n");
 		dev_err(&pdev->dev, "unable to parse the _CRS record\n");
 		return -EINVAL;
 	}
@@ -160,9 +196,8 @@  static int ged_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void ged_shutdown(struct platform_device *pdev)
+static void acpi_ged_free_events(struct acpi_ged_device *geddev)
 {
-	struct acpi_ged_device *geddev = platform_get_drvdata(pdev);
 	struct acpi_ged_event *event, *next;
 
 	list_for_each_entry_safe(event, next, &geddev->event_list, node) {
@@ -173,6 +208,11 @@  static void ged_shutdown(struct platform_device *pdev)
 	}
 }
 
+static void ged_shutdown(struct platform_device *pdev)
+{
+	acpi_ged_free_events(platform_get_drvdata(pdev));
+}
+
 static void ged_remove(struct platform_device *pdev)
 {
 	ged_shutdown(pdev);