Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175034 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH/RFC RESEND] leds: Use set_brightness_work for  brightness_set ops that can sleep Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-07-01 09:30 +0200
    Re: [PATCH/RFC RESEND] leds: Use set_brightness_work for  brightness_set ops that can sleep Pavel Machek <pavel@denx.de> - 2015-07-01 09:50 +0200

#1175034 — Re: [PATCH/RFC RESEND] leds: Use set_brightness_work for brightness_set ops that can sleep

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.

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.

> I'm not a lockdep expert, but mixing functions with different
> semantics under one function pointer is wrong thing to do. If it does
> not produce warnings today, it may start producing them tommorow. Just
> don't do it.

-- 
Best Regards,
Jacek Anaszewski
--
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/

[toc] | [next] | [standalone]

#1175047

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.

								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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web