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

Groups > linux.kernel > #1518430 > unrolled thread

PM regression with LED changes in next-20161109

Back to article view | Back to linux.kernel

Contents

  PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-09 20:30 +0100
    Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-09 21:50 +0100
      Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:50 +0100
        Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 14:00 +0100
          Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 14:10 +0100
            Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-11-10 15:00 +0100
              Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:40 +0100
          Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 17:30 +0100
            Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 17:50 +0100
              Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:50 +0100
                Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 09:30 +0100
            Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 19:10 +0100
              Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-10 21:30 +0100
                Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-10 22:40 +0100
                  Re: PM regression with LED changes in next-20161109 Pavel Machek <pavel@ucw.cz> - 2016-11-11 13:10 +0100
                    Re: PM regression with LED changes in next-20161109 Jacek Anaszewski <jacek.anaszewski@gmail.com> - 2016-11-11 18:10 +0100
                      Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-11 20:30 +0100
    Re: PM regression with LED changes in next-20161109 Hans de Goede <hdegoede@redhat.com> - 2016-11-10 09:40 +0100
      Re: PM regression with LED changes in next-20161109 Tony Lindgren <tony@atomide.com> - 2016-11-10 16:20 +0100

#1518430 — PM regression with LED changes in next-20161109

Hi,

Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
the sysfs brightness attr for changes.") breaks runtime PM for me.

On my omap dm3730 based test system, idle power consumption is over 70
times higher now with this patch! It goes from about 6mW for the core
system to over 440mW during idle meaning there's some busy timer now
active.

Reverting this patch fixes the issue. Any ideas?

Regards,

Tony

[toc] | [next] | [standalone]

#1518475

Hi Tony,

On 11/09/2016 08:23 PM, Tony Lindgren wrote:
> Hi,
>
> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> the sysfs brightness attr for changes.") breaks runtime PM for me.
>
> On my omap dm3730 based test system, idle power consumption is over 70
> times higher now with this patch! It goes from about 6mW for the core
> system to over 440mW during idle meaning there's some busy timer now
> active.
>
> Reverting this patch fixes the issue. Any ideas?

Thanks for the report. This is probably caused by sysfs_notify_dirent().
I'm afraid that we can't keep this feature in the current shape.
Hans, I'm dropping the patch. We probably will have to delegate this
call to a workqueue task. Think about use cases when the LED is blinked
with high frequency e.g. from ledtrig-disk.c.

Also, IMHO the notifications should be enabled only if explicitly
selected in the kernel config.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1518747

Hi,

On 09-11-16 21:45, Jacek Anaszewski wrote:
> Hi Tony,
>
> On 11/09/2016 08:23 PM, Tony Lindgren wrote:
>> Hi,
>>
>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>
>> On my omap dm3730 based test system, idle power consumption is over 70
>> times higher now with this patch! It goes from about 6mW for the core
>> system to over 440mW during idle meaning there's some busy timer now
>> active.
>>
>> Reverting this patch fixes the issue. Any ideas?
>
> Thanks for the report. This is probably caused by sysfs_notify_dirent().
> I'm afraid that we can't keep this feature in the current shape.
> Hans, I'm dropping the patch. We probably will have to delegate this
> call to a workqueue task. Think about use cases when the LED is blinked
> with high frequency e.g. from ledtrig-disk.c.

sysfs_notify_dirent() already uses a workqueue, here is the actual
implementation of it (from fs/kernfs/file.c) :

void kernfs_notify(struct kernfs_node *kn)
{
         static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
         unsigned long flags;

         if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
                 return;

         spin_lock_irqsave(&kernfs_notify_lock, flags);
         if (!kn->attr.notify_next) {
                 kernfs_get(kn);
                 kn->attr.notify_next = kernfs_notify_list;
                 kernfs_notify_list = kn;
                 schedule_work(&kernfs_notify_work);
         }
         spin_unlock_irqrestore(&kernfs_notify_lock, flags);
}

So using a workqueue is not going to help. Note that I already
feared this, which is why my initial implementation only called
sysfs_notify_dirent() for user initiated changes and not for
triggers / blinking.

I think we may need to reconsider what getting the brightness
sysfs atrribute actually returns. Currently when a LED is
blinking it will return 0 resp. the actual brightness depending
on when in the blink cycle the user reads the brightness
sysfs atrribute.

So a user can do "echo 128 > brightness && cat brightness" and
get out 0, or 128, depending purely on timing.

This seems to contradict what Documentation/ABI/testing/sysfs-class-led
has to say:

What:           /sys/class/leds/<led>/brightness
Date:           March 2006
KernelVersion:  2.6.17
Contact:        Richard Purdie <rpurdie@rpsys.net>
Description:
                 Set the brightness of the LED. Most LEDs don't
                 have hardware brightness support, so will just be turned on for
                 non-zero brightness settings. The value is between 0 and
                 /sys/class/leds/<led>/max_brightness.

                 Writing 0 to this file clears active trigger.

                 Writing non-zero to this file while trigger is active changes the
                 top brightness trigger is going to use.

Even though it only talks about writing, the logical thing would be for
reading to be the exact opposite of writing, so we would get:

                 Reading from this file while a trigger is active returns the
                 top brightness trigger is going to use.

