[RFC][PATCH] irqchip/stm32: Retrigger hierarchy for LEVEL triggered IRQs in tasklet
Marc Zyngier
maz at kernel.org
Sat Apr 16 02:25:20 PDT 2022
On Fri, 15 Apr 2022 22:55:50 +0100,
Marek Vasut <marex at denx.de> wrote:
>
> The current EOI handler for LEVEL triggered interrupts calls clk_enable(),
> register IO, clk_disable(). The clock manipulation requires locking which
> happens with IRQs disabled in clk_enable_lock(). Move the LEVEL IRQ test
> and retrigger into dedicated tasklet and schedule the tasklet every time
> a LEVEL IRQ triggers. This makes EOI fast for majority of IRQs on this
> platform again, since those are edge triggered IRQs, and LEVEL triggered
> IRQs are the exception.
>
> This also fixes the following splat found when using preempt-rt:
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at kernel/locking/rtmutex.c:2040 __rt_mutex_trylock+0x37/0x62
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.10.109-rt65-stable-standard-00068-g6a5afc4b1217 #85
> Hardware name: STM32 (Device Tree Support)
> [<c010a45d>] (unwind_backtrace) from [<c010766f>] (show_stack+0xb/0xc)
> [<c010766f>] (show_stack) from [<c06353ab>] (dump_stack+0x6f/0x84)
> [<c06353ab>] (dump_stack) from [<c01145e3>] (__warn+0x7f/0xa4)
> [<c01145e3>] (__warn) from [<c063386f>] (warn_slowpath_fmt+0x3b/0x74)
> [<c063386f>] (warn_slowpath_fmt) from [<c063b43d>] (__rt_mutex_trylock+0x37/0x62)
> [<c063b43d>] (__rt_mutex_trylock) from [<c063c053>] (rt_spin_trylock+0x7/0x16)
> [<c063c053>] (rt_spin_trylock) from [<c036a2f3>] (clk_enable_lock+0xb/0x80)
> [<c036a2f3>] (clk_enable_lock) from [<c036ba69>] (clk_core_enable_lock+0x9/0x18)
> [<c036ba69>] (clk_core_enable_lock) from [<c034e9f3>] (stm32_gpio_get+0x11/0x24)
> [<c034e9f3>] (stm32_gpio_get) from [<c034ef43>] (stm32_gpio_irq_trigger+0x1f/0x48)
> [<c034ef43>] (stm32_gpio_irq_trigger) from [<c014aa53>] (handle_fasteoi_irq+0x71/0xa8)
> [<c014aa53>] (handle_fasteoi_irq) from [<c0147111>] (generic_handle_irq+0x19/0x22)
> [<c0147111>] (generic_handle_irq) from [<c014752d>] (__handle_domain_irq+0x55/0x64)
> [<c014752d>] (__handle_domain_irq) from [<c0346f13>] (gic_handle_irq+0x53/0x64)
> [<c0346f13>] (gic_handle_irq) from [<c0100ba5>] (__irq_svc+0x65/0xc0)
> Exception stack(0xc0e01f18 to 0xc0e01f60)
> 1f00: 0000300c 00000000
> 1f20: 0000300c c010ff01 00000000 00000000 c0e00000 c0e07714 00000001 c0e01f78
> 1f40: c0e07758 00000000 ef7cd0ff c0e01f68 c010554b c0105542 40000033 ffffffff
> [<c0100ba5>] (__irq_svc) from [<c0105542>] (arch_cpu_idle+0xc/0x1e)
> [<c0105542>] (arch_cpu_idle) from [<c063be95>] (default_idle_call+0x21/0x3c)
> [<c063be95>] (default_idle_call) from [<c01324f7>] (do_idle+0xe3/0x1e4)
> [<c01324f7>] (do_idle) from [<c01327b3>] (cpu_startup_entry+0x13/0x14)
> [<c01327b3>] (cpu_startup_entry) from [<c0a00c13>] (start_kernel+0x397/0x3d4)
> [<c0a00c13>] (start_kernel) from [<00000000>] (0x0)
> ---[ end trace 0000000000000002 ]---
>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>
> Cc: Fabien Dessenne <fabien.dessenne at foss.st.com>
> Cc: Linus Walleij <linus.walleij at linaro.org>
> Cc: Marc Zyngier <maz at kernel.org>
> Cc: Thomas Gleixner <tglx at linutronix.de>
> Cc: linux-stm32 at st-md-mailman.stormreply.com
> Cc: linux-arm-kernel at lists.infradead.org
> To: linux-gpio at vger.kernel.org
> ---
> drivers/pinctrl/stm32/pinctrl-stm32.c | 55 +++++++++++++++++++++------
> 1 file changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 242d1c37c6e4b..f4287fc18cf9a 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -10,6 +10,7 @@
> #include <linux/gpio/driver.h>
> #include <linux/hwspinlock.h>
> #include <linux/io.h>
> +#include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> @@ -95,6 +96,7 @@ struct stm32_gpio_bank {
> u32 bank_ioport_nr;
> u32 pin_backup[STM32_GPIO_PINS_PER_BANK];
> u8 irq_type[STM32_GPIO_PINS_PER_BANK];
> + struct tasklet_struct tasklet;
> };
>
> struct stm32_pinctrl {
> @@ -307,20 +309,43 @@ static const struct gpio_chip stm32_gpio_template = {
> .set_config = gpiochip_generic_config,
> };
>
> +static void stm32_gpio_irq_tasklet(struct tasklet_struct *t)
> +{
> + struct stm32_gpio_bank *bank = from_tasklet(bank, t, tasklet);
> + struct irq_desc *desc;
> + struct irq_data *data;
> + int irq, pin, level;
> +
> + /* Retrigger all LEVEL triggered pins which are still asserted. */
> + for (pin = 0; pin < STM32_GPIO_PINS_PER_BANK; pin++) {
> + if (!(bank->irq_type[pin] & IRQ_TYPE_LEVEL_MASK))
> + continue;
> +
> + level = stm32_gpio_get(&bank->gpio_chip, pin);
So you are looking at all the LEVEL irqs in a given bank. Given that
your initial patch was based on the idea that accessing the GPIO bank
is wasteful, this looks like a step backward.
It probably is also racy if two level interrupts are EOId on the
different CPUs, potentially resulting in the level being regenerated
multiple times (nice amplification effect).
> + if ((level == 0 && bank->irq_type[pin] == IRQ_TYPE_LEVEL_LOW) ||
> + (level == 1 && bank->irq_type[pin] == IRQ_TYPE_LEVEL_HIGH)) {
> + irq = irq_find_mapping(bank->domain, pin);
> +
> + desc = irq_to_desc(irq);
> + if (!desc)
> + continue;
Please turn the whole irq_find_mapping()+irq_to_desc() into something
that doesn't completely suck. like __irq_resolve_mapping(). Otherwise,
you get the privilege of parsing both the domain and the irqdesc tree
instead of just the former.
But dealing with a single interrupt at a time would be much better,
and would avoid all this pointless work.
> +
> + data = irq_desc_get_irq_data(desc);
> + if (!data)
> + continue;
> +
> + irq_chip_retrigger_hierarchy(data);
Err... No. You don't hold the lock for this interrupt, so you can't
blindly call into the core code like this.
A way to fix this is to implement the IRQ state callbacks in the
low-level irqchip, and call into it using the top-level API which will
take care of the locking.
M.
--
Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list