Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Vladimir Oltean]

On Thu, Aug 26, 2021 at 11:01:31AM +0200, Rasmus Villemoes wrote:
> On 25/08/2021 15.54, Vladimir Oltean wrote:
> > Hi,
> >
> > Apologies for my novice level of understanding. I see a stack trace on
> > my system and would like to understand what is the correct way to get
> > rid of it.
> >
> > I have a consumer of the drivers/irqchip/irq-ls-extirq.c driver which
> > calls request_threaded_irq.
> >
> > struct irq_desc has a lock which is a raw spinlock.
> > The __setup_irq function takes this desc->lock raw spinlock, then calls
> > __irq_set_trigger. Finally this calls chip->irq_set_type which is
> > implemented by ls_extirq_set_type.
> >
> > The problem is that ls_extirq_set_type uses regmap_update_bits, which
> > ends up taking a non-raw spin lock, the kind that becomes sleepable on RT
> > (this system is not RT, but still).
> > So that's kind of bad, and this is what the stack trace below is saying:
> >
>
> 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".

>
> 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?
It is _a_single_register_, accessed once per IRQ line, all at consumer driver probe time
(typically boot time).

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.

> Then there's your suggestion. While there's nothing wrong with adding
> raw_spinlock lock support in regmap, the fact that nobody has needed
> that till now suggests that one should at least pause and think about
> other options. And you point out the weaknesses of basing the
> .use_raw_spinlock on a .compatible string yourself.
>
> > What to do?
>
> TL;DR: I don't know.
>
> Rasmus