[PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

The CPU trigger is invoked on ARM from CPU-idle. That trigger later
invokes led_trigger_event() which acquires a read lock in an IRQ-off
section. The problematic part on PREEMPT_RT is that this lock is turned
into a sleeping lock and must not be acquired with disabled interrupts.

The problem continues as that the LED driver underneath must not acquire
any sleeping locks itself.

Disable the CPU trigger on PREEMPT_RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/leds/trigger/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 1f1d572880859..dc6816d36d069 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -64,6 +64,7 @@ config LEDS_TRIGGER_BACKLIGHT
 
 config LEDS_TRIGGER_CPU
 	bool "LED CPU Trigger"
+	depends on !PREEMPT_RT
 	help
 	  This allows LEDs to be controlled by active CPUs. This shows
 	  the active CPUs across an array of LEDs so you can see which
-- 
2.33.0

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

Hi!

> The CPU trigger is invoked on ARM from CPU-idle. That trigger later
> invokes led_trigger_event() which acquires a read lock in an IRQ-off
> section. The problematic part on PREEMPT_RT is that this lock is turned
> into a sleeping lock and must not be acquired with disabled interrupts.
> 
> The problem continues as that the LED driver underneath must not acquire
> any sleeping locks itself.
> 
> Disable the CPU trigger on PREEMPT_RT.

I know locking with leds is problematic, but I believe you'll hit
similar problem elsewhere... Disabling triggers is not solution.

Can you take a look at:

Subject: [PATCH] leds: trigger: use RCU to protect the led_cdevs list
From: Johannes Berg <johannes.berg@intel.com>

It changes affected area, and there's good chance RCU solves your
problem. (And you may be better person to review it than I am).

I'll apply Johanness' patch to the -next, and see what happens. If
someone sees a problem, speak up.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Thomas Gleixner]

Pavel,

On Mon, Sep 27 2021 at 16:23, Pavel Machek wrote:
>
>> The CPU trigger is invoked on ARM from CPU-idle. That trigger later
>> invokes led_trigger_event() which acquires a read lock in an IRQ-off
>> section. The problematic part on PREEMPT_RT is that this lock is turned
>> into a sleeping lock and must not be acquired with disabled interrupts.
>> 
>> The problem continues as that the LED driver underneath must not acquire
>> any sleeping locks itself.
>> 
>> Disable the CPU trigger on PREEMPT_RT.
>
> I know locking with leds is problematic, but I believe you'll hit
> similar problem elsewhere... Disabling triggers is not solution.

Disabling stuff which is not working well with RT is very much a
solution in order to make progress.

We have surely more essential problems to solve than making leds blink
and if anyone cares enough about them, then this can be worked on in
parallel. That has been disabled on RT for ever and so far nobody
complained or cared enough about it.

Thanks,

        tglx

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

On Mon 2021-09-27 17:35:02, Thomas Gleixner wrote:
> Pavel,
> 
> On Mon, Sep 27 2021 at 16:23, Pavel Machek wrote:
> >
> >> The CPU trigger is invoked on ARM from CPU-idle. That trigger later
> >> invokes led_trigger_event() which acquires a read lock in an IRQ-off
> >> section. The problematic part on PREEMPT_RT is that this lock is turned
> >> into a sleeping lock and must not be acquired with disabled interrupts.
> >> 
> >> The problem continues as that the LED driver underneath must not acquire
> >> any sleeping locks itself.
> >> 
> >> Disable the CPU trigger on PREEMPT_RT.
> >
> > I know locking with leds is problematic, but I believe you'll hit
> > similar problem elsewhere... Disabling triggers is not solution.
> 
> Disabling stuff which is not working well with RT is very much a
> solution in order to make progress.
> 
> We have surely more essential problems to solve than making leds blink
> and if anyone cares enough about them, then this can be worked on in
> parallel. That has been disabled on RT for ever and so far nobody
> complained or cared enough about it.

Would you mind reading and responding to the rest of the email?

I'm not applying this.
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2021-09-27 17:44:51 [+0200], Pavel Machek wrote:
> 
> Would you mind reading and responding to the rest of the email?

The patch you mentioned,
  https://https://lkml.kernel.org/r/.kernel.org/all/20210915181601.99a68f5718be.I1a28b342d2d52cdeeeb81ecd6020c25cbf1dbfc0@changeid/

would remove the lock from led_trigger_event().
Are there any guarantees how many callbacks maybe invoked and what kind
of locks may be acquired?
Leaving kworker usage aside there are still things like spinlock_t usage
in input_leds_brightness_set(), nic78bx_brightness_set() (from a quick
grep) which have the same problems.

> I'm not applying this.

I hope you reconsider. It is not all LED usage, just the CPU trigger.

> 								Pavel

Sebastian

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Johannes Berg]

On Mon, 2021-09-27 at 17:18 +0000, Sebastian Andrzej Siewior wrote:
> On 2021-09-27 17:44:51 [+0200], Pavel Machek wrote:
> > 
> > Would you mind reading and responding to the rest of the email?
> 
> The patch you mentioned,
>   https://https://lkml.kernel.org/r/.kernel.org/all/20210915181601.99a68f5718be.I1a28b342d2d52cdeeeb81ecd6020c25cbf1dbfc0@changeid/
> 
> would remove the lock from led_trigger_event().
> Are there any guarantees how many callbacks maybe invoked and what kind
> of locks may be acquired?

In a way, the point of my patch is that after it we don't have to *care*
what locking happens in the callbacks.

The problem in our driver was that we did a spinlock without disabling
IRQs there in the callback, and that interacts badly with the rwlock in
the LED subsystem.

> Leaving kworker usage aside there are still things like spinlock_t usage
> in input_leds_brightness_set(), nic78bx_brightness_set() (from a quick
> grep) which have the same problems.

Looks like *everything* (that can work on ARM) and uses a spinlock would
have this problem? My patch doesn't address that at all. In fact, like I
said above, the entire point of the patch is to remove restrictions on
what the locking in the underlying LED driver can be.

johannes

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Thomas Gleixner]

Pavel,

On Mon, Sep 27 2021 at 17:44, Pavel Machek wrote:
> On Mon 2021-09-27 17:35:02, Thomas Gleixner wrote:
>> Disabling stuff which is not working well with RT is very much a
>> solution in order to make progress.
>> 
>> We have surely more essential problems to solve than making leds blink
>> and if anyone cares enough about them, then this can be worked on in
>> parallel. That has been disabled on RT for ever and so far nobody
>> complained or cared enough about it.
>
> Would you mind reading and responding to the rest of the email?

Sure. As Sebastian said, its not only the rwlock in led-triggers, it's
the whole call chain down to led_cdec->set_brightness() with a gazillion
of drivers and quite some of them take locks.

> I'm not applying this.

That's fine with me, I just move arch/arm RT support to the very end of
my todo list then.

Thanks for your support!

       tglx

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

Hi!

> > Would you mind reading and responding to the rest of the email?
> 
> The patch you mentioned,
>   https://https://lkml.kernel.org/r/.kernel.org/all/20210915181601.99a68f5718be.I1a28b342d2d52cdeeeb81ecd6020c25cbf1dbfc0@changeid/
> 
> would remove the lock from led_trigger_event().
> Are there any guarantees how many callbacks maybe invoked and what kind
> of locks may be acquired?

These kind of functions should not sleep other than that, there are no
restrictions AFAICT.

Other triggers are relying on that non-sleeping assumption.

> Leaving kworker usage aside there are still things like spinlock_t usage
> in input_leds_brightness_set(), nic78bx_brightness_set() (from a quick
> grep) which have the same problems.
> 
> > I'm not applying this.
> 
> I hope you reconsider. It is not all LED usage, just the CPU
> trigger.