The current docs say not about (sw) blinking, but that should be treated just
like a trigger IMHO.

If we can get consensus on what the read behavior for the brightness attribute
should be, then I think that a better poll() behavior will automatically follow
from that.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1518962

Hi,

On 11/10/2016 09:49 AM, Hans de Goede wrote:
> Hi,
>
> On 09-11-16 21:45, Jacek Anaszewski wrote:
>> Hi Tony,
>>
>> On 11/09/2016 08:23 PM, Tony Lindgren wrote:
>>> Hi,
>>>
>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>
>>> On my omap dm3730 based test system, idle power consumption is over 70
>>> times higher now with this patch! It goes from about 6mW for the core
>>> system to over 440mW during idle meaning there's some busy timer now
>>> active.
>>>
>>> Reverting this patch fixes the issue. Any ideas?
>>
>> Thanks for the report. This is probably caused by sysfs_notify_dirent().
>> I'm afraid that we can't keep this feature in the current shape.
>> Hans, I'm dropping the patch. We probably will have to delegate this
>> call to a workqueue task. Think about use cases when the LED is blinked
>> with high frequency e.g. from ledtrig-disk.c.
>
> sysfs_notify_dirent() already uses a workqueue, here is the actual
> implementation of it (from fs/kernfs/file.c) :
>
> void kernfs_notify(struct kernfs_node *kn)
> {
>         static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
>         unsigned long flags;
>
>         if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
>                 return;
>
>         spin_lock_irqsave(&kernfs_notify_lock, flags);
>         if (!kn->attr.notify_next) {
>                 kernfs_get(kn);
>                 kn->attr.notify_next = kernfs_notify_list;
>                 kernfs_notify_list = kn;
>                 schedule_work(&kernfs_notify_work);
>         }
>         spin_unlock_irqrestore(&kernfs_notify_lock, flags);
> }

Indeed. As a next step of this investigation Tony could disable
particular calls made in kernfs_notify_workfn to check what
exactly causes excessive power consumption.

> So using a workqueue is not going to help. Note that I already
> feared this, which is why my initial implementation only called
> sysfs_notify_dirent() for user initiated changes and not for
> triggers / blinking.

AFAIR there were no calls to led_notify_brightness_change() in
the initial implementation and it was entirely predestined for
being called by LED class drivers on brightness changes made
by firmware.

> I think we may need to reconsider what getting the brightness
> sysfs atrribute actually returns. Currently when a LED is
> blinking it will return 0 resp. the actual brightness depending
> on when in the blink cycle the user reads the brightness
> sysfs atrribute.
>
> So a user can do "echo 128 > brightness && cat brightness" and
> get out 0, or 128, depending purely on timing.
>
> This seems to contradict what Documentation/ABI/testing/sysfs-class-led
> has to say:
>
> What:           /sys/class/leds/<led>/brightness
> Date:           March 2006
> KernelVersion:  2.6.17
> Contact:        Richard Purdie <rpurdie@rpsys.net>
> Description:
>                 Set the brightness of the LED. Most LEDs don't
>                 have hardware brightness support, so will just be turned
> on for
>                 non-zero brightness settings. The value is between 0 and
>                 /sys/class/leds/<led>/max_brightness.
>
>                 Writing 0 to this file clears active trigger.
>
>                 Writing non-zero to this file while trigger is active
> changes the
>                 top brightness trigger is going to use.
>
> Even though it only talks about writing, the logical thing would be for
> reading to be the exact opposite of writing, so we would get:
>
>                 Reading from this file while a trigger is active returns
> the
>                 top brightness trigger is going to use.
>
> The current docs say not about (sw) blinking, but that should be treated
> just
> like a trigger IMHO.

You'r right, we should describe the semantics on reading, but it would
have to be as follows:

Reading from this file returns LED brightness at given moment, i.e.
even though LED class device brightness setting is greater than 0, the
momentary brightness can be 0 if the readout occurred during low phase
of blink cycle.

> If we can get consensus on what the read behavior for the brightness
> attribute
> should be, then I think that a better poll() behavior will automatically
> follow
> from that.

It seems that we should get back to your initial approach. i.e. only
brightness changes caused by hardware should be reported.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1518966

Hi,

On 10-11-16 13:56, Jacek Anaszewski wrote:
> Hi,
>
> On 11/10/2016 09:49 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 09-11-16 21:45, Jacek Anaszewski wrote:
>>> Hi Tony,
>>>
>>> On 11/09/2016 08:23 PM, Tony Lindgren wrote:
>>>> Hi,
>>>>
>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>
>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>> times higher now with this patch! It goes from about 6mW for the core
>>>> system to over 440mW during idle meaning there's some busy timer now
>>>> active.
>>>>
>>>> Reverting this patch fixes the issue. Any ideas?
>>>
>>> Thanks for the report. This is probably caused by sysfs_notify_dirent().
>>> I'm afraid that we can't keep this feature in the current shape.
>>> Hans, I'm dropping the patch. We probably will have to delegate this
>>> call to a workqueue task. Think about use cases when the LED is blinked
>>> with high frequency e.g. from ledtrig-disk.c.
>>
>> sysfs_notify_dirent() already uses a workqueue, here is the actual
>> implementation of it (from fs/kernfs/file.c) :
>>
>> void kernfs_notify(struct kernfs_node *kn)
>> {
>>         static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
>>         unsigned long flags;
>>
>>         if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
>>                 return;
>>
>>         spin_lock_irqsave(&kernfs_notify_lock, flags);
>>         if (!kn->attr.notify_next) {
>>                 kernfs_get(kn);
>>                 kn->attr.notify_next = kernfs_notify_list;
>>                 kernfs_notify_list = kn;
>>                 schedule_work(&kernfs_notify_work);
>>         }
>>         spin_unlock_irqrestore(&kernfs_notify_lock, flags);
>> }
>
> Indeed. As a next step of this investigation Tony could disable
> particular calls made in kernfs_notify_workfn to check what
> exactly causes excessive power consumption.
>
>> So using a workqueue is not going to help. Note that I already
>> feared this, which is why my initial implementation only called
>> sysfs_notify_dirent() for user initiated changes and not for
>> triggers / blinking.
>
> AFAIR there were no calls to led_notify_brightness_change() in
> the initial implementation and it was entirely predestined for
> being called by LED class drivers on brightness changes made
> by firmware.
>
>> I think we may need to reconsider what getting the brightness
>> sysfs atrribute actually returns. Currently when a LED is
>> blinking it will return 0 resp. the actual brightness depending
>> on when in the blink cycle the user reads the brightness
>> sysfs atrribute.
>>
>> So a user can do "echo 128 > brightness && cat brightness" and
>> get out 0, or 128, depending purely on timing.
>>
>> This seems to contradict what Documentation/ABI/testing/sysfs-class-led
>> has to say:
>>
>> What:           /sys/class/leds/<led>/brightness
>> Date:           March 2006
>> KernelVersion:  2.6.17
>> Contact:        Richard Purdie <rpurdie@rpsys.net>
>> Description:
>>                 Set the brightness of the LED. Most LEDs don't
>>                 have hardware brightness support, so will just be turned
>> on for
>>                 non-zero brightness settings. The value is between 0 and
>>                 /sys/class/leds/<led>/max_brightness.
>>
>>                 Writing 0 to this file clears active trigger.
>>
>>                 Writing non-zero to this file while trigger is active
>> changes the
>>                 top brightness trigger is going to use.
>>
>> Even though it only talks about writing, the logical thing would be for
>> reading to be the exact opposite of writing, so we would get:
>>
>>                 Reading from this file while a trigger is active returns
>> the
>>                 top brightness trigger is going to use.
>>
>> The current docs say not about (sw) blinking, but that should be treated
>> just
>> like a trigger IMHO.
>
> You'r right, we should describe the semantics on reading, but it would
> have to be as follows:
>
> Reading from this file returns LED brightness at given moment, i.e.
> even though LED class device brightness setting is greater than 0, the
> momentary brightness can be 0 if the readout occurred during low phase
> of blink cycle.

Why would it need to read like this, because this is the current behavior ?

I doubt anyone is relying on this current behavior because it is really
unpredictable which value one can get.

I believe it would be better to change the read semantics to follow
the write semantics, this would be much more consistent.

Making the read behavior match the write behavior should be easy I would
be happy to write a patch for this.

>> If we can get consensus on what the read behavior for the brightness
>> attribute
>> should be, then I think that a better poll() behavior will automatically
>> follow
>> from that.
>
> It seems that we should get back to your initial approach. i.e. only
> brightness changes caused by hardware should be reported.

Ok, if you really want to keep the read behavior as is, I can provide
an updated patch for this.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1518990

Hi,

