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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
