linux-kernel - Re: [PATCH] irqchip/sunxi-nmi: add missing flag

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: <20250110003552.0ff180c6@minigeek.lan>
Date: Fri, 10 Jan 2025 00:35:52 +0000
From: Andre Przywara <andre.przywara@....com>
To: Philippe Simons <simons.philippe@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>, Samuel Holland
 <samuel@...lland.org>, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH] irqchip/sunxi-nmi: add missing flag

On Thu,  9 Jan 2025 16:39:35 +0100
Philippe Simons <simons.philippe@...il.com> wrote:

Hi Philippe,

thanks for sending that patch (and not just keeping it in your repo)!

> AXP717 has a PEK to wake up the device from sleep
> AXP717 is on the NMI irq pin
> add IRQCHIP_SKIP_SET_WAKE flag is required to enable wakeup from the suspend

That commit message might leave some people a bit puzzled, what about:
(with my embarrassingly little understanding of irqchip):
=================
Some boards with Allwinner SoCs connect the PMIC's IRQ pin to the SoC's
NMI pin instead of a normal GPIO. Since the power key is connected
to the PMIC, and people expect to wake up a suspended system via
this key, the NMI IRQ controller must stay alive when the
system goes into suspend.

Add the flag to prevent the sunxi NMI controller from going to sleep,
so that the power key can wake up those systems.
=================

Needs also your Signed-off-by:!

> ---
>  drivers/irqchip/irq-sunxi-nmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
> index bb92fd85e..3e87ca724 100644
> --- a/drivers/irqchip/irq-sunxi-nmi.c
> +++ b/drivers/irqchip/irq-sunxi-nmi.c
> @@ -186,7 +186,8 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
>  	gc->chip_types[0].chip.irq_unmask	= irq_gc_mask_set_bit;
>  	gc->chip_types[0].chip.irq_eoi		= irq_gc_ack_set_bit;
>  	gc->chip_types[0].chip.irq_set_type	= sunxi_sc_nmi_set_type;
> -	gc->chip_types[0].chip.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED;
> +	gc->chip_types[0].chip.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED
> +		| IRQCHIP_SKIP_SET_WAKE;

I think it's customary to put the operators last on the line, to make
it more obvious that this statement continues on the next line. I'd
recommend to put each flag on a line of its own here, and align them
vertically:
	flags	= IRQCHIP_... |
		  IRQCHIP_... |
		  IRQCHIP_...;

Cheers,
Andre

>  	gc->chip_types[0].regs.ack		= reg_offs->pend;
>  	gc->chip_types[0].regs.mask		= reg_offs->enable;
>  	gc->chip_types[0].regs.type		= reg_offs->ctrl;