On 11/10/2016 02:04 PM, Hans de Goede wrote:
> Hi,
>
> On 10-11-16 13:56, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 11/10/2016 09:49 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 09-11-16 21:45, Jacek Anaszewski wrote:
>>>> Hi Tony,
>>>>
>>>> On 11/09/2016 08:23 PM, Tony Lindgren wrote:
>>>>> Hi,
>>>>>
>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>>
>>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>>> times higher now with this patch! It goes from about 6mW for the core
>>>>> system to over 440mW during idle meaning there's some busy timer now
>>>>> active.
>>>>>
>>>>> Reverting this patch fixes the issue. Any ideas?
>>>>
>>>> Thanks for the report. This is probably caused by
>>>> sysfs_notify_dirent().
>>>> I'm afraid that we can't keep this feature in the current shape.
>>>> Hans, I'm dropping the patch. We probably will have to delegate this
>>>> call to a workqueue task. Think about use cases when the LED is blinked
>>>> with high frequency e.g. from ledtrig-disk.c.
>>>
>>> sysfs_notify_dirent() already uses a workqueue, here is the actual
>>> implementation of it (from fs/kernfs/file.c) :
>>>
>>> void kernfs_notify(struct kernfs_node *kn)
>>> {
>>>         static DECLARE_WORK(kernfs_notify_work, kernfs_notify_workfn);
>>>         unsigned long flags;
>>>
>>>         if (WARN_ON(kernfs_type(kn) != KERNFS_FILE))
>>>                 return;
>>>
>>>         spin_lock_irqsave(&kernfs_notify_lock, flags);
>>>         if (!kn->attr.notify_next) {
>>>                 kernfs_get(kn);
>>>                 kn->attr.notify_next = kernfs_notify_list;
>>>                 kernfs_notify_list = kn;
>>>                 schedule_work(&kernfs_notify_work);
>>>         }
>>>         spin_unlock_irqrestore(&kernfs_notify_lock, flags);
>>> }
>>
>> Indeed. As a next step of this investigation Tony could disable
>> particular calls made in kernfs_notify_workfn to check what
>> exactly causes excessive power consumption.
>>
>>> So using a workqueue is not going to help. Note that I already
>>> feared this, which is why my initial implementation only called
>>> sysfs_notify_dirent() for user initiated changes and not for
>>> triggers / blinking.
>>
>> AFAIR there were no calls to led_notify_brightness_change() in
>> the initial implementation and it was entirely predestined for
>> being called by LED class drivers on brightness changes made
>> by firmware.
>>
>>> I think we may need to reconsider what getting the brightness
>>> sysfs atrribute actually returns. Currently when a LED is
>>> blinking it will return 0 resp. the actual brightness depending
>>> on when in the blink cycle the user reads the brightness
>>> sysfs atrribute.
>>>
>>> So a user can do "echo 128 > brightness && cat brightness" and
>>> get out 0, or 128, depending purely on timing.
>>>
>>> This seems to contradict what Documentation/ABI/testing/sysfs-class-led
>>> has to say:
>>>
>>> What:           /sys/class/leds/<led>/brightness
>>> Date:           March 2006
>>> KernelVersion:  2.6.17
>>> Contact:        Richard Purdie <rpurdie@rpsys.net>
>>> Description:
>>>                 Set the brightness of the LED. Most LEDs don't
>>>                 have hardware brightness support, so will just be turned
>>> on for
>>>                 non-zero brightness settings. The value is between 0 and
>>>                 /sys/class/leds/<led>/max_brightness.
>>>
>>>                 Writing 0 to this file clears active trigger.
>>>
>>>                 Writing non-zero to this file while trigger is active
>>> changes the
>>>                 top brightness trigger is going to use.
>>>
>>> Even though it only talks about writing, the logical thing would be for
>>> reading to be the exact opposite of writing, so we would get:
>>>
>>>                 Reading from this file while a trigger is active returns
>>> the
>>>                 top brightness trigger is going to use.
>>>
>>> The current docs say not about (sw) blinking, but that should be treated
>>> just
>>> like a trigger IMHO.
>>
>> You'r right, we should describe the semantics on reading, but it would
>> have to be as follows:
>>
>> Reading from this file returns LED brightness at given moment, i.e.
>> even though LED class device brightness setting is greater than 0, the
>> momentary brightness can be 0 if the readout occurred during low phase
>> of blink cycle.
>
> Why would it need to read like this, because this is the current behavior ?

We have led_update_brightness() which was introduced long time ago and
is used in brightness_show(). Note that if LED controller changed
actual LED brightness e.g. due to battery voltage dropping below
certain threshold, we wouldn't be able to find it out otherwise
(except if we added separate sysfs file for that).

>
> I doubt anyone is relying on this current behavior because it is really
> unpredictable which value one can get.
>
> I believe it would be better to change the read semantics to follow
> the write semantics, this would be much more consistent.
>
> Making the read behavior match the write behavior should be easy I would
> be happy to write a patch for this.

Let's better agree on the description of the current semantics.
It has been around for a long time.

>>> If we can get consensus on what the read behavior for the brightness
>>> attribute
>>> should be, then I think that a better poll() behavior will automatically
>>> follow
>>> from that.
>>
>> It seems that we should get back to your initial approach. i.e. only
>> brightness changes caused by hardware should be reported.
>
> Ok, if you really want to keep the read behavior as is, I can provide
> an updated patch for this.

Yes please.

-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1519123

[Multipart message — attachments visible in raw view] — view raw

Hi!


> >>>The current docs say not about (sw) blinking, but that should be treated
> >>>just
> >>>like a trigger IMHO.
> >>
> >>You'r right, we should describe the semantics on reading, but it would
> >>have to be as follows:
> >>
> >>Reading from this file returns LED brightness at given moment, i.e.
> >>even though LED class device brightness setting is greater than 0, the
> >>momentary brightness can be 0 if the readout occurred during low phase
> >>of blink cycle.
> >
> >Why would it need to read like this, because this is the current behavior ?
> 
> We have led_update_brightness() which was introduced long time ago and
> is used in brightness_show(). Note that if LED controller changed
> actual LED brightness e.g. due to battery voltage dropping below
> certain threshold, we wouldn't be able to find it out otherwise
> (except if we added separate sysfs file for that).

And we should have a separate sysfs file for that. Note that on some
hardware leds, you are able to let hardware control them, but if you
do, you can't really tell the current state. Examples are n900 and
thinkpad-acpi. So it is better we don't pretend we can get that value
for userspace.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1519114

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> >>>
> >>>On my omap dm3730 based test system, idle power consumption is over 70
> >>>times higher now with this patch! It goes from about 6mW for the core
> >>>system to over 440mW during idle meaning there's some busy timer now
> >>>active.
> >>>
> >>>Reverting this patch fixes the issue. Any ideas?

