Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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