Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754259AbbGANhl (ORCPT ); Wed, 1 Jul 2015 09:37:41 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:53973 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754147AbbGANhf (ORCPT ); Wed, 1 Jul 2015 09:37:35 -0400 Date: Wed, 1 Jul 2015 15:37:32 +0200 From: Pavel Machek To: Jacek Anaszewski Cc: linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, Bryan Wu , Richard Purdie , Stas Sergeev , Sakari Ailus , Andreas Werner , Andrew Lunn , Antonio Ospite , Atsushi Nemoto , Ben Dooks , Chris Boot , Dan Murphy , Daniel Jeong , Daniel Mack , "David S. Miller" , Fabio Baltieri , Felipe Balbi , Florian Fainelli , "G.Shark Jeong" , Guennadi Liakhovetski , Ingi Kim , Jan-Simon Moeller , Johan Hovold , John Lenz , Jonas Gorski , Kim Kyuwon , Kristian Kielhofner , Kristoffer Ericson , Linus Walleij , Mark Brown , Michael Hennerich , Milo Kim , =?iso-8859-1?Q?M=E1rton_N=E9meth?= , Nate Case , NeilBrown , Nick Forbes , Paul Parsons , Peter Meerwald , Phil Sutter , Philippe Retornaz , Raphael Assenat , Richard Purdie , Rod Whitby , Dave Hansen , Rodolfo Giometti , "Sebastian A. Siewior" , Shuah Khan , Simon Guinot , =?iso-8859-1?Q?=C1lvaro_Fern=E1ndez?= Rojas Subject: Re: [PATCH/RFC RESEND] leds: Use set_brightness_work for brightness_set ops that can sleep Message-ID: <20150701133732.GB16250@amd> References: <1435651268-9657-1-git-send-email-j.anaszewski@samsung.com> <20150630115809.GA13605@amd> <5592944B.5020809@samsung.com> <20150630174619.GA1852@amd> <559396B4.9020504@samsung.com> <20150701074327.GB11385@amd> <5593C526.9070300@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5593C526.9070300@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3140 Lines: 66 On Wed 2015-07-01 12:47:02, Jacek Anaszewski wrote: > On 07/01/2015 09:43 AM, Pavel Machek wrote: > >On Wed 2015-07-01 09:28:52, Jacek Anaszewski wrote: > >>On 06/30/2015 07:46 PM, Pavel Machek wrote: > >>>On Tue 2015-06-30 15:06:19, Jacek Anaszewski wrote: > >>>>On 06/30/2015 01:58 PM, Pavel Machek wrote: > >>>>>On Tue 2015-06-30 10:01:08, Jacek Anaszewski wrote: > >>>>>>This patch rearranges the core LED subsystem code, so that it > >>>>>>now removes from drivers the responsibility of using work queues > >>>>>>internally in case their brightness_set ops can sleep. > >>>>>>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 allow 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. > >>>>> > >>>>>Are you sure this is good idea? > >>>>> > >>>>>You'll now use single callback for blocking and non-blocking > >>>>>behaviour. I'm pretty sure stuff like lockdep will have some fun with > >>>>>that. > >>>> > >>>>I enabled "Lock Debugging" options and didn't get any warning. > >>>>Could you describe the use case you are thinking of? > >>> > >>>You may get one when one of the sleeping functions uses some lock... > >> > >>Drivers which use spin_lock in their brightness_set op will have to > >>set LED_BRIGHTNESS_FAST flag, which will instruct the LED core to > >>call the op synchronously. On the other hand drivers which can sleep > >>in their brightness_set op won't set the flag, which will make LED core > >>delegating the op to the work queue task. It is also possible that > >>driver with brightness_set op that can sleep set SET_BRIGHTNESS_SYNC > >>flag - then LED core will call it in a synchronous way from > >>led_brightness_set and it will schedule work queue task in case > >>the op is called from triggers. > > > >I understand this "works". > > > >>If you want to NAK the patch, please come up with detailed analysis > >>on how it can cause problems. Without this I infer that you didn't > >>spend a second on analyzing the code. This is counterproductive. > > > >NAK. > > > >Because calling two functions with different semantics through same > >function pointer is extremely ugly, and _will_ cause lockdep > >problems. Talk to the lockdep people for details. > > Which two functions are you thinking of? There is a single > brightness_set op to call. Yes, and brightness_set may or may not sleep according to a flag somewhere. Just use two function pointers, one of them will be always NULL. You can keep the flag if you want to. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/