Message-ID: <CANA+-vBKg_W88Oy_wJs1NNYaZ2ciJKO=Mrs47etYTDNXUKW9Uw@mail.gmail.com>
Date:   Tue, 30 Jul 2019 11:39:34 -0700
From:   Tri Vo <trong@...roid.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Hridya Valsaraju <hridya@...gle.com>,
        Sandeep Patil <sspatil@...gle.com>,
        Kalesh Singh <kaleshsingh@...gle.com>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        kbuild test robot <lkp@...el.com>
Subject: Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs

On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@...roid.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@...uxfoundation.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Co-developed-by: Stephen Boyd <swboyd@...omium.org>
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> > Signed-off-by: Tri Vo <trong@...roid.com>
> > Tested-by: Tri Vo <trong@...roid.com>
> > Tested-by: Kalesh Singh <kaleshsingh@...gle.com>
> > Reported-by: kbuild test robot <lkp@...el.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@...el.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?

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.
>
> 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?

Powered by blists - more mailing lists