Re: [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag

From: Thomas Gleixner <tglx@linutronix.de>
To: Doug Anderson <dianders@chromium.org>
Cc: Maulik Shah <mkshah@codeaurora.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Evan Green <evgreen@chromium.org>,
	LinusW <linus.walleij@linaro.org>, Marc Zyngier <maz@kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"open list\:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Andy Gross <agross@kernel.org>,
	Jason Cooper <jason@lakedaemon.net>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Srinivas Rao L <lsrao@codeaurora.org>
Subject: Re: [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag
Date: Tue, 01 Sep 2020 11:51:41 +0200	[thread overview]
Message-ID: <877dtdj042.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <CAD=FV=XXf3_tjqK14WdMuKygJptMTS+bKhH_ceiUE3wyYoCnxg@mail.gmail.com>

On Mon, Aug 31 2020 at 08:12, Doug Anderson wrote:
> On Wed, Aug 26, 2020 at 3:15 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>> There are two reasonable choices here:
>>
>> 1) Do the symmetric thing
>>
>> 2) Let the drivers call a new function disable_wakeup_irq_for_suspend()
>>    which marks the interrupt to be enabled from the core on suspend and
>>    remove the enable call on the resume callback of the driver.
>>
>>    Then you don't need the resume part in the core and state still is
>>    consistent.
>>
>> I'm leaning towards #2 because that makes a lot of sense.
>
> IIUC, #2 requires that we change existing drivers that are currently
> using disable_irq() + enable_irq_wake(), right?  Presumably, if we're
> going to do #2, we should declare that what drivers used to do is now
> considered illegal, right?  Perhaps we could detect that and throw a
> warning so that they know that they need to change to use the new
> disable_wakeup_irq_for_suspend() API.  Otherwise these drivers will
> work fine on some systems (like they always have) but will fail in
> weird corner cases for systems that are relying on drivers to call
> disable_wakeup_irq_for_suspend().  That doesn't sound super great to
> me...

Hmm. With disable_irq() + enable_irq_wake() in the driver suspend path
the driver already makes an implicit assumption about the underlying irq
chip functionality, i.e. it expects that even with the interrupt
disabled the irq chip can wake up the system.

Now with the new flag magic and #1 we are just working around the driver
assumptions at the interrupt chip level.

That's inconsistent at best.

How many drivers are doing that sequence?  And the more important
question is why are they calling disable_irq() in the first place if
they want to be woken up by that interrupt.

The point is that the core suspend code disables all interrupts which
are not marked as wakeup enabled automatically and reenables them after
resume. So why would any driver invoke disable_irq() in the suspend
function at all? Historical raisins?

Thanks,

        tglx