Re: [RFC][PATCH] irqchip/stm32: Retrigger hierarchy for LEVEL triggered IRQs in tasklet

[Marc Zyngier <maz@xxxxxxxxxx>]

On Fri, 15 Apr 2022 22:55:50 +0100,
Marek Vasut <marex@xxxxxxx> 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@xxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>
> Cc: Fabien Dessenne <fabien.dessenne@xxxxxxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> To: linux-gpio@xxxxxxxxxxxxxxx
> ---
>  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.