Are you using any LED that toggles with high frequency? Like perhaps
LED that is lit when CPU is active?

> >So a user can do "echo 128 > brightness && cat brightness" and
> >get out 0, or 128, depending purely on timing.
...
> >                Reading from this file while a trigger is active returns
> >the
> >                top brightness trigger is going to use.

Yes, that sounds sane.

> It seems that we should get back to your initial approach. i.e. only
> brightness changes caused by hardware should be reported.

I don't think enabling poll() here is good idea. Some hardware won't
be able to tell you that it changed the state. Returning maximum
brightness trigger is going to use seems easier/better.

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1519128

Hi,

On 10-11-16 17:29, Pavel Machek wrote:
> Hi!
>
>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>>
>>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>>> times higher now with this patch! It goes from about 6mW for the core
>>>>> system to over 440mW during idle meaning there's some busy timer now
>>>>> active.
>>>>>
>>>>> Reverting this patch fixes the issue. Any ideas?
>
> Are you using any LED that toggles with high frequency? Like perhaps
> LED that is lit when CPU is active?
>
>>> So a user can do "echo 128 > brightness && cat brightness" and
>>> get out 0, or 128, depending purely on timing.
> ...
>>>                Reading from this file while a trigger is active returns
>>> the
>>>                top brightness trigger is going to use.
>
> Yes, that sounds sane.
>
>> It seems that we should get back to your initial approach. i.e. only
>> brightness changes caused by hardware should be reported.
>
> I don't think enabling poll() here is good idea. Some hardware won't
> be able to tell you that it changed the state. Returning maximum
> brightness trigger is going to use seems easier/better.

The idea here is to allow userspace to poll() on the brightness
sysfs atrribute to detect changes autonomously done by the hardware,
such as e.g. happens on both Dell and Thinkpad laptops when pressing
the keyboard backlight cycle hotkey. Note that these keys do not
generate key-press events, the cycling through the brightness levels
(including off) is done entirely in firmware.

But we do get other ACPI events for this which we can use to let
userspace know this happens, which is something which user-
interfaces which allow control over the kbd backlight want to know.

I understand that we will not always be able to do this, here is the
Documentation/ABI/testing/sysfs-class-led text I have in mind:

		The file supports poll() to detect changes, changes are only
		signalled when this file is written or when the hardware /
		firmware changes the brightness itself and the driver can detect
		this. Changes done by kernel triggers / software blinking are
		not signalled.

Note the "and the driver can detect this" language, that has been there
since v1 of the poll() notification patch since I already expected not
all hardware to be able to signal this.

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1519352

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>It seems that we should get back to your initial approach. i.e. only
> >>brightness changes caused by hardware should be reported.
> >
> >I don't think enabling poll() here is good idea. Some hardware won't
> >be able to tell you that it changed the state. Returning maximum
> >brightness trigger is going to use seems easier/better.
> 
> The idea here is to allow userspace to poll() on the brightness
> sysfs atrribute to detect changes autonomously done by the hardware,
> such as e.g. happens on both Dell and Thinkpad laptops when pressing
> the keyboard backlight cycle hotkey. Note that these keys do not
> generate key-press events, the cycling through the brightness levels
> (including off) is done entirely in firmware.

Ok, so you can do that for keyboard backlight on thinkpad... I guess
you handle that as a special trigger on the keyboard leds? Can other
triggers, such as heartbeat, be assigned to that "led"? 

> But we do get other ACPI events for this which we can use to let
> userspace know this happens, which is something which user-
> interfaces which allow control over the kbd backlight want to know.

Yes, you can do that for keyboard backlight... but on thinkpads there
are more leds, such as battery led. That can blink on battery low, and
I don't think you can read the current status from hardware.

Getting current state of led blinking with cpu trigger is also not
quite a good idea. 
So IMO this should not be done in generic code. Instead,
kbd-backlight trigger should have special attribute, and that one
should be pollable.

> I understand that we will not always be able to do this, here is the
> Documentation/ABI/testing/sysfs-class-led text I have in mind:
> 
> 		The file supports poll() to detect changes, changes are only
> 		signalled when this file is written or when the hardware /
> 		firmware changes the brightness itself and the driver can detect
> 		this. Changes done by kernel triggers / software blinking are
> 		not signalled.
> 
> Note the "and the driver can detect this" language, that has been there
> since v1 of the poll() notification patch since I already expected not
> all hardware to be able to signal this.

Lets move it to separate attribute, for triggers that can do that,
please.

We do want a way to read maximum brightness for the heartbeat trigger,
for example..

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1519602

Hi,

On 10-11-16 21:48, Pavel Machek wrote:
> Hi!
>
>>>> It seems that we should get back to your initial approach. i.e. only
>>>> brightness changes caused by hardware should be reported.
>>>
>>> I don't think enabling poll() here is good idea. Some hardware won't
>>> be able to tell you that it changed the state. Returning maximum
>>> brightness trigger is going to use seems easier/better.
>>
>> The idea here is to allow userspace to poll() on the brightness
>> sysfs atrribute to detect changes autonomously done by the hardware,
>> such as e.g. happens on both Dell and Thinkpad laptops when pressing
>> the keyboard backlight cycle hotkey. Note that these keys do not
>> generate key-press events, the cycling through the brightness levels
>> (including off) is done entirely in firmware.
>
> Ok, so you can do that for keyboard backlight on thinkpad... I guess
> you handle that as a special trigger on the keyboard leds?

