From: Marc Zyngier <maz@kernel.org>
To: Lukas Wunner <lukas@wunner.de>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
Florian Fainelli <f.fainelli@gmail.com>,
Ray Jui <rjui@broadcom.com>,
Scott Branden <sbranden@broadcom.com>,
bcm-kernel-feedback-list@broadcom.com,
linux-kernel@vger.kernel.org,
linux-rpi-kernel@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
Serge Schneider <serge@raspberrypi.org>,
Kristina Brooks <notstina@gmail.com>,
Stefan Wahren <wahrenst@gmx.net>,
Matthias Brugger <mbrugger@suse.com>,
Martin Sperl <kernel@martin.sperl.org>,
Phil Elwell <phil@raspberrypi.org>
Subject: Re: [PATCH] irqchip/bcm2835: Quiesce IRQs left enabled by bootloader
Date: Fri, 07 Feb 2020 16:11:59 +0000 [thread overview]
Message-ID: <713627a200d9c8fd7cac424d69e98166@kernel.org> (raw)
In-Reply-To: <988737dbbc4e499c2faaaa4e567ba3ed8deb9a89.1581089797.git.lukas@wunner.de>
Hi Lukas,
On 2020-02-07 15:46, Lukas Wunner wrote:
> Customers of our "Revolution Pi" open source PLCs (which are based on
> the Raspberry Pi) have reported random lockups as well as jittery eMMC,
> UART and SPI latency. We were able to reproduce the lockups in our lab
> and hooked up a JTAG debugger:
>
> It turns out that the USB controller's interrupt is already enabled
> when
> the kernel boots. All interrupts are disabled when the chip comes out
> of power-on reset, according to the spec. So apparently the bootloader
> enables the interrupt but neglects to disable it before handing over
> control to the kernel.
>
> The bootloader is a closed source blob provided by the Raspberry Pi
> Foundation. Development of an alternative open source bootloader was
> begun by Kristina Brooks but it's not fully functional yet. Usage of
> the blob is thus without alternative for the time being.
>
> The Raspberry Pi Foundation's downstream kernel has a performance-
> optimized USB driver (which we use on our Revolution Pi products).
> The driver takes advantage of the FIQ fast interrupt. Because the
> regular USB interrupt was left enabled by the bootloader, both the
> FIQ and the normal interrupt is enabled once the USB driver probes.
>
> The spec has the following to say on simultaneously enabling the FIQ
> and the normal interrupt of a peripheral:
>
> "One interrupt source can be selected to be connected to the ARM FIQ
> input. An interrupt which is selected as FIQ should have its normal
> interrupt enable bit cleared. Otherwise a normal and an FIQ interrupt
> will be fired at the same time. Not a good idea!"
Or to spell it out more clearly: Braindead hardware. Really.
> ^^^^^^^^^^^^^^^
> https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> page 110
>
> On a multicore Raspberry Pi, the Foundation's kernel routes all normal
> interrupts to CPU 0 and the FIQ to CPU 1. Because both the FIQ and the
> normal interrupt is enabled, a USB interrupt causes CPU 0 to spin in
> bcm2836_chained_handle_irq() until the FIQ on CPU 1 has cleared it.
> Interrupts with a lower priority than USB are starved as long.
>
> That explains the jittery eMMC, UART and SPI latency: On one occasion
> I've seen CPU 0 blocked for no less than 2.9 msec. Basically,
> everything not USB takes a performance hit: Whereas eMMC throughput
> on a Compute Module 3 remains relatively constant at 23.5 MB/s with
> this commit, it irregularly dips to 23.0 MB/s without this commit.
>
> The lockups occur when CPU 0 receives a USB interrupt while holding a
> lock which CPU 1 is trying to acquire while the FIQ is temporarily
> disabled on CPU 1.
>
> I've tested old releases of the Foundation's bootloader as far back as
> 1.20160202-1 and they all leave the USB interrupt enabled. Still older
> releases fail to boot a contemporary kernel on a Compute Module 1 or 3,
> which are the only Raspberry Pi variants I have at my disposal for
> testing.
>
> Fix by disabling IRQs left enabled by the bootloader. Although the
> impact is most pronounced on the Foundation's downstream kernel,
> it seems prudent to apply the fix to the upstream kernel to guard
> against such mistakes in any present and future bootloader.
>
> An alternative, though more convoluted approach would be to clear the
> IRQD_IRQ_MASKED flag on all interrupts left enabled on boot. Then the
> first invocation of handle_level_irq() would mask and thereby quiesce
> those interrupts.
Nah, that's terrible. The right thing to do is indeed to mop up the mess
that the bootloader is bound to leave and start with a clean slate.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: Serge Schneider <serge@raspberrypi.org>
> Cc: Kristina Brooks <notstina@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> drivers/irqchip/irq-bcm2835.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/irqchip/irq-bcm2835.c
> b/drivers/irqchip/irq-bcm2835.c
> index 418245d31921..0d9a5a7ebe2c 100644
> --- a/drivers/irqchip/irq-bcm2835.c
> +++ b/drivers/irqchip/irq-bcm2835.c
> @@ -150,6 +150,13 @@ static int __init armctrl_of_init(struct
> device_node *node,
> intc.enable[b] = base + reg_enable[b];
> intc.disable[b] = base + reg_disable[b];
>
> + irq = readl(intc.enable[b]);
readl_relaxed(), please. irq is not quite the right type either, please
use a u32.
> + if (irq) {
> + writel(irq, intc.disable[b]);
writel_relaxed().
> + pr_err(FW_BUG "Bootloader left irq enabled: "
> + "bank %d irq %*pbl\n", b, IRQS_PER_BANK, &irq);
> + }
> +
> for (i = 0; i < bank_irqs[b]; i++) {
> irq = irq_create_mapping(intc.domain, MAKE_HWIRQ(b, i));
> BUG_ON(irq <= 0);
Don't you need to do something about the FIQ side as well?
M.
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2020-02-07 16:12 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-07 15:46 [PATCH] irqchip/bcm2835: Quiesce IRQs left enabled by bootloader Lukas Wunner
2020-02-07 16:11 ` Marc Zyngier [this message]
2020-02-10 9:52 ` [PATCH v2] " Lukas Wunner
2020-02-12 4:47 ` Florian Fainelli
2020-02-12 8:13 ` Marc Zyngier
2020-02-12 12:36 ` Lukas Wunner
2020-02-12 12:55 ` Nicolas Saenz Julienne
2020-02-23 17:59 ` Stefan Wahren
2020-02-23 18:24 ` Lukas Wunner
2020-02-24 9:21 ` Stefan Wahren
2020-02-25 9:50 ` [PATCH v4] " Lukas Wunner
2020-03-29 20:26 ` [tip: irq/core] " tip-bot2 for Lukas Wunner
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=713627a200d9c8fd7cac424d69e98166@kernel.org \
--to=maz@kernel.org \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=f.fainelli@gmail.com \
--cc=jason@lakedaemon.net \
--cc=kernel@martin.sperl.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=lukas@wunner.de \
--cc=mbrugger@suse.com \
--cc=notstina@gmail.com \
--cc=nsaenzjulienne@suse.de \
--cc=phil@raspberrypi.org \
--cc=rjui@broadcom.com \
--cc=sbranden@broadcom.com \
--cc=serge@raspberrypi.org \
--cc=tglx@linutronix.de \
--cc=wahrenst@gmx.net \
/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).