linux-kernel - Re: [RFC PATCH v3 6/7] gpiolib: enable GPIO interrupt to wake up a system from sleep

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACRpkdajd1WkzscPiZL8JKvf10VHy5ppYjy-zAOaNTh0cFXtbQ@mail.gmail.com>
Date: Mon, 27 May 2024 14:31:51 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: Alex Soo <yuklin.soo@...rfivetech.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Hal Feng <hal.feng@...rfivetech.com>, 
	Ley Foon Tan <leyfoon.tan@...rfivetech.com>, 
	Jianlong Huang <jianlong.huang@...rfivetech.com>, Emil Renner Berthing <kernel@...il.dk>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, 
	Conor Dooley <conor+dt@...nel.org>, Drew Fustini <drew@...gleboard.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [RFC PATCH v3 6/7] gpiolib: enable GPIO interrupt to wake up a
 system from sleep

On Fri, May 3, 2024 at 1:15 PM Alex Soo <yuklin.soo@...rfivetech.com> wrote:

> Add function gpiochip_wakeup_irq_setup() to configure and enable a
> GPIO pin with interrupt wakeup capability according to user-defined
> wakeup-gpios property in the device tree. Interrupt generated by
> toggling the logic level (rising/falling edge) on the specified
> GPIO pin can wake up a system from sleep mode.
>
> Signed-off-by: Alex Soo <yuklin.soo@...rfivetech.com>

This is a very helpful patch I think.

I'm looking forward to the next iteration.

> @@ -1045,8 +1047,15 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 if (ret)
>                         goto err_remove_irqchip;
>         }
> +
> +       ret = gpiochip_wakeup_irq_setup(gc);
> +       if (ret)
> +               goto err_remove_device;

Do we have any in-tree drivers that do this by themselves already?

In that case we should convert them to use this function in the same
patch to avoid regressions.

> +static irqreturn_t gpio_wake_irq_handler(int irq, void *data)
> +{
> +       struct irq_data *irq_data = data;

I'm minimalist so I usually just call the parameter "d" instead of "data"
and irq_data I would call *id but it's your pick.

> +
> +       if (!irq_data || irq != irq_data->irq)
> +               return IRQ_NONE;
> +
> +       return IRQ_HANDLED;

Please add some debug print:

struct gpio_chip *gc = irq_data->chip_data;

chip_dbg(gc, "GPIO wakeup on IRQ %d\n", irq);

The rest looks good to me (after fixing Andy's comments!)

I would perhaps put some
debug print that "GPIO wakeup enabled at offset %d" in the
end as well, so people can easily follow this in the debug prints.

Yours,
Linus Walleij