From: Geert Uytterhoeven <geert@linux-m68k.org> To: Marc Zyngier <maz@kernel.org>, Russell King <linux@arm.linux.org.uk> Cc: "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: Thu, 9 Sep 2021 17:22:01 +0200 [thread overview] Message-ID: <CAMuHMdV+Ev47K5NO8XHsanSq5YRMCHn2gWAQyV-q2LpJVy9HiQ@mail.gmail.com> (raw) In-Reply-To: <20200624195811.435857-8-maz@kernel.org> Hi Marc, Russell, 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. 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? 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? Thanks for your comments! > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) > static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > bool force) > { > - void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); > - unsigned int cpu, shift = (gic_irq(d) % 4) * 8; > - u32 val, mask, bit; > - unsigned long flags; > + void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d); > + unsigned int cpu; > > if (!force) > cpu = cpumask_any_and(mask_val, cpu_online_mask); > @@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) > return -EINVAL; > > - gic_lock_irqsave(flags); > - mask = 0xff << shift; > - bit = gic_cpu_map[cpu] << shift; > - val = readl_relaxed(reg) & ~mask; > - writel_relaxed(val | bit, reg); > - gic_unlock_irqrestore(flags); > - > + writeb_relaxed(gic_cpu_map[cpu], reg); > irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > return IRQ_SET_MASK_OK_DONE; 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
WARNING: multiple messages have this Message-ID (diff)
From: Geert Uytterhoeven <geert@linux-m68k.org> To: Marc Zyngier <maz@kernel.org>, Russell King <linux@arm.linux.org.uk> Cc: "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: Thu, 9 Sep 2021 17:22:01 +0200 [thread overview] Message-ID: <CAMuHMdV+Ev47K5NO8XHsanSq5YRMCHn2gWAQyV-q2LpJVy9HiQ@mail.gmail.com> (raw) In-Reply-To: <20200624195811.435857-8-maz@kernel.org> Hi Marc, Russell, 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. 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? 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? Thanks for your comments! > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) > static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > bool force) > { > - void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); > - unsigned int cpu, shift = (gic_irq(d) % 4) * 8; > - u32 val, mask, bit; > - unsigned long flags; > + void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d); > + unsigned int cpu; > > if (!force) > cpu = cpumask_any_and(mask_val, cpu_online_mask); > @@ -342,13 +340,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) > return -EINVAL; > > - gic_lock_irqsave(flags); > - mask = 0xff << shift; > - bit = gic_cpu_map[cpu] << shift; > - val = readl_relaxed(reg) & ~mask; > - writel_relaxed(val | bit, reg); > - gic_unlock_irqrestore(flags); > - > + writeb_relaxed(gic_cpu_map[cpu], reg); > irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > return IRQ_SET_MASK_OK_DONE; 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-09 15:22 UTC|newest] Thread overview: 99+ 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 ` Marc Zyngier 2020-06-24 19:57 ` [PATCH v2 01/17] genirq: Add fasteoi IPI flow Marc Zyngier 2020-06-24 19:57 ` 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 ` Marc Zyngier 2020-06-24 19:57 ` [PATCH v2 03/17] arm64: Allow IPIs to be handled as normal interrupts Marc Zyngier 2020-06-24 19:57 ` Marc Zyngier 2020-06-25 18:25 ` Valentin Schneider 2020-06-25 18:25 ` Valentin Schneider 2020-07-10 19:58 ` Valentin Schneider 2020-07-10 19:58 ` Valentin Schneider 2020-06-24 19:57 ` [PATCH v2 04/17] ARM: " Marc Zyngier 2020-06-24 19:57 ` Marc Zyngier 2020-06-25 18:25 ` Valentin Schneider 2020-06-25 18:25 ` Valentin Schneider 2020-06-29 9:37 ` Marc Zyngier 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:57 ` Marc Zyngier 2020-06-24 19:58 ` [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-06-25 18:25 ` Valentin Schneider 2020-06-25 18:25 ` Valentin Schneider 2020-06-30 10:15 ` Marc Zyngier 2020-06-30 10:15 ` Marc Zyngier 2020-07-02 13:23 ` Valentin Schneider 2020-07-02 13:23 ` Valentin Schneider 2020-07-02 13:48 ` Marc Zyngier 2020-07-02 13:48 ` Marc Zyngier 2020-07-02 14:24 ` Valentin Schneider 2020-07-02 14:24 ` Valentin Schneider 2020-06-24 19:58 ` [PATCH v2 07/17] irqchip/gic: Atomically update affinity Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-07-01 19:33 ` Sasha Levin 2020-07-01 19:33 ` Sasha Levin 2020-07-10 14:02 ` Sasha Levin 2020-07-10 14:02 ` Sasha Levin 2021-09-09 15:22 ` Geert Uytterhoeven [this message] 2021-09-09 15:22 ` Geert Uytterhoeven 2021-09-09 15:37 ` Russell King (Oracle) 2021-09-09 15:37 ` Russell King (Oracle) 2021-09-10 10:22 ` Marc Zyngier 2021-09-10 10:22 ` Marc Zyngier 2021-09-10 13:19 ` Geert Uytterhoeven 2021-09-10 13:19 ` Geert Uytterhoeven 2021-09-11 2:49 ` Magnus Damm 2021-09-11 2:49 ` Magnus Damm 2021-09-11 19:32 ` Marc Zyngier 2021-09-11 19:32 ` Marc Zyngier 2021-09-12 5:40 ` Magnus Damm 2021-09-12 5:40 ` Magnus Damm 2021-09-13 8:05 ` Geert Uytterhoeven 2021-09-13 8:05 ` Geert Uytterhoeven 2021-09-15 3:28 ` Magnus Damm 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 ` 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 ` 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 ` 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 ` Marc Zyngier 2020-06-24 19:58 ` [PATCH v2 12/17] irqchip/hip04: Configure IPIs " Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-06-24 19:58 ` [PATCH v2 13/17] irqchip/armada-370-xp: " Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-06-24 19:58 ` [PATCH v2 14/17] arm64: Kill __smp_cross_call and co Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-06-25 18:25 ` Valentin Schneider 2020-06-25 18:25 ` Valentin Schneider 2020-07-02 13:37 ` Marc Zyngier 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-24 19:58 ` Marc Zyngier 2020-06-25 18:26 ` Valentin Schneider 2020-06-25 18:26 ` Valentin Schneider 2020-06-26 11:58 ` Marc Zyngier 2020-06-26 11:58 ` Marc Zyngier 2020-06-26 23:15 ` Valentin Schneider 2020-06-26 23:15 ` Valentin Schneider 2020-06-27 11:42 ` Marc Zyngier 2020-06-27 11:42 ` Marc Zyngier 2020-07-10 19:58 ` Valentin Schneider 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 ` Marc Zyngier 2020-06-24 19:58 ` [PATCH v2 17/17] ARM: Remove custom IRQ stat accounting Marc Zyngier 2020-06-24 19:58 ` Marc Zyngier 2020-06-25 18:24 ` [PATCH v2 00/17] arm/arm64: Turning IPIs into normal interrupts Valentin Schneider 2020-06-25 18:24 ` Valentin Schneider 2020-07-10 19:58 ` Valentin Schneider 2020-07-10 19:58 ` Valentin Schneider 2020-08-11 13:15 ` Sumit Garg 2020-08-11 13:15 ` Sumit Garg 2020-08-11 13:58 ` Marc Zyngier 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+Ev47K5NO8XHsanSq5YRMCHn2gWAQyV-q2LpJVy9HiQ@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.