From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3680BC433EF for ; Sat, 16 Apr 2022 09:25:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231286AbiDPJ2D (ORCPT ); Sat, 16 Apr 2022 05:28:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48630 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231326AbiDPJ17 (ORCPT ); Sat, 16 Apr 2022 05:27:59 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E73EEE65 for ; Sat, 16 Apr 2022 02:25:26 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 1D1D5B80ED0 for ; Sat, 16 Apr 2022 09:25:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC07DC385A3; Sat, 16 Apr 2022 09:25:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1650101123; bh=mUKVFgQnfP2+dMgvolMPFvUFnfQUuNFchWXotQmYL8s=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=GlVUg3FMz3mxyF5rOAWwiF7P+gtOSZJrKk/eTrxshOxjgVr8tianv6tLpjgQUnSEM PD/FFSdRyo+tGkYq04gEXmmmmjySd4p0tucszp9kZpLek3J7e4IduqRjbLLUnJ7No9 pRmCNBtYYh2gyQXsBaxk3OB3Pmswwz9vi73UH3aMM+TlXaxVdfzQd59jn86c4jD6IU zuwiqIKXhLL8HtLg21alALX7pHzsJbQP5Bi8ObguXUZAJG7Q3lPsG9pLbQQtAAWRcF vJX3s5pKTMbJzzgMtUrWsapGikFuru9OATDbdP0rFSw/3Hox+4PH1JpXk342k+esq8 qEfov3dmkTx1g== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nfegD-004itj-BU; Sat, 16 Apr 2022 10:25:21 +0100 Date: Sat, 16 Apr 2022 10:25:20 +0100 Message-ID: <87pmlhidmn.wl-maz@kernel.org> From: Marc Zyngier To: Marek Vasut Cc: linux-gpio@vger.kernel.org, Alexandre Torgue , Fabien Dessenne , Linus Walleij , Thomas Gleixner , linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC][PATCH] irqchip/stm32: Retrigger hierarchy for LEVEL triggered IRQs in tasklet In-Reply-To: <20220415215550.498381-1-marex@denx.de> References: <20220415215550.498381-1-marex@denx.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: marex@denx.de, linux-gpio@vger.kernel.org, alexandre.torgue@foss.st.com, fabien.dessenne@foss.st.com, linus.walleij@linaro.org, tglx@linutronix.de, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Fri, 15 Apr 2022 22:55:50 +0100, Marek Vasut 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) > [] (unwind_backtrace) from [] (show_stack+0xb/0xc) > [] (show_stack) from [] (dump_stack+0x6f/0x84) > [] (dump_stack) from [] (__warn+0x7f/0xa4) > [] (__warn) from [] (warn_slowpath_fmt+0x3b/0x74) > [] (warn_slowpath_fmt) from [] (__rt_mutex_trylock+0x37/0x62) > [] (__rt_mutex_trylock) from [] (rt_spin_trylock+0x7/0x16) > [] (rt_spin_trylock) from [] (clk_enable_lock+0xb/0x80) > [] (clk_enable_lock) from [] (clk_core_enable_lock+0x9/0x18) > [] (clk_core_enable_lock) from [] (stm32_gpio_get+0x11/0x24) > [] (stm32_gpio_get) from [] (stm32_gpio_irq_trigger+0x1f/0x48) > [] (stm32_gpio_irq_trigger) from [] (handle_fasteoi_irq+0x71/0xa8) > [] (handle_fasteoi_irq) from [] (generic_handle_irq+0x19/0x22) > [] (generic_handle_irq) from [] (__handle_domain_irq+0x55/0x64) > [] (__handle_domain_irq) from [] (gic_handle_irq+0x53/0x64) > [] (gic_handle_irq) from [] (__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 > [] (__irq_svc) from [] (arch_cpu_idle+0xc/0x1e) > [] (arch_cpu_idle) from [] (default_idle_call+0x21/0x3c) > [] (default_idle_call) from [] (do_idle+0xe3/0x1e4) > [] (do_idle) from [] (cpu_startup_entry+0x13/0x14) > [] (cpu_startup_entry) from [] (start_kernel+0x397/0x3d4) > [] (start_kernel) from [<00000000>] (0x0) > ---[ end trace 0000000000000002 ]--- > > Signed-off-by: Marek Vasut > Cc: Alexandre Torgue > Cc: Fabien Dessenne > Cc: Linus Walleij > Cc: Marc Zyngier > Cc: Thomas Gleixner > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: linux-arm-kernel@lists.infradead.org > To: linux-gpio@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 > #include > #include > +#include > #include > #include > #include > @@ -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.