Re: [bug report] leds: ns2: Remove work queue

[Simon Guinot <simon.guinot@sequanux.org>]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]