No, as said this is all done in firmware, as in this is all dealt
with by (presumably) the acpi-ec (acpi-embedded-controller) the kernel
does not do anything here, the key is "hardwired" to control the
keyboard backlight from the kernels pov.

 > Can other
> triggers, such as heartbeat, be assigned to that "led"?
>
>> But we do get other ACPI events for this which we can use to let
>> userspace know this happens, which is something which user-
>> interfaces which allow control over the kbd backlight want to know.
>
> Yes, you can do that for keyboard backlight... but on thinkpads there
> are more leds, such as battery led. That can blink on battery low, and
> I don't think you can read the current status from hardware.

Well the battery LED does not show up under /sys/class/led so that
is not relevant for this situation, anyways ...

> Getting current state of led blinking with cpu trigger is also not
> quite a good idea.

I agree with you that it would be better if reading the brightness
sysfs attribute would always return the max brightness for LEDs
which are blinking or have a trigger set. But it seems that Jacek
disagrees, I will leave further discussion of this up to you and
Jacek.

> So IMO this should not be done in generic code. Instead,
> kbd-backlight trigger should have special attribute, and that one
> should be pollable.

Again there is no kbd-backlight trigger.

>> I understand that we will not always be able to do this, here is the
>> Documentation/ABI/testing/sysfs-class-led text I have in mind:
>>
>> 		The file supports poll() to detect changes, changes are only
>> 		signalled when this file is written or when the hardware /
>> 		firmware changes the brightness itself and the driver can detect
>> 		this. Changes done by kernel triggers / software blinking are
>> 		not signalled.
>>
>> Note the "and the driver can detect this" language, that has been there
>> since v1 of the poll() notification patch since I already expected not
>> all hardware to be able to signal this.
>
> Lets move it to separate attribute, for triggers that can do that,
> please.

As explained above this has nothing to do with triggers...

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1519256

* Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> Hi!
> 
> > >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> > >>>
> > >>>On my omap dm3730 based test system, idle power consumption is over 70
> > >>>times higher now with this patch! It goes from about 6mW for the core
> > >>>system to over 440mW during idle meaning there's some busy timer now
> > >>>active.
> > >>>
> > >>>Reverting this patch fixes the issue. Any ideas?
> 
> Are you using any LED that toggles with high frequency? Like perhaps
> LED that is lit when CPU is active?

Yeah one of them seems to have cpu0 as the default trigger.

Tony

[toc] | [prev] | [next] | [standalone]

#1519343

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> > Hi!
> > 
> > > >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > > >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> > > >>>
> > > >>>On my omap dm3730 based test system, idle power consumption is over 70
> > > >>>times higher now with this patch! It goes from about 6mW for the core
> > > >>>system to over 440mW during idle meaning there's some busy timer now
> > > >>>active.
> > > >>>
> > > >>>Reverting this patch fixes the issue. Any ideas?
> > 
> > Are you using any LED that toggles with high frequency? Like perhaps
> > LED that is lit when CPU is active?
> 
> Yeah one of them seems to have cpu0 as the default trigger.

Aha. Its quite obvious we don't want to notify sysfs each time that
one is toggled, right?

IMO brightness should display max brightness for the trigger, as Hans
suggested, anything else is madness for trigger such as cpu activity.

Thanks and best regards,

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1519376

Hi,

On 11/10/2016 09:29 PM, Pavel Machek wrote:
> On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
>> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
>>> Hi!
>>>
>>>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>>>>
>>>>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>>>>> times higher now with this patch! It goes from about 6mW for the core
>>>>>>> system to over 440mW during idle meaning there's some busy timer now
>>>>>>> active.
>>>>>>>
>>>>>>> Reverting this patch fixes the issue. Any ideas?
>>>
>>> Are you using any LED that toggles with high frequency? Like perhaps
>>> LED that is lit when CPU is active?
>>
>> Yeah one of them seems to have cpu0 as the default trigger.
>
> Aha. Its quite obvious we don't want to notify sysfs each time that
> one is toggled, right?
>
> IMO brightness should display max brightness for the trigger, as Hans
> suggested, anything else is madness for trigger such as cpu activity.

Are you suggesting that we should revert changes introduced
by below patch?

commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date:   Tue Mar 18 09:47:48 2008 +0000

     leds: Add support to leds with readable status

     Some led hardware allows drivers to query the led state, and this patch
     adds a hook to let the led class take advantage of that information 
when
     available.

     Without this functionality, when access to the led hardware is not
     exclusive (i.e. firmware or hardware might change its state behind the
     kernel's back), reality goes out of sync with the led class' idea 
of what
     the led is doing, which is annoying at best.

     Behaviour for drivers that do not or cannot read the led status is
     unchanged.

     Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
     Signed-off-by: Richard Purdie <rpurdie@rpsys.net>


-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1519741

[Multipart message — attachments visible in raw view] — view raw

On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
> Hi,
> 
> On 11/10/2016 09:29 PM, Pavel Machek wrote:
> >On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
> >>* Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> >>>Hi!
> >>>
> >>>>>>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> >>>>>>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> >>>>>>>
> >>>>>>>On my omap dm3730 based test system, idle power consumption is over 70
> >>>>>>>times higher now with this patch! It goes from about 6mW for the core
> >>>>>>>system to over 440mW during idle meaning there's some busy timer now
> >>>>>>>active.
> >>>>>>>
> >>>>>>>Reverting this patch fixes the issue. Any ideas?
> >>>
> >>>Are you using any LED that toggles with high frequency? Like perhaps
> >>>LED that is lit when CPU is active?
> >>
> >>Yeah one of them seems to have cpu0 as the default trigger.
> >
> >Aha. Its quite obvious we don't want to notify sysfs each time that
> >one is toggled, right?
> >
> >IMO brightness should display max brightness for the trigger, as Hans
> >suggested, anything else is madness for trigger such as cpu activity.
> 
> Are you suggesting that we should revert changes introduced
> by below patch?
> 
> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Date:   Tue Mar 18 09:47:48 2008 +0000
> 
>     leds: Add support to leds with readable status
> 
>     Some led hardware allows drivers to query the led state, and this patch
>     adds a hook to let the led class take advantage of that information when
>     available.
> 
>     Without this functionality, when access to the led hardware is not
>     exclusive (i.e. firmware or hardware might change its state behind the
>     kernel's back), reality goes out of sync with the led class' idea of
> what
>     the led is doing, which is annoying at best.

Hmm. So userland can read the LED state, and it can get _some_ value
back, but it can not know if it is current state or not.

I don't think that's a good interface. I see it is from 2008... is
someone using it? Maybe it is too late for revert.

But I'd certainly not extend it with poll.

IMO reading/polling should only be available with some triggers. It
does not make sense with "CPU load" trigger. It makes sense with
"keyboard light changeable by hardware" trigger.

Best regards,
									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1519914

On 11/11/2016 01:01 PM, Pavel Machek wrote:
> On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 11/10/2016 09:29 PM, Pavel Machek wrote:
>>> On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
>>>> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
>>>>> Hi!
>>>>>
>>>>>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>>>>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>>>>>>
>>>>>>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>>>>>>> times higher now with this patch! It goes from about 6mW for the core
>>>>>>>>> system to over 440mW during idle meaning there's some busy timer now
>>>>>>>>> active.
>>>>>>>>>
>>>>>>>>> Reverting this patch fixes the issue. Any ideas?
>>>>>
>>>>> Are you using any LED that toggles with high frequency? Like perhaps
>>>>> LED that is lit when CPU is active?
>>>>
>>>> Yeah one of them seems to have cpu0 as the default trigger.
>>>
>>> Aha. Its quite obvious we don't want to notify sysfs each time that
>>> one is toggled, right?
>>>
>>> IMO brightness should display max brightness for the trigger, as Hans
>>> suggested, anything else is madness for trigger such as cpu activity.
>>
>> Are you suggesting that we should revert changes introduced
>> by below patch?
>>
>> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
>> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>> Date:   Tue Mar 18 09:47:48 2008 +0000
>>
>>     leds: Add support to leds with readable status
>>
>>     Some led hardware allows drivers to query the led state, and this patch
>>     adds a hook to let the led class take advantage of that information when
>>     available.
>>
>>     Without this functionality, when access to the led hardware is not
>>     exclusive (i.e. firmware or hardware might change its state behind the
>>     kernel's back), reality goes out of sync with the led class' idea of
>> what
>>     the led is doing, which is annoying at best.
>
> Hmm. So userland can read the LED state, and it can get _some_ value
> back, but it can not know if it is current state or not.
>
> I don't think that's a good interface. I see it is from 2008... is
> someone using it? Maybe it is too late for revert.

I can imagine it being used in flash LED use case. E.g. one
could use oneshot trigger to trigger flash strobe, and then
he could periodically read brightness file to check, for whatever
reason, whether the flash is strobing.

> But I'd certainly not extend it with poll.

We could add a dedicated file e.g. hw_brightness_change for that
(maybe someone will have a better candidate for the file name).

This way it would be semantically consistent to report only
hardware originating brightness changes on it, which was the
initial reason for adding the brightness change notification
feature.

Moreover, LED class drivers could report on this file the
brightness level which was set by the firmware, which wouldn't
affect current LED class device brightness setting, unless
brightness file is read (and brightness_get op is supported).

> IMO reading/polling should only be available with some triggers. It
> does not make sense with "CPU load" trigger. It makes sense with
> "keyboard light changeable by hardware" trigger.


-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1519986

Hi,

On 11-11-16 18:03, Jacek Anaszewski wrote:
> On 11/11/2016 01:01 PM, Pavel Machek wrote:
>> On Thu 2016-11-10 22:34:07, Jacek Anaszewski wrote:
>>> Hi,
>>>
>>> On 11/10/2016 09:29 PM, Pavel Machek wrote:
>>>> On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
>>>>> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
>>>>>> Hi!
>>>>>>
>>>>>>>>>> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
>>>>>>>>>> the sysfs brightness attr for changes.") breaks runtime PM for me.
>>>>>>>>>>
>>>>>>>>>> On my omap dm3730 based test system, idle power consumption is over 70
>>>>>>>>>> times higher now with this patch! It goes from about 6mW for the core
>>>>>>>>>> system to over 440mW during idle meaning there's some busy timer now
>>>>>>>>>> active.
>>>>>>>>>>
>>>>>>>>>> Reverting this patch fixes the issue. Any ideas?
>>>>>>
>>>>>> Are you using any LED that toggles with high frequency? Like perhaps
>>>>>> LED that is lit when CPU is active?
>>>>>
>>>>> Yeah one of them seems to have cpu0 as the default trigger.
>>>>
>>>> Aha. Its quite obvious we don't want to notify sysfs each time that
>>>> one is toggled, right?
>>>>
>>>> IMO brightness should display max brightness for the trigger, as Hans
>>>> suggested, anything else is madness for trigger such as cpu activity.
>>>
>>> Are you suggesting that we should revert changes introduced
>>> by below patch?
>>>
>>> commit 29d76dfa29fe22583aefddccda0bc56aa81035dc
>>> Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
>>> Date:   Tue Mar 18 09:47:48 2008 +0000
>>>
>>>     leds: Add support to leds with readable status
>>>
>>>     Some led hardware allows drivers to query the led state, and this patch
>>>     adds a hook to let the led class take advantage of that information when
>>>     available.
>>>
>>>     Without this functionality, when access to the led hardware is not
>>>     exclusive (i.e. firmware or hardware might change its state behind the
>>>     kernel's back), reality goes out of sync with the led class' idea of
>>> what
>>>     the led is doing, which is annoying at best.
>>
>> Hmm. So userland can read the LED state, and it can get _some_ value
>> back, but it can not know if it is current state or not.
>>
>> I don't think that's a good interface. I see it is from 2008... is
>> someone using it? Maybe it is too late for revert.
>
> I can imagine it being used in flash LED use case. E.g. one
> could use oneshot trigger to trigger flash strobe, and then
> he could periodically read brightness file to check, for whatever
> reason, whether the flash is strobing.
>
>> But I'd certainly not extend it with poll.
>
> We could add a dedicated file e.g. hw_brightness_change for that
> (maybe someone will have a better candidate for the file name).

Why a dedicated file? Are we going to mirror brightness here
wrt r/w (show/store) behavior ? If not userspace now needs
2 open fds which is not really nice. If we are and we are
not going to use poll for something else on brightness itself
then why not just poll directly on brightness ?

Thinking more about this, I'm strongly against having to do
poll on /sys/.../bar to detect changes on /sys/.../foo that
is something which no other interface does. So my vote on this
is NACK for the having a separate file for this.

Regards,

Hans



>
> This way it would be semantically consistent to report only
> hardware originating brightness changes on it, which was the
> initial reason for adding the brightness change notification
> feature.
>
> Moreover, LED class drivers could report on this file the
> brightness level which was set by the firmware, which wouldn't
> affect current LED class device brightness setting, unless
> brightness file is read (and brightness_get op is supported).
>
>> IMO reading/polling should only be available with some triggers. It
>> does not make sense with "CPU load" trigger. It makes sense with
>> "keyboard light changeable by hardware" trigger.
>
>

[toc] | [prev] | [next] | [standalone]

#1518740

Hi,

On 09-11-16 20:23, Tony Lindgren wrote:
> Hi,
>
> Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> the sysfs brightness attr for changes.") breaks runtime PM for me.
>
> On my omap dm3730 based test system, idle power consumption is over 70
> times higher now with this patch! It goes from about 6mW for the core
> system to over 440mW during idle meaning there's some busy timer now
> active.

Do you have any blinking LEDs or LED triggers defined on the system ?

> Reverting this patch fixes the issue. Any ideas?

All I can think of is something calling led_set_brightness quite often,
the patch in question makes led_set_brightness somewhat more expensive,
but it should not cause such a big difference unless something is
really calling led_set_brightness quite often maybe something is calling
it with the same value all the time ?

Regards,

Hans

[toc] | [prev] | [next] | [standalone]

#1519049

* Hans de Goede <hdegoede@redhat.com> [161110 01:35]:
> Hi,
> 
> On 09-11-16 20:23, Tony Lindgren wrote:
> > Hi,
> > 
> > Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > the sysfs brightness attr for changes.") breaks runtime PM for me.
> > 
> > On my omap dm3730 based test system, idle power consumption is over 70
> > times higher now with this patch! It goes from about 6mW for the core
> > system to over 440mW during idle meaning there's some busy timer now
> > active.
> 
> Do you have any blinking LEDs or LED triggers defined on the system ?

There are some configured in the dts file:

$ grep -i led arch/arm/boot/dts/*torpedo*.dts*

And the gpio controlled led1 is configured to blink with
linux,default-trigger = "cpu0".

> > Reverting this patch fixes the issue. Any ideas?
> 
> All I can think of is something calling led_set_brightness quite often,
> the patch in question makes led_set_brightness somewhat more expensive,
> but it should not cause such a big difference unless something is
> really calling led_set_brightness quite often maybe something is calling
> it with the same value all the time ?

I don't think this one has any brightness control.

Regards,

Tony

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web