Message-ID: <20150701213623.GT5904@valkosipuli.retiisi.org.uk>
Date:	Thu, 2 Jul 2015 00:36:23 +0300
From:	Sakari Ailus <sakari.ailus@....fi>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
	cooloney@...il.com, rpurdie@...ys.net, stsp@...rs.sourceforge.net,
	pavel@....cz, sakari.ailus@...ux.intel.com, andreas.werner@....de,
	andrew@...n.ch, ospite@...denti.unina.it, anemo@....ocn.ne.jp,
	ben@...tec.co.uk, bootc@...tc.net, dmurphy@...com,
	daniel.jeong@...com, daniel@...que.org, davem@...emloft.net,
	fabio.baltieri@...il.com, balbi@...com, florian@...nwrt.org,
	gshark.jeong@...il.com, g.liakhovetski@....de,
	ingi2.kim@...sung.com, dl9pf@....de, johan@...nel.org,
	lenz@...wisc.edu, jogo@...nwrt.org, q1.kim@...sung.com,
	kris@...sk.org, kristoffer.ericson@...il.com,
	linus.walleij@...aro.org, broonie@...nel.org,
	michael.hennerich@...log.com, milo.kim@...com, nm127@...email.hu,
	ncase@...-inc.com, neilb@...e.de, nick.forbes@...epta.com,
	lost.distance@...oo.com, p.meerwald@...-electronic.com,
	n0-1@...ewrt.org, philippe.retornaz@...l.ch, raph@...com,
	rpurdie@...nedhand.com, rod@...tby.id.au, dave@...1.net,
	giometti@...ux.it, bigeasy@...utronix.de, shuahkhan@...il.com,
	sguinot@...ie.com
Subject: Re: [PATCH/RFC v2 1/5] leds: Use set_brightness_work for
 brightness_set ops that can sleep

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. They could be fixed later on; I
wonder how testing could be arranged.

> >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.

> >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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi	XMPP: sailus@...iisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists