linux-kernel - Re: [PATCH] genirq/PM: Fix IRQCHIP_ENABLE_WAKEUP_ON

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=WYk+cXZCyPoeWwqpVc9hUXcU_BiafJ9s7B2W_Ow2eyWA@mail.gmail.com>
Date: Tue, 13 May 2025 07:31:35 -0700
From: Doug Anderson <dianders@...omium.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>, chintanpandya@...gle.com, 
	Maulik Shah <quic_mkshah@...cinc.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] genirq/PM: Fix IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND if depth
 > 1

Hi,

On Tue, May 13, 2025 at 12:49 AM Marc Zyngier <maz@...nel.org> wrote:
>
> Hey Doug,
>
> On Tue, 13 May 2025 01:32:52 +0100,
> Douglas Anderson <dianders@...omium.org> wrote:
> >
> > The IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag doesn't work properly if the
> > IRQ disable depth is not 0 or 1 at suspend time. In this case, the
> > IRQ's depth will be decremented but the IRQ won't be enabled to wake
> > the system up. Add a special case for when we're suspending and always
> > enable the IRQ in that case.
>
> For my own edification, can you explain why you end-up in this
> situation?

Dang, I meant to put it in the commit message. ...but this patch
doesn't fix any problems we're experiencing. The issue was found by
code inspection during a code review.

> Because I think I can see a use case for the current
> behaviour, where a driver controls whether it wants to use this
> interrupt as a wake-up source or not dynamically, irrespective of the
> underlying chip's capabilities (which I suspect is pinctrl-msm).

If a driver wants to dynamically choose whether it's a wake-up source,
wouldn't it just call disable_irq_wake() and enable_irq_wake()?

> This is also consistent with the way disable_irq() nests, making
> IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND the equivalent of an enable_irq()
> call.

To me it feels like we need to go back to what we were trying to
accomplish when we added IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND. The main
issue we were trying to solve was to make it so that an interrupt that
was masked by disable_irq() could still wake the system. IMO an
interrupt that has been masked _twice_ by disable_irq() is logically
in the same state and should still be able to wake the system up.

I think we'd also have to compare this to how it would work on systems
where there is a different bit for enabling an interrupt and enabling
wakeup. Those are the kinds of controllers that _don't_ need
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND. In those controllers, we're not
going to look at how many disable_irq() refcounts we have when we
decide to set the bit enabling the wakeup...

-Doug