What makes the CPU trigger special with RT? Other triggers will be
called from interesting places, too... Johanes pointed out other
problems with that rwlock, and we are getting rid of the rwlock.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2021-09-27 21:06:50 [+0200], Pavel Machek wrote:
> Hi!
Hi,

> What makes the CPU trigger special with RT? Other triggers will be
> called from interesting places, too... Johanes pointed out other
> problems with that rwlock, and we are getting rid of the rwlock.

There seem to be a few misunderstandings. Let me clear them up for you:
- A non sleeping lock is spinlock_t and rwlock_t as you point out but
  only on !PREEMPT_RT. In context of PREEMPT_RT both of them are
  sleeping locks.

- An interrupt handler on PREEMPT_RT is always threaded. From threaded
  context sleeping is possible and therefore a spinlock_t can be
  acquired by regular request_irq().

- The timer callback (timer_list or hrtimer) is *usually* invoked from
  softirq context which is preemptible on PREEMPT_RT. Therefore sleeping
  is possible in order to acquire a spinlock_t. The exceptional
  callbacks (which are not invoked from softirq thus the term usually
  previously) must not acquire spinlock_t and are specially marked.

- The idle loop (x86 or ARM makes no difference) is always invoked with
  disabled interrupts and so sleeping is a no no on PREEMPT_RT. So a
  spinlock_t can not be acquired in this context.

You mention "interesting places" above. Either this is not the case or
nobody reported it yet.

> Best regards,
> 								Pavel

Sebastian

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Thomas Gleixner]

Pavel,

On Mon, Sep 27 2021 at 21:06, Pavel Machek wrote:
>> I hope you reconsider. It is not all LED usage, just the CPU
>> trigger.
>
> What makes the CPU trigger special with RT?

Care to look at the call sites?

> Other triggers will be called from interesting places, too...

Can you please define "called from interesting places" in terms of RT
related semantics?

Once you've done that you might have the courtesy to explain which RT
related problem is "too...".

May I also recommend to think about the fact that RT explicitely
disables a particular LED trigger and not ALL of them. There might be a
reason. Hint: See the first question above.

> Johanes pointed out other problems with that rwlock, and we are
> getting rid of the rwlock.

That solves the problem in which way?

May I recommend to read:

  https://www.kernel.org/doc/html/latest/locking/locktypes.html

which clearly explains the constraints of RT vs. locking.

Now if you just look at the callsites of ledtrig_cpu() in arch/arm/ then
you might notice that these are in code sections which are not
preemtible even on RT enabled kernels for obvious reasons.

Of course the primary offender on RT is the rwlock but even if you get
rid of it, how is any of the regular spinlocks which are taken in the
deeper call chain via the set_brightness() callbacks not going to cause
the same problem?

IOW, you can point us at Johannes' patch as much as you want, it won't
solve the problems in the subsequently invoked callbacks.

Sorry for not having provided enough context for you in the first place,
but I was under the impression that the CIP's SLT-RT maintainer [1]
understands at least the basic principles of RT.

And of course the stable RT kernels you maintain there contain the very
same patch, but obviously it's not a problem for those kernels because
otherwise you or someone else would have complained before.

But of course for integrating RT into mainline it's essential to support
this, right?

We're definitely going to pay more attention next time when submitting
that patch unless it becomes obsolete because someone who cares deeply
about ledtrigg_cpu() working correctly with RT enabled kernels on
obsolete hardware has fixed all underlying isues.

That hasn't happened in the past 15+ years and I'm happy to postpone any
attempt of supporting RT on arch/arm/ for another 15+ years.

Thanks,

        tglx

[1] https://wiki.linuxfoundation.org/civilinfrastructureplatform/start

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

Hi!

> - The idle loop (x86 or ARM makes no difference) is always invoked with
>   disabled interrupts and so sleeping is a no no on PREEMPT_RT. So a
>   spinlock_t can not be acquired in this context.

