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

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

[Tri Vo]

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, expose wakeup
sources statistics in sysfs under /sys/power/wakeup_sources/<name>/

Add following attributes to each wakeup source. These attributes match
the columns of /sys/kernel/debug/wakeup_sources.

  active_count
  event_count
  wakeup_count
  expire_count
  active_time (named "active_since" in debugfs)
  total_time
  max_time
  last_change
  prevent_suspend_time

Embedding a struct kobject into struct wakeup_source changes lifetime
requirements on the latter. To that end, change deallocation of struct
wakeup_source using kfree to kobject_put().

Signed-off-by: Tri Vo <trong@android.com>
---
 drivers/base/power/Makefile       |   2 +-
 drivers/base/power/wakeup.c       |  13 +-
 drivers/base/power/wakeup_sysfs.c | 204 ++++++++++++++++++++++++++++++
 include/linux/pm_wakeup.h         |   9 ++
 kernel/power/wakelock.c           |  13 +-
 5 files changed, 236 insertions(+), 5 deletions(-)
 create mode 100644 drivers/base/power/wakeup_sysfs.c

diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..e6ea9eb46275 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
-obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o wakeup_sysfs.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..6fcbb4d2045f 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -96,12 +96,22 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
 struct wakeup_source *wakeup_source_create(const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;
 
 	ws = kmalloc(sizeof(*ws), GFP_KERNEL);
 	if (!ws)
 		return NULL;
 
 	wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
+
+	ws->kobj.kset = wakeup_source_kset;
+	ret = kobject_init_and_add(&ws->kobj, &wakeup_source_ktype, NULL, "%s",
+			ws->name);
+	if (ret) {
+		kobject_put(&ws->kobj);
+		return NULL;
+	}
+
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_create);
@@ -147,8 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
 
 	__pm_relax(ws);
 	wakeup_source_record(ws);
-	kfree_const(ws->name);
-	kfree(ws);
+	kobject_put(&ws->kobj);
 }
 EXPORT_SYMBOL_GPL(wakeup_source_destroy);
 
diff --git a/drivers/base/power/wakeup_sysfs.c b/drivers/base/power/wakeup_sysfs.c
new file mode 100644
index 000000000000..d872afc645d9
--- /dev/null
+++ b/drivers/base/power/wakeup_sysfs.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/slab.h>
+
+#include "power.h"
+
+struct wakeup_source_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct wakeup_source *ws,
+			struct wakeup_source_attribute *attr, char *buf);
+	ssize_t (*store)(struct wakeup_source *ws,
+			 struct wakeup_source_attribute *attr, const char *buf,
+			 size_t count);
+};
+
+#define to_wakeup_source_obj(x) container_of(x, struct wakeup_source, kobj)
+#define to_wakeup_source_attr(x) \
+	container_of(x, struct wakeup_source_attribute, attr)
+
+static ssize_t wakeup_source_attr_show(struct kobject *kobj,
+				       struct attribute *attr,
+				       char *buf)
+{
+	struct wakeup_source_attribute *attribute;
+	struct wakeup_source *ws;
+
+	ws = to_wakeup_source_obj(kobj);
+	attribute = to_wakeup_source_attr(attr);
+
+	if (!attribute->show)
+		return -EIO;
+
+	return attribute->show(ws, attribute, buf);
+}
+
+static const struct sysfs_ops wakeup_source_sysfs_ops = {
+	.show = wakeup_source_attr_show,
+};
+
+static void wakeup_source_release(struct kobject *kobj)
+{
+	struct wakeup_source *ws;
+
+	ws = to_wakeup_source_obj(kobj);
+	kfree_const(ws->name);
+	kfree(ws);
+}
+
+static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
+					struct wakeup_source_attribute *attr,
+					char *buf)
+{
+	unsigned long flags;
+	unsigned long var;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	if (strcmp(attr->attr.name, "active_count") == 0)
+		var = ws->active_count;
+	else if (strcmp(attr->attr.name, "event_count") == 0)
+		var = ws->event_count;
+	else if (strcmp(attr->attr.name, "wakeup_count") == 0)
+		var = ws->wakeup_count;
+	else
+		var = ws->expire_count;
+	spin_unlock_irqrestore(&ws->lock, flags);
+
+	return sprintf(buf, "%lu\n", var);
+}
+
+#define wakeup_source_count_attr(_name)			\
+static struct wakeup_source_attribute _name##_attr = {	\
+	.attr	= {					\
+		.name = __stringify(_name),		\
+		.mode = 0444,				\
+	},						\
+	.show	= wakeup_source_count_show,		\
+}
+
+wakeup_source_count_attr(active_count);
+wakeup_source_count_attr(event_count);
+wakeup_source_count_attr(wakeup_count);
+wakeup_source_count_attr(expire_count);
+
+#define wakeup_source_attr(_name) \
+static struct wakeup_source_attribute _name##_attr = __ATTR_RO(_name)
+
+static ssize_t active_time_show(struct wakeup_source *ws,
+				struct wakeup_source_attribute *attr,
+				char *buf)
+{
+	unsigned long flags;
+	ktime_t active_time;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	active_time = ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
+	spin_unlock_irqrestore(&ws->lock, flags);
+	return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
+}
+wakeup_source_attr(active_time);
+
+static ssize_t total_time_show(struct wakeup_source *ws,
+			       struct wakeup_source_attribute *attr,
+			       char *buf)
+{
+	unsigned long flags;
+	ktime_t active_time;
+	ktime_t total_time;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	total_time = ws->total_time;
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		total_time = ktime_add(total_time, active_time);
+	}
+	spin_unlock_irqrestore(&ws->lock, flags);
+	return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
+}
+wakeup_source_attr(total_time);
+
+static ssize_t max_time_show(struct wakeup_source *ws,
+			     struct wakeup_source_attribute *attr,
+			     char *buf)
+{
+	unsigned long flags;
+	ktime_t active_time;
+	ktime_t max_time;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	max_time = ws->max_time;
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		if (active_time > max_time)
+			max_time = active_time;
+	}
+	spin_unlock_irqrestore(&ws->lock, flags);
+	return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
+}
+wakeup_source_attr(max_time);
+
+static ssize_t last_change_show(struct wakeup_source *ws,
+				struct wakeup_source_attribute *attr,
+				char *buf)
+{
+	unsigned long flags;
+	ktime_t last_time;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	last_time = ws->last_time;
+	spin_unlock_irqrestore(&ws->lock, flags);
+	return sprintf(buf, "%lld\n", ktime_to_ms(last_time));
+}
+wakeup_source_attr(last_change);
+
+static ssize_t prevent_suspend_time_show(struct wakeup_source *ws,
+					 struct wakeup_source_attribute *attr,
+					 char *buf)
+{
+	unsigned long flags;
+	ktime_t prevent_sleep_time;
+
+	spin_lock_irqsave(&ws->lock, flags);
+	prevent_sleep_time = ws->prevent_sleep_time;
+	if (ws->active && ws->autosleep_enabled) {
+		prevent_sleep_time = ktime_add(prevent_sleep_time,
+			ktime_sub(ktime_get(), ws->start_prevent_time));
+	}
+	spin_unlock_irqrestore(&ws->lock, flags);
+	return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
+}
+wakeup_source_attr(prevent_suspend_time);
+
+static struct attribute *wakeup_source_default_attrs[] = {
+	&active_count_attr.attr,
+	&event_count_attr.attr,
+	&wakeup_count_attr.attr,
+	&expire_count_attr.attr,
+	&active_time_attr.attr,
+	&total_time_attr.attr,
+	&max_time_attr.attr,
+	&last_change_attr.attr,
+	&prevent_suspend_time_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_source_default);
+
+struct kobj_type wakeup_source_ktype = {
+	.sysfs_ops = &wakeup_source_sysfs_ops,
+	.release = wakeup_source_release,
+	.default_groups = wakeup_source_default_groups,
+};
+
+struct kset *wakeup_source_kset;
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_source_kset = kset_create_and_add("wakeup_sources", NULL,
+			power_kobj);
+	if (!wakeup_source_kset)
+		return -ENOMEM;
+
+	return 0;
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index ce57771fab9b..9af3bdd50557 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -57,6 +57,7 @@ struct wakeup_source {
 	unsigned long		wakeup_count;
 	bool			active:1;
 	bool			autosleep_enabled:1;
+	struct kobject		kobj;
 };
 
 #ifdef CONFIG_PM_SLEEP
@@ -100,6 +101,10 @@ extern void pm_relax(struct device *dev);
 extern void pm_wakeup_ws_event(struct wakeup_source *ws, unsigned int msec, bool hard);
 extern void pm_wakeup_dev_event(struct device *dev, unsigned int msec, bool hard);
 
+/* drivers/base/power/wakeup_sysfs.c */
+extern struct kobj_type wakeup_source_ktype;
+extern struct kset *wakeup_source_kset;
+
 #else /* !CONFIG_PM_SLEEP */
 
 static inline void device_set_wakeup_capable(struct device *dev, bool capable)
@@ -179,6 +184,10 @@ static inline void pm_wakeup_ws_event(struct wakeup_source *ws,
 static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
 				       bool hard) {}
 
+/* drivers/base/power/wakeup_sysfs.c */
+static struct kobj_type wakeup_source_ktype;
+static struct kset *wakeup_source_kset;
+
 #endif /* !CONFIG_PM_SLEEP */
 
 static inline void wakeup_source_init(struct wakeup_source *ws,
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..71535eebfbbf 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -124,8 +124,7 @@ static void __wakelocks_gc(struct work_struct *work)
 			wakeup_source_remove(&wl->ws);
 			rb_erase(&wl->node, &wakelocks_tree);
 			list_del(&wl->lru);
-			kfree(wl->name);
-			kfree(wl);
+			kobject_put(&wl->ws->kobj);
 			decrement_wakelocks_number();
 		}
 	}
@@ -153,6 +152,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	struct rb_node **node = &wakelocks_tree.rb_node;
 	struct rb_node *parent = *node;
 	struct wakelock *wl;
+	int ret;
 
 	while (*node) {
 		int diff;
@@ -189,6 +189,15 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	}
 	wl->ws.name = wl->name;
 	wl->ws.last_time = ktime_get();
+
+	wl->ws.kobj.kset = wakeup_source_kset;
+	ret = kobject_init_and_add(&wl->ws.kobj, &wakeup_source_ktype, NULL,
+			"%s", wl->ws.name);
+	if (ret) {
+		kobject_put(&wl->ws.kobj);
+		return ERR_PTR(ret);
+	}
+
 	wakeup_source_add(&wl->ws);
 	rb_link_node(&wl->node, parent, node);
 	rb_insert_color(&wl->node, &wakelocks_tree);
-- 
2.22.0.410.gd8fdbe21b5-goog

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

[Tri Vo]

