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)
