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 X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03109C433FE for ; Fri, 10 Sep 2021 10:23:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DC7B26103D for ; Fri, 10 Sep 2021 10:23:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232290AbhIJKY3 (ORCPT ); Fri, 10 Sep 2021 06:24:29 -0400 Received: from mail.kernel.org ([198.145.29.99]:53170 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232196AbhIJKYL (ORCPT ); Fri, 10 Sep 2021 06:24:11 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 98D356103D; Fri, 10 Sep 2021 10:22:59 +0000 (UTC) Received: from 82-132-222-91.dab.02.net ([82.132.222.91] 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 1mOdgP-00A1cK-B3; Fri, 10 Sep 2021 11:22:57 +0100 Date: Fri, 10 Sep 2021 11:22:56 +0100 Message-ID: <875yv8d91b.wl-maz@kernel.org> From: Marc Zyngier To: Geert Uytterhoeven Cc: Russell King , Linux ARM , Linux Kernel Mailing List , Will Deacon , Catalin Marinas , Thomas Gleixner , Jason Cooper , Sumit Garg , Valentin Schneider , Florian Fainelli , Gregory Clement , Andrew Lunn , Android Kernel Team , stable , Magnus Damm , Niklas =?UTF-8?B?U8O2ZGVybHVuZA==?= , Linux-Renesas Subject: Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity In-Reply-To: References: <20200624195811.435857-1-maz@kernel.org> <20200624195811.435857-8-maz@kernel.org> 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: 82.132.222.91 X-SA-Exim-Rcpt-To: geert@linux-m68k.org, linux@arm.linux.org.uk, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, catalin.marinas@arm.com, tglx@linutronix.de, jason@lakedaemon.net, sumit.garg@linaro.org, Valentin.Schneider@arm.com, f.fainelli@gmail.com, gregory.clement@bootlin.com, andrew@lunn.ch, kernel-team@android.com, stable@vger.kernel.org, damm+renesas@opensource.se, niklas.soderlund+renesas@ragnatech.se, linux-renesas-soc@vger.kernel.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-kernel@vger.kernel.org Hi Geert, On Thu, 09 Sep 2021 16:22:01 +0100, Geert Uytterhoeven wrote: > > Hi Marc, Russell, > > On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier 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 > > 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. Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue... > 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? Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently. > 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? There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems. I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps. M. diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index d329ec3d64d8..dca40a974b7a 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock); #endif +static DEFINE_STATIC_KEY_FALSE(needs_rmw_access); + /* * The GIC mapping of CPU interfaces does not necessarily match * the logical CPU numbering. Let's use a mapping as returned @@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic) #endif #ifdef CONFIG_SMP +static void rmw_writeb(u8 bval, void __iomem *addr) +{ + static DEFINE_RAW_SPINLOCK(rmw_lock); + unsigned long offset = (unsigned long)addr & ~3UL; + unsigned long shift = offset * 8; + unsigned long flags; + u32 val; + + raw_spin_lock_irqsave(&rmw_lock, flags); + + addr -= offset; + val = readl_relaxed(addr); + val &= ~(0xffUL << shift); + val |= (u32)bval << shift; + writel_relaxed(val, addr); + + raw_spin_unlock_irqrestore(&rmw_lock, flags); +} + static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force) { @@ -788,7 +809,10 @@ 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; - writeb_relaxed(gic_cpu_map[cpu], reg); + if (static_branch_unlikely(&needs_rmw_access)) + rmw_writeb(gic_cpu_map[cpu], reg); + else + writeb_relaxed(gic_cpu_map[cpu], reg); irq_data_update_effective_affinity(d, cpumask_of(cpu)); return IRQ_SET_MASK_OK_DONE; @@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base) return true; } +static bool gic_enable_rmw_access(void *data) +{ + /* + * The EMEV2 class of machines has a broken interconnect, and + * locks up on accesses that are less than 32bit. So far, only + * the affinity setting requires it. + */ + if (of_machine_is_compatible("renesas,emev2")) { + static_branch_enable(&needs_rmw_access); + return true; + } + + return false; +} + +static const struct gic_quirk gic_quirks[] = { + { + .desc = "Implementation with broken byte access", + .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + }, +}; + static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node) { if (!gic || !node) @@ -1391,6 +1438,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node) if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset)) gic->percpu_offset = 0; + gic_enable_of_quirks(node, gic_quirks, gic); + return 0; error: -- Without deviation from the norm, progress is not possible. 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 X-Spam-Level: X-Spam-Status: No, score=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E76ADC433F5 for ; Fri, 10 Sep 2021 10:25:47 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id AE0FC60FDC for ; Fri, 10 Sep 2021 10:25:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org AE0FC60FDC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=88TzLIv3qJzpZM7VRGenQQgUNrbDEYzMtE5LK3wyoEI=; b=jDPQ6T02BdpeiH ZjdveDqJDYdf8UW293gqodgWveuarVch2d9DpFX1BCO9Iyfv40z2ToELZmW513SAyb37l3oLIbQrt HRxdAnFwxbWog0RdFVhg3E3dIdYAfWWuzJPJ4Atyipr3yIvdicjKwSsdm9/Kjcu9mrplMKyBH9+pB TB2e7U0cIp6xc0YetFrdLjZIzeEHZ7V4HIzIg9X/Qr4Yidt64NLvAE8NWQhajNfx9kAKNt3LMsr0H 2mYYn1K4drZqGIxeYlkA8VGDCqa3+DBsKxOkyQQxSNCrIakQDPrGuzjrWMC5DqHr1vEeU9yag5ahr LRb55TFelxTMyB6sfP6g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOdgW-00CS1V-5Q; Fri, 10 Sep 2021 10:23:04 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOdgS-00CS0r-LS for linux-arm-kernel@lists.infradead.org; Fri, 10 Sep 2021 10:23:02 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 98D356103D; Fri, 10 Sep 2021 10:22:59 +0000 (UTC) Received: from 82-132-222-91.dab.02.net ([82.132.222.91] 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 1mOdgP-00A1cK-B3; Fri, 10 Sep 2021 11:22:57 +0100 Date: Fri, 10 Sep 2021 11:22:56 +0100 Message-ID: <875yv8d91b.wl-maz@kernel.org> From: Marc Zyngier To: Geert Uytterhoeven Cc: Russell King , Linux ARM , Linux Kernel Mailing List , Will Deacon , Catalin Marinas , Thomas Gleixner , Jason Cooper , Sumit Garg , Valentin Schneider , Florian Fainelli , Gregory Clement , Andrew Lunn , Android Kernel Team , stable , Magnus Damm , Niklas =?UTF-8?B?U8O2ZGVybHVuZA==?= , Linux-Renesas Subject: Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity In-Reply-To: References: <20200624195811.435857-1-maz@kernel.org> <20200624195811.435857-8-maz@kernel.org> 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") X-SA-Exim-Connect-IP: 82.132.222.91 X-SA-Exim-Rcpt-To: geert@linux-m68k.org, linux@arm.linux.org.uk, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, catalin.marinas@arm.com, tglx@linutronix.de, jason@lakedaemon.net, sumit.garg@linaro.org, Valentin.Schneider@arm.com, f.fainelli@gmail.com, gregory.clement@bootlin.com, andrew@lunn.ch, kernel-team@android.com, stable@vger.kernel.org, damm+renesas@opensource.se, niklas.soderlund+renesas@ragnatech.se, linux-renesas-soc@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210910_032300_786998_BEB10E46 X-CRM114-Status: GOOD ( 45.66 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Geert, On Thu, 09 Sep 2021 16:22:01 +0100, Geert Uytterhoeven wrote: > > Hi Marc, Russell, > > On Wed, Jun 24, 2020 at 9:59 PM Marc Zyngier 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 > > 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. Urgh. That is definitely a pretty poor integration. How about the priority registers? I guess they suffer from the same issue... > 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? Not that I know. A lock was probably fine on a two CPU system. Less so on a busy 8 CPU machine where interrupts are often migrated. The GIC architecture makes a point in not requiring locking for most of the registers that can be accessed concurrently. > 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? There isn't much we can do aside from reintroducing the RMW+spinlock approach, and for real this time. It would have to be handled as a quirk though, as I'm not keen on reintroducing this for all systems. I wrote the patchlet below, which is totally untested. Please give it a go and let me know if it helps. M. diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index d329ec3d64d8..dca40a974b7a 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -107,6 +107,8 @@ static DEFINE_RAW_SPINLOCK(cpu_map_lock); #endif +static DEFINE_STATIC_KEY_FALSE(needs_rmw_access); + /* * The GIC mapping of CPU interfaces does not necessarily match * the logical CPU numbering. Let's use a mapping as returned @@ -774,6 +776,25 @@ static int gic_pm_init(struct gic_chip_data *gic) #endif #ifdef CONFIG_SMP +static void rmw_writeb(u8 bval, void __iomem *addr) +{ + static DEFINE_RAW_SPINLOCK(rmw_lock); + unsigned long offset = (unsigned long)addr & ~3UL; + unsigned long shift = offset * 8; + unsigned long flags; + u32 val; + + raw_spin_lock_irqsave(&rmw_lock, flags); + + addr -= offset; + val = readl_relaxed(addr); + val &= ~(0xffUL << shift); + val |= (u32)bval << shift; + writel_relaxed(val, addr); + + raw_spin_unlock_irqrestore(&rmw_lock, flags); +} + static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, bool force) { @@ -788,7 +809,10 @@ 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; - writeb_relaxed(gic_cpu_map[cpu], reg); + if (static_branch_unlikely(&needs_rmw_access)) + rmw_writeb(gic_cpu_map[cpu], reg); + else + writeb_relaxed(gic_cpu_map[cpu], reg); irq_data_update_effective_affinity(d, cpumask_of(cpu)); return IRQ_SET_MASK_OK_DONE; @@ -1375,6 +1399,29 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base) return true; } +static bool gic_enable_rmw_access(void *data) +{ + /* + * The EMEV2 class of machines has a broken interconnect, and + * locks up on accesses that are less than 32bit. So far, only + * the affinity setting requires it. + */ + if (of_machine_is_compatible("renesas,emev2")) { + static_branch_enable(&needs_rmw_access); + return true; + } + + return false; +} + +static const struct gic_quirk gic_quirks[] = { + { + .desc = "Implementation with broken byte access", + .compatible = "arm,pl390", + .init = gic_enable_rmw_access, + }, +}; + static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node) { if (!gic || !node) @@ -1391,6 +1438,8 @@ static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node) if (of_property_read_u32(node, "cpu-offset", &gic->percpu_offset)) gic->percpu_offset = 0; + gic_enable_of_quirks(node, gic_quirks, gic); + return 0; error: -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel