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

[PATCH v6] 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/ws<ID>/*.

Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Tri Vo <trong@android.com>
Tested-by: Tri Vo <trong@android.com>
Tested-by: Kalesh Singh <kaleshsingh@google.com>
---
 Documentation/ABI/testing/sysfs-class-wakeup |  76 +++++++++
 drivers/acpi/device_pm.c                     |   3 +-
 drivers/base/power/Makefile                  |   2 +-
 drivers/base/power/wakeup.c                  |  18 +-
 drivers/base/power/wakeup_stats.c            | 171 +++++++++++++++++++
 fs/eventpoll.c                               |   4 +-
 include/linux/pm_wakeup.h                    |  15 +-
 kernel/power/autosleep.c                     |   2 +-
 kernel/power/wakelock.c                      |  10 ++
 kernel/time/alarmtimer.c                     |   2 +-
 10 files changed, 291 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
 create mode 100644 drivers/base/power/wakeup_stats.c

v2:
- Updated Documentation/ABI/, as per Greg.
- Removed locks in attribute functions, as per Greg.
- Lifetimes of struct wakelock and struck wakeup_source are now different due to
  the latter embedding a refcounted kobject. Changed it so that struct wakelock
  only has a pointer to struct wakeup_source, instead of embedding it.
- Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
  sysfs.

v3:
Changes by Greg:
- Reworked code to use 'struct device' instead of raw kobjects.
- Updated documentation file.
- Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
Changes by Tri:
- Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
  operations. So no need to handle lifetimes in wakelock.c

v4:
- Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
- Moved new documentation to a separate file
  Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
- Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.

v5:
- Removed CONFIG_PM_SLEEP_STATS
- Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by
  kbuild test robot <lkp@intel.com>
- Stephen reported that a call to device_init_wakeup() and writing 'enabled' to
  that device's power/wakeup file results in multiple wakeup source being
  allocated for that device.  Changed device_wakeup_enable() to check if device
  wakeup was previously enabled.
Changes by Stephen:
- Changed stats location from /sys/class/wakeup/<name>/* to
  /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This
  avoids name collisions in /sys/class/wakeup/ directory.
- Added a "name" attribute to wakeup sources, and updated documentation.
- Device registering the wakeup source is now the parent of the wakeup source.
  Updated wakeup_source_register()'s signature and its callers accordingly.

v6:
- Changed stats location to /sys/class/wakeup/ws<ID>/*
- Replaced ida_simple_get()/ida_simple_remove() with ida_alloc()/ida_free() as
  the former is deprecated.
- Reverted changes to device_init_wakeup(). Rafael is preparing a patch to deal
  with extra wakeup source allocation in a separate patch.

diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup
new file mode 100644
index 000000000000..754aab8b6dcd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-wakeup
@@ -0,0 +1,76 @@
+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/.../name
+Date:		June 2019
+Contact:	Tri Vo <trong@android.com>
+Description:
+		This file contains the name of the wakeup source.
+
+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/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 28cffaaf9d82..91634e2dba8a 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -495,7 +495,8 @@ acpi_status acpi_add_pm_notifier(struct acpi_device *adev, struct device *dev,
 		goto out;

 	mutex_lock(&acpi_pm_notifier_lock);
-	adev->wakeup.ws = wakeup_source_register(dev_name(&adev->dev));
+	adev->wakeup.ws = wakeup_source_register(&adev->dev,
+						 dev_name(&adev->dev));
 	adev->wakeup.context.dev = dev;
 	adev->wakeup.context.func = func;
 	adev->wakeup.flags.notifier_present = true;
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/wakeup.c b/drivers/base/power/wakeup.c
index ee31d4f8d856..79668b45eae6 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -200,16 +200,25 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);

 /**
  * wakeup_source_register - Create wakeup source and add it to the list.
+ * @dev: Device this wakeup source is associated with (or NULL if virtual).
  * @name: Name of the wakeup source to register.
  */
-struct wakeup_source *wakeup_source_register(const char *name)
+struct wakeup_source *wakeup_source_register(struct device *dev,
+					     const char *name)
 {
 	struct wakeup_source *ws;
+	int ret;

 	ws = wakeup_source_create(name);
-	if (ws)
+	if (ws) {
+		ret = wakeup_source_sysfs_add(dev, 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 +231,7 @@ void wakeup_source_unregister(struct wakeup_source *ws)
 {
 	if (ws) {
 		wakeup_source_remove(ws);
+		wakeup_source_sysfs_remove(ws);
 		wakeup_source_destroy(ws);
 	}
 }
@@ -265,7 +275,7 @@ int device_wakeup_enable(struct device *dev)
 	if (pm_suspend_target_state != PM_SUSPEND_ON)
 		dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);

-	ws = wakeup_source_register(dev_name(dev));
+	ws = wakeup_source_register(dev, dev_name(dev));
 	if (!ws)
 		return -ENOMEM;

diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
new file mode 100644
index 000000000000..a26f019faca9
--- /dev/null
+++ b/drivers/base/power/wakeup_stats.c
@@ -0,0 +1,171 @@
+// 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/idr.h>
+#include <linux/kdev_t.h>
+#include <linux/slab.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 name_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct wakeup_source *ws = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", ws->name);
+}
+static DEVICE_ATTR_RO(name);
+
+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_name.attr,
+	&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);
+
+static DEFINE_IDA(wakeup_ida);
+
+/**
+ * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
+ * @parent: Device given wakeup source is associated with (or NULL if virtual).
+ * @ws: Wakeup source to be added in sysfs.
+ */
+int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
+{
+	struct device *dev;
+	int id;
+
+	id = ida_alloc(&wakeup_ida, GFP_KERNEL);
+	if (id < 0)
+		return id;
+	ws->id = id;
+
+	dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
+					wakeup_source_groups, "ws%d",
+					ws->id);
+	if (IS_ERR(dev)) {
+		ida_free(&wakeup_ida, ws->id);
+		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);
+	ida_simple_remove(&wakeup_ida, ws->id);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
+
+static int __init wakeup_sources_sysfs_init(void)
+{
+	wakeup_class = class_create(THIS_MODULE, "wakeup");
+
+	return PTR_ERR_OR_ZERO(wakeup_class);
+}
+
+postcore_initcall(wakeup_sources_sysfs_init);
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index d7f1f5011fac..c4159bcc05d9 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1459,13 +1459,13 @@ static int ep_create_wakeup_source(struct epitem *epi)
 	struct wakeup_source *ws;

 	if (!epi->ep->ws) {
-		epi->ep->ws = wakeup_source_register("eventpoll");
+		epi->ep->ws = wakeup_source_register(NULL, "eventpoll");
 		if (!epi->ep->ws)
 			return -ENOMEM;
 	}

 	name = epi->ffd.file->f_path.dentry->d_name.name;
-	ws = wakeup_source_register(name);
+	ws = wakeup_source_register(NULL, name);

 	if (!ws)
 		return -ENOMEM;
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 91027602d137..f39f768389c8 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -21,6 +21,7 @@ struct wake_irq;
  * struct wakeup_source - Representation of wakeup sources
  *
  * @name: Name of the wakeup source
+ * @id: Wakeup source id
  * @entry: Wakeup source list entry
  * @lock: Wakeup source lock
  * @wakeirq: Optional device specific wakeirq
@@ -35,11 +36,13 @@ 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.
  * @autosleep_enabled: Autosleep is active, so update @prevent_sleep_time.
  */
 struct wakeup_source {
 	const char 		*name;
+	int			id;
 	struct list_head	entry;
 	spinlock_t		lock;
 	struct wake_irq		*wakeirq;
@@ -55,6 +58,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;
 };
@@ -86,7 +90,8 @@ extern struct wakeup_source *wakeup_source_create(const char *name);
 extern void wakeup_source_destroy(struct wakeup_source *ws);
 extern void wakeup_source_add(struct wakeup_source *ws);
 extern void wakeup_source_remove(struct wakeup_source *ws);
-extern struct wakeup_source *wakeup_source_register(const char *name);
+extern struct wakeup_source *wakeup_source_register(struct device *dev,
+						    const char *name);
 extern void wakeup_source_unregister(struct wakeup_source *ws);
 extern int device_wakeup_enable(struct device *dev);
 extern int device_wakeup_disable(struct device *dev);
@@ -100,6 +105,11 @@ 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_stats.c */
+extern int wakeup_source_sysfs_add(struct device *parent,
+				   struct wakeup_source *ws);
+extern void wakeup_source_sysfs_remove(struct wakeup_source *ws);
+
 #else /* !CONFIG_PM_SLEEP */

 static inline void device_set_wakeup_capable(struct device *dev, bool capable)
@@ -126,7 +136,8 @@ static inline void wakeup_source_add(struct wakeup_source *ws) {}

 static inline void wakeup_source_remove(struct wakeup_source *ws) {}

-static inline struct wakeup_source *wakeup_source_register(const char *name)
+static inline struct wakeup_source *wakeup_source_register(struct device *dev,
+							   const char *name)
 {
 	return NULL;
 }
diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index 41e83a779e19..9af5a50d3489 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -116,7 +116,7 @@ int pm_autosleep_set_state(suspend_state_t state)

 int __init pm_autosleep_init(void)
 {
-	autosleep_ws = wakeup_source_register("autosleep");
+	autosleep_ws = wakeup_source_register(NULL, "autosleep");
 	if (!autosleep_ws)
 		return -ENOMEM;

diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 4210152e56f0..826fcd97647a 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(NULL, &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);
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 57518efc3810..93b382d9701c 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -97,7 +97,7 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	if (!device_may_wakeup(rtc->dev.parent))
 		return -1;

-	__ws = wakeup_source_register("alarmtimer");
+	__ws = wakeup_source_register(dev, "alarmtimer");

 	spin_lock_irqsave(&rtcdev_lock, flags);
 	if (!rtcdev) {
--
2.22.0.709.g102302147b-goog

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

[Stephen Boyd]

Quoting Tri Vo (2019-07-31 14:55:14)
> +/**
> + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> + * @ws: Wakeup source to be added in sysfs.
> + */
> +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> +{
> +       struct device *dev;
> +       int id;
> +
> +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> +       if (id < 0)
> +               return id;
> +       ws->id = id;
> +
> +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> +                                       wakeup_source_groups, "ws%d",

I thought the name was going to still be 'wakeupN'?

> +                                       ws->id);
> +       if (IS_ERR(dev)) {
> +               ida_free(&wakeup_ida, ws->id);
> +               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);
> +       ida_simple_remove(&wakeup_ida, ws->id);

Should be ida_free()?

> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
> +
> +static int __init wakeup_sources_sysfs_init(void)
> +{
> +       wakeup_class = class_create(THIS_MODULE, "wakeup");
> +
> +       return PTR_ERR_OR_ZERO(wakeup_class);
> +}
> +
> +postcore_initcall(wakeup_sources_sysfs_init);

Style nitpick: Stick the initcall to the function it calls by dropping
the extra newline between them.

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

[Tri Vo]

On Wed, Jul 31, 2019 at 2:59 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tri Vo (2019-07-31 14:55:14)
> > +/**
> > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > + * @ws: Wakeup source to be added in sysfs.
> > + */
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > +{
> > +       struct device *dev;
> > +       int id;
> > +
> > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> > +       if (id < 0)
> > +               return id;
> > +       ws->id = id;
> > +
> > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > +                                       wakeup_source_groups, "ws%d",
>
> I thought the name was going to still be 'wakeupN'?

I don't really have an opinion on this. Rafael seems to prefer "ws",
and he's the maintainer :)
>
> > +                                       ws->id);
> > +       if (IS_ERR(dev)) {
> > +               ida_free(&wakeup_ida, ws->id);
> > +               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);
> > +       ida_simple_remove(&wakeup_ida, ws->id);
>
> Should be ida_free()?

oops
>
> > +}
> > +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
> > +
> > +static int __init wakeup_sources_sysfs_init(void)
> > +{
> > +       wakeup_class = class_create(THIS_MODULE, "wakeup");
> > +
> > +       return PTR_ERR_OR_ZERO(wakeup_class);
> > +}
> > +
> > +postcore_initcall(wakeup_sources_sysfs_init);
>
> Style nitpick: Stick the initcall to the function it calls by dropping
> the extra newline between them.

will do

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

[Rafael J. Wysocki]

On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> Quoting Tri Vo (2019-07-31 14:55:14)
> > +/**
> > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > + * @ws: Wakeup source to be added in sysfs.
> > + */
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > +{
> > +       struct device *dev;
> > +       int id;
> > +
> > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);

So can anyone remind me why the IDA thing is needed here at all?

> > +       if (id < 0)
> > +               return id;
> > +       ws->id = id;
> > +
> > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > +                                       wakeup_source_groups, "ws%d",
> 
> I thought the name was going to still be 'wakeupN'?

So can't we prefix the wakeup source name with something like "wakeup:" or similar here?

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

[Tri Vo]

On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > Quoting Tri Vo (2019-07-31 14:55:14)
> > > +/**
> > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > + * @ws: Wakeup source to be added in sysfs.
> > > + */
> > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > +{
> > > +       struct device *dev;
> > > +       int id;
> > > +
> > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
>
> So can anyone remind me why the IDA thing is needed here at all?

IDA is used to generate the directory name ("ws%d", ID) that is unique
among wakeup_sources. That is what ends up in
/sys/class/wakeup/ws<ID>/* path.
>
> > > +       if (id < 0)
> > > +               return id;
> > > +       ws->id = id;
> > > +
> > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > +                                       wakeup_source_groups, "ws%d",
> >
> > I thought the name was going to still be 'wakeupN'?
>
> So can't we prefix the wakeup source name with something like "wakeup:" or similar here?

"ws%d" here is the name in the sysfs path rather than the name of the
wakeup source. Wakeup source name is not altered in this patch.

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

[Rafael J. Wysocki]

On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > +/**
> > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > > + * @ws: Wakeup source to be added in sysfs.
> > > > + */
> > > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > > +{
> > > > +       struct device *dev;
> > > > +       int id;
> > > > +
> > > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> >
> > So can anyone remind me why the IDA thing is needed here at all?
> 
> IDA is used to generate the directory name ("ws%d", ID) that is unique
> among wakeup_sources. That is what ends up in
> /sys/class/wakeup/ws<ID>/* path.

That's not my point (see below).

> > > > +       if (id < 0)
> > > > +               return id;
> > > > +       ws->id = id;
> > > > +
> > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > +                                       wakeup_source_groups, "ws%d",
> > >
> > > I thought the name was going to still be 'wakeupN'?
> >
> > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> 
> "ws%d" here is the name in the sysfs path rather than the name of the
> wakeup source. Wakeup source name is not altered in this patch.
> 

So why wouldn't something like this suffice:

dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
                                wakeup_source_groups, "wakeup:%s", ws->name);

?

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

[Tri Vo]

On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > +/**
> > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > + */
> > > > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > > > +{
> > > > > +       struct device *dev;
> > > > > +       int id;
> > > > > +
> > > > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> > >
> > > So can anyone remind me why the IDA thing is needed here at all?
> >
> > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > among wakeup_sources. That is what ends up in
> > /sys/class/wakeup/ws<ID>/* path.
>
> That's not my point (see below).
>
> > > > > +       if (id < 0)
> > > > > +               return id;
> > > > > +       ws->id = id;
> > > > > +
> > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > +                                       wakeup_source_groups, "ws%d",
> > > >
> > > > I thought the name was going to still be 'wakeupN'?
> > >
> > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> >
> > "ws%d" here is the name in the sysfs path rather than the name of the
> > wakeup source. Wakeup source name is not altered in this patch.
> >
>
> So why wouldn't something like this suffice:
>
> dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
>                                 wakeup_source_groups, "wakeup:%s", ws->name);
>
> ?

ws->name is inherited from the device name. IIUC device names are not
guaranteed to be unique. So if different devices with the same name
register wakeup sources, there is an error.

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

[Rafael J. Wysocki]

On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
>
> On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > > +/**
> > > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > > + */
> > > > > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > > > > +{
> > > > > > +       struct device *dev;
> > > > > > +       int id;
> > > > > > +
> > > > > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> > > >
> > > > So can anyone remind me why the IDA thing is needed here at all?
> > >
> > > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > > among wakeup_sources. That is what ends up in
> > > /sys/class/wakeup/ws<ID>/* path.
> >
> > That's not my point (see below).
> >
> > > > > > +       if (id < 0)
> > > > > > +               return id;
> > > > > > +       ws->id = id;
> > > > > > +
> > > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > +                                       wakeup_source_groups, "ws%d",
> > > > >
> > > > > I thought the name was going to still be 'wakeupN'?
> > > >
> > > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> > >
> > > "ws%d" here is the name in the sysfs path rather than the name of the
> > > wakeup source. Wakeup source name is not altered in this patch.
> > >
> >
> > So why wouldn't something like this suffice:
> >
> > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> >
> > ?
>
> ws->name is inherited from the device name. IIUC device names are not
> guaranteed to be unique. So if different devices with the same name
> register wakeup sources, there is an error.

OK

So I guess the names are retained for backwards compatibility with
existing user space that may be using them?

That's kind of fair enough, but having two different identification
schemes for wakeup sources will end up confusing.

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

[Tri Vo]

On Wed, Jul 31, 2019 at 4:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > > > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > > > +/**
> > > > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > > > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > > > + */
> > > > > > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > > > > > +{
> > > > > > > +       struct device *dev;
> > > > > > > +       int id;
> > > > > > > +
> > > > > > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> > > > >
> > > > > So can anyone remind me why the IDA thing is needed here at all?
> > > >
> > > > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > > > among wakeup_sources. That is what ends up in
> > > > /sys/class/wakeup/ws<ID>/* path.
> > >
> > > That's not my point (see below).
> > >
> > > > > > > +       if (id < 0)
> > > > > > > +               return id;
> > > > > > > +       ws->id = id;
> > > > > > > +
> > > > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > > +                                       wakeup_source_groups, "ws%d",
> > > > > >
> > > > > > I thought the name was going to still be 'wakeupN'?
> > > > >
> > > > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> > > >
> > > > "ws%d" here is the name in the sysfs path rather than the name of the
> > > > wakeup source. Wakeup source name is not altered in this patch.
> > > >
> > >
> > > So why wouldn't something like this suffice:
> > >
> > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > >
> > > ?
> >
> > ws->name is inherited from the device name. IIUC device names are not
> > guaranteed to be unique. So if different devices with the same name
> > register wakeup sources, there is an error.
>
> OK
>
> So I guess the names are retained for backwards compatibility with
> existing user space that may be using them?

Yes, in Android we do rely on the name to aggregate statistics across
a fleet of devices. That wouldn't be possible with just the id, as
those are generated at dynamically runtime.
>
> That's kind of fair enough, but having two different identification
> schemes for wakeup sources will end up confusing.

It's not without precedent though. rtc, input, and other devices have
a similar scheme.

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

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > That's not my point (see below).
> > >
> > > > > > > +       if (id < 0)
> > > > > > > +               return id;
> > > > > > > +       ws->id = id;
> > > > > > > +
> > > > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > > +                                       wakeup_source_groups, "ws%d",
> > > > > >
> > > > > > I thought the name was going to still be 'wakeupN'?
> > > > >
> > > > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> > > >
> > > > "ws%d" here is the name in the sysfs path rather than the name of the
> > > > wakeup source. Wakeup source name is not altered in this patch.
> > > >
> > >
> > > So why wouldn't something like this suffice:
> > >
> > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > >
> > > ?
> >
> > ws->name is inherited from the device name. IIUC device names are not
> > guaranteed to be unique. So if different devices with the same name
> > register wakeup sources, there is an error.
> 
> OK
> 
> So I guess the names are retained for backwards compatibility with
> existing user space that may be using them?
> 
> That's kind of fair enough, but having two different identification
> schemes for wakeup sources will end up confusing.

I understand your concern about the IDA now. Thanks for clarifying.

How about we name the devices 'wakeupN' with the IDA when they're
registered with a non-NULL device pointer and then name them whatever
the name argument is when the device pointer is NULL. If we have this,
we should be able to drop the name attribute in sysfs and figure out the
name either by looking at the device name in /sys/class/wakeup/ if it
isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
and look at the parent device name there.

The only problem I see is the alarmtimer code where it might register a
second wakeup source for the same rtc device. In this case, we probably
want to use whatever name is passed in ("alarmtimer") instead of the
IDA.

This approach also nicely detects duplicate wakeup source names in the
case that the string passed in to wakeup_source_register() is already
used on the virtual bus.

---8<----
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 79668b45eae6..1c98f83c576e 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 /**
  * wakeup_source_register - Create wakeup source and add it to the list.
  * @dev: Device this wakeup source is associated with (or NULL if virtual).
- * @name: Name of the wakeup source to register.
+ * @name: Name of the wakeup source to register (or NULL if device wakeup).
  */
 struct wakeup_source *wakeup_source_register(struct device *dev,
 					     const char *name)
@@ -209,6 +209,9 @@ struct wakeup_source *wakeup_source_register(struct device *dev,
 	struct wakeup_source *ws;
 	int ret;
 
+	if (!name)
+		name = dev_name(dev);
+
 	ws = wakeup_source_create(name);
 	if (ws) {
 		ret = wakeup_source_sysfs_add(dev, ws);
@@ -275,7 +278,7 @@ int device_wakeup_enable(struct device *dev)
 	if (pm_suspend_target_state != PM_SUSPEND_ON)
 		dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
 
-	ws = wakeup_source_register(dev, dev_name(dev));
+	ws = wakeup_source_register(dev, NULL);
 	if (!ws)
 		return -ENOMEM;
 
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
index a26f019faca9..11e2906dca4c 100644
--- a/drivers/base/power/wakeup_stats.c
+++ b/drivers/base/power/wakeup_stats.c
@@ -132,16 +132,22 @@ int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
 	struct device *dev;
 	int id;
 
-	id = ida_alloc(&wakeup_ida, GFP_KERNEL);
-	if (id < 0)
-		return id;
-	ws->id = id;
+	if (parent) {
+		id = ida_alloc(&wakeup_ida, GFP_KERNEL);
+		if (id < 0)
+			return id;
+		ws->id = id;
+	} else {
+		ws->id = -1;
+	}
 
 	dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
-					wakeup_source_groups, "ws%d",
-					ws->id);
+					wakeup_source_groups,
+					ws->id >= 0 ? "wakeup%d" : "%s",
+					ws->id >= 0 ? ws->id : ws->name);
 	if (IS_ERR(dev)) {
-		ida_free(&wakeup_ida, ws->id);
+		if (ws->id >= 0)
+			ida_free(&wakeup_ida, ws->id);
 		return PTR_ERR(dev);
 	}
 

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

[Stephen Boyd]

Quoting Stephen Boyd (2019-07-31 16:45:31)
> 
> This approach also nicely detects duplicate wakeup source names in the
> case that the string passed in to wakeup_source_register() is already
> used on the virtual bus.

This was clearly untested! Here's a better one. This is what I see on my
device with this patch squashed in:

localhost ~ # cat /sys/kernel/debug/wakeup_sources 
name            active_count    event_count     wakeup_count    expire_count    active_since    total_time      max_time        last_change  prevent_suspend_time
1-1.2.4.1       0               0               0               0               0               0               0               0   0
1-1.1           0               0               0               0               0               0               0               0   0
gpio-keys       0               0               0               0               0               0               0               0   0
spi10.0         0               0               0               0               0               0               0               0   0
a88000.spi:ec@0:keyboard-controller     0               0               0               0               0               0           0
                0               0
alarmtimer      0               0               0               0               0               0               0               0   0
cros-ec-rtc.1.auto      0               0               0               0               0               0               0           0
                0
a8f8800.usb     0               0               0               0               0               0               0               0   0
a6f8800.usb     0               0               0               0               0               0               0               0   0
localhost ~ # ls -l /sys/class/wakeup/ 
total 0
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 alarmtimer -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/rtc/rtc0/alarmtimer
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup0 -> ../../devices/platform/soc/a6f8800.usb/wakeup/wakeup0
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup1 -> ../../devices/platform/soc/a8f8800.usb/wakeup/wakeup1
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup2 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/wakeup/wakeup2
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup3 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/a88000.spi:ec@0:keyboard-controller/wakeup/wakeup3
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup4 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/wakeup/wakeup4
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup5 -> ../../devices/platform/gpio-keys/wakeup/wakeup5
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup6 -> ../../devices/platform/soc/a8f8800.usb/a800000.dwc3/xhci-hcd.7.auto/usb1/1-1/1-1.1/wakeup/wakeup6
lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup7 -> ../../devices/platform/soc/a8f8800.usb/a800000.dwc3/xhci-hcd.7.auto/usb1/1-1/1-1.2/1-1.2.4/1-1.2.4.1/wakeup/wakeup7

----8<----
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index 79668b45eae6..ec414f0db0b1 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -201,7 +201,7 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
 /**
  * wakeup_source_register - Create wakeup source and add it to the list.
  * @dev: Device this wakeup source is associated with (or NULL if virtual).
- * @name: Name of the wakeup source to register.
+ * @name: Name of the wakeup source to register (or NULL if device wakeup).
  */
 struct wakeup_source *wakeup_source_register(struct device *dev,
 					     const char *name)
@@ -209,9 +209,9 @@ struct wakeup_source *wakeup_source_register(struct device *dev,
 	struct wakeup_source *ws;
 	int ret;
 
-	ws = wakeup_source_create(name);
+	ws = wakeup_source_create(name ? : dev_name(dev));
 	if (ws) {
-		ret = wakeup_source_sysfs_add(dev, ws);
+		ret = wakeup_source_sysfs_add(dev, ws, !!name);
 		if (ret) {
 			kfree_const(ws->name);
 			kfree(ws);
@@ -275,7 +275,7 @@ int device_wakeup_enable(struct device *dev)
 	if (pm_suspend_target_state != PM_SUSPEND_ON)
 		dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
 
-	ws = wakeup_source_register(dev, dev_name(dev));
+	ws = wakeup_source_register(dev, NULL);
 	if (!ws)
 		return -ENOMEM;
 
diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
index a26f019faca9..0f4c59b02d5d 100644
--- a/drivers/base/power/wakeup_stats.c
+++ b/drivers/base/power/wakeup_stats.c
@@ -81,15 +81,6 @@ static ssize_t last_change_ms_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(last_change_ms);
 
-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, "%s\n", ws->name);
-}
-static DEVICE_ATTR_RO(name);
-
 static ssize_t prevent_suspend_time_ms_show(struct device *dev,
 					    struct device_attribute *attr,
 					    char *buf)
@@ -106,7 +97,6 @@ static ssize_t prevent_suspend_time_ms_show(struct device *dev,
 static DEVICE_ATTR_RO(prevent_suspend_time_ms);
 
 static struct attribute *wakeup_source_attrs[] = {
-	&dev_attr_name.attr,
 	&dev_attr_active_count.attr,
 	&dev_attr_event_count.attr,
 	&dev_attr_wakeup_count.attr,
@@ -126,22 +116,35 @@ static DEFINE_IDA(wakeup_ida);
  * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
  * @parent: Device given wakeup source is associated with (or NULL if virtual).
  * @ws: Wakeup source to be added in sysfs.
+ * @use_ws_name: True to use ws->name or false to use 'wakeupN' for device name
  */
-int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
+int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws,
+			    bool use_ws_name)
 {
 	struct device *dev;
 	int id;
 
-	id = ida_alloc(&wakeup_ida, GFP_KERNEL);
-	if (id < 0)
-		return id;
-	ws->id = id;
+	ws->id = -1;
+	if (use_ws_name) {
+		dev = device_create_with_groups(wakeup_class, parent,
+						MKDEV(0, 0), ws,
+						wakeup_source_groups,
+						ws->name);
+	} else {
+		id = ida_alloc(&wakeup_ida, GFP_KERNEL);
+		if (id < 0)
+			return id;
+		ws->id = id;
+
+		dev = device_create_with_groups(wakeup_class, parent,
+						MKDEV(0, 0), ws,
+						wakeup_source_groups,
+						"wakeup%d", ws->id);
+	}
 
-	dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
-					wakeup_source_groups, "ws%d",
-					ws->id);
 	if (IS_ERR(dev)) {
-		ida_free(&wakeup_ida, ws->id);
+		if (ws->id >= 0)
+			ida_free(&wakeup_ida, ws->id);
 		return PTR_ERR(dev);
 	}
 
@@ -157,7 +160,8 @@ EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
 void wakeup_source_sysfs_remove(struct wakeup_source *ws)
 {
 	device_unregister(ws->dev);
-	ida_simple_remove(&wakeup_ida, ws->id);
+	if (ws->id >= 0)
+		ida_free(&wakeup_ida, ws->id);
 }
 EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
 
diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index f39f768389c8..c9fb00fca22e 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -107,7 +107,7 @@ extern void pm_wakeup_dev_event(struct device *dev, unsigned int msec, bool hard
 
 /* drivers/base/power/wakeup_stats.c */
 extern int wakeup_source_sysfs_add(struct device *parent,
-				   struct wakeup_source *ws);
+				   struct wakeup_source *ws, bool use_ws_name);
 extern void wakeup_source_sysfs_remove(struct wakeup_source *ws);
 
 #else /* !CONFIG_PM_SLEEP */
diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 826fcd97647a..7f2fc5f9b3b3 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -192,7 +192,7 @@ 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(NULL, &wl->ws);
+	ret = wakeup_source_sysfs_add(NULL, &wl->ws, true);
 	if (ret) {
 		kfree(wl->name);
 		kfree(wl);

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

[Rafael J. Wysocki]

On Thu, Aug 1, 2019 at 2:45 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Stephen Boyd (2019-07-31 16:45:31)
> >
> > This approach also nicely detects duplicate wakeup source names in the
> > case that the string passed in to wakeup_source_register() is already
> > used on the virtual bus.
>
> This was clearly untested! Here's a better one. This is what I see on my
> device with this patch squashed in:
>
> localhost ~ # cat /sys/kernel/debug/wakeup_sources
> name            active_count    event_count     wakeup_count    expire_count    active_since    total_time      max_time        last_change  prevent_suspend_time
> 1-1.2.4.1       0               0               0               0               0               0               0               0   0
> 1-1.1           0               0               0               0               0               0               0               0   0
> gpio-keys       0               0               0               0               0               0               0               0   0
> spi10.0         0               0               0               0               0               0               0               0   0
> a88000.spi:ec@0:keyboard-controller     0               0               0               0               0               0           0
>                 0               0
> alarmtimer      0               0               0               0               0               0               0               0   0
> cros-ec-rtc.1.auto      0               0               0               0               0               0               0           0
>                 0
> a8f8800.usb     0               0               0               0               0               0               0               0   0
> a6f8800.usb     0               0               0               0               0               0               0               0   0
> localhost ~ # ls -l /sys/class/wakeup/
> total 0
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 alarmtimer -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/rtc/rtc0/alarmtimer

So why is this not "(...)rtc0/wakeup/alarmtimer" ?

This particular bit looks kind of inconsistent.

I guess without your patch you'd see "(...)rtc0/wakeup/wakeup0" instead, right?

> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup0 -> ../../devices/platform/soc/a6f8800.usb/wakeup/wakeup0
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup1 -> ../../devices/platform/soc/a8f8800.usb/wakeup/wakeup1
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup2 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/wakeup/wakeup2
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup3 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/a88000.spi:ec@0:keyboard-controller/wakeup/wakeup3
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup4 -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/wakeup/wakeup4
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup5 -> ../../devices/platform/gpio-keys/wakeup/wakeup5
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup6 -> ../../devices/platform/soc/a8f8800.usb/a800000.dwc3/xhci-hcd.7.auto/usb1/1-1/1-1.1/wakeup/wakeup6
> lrwxrwxrwx. 1 root root 0 Jul 31 17:43 wakeup7 -> ../../devices/platform/soc/a8f8800.usb/a800000.dwc3/xhci-hcd.7.auto/usb1/1-1/1-1.2/1-1.2.4/1-1.2.4.1/wakeup/wakeup7
>

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

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-08-01 01:09:22)
> On Thu, Aug 1, 2019 at 2:45 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Stephen Boyd (2019-07-31 16:45:31)
> > >
> > > This approach also nicely detects duplicate wakeup source names in the
> > > case that the string passed in to wakeup_source_register() is already
> > > used on the virtual bus.
> >
> > This was clearly untested! Here's a better one. This is what I see on my
> > device with this patch squashed in:
> >
> > localhost ~ # cat /sys/kernel/debug/wakeup_sources
> > name            active_count    event_count     wakeup_count    expire_count    active_since    total_time      max_time        last_change  prevent_suspend_time
> > 1-1.2.4.1       0               0               0               0               0               0               0               0   0
> > 1-1.1           0               0               0               0               0               0               0               0   0
> > gpio-keys       0               0               0               0               0               0               0               0   0
> > spi10.0         0               0               0               0               0               0               0               0   0
> > a88000.spi:ec@0:keyboard-controller     0               0               0               0               0               0           0
> >                 0               0
> > alarmtimer      0               0               0               0               0               0               0               0   0
> > cros-ec-rtc.1.auto      0               0               0               0               0               0               0           0
> >                 0
> > a8f8800.usb     0               0               0               0               0               0               0               0   0
> > a6f8800.usb     0               0               0               0               0               0               0               0   0
> > localhost ~ # ls -l /sys/class/wakeup/
> > total 0
> > lrwxrwxrwx. 1 root root 0 Jul 31 17:43 alarmtimer -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/rtc/rtc0/alarmtimer
> 
> So why is this not "(...)rtc0/wakeup/alarmtimer" ?
> 
> This particular bit looks kind of inconsistent.

I believe this is the code you're looking for in drivers/base/core.c

                /*
                 * If we have no parent, we live in "virtual".
                 * Class-devices with a non class-device as parent, live
                 * in a "glue" directory to prevent namespace collisions.
                 */
                if (parent == NULL)
                        parent_kobj = virtual_device_parent(dev);
                else if (parent->class && !dev->class->ns_type)
                        return &parent->kobj;
                else
                        parent_kobj = &parent->kobj;


> 
> I guess without your patch you'd see "(...)rtc0/wakeup/wakeup0" instead, right?

No, it would be rtc0/wakeup0. That's because rtc is a class, and rtc0 is
part of that class, so we don't try to make a glue directory named after
the class to avoid collisions (see class_dir_create_and_add()
implementation).

BTW, paths in /sys/devices aren't supposed to matter too much. In this
case, I'd expect to see userspace looking at the /sys/class/wakeup path
to follow the symlink to figure out what device triggered a wakeup. It
can look at the 'device' symlink inside the directory for the wakeup
device to figure out which one it is.

Final thought, might want to suppress the power directory from being
created for the wakeup class. It looks odd to have
/sys/class/wakeup/wakeup0/power when the presumably does nothing.

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

[Rafael J. Wysocki]

On Thu, Aug 1, 2019 at 5:31 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-08-01 01:09:22)
> > On Thu, Aug 1, 2019 at 2:45 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Stephen Boyd (2019-07-31 16:45:31)
> > > >
> > > > This approach also nicely detects duplicate wakeup source names in the
> > > > case that the string passed in to wakeup_source_register() is already
> > > > used on the virtual bus.
> > >
> > > This was clearly untested! Here's a better one. This is what I see on my
> > > device with this patch squashed in:
> > >
> > > localhost ~ # cat /sys/kernel/debug/wakeup_sources
> > > name            active_count    event_count     wakeup_count    expire_count    active_since    total_time      max_time        last_change  prevent_suspend_time
> > > 1-1.2.4.1       0               0               0               0               0               0               0               0   0
> > > 1-1.1           0               0               0               0               0               0               0               0   0
> > > gpio-keys       0               0               0               0               0               0               0               0   0
> > > spi10.0         0               0               0               0               0               0               0               0   0
> > > a88000.spi:ec@0:keyboard-controller     0               0               0               0               0               0           0
> > >                 0               0
> > > alarmtimer      0               0               0               0               0               0               0               0   0
> > > cros-ec-rtc.1.auto      0               0               0               0               0               0               0           0
> > >                 0
> > > a8f8800.usb     0               0               0               0               0               0               0               0   0
> > > a6f8800.usb     0               0               0               0               0               0               0               0   0
> > > localhost ~ # ls -l /sys/class/wakeup/
> > > total 0
> > > lrwxrwxrwx. 1 root root 0 Jul 31 17:43 alarmtimer -> ../../devices/platform/soc/ac0000.geniqup/a88000.spi/spi_master/spi10/spi10.0/cros-ec-dev.0.auto/cros-ec-rtc.1.auto/rtc/rtc0/alarmtimer
> >
> > So why is this not "(...)rtc0/wakeup/alarmtimer" ?
> >
> > This particular bit looks kind of inconsistent.
>
> I believe this is the code you're looking for in drivers/base/core.c
>
>                 /*
>                  * If we have no parent, we live in "virtual".
>                  * Class-devices with a non class-device as parent, live
>                  * in a "glue" directory to prevent namespace collisions.
>                  */
>                 if (parent == NULL)
>                         parent_kobj = virtual_device_parent(dev);
>                 else if (parent->class && !dev->class->ns_type)
>                         return &parent->kobj;
>                 else
>                         parent_kobj = &parent->kobj;
>

OK, so it looks like there really is a little benefit from making the
device associated with the wakeup source be the parent of its virtual
dev.

> >
> > I guess without your patch you'd see "(...)rtc0/wakeup/wakeup0" instead, right?
>
> No, it would be rtc0/wakeup0. That's because rtc is a class, and rtc0 is
> part of that class, so we don't try to make a glue directory named after
> the class to avoid collisions (see class_dir_create_and_add()
> implementation).

That's not really consistent.

> BTW, paths in /sys/devices aren't supposed to matter too much. In this
> case, I'd expect to see userspace looking at the /sys/class/wakeup path
> to follow the symlink to figure out what device triggered a wakeup. It
> can look at the 'device' symlink inside the directory for the wakeup
> device to figure out which one it is.

But if you go from the device, it would be good to be able to figure
out which wakeup sources are associated with it and in the alarmtimer
example you don't even see that it is a wakeup source without
following the link.

So the "wakeupN" virtual dev names for all wakeup source objects are
less confusing IMO.

It would be good to avoid the glue dir creation in all cases somehow too.

> Final thought, might want to suppress the power directory from being
> created for the wakeup class. It looks odd to have
> /sys/class/wakeup/wakeup0/power when the presumably does nothing.

I agree and there is a flag for that IIRC.

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

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-08-01 10:21:44)
> On Thu, Aug 1, 2019 at 5:31 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > BTW, paths in /sys/devices aren't supposed to matter too much. In this
> > case, I'd expect to see userspace looking at the /sys/class/wakeup path
> > to follow the symlink to figure out what device triggered a wakeup. It
> > can look at the 'device' symlink inside the directory for the wakeup
> > device to figure out which one it is.
> 
> But if you go from the device, it would be good to be able to figure
> out which wakeup sources are associated with it and in the alarmtimer
> example you don't even see that it is a wakeup source without
> following the link.

Userspace shouldn't go from the device path (/sys/devices/.../rtc0 in
this example). That's incorrect. Instead, userspace should go from the
/sys/class/wakeup/... path. It should iterate over all the devices in
the class path and look at the device pointers instead.

# ls /sys/class/wakeup/*/device -l
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/alarmtimer/device -> ../../rtc0
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup0/device -> ../../../a6f8800.usb
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup1/device -> ../../../a8f8800.usb
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup2/device -> ../../../cros-ec-rtc.1.auto
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup3/device -> ../../sbs-16-000b
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup4/device -> ../../../a88000.spi:ec@0:keyboard-controller
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup5/device -> ../../../spi10.0
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup6/device -> ../../../gpio-keys
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup7/device -> ../../../1-1.1
lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup8/device -> ../../../1-1.2.4.1

> 
> So the "wakeupN" virtual dev names for all wakeup source objects are
> less confusing IMO.
> 
> It would be good to avoid the glue dir creation in all cases somehow too.

I recall some differences between a bus_type and a class. Are you
suggesting to use a bus_type for the wakeup sources? I like the class
approach taken here to use different device names because it avoids the
name collisions, avoids making another attribute to express the name of
the wakeup source, and doesn't make a more heavyweight driver
abstraction on top of wakeup sources.

In fact, that ls command above pretty much sums up the wakeup source
name and the device that it's associated with. Whatever goes on inside
/sys/devices/... with respect to where the devices go and how they're
structured is not important, at least to me. Why is it important to you?

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

[Greg Kroah-Hartman]

On Thu, Aug 01, 2019 at 12:25:04PM -0700, Stephen Boyd wrote:
> Quoting Rafael J. Wysocki (2019-08-01 10:21:44)
> > On Thu, Aug 1, 2019 at 5:31 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > BTW, paths in /sys/devices aren't supposed to matter too much. In this
> > > case, I'd expect to see userspace looking at the /sys/class/wakeup path
> > > to follow the symlink to figure out what device triggered a wakeup. It
> > > can look at the 'device' symlink inside the directory for the wakeup
> > > device to figure out which one it is.
> > 
> > But if you go from the device, it would be good to be able to figure
> > out which wakeup sources are associated with it and in the alarmtimer
> > example you don't even see that it is a wakeup source without
> > following the link.
> 
> Userspace shouldn't go from the device path (/sys/devices/.../rtc0 in
> this example). That's incorrect. Instead, userspace should go from the
> /sys/class/wakeup/... path. It should iterate over all the devices in
> the class path and look at the device pointers instead.
> 
> # ls /sys/class/wakeup/*/device -l
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/alarmtimer/device -> ../../rtc0
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup0/device -> ../../../a6f8800.usb
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup1/device -> ../../../a8f8800.usb
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup2/device -> ../../../cros-ec-rtc.1.auto
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup3/device -> ../../sbs-16-000b
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup4/device -> ../../../a88000.spi:ec@0:keyboard-controller
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup5/device -> ../../../spi10.0
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup6/device -> ../../../gpio-keys
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup7/device -> ../../../1-1.1
> lrwxrwxrwx. 1 root root 0 Aug  1 12:13 /sys/class/wakeup/wakeup8/device -> ../../../1-1.2.4.1
> 
> > 
> > So the "wakeupN" virtual dev names for all wakeup source objects are
> > less confusing IMO.
> > 
> > It would be good to avoid the glue dir creation in all cases somehow too.
> 
> I recall some differences between a bus_type and a class. Are you
> suggesting to use a bus_type for the wakeup sources? I like the class
> approach taken here to use different device names because it avoids the
> name collisions, avoids making another attribute to express the name of
> the wakeup source, and doesn't make a more heavyweight driver
> abstraction on top of wakeup sources.

This should be a class, as-is, not a bus type as these are all the same
type of "interface" for many different individual devices.

The difference between bus type and class is:
	- class is almost always how userspace sees the device, and cuts
	  across types of devices.  For example, a keyboard is a type of
	  an input device, and it can be a serial, PS/2, bluetooth, or
	  USB type of device.
	- a bus is a common type of devices usually at the hardware
	  level, where a driver is needed to send class-specific
	  commands down to a specific hardware device.  Busses have
	  drivers that bind a specific class to a specific device.
	  For example, there is a USB bus, and it has USB drivers for
	  things like a keyboard.  That USB keyboard driver knows how to
	  talk to the specific USB commands for the hardware and
	  translate them into specific input calls for the input class.

Hope this helps explain things better.

thanks,

greg k-h

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

[Tri Vo]

On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > >
> > > On Wed, Jul 31, 2019 at 3:42 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > That's not my point (see below).
> > > >
> > > > > > > > +       if (id < 0)
> > > > > > > > +               return id;
> > > > > > > > +       ws->id = id;
> > > > > > > > +
> > > > > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > > > +                                       wakeup_source_groups, "ws%d",
> > > > > > >
> > > > > > > I thought the name was going to still be 'wakeupN'?
> > > > > >
> > > > > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> > > > >
> > > > > "ws%d" here is the name in the sysfs path rather than the name of the
> > > > > wakeup source. Wakeup source name is not altered in this patch.
> > > > >
> > > >
> > > > So why wouldn't something like this suffice:
> > > >
> > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > >
> > > > ?
> > >
> > > ws->name is inherited from the device name. IIUC device names are not
> > > guaranteed to be unique. So if different devices with the same name
> > > register wakeup sources, there is an error.
> >
> > OK
> >
> > So I guess the names are retained for backwards compatibility with
> > existing user space that may be using them?
> >
> > That's kind of fair enough, but having two different identification
> > schemes for wakeup sources will end up confusing.
>
> I understand your concern about the IDA now. Thanks for clarifying.
>
> How about we name the devices 'wakeupN' with the IDA when they're
> registered with a non-NULL device pointer and then name them whatever
> the name argument is when the device pointer is NULL. If we have this,
> we should be able to drop the name attribute in sysfs and figure out the
> name either by looking at the device name in /sys/class/wakeup/ if it
> isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> and look at the parent device name there.

This makes it difficult for userspace to query the name a wakeup
source, as it now has to first figure out if a wakeup source is
associated with a device or not. The criteria for that is also
awkward, userspase has to check if directory path contains "wakeupN",
then it's a virtual wakeup source.

IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
for every wakeup source.

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

[Stephen Boyd]

Quoting Tri Vo (2019-08-01 12:50:25)
> On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > >
> > > > >
> > > > > So why wouldn't something like this suffice:
> > > > >
> > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > >
> > > > > ?
> > > >
> > > > ws->name is inherited from the device name. IIUC device names are not
> > > > guaranteed to be unique. So if different devices with the same name
> > > > register wakeup sources, there is an error.
> > >
> > > OK
> > >
> > > So I guess the names are retained for backwards compatibility with
> > > existing user space that may be using them?
> > >
> > > That's kind of fair enough, but having two different identification
> > > schemes for wakeup sources will end up confusing.
> >
> > I understand your concern about the IDA now. Thanks for clarifying.
> >
> > How about we name the devices 'wakeupN' with the IDA when they're
> > registered with a non-NULL device pointer and then name them whatever
> > the name argument is when the device pointer is NULL. If we have this,
> > we should be able to drop the name attribute in sysfs and figure out the
> > name either by looking at the device name in /sys/class/wakeup/ if it
> > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > and look at the parent device name there.
> 
> This makes it difficult for userspace to query the name a wakeup
> source, as it now has to first figure out if a wakeup source is
> associated with a device or not. The criteria for that is also
> awkward, userspase has to check if directory path contains "wakeupN",
> then it's a virtual wakeup source.

I think you mean if it doesn't match wakeupN then it's a virtual wakeup
source?

> 
> IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> for every wakeup source.

I don't find it awkward or difficult. Just know what the name of the
/sys/class/wakeup/ path is and then extract the name from there if it
doesn't match wakeupN, otherwise read the 'device' symlink and run it
through basename.

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

[Tri Vo]

On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tri Vo (2019-08-01 12:50:25)
> > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > > >
> > > > > >
> > > > > > So why wouldn't something like this suffice:
> > > > > >
> > > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > > >
> > > > > > ?
> > > > >
> > > > > ws->name is inherited from the device name. IIUC device names are not
> > > > > guaranteed to be unique. So if different devices with the same name
> > > > > register wakeup sources, there is an error.
> > > >
> > > > OK
> > > >
> > > > So I guess the names are retained for backwards compatibility with
> > > > existing user space that may be using them?
> > > >
> > > > That's kind of fair enough, but having two different identification
> > > > schemes for wakeup sources will end up confusing.
> > >
> > > I understand your concern about the IDA now. Thanks for clarifying.
> > >
> > > How about we name the devices 'wakeupN' with the IDA when they're
> > > registered with a non-NULL device pointer and then name them whatever
> > > the name argument is when the device pointer is NULL. If we have this,
> > > we should be able to drop the name attribute in sysfs and figure out the
> > > name either by looking at the device name in /sys/class/wakeup/ if it
> > > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > > and look at the parent device name there.
> >
> > This makes it difficult for userspace to query the name a wakeup
> > source, as it now has to first figure out if a wakeup source is
> > associated with a device or not. The criteria for that is also
> > awkward, userspase has to check if directory path contains "wakeupN",
> > then it's a virtual wakeup source.
>
> I think you mean if it doesn't match wakeupN then it's a virtual wakeup
> source?

Yes
>
> >
> > IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> > for every wakeup source.
>
> I don't find it awkward or difficult. Just know what the name of the
> /sys/class/wakeup/ path is and then extract the name from there if it
> doesn't match wakeupN, otherwise read the 'device' symlink and run it
> through basename.

The concern was that having both "id" and "name" around might be
confusing. I don't think that making the presence of "name"
conditional helps here. And we have to maintain additional logic in
both kernel and userspace to support this.

Also, say, userspace grabs a wakelock named "wakeup0". In the current
patch, this results in a name collision and an error. Even assuming
that userspace doesn't have ill intent, it still needs to be aware of
"wakeupN" naming pattern to avoid this error condition.

All wakeup sources in the /sys/class/wakeup/ are in the same namespace
regardless of where they originate from, i.e. we have to either (1)
inspect the name of a wakeup source and make sure it's unique before
using it as a directory name OR (2) generate the directory name on
behalf of whomever is registering a wakeup source, which I think is a
much simpler solution.

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

[Rafael J. Wysocki]

On Thu, Aug 1, 2019 at 11:45 PM Tri Vo <trong@android.com> wrote:
>
> On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Tri Vo (2019-08-01 12:50:25)
> > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > > > >
> > > > > > >
> > > > > > > So why wouldn't something like this suffice:
> > > > > > >
> > > > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > > > >
> > > > > > > ?
> > > > > >
> > > > > > ws->name is inherited from the device name. IIUC device names are not
> > > > > > guaranteed to be unique. So if different devices with the same name
> > > > > > register wakeup sources, there is an error.
> > > > >
> > > > > OK
> > > > >
> > > > > So I guess the names are retained for backwards compatibility with
> > > > > existing user space that may be using them?
> > > > >
> > > > > That's kind of fair enough, but having two different identification
> > > > > schemes for wakeup sources will end up confusing.
> > > >
> > > > I understand your concern about the IDA now. Thanks for clarifying.
> > > >
> > > > How about we name the devices 'wakeupN' with the IDA when they're
> > > > registered with a non-NULL device pointer and then name them whatever
> > > > the name argument is when the device pointer is NULL. If we have this,
> > > > we should be able to drop the name attribute in sysfs and figure out the
> > > > name either by looking at the device name in /sys/class/wakeup/ if it
> > > > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > > > and look at the parent device name there.
> > >
> > > This makes it difficult for userspace to query the name a wakeup
> > > source, as it now has to first figure out if a wakeup source is
> > > associated with a device or not. The criteria for that is also
> > > awkward, userspase has to check if directory path contains "wakeupN",
> > > then it's a virtual wakeup source.
> >
> > I think you mean if it doesn't match wakeupN then it's a virtual wakeup
> > source?
>
> Yes
> >
> > >
> > > IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> > > for every wakeup source.
> >
> > I don't find it awkward or difficult. Just know what the name of the
> > /sys/class/wakeup/ path is and then extract the name from there if it
> > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > through basename.
>
> The concern was that having both "id" and "name" around might be
> confusing. I don't think that making the presence of "name"
> conditional helps here. And we have to maintain additional logic in
> both kernel and userspace to support this.
>
> Also, say, userspace grabs a wakelock named "wakeup0". In the current
> patch, this results in a name collision and an error. Even assuming
> that userspace doesn't have ill intent, it still needs to be aware of
> "wakeupN" naming pattern to avoid this error condition.
>
> All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> regardless of where they originate from, i.e. we have to either (1)
> inspect the name of a wakeup source and make sure it's unique before
> using it as a directory name OR (2) generate the directory name on
> behalf of whomever is registering a wakeup source, which I think is a
> much simpler solution.

OK, whatever.

Let's use the IDA as originally proposed and retain the names for
backwards compatibility only.

Maybe just allocate the ID at the wakeup source object creation time
already (ISTR that you did that before attempting to create a virtual
device for the wakeup source).

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

[Tri Vo]

On Thu, Aug 1, 2019 at 3:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Aug 1, 2019 at 11:45 PM Tri Vo <trong@android.com> wrote:
> >
> > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Tri Vo (2019-08-01 12:50:25)
> > > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > > > > >
> > > > > > > >
> > > > > > > > So why wouldn't something like this suffice:
> > > > > > > >
> > > > > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > > > > >
> > > > > > > > ?
> > > > > > >
> > > > > > > ws->name is inherited from the device name. IIUC device names are not
> > > > > > > guaranteed to be unique. So if different devices with the same name
> > > > > > > register wakeup sources, there is an error.
> > > > > >
> > > > > > OK
> > > > > >
> > > > > > So I guess the names are retained for backwards compatibility with
> > > > > > existing user space that may be using them?
> > > > > >
> > > > > > That's kind of fair enough, but having two different identification
> > > > > > schemes for wakeup sources will end up confusing.
> > > > >
> > > > > I understand your concern about the IDA now. Thanks for clarifying.
> > > > >
> > > > > How about we name the devices 'wakeupN' with the IDA when they're
> > > > > registered with a non-NULL device pointer and then name them whatever
> > > > > the name argument is when the device pointer is NULL. If we have this,
> > > > > we should be able to drop the name attribute in sysfs and figure out the
> > > > > name either by looking at the device name in /sys/class/wakeup/ if it
> > > > > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > > > > and look at the parent device name there.
> > > >
> > > > This makes it difficult for userspace to query the name a wakeup
> > > > source, as it now has to first figure out if a wakeup source is
> > > > associated with a device or not. The criteria for that is also
> > > > awkward, userspase has to check if directory path contains "wakeupN",
> > > > then it's a virtual wakeup source.
> > >
> > > I think you mean if it doesn't match wakeupN then it's a virtual wakeup
> > > source?
> >
> > Yes
> > >
> > > >
> > > > IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> > > > for every wakeup source.
> > >
> > > I don't find it awkward or difficult. Just know what the name of the
> > > /sys/class/wakeup/ path is and then extract the name from there if it
> > > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > > through basename.
> >
> > The concern was that having both "id" and "name" around might be
> > confusing. I don't think that making the presence of "name"
> > conditional helps here. And we have to maintain additional logic in
> > both kernel and userspace to support this.
> >
> > Also, say, userspace grabs a wakelock named "wakeup0". In the current
> > patch, this results in a name collision and an error. Even assuming
> > that userspace doesn't have ill intent, it still needs to be aware of
> > "wakeupN" naming pattern to avoid this error condition.
> >
> > All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> > regardless of where they originate from, i.e. we have to either (1)
> > inspect the name of a wakeup source and make sure it's unique before
> > using it as a directory name OR (2) generate the directory name on
> > behalf of whomever is registering a wakeup source, which I think is a
> > much simpler solution.
>
> OK, whatever.
>
> Let's use the IDA as originally proposed and retain the names for
> backwards compatibility only.
>
> Maybe just allocate the ID at the wakeup source object creation time
> already (ISTR that you did that before attempting to create a virtual
> device for the wakeup source).

Yes, allocating the ID when creating the wakeup source object makes
sense. However, kernel/power/wakelock.c allocates its wakeup sources
manually. I imagine we don't want these IDs to be created in more than
one place.

Making wakelock.c only use wakeup_source_*() family of functions when
dealing with wakeup sources  might be a worthwhile change though. Then
we won't have to worry about ID allocation in wakelock.c. WDYT?

Also, it sounds like we all agree with "/sys/class/wakeup/wsN/" path
and "/sys/class/wakeup/wsN/name" attribute for each wakeup source,
right?

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

[Rafael J. Wysocki]

On Sun, Aug 4, 2019 at 12:40 AM Tri Vo <trong@android.com> wrote:
>
> On Thu, Aug 1, 2019 at 3:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Aug 1, 2019 at 11:45 PM Tri Vo <trong@android.com> wrote:
> > >
> > > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Tri Vo (2019-08-01 12:50:25)
> > > > > On Wed, Jul 31, 2019 at 4:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Rafael J. Wysocki (2019-07-31 16:10:38)
> > > > > > > On Thu, Aug 1, 2019 at 12:59 AM Tri Vo <trong@android.com> wrote:
> > > > > > > >
> > > > > > > > >
> > > > > > > > > So why wouldn't something like this suffice:
> > > > > > > > >
> > > > > > > > > dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > > > > >                                 wakeup_source_groups, "wakeup:%s", ws->name);
> > > > > > > > >
> > > > > > > > > ?
> > > > > > > >
> > > > > > > > ws->name is inherited from the device name. IIUC device names are not
> > > > > > > > guaranteed to be unique. So if different devices with the same name
> > > > > > > > register wakeup sources, there is an error.
> > > > > > >
> > > > > > > OK
> > > > > > >
> > > > > > > So I guess the names are retained for backwards compatibility with
> > > > > > > existing user space that may be using them?
> > > > > > >
> > > > > > > That's kind of fair enough, but having two different identification
> > > > > > > schemes for wakeup sources will end up confusing.
> > > > > >
> > > > > > I understand your concern about the IDA now. Thanks for clarifying.
> > > > > >
> > > > > > How about we name the devices 'wakeupN' with the IDA when they're
> > > > > > registered with a non-NULL device pointer and then name them whatever
> > > > > > the name argument is when the device pointer is NULL. If we have this,
> > > > > > we should be able to drop the name attribute in sysfs and figure out the
> > > > > > name either by looking at the device name in /sys/class/wakeup/ if it
> > > > > > isn't 'wakeupN', or follow the symlink to the device in /sys/devices/
> > > > > > and look at the parent device name there.
> > > > >
> > > > > This makes it difficult for userspace to query the name a wakeup
> > > > > source, as it now has to first figure out if a wakeup source is
> > > > > associated with a device or not. The criteria for that is also
> > > > > awkward, userspase has to check if directory path contains "wakeupN",
> > > > > then it's a virtual wakeup source.
> > > >
> > > > I think you mean if it doesn't match wakeupN then it's a virtual wakeup
> > > > source?
> > >
> > > Yes
> > > >
> > > > >
> > > > > IMO it's cleaner to consistently have /sys/class/wakeup/wakeupN/name
> > > > > for every wakeup source.
> > > >
> > > > I don't find it awkward or difficult. Just know what the name of the
> > > > /sys/class/wakeup/ path is and then extract the name from there if it
> > > > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > > > through basename.
> > >
> > > The concern was that having both "id" and "name" around might be
> > > confusing. I don't think that making the presence of "name"
> > > conditional helps here. And we have to maintain additional logic in
> > > both kernel and userspace to support this.
> > >
> > > Also, say, userspace grabs a wakelock named "wakeup0". In the current
> > > patch, this results in a name collision and an error. Even assuming
> > > that userspace doesn't have ill intent, it still needs to be aware of
> > > "wakeupN" naming pattern to avoid this error condition.
> > >
> > > All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> > > regardless of where they originate from, i.e. we have to either (1)
> > > inspect the name of a wakeup source and make sure it's unique before
> > > using it as a directory name OR (2) generate the directory name on
> > > behalf of whomever is registering a wakeup source, which I think is a
> > > much simpler solution.
> >
> > OK, whatever.
> >
> > Let's use the IDA as originally proposed and retain the names for
> > backwards compatibility only.
> >
> > Maybe just allocate the ID at the wakeup source object creation time
> > already (ISTR that you did that before attempting to create a virtual
> > device for the wakeup source).
>
> Yes, allocating the ID when creating the wakeup source object makes
> sense. However, kernel/power/wakelock.c allocates its wakeup sources
> manually. I imagine we don't want these IDs to be created in more than
> one place.

No, we don't.

> Making wakelock.c only use wakeup_source_*() family of functions when
> dealing with wakeup sources  might be a worthwhile change though. Then
> we won't have to worry about ID allocation in wakelock.c. WDYT?

Sounds reasonable to me.

> Also, it sounds like we all agree with "/sys/class/wakeup/wsN/" path
> and "/sys/class/wakeup/wsN/name" attribute for each wakeup source,
> right?

Generally yes, but please make it "wakeupN".

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

[Stephen Boyd]

Quoting Tri Vo (2019-08-01 14:44:52)
> On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> >
> > I don't find it awkward or difficult. Just know what the name of the
> > /sys/class/wakeup/ path is and then extract the name from there if it
> > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > through basename.
> 
> The concern was that having both "id" and "name" around might be
> confusing. I don't think that making the presence of "name"
> conditional helps here. And we have to maintain additional logic in
> both kernel and userspace to support this.
> 
> Also, say, userspace grabs a wakelock named "wakeup0". In the current
> patch, this results in a name collision and an error. Even assuming
> that userspace doesn't have ill intent, it still needs to be aware of
> "wakeupN" naming pattern to avoid this error condition.
> 
> All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> regardless of where they originate from, i.e. we have to either (1)
> inspect the name of a wakeup source and make sure it's unique before
> using it as a directory name OR (2) generate the directory name on
> behalf of whomever is registering a wakeup source, which I think is a
> much simpler solution.

Ok. If the device name is going to be something generic like 'wakeupN',
then we need to make sure that the wakeup source name is unique.
Otherwise, I'm not able to see how userspace will differentiate between
two of the same named wakelocks. Before this patch the wakeup source
name looks to have been used for debugging, but now it's being used
programmatically to let userspace act upon it somehow. Maybe it's for
debug still, but I could see how userspace may want to hunt down the
wakelock that's created in userspace and penalize or kill the task
that's waking up the device.

I see that wakelock_lookup_add() already checks the list of wakelock
wakeup sources, but I don't see how I can't create an "alarmtimer"
wakelock again, but this time for userspace, by writing into
/sys/power/wake_lock.

What happens with namespaces here BTW? Can a wakelock be made in one
namespace and that is the same name as another wakelock in a different
namespace? Right now it doesn't look possible because of the global name
matching, but it probably makes sense to support this? Maybe we just
shouldn't make anything in sysfs for wake sources that can be any random
name created from the wakelock path right now. I don't see how it can be
traced back to the process that created it in any reasonable way.

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

[Tri Vo]

On Thu, Aug 1, 2019 at 3:11 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tri Vo (2019-08-01 14:44:52)
> > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > >
> > > I don't find it awkward or difficult. Just know what the name of the
> > > /sys/class/wakeup/ path is and then extract the name from there if it
> > > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > > through basename.
> >
> > The concern was that having both "id" and "name" around might be
> > confusing. I don't think that making the presence of "name"
> > conditional helps here. And we have to maintain additional logic in
> > both kernel and userspace to support this.
> >
> > Also, say, userspace grabs a wakelock named "wakeup0". In the current
> > patch, this results in a name collision and an error. Even assuming
> > that userspace doesn't have ill intent, it still needs to be aware of
> > "wakeupN" naming pattern to avoid this error condition.
> >
> > All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> > regardless of where they originate from, i.e. we have to either (1)
> > inspect the name of a wakeup source and make sure it's unique before
> > using it as a directory name OR (2) generate the directory name on
> > behalf of whomever is registering a wakeup source, which I think is a
> > much simpler solution.
>
> Ok. If the device name is going to be something generic like 'wakeupN',
> then we need to make sure that the wakeup source name is unique.

If we could easily make sure that wakeup source names are unique, then
we wouldn't need to generate "wakeupN" ids :)

> Otherwise, I'm not able to see how userspace will differentiate between
> two of the same named wakelocks. Before this patch the wakeup source
> name looks to have been used for debugging, but now it's being used
> programmatically to let userspace act upon it somehow. Maybe it's for
> debug still, but I could see how userspace may want to hunt down the
> wakelock that's created in userspace and penalize or kill the task
> that's waking up the device.

Two wakelocks can't have the same name. So they are still
distinguishable from userspace. However, there is still no way to
figure out from userspace which process created which wake lock.
That's a weakness of /sys/power/wake_lock API, independent of this
patch.
>
> I see that wakelock_lookup_add() already checks the list of wakelock
> wakeup sources, but I don't see how I can't create an "alarmtimer"
> wakelock again, but this time for userspace, by writing into
> /sys/power/wake_lock.

Behind the scenes, writing "alarmtimer" to /sys/power/wake_lock
creates a wakeup source named "alarmtimer", which in turn creates a
directory /sys/class/wakeup/alarmtimer (in you patch), which is likely
already created by alarmtimer. This leads to an error. The error is
resolved if wakeup source's sysfs entry is /sys/class/wakeup/wakeupN
instead.
>
> What happens with namespaces here BTW? Can a wakelock be made in one
> namespace and that is the same name as another wakelock in a different
> namespace? Right now it doesn't look possible because of the global name
> matching, but it probably makes sense to support this? Maybe we just
> shouldn't make anything in sysfs for wake sources that can be any random
> name created from the wakelock path right now. I don't see how it can be
> traced back to the process that created it in any reasonable way.

It should be OK if we don't use the arbitrary wakelock name in the
path, but instead use the generated id "wakeupN".

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

[Stephen Boyd]

Quoting Tri Vo (2019-08-01 15:44:40)
> On Thu, Aug 1, 2019 at 3:11 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Tri Vo (2019-08-01 14:44:52)
> > >
> > > The concern was that having both "id" and "name" around might be
> > > confusing. I don't think that making the presence of "name"
> > > conditional helps here. And we have to maintain additional logic in
> > > both kernel and userspace to support this.
> > >
> > > Also, say, userspace grabs a wakelock named "wakeup0". In the current
> > > patch, this results in a name collision and an error. Even assuming
> > > that userspace doesn't have ill intent, it still needs to be aware of
> > > "wakeupN" naming pattern to avoid this error condition.
> > >
> > > All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> > > regardless of where they originate from, i.e. we have to either (1)
> > > inspect the name of a wakeup source and make sure it's unique before
> > > using it as a directory name OR (2) generate the directory name on
> > > behalf of whomever is registering a wakeup source, which I think is a
> > > much simpler solution.
> >
> > Ok. If the device name is going to be something generic like 'wakeupN',
> > then we need to make sure that the wakeup source name is unique.
> 
> If we could easily make sure that wakeup source names are unique, then
> we wouldn't need to generate "wakeupN" ids :)

It's not hard to make sure the device names are unique, we just use an
IDA and we're done. The problem is making it easy for the user to
understand what wakeup source it is. If the ws->name is duplicated that
is harder. It's an orthogonal problem.

> 
> > Otherwise, I'm not able to see how userspace will differentiate between
> > two of the same named wakelocks. Before this patch the wakeup source
> > name looks to have been used for debugging, but now it's being used
> > programmatically to let userspace act upon it somehow. Maybe it's for
> > debug still, but I could see how userspace may want to hunt down the
> > wakelock that's created in userspace and penalize or kill the task
> > that's waking up the device.
> 
> Two wakelocks can't have the same name. So they are still
> distinguishable from userspace. However, there is still no way to
> figure out from userspace which process created which wake lock.
> That's a weakness of /sys/power/wake_lock API, independent of this
> patch.

Even without knowing the process, we can have a problem if kernelspace
makes the same named wake source as one made in userspace through the
wakelock APIs. We won't be able to distinguish the two. Sounds like
we've never had this problem though, so I guess we ignore it.

> >
> > I see that wakelock_lookup_add() already checks the list of wakelock
> > wakeup sources, but I don't see how I can't create an "alarmtimer"
> > wakelock again, but this time for userspace, by writing into
> > /sys/power/wake_lock.
> 
> Behind the scenes, writing "alarmtimer" to /sys/power/wake_lock
> creates a wakeup source named "alarmtimer", which in turn creates a
> directory /sys/class/wakeup/alarmtimer (in you patch), which is likely
> already created by alarmtimer. This leads to an error. The error is
> resolved if wakeup source's sysfs entry is /sys/class/wakeup/wakeupN
> instead.

Right.

> >
> > What happens with namespaces here BTW? Can a wakelock be made in one
> > namespace and that is the same name as another wakelock in a different
> > namespace? Right now it doesn't look possible because of the global name
> > matching, but it probably makes sense to support this? Maybe we just
> > shouldn't make anything in sysfs for wake sources that can be any random
> > name created from the wakelock path right now. I don't see how it can be
> > traced back to the process that created it in any reasonable way.
> 
> It should be OK if we don't use the arbitrary wakelock name in the
> path, but instead use the generated id "wakeupN".

I'm more concerned about namespaces in general and how the wake_lock
file in sysfs is supposed to work with it. It sounds like it just
doesn't work and userspace has to be careful to not reuse the same name
for some sort of wakelock and sit some daemon on top of the kernel
interface.

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

[Rafael J. Wysocki]

On Fri, Aug 2, 2019 at 12:11 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Tri Vo (2019-08-01 14:44:52)
> > On Thu, Aug 1, 2019 at 1:23 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > >
> > > I don't find it awkward or difficult. Just know what the name of the
> > > /sys/class/wakeup/ path is and then extract the name from there if it
> > > doesn't match wakeupN, otherwise read the 'device' symlink and run it
> > > through basename.
> >
> > The concern was that having both "id" and "name" around might be
> > confusing. I don't think that making the presence of "name"
> > conditional helps here. And we have to maintain additional logic in
> > both kernel and userspace to support this.
> >
> > Also, say, userspace grabs a wakelock named "wakeup0". In the current
> > patch, this results in a name collision and an error. Even assuming
> > that userspace doesn't have ill intent, it still needs to be aware of
> > "wakeupN" naming pattern to avoid this error condition.
> >
> > All wakeup sources in the /sys/class/wakeup/ are in the same namespace
> > regardless of where they originate from, i.e. we have to either (1)
> > inspect the name of a wakeup source and make sure it's unique before
> > using it as a directory name OR (2) generate the directory name on
> > behalf of whomever is registering a wakeup source, which I think is a
> > much simpler solution.
>
> Ok. If the device name is going to be something generic like 'wakeupN',
> then we need to make sure that the wakeup source name is unique.
> Otherwise, I'm not able to see how userspace will differentiate between
> two of the same named wakelocks. Before this patch the wakeup source
> name looks to have been used for debugging, but now it's being used
> programmatically to let userspace act upon it somehow.

I'm not actually sure if this patch changes the situation with respect
to wakeup source names.   User space still can use them for whatever
it used to use the list in debugfs and that's it.

That's what I mean by retaining the names for "backwards compatibility only".

> Maybe it's for debug still, but I could see how userspace may want to hunt down the
> wakelock that's created in userspace and penalize or kill the task
> that's waking up the device.

It can't do that right now.

> I see that wakelock_lookup_add() already checks the list of wakelock
> wakeup sources, but I don't see how I can't create an "alarmtimer"
> wakelock again, but this time for userspace, by writing into
> /sys/power/wake_lock.
>
> What happens with namespaces here BTW? Can a wakelock be made in one
> namespace and that is the same name as another wakelock in a different
> namespace? Right now it doesn't look possible because of the global name
> matching, but it probably makes sense to support this? Maybe we just
> shouldn't make anything in sysfs for wake sources that can be any random
> name created from the wakelock path right now. I don't see how it can be
> traced back to the process that created it in any reasonable way.

It can't.

The assumption was that there would be a "manager" process in user
space controlling access to this interface and it would do its own
tracking.  That predated namespaces though. :-)

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

[Stephen Boyd]

Quoting Rafael J. Wysocki (2019-08-01 15:46:33)
> On Fri, Aug 2, 2019 at 12:11 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Ok. If the device name is going to be something generic like 'wakeupN',
> > then we need to make sure that the wakeup source name is unique.
> > Otherwise, I'm not able to see how userspace will differentiate between
> > two of the same named wakelocks. Before this patch the wakeup source
> > name looks to have been used for debugging, but now it's being used
> > programmatically to let userspace act upon it somehow.
> 
> I'm not actually sure if this patch changes the situation with respect
> to wakeup source names.   User space still can use them for whatever
> it used to use the list in debugfs and that's it.
> 
> That's what I mean by retaining the names for "backwards compatibility only".
> 

Ok, got it. Maybe one day the name attribute will become unimportant if
we have a namespace and process aware wake lock API in userspace.

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

[Rafael J. Wysocki]

On Thu, Aug 1, 2019 at 12:42 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Thursday, August 1, 2019 12:31:16 AM CEST Tri Vo wrote:
> > On Wed, Jul 31, 2019 at 3:17 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Wednesday, July 31, 2019 11:59:32 PM CEST Stephen Boyd wrote:
> > > > Quoting Tri Vo (2019-07-31 14:55:14)
> > > > > +/**
> > > > > + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> > > > > + * @parent: Device given wakeup source is associated with (or NULL if virtual).
> > > > > + * @ws: Wakeup source to be added in sysfs.
> > > > > + */
> > > > > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > > > > +{
> > > > > +       struct device *dev;
> > > > > +       int id;
> > > > > +
> > > > > +       id = ida_alloc(&wakeup_ida, GFP_KERNEL);
> > >
> > > So can anyone remind me why the IDA thing is needed here at all?
> >
> > IDA is used to generate the directory name ("ws%d", ID) that is unique
> > among wakeup_sources. That is what ends up in
> > /sys/class/wakeup/ws<ID>/* path.
>
> That's not my point (see below).
>
> > > > > +       if (id < 0)
> > > > > +               return id;
> > > > > +       ws->id = id;
> > > > > +
> > > > > +       dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > > > > +                                       wakeup_source_groups, "ws%d",
> > > >
> > > > I thought the name was going to still be 'wakeupN'?
> > >
> > > So can't we prefix the wakeup source name with something like "wakeup:" or similar here?
> >
> > "ws%d" here is the name in the sysfs path rather than the name of the
> > wakeup source. Wakeup source name is not altered in this patch.
> >
>
> So why wouldn't something like this suffice:
>
> dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
>                                 wakeup_source_groups, "wakeup:%s", ws->name);
>
> ?

And more generally speaking, we are adding another way to identify
wakeup sources (by id), which is not related to the one we already
have (by name).  Why do we need both of them?