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;
MIME-Version: 1.0
References: <20220902124354.630826-1-horatiu.vultur@microchip.com>
In-Reply-To: <20220902124354.630826-1-horatiu.vultur@microchip.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 2 Sep 2022 17:51:14 +0300
Message-ID: <CAHp75Ve7EkE3q3_nOvT_KLmpmnXzMw179nbOxYEYzUeLY0QRnw@mail.gmail.com>
Subject: Re: [PATCH] pinctrl: ocelot: Fix interrupt controller
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Sep 2, 2022 at 3:40 PM Horatiu Vultur
<horatiu.vultur@microchip.com> wrote:
>
> When an external device generated a level based interrupt then the
> interrupt controller could miss the interrupt. The reason is that the
> interrupt controller can detect only link changes.
>
> In the following example, if there is a PHY that generates an interrupt
> then the following would be happen. The GPIO detected that the interrupt

would happen

> line changed, and then the 'ocelot_irq_handler' will be called. Here it
> detects which GPIO line seen the change and for that will call the
> following:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> And this works fine for simple cases, but if the PHY generates many
> interrupts, for example when doing PTP timestamping, then the following
> could happen. Again the function 'ocelot_irq_handler' will be called
> and then from here the following could happen:
> 1. irq_mask
> 2. phy interrupt routine
> 3. irq_eoi
> 4. irq_unmask
>
> Right before step 3(irq_eoi), the PHY will generate another interrupt.
> Now the interrupt controller will acknowledge the change in the
> interrupt line. So we miss the interrupt.
>
> A solution will be to use 'handle_level_irq' instead of
> 'handle_fasteoi_irq', because for this will change routine order of
> handling the interrupt.
> 1. irq_mask
> 2. irq_ack
> 3. phy interrupt routine
> 4. irq_unmask
>
> And now if the PHY will generate a new interrupt before irq_unmask, the
> interrupt controller will detect this because it already acknowledge the
> change in interrupt line at step 2(irq_ack).
>
> But this is not the full solution because there is another issue. In
> case there are 2 PHYs that shared the interrupt line. For example phy1

share

> generates an interruot, then the following can happen:

interrupt

> 1.irq_mask
> 2.irq_ack
> 3.phy0 interrupt routine
> 4.phy1 interrupt routine
> 5.irq_unmask
>
> In case phy0 will generate an interrupt while clearing the interrupt
> source in phy1, then the interrupt line will be kept down by phy0. So
> the interrupt controller will not see any changes in the interrupt line.
> The solution here is to update 'irq_unmask' such that it can detect if
> the interrupt line is still active or not. And if it is active then call
> again the procedure to clear the interrupts. But we don't want to do it
> every time, only if we know that the interrupt controller have not seen
> already that the interrupt line has changed.
>
> While at this, add support also for IRQ_TYPE_LEVEL_LOW.

...

> +       /*
> +        * It is enough to read only one action because the trigger level is the
> +        * same for all of them.
> +        */

Hmm... this is interesting. How is the hardware supposed to work if
the user asks for two contradictory levels for two different IRQs?

...

> +       /*
> +        * Check if the interrupt controller has seen any changes in the
> +        * interrupt line

Missed period.

> +        */

...

> +       /*
> +        * 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.
> +        * So we missed that one, so we need to kick again the interrupt handler
> +        */

Ditto.

...

> +               struct ocelot_irq_work *work = kmalloc(sizeof(*work), GFP_ATOMIC);

It's more visible what's going on if you split this to definition and
assignment and move assignment closer to its first user.

> +               if (!work)
> +                       return;

...

>         type &= IRQ_TYPE_SENSE_MASK;

This seems redundant, see below.


> -       if (!(type & (IRQ_TYPE_EDGE_BOTH | IRQ_TYPE_LEVEL_HIGH)))
> +       if (type == IRQ_TYPE_NONE)
>                 return -EINVAL;

Is it ever possible? IIRC the IRQ chip code, the set->type won't be
called at all in such a case. Also type is already limited to the
sense mask, no?

-- 
With Best Regards,
Andy Shevchenko