On Tue, Jun 25, 2019 at 5:55 PM 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, expose wakeup
> sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
>
> Add following attributes to each wakeup source. These attributes match
> the columns of /sys/kernel/debug/wakeup_sources.
>
>   active_count
>   event_count
>   wakeup_count
>   expire_count
>   active_time (named "active_since" in debugfs)
>   total_time
>   max_time
>   last_change
>   prevent_suspend_time
>
> Embedding a struct kobject into struct wakeup_source changes lifetime
> requirements on the latter. To that end, change deallocation of struct
> wakeup_source using kfree to kobject_put().
>
> Signed-off-by: Tri Vo <trong@android.com>
> ---
>  drivers/base/power/Makefile       |   2 +-
>  drivers/base/power/wakeup.c       |  13 +-
>  drivers/base/power/wakeup_sysfs.c | 204 ++++++++++++++++++++++++++++++
>  include/linux/pm_wakeup.h         |   9 ++
>  kernel/power/wakelock.c           |  13 +-
>  5 files changed, 236 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/base/power/wakeup_sysfs.c
>
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index e1bb691cf8f1..e6ea9eb46275 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,6 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_PM)       += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
> -obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> +obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_sysfs.o
>  obj-$(CONFIG_PM_TRACE_RTC)     += trace.o
>  obj-$(CONFIG_PM_GENERIC_DOMAINS)       +=  domain.o domain_governor.o
>  obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 5b2b6a05a4f3..6fcbb4d2045f 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -96,12 +96,22 @@ EXPORT_SYMBOL_GPL(wakeup_source_prepare);
>  struct wakeup_source *wakeup_source_create(const char *name)
>  {
>         struct wakeup_source *ws;
> +       int ret;
>
>         ws = kmalloc(sizeof(*ws), GFP_KERNEL);
>         if (!ws)
>                 return NULL;
>
>         wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
> +
> +       ws->kobj.kset = wakeup_source_kset;
> +       ret = kobject_init_and_add(&ws->kobj, &wakeup_source_ktype, NULL, "%s",
> +                       ws->name);
> +       if (ret) {
> +               kobject_put(&ws->kobj);
> +               return NULL;
> +       }
> +
>         return ws;
>  }
>  EXPORT_SYMBOL_GPL(wakeup_source_create);
> @@ -147,8 +157,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
>
>         __pm_relax(ws);
>         wakeup_source_record(ws);
> -       kfree_const(ws->name);
> -       kfree(ws);
> +       kobject_put(&ws->kobj);
>  }
>  EXPORT_SYMBOL_GPL(wakeup_source_destroy);
>
> diff --git a/drivers/base/power/wakeup_sysfs.c b/drivers/base/power/wakeup_sysfs.c
> new file mode 100644
> index 000000000000..d872afc645d9
> --- /dev/null
> +++ b/drivers/base/power/wakeup_sysfs.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/slab.h>
> +
> +#include "power.h"
> +
> +struct wakeup_source_attribute {
> +       struct attribute attr;
> +       ssize_t (*show)(struct wakeup_source *ws,
> +                       struct wakeup_source_attribute *attr, char *buf);
> +       ssize_t (*store)(struct wakeup_source *ws,
> +                        struct wakeup_source_attribute *attr, const char *buf,
> +                        size_t count);
> +};
> +
> +#define to_wakeup_source_obj(x) container_of(x, struct wakeup_source, kobj)
> +#define to_wakeup_source_attr(x) \
> +       container_of(x, struct wakeup_source_attribute, attr)
> +
> +static ssize_t wakeup_source_attr_show(struct kobject *kobj,
> +                                      struct attribute *attr,
> +                                      char *buf)
> +{
> +       struct wakeup_source_attribute *attribute;
> +       struct wakeup_source *ws;
> +
> +       ws = to_wakeup_source_obj(kobj);
> +       attribute = to_wakeup_source_attr(attr);
> +
> +       if (!attribute->show)
> +               return -EIO;
> +
> +       return attribute->show(ws, attribute, buf);
> +}
> +
> +static const struct sysfs_ops wakeup_source_sysfs_ops = {
> +       .show = wakeup_source_attr_show,
> +};
> +
> +static void wakeup_source_release(struct kobject *kobj)
> +{
> +       struct wakeup_source *ws;
> +
> +       ws = to_wakeup_source_obj(kobj);
> +       kfree_const(ws->name);
> +       kfree(ws);
> +}
> +
> +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> +                                       struct wakeup_source_attribute *attr,
> +                                       char *buf)
> +{
> +       unsigned long flags;
> +       unsigned long var;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       if (strcmp(attr->attr.name, "active_count") == 0)
> +               var = ws->active_count;
> +       else if (strcmp(attr->attr.name, "event_count") == 0)
> +               var = ws->event_count;
> +       else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> +               var = ws->wakeup_count;
> +       else
> +               var = ws->expire_count;
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +
> +       return sprintf(buf, "%lu\n", var);
> +}
> +
> +#define wakeup_source_count_attr(_name)                        \
> +static struct wakeup_source_attribute _name##_attr = { \
> +       .attr   = {                                     \
> +               .name = __stringify(_name),             \
> +               .mode = 0444,                           \
> +       },                                              \
> +       .show   = wakeup_source_count_show,             \
> +}
> +
> +wakeup_source_count_attr(active_count);
> +wakeup_source_count_attr(event_count);
> +wakeup_source_count_attr(wakeup_count);
> +wakeup_source_count_attr(expire_count);
> +
> +#define wakeup_source_attr(_name) \
> +static struct wakeup_source_attribute _name##_attr = __ATTR_RO(_name)
> +
> +static ssize_t active_time_show(struct wakeup_source *ws,
> +                               struct wakeup_source_attribute *attr,
> +                               char *buf)
> +{
> +       unsigned long flags;
> +       ktime_t active_time;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       active_time = ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +       return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
> +}
> +wakeup_source_attr(active_time);
> +
> +static ssize_t total_time_show(struct wakeup_source *ws,
> +                              struct wakeup_source_attribute *attr,
> +                              char *buf)
> +{
> +       unsigned long flags;
> +       ktime_t active_time;
> +       ktime_t total_time;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       total_time = ws->total_time;
> +       if (ws->active) {
> +               active_time = ktime_sub(ktime_get(), ws->last_time);
> +               total_time = ktime_add(total_time, active_time);
> +       }
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +       return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
> +}
> +wakeup_source_attr(total_time);
> +
> +static ssize_t max_time_show(struct wakeup_source *ws,
> +                            struct wakeup_source_attribute *attr,
> +                            char *buf)
> +{
> +       unsigned long flags;
> +       ktime_t active_time;
> +       ktime_t max_time;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       max_time = ws->max_time;
> +       if (ws->active) {
> +               active_time = ktime_sub(ktime_get(), ws->last_time);
> +               if (active_time > max_time)
> +                       max_time = active_time;
> +       }
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +       return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
> +}
> +wakeup_source_attr(max_time);
> +
> +static ssize_t last_change_show(struct wakeup_source *ws,
> +                               struct wakeup_source_attribute *attr,
> +                               char *buf)
> +{
> +       unsigned long flags;
> +       ktime_t last_time;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       last_time = ws->last_time;
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +       return sprintf(buf, "%lld\n", ktime_to_ms(last_time));
> +}
> +wakeup_source_attr(last_change);
> +
> +static ssize_t prevent_suspend_time_show(struct wakeup_source *ws,
> +                                        struct wakeup_source_attribute *attr,
> +                                        char *buf)
> +{
> +       unsigned long flags;
> +       ktime_t prevent_sleep_time;
> +
> +       spin_lock_irqsave(&ws->lock, flags);
> +       prevent_sleep_time = ws->prevent_sleep_time;
> +       if (ws->active && ws->autosleep_enabled) {
> +               prevent_sleep_time = ktime_add(prevent_sleep_time,
> +                       ktime_sub(ktime_get(), ws->start_prevent_time));
> +       }
> +       spin_unlock_irqrestore(&ws->lock, flags);
> +       return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
> +}
> +wakeup_source_attr(prevent_suspend_time);
> +
> +static struct attribute *wakeup_source_default_attrs[] = {
> +       &active_count_attr.attr,
> +       &event_count_attr.attr,
> +       &wakeup_count_attr.attr,
> +       &expire_count_attr.attr,
> +       &active_time_attr.attr,
> +       &total_time_attr.attr,
> +       &max_time_attr.attr,
> +       &last_change_attr.attr,
> +       &prevent_suspend_time_attr.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(wakeup_source_default);
> +
> +struct kobj_type wakeup_source_ktype = {
> +       .sysfs_ops = &wakeup_source_sysfs_ops,
> +       .release = wakeup_source_release,
> +       .default_groups = wakeup_source_default_groups,
> +};
> +
> +struct kset *wakeup_source_kset;
> +
> +static int __init wakeup_sources_sysfs_init(void)
> +{
> +       wakeup_source_kset = kset_create_and_add("wakeup_sources", NULL,
> +                       power_kobj);
> +       if (!wakeup_source_kset)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +postcore_initcall(wakeup_sources_sysfs_init);
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index ce57771fab9b..9af3bdd50557 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -57,6 +57,7 @@ struct wakeup_source {
>         unsigned long           wakeup_count;
>         bool                    active:1;
>         bool                    autosleep_enabled:1;
> +       struct kobject          kobj;
>  };
>
>  #ifdef CONFIG_PM_SLEEP
> @@ -100,6 +101,10 @@ extern void pm_relax(struct device *dev);
>  extern void pm_wakeup_ws_event(struct wakeup_source *ws, unsigned int msec, bool hard);
>  extern void pm_wakeup_dev_event(struct device *dev, unsigned int msec, bool hard);
>
> +/* drivers/base/power/wakeup_sysfs.c */
> +extern struct kobj_type wakeup_source_ktype;
> +extern struct kset *wakeup_source_kset;
> +
>  #else /* !CONFIG_PM_SLEEP */
>
>  static inline void device_set_wakeup_capable(struct device *dev, bool capable)
> @@ -179,6 +184,10 @@ static inline void pm_wakeup_ws_event(struct wakeup_source *ws,
>  static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
>                                        bool hard) {}
>
> +/* drivers/base/power/wakeup_sysfs.c */
> +static struct kobj_type wakeup_source_ktype;
> +static struct kset *wakeup_source_kset;
> +
>  #endif /* !CONFIG_PM_SLEEP */
>
>  static inline void wakeup_source_init(struct wakeup_source *ws,
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 4210152e56f0..71535eebfbbf 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -124,8 +124,7 @@ static void __wakelocks_gc(struct work_struct *work)
>                         wakeup_source_remove(&wl->ws);
>                         rb_erase(&wl->node, &wakelocks_tree);
>                         list_del(&wl->lru);
> -                       kfree(wl->name);
> -                       kfree(wl);
> +                       kobject_put(&wl->ws->kobj);
>                         decrement_wakelocks_number();
>                 }
>         }
> @@ -153,6 +152,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>         struct rb_node **node = &wakelocks_tree.rb_node;
>         struct rb_node *parent = *node;
>         struct wakelock *wl;
> +       int ret;
>
>         while (*node) {
>                 int diff;
> @@ -189,6 +189,15 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>         }
>         wl->ws.name = wl->name;
>         wl->ws.last_time = ktime_get();
> +
> +       wl->ws.kobj.kset = wakeup_source_kset;
> +       ret = kobject_init_and_add(&wl->ws.kobj, &wakeup_source_ktype, NULL,
> +                       "%s", wl->ws.name);
> +       if (ret) {
> +               kobject_put(&wl->ws.kobj);
> +               return ERR_PTR(ret);
> +       }
> +
>         wakeup_source_add(&wl->ws);
>         rb_link_node(&wl->node, parent, node);
>         rb_insert_color(&wl->node, &wakelocks_tree);
> --
> 2.22.0.410.gd8fdbe21b5-goog
>

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

[Greg KH]

On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo 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, expose wakeup
> sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> 
> Add following attributes to each wakeup source. These attributes match
> the columns of /sys/kernel/debug/wakeup_sources.
> 
>   active_count
>   event_count
>   wakeup_count
>   expire_count
>   active_time (named "active_since" in debugfs)
>   total_time
>   max_time
>   last_change
>   prevent_suspend_time

Can you also add a Documentation/ABI/ update for your new sysfs files so
that we can properly review this?

> Embedding a struct kobject into struct wakeup_source changes lifetime
> requirements on the latter. To that end, change deallocation of struct
> wakeup_source using kfree to kobject_put().

Ick, are you sure you need a new kobject here?  Why wouldn't a named
attribute group work instead?  That should keep this patch much smaller
and simpler.

> +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> +					struct wakeup_source_attribute *attr,
> +					char *buf)
> +{
> +	unsigned long flags;
> +	unsigned long var;
> +
> +	spin_lock_irqsave(&ws->lock, flags);
> +	if (strcmp(attr->attr.name, "active_count") == 0)
> +		var = ws->active_count;
> +	else if (strcmp(attr->attr.name, "event_count") == 0)
> +		var = ws->event_count;
> +	else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> +		var = ws->wakeup_count;
> +	else
> +		var = ws->expire_count;
> +	spin_unlock_irqrestore(&ws->lock, flags);
> +
> +	return sprintf(buf, "%lu\n", var);
> +}

Why is this lock always needed to be grabbed?  You are just reading a
value, who cares if it changes inbetween reading it and returning the
buffer string as it can change at that point in time anyway?

thanks,

greg k-h

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

[Tri Vo]

On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo 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, expose wakeup
> > sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> >
> > Add following attributes to each wakeup source. These attributes match
> > the columns of /sys/kernel/debug/wakeup_sources.
> >
> >   active_count
> >   event_count
> >   wakeup_count
> >   expire_count
> >   active_time (named "active_since" in debugfs)
> >   total_time
> >   max_time
> >   last_change
> >   prevent_suspend_time
>
> Can you also add a Documentation/ABI/ update for your new sysfs files so
> that we can properly review this?
>
> > Embedding a struct kobject into struct wakeup_source changes lifetime
> > requirements on the latter. To that end, change deallocation of struct
> > wakeup_source using kfree to kobject_put().

Will do.
>
> Ick, are you sure you need a new kobject here?  Why wouldn't a named
> attribute group work instead?  That should keep this patch much smaller
> and simpler.

Yeah, named attribute groups might be a much cleaner way to do this.
Let me investigate.
>
> > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > +                                     struct wakeup_source_attribute *attr,
> > +                                     char *buf)
> > +{
> > +     unsigned long flags;
> > +     unsigned long var;
> > +
> > +     spin_lock_irqsave(&ws->lock, flags);
> > +     if (strcmp(attr->attr.name, "active_count") == 0)
> > +             var = ws->active_count;
> > +     else if (strcmp(attr->attr.name, "event_count") == 0)
> > +             var = ws->event_count;
> > +     else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > +             var = ws->wakeup_count;
> > +     else
> > +             var = ws->expire_count;
> > +     spin_unlock_irqrestore(&ws->lock, flags);
> > +
> > +     return sprintf(buf, "%lu\n", var);
> > +}
>
> Why is this lock always needed to be grabbed?  You are just reading a
> value, who cares if it changes inbetween reading it and returning the
> buffer string as it can change at that point in time anyway?

Right, we don't care if the value changes in between us reading and
printing it. However, IIUC not grabbing this lock results in a data
race, which is undefined behavior.

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

[Greg KH]

On Tue, Jun 25, 2019 at 06:33:08PM -0700, Tri Vo wrote:
> On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > > +                                     struct wakeup_source_attribute *attr,
> > > +                                     char *buf)
> > > +{
> > > +     unsigned long flags;
> > > +     unsigned long var;
> > > +
> > > +     spin_lock_irqsave(&ws->lock, flags);
> > > +     if (strcmp(attr->attr.name, "active_count") == 0)
> > > +             var = ws->active_count;
> > > +     else if (strcmp(attr->attr.name, "event_count") == 0)
> > > +             var = ws->event_count;
> > > +     else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > > +             var = ws->wakeup_count;
> > > +     else
> > > +             var = ws->expire_count;
> > > +     spin_unlock_irqrestore(&ws->lock, flags);
> > > +
> > > +     return sprintf(buf, "%lu\n", var);
> > > +}
> >
> > Why is this lock always needed to be grabbed?  You are just reading a
> > value, who cares if it changes inbetween reading it and returning the
> > buffer string as it can change at that point in time anyway?
> 
> Right, we don't care if the value changes in between us reading and
> printing it. However, IIUC not grabbing this lock results in a data
> race, which is undefined behavior.

A data race where?  Writing to the value?  How can that happen?  All you
are doing is incrementing this variable elsewhere, what is the worst
that can happen?

thanks,

greg k-h

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

[Tri Vo]

On Tue, Jun 25, 2019 at 6:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jun 25, 2019 at 06:33:08PM -0700, Tri Vo wrote:
> > On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > +static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
> > > > +                                     struct wakeup_source_attribute *attr,
> > > > +                                     char *buf)
> > > > +{
> > > > +     unsigned long flags;
> > > > +     unsigned long var;
> > > > +
> > > > +     spin_lock_irqsave(&ws->lock, flags);
> > > > +     if (strcmp(attr->attr.name, "active_count") == 0)
> > > > +             var = ws->active_count;
> > > > +     else if (strcmp(attr->attr.name, "event_count") == 0)
> > > > +             var = ws->event_count;
> > > > +     else if (strcmp(attr->attr.name, "wakeup_count") == 0)
> > > > +             var = ws->wakeup_count;
> > > > +     else
> > > > +             var = ws->expire_count;
> > > > +     spin_unlock_irqrestore(&ws->lock, flags);
> > > > +
> > > > +     return sprintf(buf, "%lu\n", var);
> > > > +}
> > >
> > > Why is this lock always needed to be grabbed?  You are just reading a
> > > value, who cares if it changes inbetween reading it and returning the
> > > buffer string as it can change at that point in time anyway?
> >
> > Right, we don't care if the value changes in between us reading and
> > printing it. However, IIUC not grabbing this lock results in a data
> > race, which is undefined behavior.
>
> A data race where?  Writing to the value?  How can that happen?  All you
> are doing is incrementing this variable elsewhere, what is the worst
> that can happen?

Ok, I'll remove the locks.

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

[Tri Vo]

On Tue, Jun 25, 2019 at 6:33 PM Tri Vo <trong@android.com> wrote:
>
> On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote:
> > > Embedding a struct kobject into struct wakeup_source changes lifetime
> > > requirements on the latter. To that end, change deallocation of struct
> > > wakeup_source using kfree to kobject_put().
> >
> > Ick, are you sure you need a new kobject here?  Why wouldn't a named
> > attribute group work instead?  That should keep this patch much smaller
> > and simpler.
>
> Yeah, named attribute groups might be a much cleaner way to do this.
> Let me investigate.

Say, we read /sys/power/wakeup_sources/foo/active_count. This
attribute's show function needs to find wakeup_source struct of "foo".
I'm not sure how to do that without embedding a kobject inside of
wakeup_source.

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

[Greg KH]

On Wed, Jun 26, 2019 at 03:48:58PM -0700, Tri Vo wrote:
> On Tue, Jun 25, 2019 at 6:33 PM Tri Vo <trong@android.com> wrote:
> >
> > On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote:
> > > > Embedding a struct kobject into struct wakeup_source changes lifetime
> > > > requirements on the latter. To that end, change deallocation of struct
> > > > wakeup_source using kfree to kobject_put().
> > >
> > > Ick, are you sure you need a new kobject here?  Why wouldn't a named
> > > attribute group work instead?  That should keep this patch much smaller
> > > and simpler.
> >
> > Yeah, named attribute groups might be a much cleaner way to do this.
> > Let me investigate.
> 
> Say, we read /sys/power/wakeup_sources/foo/active_count.

What is "foo" here?  You didn't include Documentation of the sysfs
files so it was pretty impossible to say what exactly your heirachy was
going to be in order to determine this :)

> This
> attribute's show function needs to find wakeup_source struct of "foo".
> I'm not sure how to do that without embedding a kobject inside of
> wakeup_source.

Again, without knowing what "foo" is, I can't really answer this.
Surely "foo" is not a flat namespace of all 'struct device' in the
kernel, right?

thanks,

greg k-h

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

[Tri Vo]

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, expose wakeup
sources statistics in sysfs under /sys/power/wakeup_sources/<name>/

Embedding a struct kobject into struct wakeup_source changes lifetime
requirements on the latter. To that end, change deallocation of struct
wakeup_source using kfree to kobject_put().

Change struct wakelock's wakeup_source member to a pointer to decouple
lifetimes of struct wakelock and struct wakeup_source for above reason.

Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
source statistics in sysfs.

Signed-off-by: Tri Vo <trong@android.com>
---
 Documentation/ABI/testing/sysfs-power |  80 ++++++++++-
 drivers/base/power/Makefile           |   2 +-
 drivers/base/power/power.h            |   7 +
 drivers/base/power/wakeup.c           |  16 ++-
 drivers/base/power/wakeup_stats.c     | 194 ++++++++++++++++++++++++++
 include/linux/pm_wakeup.h             |  15 ++
 kernel/power/Kconfig                  |  10 ++
 kernel/power/wakelock.c               |  42 ++++--
 8 files changed, 347 insertions(+), 19 deletions(-)
 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.

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 18b7dc929234..ba35f1eef73b 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -300,4 +300,82 @@ Description:
 		attempt.

 		Using this sysfs file will override any values that were
-		set using the kernel command line for disk offset.
\ No newline at end of file
+		set using the kernel command line for disk offset.
+
+What:		/sys/power/wakeup_sources/.../
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../ directory contains attributes
+		that expose statistics about a given wakeup source to user
+		space.  Attributes in this directory are read-only.
+
+What:		/sys/power/wakeup_sources/.../active_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../active_count file contains the
+		number of times the wakeup source was activated.
+
+What:		/sys/power/wakeup_sources/.../event_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../event_count file contains the
+		number of signaled wakeup events associated with the wakeup
+		source.
+
+What:		/sys/power/wakeup_sources/.../wakeup_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../wakeup_count file contains the
+		number of times the wakeup source might abort suspend.
+
+What:		/sys/power/wakeup_sources/.../expire_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../expire_count file contains the
+		number of times the wakeup source's timeout has expired.
+
+What:		/sys/power/wakeup_sources/.../active_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../active_time_ms file contains
+		the amount of time the wakeup source has been continuously
+		active, in milliseconds.  If the wakeup source is not active,
+		this file contains '0'.
+
+What:		/sys/power/wakeup_sources/.../total_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../total_time_ms file contains
+		the total amount of time this wakeup source has been active, in
+		milliseconds.
+
+What:		/sys/power/wakeup_sources/.../max_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../max_time_ms file contains the
+		maximum amount of time this wakeup source has been continuously
+		active, in milliseconds.
+
+What:		/sys/power/wakeup_sources/.../last_change_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../last_change_ms file contains
+		the monotonic clock time when the wakeup source was touched last
+		time, in milliseconds.
+
+What:		/sys/power/wakeup_sources/.../prevent_suspend_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/power/wakeup_sources/.../prevent_suspend_time_ms file
+		contains the total amount of time this wakeup source has been
+		preventing autosleep, in milliseconds.
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..ec5bb190b9d0 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
-obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o wakeup_stats.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index ec33fbdb919b..c288298f57c0 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -149,3 +149,10 @@ static inline void device_pm_init(struct device *dev)
 	device_pm_sleep_init(dev);
 	pm_runtime_init(dev);
 }
+
+#ifdef CONFIG_PM_SLEEP
+
+/* drivers/base/power/wakeup_stats.c */
+extern struct kobj_type wakeup_source_ktype;
+
+#endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..620c87923a3e 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -102,6 +102,9 @@ struct wakeup_source *wakeup_source_create(const char *name)
 		return NULL;

 	wakeup_source_prepare(ws, name ? kstrdup_const(name, GFP_KERNEL) : NULL);
+
+	kobject_init(&ws->kobj, &wakeup_source_ktype);
+
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_create);
@@ -147,8 +150,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)

 	__pm_relax(ws);
 	wakeup_source_record(ws);
-	kfree_const(ws->name);
-	kfree(ws);
+	kobject_put(&ws->kobj);
 }
 EXPORT_SYMBOL_GPL(wakeup_source_destroy);

@@ -205,11 +207,17 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 struct wakeup_source *wakeup_source_register(const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;

 	ws = wakeup_source_create(name);
-	if (ws)
+	if (ws) {
+		ret = wakeup_source_sysfs_add(ws);
+		if (ret) {
+			kobject_put(&ws->kobj);
+			return NULL;
+		}
 		wakeup_source_add(ws);
-
+	}
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_register);
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
new file mode 100644
index 000000000000..05ab8283ed66
--- /dev/null
+++ b/drivers/base/power/wakeup_stats.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/slab.h>
+
+#include "power.h"
+
+struct wakeup_source_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct wakeup_source *ws,
+			struct wakeup_source_attribute *attr, char *buf);
+	ssize_t (*store)(struct wakeup_source *ws,
+			 struct wakeup_source_attribute *attr, const char *buf,
+			 size_t count);
+};
+
+#define to_wakeup_source_obj(x) container_of(x, struct wakeup_source, kobj)
+#define to_wakeup_source_attr(x) \
+	container_of(x, struct wakeup_source_attribute, attr)
+
+static ssize_t wakeup_source_attr_show(struct kobject *kobj,
+				       struct attribute *attr,
+				       char *buf)
+{
+	struct wakeup_source_attribute *attribute;
+	struct wakeup_source *ws;
+
+	ws = to_wakeup_source_obj(kobj);
+	attribute = to_wakeup_source_attr(attr);
+
+	if (!attribute->show)
+		return -EIO;
+
+	return attribute->show(ws, attribute, buf);
+}
+
+static const struct sysfs_ops wakeup_source_sysfs_ops = {
+	.show = wakeup_source_attr_show,
+};
+
+static void wakeup_source_release(struct kobject *kobj)
+{
+	struct wakeup_source *ws;
+
+	ws = to_wakeup_source_obj(kobj);
+	kfree_const(ws->name);
+	kfree(ws);
+}
+
+static ssize_t wakeup_source_count_show(struct wakeup_source *ws,
+					struct wakeup_source_attribute *attr,
+					char *buf)
+{
+	unsigned long var;
+
+	if (strcmp(attr->attr.name, "active_count") == 0)
+		var = ws->active_count;
+	else if (strcmp(attr->attr.name, "event_count") == 0)
+		var = ws->event_count;
+	else if (strcmp(attr->attr.name, "wakeup_count") == 0)
+		var = ws->wakeup_count;
+	else
+		var = ws->expire_count;
+
+	return sprintf(buf, "%lu\n", var);
+}
+
+#define wakeup_source_count_attr(_name)			\
+static struct wakeup_source_attribute _name##_attr = {	\
+	.attr	= {					\
+		.name = __stringify(_name),		\
+		.mode = 0444,				\
+	},						\
+	.show	= wakeup_source_count_show,		\
+}
+
+wakeup_source_count_attr(active_count);
+wakeup_source_count_attr(event_count);
+wakeup_source_count_attr(wakeup_count);
+wakeup_source_count_attr(expire_count);
+
+#define wakeup_source_attr(_name) \
+static struct wakeup_source_attribute _name##_attr = __ATTR_RO(_name)
+
+static ssize_t active_time_ms_show(struct wakeup_source *ws,
+				struct wakeup_source_attribute *attr,
+				char *buf)
+{
+	ktime_t active_time =
+		ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
+	return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
+}
+wakeup_source_attr(active_time_ms);
+
+static ssize_t total_time_ms_show(struct wakeup_source *ws,
+			       struct wakeup_source_attribute *attr,
+			       char *buf)
+{
+	ktime_t active_time;
+	ktime_t total_time = ws->total_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		total_time = ktime_add(total_time, active_time);
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
+}
+wakeup_source_attr(total_time_ms);
+
+static ssize_t max_time_ms_show(struct wakeup_source *ws,
+			     struct wakeup_source_attribute *attr,
+			     char *buf)
+{
+	ktime_t active_time;
+	ktime_t max_time = ws->max_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		if (active_time > max_time)
+			max_time = active_time;
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
+}
+wakeup_source_attr(max_time_ms);
+
+static ssize_t last_change_ms_show(struct wakeup_source *ws,
+				struct wakeup_source_attribute *attr,
+				char *buf)
+{
+	return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
+}
+wakeup_source_attr(last_change_ms);
+
+static ssize_t prevent_suspend_time_ms_show(struct wakeup_source *ws,
+					 struct wakeup_source_attribute *attr,
+					 char *buf)
+{
+	ktime_t prevent_sleep_time = ws->prevent_sleep_time;
+
+	if (ws->active && ws->autosleep_enabled) {
+		prevent_sleep_time = ktime_add(prevent_sleep_time,
+			ktime_sub(ktime_get(), ws->start_prevent_time));
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
+}
+wakeup_source_attr(prevent_suspend_time_ms);
+
+static struct attribute *wakeup_source_default_attrs[] = {
+	&active_count_attr.attr,
+	&event_count_attr.attr,
+	&wakeup_count_attr.attr,
+	&expire_count_attr.attr,
+	&active_time_ms_attr.attr,
+	&total_time_ms_attr.attr,
+	&max_time_ms_attr.attr,
+	&last_change_ms_attr.attr,
+	&prevent_suspend_time_ms_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_source_default);
+
+struct kobj_type wakeup_source_ktype = {
+	.sysfs_ops = &wakeup_source_sysfs_ops,
+	.release = wakeup_source_release,
+	.default_groups = wakeup_source_default_groups,
+};
+
+#ifdef CONFIG_PM_SLEEP_STATS
+
+static struct kset *wakeup_source_kset;
+
+/**
+ * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
+ * @ws: Wakeup source to be exposed in sysfs.
+ */
+int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	ws->kobj.kset = wakeup_source_kset;
+	return kobject_add(&ws->kobj, NULL, "%s", ws->name);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_source_kset = kset_create_and_add("wakeup_sources", NULL,
+			power_kobj);
+	if (!wakeup_source_kset)
+		return -ENOMEM;
+
+	return 0;
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
+
+#endif /* !CONFIG_PM_SLEEP_STATS */
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index ce57771fab9b..67026c52975c 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -57,6 +57,7 @@ struct wakeup_source {
 	unsigned long		wakeup_count;
 	bool			active:1;
 	bool			autosleep_enabled:1;
+	struct kobject		kobj;
 };

 #ifdef CONFIG_PM_SLEEP
@@ -181,6 +182,20 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,

 #endif /* !CONFIG_PM_SLEEP */

+#ifdef CONFIG_PM_SLEEP_STATS
+
+/* drivers/base/power/wakeup_stats.c */
+extern int wakeup_source_sysfs_add(struct wakeup_source *ws);
+
+#else /* !CONFIG_PM_SLEEP_STATS */
+
+static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	return 0;
+}
+
+#endif /* !CONFIG_PM_SLEEP_STATS */
+
 static inline void wakeup_source_init(struct wakeup_source *ws,
 				      const char *name)
 {
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index ff8592ddedee..4d258f34020b 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
 	def_bool y
 	depends on PM_DEBUG && PM_SLEEP

+config PM_SLEEP_STATS
+	bool "Wakeup sources statistics"
+	depends on PM_SLEEP
+	help
+	  Expose wakeup sources statistics to user space via sysfs. Collected
+	  statistics are located under /sys/power/wakeup_sources. For more
+	  information, read <file:Documentation/ABI/testing/sysfs-power>.
+
+	  If in doubt, say N.
+
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
 	depends on PM_DEBUG && PSTORE && EXPERT
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..eebbc537321f 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -27,7 +27,7 @@ static DEFINE_MUTEX(wakelocks_lock);
 struct wakelock {
 	char			*name;
 	struct rb_node		node;
-	struct wakeup_source	ws;
+	struct wakeup_source	*ws;
 #ifdef CONFIG_PM_WAKELOCKS_GC
 	struct list_head	lru;
 #endif
@@ -46,7 +46,7 @@ ssize_t pm_show_wakelocks(char *buf, bool show_active)

 	for (node = rb_first(&wakelocks_tree); node; node = rb_next(node)) {
 		wl = rb_entry(node, struct wakelock, node);
-		if (wl->ws.active == show_active)
+		if (wl->ws->active == show_active)
 			str += scnprintf(str, end - str, "%s ", wl->name);
 	}
 	if (str > buf)
@@ -112,18 +112,19 @@ static void __wakelocks_gc(struct work_struct *work)
 		u64 idle_time_ns;
 		bool active;

-		spin_lock_irq(&wl->ws.lock);
-		idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws.last_time));
-		active = wl->ws.active;
-		spin_unlock_irq(&wl->ws.lock);
+		spin_lock_irq(&wl->ws->lock);
+		idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws->last_time));
+		active = wl->ws->active;
+		spin_unlock_irq(&wl->ws->lock);

 		if (idle_time_ns < ((u64)WL_GC_TIME_SEC * NSEC_PER_SEC))
 			break;

 		if (!active) {
-			wakeup_source_remove(&wl->ws);
+			wakeup_source_remove(wl->ws);
 			rb_erase(&wl->node, &wakelocks_tree);
 			list_del(&wl->lru);
+			kobject_put(&wl->ws->kobj);
 			kfree(wl->name);
 			kfree(wl);
 			decrement_wakelocks_number();
@@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	struct rb_node **node = &wakelocks_tree.rb_node;
 	struct rb_node *parent = *node;
 	struct wakelock *wl;
+	int ret;

 	while (*node) {
 		int diff;
@@ -187,9 +189,23 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 		kfree(wl);
 		return ERR_PTR(-ENOMEM);
 	}
-	wl->ws.name = wl->name;
-	wl->ws.last_time = ktime_get();
-	wakeup_source_add(&wl->ws);
+
+	wl->ws = wakeup_source_create(wl->name);
+	if (!wl->ws) {
+		kfree(wl);
+		return ERR_PTR(-ENOMEM);
+	}
+	wl->ws->last_time = ktime_get();
+
+	ret = wakeup_source_sysfs_add(wl->ws);
+	if (ret) {
+		kobject_put(&wl->ws->kobj);
+		kfree(wl->name);
+		kfree(wl);
+		return ERR_PTR(ret);
+	}
+
+	wakeup_source_add(wl->ws);
 	rb_link_node(&wl->node, parent, node);
 	rb_insert_color(&wl->node, &wakelocks_tree);
 	wakelocks_lru_add(wl);
@@ -233,9 +249,9 @@ int pm_wake_lock(const char *buf)
 		u64 timeout_ms = timeout_ns + NSEC_PER_MSEC - 1;

 		do_div(timeout_ms, NSEC_PER_MSEC);
-		__pm_wakeup_event(&wl->ws, timeout_ms);
+		__pm_wakeup_event(wl->ws, timeout_ms);
 	} else {
-		__pm_stay_awake(&wl->ws);
+		__pm_stay_awake(wl->ws);
 	}

 	wakelocks_lru_most_recent(wl);
@@ -271,7 +287,7 @@ int pm_wake_unlock(const char *buf)
 		ret = PTR_ERR(wl);
 		goto out;
 	}
-	__pm_relax(&wl->ws);
+	__pm_relax(wl->ws);

 	wakelocks_lru_most_recent(wl);
 	wakelocks_gc();
--
2.22.0.410.gd8fdbe21b5-goog

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

[Tri Vo]

On Wed, Jun 26, 2019 at 5:04 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 26, 2019 at 03:48:58PM -0700, Tri Vo wrote:
> > On Tue, Jun 25, 2019 at 6:33 PM Tri Vo <trong@android.com> wrote:
> > >
> > > On Tue, Jun 25, 2019 at 6:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Jun 25, 2019 at 05:54:49PM -0700, Tri Vo wrote:
> > > > > Embedding a struct kobject into struct wakeup_source changes lifetime
> > > > > requirements on the latter. To that end, change deallocation of struct
> > > > > wakeup_source using kfree to kobject_put().
> > > >
> > > > Ick, are you sure you need a new kobject here?  Why wouldn't a named
> > > > attribute group work instead?  That should keep this patch much smaller
> > > > and simpler.
> > >
> > > Yeah, named attribute groups might be a much cleaner way to do this.
> > > Let me investigate.
> >
> > Say, we read /sys/power/wakeup_sources/foo/active_count.
>
> What is "foo" here?  You didn't include Documentation of the sysfs
> files so it was pretty impossible to say what exactly your heirachy was
> going to be in order to determine this :)

Sorry about that. I sent out a new version of the patch with updated
documentation.

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

[Greg KH]

On Thu, Jun 27, 2019 at 03:53:35PM -0700, Tri Vo 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, expose wakeup
> sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> 
> Embedding a struct kobject into struct wakeup_source changes lifetime
> requirements on the latter. To that end, change deallocation of struct
> wakeup_source using kfree to kobject_put().
> 
> Change struct wakelock's wakeup_source member to a pointer to decouple
> lifetimes of struct wakelock and struct wakeup_source for above reason.
> 
> Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> source statistics in sysfs.
> 
> Signed-off-by: Tri Vo <trong@android.com>

Ok, this looks much better, but I don't like the use of a "raw" kobject
here.  It is much simpler, and less code, to use 'struct device'
instead.

As proof, I reworked the patch to do just that, and it saves over 50
lines of .c code, which is always nice :)

Attached below is the reworked code, along with the updated
documentation file.  It creates devices in a virtual class, and you can
easily iterate over them all by looking in /sys/class/wakeup/.

Note, I'm note quite sure you need all of the changes you made in
kernel/power/wakelock.c when you make the structure contain a pointer to
the wakeup source and not the structure itself, but I just went with it
and got it all to build properly.

Also note, I've not actually tested this at all, only built it, so I
_strongly_ suggest that you test this to make sure it really works :)

What do you think?

thanks,

greg k-h

------------------



---
 Documentation/ABI/testing/sysfs-power |  73 ++++++++++++-
 drivers/base/power/Makefile           |   1 +
 drivers/base/power/wakeup.c           |  11 +-
 drivers/base/power/wakeup_stats.c     | 144 ++++++++++++++++++++++++++
 include/linux/pm_wakeup.h             |  19 ++++
 kernel/power/Kconfig                  |  10 ++
 kernel/power/wakelock.c               |  40 ++++---
 7 files changed, 280 insertions(+), 18 deletions(-)
 create mode 100644 drivers/base/power/wakeup_stats.c

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 18b7dc929234..ef92628e6fc3 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -300,4 +300,75 @@ Description:
 		attempt.
 
 		Using this sysfs file will override any values that were
-		set using the kernel command line for disk offset.
\ No newline at end of file
+		set using the kernel command line for disk offset.
+
+What:		/sys/class/wakeup/
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/class/wakeup/ directory contains pointers to all
+		wakeup sources in the kernel at that moment in time.
+
+What:		/sys/class/wakeup/.../active_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source was
+		activated.
+
+What:		/sys/class/wakeup/.../event_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of signaled wakeup events
+		associated with the wakeup source.
+
+What:		/sys/class/wakeup/.../wakeup_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source might
+		abort suspend.
+
+What:		/sys/class/wakeup/.../expire_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source's
+		timeout has expired.
+
+What:		/sys/class/wakeup/.../active_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the amount of time the wakeup source has
+		been continuously active, in milliseconds.  If the wakeup
+		source is not active, this file contains '0'.
+
+What:		/sys/class/wakeup/.../total_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the total amount of time this wakeup source
+		has been active, in milliseconds.
+
+What:		/sys/class/wakeup/.../max_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the maximum amount of time this wakeup
+		source has been continuously active, in milliseconds.
+
+What:		/sys/class/wakeup/.../last_change_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the monotonic clock time when the wakeup
+		source was touched last time, in milliseconds.
+
+What:		/sys/class/wakeup/.../prevent_suspend_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The file contains the total amount of time this wakeup source
+		has been preventing autosleep, in milliseconds.
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..1963f53d9982 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP_STATS)	+= wakeup_stats.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..a5e5ffaa4532 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -147,8 +147,7 @@ void wakeup_source_destroy(struct wakeup_source *ws)
 
 	__pm_relax(ws);
 	wakeup_source_record(ws);
-	kfree_const(ws->name);
-	kfree(ws);
+	wakeup_source_sysfs_remove(ws);
 }
 EXPORT_SYMBOL_GPL(wakeup_source_destroy);
 
@@ -205,11 +204,15 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 struct wakeup_source *wakeup_source_register(const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;
 
 	ws = wakeup_source_create(name);
-	if (ws)
+	if (ws) {
+		ret = wakeup_source_sysfs_add(ws);
+		if (ret)
+			return NULL;
 		wakeup_source_add(ws);
-
+	}
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_register);
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
new file mode 100644
index 000000000000..e0c4767304be
--- /dev/null
+++ b/drivers/base/power/wakeup_stats.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wakeup statistics in sysfs
+ *
+ * Copyright (c) 2019 Linux Foundation <gregkh@linuxfoundation.org>
+ * Copyright (c) 2019 Google Inc.
+ */
+
+#include <linux/slab.h>
+#include <linux/kdev_t.h>
+#include <linux/device.h>
+
+#include "power.h"
+
+static struct class *wakeup_class;
+
+#define wakeup_attr(_name)						\
+static ssize_t _name##_show(struct device *dev,				\
+			    struct device_attribute *attr, char *buf)	\
+{									\
+	struct wakeup_source *ws = dev_get_drvdata(dev);		\
+									\
+	return sprintf(buf, "%lu\n", ws->_name);				\
+}									\
+static DEVICE_ATTR_RO(_name)
+
+wakeup_attr(active_count);
+wakeup_attr(event_count);
+wakeup_attr(wakeup_count);
+wakeup_attr(expire_count);
+
+static ssize_t active_time_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time =
+		ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
+}
+static DEVICE_ATTR_RO(active_time_ms);
+
+static ssize_t total_time_ms_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t total_time = ws->total_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		total_time = ktime_add(total_time, active_time);
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
+}
+static DEVICE_ATTR_RO(total_time_ms);
+
+static ssize_t max_time_ms_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t max_time = ws->max_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		if (active_time > max_time)
+			max_time = active_time;
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
+}
+static DEVICE_ATTR_RO(max_time_ms);
+
+static ssize_t last_change_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
+}
+static DEVICE_ATTR_RO(last_change_ms);
+
+static ssize_t prevent_suspend_time_ms_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t prevent_sleep_time = ws->prevent_sleep_time;
+
+	if (ws->active && ws->autosleep_enabled) {
+		prevent_sleep_time = ktime_add(prevent_sleep_time,
+			ktime_sub(ktime_get(), ws->start_prevent_time));
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
+}
+static DEVICE_ATTR_RO(prevent_suspend_time_ms);
+
+static struct attribute *wakeup_source_attrs[] = {
+	&dev_attr_active_count.attr,
+	&dev_attr_event_count.attr,
+	&dev_attr_wakeup_count.attr,
+	&dev_attr_expire_count.attr,
+	&dev_attr_active_time_ms.attr,
+	&dev_attr_total_time_ms.attr,
+	&dev_attr_max_time_ms.attr,
+	&dev_attr_last_change_ms.attr,
+	&dev_attr_prevent_suspend_time_ms.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_source);
+
+/**
+ * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
+ * @ws: Wakeup source to be exposed in sysfs.
+ */
+int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	struct device *dev;
+
+	dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
+					wakeup_source_groups, "%s", ws->name);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+	ws->dev = dev;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
+
+void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{
+	put_device(ws->dev);
+	device_unregister(ws->dev);
+}
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_class = class_create(THIS_MODULE, "wakeup");
+	if (IS_ERR(wakeup_class))
+		return PTR_ERR(wakeup_class);
+
+	return 0;
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index ce57771fab9b..18ebc4ce817c 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -35,6 +35,7 @@ struct wake_irq;
  * @relax_count: Number of times the wakeup source was deactivated.
  * @expire_count: Number of times the wakeup source's timeout has expired.
  * @wakeup_count: Number of times the wakeup source might abort suspend.
+ * @dev: struct device for sysfs statistics about the wakeup source
  * @active: Status of the wakeup source.
  * @has_timeout: The wakeup source has been activated with a timeout.
  */
@@ -55,6 +56,7 @@ struct wakeup_source {
 	unsigned long		relax_count;
 	unsigned long		expire_count;
 	unsigned long		wakeup_count;
+	struct device		*dev;
 	bool			active:1;
 	bool			autosleep_enabled:1;
 };
@@ -181,6 +183,23 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM_SLEEP_STATS
+
+/* drivers/base/power/wakeup_stats.c */
+int wakeup_source_sysfs_add(struct wakeup_source *ws);
+void wakeup_source_sysfs_remove(struct wakeup_source *ws);
+
+#else /* !CONFIG_PM_SLEEP_STATS */
+
+static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	return 0;
+}
+static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{ }
+
+#endif /* !CONFIG_PM_SLEEP_STATS */
+
 static inline void wakeup_source_init(struct wakeup_source *ws,
 				      const char *name)
 {
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index ff8592ddedee..4d258f34020b 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
 	def_bool y
 	depends on PM_DEBUG && PM_SLEEP
 
+config PM_SLEEP_STATS
+	bool "Wakeup sources statistics"
+	depends on PM_SLEEP
+	help
+	  Expose wakeup sources statistics to user space via sysfs. Collected
+	  statistics are located under /sys/power/wakeup_sources. For more
+	  information, read <file:Documentation/ABI/testing/sysfs-power>.
+
+	  If in doubt, say N.
+
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
 	depends on PM_DEBUG && PSTORE && EXPERT
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..dce02c2f4496 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -27,7 +27,7 @@ static DEFINE_MUTEX(wakelocks_lock);
 struct wakelock {
 	char			*name;
 	struct rb_node		node;
-	struct wakeup_source	ws;
+	struct wakeup_source	*ws;
 #ifdef CONFIG_PM_WAKELOCKS_GC
 	struct list_head	lru;
 #endif
@@ -46,7 +46,7 @@ ssize_t pm_show_wakelocks(char *buf, bool show_active)
 
 	for (node = rb_first(&wakelocks_tree); node; node = rb_next(node)) {
 		wl = rb_entry(node, struct wakelock, node);
-		if (wl->ws.active == show_active)
+		if (wl->ws->active == show_active)
 			str += scnprintf(str, end - str, "%s ", wl->name);
 	}
 	if (str > buf)
@@ -112,16 +112,16 @@ static void __wakelocks_gc(struct work_struct *work)
 		u64 idle_time_ns;
 		bool active;
 
-		spin_lock_irq(&wl->ws.lock);
-		idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws.last_time));
-		active = wl->ws.active;
-		spin_unlock_irq(&wl->ws.lock);
+		spin_lock_irq(&wl->ws->lock);
+		idle_time_ns = ktime_to_ns(ktime_sub(now, wl->ws->last_time));
+		active = wl->ws->active;
+		spin_unlock_irq(&wl->ws->lock);
 
 		if (idle_time_ns < ((u64)WL_GC_TIME_SEC * NSEC_PER_SEC))
 			break;
 
 		if (!active) {
-			wakeup_source_remove(&wl->ws);
+			wakeup_source_remove(wl->ws);
 			rb_erase(&wl->node, &wakelocks_tree);
 			list_del(&wl->lru);
 			kfree(wl->name);
@@ -153,6 +153,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	struct rb_node **node = &wakelocks_tree.rb_node;
 	struct rb_node *parent = *node;
 	struct wakelock *wl;
+	int ret;
 
 	while (*node) {
 		int diff;
@@ -187,9 +188,22 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 		kfree(wl);
 		return ERR_PTR(-ENOMEM);
 	}
-	wl->ws.name = wl->name;
-	wl->ws.last_time = ktime_get();
-	wakeup_source_add(&wl->ws);
+
+	wl->ws = wakeup_source_create(wl->name);
+	if (!wl->ws) {
+		kfree(wl);
+		return ERR_PTR(-ENOMEM);
+	}
+	wl->ws->last_time = ktime_get();
+
+	ret = wakeup_source_sysfs_add(wl->ws);
+	if (ret) {
+		kfree(wl->name);
+		kfree(wl);
+		return ERR_PTR(ret);
+	}
+
+	wakeup_source_add(wl->ws);
 	rb_link_node(&wl->node, parent, node);
 	rb_insert_color(&wl->node, &wakelocks_tree);
 	wakelocks_lru_add(wl);
@@ -233,9 +247,9 @@ int pm_wake_lock(const char *buf)
 		u64 timeout_ms = timeout_ns + NSEC_PER_MSEC - 1;
 
 		do_div(timeout_ms, NSEC_PER_MSEC);
-		__pm_wakeup_event(&wl->ws, timeout_ms);
+		__pm_wakeup_event(wl->ws, timeout_ms);
 	} else {
-		__pm_stay_awake(&wl->ws);
+		__pm_stay_awake(wl->ws);
 	}
 
 	wakelocks_lru_most_recent(wl);
@@ -271,7 +285,7 @@ int pm_wake_unlock(const char *buf)
 		ret = PTR_ERR(wl);
 		goto out;
 	}
-	__pm_relax(&wl->ws);
+	__pm_relax(wl->ws);
 
 	wakelocks_lru_most_recent(wl);
 	wakelocks_gc();
-- 
2.22.0

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

[Rafael J. Wysocki]

On Friday, June 28, 2019 5:10:40 PM CEST Greg KH wrote:
> On Thu, Jun 27, 2019 at 03:53:35PM -0700, Tri Vo 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, expose wakeup
> > sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> > 
> > Embedding a struct kobject into struct wakeup_source changes lifetime
> > requirements on the latter. To that end, change deallocation of struct
> > wakeup_source using kfree to kobject_put().
> > 
> > Change struct wakelock's wakeup_source member to a pointer to decouple
> > lifetimes of struct wakelock and struct wakeup_source for above reason.
> > 
> > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > source statistics in sysfs.
> > 
> > Signed-off-by: Tri Vo <trong@android.com>
> 
> Ok, this looks much better, but I don't like the use of a "raw" kobject
> here.  It is much simpler, and less code, to use 'struct device'
> instead.
> 
> As proof, I reworked the patch to do just that, and it saves over 50
> lines of .c code, which is always nice :)

Thanks for taking the time to do that!

> Attached below is the reworked code, along with the updated
> documentation file.  It creates devices in a virtual class, and you can
> easily iterate over them all by looking in /sys/class/wakeup/.

That actually is nice - no need to add anything under /sys/power/.

> Note, I'm note quite sure you need all of the changes you made in
> kernel/power/wakelock.c when you make the structure contain a pointer to
> the wakeup source and not the structure itself, but I just went with it
> and got it all to build properly.

I'm not really sure about it either.

> Also note, I've not actually tested this at all, only built it, so I
> _strongly_ suggest that you test this to make sure it really works :)
> 
> What do you think?

I agree with the direction. :-)

Cheers!

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

[Tri Vo]

On Thu, Jul 4, 2019 at 7:31 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Friday, June 28, 2019 5:10:40 PM CEST Greg KH wrote:
> > On Thu, Jun 27, 2019 at 03:53:35PM -0700, Tri Vo 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, expose wakeup
> > > sources statistics in sysfs under /sys/power/wakeup_sources/<name>/
> > >
> > > Embedding a struct kobject into struct wakeup_source changes lifetime
> > > requirements on the latter. To that end, change deallocation of struct
> > > wakeup_source using kfree to kobject_put().
> > >
> > > Change struct wakelock's wakeup_source member to a pointer to decouple
> > > lifetimes of struct wakelock and struct wakeup_source for above reason.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> > >
> > > Signed-off-by: Tri Vo <trong@android.com>
> >
> > Ok, this looks much better, but I don't like the use of a "raw" kobject
> > here.  It is much simpler, and less code, to use 'struct device'
> > instead.
> >
> > As proof, I reworked the patch to do just that, and it saves over 50
> > lines of .c code, which is always nice :)
>
> Thanks for taking the time to do that!

Thanks a lot, Greg!
>
> > Attached below is the reworked code, along with the updated
> > documentation file.  It creates devices in a virtual class, and you can
> > easily iterate over them all by looking in /sys/class/wakeup/.
>
> That actually is nice - no need to add anything under /sys/power/.
>
> > Note, I'm note quite sure you need all of the changes you made in
> > kernel/power/wakelock.c when you make the structure contain a pointer to
> > the wakeup source and not the structure itself, but I just went with it
> > and got it all to build properly.
>
> I'm not really sure about it either.
>
> > Also note, I've not actually tested this at all, only built it, so I
> > _strongly_ suggest that you test this to make sure it really works :)
> >
> > What do you think?
>
> I agree with the direction. :-)

I'll test things out and send v3 of the patch. Thanks!

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

[Tri Vo]

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/<name>/.

Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
source statistics in sysfs.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Tri Vo <trong@android.com>
Tested-by: Tri Vo <trong@android.com>
---
 Documentation/ABI/testing/sysfs-power |  73 ++++++++++++-
 drivers/base/power/Makefile           |   1 +
 drivers/base/power/wakeup.c           |  12 ++-
 drivers/base/power/wakeup_stats.c     | 148 ++++++++++++++++++++++++++
 include/linux/pm_wakeup.h             |  19 ++++
 kernel/power/Kconfig                  |  10 ++
 kernel/power/wakelock.c               |  10 ++
 7 files changed, 270 insertions(+), 3 deletions(-)
 create mode 100644 drivers/base/power/wakeup_stats.c

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 18b7dc929234..ef92628e6fc3 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -300,4 +300,75 @@ Description:
 		attempt.
 
 		Using this sysfs file will override any values that were
-		set using the kernel command line for disk offset.
\ No newline at end of file
+		set using the kernel command line for disk offset.
+
+What:		/sys/class/wakeup/
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/class/wakeup/ directory contains pointers to all
+		wakeup sources in the kernel at that moment in time.
+
+What:		/sys/class/wakeup/.../active_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source was
+		activated.
+
+What:		/sys/class/wakeup/.../event_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of signaled wakeup events
+		associated with the wakeup source.
+
+What:		/sys/class/wakeup/.../wakeup_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source might
+		abort suspend.
+
+What:		/sys/class/wakeup/.../expire_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source's
+		timeout has expired.
+
+What:		/sys/class/wakeup/.../active_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the amount of time the wakeup source has
+		been continuously active, in milliseconds.  If the wakeup
+		source is not active, this file contains '0'.
+
+What:		/sys/class/wakeup/.../total_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the total amount of time this wakeup source
+		has been active, in milliseconds.
+
+What:		/sys/class/wakeup/.../max_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the maximum amount of time this wakeup
+		source has been continuously active, in milliseconds.
+
+What:		/sys/class/wakeup/.../last_change_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the monotonic clock time when the wakeup
+		source was touched last time, in milliseconds.
+
+What:		/sys/class/wakeup/.../prevent_suspend_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The file contains the total amount of time this wakeup source
+		has been preventing autosleep, in milliseconds.
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..1963f53d9982 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP_STATS)	+= wakeup_stats.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..fd48e78c06b9 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -205,11 +205,18 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 struct wakeup_source *wakeup_source_register(const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;
 
 	ws = wakeup_source_create(name);
-	if (ws)
+	if (ws) {
+		ret = wakeup_source_sysfs_add(ws);
+		if (ret) {
+			kfree_const(ws->name);
+			kfree(ws);
+			return NULL;
+		}
 		wakeup_source_add(ws);
-
+	}
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_register);
@@ -222,6 +229,7 @@ void wakeup_source_unregister(struct wakeup_source *ws)
 {
 	if (ws) {
 		wakeup_source_remove(ws);
+		wakeup_source_sysfs_remove(ws);
 		wakeup_source_destroy(ws);
 	}
 }
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
new file mode 100644
index 000000000000..2a4d9ace1e19
--- /dev/null
+++ b/drivers/base/power/wakeup_stats.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wakeup statistics in sysfs
+ *
+ * Copyright (c) 2019 Linux Foundation <gregkh@linuxfoundation.org>
+ * Copyright (c) 2019 Google Inc.
+ */
+
+#include <linux/slab.h>
+#include <linux/kdev_t.h>
+
+#include "power.h"
+
+static struct class *wakeup_class;
+
+#define wakeup_attr(_name)						\
+static ssize_t _name##_show(struct device *dev,				\
+			    struct device_attribute *attr, char *buf)	\
+{									\
+	struct wakeup_source *ws = dev_get_drvdata(dev);		\
+									\
+	return sprintf(buf, "%lu\n", ws->_name);			\
+}									\
+static DEVICE_ATTR_RO(_name)
+
+wakeup_attr(active_count);
+wakeup_attr(event_count);
+wakeup_attr(wakeup_count);
+wakeup_attr(expire_count);
+
+static ssize_t active_time_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time =
+		ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
+}
+static DEVICE_ATTR_RO(active_time_ms);
+
+static ssize_t total_time_ms_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t total_time = ws->total_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		total_time = ktime_add(total_time, active_time);
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
+}
+static DEVICE_ATTR_RO(total_time_ms);
+
+static ssize_t max_time_ms_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t max_time = ws->max_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		if (active_time > max_time)
+			max_time = active_time;
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
+}
+static DEVICE_ATTR_RO(max_time_ms);
+
+static ssize_t last_change_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
+}
+static DEVICE_ATTR_RO(last_change_ms);
+
+static ssize_t prevent_suspend_time_ms_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t prevent_sleep_time = ws->prevent_sleep_time;
+
+	if (ws->active && ws->autosleep_enabled) {
+		prevent_sleep_time = ktime_add(prevent_sleep_time,
+			ktime_sub(ktime_get(), ws->start_prevent_time));
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
+}
+static DEVICE_ATTR_RO(prevent_suspend_time_ms);
+
+static struct attribute *wakeup_source_attrs[] = {
+	&dev_attr_active_count.attr,
+	&dev_attr_event_count.attr,
+	&dev_attr_wakeup_count.attr,
+	&dev_attr_expire_count.attr,
+	&dev_attr_active_time_ms.attr,
+	&dev_attr_total_time_ms.attr,
+	&dev_attr_max_time_ms.attr,
+	&dev_attr_last_change_ms.attr,
+	&dev_attr_prevent_suspend_time_ms.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_source);
+
+/**
+ * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
+ * @ws: Wakeup source to be added in sysfs.
+ */
+int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	struct device *dev;
+
+	dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
+					wakeup_source_groups, "%s", ws->name);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	ws->dev = dev;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
+
+/**
+ * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
+ * @ws: Wakeup source to be removed from sysfs.
+ */
+void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{
+	device_unregister(ws->dev);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_class = class_create(THIS_MODULE, "wakeup");
+	if (IS_ERR(wakeup_class))
+		return PTR_ERR(wakeup_class);
+
+	return 0;
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index ce57771fab9b..734141869c17 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -35,6 +35,7 @@ struct wake_irq;
  * @relax_count: Number of times the wakeup source was deactivated.
  * @expire_count: Number of times the wakeup source's timeout has expired.
  * @wakeup_count: Number of times the wakeup source might abort suspend.
+ * @dev: Struct device for sysfs statistics about the wakeup source.
  * @active: Status of the wakeup source.
  * @has_timeout: The wakeup source has been activated with a timeout.
  */
@@ -55,6 +56,7 @@ struct wakeup_source {
 	unsigned long		relax_count;
 	unsigned long		expire_count;
 	unsigned long		wakeup_count;
+	struct device		*dev;
 	bool			active:1;
 	bool			autosleep_enabled:1;
 };
@@ -181,6 +183,23 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
 
 #endif /* !CONFIG_PM_SLEEP */
 
+#ifdef CONFIG_PM_SLEEP_STATS
+
+/* drivers/base/power/wakeup_stats.c */
+int wakeup_source_sysfs_add(struct wakeup_source *ws);
+void wakeup_source_sysfs_remove(struct wakeup_source *ws);
+
+#else /* !CONFIG_PM_SLEEP_STATS */
+
+static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	return 0;
+}
+static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{ }
+
+#endif /* !CONFIG_PM_SLEEP_STATS */
+
 static inline void wakeup_source_init(struct wakeup_source *ws,
 				      const char *name)
 {
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index ff8592ddedee..4d258f34020b 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
 	def_bool y
 	depends on PM_DEBUG && PM_SLEEP
 
+config PM_SLEEP_STATS
+	bool "Wakeup sources statistics"
+	depends on PM_SLEEP
+	help
+	  Expose wakeup sources statistics to user space via sysfs. Collected
+	  statistics are located under /sys/power/wakeup_sources. For more
+	  information, read <file:Documentation/ABI/testing/sysfs-power>.
+
+	  If in doubt, say N.
+
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
 	depends on PM_DEBUG && PSTORE && EXPERT
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..32726da3d6e6 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -122,6 +122,7 @@ static void __wakelocks_gc(struct work_struct *work)
 
 		if (!active) {
 			wakeup_source_remove(&wl->ws);
+			wakeup_source_sysfs_remove(&wl->ws);
 			rb_erase(&wl->node, &wakelocks_tree);
 			list_del(&wl->lru);
 			kfree(wl->name);
@@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	struct rb_node **node = &wakelocks_tree.rb_node;
 	struct rb_node *parent = *node;
 	struct wakelock *wl;
+	int ret;
 
 	while (*node) {
 		int diff;
@@ -189,6 +191,14 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	}
 	wl->ws.name = wl->name;
 	wl->ws.last_time = ktime_get();
+
+	ret = wakeup_source_sysfs_add(&wl->ws);
+	if (ret) {
+		kfree(wl->name);
+		kfree(wl);
+		return ERR_PTR(ret);
+	}
+
 	wakeup_source_add(&wl->ws);
 	rb_link_node(&wl->node, parent, node);
 	rb_insert_color(&wl->node, &wakelocks_tree);
-- 
2.22.0.510.g264f2c817a-goog

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

[Greg KH]

On Mon, Jul 15, 2019 at 01:11:16PM -0700, Tri Vo 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/<name>/.
> 
> Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> source statistics in sysfs.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Tri Vo <trong@android.com>
> Tested-by: Tri Vo <trong@android.com>

Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
perhaps given that I rewrote the whole file last time?  :)


> ---
>  Documentation/ABI/testing/sysfs-power |  73 ++++++++++++-
>  drivers/base/power/Makefile           |   1 +
>  drivers/base/power/wakeup.c           |  12 ++-
>  drivers/base/power/wakeup_stats.c     | 148 ++++++++++++++++++++++++++
>  include/linux/pm_wakeup.h             |  19 ++++
>  kernel/power/Kconfig                  |  10 ++
>  kernel/power/wakelock.c               |  10 ++
>  7 files changed, 270 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/base/power/wakeup_stats.c

What changed from v2?  :)

> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index 18b7dc929234..ef92628e6fc3 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -300,4 +300,75 @@ Description:
>  		attempt.
>  
>  		Using this sysfs file will override any values that were
> -		set using the kernel command line for disk offset.
> \ No newline at end of file
> +		set using the kernel command line for disk offset.
> +
> +What:		/sys/class/wakeup/

My fault, this should be a new ABI/testing file, don't use sysfs-power
as it does not make any sense now, right?

> --- /dev/null
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wakeup statistics in sysfs
> + *
> + * Copyright (c) 2019 Linux Foundation <gregkh@linuxfoundation.org>

My fault on the original here, this line should be 2 lines:

> + * Copyright (c) 2019 Linux Foundation
> + * Copyright (c) 2019 Greg Kroah-Hartman <gregkh@linuxfoundation.org>

as those are two different entities that have copyright here on this
file.

> +config PM_SLEEP_STATS
> +	bool "Wakeup sources statistics"
> +	depends on PM_SLEEP
> +	help
> +	  Expose wakeup sources statistics to user space via sysfs. Collected
> +	  statistics are located under /sys/power/wakeup_sources. For more
> +	  information, read <file:Documentation/ABI/testing/sysfs-power>.

Filename should be changed.


