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

From: Thomas Gleixner <tglx@linutronix.de>
To: Pavel Machek <pavel@ucw.cz>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: johannes.berg@intel.com, linux-leds@vger.kernel.org
Subject: Re: [PATCH] leds: trigger: Disable CPU trigger on PREEMPT_RT
Date: Tue, 28 Sep 2021 02:14:00 +0200	[thread overview]
Message-ID: <87bl4dblpz.ffs@tglx> (raw)
In-Reply-To: <20210927190650.GA13992@duo.ucw.cz>

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