Re: use of preempt_count instead of in_atomic() at leds-gpio.c

[Henrique de Moraes Holschuh <hmh@hmh.eng.br>]

On Fri, 21 Mar 2008, Richard Purdie wrote:
> The LED interface said that the brightness_set implementation should not
> sleep since it was intended to be a 'cheap' function and to allow LED
> triggers changing the LED brightness to be simple. A lot of embedded LED
> hardware doesn't need to sleep to toggle gpios.

So far so good.

> Some drivers do have a problem with that however and its usually been
> suggested they offload the brightness changes into a workqueue. The gpio

Also good.  But the fact is, the LED core *does* know when it is calling
from a scheduleable context (e.g. from sysfs handlers), and that's not an
uncommon path either.

The trigger code is more complicated, I don't know if most of its calls to
brightness_set are in safe or unsafe contexts for sleep.  But the people
calling the trigger code certainly would.

> * fix the gpio driver not to be so clever and clearly document
> * move the workqueue into the LED class, use it for everyone and remove
> the limitation of the function (punishes the hardware which doesn't need
> to sleep)
> * move the workqueue into the LED class and have LED drivers state
> whether they can sleep or not
> * start passing around GFP_* flags
> 
> Passing flags around and maintaining a track of schedulable state for
> the LED class sounds like overkill. I also don't like the idea of

It is the preferred way to do these things.  If you don't do it like that,
both gpio and *all* ACPI-based LED devices will have to always defer to
workqueues.

> So I'm leaning towards 'fixing' the gpio driver as I think David has
> already offered. I will also improve the documentation on this function
> and its requirements as I agree the current isn't as clear as it should
> be.

And we will have to always defer to workqueues on drivers that can't operate
from an atomic/interrupt context?  Even when there would be no need for it
because brightness_set is not being called from an non-scheduleable context
at all?

I hope I can live with that for LEDs (I have to think about LED
brightness_get first before I am sure about that), but I don't like it at
all for the long term.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh