Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Oct 2022 11:49:38 +0200
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Michael Walle <michael@walle.cc>
CC:     <UNGLinuxDriver@microchip.com>, <andy.shevchenko@gmail.com>,
        <linus.walleij@linaro.org>, <linux-gpio@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller
Message-ID: <20221007094938.qqf7exuthvz5gkdq@soft-dev3-1.localhost>
References: <20220909145942.844102-1-horatiu.vultur@microchip.com>
 <20220920120642.690340-1-michael@walle.cc>
 <20220920193033.bpmyt6pdob5b45id@soft-dev3-1.localhost>
 <683fc322fddebe39a93a46aefcd5e2dd@walle.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <683fc322fddebe39a93a46aefcd5e2dd@walle.cc>
Precedence: bulk

The 10/06/2022 13:43, Michael Walle wrote:

Hi Walle,

> Seeing 20 was definitely fishy, seeing two instead of one maybe not
> so much. I guess it will create one spurious interrupt if none of
> the registered handlers will care.
> 
> OTOH, the code below won't work in all cases anyway, right? It's just
> best effort.

I was expecting to work in all cases, but if you found some cases that
would not work, please point them out.

> 
> > Below I have a diff that I tried with LAN8814 PHYs and I could see that
> > count in /proc/interrupts is increasing correctly.
> > 
> > > I've verified that there is only one low pulse on the interrupt line.
> > > I've
> > > noticed though, that the number of interrupts seem to be correlating
> > > with
> > > the length of the low pulse.
> > ---
> > diff --git a/drivers/pinctrl/pinctrl-ocelot.c
> > b/drivers/pinctrl/pinctrl-ocelot.c
> > index c7df8c5fe5854..105771ff82e62 100644
> > --- a/drivers/pinctrl/pinctrl-ocelot.c
> > +++ b/drivers/pinctrl/pinctrl-ocelot.c
> > @@ -1863,19 +1863,28 @@ static void ocelot_irq_unmask_level(struct
> > irq_data *data)
> >       if (val & bit)
> >               ack = true;
> > 
> > +     /* Try to clear any rising edges */
> > +     if (!active && ack)
> > +             regmap_write_bits(info->map, REG(OCELOT_GPIO_INTR, info, gpio),
> > +                               bit, bit);
> 
> Might we lose interrupts here, if the line would go active again right
> after the read of the line state and before reading the "ack" bit?

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.

> 
> > +
> >       /* 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