linux-kernel - Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

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: <20221013141149.zrcdtcfragerxdyw@soft-dev3-1>
Date:   Thu, 13 Oct 2022 16:11:49 +0200
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Michael Walle <michael@...le.cc>
CC:     <UNGLinuxDriver@...rochip.com>, <andy.shevchenko@...il.com>,
        <linus.walleij@...aro.org>, <linux-gpio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

The 10/13/2022 09:30, Michael Walle wrote:

Hi Michael,

> > We lose the interrupt here, as the HW will not generate another one
> > but at later point we read again the line status. And if the line is
> > active then we kick again the interrupt handler again.
> 
> Ahh, thanks for explaining. That also explains the read below.
> 
> Will you send a proper patch?

No worries. Yes, I will do that.

> 
> -michael
> 
> > 
> > > 
> > > > +
> > > >       /* Enable the interrupt now */
> > > >       gpiochip_enable_irq(chip, gpio);
> > > >       regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > > >                          bit, bit);
> > > >
> > > >       /*
> > > > -      * In case the interrupt line is still active and the interrupt
> > > > -      * controller has not seen any changes in the interrupt line, then it
> > > > -      * means that there happen another interrupt while the line was
> > > > active.
> > > > +      * In case the interrupt line is still active then it means that
> > > > +      * there happen another interrupt while the line was active.
> > > >        * So we missed that one, so we need to kick the interrupt again
> > > >        * handler.
> > > >        */
> > > > -     if (active && !ack) {
> > > > +     regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > > > +     if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > > > +           (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > > > +             active = true;
> > > 
> > > Why do you read the line state twice? What happens if the line state
> > > changes right after you've read it?
> > 
> > Here we need to read again the status because we might have clear the
> > ack of interrupt.
> > If the line becomes active right after this read, then the HW will
> > generate another interrupt as the interrupt is enabled and ack is
> > cleared.
> > 
> > > 
> > > > +
> > > > +     if (active) {
> > > >               struct ocelot_irq_work *work;
> > > >
> > > >               work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > > 
> > > So yes, maybe the trade-off that there will be two interrupts are
> > > better than this additional patch. But it should be documented
> > > somewhere, even if it's just a comment in this driver.
> > > 
> > > -michael

-- 
/Horatiu