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

[Stas Sergeev]

30.06.2015 15:41, Jacek Anaszewski ÐÐÑÐÑ:
> On 06/30/2015 01:41 PM, Stas Sergeev wrote:
>> 30.06.2015 11:27, Jacek Anaszewski ÐÐÑÐÑ:
>>> On 06/29/2015 05:17 PM, Stas Sergeev wrote:> 29.06.2015 17:05, Jacek Anaszewski ÐÐÑÐÑ:
>>>>> +         * If need to disable soft blinking delegate this to the
>>>>> +         * work queue task to avoid problems in case we are
>>>>> +         * called from hard irq context.
>>>>> +         */
>>>>> +        led_cdev->flags |= LED_BLINK_DISABLE;
>>>> Wouldn't it be better to just enforce the callers
>>>> to explicitly disable software blink, so that it to
>>>> never happen from irq context? Something like in this
>>>> patch:
>>>> https://lkml.org/lkml/2015/5/13/491
>>>>
>>>
>>> Blinking can be disabled not only by removing trigger explicitly,
>>> but also by setting brightness to 0 and led_set_brightness
>>> can be called from hard irq context. set_brightness_work
>>> was originally introduced exactly for this use case.
>> Could you please describe where does this happen?
> This in fact doesn't take place in the mainline kernel,
> however there are some out of tree users apparently [1].
Oh, IMHO the hooks that are needed only for out-of-tree code
require the appropriate comment, at least. I was entirely
confused by its existence. IIRC in the past there was even
the policy to not include any hooks for out-of-tree code at
all.

> Modifications I am proposing also need set_brightness_work,
> so there is almost no cost of keeping the support for calling led_set_brightness from hard irq context intact.
I wonder if there are possible races, eg you schedule disabling
of the softblink timer, and someone else at the same time also
disables it (and probably also re-enables).
Well if you think that code is safe, I would agree that the cost
is now very small with your patch, so maybe it is not a big deal
any more.
--
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/