Re: "leds: trigger: use RCU to protect the led_cdevs list" triggers RCU error checks

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

> So I just checked and the following LED drivers all have
> a blink_set() implementation which calls mutex_lock()
> and/or does I2C transfers:

<snip>

Yay ...

> And looking at: include/linux/leds.h
> 
> Then the brightness_set callback is explicitly marked as
> "Must not sleep." (and there is a brightness_set_blocking
> which e.g. I2C drivers can use instead).
> 
> So I believe that the intention has always been that
> a driver's blink_set callback is allowed to sleep.

I guess I wouldn't really go that far from the lack of a comment saying
it cannot sleep :-)


> With that said you seem to be right that there seems to
> be a long standing bug where led_trigger_blink[_oneshot]
> calls led_classdev.blink_set() in a context where it may
> not sleep.
> 
> But that is more of a LED (trigger) core bug then an
> issue with the driver(s).

IMHO that's arguable, but I'm not going to quibble over it.

> Hmm, so the irqsave part of this was introduced by commit 27af8e2c90fb
> ("leds: trigger: fix potential deadlock with libata") 

Indeed.

> but even before
> then I think sleeping here was not allowed, given that an rwlock is
> a spinlock variant the non irqsave version also leads to a section
> where sleeping is not allowed I believe.

Right. Which goes back to 0b9536c95709 ("leds: Add ability to blink via
simple trigger") where blinking was made possible from a trigger ...

> Further git archeology seems to indicate that this problem has existed
> for a long long time already. I guess I'm the first user of a trigger
> which calls led_trigger_blink[_oneshot] on a led_classdev with
> a hw blink_set() implementation which sleeps. Or at least I'm
> the first user to do so with various lock-debugging options
> enabled ...

Right...

> but this seems to be a (trigger) core bug
> so lets try to solve it there.
> 

It's not so easy to fix I guess, other than maybe to defer to a
workqueue, but then you have the issue of cancelling?

Note that led_trigger_blink() also doesn't document when it's allowed to
be called, and at least in mac80211 (my code, yay) we're calling it from
a timer, so can't possibly sleep there. There's only one other caller in
the power supply code, but that can actually sleep.

I'd have a least thought of srcu if that timer weren't the case in
mac80211, but as is that doesn't help ...

So not sure. Clearly it's a long-standing issue, and given that many
drivers are affected probably better to fix it in the LED core, but I
don't really know my way around it very well either.

johannes