Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220427142647.1736658-1-daniel.thompson@linaro.org>
In-Reply-To: <20220427142647.1736658-1-daniel.thompson@linaro.org>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Wed, 27 Apr 2022 17:29:33 +0200
Message-ID: <CAMj1kXEYO_HRwZ=Wz5aTWN8XZctemVv2op4B2=DbFEozuxTNVw@mail.gmail.com>
Subject: Re: [PATCH] irqchip/exiu: Fix acknowledgment of edge triggered interrupts
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Marc Zyngier <maz@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 27 Apr 2022 at 16:27, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently the EXIU uses the fasteoi interrupt flow that is configured by
> it's parent (irq-gic-v3.c). With this flow the only chance to clear the
> interrupt request happens during .irq_eoi() and (obviously) this happens
> after the interrupt handler has run. EXIU requires edge triggered
> interrupts to be acked prior to interrupt handling. Without this we
> risk incorrect interrupt dismissal when a new interrupt is delivered
> after the handler reads and acknowledges the peripheral but before the
> irq_eoi() takes place.
>
> Fix this by clearing the interrupt request from .irq_ack() instead. This
> requires switching to the fasteoi-ack flow instead of the fasteoi flow.
> This approach is inspired by the nmi code found in irq-sun6i-r.c .
>

How are level triggered EXIU interrupts affected by this change?

> Note: It *is* intentional not to call irq_chip_ack_parent() from
>       exiu_irq_ack(). Parents that implement the fasteoi-ack flow don't
>       want us to do that (and we'd crash if we tried).
>
> This problem was discovered through code review rather then reproducing
> missed interrupts in a real platform. Nevertheless the change has been
> tested for regression on a Developerbox/SC2A11 using the power button
> (which is edge triggered and is the only thing connected to the EXIU on
> this platform).
>

This is not entirely true. The PHY interrupt is also wired to the
EXIU, but this never worked reliably, so we don't expose this in the
DT or ACPI tables.
I wonder if this change would help, but I don't remember exactly what
the problem was.

> Fixes: 706cffc1b912 ("irqchip/exiu: Add support for Socionext Synquacer EXIU controller")
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  arch/arm64/Kconfig.platforms   |  1 +
>  drivers/irqchip/irq-sni-exiu.c | 16 ++++++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 30b123cde02c..aaeaf57c8222 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -253,6 +253,7 @@ config ARCH_INTEL_SOCFPGA
>
>  config ARCH_SYNQUACER
>         bool "Socionext SynQuacer SoC Family"
> +       select IRQ_FASTEOI_HIERARCHY_HANDLERS
>
>  config ARCH_TEGRA
>         bool "NVIDIA Tegra SoC Family"
> diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
> index abd011fcecf4..1cc2ec272ebd 100644
> --- a/drivers/irqchip/irq-sni-exiu.c
> +++ b/drivers/irqchip/irq-sni-exiu.c
> @@ -37,12 +37,11 @@ struct exiu_irq_data {
>         u32             spi_base;
>  };
>
> -static void exiu_irq_eoi(struct irq_data *d)
> +static void exiu_irq_ack(struct irq_data *d)
>  {
>         struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);
>
>         writel(BIT(d->hwirq), data->base + EIREQCLR);
> -       irq_chip_eoi_parent(d);
>  }
>
>  static void exiu_irq_mask(struct irq_data *d)
> @@ -104,7 +103,8 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
>
>  static struct irq_chip exiu_irq_chip = {
>         .name                   = "EXIU",
> -       .irq_eoi                = exiu_irq_eoi,
> +       .irq_ack                = exiu_irq_ack,
> +       .irq_eoi                = irq_chip_eoi_parent,
>         .irq_enable             = exiu_irq_enable,
>         .irq_mask               = exiu_irq_mask,
>         .irq_unmask             = exiu_irq_unmask,
> @@ -148,6 +148,7 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
>         struct irq_fwspec parent_fwspec;
>         struct exiu_irq_data *info = dom->host_data;
>         irq_hw_number_t hwirq;
> +       int i, ret;
>
>         parent_fwspec = *fwspec;
>         if (is_of_node(dom->parent->fwnode)) {
> @@ -165,7 +166,14 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
>         irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &exiu_irq_chip, info);
>
>         parent_fwspec.fwnode = dom->parent->fwnode;
> -       return irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
> +       ret = irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < nr_irqs; i++)
> +               irq_set_handler(virq+i, handle_fasteoi_ack_irq);
> +
> +       return 0;
>  }
>
>  static const struct irq_domain_ops exiu_domain_ops = {
>
> base-commit: b2d229d4ddb17db541098b83524d901257e93845
> --
> 2.35.1
>