Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Rasmus Villemoes]

On 26/08/2021 15.33, Vladimir Oltean wrote:
> On Thu, Aug 26, 2021 at 11:01:31AM +0200, Rasmus Villemoes wrote:

>> So, we've been using the irq-ls-extirq driver for years, on an RT
>> kernel, without seeing something like that. Does it require certain
>> debug knobs in .config? Or perhaps the sanity checks have been added
>> later, we've mostly been using it on 4.14.y and 4.19.y.
> 
> Grepping for "BUG: Invalid wait context" in the kernel yields a single
> hit, and answers both questions. It was added by commit
> 
> commit de8f5e4f2dc1f032b46afda0a78cab5456974f89
> Author: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Date:   Sat Mar 21 12:26:01 2020 +0100
> 
>     lockdep: Introduce wait-type checks
> 
>     Extend lockdep to validate lock wait-type context.
> 
> and selectable via "config PROVE_RAW_LOCK_NESTING".

OK, thanks. Yes, that explains it.

>>
>> I don't know what the right fix is. Am I right when a say that for !RT,
>> spinlock==raw_spinlock? If so, switching regmap's spinlock to
>> raw_spinlock would be nop for !RT and fix this issue, but would of
>> course have quite far-reaching effects on RT kernels.
> 
> far-reaching? Explain?

I was "suggesting" (or more accurately just considering the implications
of) _switching_ the core regmap code to use a raw spinlock
unconditionally for its "fast io" case. A change that would affect all
such regmaps throughout the kernel. Thus far-reaching (though IIUC a nop
for !RT).

> It is _a_single_register_, accessed once per IRQ line, all at consumer driver probe time
> (typically boot time).
> 

I know, I wrote the driver, and our platform is also a ls1021a.

> We are not switching regmap from normal to raw spinlocks, we are just
> adding the option for raw spinlocks for this one register.
> 
>> Perhaps it's silly for the driver to use syscon's regmap to access that
>> single register, maybe it should just iomap it itself, and protect
>> access with a raw_spinlock of its own. While syscon's regmap would cover
>> that intpcr register, nobody else would ever access it, so that should
>> work fine.
> 
> I believe my competence ends here, I will re-read Arnd's email too, but
> I just don't see how the syscon API gives you something that is "not a
> regmap", something you can ioremap yourself as a consumer driver.

Surely the driver could be rewritten to be completely ignorant of the
containing scfg node and just iomap the register itself.

Yes, there'd probably also be a syscon driver with a regmap covering
                     reg = <0x0 0x1570000 0x0 0x10000>, but none of the
potential other consumers of that regmap would write to intpcr through
that regmap, since intpcr is solely of use to ls-extirq. So the
ls-extirq driver could just have its own raw spinlock in "struct
ls_extirq_data" next to a "void __iomem *" pointer, replacing the
"struct regmap *syscon;" member.

Rasmus