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=-13.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 C1C56C4332F for ; Thu, 9 Sep 2021 15:22:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8F9E960E05 for ; Thu, 9 Sep 2021 15:22:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238416AbhIIPXZ (ORCPT ); Thu, 9 Sep 2021 11:23:25 -0400 Received: from mail-vk1-f178.google.com ([209.85.221.178]:38849 "EHLO mail-vk1-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232656AbhIIPXX (ORCPT ); Thu, 9 Sep 2021 11:23:23 -0400 Received: by mail-vk1-f178.google.com with SMTP id k124so845408vke.5; Thu, 09 Sep 2021 08:22:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jMdZxCtACotNgu9fcSUDCRRpS8pLXrlQN9yO6urHYyg=; b=w7+QDR7LdJfgxtECbMMqXzBUcJvueG743R0dwLScZiGXnLbTN869yZ2jpkbMC06QaK ZiBbp/7NOeWhMxa69+xRohQQMdfm1E/QCjXLdxUhlyIJw28LNV4bwPvoH27tH7Hpb4xp y04i9sHX8aNBzyIKx4HrnpHY6W52eQ+ZwjVbwruCukUgXJd9wtwscQCbRPnjoHZrMGW5 kevtVsWMd0f/AjBLo4w5gC93siiPVIIjZLdG3G+0H1/vUClhawh7D1XBlacCPHiEx3QX fNkdzeeqkka/W7cFMR8mYIDIDKqsAO0n/LLoMQH24OfTxPzjV9VAZ8X+ya8fHS47XgTR ZZ8w== X-Gm-Message-State: AOAM530ygaFR/a1e1XurGzFMapemUopC/+xVpjjKbTocvbZ8cj3j6/p7 5K5X8tPg1DnUE6FgyZuVZaMDfsyDXYhxY0FieDY= X-Google-Smtp-Source: ABdhPJy4AEeHXVpZsVsOzXcULq6QeeWxboOzc3kpSRyQieHqZC+uyu5zFuVnW1w0ubYQbeCDPw1BGP5cIZOPos8YxVI= X-Received: by 2002:a1f:2055:: with SMTP id g82mr1978381vkg.11.1631200933555; Thu, 09 Sep 2021 08:22:13 -0700 (PDT) MIME-Version: 1.0 References: <20200624195811.435857-1-maz@kernel.org> <20200624195811.435857-8-maz@kernel.org> In-Reply-To: <20200624195811.435857-8-maz@kernel.org> From: Geert Uytterhoeven Date: Thu, 9 Sep 2021 17:22:01 +0200 Message-ID: Subject: Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity To: Marc Zyngier , Russell King Cc: 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 , =?UTF-8?Q?Niklas_S=C3=B6derlund?= , Linux-Renesas Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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? 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? Thanks for your comments! > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) > static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > bool force) > { > - void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); > - unsigned int cpu, shift = (gic_irq(d) % 4) * 8; > - u32 val, mask, bit; > - unsigned long flags; > + void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d); > + unsigned int cpu; > > if (!force) > cpu = cpumask_any_and(mask_val, cpu_online_mask); > @@ -342,13 +340,7 @@ 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; > > - gic_lock_irqsave(flags); > - mask = 0xff << shift; > - bit = gic_cpu_map[cpu] << shift; > - val = readl_relaxed(reg) & ~mask; > - writel_relaxed(val | bit, reg); > - gic_unlock_irqrestore(flags); > - > + writeb_relaxed(gic_cpu_map[cpu], reg); > irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > return IRQ_SET_MASK_OK_DONE; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds 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.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 79179C433EF for ; Thu, 9 Sep 2021 15:24:11 +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 3E60B60E05 for ; Thu, 9 Sep 2021 15:24:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 3E60B60E05 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-m68k.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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=KQU4Iohj5l5Ej54zCnlz/0u+ciUJue2a0HUN06LbFjo=; b=kzc9EqQfnXt08Z aWvZs/4gpkXIWA7fgfDMFGSTSLjM5sFNo4BYN1y53xBCvCqoQRTxcQrjuulPhYhX9Lsrh6nfpTfz9 rw6+o0RrZ7ZNpTPCfCZgF/oI1d2HtvcWvFKmoajmLl9fnJnd7zKb1WCnQ/PQohCWhUSq1CzRTtxc8 aaWfebyLhupCDOw+xZBeO5qKjsO/20rkvAqSZpgkSKM9ZOO3UB9EwhNmI1PuEuXq5e52F3TDLjA3+ PeedLgtpcg8cEb+ZCBaILVdy/Gf1+UvUGQjaDBTmK2hLphLFmoWTO0M5iAMzFqGFTYMF/MEOEfbEN W+ky6X+9xLLMRsu2wSbg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOLsX-00AGkC-OT; Thu, 09 Sep 2021 15:22:17 +0000 Received: from mail-vk1-f171.google.com ([209.85.221.171]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mOLsU-00AGjs-IJ for linux-arm-kernel@lists.infradead.org; Thu, 09 Sep 2021 15:22:16 +0000 Received: by mail-vk1-f171.google.com with SMTP id h13so836353vkc.12 for ; Thu, 09 Sep 2021 08:22:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jMdZxCtACotNgu9fcSUDCRRpS8pLXrlQN9yO6urHYyg=; b=XbCkyOVILeAM+qLkYK8VgNbT8VczuMlh9dXFEM9Jl3PLvscjM/Ck1xziIwaRokZxfZ fgjVrwXNgAHp4uHNb26EWkfPfVnOBqLcywnZ4GRPMALKRz9VRJAIrZZS/WROCseptuM1 bvjZu9yqEKgrMoz09k8dpO+UcNQi/xHRVzqrKZeNCNE6oPB8Yljs/FDPGxsS+m9eumTk 8Hc00m6rhA7/OTiK0K1GdFNpjA12ncWzSsqBKlbLnHIlFi1HHTA7n+Oscq0qoZegellr sFvWmnxWcJ16Lt6ZVrzgJw9bx+/MZxFKXe1b6AJ49NiYjnFq7e1bRv19oTdD0lqnKHBh 3Z6A== X-Gm-Message-State: AOAM5317SAzkIBFI7JRMBU8Nn1wCMV+L3f4uBNuZWIYZF9U0lnB4LFSc iGKkC+X4qDsv5wrp0GC4uf/arzSAjNY5ddRF5mc= X-Google-Smtp-Source: ABdhPJy4AEeHXVpZsVsOzXcULq6QeeWxboOzc3kpSRyQieHqZC+uyu5zFuVnW1w0ubYQbeCDPw1BGP5cIZOPos8YxVI= X-Received: by 2002:a1f:2055:: with SMTP id g82mr1978381vkg.11.1631200933555; Thu, 09 Sep 2021 08:22:13 -0700 (PDT) MIME-Version: 1.0 References: <20200624195811.435857-1-maz@kernel.org> <20200624195811.435857-8-maz@kernel.org> In-Reply-To: <20200624195811.435857-8-maz@kernel.org> From: Geert Uytterhoeven Date: Thu, 9 Sep 2021 17:22:01 +0200 Message-ID: Subject: Re: [PATCH v2 07/17] irqchip/gic: Atomically update affinity To: Marc Zyngier , Russell King Cc: 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 , =?UTF-8?Q?Niklas_S=C3=B6derlund?= , Linux-Renesas X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210909_082214_651140_16C29AD9 X-CRM114-Status: GOOD ( 35.29 ) 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 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. 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? 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? Thanks for your comments! > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -329,10 +329,8 @@ static int gic_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu) > static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > bool force) > { > - void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); > - unsigned int cpu, shift = (gic_irq(d) % 4) * 8; > - u32 val, mask, bit; > - unsigned long flags; > + void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + gic_irq(d); > + unsigned int cpu; > > if (!force) > cpu = cpumask_any_and(mask_val, cpu_online_mask); > @@ -342,13 +340,7 @@ 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; > > - gic_lock_irqsave(flags); > - mask = 0xff << shift; > - bit = gic_cpu_map[cpu] << shift; > - val = readl_relaxed(reg) & ~mask; > - writel_relaxed(val | bit, reg); > - gic_unlock_irqrestore(flags); > - > + writeb_relaxed(gic_cpu_map[cpu], reg); > irq_data_update_effective_affinity(d, cpumask_of(cpu)); > > return IRQ_SET_MASK_OK_DONE; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel