Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190730024309.233728-1-trong@android.com>
In-Reply-To: <20190730024309.233728-1-trong@android.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 30 Jul 2019 07:46:44 +0200
Message-ID: <CAJZ5v0jJn=vHdYExbzwRAMsk=Ad5bhvOAvHEXe-FHOj2R4Gwig@mail.gmail.com>
Subject: Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
To:     Tri Vo <trong@android.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Hridya Valsaraju <hridya@google.com>,
        Sandeep Patil <sspatil@google.com>,
        Kalesh Singh <kaleshsingh@google.com>,
        Ravi Chandra Sadineni <ravisadineni@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Cc: Android Kernel" <kernel-team@android.com>,
        kbuild test robot <lkp@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

Couldn't you find a simpler way to avoid the name collisions?