linux-kernel - Re: [PATCH v3 1/5] pinctrl: qcom: Remove irq

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=WcbEH2O+7xqCyDfrPR0+g+MpWWsgORNPepC=VrhOanFQ@mail.gmail.com>
Date:   Mon, 13 Jul 2020 15:17:55 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        LinusW <linus.walleij@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Evan Green <evgreen@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Srinivas Rao L <lsrao@...eaurora.org>
Subject: Re: [PATCH v3 1/5] pinctrl: qcom: Remove irq_disable callback from
 msmgpio irqchip

Hi,

On Mon, Jun 22, 2020 at 2:32 AM Maulik Shah <mkshah@...eaurora.org> wrote:
>
> The gpio can be marked for wakeup and drivers can invoke disable_irq()
> during suspend, in such cases unlazy approach will also disable at HW
> and such gpios will not wakeup device from suspend to RAM.
>
> Remove irq_disable callback to allow gpio interrupts to lazy disabled.
> The gpio interrupts will get disabled during irq_mask callback.
>
> Acked-by: Linus Walleij <linus.walleij@...aro.org>
> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 13 -------------
>  1 file changed, 13 deletions(-)

While the code of this patch looks fairly correct to me (there's no
need to implement the irq_disable callback and we can just rely on the
masking), I'm slightly anxious about the description.  It almost feels
like you're somehow relying on the laziness to "fix" your issue here.
...but the laziness is supposed to just be an optimization.
Specifically if an interrupt happens to fire at any point in time
after a driver has called disable_irq() then it acts just like a
non-lazy disable.

Said another way, I think this is a valid thing for a driver to do and
it should get woken up if the irq fires in suspend:

disable_irq();
msleep(1000);
enable_irq_wake()

Specifically if an IRQ comes in during that sleep then it will be just
like you had a non-lazy IRQ.

So while I'm for this patch, I'd suggest a simpler description
(assuming my understanding is correct):

There is no reason to implement irq_disable() and irq_mask().  Let's just
use irq_mask() and let the rest of the core handle it.

Also: it feels really weird to me that you're getting rid of the
irq_disable() but keeping irq_enable().  That seems like asking for
trouble, though I'd have to do more research to see if I could figure
out exactly what might go wrong.  Could you remove your irq_enable()
too?

-Doug