From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH] asm-generic/mmiowb: Get cpu in mmiowb_set_pending Date: Wed, 15 Jul 2020 11:42:46 +0100 Message-ID: <20200715104246.GA3143299@google.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44700 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729010AbgGOKmx (ORCPT ); Wed, 15 Jul 2020 06:42:53 -0400 Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 56018C061755 for ; Wed, 15 Jul 2020 03:42:53 -0700 (PDT) Received: by mail-wr1-x443.google.com with SMTP id z2so2089258wrp.2 for ; Wed, 15 Jul 2020 03:42:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Palmer Dabbelt Cc: kernel@esmil.dk, guoren@kernel.org, linux-riscv@lists.infradead.org, Arnd Bergmann , Paul Walmsley , linux-arch@vger.kernel.org On Tue, Jul 14, 2020 at 11:45:11PM -0700, Palmer Dabbelt wrote: > > > > > > > > [] regmap_mmio_write32le+0x18/0x46 > > > > > > > > [] check_preemption_disabled+0xa4/0xaa > > > > > > > > [] regmap_mmio_write32le+0x18/0x46 > > > > > > > > [] regmap_mmio_write+0x26/0x44 > > > > > > > > [] regmap_write+0x28/0x48 > > > > > > > > [] sifive_gpio_probe+0xc0/0x1da > > > > > > > > [] rdinit_setup+0x22/0x26 > > > > > > > > [] platform_drv_probe+0x24/0x52 > > > > > > > > [] really_probe+0x92/0x21a > > > > > > > > [] device_driver_attach+0x42/0x4a > > > > > > > > [] device_driver_attach+0x46/0x4a > > > > > > > > [] __driver_attach+0x40/0xac > > > > > > > > [] device_driver_attach+0x46/0x4a > > > > > > > > [] bus_for_each_dev+0x3c/0x64 > > > > > > > > [] bus_add_driver+0x11e/0x184 > > > > > > > > [] driver_register+0x32/0xc6 > > > > > > > > [] gpiolib_sysfs_init+0xaa/0xae > > > > > > > > [] do_one_initcall+0x50/0xfc > > > > > > > > Hmm.. the problem is that preemption is *not* disabled when > > > > smp_processor_id is called, right? > > > > > > Yes! > > > > > > smp_processor_id is defined as: > > > > > > * This is the normal accessor to the CPU id and should be used > > > * whenever possible. > > > * > > > * The CPU id is stable when: > > > * > > > * - IRQs are disabled; > > > * - preemption is disabled; > > > * - the task is CPU affine. > > > * > > > * When CONFIG_DEBUG_PREEMPT; we verify these assumption and WARN > > > * when smp_processor_id() is used when the CPU id is not stable. > > > > > > So regmap_write->regmap_mmio_write should be PREEMPT disabled in > > > sifive_gpio_probe(). > > > > Ah! Sorry, now I think I understand. So you're saying that the real > > problem is that the driver framework should have disabled preemption > > before calling any .probe functions, but for some reason that doesn't > > happen on RISC-V? > > I think it's actually an issue with the generic mmiowb stuff and that we should > just elide the check. I'm adding Will, for context. I'll send out a patch. Hmm. Although I _think_ something like the diff below ought to work, are you sure you want to be doing MMIO writes in preemptible context? Setting '.disable_locking = true' in 'sifive_gpio_regmap_config' implies to me that you should be handling the locking within the driver itself, and all the other regmap writes are protected by '&gc->bgpio_lock'. Given that riscv is one of the few architectures needing an implementation of mmiowb(), doing MMIO in a preemptible section seems especially dangerous as you have no way to ensure completion of the writes without adding an mmiowb() to the CPU migration path (i.e. context switch). Will --->8 diff --git a/include/asm-generic/mmiowb.h b/include/asm-generic/mmiowb.h index 9439ff037b2d..5698fca3bf56 100644 --- a/include/asm-generic/mmiowb.h +++ b/include/asm-generic/mmiowb.h @@ -27,7 +27,7 @@ #include DECLARE_PER_CPU(struct mmiowb_state, __mmiowb_state); -#define __mmiowb_state() this_cpu_ptr(&__mmiowb_state) +#define __mmiowb_state() raw_cpu_ptr(&__mmiowb_state) #else #define __mmiowb_state() arch_mmiowb_state() #endif /* arch_mmiowb_state */ @@ -35,7 +35,9 @@ DECLARE_PER_CPU(struct mmiowb_state, __mmiowb_state); static inline void mmiowb_set_pending(void) { struct mmiowb_state *ms = __mmiowb_state(); - ms->mmiowb_pending = ms->nesting_count; + + if (likely(ms->nesting_count)) + ms->mmiowb_pending = ms->nesting_count; } static inline void mmiowb_spin_lock(void)