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: <20220820095933.20234-1-jeffy.chen@rock-chips.com> <20220820095933.20234-2-jeffy.chen@rock-chips.com>
In-Reply-To: <20220820095933.20234-2-jeffy.chen@rock-chips.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Mon, 22 Aug 2022 10:08:10 -0700
Message-ID: <CAD=FV=X0qJ2OC1SrAmhSQ5YeKEwvsSCbfVGPh457YYEuPCbRtg@mail.gmail.com>
Subject: Re: [PATCH 2/2] gpio/rockchip: Toggle edge trigger mode after acking
To:     Jeffy Chen <jeffy.chen@rock-chips.com>
Cc:     Heiko Stuebner <heiko@sntech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Linus Walleij <linus.walleij@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Brian Norris <briannorris@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Sat, Aug 20, 2022 at 3:07 AM Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
>
> Otherwise the trigger mode might be out-of-sync when a level change
> occurred in between.
>
> Fixes: 53b1bfc76df2 ("pinctrl: rockchip: Avoid losing interrupts when supporting both edges")
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
>
>  drivers/gpio/gpio-rockchip.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index a98351cd6821..736b4d90f1ca 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -338,7 +338,7 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                 irq = __ffs(pend);
>                 pend &= ~BIT(irq);
>
> -               dev_dbg(bank->dev, "handling irq %d\n", irq);
> +               generic_handle_domain_irq(bank->domain, irq);
>
>                 /*
>                  * Triggering IRQ on both rising and falling edge
> @@ -370,8 +370,6 @@ static void rockchip_irq_demux(struct irq_desc *desc)
>                                                      bank->gpio_regs->ext_port);
>                         } while ((data & BIT(irq)) != (data_old & BIT(irq)));
>                 }
> -
> -               generic_handle_domain_irq(bank->domain, irq);

I'm happy to let others say for sure, but from my point of view I'm
not convinced. It feels like with your new code you could lose edges.

The abstraction I always assume for edge triggered interrupts is that
multiple edges are coalesced into one IRQ but that if an edge comes in
after the first line of the IRQ handler starts executing then the IRQ
handler will run again. In other words:

- edge A
- edge B
- IRQ handler starts running (once for A/B)
- IRQ handler finishes running
- <idle>
- edge C
- IRQ handler starts running (for C)
- edge D
- edge E
- IRQ handler finishes running
- IRQ handler starts running (for D/E)
- IRQ handler finishes running
- <idle>

For your new code I don't think that will necessarily be the case. I
think this can happen with your new code:

- rising edge
- IRQ handler starts running for rising edge
- IRQ handler finishes running for rising edge
- falling edge (not latched since we're looking for rising edges)
- notice that level is low
- keep it configured for rising edge

...in other words an edge happened _after_ the IRQ handler ran but we
didn't call the IRQ handler again. I don't think this is right.


What problem are you trying to solve?

-Doug