Re: [PATCH/RFC v2 1/5] leds: Use set_brightness_work for brightness_set ops that can sleep

[Jacek Anaszewski]

Hi Sakari,

On 07/01/2015 11:36 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Wed, Jul 01, 2015 at 11:52:11AM +0200, Jacek Anaszewski wrote:
> > Hi Sakari,
> >
> > On 07/01/2015 12:24 AM, Sakari Ailus wrote:
> > > Hi Jacek,
> > >
> > > On Tue, Jun 30, 2015 at 03:59:26PM +0200, Jacek Anaszewski wrote:
> > > > This patch rearranges the core LED subsystem code, so that it
> > > > now shifts the responsibility for using work queues from drivers,
> > > > in case their brightness_set ops can sleep, onto the LED core
> > > > Addition of two flags: LED_BRIGHTNESS_FAST and LED_BLINK_DISABLE
> > > > as well as new_brightness_value property to the struct led_classdev
> > > > allows for employing existing set_brightness_work to do the job.
> > > > The modifications allows also to get rid of brightness_set_sync op,
> > > > as flash LED devices can now be handled properly only basing on the
> > > > SET_BRIGHTNESS_SYNC flag.
> > >
> > > Nice patch! Thanks!
> > >
> > > Looks like this is the favourite topic nowadays. ;-)
> >
> > Yeah, this allows to believe that we will manage to tackle the issue
> > finally :)
> >
> > > The documentation should be improved to tell how the API is expected to be
> > > have, e.g. which functions may block. I think this is out of scope for this
> > > patch though.
> >
> > Yes, I planned to cover this after the patch is accepted.
>
> Ok.
>
> > > I think all the existing drivers that implement the set_brightness()
> > > callback have a fast (and non-blocking) implementation, and some of these
> > > drivers use a work queue. In order to avoid modifying those drivers right
> > > now, how about adding a flag for slow devices instead? "Slow" handlers
> > > should be those that do at least one of the following: 1) sleep and 2) take
> > > excessive amount of time to run.
> >
> > As Andrew Lunn mentioned, he was also working on this issue and he did
> > the vast majority of work [1] needed to remove work queues from existing
> > drivers. Only new flags would have to be added.
>
> Excellent! This is what I was thinking as well.
>
> It think it'd be good if we could get the framework improvements in without
> having to change the existing drivers.

It would be hard to achieve without loosing performance. With the third
version of the patch set drivers will still work, but LED core will
schedule work queue tasks while setting brightness, despite that
some drivers don't need that all implement them on their own.

> They could be fixed later on; I wonder how testing could be arranged.

Well, majority of people who are familiar with LED drivers is on CC
for this thread. I'd add patches removing work queues from drivers
and adding LED_BRIGHTNESS_FAST flag where applicable and ask people to
test the patches.

> > > How about splitting the patch as follows:
> > >
> > > - set_brightness()/set_brightness_sync() -> set_brightness() +
> > >    LED_BRIGHTNESS_FAST + slow handlers in a work queue,
> > > - add LED_BLINK_DISABLE flag,
> > > - fix the heartbeat trigger (it's sleeping in a timer if LED_BRIGHTNESS_SYNC
> > >    is set).
> >
> > With my solution heartbeat trigger is not sleeping even if
> > LED_BRIGHTNESS_SYNC is set as all triggers use the new function -
> > led_set_brightness_nosleep.
>
> Correct. I'd suggest still to put that into a separate patch as it's a
> bugfix.

Actually this is rather 'semantic' bug, which doesn't break anything
in comparison to the state from time before the led_set_brightness_async
was introduced. brightness_set ops are implemented for all drivers, so
that they are guaranteed no to sleep. Buggy is the function name which
misleadingly implies that it calls brightness_set op asynchronously,
which is not true for drivers that don't use work queues.

But of course, I will gather replacing led_set_brightness_async
in the triggers with led_set_brightness in the separate patch.

> > > I'd propose to drop led_set_brightness_async() and just make
> > > led_set_brightness() asynchronous (or non-blocking if you wish) as it was
> > > before the LED flash class patches. Considering the nature and tradition of
> > > the framework, that's probably how most users want it to be. One can always
> > > use led_set_brightness_sync() if needed.
> > >
> > > The caller should indeed decide whether the operation is synchronous or not,
> > > that's not really a property of the LED. I requested that for the V4L2
> > > framework due to the very different use cases that are typical for the LED
> > > class devices.
> > >
> > > I have some patches along these lines, but I probably won't have much time
> > > to work on them, and I can rebase mine on yours later on. If you're
> > > interested in taking a peek they're here:
> > >
> > > <URL:http://salottisipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=shortlog;h=refs/heads/leds-as3645a>
> >
> > I've skimmed through the patches and I find them valuable.
> > Could you elaborate on the patch
> > "leds: Serialise access to LED framework data and drivers",
> > what problem does it solve?
>
> The LED class framework is quite simple, yet it doesn't do much for drivers.
> For that reason, there are quite a few drivers repeating the same patterns
> such as implementing serialisation in drivers. That patch does it in the
> framework itself, while also making it possible to remove locks in the
> drivers.

Thanks for the explanation. Wouldn't the gain be too small in comparison
to the increased code complexity?

--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/