lkml.org 
[lkml]   [2019]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jul 30, 2019 at 11:39:34AM -0700, Tri Vo wrote:
> On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > >
> > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > blocking wakeup sources over device's boot cycle. This information can
> > > then be used (1) for power-specific bug reporting and (2) towards
> > > attributing battery consumption to specific processes over a period of
> > > time.
> > >
> > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > /sys/class/wakeup/wakeup<ID>/*.
> > >
> > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > Signed-off-by: Tri Vo <trong@android.com>
> > > Tested-by: Tri Vo <trong@android.com>
> > > Tested-by: Kalesh Singh <kaleshsingh@google.com>
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > ---
> > > Documentation/ABI/testing/sysfs-class-wakeup | 76 +++++++++
> > > drivers/acpi/device_pm.c | 3 +-
> > > drivers/base/power/Makefile | 2 +-
> > > drivers/base/power/wakeup.c | 21 ++-
> > > drivers/base/power/wakeup_stats.c | 171 +++++++++++++++++++
> > > fs/eventpoll.c | 4 +-
> > > include/linux/pm_wakeup.h | 15 +-
> > > kernel/power/autosleep.c | 2 +-
> > > kernel/power/wakelock.c | 10 ++
> > > kernel/time/alarmtimer.c | 2 +-
> > > 10 files changed, 294 insertions(+), 12 deletions(-)
> > > create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
> > > create mode 100644 drivers/base/power/wakeup_stats.c
> > >
> > > v2:
> > > - Updated Documentation/ABI/, as per Greg.
> > > - Removed locks in attribute functions, as per Greg.
> > > - Lifetimes of struct wakelock and struck wakeup_source are now different due to
> > > the latter embedding a refcounted kobject. Changed it so that struct wakelock
> > > only has a pointer to struct wakeup_source, instead of embedding it.
> > > - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
> > > sysfs.
> > >
> > > v3:
> > > Changes by Greg:
> > > - Reworked code to use 'struct device' instead of raw kobjects.
> > > - Updated documentation file.
> > > - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
> > > Changes by Tri:
> > > - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
> > > operations. So no need to handle lifetimes in wakelock.c
> > >
> > > v4:
> > > - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
> > > - Moved new documentation to a separate file
> > > Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
> > > - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.
> > >
> > > v5:
> > > - Removed CONFIG_PM_SLEEP_STATS
> > > - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by
> > > kbuild test robot <lkp@intel.com>
> > > - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to
> > > that device's power/wakeup file results in multiple wakeup source being
> > > allocated for that device. Changed device_wakeup_enable() to check if device
> > > wakeup was previously enabled.
> > > Changes by Stephen:
> > > - Changed stats location from /sys/class/wakeup/<name>/* to
> > > /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This
> > > avoids name collisions in /sys/class/wakeup/ directory.
> > > - Added a "name" attribute to wakeup sources, and updated documentation.
> > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > Updated wakeup_source_register()'s signature and its callers accordingly.
> >
> > And I really don't like these changes. Especially having "wakeup"
> > twice in the path.
>
> I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?

Yes.

> About the other change, I think making the registering device the
> parent of the wakeup source is a worthwhile change, since that way one
> can associate a wakeup source sysfs entry with the device that created
> it.

that's fine with me.

> > Couldn't you find a simpler way to avoid the name collisions?
>
> I could also simply log an error in case of a name collision instead
> of failing hard. That way I can keep the old path with the wakeup
> source name in it. Other than that, I can't think of a way to resolve
> the directory name collisions without making that directory name
> unique, i.e. generating IDs is probably the simplest way. I'm still
> learning about the kernel, and I might be wrong though. What do you
> think?

Uniqe ids for the class should be fine, that's all you need to have.

thanks,

greg k-h

\
 
 \ /
  Last update: 2019-07-30 20:49    [W:1.159 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site