Re: [PATCH] gpiolib: acpi: Only trigger ActiveBoth interrupts on boot

[Hans de Goede <hansg@kernel.org>]

Hi Mario,

Thank you for fixing this.

On 29-Apr-26 04:52, Mario Limonciello wrote:
> Commit ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events at
> least once on boot") introduced logic to trigger edge-based GPIO
> interrupts during initialization to ensure proper initial state setup
> when firmware doesn't initialize it.
> 
> However, according to the Microsoft GPIO documentation, triggering GPIO
> interrupts during initialization should only happen for interrupts
> marked as ActiveBoth (both IRQF_TRIGGER_RISING and IRQF_TRIGGER_FALLING)
> and only when the associated GPIO line is already asserted (logic level
> low).
> 
> The current implementation incorrectly triggers:
> 1. Any edge-triggered interrupt (RISING-only or FALLING-only)
> 2. RISING interrupts when value is high and FALLING when value is low
> 
> This causes problems at bootup for single-edge interrupts that
> don't follow the ActiveBoth pattern.
> 
> Fix this by:
> - Only triggering when BOTH rising and falling edges are configured
> - Only triggering when the GPIO line is asserted (value == 0)
> 
> Reported-by: Francesco Lauritano <francesco.lauritano1@protonmail.com>
> Closes: https://lore.kernel.org/all/6iFCwGH2vssb7NRUTWGpkubGMNbgIlBHSz40z8ZsezjxngXpoiiRiJaijviNvhiDAGIr43bfUmdxLmxYoHDjyft4DgwFc3Pnu5hzPguTa0s=@protonmail.com/
> Tested-by: Marco Scardovi <mscardovi95@gmail.com>
> Fixes: ca876c7483b69 ("gpiolib-acpi: make sure we trigger edge events at least once on boot")
> Link: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/general-purpose-i-o--gpio-
> Suggested-by: Armin Wolf <W_Armin@gmx.de>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index 09f860200a059..eb8a40cfb7a98 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -233,12 +233,23 @@ static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>  
>  	event->irq_requested = true;
>  
> -	/* Make sure we trigger the initial state of edge-triggered IRQs */
> +	/*
> +	 * Make sure we trigger the initial state of ActiveBoth IRQs.
> +	 *
> +	 * According to the Microsoft GPIO documentation, triggering GPIO
> +	 * interrupts marked as ActiveBoth during initialization is correct
> +	 * as long as the associated GPIO line is already "asserted"
> +	 * (logic level low). We should not trigger edge-based GPIO
> +	 * interrupts not marked as ActiveBoth.
> +	 *
> +	 * See: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/general-purpose-i-o--gpio-
> +	 * Section: "GPIO controllers and ActiveBoth interrupts"
> +	 */
>  	if (acpi_gpio_need_run_edge_events_on_boot() &&
> -	    (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> +	    ((event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) ==
> +	     (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
>  		value = gpiod_get_raw_value_cansleep(event->desc);
> -		if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> -		    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> +		if (value == 0)
>  			event->handler(event->irq, event);
>  	}
>  }

One nitpick, which can be a follow-up patch since Andy has already picked this
one up.

I think that now that the second if condition has been simplified to just
value == 0, it can be added to the first if as " && value == 0" dropping
the nested if.

Regards,

Hans