| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220429183605.gld37gz6taa5k7fk@maple.lan>
Date: Fri, 29 Apr 2022 19:36:05 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Marc Zyngier <maz@...nel.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Ard Biesheuvel <ardb@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] irqchip/exiu: Fix acknowledgment of edge triggered
interrupts
On Fri, Apr 29, 2022 at 06:39:51PM +0100, Marc Zyngier wrote:
> On Fri, 29 Apr 2022 17:53:14 +0100,
> Daniel Thompson <daniel.thompson@...aro.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() if we are
> > configured for edge triggered interrupts. This requires adopting the
> > fasteoi-ack flow instead of the fasteoi to ensure the ack gets called.
> >
> > These changes have been tested using the power button on a
> > Developerbox/SC2A11 combined with some hackery in gpio-keys so I can
> > play with the different trigger mode (and an mdelay(500) so I can
> > can check what happens on a double click in both modes.
> >
> > Fixes: 706cffc1b912 ("irqchip/exiu: Add support for Socionext Synquacer EXIU controller")
> > Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
> > ---
> >
> > Notes:
> > Changes in v2:
> >
> > * Switch to dynamic selection of handle_fasteoi_irq and
> > handle_fasteoi_ack_irq and reintroduce exiu_irq_eoi() since we need
> > that for level triggered interrupts (Ard B).
> > * Above changes mean we are no longer using sun6i NMI code as a
> > template to tidy up the description accordingly.
> >
> > arch/arm64/Kconfig.platforms | 1 +
> > drivers/irqchip/irq-sni-exiu.c | 33 +++++++++++++++++++++++++++++----
> > 2 files changed, 30 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..651a82dead01 100644
> > --- a/drivers/irqchip/irq-sni-exiu.c
> > +++ b/drivers/irqchip/irq-sni-exiu.c
> > @@ -37,11 +37,20 @@ 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);
> > +}
> > +
> > +static void exiu_irq_eoi(struct irq_data *d)
> > +{
> > + struct exiu_irq_data *data = irq_data_get_irq_chip_data(d);
> > + u32 edge_triggered = readl_relaxed(data->base + EIEDG);
>
> I expect this to be pretty expensive. Why not directly check the state
> flags with irqd_is_level_type()?
Good point. Will change this.
> > +
> > + if (!(edge_triggered & BIT(d->hwirq)))
> > + writel(BIT(d->hwirq), data->base + EIREQCLR);
>
> Is this write even needed for a level interrupt? Or does the register
> always behave as a latch irrespective of the trigger?
It is unconditionally latched; must be cleared or the interrupt will be
jammed on. Of course, for level interrupts that are still asserted then
the write will not clear the interrupt.
> > irq_chip_eoi_parent(d);
> > }
> >
> > @@ -91,10 +100,13 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
> > writel_relaxed(val, data->base + EILVL);
> >
> > val = readl_relaxed(data->base + EIEDG);
> > - if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH)
> > + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) {
> > val &= ~BIT(d->hwirq);
> > - else
> > + irq_set_handler_locked(d, handle_fasteoi_irq);
> > + } else {
> > val |= BIT(d->hwirq);
> > + irq_set_handler_locked(d, handle_fasteoi_ack_irq);
> > + }
> > writel_relaxed(val, data->base + EIEDG);
> >
> > writel_relaxed(BIT(d->hwirq), data->base + EIREQCLR);
> > @@ -104,6 +116,7 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type)
> >
> > static struct irq_chip exiu_irq_chip = {
> > .name = "EXIU",
> > + .irq_ack = exiu_irq_ack,
> > .irq_eoi = exiu_irq_eoi,
> > .irq_enable = exiu_irq_enable,
> > .irq_mask = exiu_irq_mask,
> > @@ -148,6 +161,8 @@ 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;
> > + u32 edge_triggered;
> >
> > parent_fwspec = *fwspec;
> > if (is_of_node(dom->parent->fwnode)) {
> > @@ -165,7 +180,17 @@ 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;
> > +
> > + edge_triggered = readl_relaxed(info->base + EIEDG);
> > + for (i = 0; i < nr_irqs; i++)
> > + irq_set_handler(virq + i, edge_triggered & BIT(i) ?
> > + handle_fasteoi_ack_irq :
> > + handle_fasteoi_irq);
> > +
> > + return 0;
>
> Why do you need this at allocation time? I would have expected the
> trigger configuration to be enough.
I saw the following in the description of the interrupt trigger modes
: When requesting an interrupt without specifying a IRQF_TRIGGER, the
: setting should be assumed to be "as already configured", which may
: be as per machine or firmware initialisation.
>From that I was concerned that the callback to set the trigger mode
would not be called in all cases. Thus when I saw that calling
__irq_set_trigger() was on a conditional code path in __setup_irq()
then I wrote the above logic.
I assume I overlooked something? Is a call to exiu_irq_set_type()
guaranteed to happen in all cases?
Daniel.
Powered by blists - more mailing lists