> +
> +	  If in doubt, say N.
> +
>  config DPM_WATCHDOG
>  	bool "Device suspend/resume watchdog"
>  	depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 4210152e56f0..32726da3d6e6 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -122,6 +122,7 @@ static void __wakelocks_gc(struct work_struct *work)
>  
>  		if (!active) {
>  			wakeup_source_remove(&wl->ws);
> +			wakeup_source_sysfs_remove(&wl->ws);
>  			rb_erase(&wl->node, &wakelocks_tree);
>  			list_del(&wl->lru);
>  			kfree(wl->name);
> @@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>  	struct rb_node **node = &wakelocks_tree.rb_node;
>  	struct rb_node *parent = *node;
>  	struct wakelock *wl;
> +	int ret;
>  
>  	while (*node) {
>  		int diff;
> @@ -189,6 +191,14 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>  	}
>  	wl->ws.name = wl->name;
>  	wl->ws.last_time = ktime_get();
> +
> +	ret = wakeup_source_sysfs_add(&wl->ws);
> +	if (ret) {
> +		kfree(wl->name);
> +		kfree(wl);
> +		return ERR_PTR(ret);
> +	}
> +
>  	wakeup_source_add(&wl->ws);
>  	rb_link_node(&wl->node, parent, node);
>  	rb_insert_color(&wl->node, &wakelocks_tree);

Nice, you got rid of the extra change in this file, making this much
simpler.

Can you make these minor tweaks?  If so, I'll be glad to queue this up
after 5.3-rc1 is out.

And I am guessing that you actually tested this all out, and it works
for you?  Have you changed Android userspace to use the new api with no
problems?

thanks,

greg k-h

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

[Tri Vo]

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/<name>/.

Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
source statistics in sysfs.

Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Tri Vo <trong@android.com>
Tested-by: Tri Vo <trong@android.com>
Tested-by: Kalesh Singh <kaleshsingh@google.com>
---
 Documentation/ABI/testing/sysfs-class-wakeup |  70 +++++++++
 drivers/base/power/Makefile                  |   1 +
 drivers/base/power/wakeup.c                  |  12 +-
 drivers/base/power/wakeup_stats.c            | 149 +++++++++++++++++++
 include/linux/pm_wakeup.h                    |  19 +++
 kernel/power/Kconfig                         |  10 ++
 kernel/power/wakelock.c                      |  10 ++
 7 files changed, 269 insertions(+), 2 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.

diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup
new file mode 100644
index 000000000000..30fb23eb9112
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-wakeup
@@ -0,0 +1,70 @@
+What:		/sys/class/wakeup/
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The /sys/class/wakeup/ directory contains pointers to all
+		wakeup sources in the kernel at that moment in time.
+
+What:		/sys/class/wakeup/.../active_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source was
+		activated.
+
+What:		/sys/class/wakeup/.../event_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of signaled wakeup events
+		associated with the wakeup source.
+
+What:		/sys/class/wakeup/.../wakeup_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source might
+		abort suspend.
+
+What:		/sys/class/wakeup/.../expire_count
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the number of times the wakeup source's
+		timeout has expired.
+
+What:		/sys/class/wakeup/.../active_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the amount of time the wakeup source has
+		been continuously active, in milliseconds.  If the wakeup
+		source is not active, this file contains '0'.
+
+What:		/sys/class/wakeup/.../total_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the total amount of time this wakeup source
+		has been active, in milliseconds.
+
+What:		/sys/class/wakeup/.../max_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the maximum amount of time this wakeup
+		source has been continuously active, in milliseconds.
+
+What:		/sys/class/wakeup/.../last_change_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the monotonic clock time when the wakeup
+		source was touched last time, in milliseconds.
+
+What:		/sys/class/wakeup/.../prevent_suspend_time_ms
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		The file contains the total amount of time this wakeup source
+		has been preventing autosleep, in milliseconds.
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index e1bb691cf8f1..1963f53d9982 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
+obj-$(CONFIG_PM_SLEEP_STATS)	+= wakeup_stats.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 5b2b6a05a4f3..fd48e78c06b9 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -205,11 +205,18 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 struct wakeup_source *wakeup_source_register(const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;

 	ws = wakeup_source_create(name);
-	if (ws)
+	if (ws) {
+		ret = wakeup_source_sysfs_add(ws);
+		if (ret) {
+			kfree_const(ws->name);
+			kfree(ws);
+			return NULL;
+		}
 		wakeup_source_add(ws);
-
+	}
 	return ws;
 }
 EXPORT_SYMBOL_GPL(wakeup_source_register);
@@ -222,6 +229,7 @@ void wakeup_source_unregister(struct wakeup_source *ws)
 {
 	if (ws) {
 		wakeup_source_remove(ws);
+		wakeup_source_sysfs_remove(ws);
 		wakeup_source_destroy(ws);
 	}
 }
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
new file mode 100644
index 000000000000..45e15856105b
--- /dev/null
+++ b/drivers/base/power/wakeup_stats.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wakeup statistics in sysfs
+ *
+ * Copyright (c) 2019 Linux Foundation
+ * Copyright (c) 2019 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+ * Copyright (c) 2019 Google Inc.
+ */
+
+#include <linux/slab.h>
+#include <linux/kdev_t.h>
+
+#include "power.h"
+
+static struct class *wakeup_class;
+
+#define wakeup_attr(_name)						\
+static ssize_t _name##_show(struct device *dev,				\
+			    struct device_attribute *attr, char *buf)	\
+{									\
+	struct wakeup_source *ws = dev_get_drvdata(dev);		\
+									\
+	return sprintf(buf, "%lu\n", ws->_name);			\
+}									\
+static DEVICE_ATTR_RO(_name)
+
+wakeup_attr(active_count);
+wakeup_attr(event_count);
+wakeup_attr(wakeup_count);
+wakeup_attr(expire_count);
+
+static ssize_t active_time_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time =
+		ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
+}
+static DEVICE_ATTR_RO(active_time_ms);
+
+static ssize_t total_time_ms_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t total_time = ws->total_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		total_time = ktime_add(total_time, active_time);
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
+}
+static DEVICE_ATTR_RO(total_time_ms);
+
+static ssize_t max_time_ms_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t active_time;
+	ktime_t max_time = ws->max_time;
+
+	if (ws->active) {
+		active_time = ktime_sub(ktime_get(), ws->last_time);
+		if (active_time > max_time)
+			max_time = active_time;
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
+}
+static DEVICE_ATTR_RO(max_time_ms);
+
+static ssize_t last_change_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
+}
+static DEVICE_ATTR_RO(last_change_ms);
+
+static ssize_t prevent_suspend_time_ms_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+	ktime_t prevent_sleep_time = ws->prevent_sleep_time;
+
+	if (ws->active && ws->autosleep_enabled) {
+		prevent_sleep_time = ktime_add(prevent_sleep_time,
+			ktime_sub(ktime_get(), ws->start_prevent_time));
+	}
+	return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
+}
+static DEVICE_ATTR_RO(prevent_suspend_time_ms);
+
+static struct attribute *wakeup_source_attrs[] = {
+	&dev_attr_active_count.attr,
+	&dev_attr_event_count.attr,
+	&dev_attr_wakeup_count.attr,
+	&dev_attr_expire_count.attr,
+	&dev_attr_active_time_ms.attr,
+	&dev_attr_total_time_ms.attr,
+	&dev_attr_max_time_ms.attr,
+	&dev_attr_last_change_ms.attr,
+	&dev_attr_prevent_suspend_time_ms.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(wakeup_source);
+
+/**
+ * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
+ * @ws: Wakeup source to be added in sysfs.
+ */
+int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	struct device *dev;
+
+	dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
+					wakeup_source_groups, "%s", ws->name);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	ws->dev = dev;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
+
+/**
+ * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
+ * @ws: Wakeup source to be removed from sysfs.
+ */
+void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{
+	device_unregister(ws->dev);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_class = class_create(THIS_MODULE, "wakeup");
+	if (IS_ERR(wakeup_class))
+		return PTR_ERR(wakeup_class);
+
+	return 0;
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index ce57771fab9b..734141869c17 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -35,6 +35,7 @@ struct wake_irq;
  * @relax_count: Number of times the wakeup source was deactivated.
  * @expire_count: Number of times the wakeup source's timeout has expired.
  * @wakeup_count: Number of times the wakeup source might abort suspend.
+ * @dev: Struct device for sysfs statistics about the wakeup source.
  * @active: Status of the wakeup source.
  * @has_timeout: The wakeup source has been activated with a timeout.
  */
@@ -55,6 +56,7 @@ struct wakeup_source {
 	unsigned long		relax_count;
 	unsigned long		expire_count;
 	unsigned long		wakeup_count;
+	struct device		*dev;
 	bool			active:1;
 	bool			autosleep_enabled:1;
 };
@@ -181,6 +183,23 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,

 #endif /* !CONFIG_PM_SLEEP */

+#ifdef CONFIG_PM_SLEEP_STATS
+
+/* drivers/base/power/wakeup_stats.c */
+int wakeup_source_sysfs_add(struct wakeup_source *ws);
+void wakeup_source_sysfs_remove(struct wakeup_source *ws);
+
+#else /* !CONFIG_PM_SLEEP_STATS */
+
+static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
+{
+	return 0;
+}
+static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws)
+{ }
+
+#endif /* !CONFIG_PM_SLEEP_STATS */
+
 static inline void wakeup_source_init(struct wakeup_source *ws,
 				      const char *name)
 {
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index ff8592ddedee..604e1f087f14 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
 	def_bool y
 	depends on PM_DEBUG && PM_SLEEP

+config PM_SLEEP_STATS
+	bool "Wakeup sources statistics"
+	depends on PM_SLEEP
+	help
+	  Expose wakeup sources statistics to user space via sysfs. Collected
+	  statistics are located under /sys/power/wakeup_sources. For more
+	  information, read <file:Documentation/ABI/testing/sysfs-class-wakeup>.
+
+	  If in doubt, say N.
+
 config DPM_WATCHDOG
 	bool "Device suspend/resume watchdog"
 	depends on PM_DEBUG && PSTORE && EXPERT
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..32726da3d6e6 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -122,6 +122,7 @@ static void __wakelocks_gc(struct work_struct *work)

 		if (!active) {
 			wakeup_source_remove(&wl->ws);
+			wakeup_source_sysfs_remove(&wl->ws);
 			rb_erase(&wl->node, &wakelocks_tree);
 			list_del(&wl->lru);
 			kfree(wl->name);
@@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	struct rb_node **node = &wakelocks_tree.rb_node;
 	struct rb_node *parent = *node;
 	struct wakelock *wl;
+	int ret;

 	while (*node) {
 		int diff;
@@ -189,6 +191,14 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
 	}
 	wl->ws.name = wl->name;
 	wl->ws.last_time = ktime_get();
+
+	ret = wakeup_source_sysfs_add(&wl->ws);
+	if (ret) {
+		kfree(wl->name);
+		kfree(wl);
+		return ERR_PTR(ret);
+	}
+
 	wakeup_source_add(&wl->ws);
 	rb_link_node(&wl->node, parent, node);
 	rb_insert_color(&wl->node, &wakelocks_tree);
--
2.22.0.510.g264f2c817a-goog

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

[Tri Vo]

On Mon, Jul 15, 2019 at 1:37 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 01:11:16PM -0700, Tri Vo 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/<name>/.
> >
> > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > source statistics in sysfs.
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Tri Vo <trong@android.com>
> > Tested-by: Tri Vo <trong@android.com>
>
> Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> perhaps given that I rewrote the whole file last time?  :)

Thanks again for the help!
>
>
> > ---
> >  Documentation/ABI/testing/sysfs-power |  73 ++++++++++++-
> >  drivers/base/power/Makefile           |   1 +
> >  drivers/base/power/wakeup.c           |  12 ++-
> >  drivers/base/power/wakeup_stats.c     | 148 ++++++++++++++++++++++++++
> >  include/linux/pm_wakeup.h             |  19 ++++
> >  kernel/power/Kconfig                  |  10 ++
> >  kernel/power/wakelock.c               |  10 ++
> >  7 files changed, 270 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/base/power/wakeup_stats.c
>
> What changed from v2?  :)

Oops, my bad.

> And I am guessing that you actually tested this all out, and it works
> for you?

Yes, I played around with wakelocks to make sure that wakeup source
stats are added/updated/removed as expected.

> Have you changed Android userspace to use the new api with no
> problems?

Kalesh helped me test this patch (added him in Tested-by: field in
latest patch version). We haven't tested beyond booting and manual
inspection on android devices. Android userspace changes should be
fairly trivial though.

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

[Rafael J. Wysocki]

On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
>
> Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> source statistics in sysfs.

I'm not sure if this is really needed, but I'll let Greg decide.

Apart from this

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

