"BUG: Invalid wait context" in ls_extirq_set_type

"BUG: Invalid wait context" in ls_extirq_set_type

[Vladimir Oltean]

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:

[    7.530319] =============================
[    7.534316] [ BUG: Invalid wait context ]
[    7.538313] 5.14.0-rc6-07010-ga9b9500ffaac-dirty #603 Not tainted
[    7.544394] -----------------------------
[    7.548391] swapper/0/1 is trying to lock:
[    7.552475] ffff15fa400ef018 (syscon:110:(&syscon_config)->lock){....}-{3:3}, at: regmap_lock_spinlock+0x18/0x30
[    7.562652] other info that might help us debug this:
[    7.567690] context-{5:5}
[    7.570299] 4 locks held by swapper/0/1:
[    7.574209]  #0: ffff15fa4188e9b0 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xf8/0x1a0
[    7.582558]  #1: ffff15fa449431b0 (&dev->mutex){....}-{4:4}, at: __device_attach+0x3c/0x180
[    7.590903]  #2: ffff15fa4491f688 (&desc->request_mutex){+.+.}-{4:4}, at: __setup_irq+0xb8/0x790
[    7.599683]  #3: ffff15fa4491f4f8 (&irq_desc_lock_class){-...}-{2:2}, at: __setup_irq+0xdc/0x790
[    7.608462] stack backtrace:
[    7.611331] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc6-07010-ga9b9500ffaac-dirty #603
[    7.625837] Call trace:
[    7.642190]  __lock_acquire+0x91c/0x1cfc
[    7.646103]  lock_acquire.part.0+0xe4/0x220
[    7.650275]  lock_acquire+0x68/0x8c
[    7.653753]  _raw_spin_lock_irqsave+0x88/0x144
[    7.658185]  regmap_lock_spinlock+0x18/0x30
[    7.662358]  regmap_update_bits_base+0x44/0xa0
[    7.666791]  ls_extirq_set_type+0x7c/0xbc
[    7.670791]  __irq_set_trigger+0x60/0x18c
[    7.674789]  __setup_irq+0x2b0/0x790
[    7.678352]  request_threaded_irq+0xec/0x1b0
[    7.682611]  devm_request_threaded_irq+0x80/0xfc
[    7.687219]  pcf2127_probe.constprop.0+0x2e8/0x470
[    7.691999]  pcf2127_i2c_probe+0x90/0xdc
[    7.695910]  i2c_device_probe+0x320/0x360

Now, the complication is that the regmap_config for the ls-extirq driver
is not even managed by itself, it is provided by syscon_node_to_regmap().

In __regmap_init, I see the various locking options are:
- Disable locking. Again, with the regmap_config provided by the generic
  syscon driver, I think making this change would be rather overreaching.
- Use a spinlock. This is what we have now, but cannot be used from a
  context that holds a raw spinlock.
- Use a mutex. Doesn't help.
- Use a hwspinlock. Never used one, don't know what it's about?!

Either way, there seems to be no option to use a raw spinlock. "Disable
locking" sounds like the best bet, but regmap offers regmap_bulk_read()
and regmap_bulk_write() and those would be broken without locking taken
at the syscon driver level, I think?

I did google for fixes for this kind of issues that were sent in the
past, but in my limited search I did not find any other driver which
uses regmap while under a raw spinlock.

What to do?

Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Wed, Aug 25, 2021 at 04:54:38PM +0300, Vladimir Oltean wrote:

> - Use a hwspinlock. Never used one, don't know what it's about?!

hwspinlocks are for locking around things that are shared with other
processors not running Linux where the other processors might also take
the lock, they're very specialist.

> Either way, there seems to be no option to use a raw spinlock. "Disable
> locking" sounds like the best bet, but regmap offers regmap_bulk_read()
> and regmap_bulk_write() and those would be broken without locking taken
> at the syscon driver level, I think?

If you disable locks at the regmap level something needs to ensure that
there's no concurrency issues which for a MFD if any of the registers
are shared sounds off.

> I did google for fixes for this kind of issues that were sent in the
> past, but in my limited search I did not find any other driver which
> uses regmap while under a raw spinlock.

No problem with adding raw spinlocks to regmap, I think it's just nobody
needed them before.  I've not looked at the problem in sufficient detail
to figure out if that's actually the best solution here but from an initial
pass through it looks reasonableit looks reasonable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Vladimir Oltean]

On Wed, Aug 25, 2021 at 04:54:27PM +0100, Mark Brown wrote:
> > I did google for fixes for this kind of issues that were sent in the
> > past, but in my limited search I did not find any other driver which
> > uses regmap while under a raw spinlock.
> 
> No problem with adding raw spinlocks to regmap, I think it's just nobody
> needed them before.  I've not looked at the problem in sufficient detail
> to figure out if that's actually the best solution here but from an initial
> pass through it looks reasonableit looks reasonable

The question becomes how will syscon be told that one of its consumers
needs the regmap to use raw spinlock locking? The syscon regmap is
initialized before any of its consumers probe, AFAIU.

Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Wed, Aug 25, 2021 at 07:03:34PM +0300, Vladimir Oltean wrote:
> On Wed, Aug 25, 2021 at 04:54:27PM +0100, Mark Brown wrote:

> > No problem with adding raw spinlocks to regmap, I think it's just nobody
> > needed them before.  I've not looked at the problem in sufficient detail
> > to figure out if that's actually the best solution here but from an initial
> > pass through it looks reasonableit looks reasonable

> The question becomes how will syscon be told that one of its consumers
> needs the regmap to use raw spinlock locking? The syscon regmap is
> initialized before any of its consumers probe, AFAIU.

I'd expect it to figure this out based on the compatible string for the
syscon device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Rasmus Villemoes]

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.

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.

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.

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

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@infradead.org>
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

Re: "BUG: Invalid wait context" in ls_extirq_set_type

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Thu, Aug 26, 2021 at 11:01:31AM +0200, Rasmus Villemoes wrote:

> 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.

Note that regmap doesn't have a fixed kind of locking used for all
regmaps, the individual regmaps can select which (if any) kind of lock
they want to use on a per-regmap basis.  Adding raw_spinlock support
wouldn't affect any regmap that doesn't actively select raw spinlocks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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@infradead.org>
> 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