From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Marc Zyngier <maz@kernel.org>
Cc: "Russell King" <linux@arm.linux.org.uk>,
"Linux ARM" <linux-arm-kernel@lists.infradead.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Will Deacon" <will@kernel.org>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Jason Cooper" <jason@lakedaemon.net>,
"Sumit Garg" <sumit.garg@linaro.org>,
"Valentin Schneider" <Valentin.Schneider@arm.com>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Gregory Clement" <gregory.clement@bootlin.com>,
"Andrew Lunn" <andrew@lunn.ch>,
"Android Kernel Team" <kernel-team@android.com>,
stable <stable@vger.kernel.org>,
"Magnus Damm" <damm+renesas@opensource.se>,
"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity
Date: Fri, 10 Sep 2021 15:19:05 +0200 [thread overview]
Message-ID: <CAMuHMdV+ydPaXbGf1_O0S-juaPWk1gwBUOK+GeLZukZeoqtMGQ@mail.gmail.com> (raw)
In-Reply-To: <875yv8d91b.wl-maz@kernel.org>
Hi Marc,
On Fri, Sep 10, 2021 at 12:23 PM Marc Zyngier <maz@kernel.org> wrote:
> On Thu, 09 Sep 2021 16:22:01 +0100,
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier <maz@kernel.org> wrote:
> > > The GIC driver uses a RMW sequence to update the affinity, and
> > > relies on the gic_lock_irqsave/gic_unlock_irqrestore sequences
> > > to update it atomically.
> > >
> > > But these sequences only expend into anything meaningful if
> > > the BL_SWITCHER option is selected, which almost never happens.
> > >
> > > It also turns out that using a RMW and locks is just as silly,
> > > as the GIC distributor supports byte accesses for the GICD_TARGETRn
> > > registers, which when used make the update atomic by definition.
> > >
> > > Drop the terminally broken code and replace it by a byte write.
> > >
> > > Fixes: 04c8b0f82c7d ("irqchip/gic: Make locking a BL_SWITCHER only feature")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> >
> > Thanks for your patch, which is now commit 005c34ae4b44f085
> > ("irqchip/gic: Atomically update affinity"), to which I bisected a hard
> > lock-up during boot on the Renesas EMMA Mobile EV2-based KZM-A9-Dual
> > board, which has a dual Cortex-A9 with PL390.
> >
> > Despite the ARM Generic Interrupt Controller Architecture Specification
> > (both version 1.0 and 2.0) stating that the Interrupt Processor Targets
> > Registers are byte-accessible, the EMMA Mobile EV2 User's Manual
> > states that the interrupt registers can be accessed via the APB bus,
> > in 32-bit units. Using byte accesses locks up the system.
>
> Urgh. That is definitely a pretty poor integration. How about the
> priority registers? I guess they suffer from the same issue...
Yes, they do.
> > Unfortunately I only have remote access to the board showing the
> > issue. I did check that adding the writeb_relaxed() before the
> > writel_relaxed() that was used before also causes a lock-up, so the
> > issue is not an endian mismatch.
> > Looking at the driver history, these registers have always been
> > accessed using 32-bit accesses before. As byte accesses lead
> > indeed to simpler code, I'm wondering if they had been tried before,
> > and caused issues before?
>
> Not that I know. A lock was probably fine on a two CPU system. Less so
> on a busy 8 CPU machine where interrupts are often migrated. The GIC
> architecture makes a point in not requiring locking for most of the
> registers that can be accessed concurrently.
>
> > Since you said the locking was bogus before, due to the reliance on
> > the BL_SWITCHER option, I'm not suggesting a plain revert, but I'm
> > wondering what kind of locking you suggest to use instead?
>
> There isn't much we can do aside from reintroducing the RMW+spinlock
> approach, and for real this time. It would have to be handled as a
> quirk though, as I'm not keen on reintroducing this for all systems.
>
> I wrote the patchlet below, which is totally untested. Please give it
> a go and let me know if it helps.
Thanks for your quick response!
Your solution works, after making a few small modifications.
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic)
> #endif
>
> #ifdef CONFIG_SMP
> +static void rmw_writeb(u8 bval, void __iomem *addr)
> +{
> + static DEFINE_RAW_SPINLOCK(rmw_lock);
> + unsigned long offset = (unsigned long)addr & ~3UL;
Please drop the tilde.
> + unsigned long shift = offset * 8;
"unsigned int" is sufficient for offset and size.
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&rmw_lock, flags);
> +
> + addr -= offset;
> + val = readl_relaxed(addr);
> + val &= ~(0xffUL << shift);
No need for the UL suffix.
> + val |= (u32)bval << shift;
No need for the cast.
> + writel_relaxed(val, addr);
> +
> + raw_spin_unlock_irqrestore(&rmw_lock, flags);
> +}
> +
> static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> {
> @@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
> return true;
> }
>
> +static bool gic_enable_rmw_access(void *data)
> +{
> + /*
> + * The EMEV2 class of machines has a broken interconnect, and
> + * locks up on accesses that are less than 32bit. So far, only
> + * the affinity setting requires it.
> + */
> + if (of_machine_is_compatible("renesas,emev2")) {
> + static_branch_enable(&needs_rmw_access);
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static const struct gic_quirk gic_quirks[] = {
> + {
> + .desc = "Implementation with broken byte access",
The output would look better without capitalizing the first word.
I think you can drop the first two words, saving some space:
GIC: enabling workaround for broken byte access
> + .compatible = "arm,pl390",
> + .init = gic_enable_rmw_access,
> + },
Missing "{ /* sentinel */ }".
> +};
> +
> static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
> {
> if (!gic || !node)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
next prev parent reply other threads:[~2021-09-10 13:19 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-24 19:57 [PATCH v2 00/17] arm/arm64: Turning IPIs into normal interrupts Marc Zyngier
2020-06-24 19:57 ` [PATCH v2 01/17] genirq: Add fasteoi IPI flow Marc Zyngier
2020-06-24 19:57 ` [PATCH v2 02/17] genirq: Allow interrupts to be excluded from /proc/interrupts Marc Zyngier
2020-06-24 19:57 ` [PATCH v2 03/17] arm64: Allow IPIs to be handled as normal interrupts Marc Zyngier
2020-06-25 18:25 ` Valentin Schneider
2020-07-10 19:58 ` Valentin Schneider
2020-06-24 19:57 ` [PATCH v2 04/17] ARM: " Marc Zyngier
2020-06-25 18:25 ` Valentin Schneider
2020-06-29 9:37 ` Marc Zyngier
2020-06-24 19:57 ` [PATCH v2 05/17] irqchip/gic-v3: Describe the SGI range Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts Marc Zyngier
2020-06-25 18:25 ` Valentin Schneider
2020-06-30 10:15 ` Marc Zyngier
2020-07-02 13:23 ` Valentin Schneider
2020-07-02 13:48 ` Marc Zyngier
2020-07-02 14:24 ` Valentin Schneider
2020-06-24 19:58 ` [PATCH v2 07/17] irqchip/gic: Atomically update affinity Marc Zyngier
2020-07-01 19:33 ` Sasha Levin
2020-07-10 14:02 ` Sasha Levin
2021-09-09 15:22 ` Geert Uytterhoeven
2021-09-09 15:37 ` Russell King (Oracle)
2021-09-10 10:22 ` Marc Zyngier
2021-09-10 13:19 ` Geert Uytterhoeven [this message]
2021-09-11 2:49 ` Magnus Damm
2021-09-11 19:32 ` Marc Zyngier
2021-09-12 5:40 ` Magnus Damm
2021-09-13 8:05 ` Geert Uytterhoeven
2021-09-15 3:28 ` Magnus Damm
2021-09-22 13:53 ` [irqchip: irq/irqchip-fixes] irqchip/gic: Work around broken Renesas integration irqchip-bot for Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 08/17] irqchip/gic: Refactor SMP configuration Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 09/17] irqchip/gic: Configure SGIs as standard interrupts Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 10/17] irqchip/gic-common: Don't enable SGIs by default Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 11/17] irqchip/bcm2836: Configure mailbox interrupts as standard interrupts Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 12/17] irqchip/hip04: Configure IPIs " Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 13/17] irqchip/armada-370-xp: " Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 14/17] arm64: Kill __smp_cross_call and co Marc Zyngier
2020-06-25 18:25 ` Valentin Schneider
2020-07-02 13:37 ` Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 15/17] arm64: Remove custom IRQ stat accounting Marc Zyngier
2020-06-25 18:26 ` Valentin Schneider
2020-06-26 11:58 ` Marc Zyngier
2020-06-26 23:15 ` Valentin Schneider
2020-06-27 11:42 ` Marc Zyngier
2020-07-10 19:58 ` Valentin Schneider
2020-06-24 19:58 ` [PATCH v2 16/17] ARM: Kill __smp_cross_call and co Marc Zyngier
2020-06-24 19:58 ` [PATCH v2 17/17] ARM: Remove custom IRQ stat accounting Marc Zyngier
2020-06-25 18:24 ` [PATCH v2 00/17] arm/arm64: Turning IPIs into normal interrupts Valentin Schneider
2020-07-10 19:58 ` Valentin Schneider
2020-08-11 13:15 ` Sumit Garg
2020-08-11 13:58 ` Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMuHMdV+ydPaXbGf1_O0S-juaPWk1gwBUOK+GeLZukZeoqtMGQ@mail.gmail.com \
--to=geert@linux-m68k.org \
--cc=Valentin.Schneider@arm.com \
--cc=andrew@lunn.ch \
--cc=catalin.marinas@arm.com \
--cc=damm+renesas@opensource.se \
--cc=f.fainelli@gmail.com \
--cc=gregory.clement@bootlin.com \
--cc=jason@lakedaemon.net \
--cc=kernel-team@android.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=maz@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=stable@vger.kernel.org \
--cc=sumit.garg@linaro.org \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).