Hi
On Wed, Dec 15, 2021 at 09:39:55PM +0100, Pavel Machek wrote:
> Hi!
>
> > Hello LED devs,
> >
> > The patch c29e650b3af2: "leds: ns2: Remove work queue" from Nov 20,
> > 2015, leads to the following Smatch static checker warning:
> >
> > drivers/leds/leds-ns2.c:96 ns2_led_set_mode()
> > warn: sleeping in atomic context
>
> Yup, this looks wrong.
It's been a very long time since I watched this pilot or even the LED
subsystem.
The "can_sleep" code seems to me quite the same as for the led_gpio
driver. If can_sleep is set (because for example the GPIO controller
uses I2C), then the brightness_set_blocking function should be used and
then ns2_led_set() can't be called in atomic context. And also the
gpiod_set_value_cansleep variants are used.
Where is it wrong ?
>
> Plus, the code is quite crazy.
Well, there has been many changes since the version I wrote.
>
> Not sure what the write_lock in that function is supposed to protect
> against. Perhaps it can be just removed?
At the time it was possible for brightness_set() to be called in user
context (through the sysfs interface) or in interrupt/timer context
(from a trigger). And because the LED is controlled through two GPIOs,
then ns2_led_set_mode() needs to be protected against reentrancy. That's
why the write_{lock_irqsave,unlock_irqrestore} functions have been used
here.
I have no idea about what is the context situation now.
>
> Hmm. led_set_mode uses custom interface for hardware accelerated
> LED. Ideally there's more fixing to be done there :-(.
At the time, nothing else was available and the LED API was crazy.
Simon
Attachment:
signature.asc
Description: PGP signature