Ok, I have reviewed code, and your patch seems like a way forward. Can
you resubmit it with fixed changelog?

Thanks,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2021-10-13 10:08:38 [+0200], Pavel Machek wrote:
> Hi!
Hi,

> > - The idle loop (x86 or ARM makes no difference) is always invoked with
> >   disabled interrupts and so sleeping is a no no on PREEMPT_RT. So a
> >   spinlock_t can not be acquired in this context.
> 
> Ok, I have reviewed code, and your patch seems like a way forward. Can
> you resubmit it with fixed changelog?

Could you please point me what to needs to be fixed in the changelog?
I mentioned the read lock which is acquired with disabled interrupts
and this does not work on PREEMPT_RT.

> Thanks,
> 								Pavel

Sebastian

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

On Wed 2021-10-13 10:39:00, Sebastian Andrzej Siewior wrote:
> On 2021-10-13 10:08:38 [+0200], Pavel Machek wrote:
> > Hi!
> Hi,
> 
> > > - The idle loop (x86 or ARM makes no difference) is always invoked with
> > >   disabled interrupts and so sleeping is a no no on PREEMPT_RT. So a
> > >   spinlock_t can not be acquired in this context.
> > 
> > Ok, I have reviewed code, and your patch seems like a way forward. Can
> > you resubmit it with fixed changelog?
> 
> Could you please point me what to needs to be fixed in the changelog?
> I mentioned the read lock which is acquired with disabled interrupts
> and this does not work on PREEMPT_RT.

The read lock is gone in linux-next now (it was problematic for other
reasons and it was not the main problem).

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2021-10-13 10:42:37 [+0200], Pavel Machek wrote:
Hi,

> The read lock is gone in linux-next now (it was problematic for other
> reasons and it was not the main problem).

Okay, thanks for the update.

> 								Pavel

Sebastian

[PATCH v2] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Sebastian Andrzej Siewior]

The CPU trigger is invoked on ARM from CPU-idle. That trigger later
invokes led_trigger_event() which may invoke the callback of the actual driver.
That driver can acquire a spinlock_t which is okay on kernel without
PREEMPT_RT. On a PREEMPT_RT enabled kernel this lock is turned into a
sleeping lock and must not be acquired with disabled interrupts.

Disable the CPU trigger on PREEMPT_RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Link: https://lkml.kernel.org/r/20210924111501.m57cwwn7ahiyxxdd@linutronix.de
---
v1…v2: Reword commit message now that read lock has been removed.

 drivers/leds/trigger/Kconfig |    1 +
 1 file changed, 1 insertion(+)

--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -64,6 +64,7 @@ config LEDS_TRIGGER_BACKLIGHT
 
 config LEDS_TRIGGER_CPU
 	bool "LED CPU Trigger"
+	depends on !PREEMPT_RT
 	help
 	  This allows LEDs to be controlled by active CPUs. This shows
 	  the active CPUs across an array of LEDs so you can see which

Re: [PATCH v2] leds: trigger: Disable CPU trigger on PREEMPT_RT

[Pavel Machek]

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

On Wed 2021-10-13 11:37:19, Sebastian Andrzej Siewior wrote:
> The CPU trigger is invoked on ARM from CPU-idle. That trigger later
> invokes led_trigger_event() which may invoke the callback of the actual driver.
> That driver can acquire a spinlock_t which is okay on kernel without
> PREEMPT_RT. On a PREEMPT_RT enabled kernel this lock is turned into a
> sleeping lock and must not be acquired with disabled interrupts.
> 
> Disable the CPU trigger on PREEMPT_RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Link:
https://lkml.kernel.org/r/20210924111501.m57cwwn7ahiyxxdd@linutronix.de

Thanks, applied.
									Pavel
-- 
http://www.livejournal.com/~pavelmachek

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