>
> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Tri Vo <trong@android.com>
> Tested-by: Tri Vo <trong@android.com>
> Tested-by: Kalesh Singh <kaleshsingh@google.com>
> ---
>  Documentation/ABI/testing/sysfs-class-wakeup |  70 +++++++++
>  drivers/base/power/Makefile                  |   1 +
>  drivers/base/power/wakeup.c                  |  12 +-
>  drivers/base/power/wakeup_stats.c            | 149 +++++++++++++++++++
>  include/linux/pm_wakeup.h                    |  19 +++
>  kernel/power/Kconfig                         |  10 ++
>  kernel/power/wakelock.c                      |  10 ++
>  7 files changed, 269 insertions(+), 2 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.
>
> diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup
> new file mode 100644
> index 000000000000..30fb23eb9112
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-wakeup
> @@ -0,0 +1,70 @@
> +What:          /sys/class/wakeup/
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               The /sys/class/wakeup/ directory contains pointers to all
> +               wakeup sources in the kernel at that moment in time.
> +
> +What:          /sys/class/wakeup/.../active_count
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the number of times the wakeup source was
> +               activated.
> +
> +What:          /sys/class/wakeup/.../event_count
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the number of signaled wakeup events
> +               associated with the wakeup source.
> +
> +What:          /sys/class/wakeup/.../wakeup_count
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the number of times the wakeup source might
> +               abort suspend.
> +
> +What:          /sys/class/wakeup/.../expire_count
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the number of times the wakeup source's
> +               timeout has expired.
> +
> +What:          /sys/class/wakeup/.../active_time_ms
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the amount of time the wakeup source has
> +               been continuously active, in milliseconds.  If the wakeup
> +               source is not active, this file contains '0'.
> +
> +What:          /sys/class/wakeup/.../total_time_ms
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the total amount of time this wakeup source
> +               has been active, in milliseconds.
> +
> +What:          /sys/class/wakeup/.../max_time_ms
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the maximum amount of time this wakeup
> +               source has been continuously active, in milliseconds.
> +
> +What:          /sys/class/wakeup/.../last_change_ms
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               This file contains the monotonic clock time when the wakeup
> +               source was touched last time, in milliseconds.
> +
> +What:          /sys/class/wakeup/.../prevent_suspend_time_ms
> +Date:          June 2019
> +Contact:       Tri Vo <trong@android.com>
> +Description:
> +               The file contains the total amount of time this wakeup source
> +               has been preventing autosleep, in milliseconds.
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index e1bb691cf8f1..1963f53d9982 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_PM)       += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
>  obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> +obj-$(CONFIG_PM_SLEEP_STATS)   += wakeup_stats.o
>  obj-$(CONFIG_PM_TRACE_RTC)     += trace.o
>  obj-$(CONFIG_PM_GENERIC_DOMAINS)       +=  domain.o domain_governor.o
>  obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 5b2b6a05a4f3..fd48e78c06b9 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -205,11 +205,18 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
>  struct wakeup_source *wakeup_source_register(const char *name)
>  {
>         struct wakeup_source *ws;
> +       int ret;
>
>         ws = wakeup_source_create(name);
> -       if (ws)
> +       if (ws) {
> +               ret = wakeup_source_sysfs_add(ws);
> +               if (ret) {
> +                       kfree_const(ws->name);
> +                       kfree(ws);
> +                       return NULL;
> +               }
>                 wakeup_source_add(ws);
> -
> +       }
>         return ws;
>  }
>  EXPORT_SYMBOL_GPL(wakeup_source_register);
> @@ -222,6 +229,7 @@ void wakeup_source_unregister(struct wakeup_source *ws)
>  {
>         if (ws) {
>                 wakeup_source_remove(ws);
> +               wakeup_source_sysfs_remove(ws);
>                 wakeup_source_destroy(ws);
>         }
>  }
> diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
> new file mode 100644
> index 000000000000..45e15856105b
> --- /dev/null
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wakeup statistics in sysfs
> + *
> + * Copyright (c) 2019 Linux Foundation
> + * Copyright (c) 2019 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> + * Copyright (c) 2019 Google Inc.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kdev_t.h>
> +
> +#include "power.h"
> +
> +static struct class *wakeup_class;
> +
> +#define wakeup_attr(_name)                                             \
> +static ssize_t _name##_show(struct device *dev,                                \
> +                           struct device_attribute *attr, char *buf)   \
> +{                                                                      \
> +       struct wakeup_source *ws = dev_get_drvdata(dev);                \
> +                                                                       \
> +       return sprintf(buf, "%lu\n", ws->_name);                        \
> +}                                                                      \
> +static DEVICE_ATTR_RO(_name)
> +
> +wakeup_attr(active_count);
> +wakeup_attr(event_count);
> +wakeup_attr(wakeup_count);
> +wakeup_attr(expire_count);
> +
> +static ssize_t active_time_ms_show(struct device *dev,
> +                                  struct device_attribute *attr, char *buf)
> +{
> +       struct wakeup_source *ws = dev_get_drvdata(dev);
> +       ktime_t active_time =
> +               ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
> +
> +       return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
> +}
> +static DEVICE_ATTR_RO(active_time_ms);
> +
> +static ssize_t total_time_ms_show(struct device *dev,
> +                                 struct device_attribute *attr, char *buf)
> +{
> +       struct wakeup_source *ws = dev_get_drvdata(dev);
> +       ktime_t active_time;
> +       ktime_t total_time = ws->total_time;
> +
> +       if (ws->active) {
> +               active_time = ktime_sub(ktime_get(), ws->last_time);
> +               total_time = ktime_add(total_time, active_time);
> +       }
> +       return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
> +}
> +static DEVICE_ATTR_RO(total_time_ms);
> +
> +static ssize_t max_time_ms_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct wakeup_source *ws = dev_get_drvdata(dev);
> +       ktime_t active_time;
> +       ktime_t max_time = ws->max_time;
> +
> +       if (ws->active) {
> +               active_time = ktime_sub(ktime_get(), ws->last_time);
> +               if (active_time > max_time)
> +                       max_time = active_time;
> +       }
> +       return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
> +}
> +static DEVICE_ATTR_RO(max_time_ms);
> +
> +static ssize_t last_change_ms_show(struct device *dev,
> +                                  struct device_attribute *attr, char *buf)
> +{
> +       struct wakeup_source *ws = dev_get_drvdata(dev);
> +
> +       return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
> +}
> +static DEVICE_ATTR_RO(last_change_ms);
> +
> +static ssize_t prevent_suspend_time_ms_show(struct device *dev,
> +                                           struct device_attribute *attr,
> +                                           char *buf)
> +{
> +       struct wakeup_source *ws = dev_get_drvdata(dev);
> +       ktime_t prevent_sleep_time = ws->prevent_sleep_time;
> +
> +       if (ws->active && ws->autosleep_enabled) {
> +               prevent_sleep_time = ktime_add(prevent_sleep_time,
> +                       ktime_sub(ktime_get(), ws->start_prevent_time));
> +       }
> +       return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
> +}
> +static DEVICE_ATTR_RO(prevent_suspend_time_ms);
> +
> +static struct attribute *wakeup_source_attrs[] = {
> +       &dev_attr_active_count.attr,
> +       &dev_attr_event_count.attr,
> +       &dev_attr_wakeup_count.attr,
> +       &dev_attr_expire_count.attr,
> +       &dev_attr_active_time_ms.attr,
> +       &dev_attr_total_time_ms.attr,
> +       &dev_attr_max_time_ms.attr,
> +       &dev_attr_last_change_ms.attr,
> +       &dev_attr_prevent_suspend_time_ms.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(wakeup_source);
> +
> +/**
> + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> + * @ws: Wakeup source to be added in sysfs.
> + */
> +int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +{
> +       struct device *dev;
> +
> +       dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
> +                                       wakeup_source_groups, "%s", ws->name);
> +       if (IS_ERR(dev))
> +               return PTR_ERR(dev);
> +
> +       ws->dev = dev;
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> +
> +/**
> + * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
> + * @ws: Wakeup source to be removed from sysfs.
> + */
> +void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> +{
> +       device_unregister(ws->dev);
> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
> +
> +static int __init wakeup_sources_sysfs_init(void)
> +{
> +       wakeup_class = class_create(THIS_MODULE, "wakeup");
> +       if (IS_ERR(wakeup_class))
> +               return PTR_ERR(wakeup_class);
> +
> +       return 0;
> +}
> +
> +postcore_initcall(wakeup_sources_sysfs_init);
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index ce57771fab9b..734141869c17 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -35,6 +35,7 @@ struct wake_irq;
>   * @relax_count: Number of times the wakeup source was deactivated.
>   * @expire_count: Number of times the wakeup source's timeout has expired.
>   * @wakeup_count: Number of times the wakeup source might abort suspend.
> + * @dev: Struct device for sysfs statistics about the wakeup source.
>   * @active: Status of the wakeup source.
>   * @has_timeout: The wakeup source has been activated with a timeout.
>   */
> @@ -55,6 +56,7 @@ struct wakeup_source {
>         unsigned long           relax_count;
>         unsigned long           expire_count;
>         unsigned long           wakeup_count;
> +       struct device           *dev;
>         bool                    active:1;
>         bool                    autosleep_enabled:1;
>  };
> @@ -181,6 +183,23 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
>
>  #endif /* !CONFIG_PM_SLEEP */
>
> +#ifdef CONFIG_PM_SLEEP_STATS
> +
> +/* drivers/base/power/wakeup_stats.c */
> +int wakeup_source_sysfs_add(struct wakeup_source *ws);
> +void wakeup_source_sysfs_remove(struct wakeup_source *ws);
> +
> +#else /* !CONFIG_PM_SLEEP_STATS */
> +
> +static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +{
> +       return 0;
> +}
> +static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> +{ }
> +
> +#endif /* !CONFIG_PM_SLEEP_STATS */
> +
>  static inline void wakeup_source_init(struct wakeup_source *ws,
>                                       const char *name)
>  {
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index ff8592ddedee..604e1f087f14 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
>         def_bool y
>         depends on PM_DEBUG && PM_SLEEP
>
> +config PM_SLEEP_STATS
> +       bool "Wakeup sources statistics"
> +       depends on PM_SLEEP
> +       help
> +         Expose wakeup sources statistics to user space via sysfs. Collected
> +         statistics are located under /sys/power/wakeup_sources. For more
> +         information, read <file:Documentation/ABI/testing/sysfs-class-wakeup>.
> +
> +         If in doubt, say N.
> +
>  config DPM_WATCHDOG
>         bool "Device suspend/resume watchdog"
>         depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 4210152e56f0..32726da3d6e6 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -122,6 +122,7 @@ static void __wakelocks_gc(struct work_struct *work)
>
>                 if (!active) {
>                         wakeup_source_remove(&wl->ws);
> +                       wakeup_source_sysfs_remove(&wl->ws);
>                         rb_erase(&wl->node, &wakelocks_tree);
>                         list_del(&wl->lru);
>                         kfree(wl->name);
> @@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>         struct rb_node **node = &wakelocks_tree.rb_node;
>         struct rb_node *parent = *node;
>         struct wakelock *wl;
> +       int ret;
>
>         while (*node) {
>                 int diff;
> @@ -189,6 +191,14 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
>         }
>         wl->ws.name = wl->name;
>         wl->ws.last_time = ktime_get();
> +
> +       ret = wakeup_source_sysfs_add(&wl->ws);
> +       if (ret) {
> +               kfree(wl->name);
> +               kfree(wl);
> +               return ERR_PTR(ret);
> +       }
> +
>         wakeup_source_add(&wl->ws);
>         rb_link_node(&wl->node, parent, node);
>         rb_insert_color(&wl->node, &wakelocks_tree);
> --
> 2.22.0.510.g264f2c817a-goog
>

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

[Greg Kroah-Hartman]

On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
> >
> > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > source statistics in sysfs.
> 
> I'm not sure if this is really needed, but I'll let Greg decide.

You are right.  Having zillions of config options is a pain, who is
going to turn this off?

But we can always remove the option before 5.4-rc1, so I'll take this
as-is for now :)

> Apart from this
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

thanks for the review!  I'll wait for 5.3-rc1 to come out before adding
this to my tree.

greg k-h

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

[Greg KH]

On Mon, Jul 15, 2019 at 02:48:13PM -0700, Tri Vo wrote:
> > And I am guessing that you actually tested this all out, and it works
> > for you?
> 
> Yes, I played around with wakelocks to make sure that wakeup source
> stats are added/updated/removed as expected.

Great!

> > Have you changed Android userspace to use the new api with no
> > problems?
> 
> Kalesh helped me test this patch (added him in Tested-by: field in
> latest patch version). We haven't tested beyond booting and manual
> inspection on android devices. Android userspace changes should be
> fairly trivial though.

Ok, I know some people wanted to do some premature optimization with the
original discussion about this, glad to see it's not needed...

thanks,

greg k-h

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

[Tri Vo]

On Tue, Jul 16, 2019 at 11:13 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> >
> > I'm not sure if this is really needed, but I'll let Greg decide.
>
> You are right.  Having zillions of config options is a pain, who is
> going to turn this off?
>
> But we can always remove the option before 5.4-rc1, so I'll take this
> as-is for now :)
>
> > Apart from this
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> thanks for the review!  I'll wait for 5.3-rc1 to come out before adding
> this to my tree.

Greg, Rafael, thanks for taking the time to review this patch!

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

[Rafael J. Wysocki]

On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> >
> > I'm not sure if this is really needed, but I'll let Greg decide.
>
> You are right.  Having zillions of config options is a pain, who is
> going to turn this off?
>
> But we can always remove the option before 5.4-rc1, so I'll take this
> as-is for now :)
>
> > Apart from this
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> thanks for the review!  I'll wait for 5.3-rc1 to come out before adding
> this to my tree.

So it occurred to me that maybe it's better if I apply it?  After all,
this is PM material. :-)

It is fine by me either way, but then I'm not sure if you want to get
future bug reports related to this if any ...

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

[Greg Kroah-Hartman]

On Tue, Jul 16, 2019 at 10:30:48AM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
> > > >
> > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > > source statistics in sysfs.
> > >
> > > I'm not sure if this is really needed, but I'll let Greg decide.
> >
> > You are right.  Having zillions of config options is a pain, who is
> > going to turn this off?
> >
> > But we can always remove the option before 5.4-rc1, so I'll take this
> > as-is for now :)
> >
> > > Apart from this
> > >
> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > thanks for the review!  I'll wait for 5.3-rc1 to come out before adding
> > this to my tree.
> 
> So it occurred to me that maybe it's better if I apply it?  After all,
> this is PM material. :-)

Heh, true, feel free to add:
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
to the patch then.

> It is fine by me either way, but then I'm not sure if you want to get
> future bug reports related to this if any ...

I get enough emails as it is, no need to ask for more :)

thanks,

greg k-h

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

[Rafael J. Wysocki]

On Tue, Jul 16, 2019 at 10:39 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 16, 2019 at 10:30:48AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Jul 15, 2019 at 11:44 PM 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/<name>/.
> > > > >
> > > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > > > source statistics in sysfs.
> > > >
> > > > I'm not sure if this is really needed, but I'll let Greg decide.
> > >
> > > You are right.  Having zillions of config options is a pain, who is
> > > going to turn this off?
> > >
> > > But we can always remove the option before 5.4-rc1, so I'll take this
> > > as-is for now :)
> > >
> > > > Apart from this
> > > >
> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > thanks for the review!  I'll wait for 5.3-rc1 to come out before adding
> > > this to my tree.
> >
> > So it occurred to me that maybe it's better if I apply it?  After all,
> > this is PM material. :-)
>
> Heh, true, feel free to add:
>         Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> to the patch then.

I will, thank you!