Re: [PATCH] asm-generic/mmiowb: Get cpu in mmiowb_set_pending

[Will Deacon <willdeacon@google.com>]

On Tue, Jul 14, 2020 at 11:45:11PM -0700, Palmer Dabbelt wrote:
> > > > > > > > [<ffffffe00047365e>] regmap_mmio_write32le+0x18/0x46
> > > > > > > > [<ffffffe0005c4c26>] check_preemption_disabled+0xa4/0xaa
> > > > > > > > [<ffffffe00047365e>] regmap_mmio_write32le+0x18/0x46
> > > > > > > > [<ffffffe0004737c8>] regmap_mmio_write+0x26/0x44
> > > > > > > > [<ffffffe0004715c4>] regmap_write+0x28/0x48
> > > > > > > > [<ffffffe00043dccc>] sifive_gpio_probe+0xc0/0x1da
> > > > > > > > [<ffffffe00000113e>] rdinit_setup+0x22/0x26
> > > > > > > > [<ffffffe000469054>] platform_drv_probe+0x24/0x52
> > > > > > > > [<ffffffe000467e16>] really_probe+0x92/0x21a
> > > > > > > > [<ffffffe0004683a8>] device_driver_attach+0x42/0x4a
> > > > > > > > [<ffffffe0004683ac>] device_driver_attach+0x46/0x4a
> > > > > > > > [<ffffffe0004683f0>] __driver_attach+0x40/0xac
> > > > > > > > [<ffffffe0004683ac>] device_driver_attach+0x46/0x4a
> > > > > > > > [<ffffffe000466a3e>] bus_for_each_dev+0x3c/0x64
> > > > > > > > [<ffffffe000467118>] bus_add_driver+0x11e/0x184
> > > > > > > > [<ffffffe00046889a>] driver_register+0x32/0xc6
> > > > > > > > [<ffffffe00000e5ac>] gpiolib_sysfs_init+0xaa/0xae
> > > > > > > > [<ffffffe0000019ec>] 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 <asm/smp.h>
 
 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)