Re: [PATCH v3] pinctrl: ocelot: Fix interrupt controller

[Michael Walle]

Hi Horatiu,

Am 2022-10-07 11:49, schrieb Horatiu Vultur:
> 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.

Ahh, thanks for explaining. That also explains the read below.

Will you send a proper patch?

-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