linux-kernel - Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support

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: <CACRpkdYYRe4A+Zj+fDpC9SA1pgaVSgewYGB_vCNoWNRXWTtTGQ@mail.gmail.com>
Date:   Wed, 26 Apr 2017 14:03:15 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Gregory CLEMENT <gregory.clement@...e-electrons.com>
Cc:     "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Jason Cooper <jason@...edaemon.net>,
        Andrew Lunn <andrew@...n.ch>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nadav Haklai <nadavh@...vell.com>,
        Victor Gu <xigu@...vell.com>, Marcin Wojtas <mw@...ihalf.com>,
        Wilson Ding <dingwei@...vell.com>,
        Hua Jing <jinghua@...vell.com>,
        Neta Zur Hershkovits <neta@...vell.com>
Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support

On Wed, Apr 26, 2017 at 11:23 AM, Gregory CLEMENT
<gregory.clement@...e-electrons.com> wrote:
>  On lun., avril 24 2017, Linus Walleij <linus.walleij@...aro.org> wrote:

>>> +               spin_lock_irqsave(&info->irq_lock, flags);
>>> +               status = readl_relaxed(info->base + IRQ_STATUS + 4 * i);
>>> +               /* Manage only the interrupt that was enabled */
>>> +               status &= readl_relaxed(info->base + IRQ_EN + 4 * i);
>>> +               spin_unlock_irqrestore(&info->irq_lock, flags);
>>> +               while (status) {
>>> +                       u32 hwirq = ffs(status) - 1;
>>> +                       u32 virq = irq_find_mapping(d, hwirq +
>>> +                                                    i * GPIO_PER_REG);
>>> +
>>> +                       generic_handle_irq(virq);
>>> +                       status &= ~BIT(hwirq);
>>> +               }
>>
>> You hae a problem here is a new IRQ appears while you are inside
>> of this loop. You need to re-read the status register for each iteration
>> (and &= with the IRQ_EN I guess).
>
> If a new IRQ appears during the loop, then the irq handler will be
> called again because the cause of this new IRQ won't have been acked
> yet. So I think we're fine here.

That *might* be true. It is true if the CPU gets a level IRQ from the
GPIO controller. But hardware dealing with edge IRQs can be very
quirky here, and just send a pulse on the line to the CPU if the
CPU-bound IRQ is also just edge triggered. And then that
pulse would potentially be missed while dealing with the current
IRQ in this handler. (And exactly this happened to us on other
hardware.)

But anyway: why let the irq handler be called again if you can avoid it?

You would avoid a double context switch by just checking it again
in the loop before exiting the handler. And that can be really nice
for latency-sensitive stuff.

Yours,
Linus Walleij