linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
@ 2019-07-30  2:43 Tri Vo
  2019-07-30  5:46 ` Rafael J. Wysocki
  2019-07-30  6:46 ` Greg KH
  0 siblings, 2 replies; 19+ messages in thread
From: Tri Vo @ 2019-07-30  2:43 UTC (permalink / raw)
  To: rjw, gregkh, viresh.kumar
  Cc: rafael, hridya, sspatil, kaleshsingh, ravisadineni, swboyd,
	linux-kernel, linux-pm, kernel-team, Tri Vo, kbuild test robot

Userspace can use wakeup_sources debugfs node to plot history of suspend
blocking wakeup sources over device's boot cycle. This information can
then be used (1) for power-specific bug reporting and (2) towards
attributing battery consumption to specific processes over a period of
time.

However, debugfs doesn't have stable ABI. For this reason, create a
'struct device' to expose wakeup sources statistics in sysfs under
/sys/class/wakeup/wakeup<ID>/*.

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

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

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

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

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

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..1c13daa3f891 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);
 	}
 }
@@ -262,10 +272,13 @@ int device_wakeup_enable(struct device *dev)
 	if (!dev || !dev->power.can_wakeup)
 		return -EINVAL;

+	if (dev->power.wakeup)
+		return 0;
+
 	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..edc0c98e5991
--- /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_simple_get(&wakeup_ida, 0, 0, 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);
+	if (IS_ERR(dev)) {
+		ida_simple_remove(&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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30  2:43 [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs Tri Vo
@ 2019-07-30  5:46 ` Rafael J. Wysocki
  2019-07-30 18:39   ` Tri Vo
  2019-07-30  6:46 ` Greg KH
  1 sibling, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-30  5:46 UTC (permalink / raw)
  To: Tri Vo
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Viresh Kumar,
	Rafael J. Wysocki, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Stephen Boyd, Linux Kernel Mailing List,
	Linux PM, Cc: Android Kernel, kbuild test robot

On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
>
> Userspace can use wakeup_sources debugfs node to plot history of suspend
> blocking wakeup sources over device's boot cycle. This information can
> then be used (1) for power-specific bug reporting and (2) towards
> attributing battery consumption to specific processes over a period of
> time.
>
> However, debugfs doesn't have stable ABI. For this reason, create a
> 'struct device' to expose wakeup sources statistics in sysfs under
> /sys/class/wakeup/wakeup<ID>/*.
>
> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Tri Vo <trong@android.com>
> Tested-by: Tri Vo <trong@android.com>
> Tested-by: Kalesh Singh <kaleshsingh@google.com>
> Reported-by: kbuild test robot <lkp@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-wakeup |  76 +++++++++
>  drivers/acpi/device_pm.c                     |   3 +-
>  drivers/base/power/Makefile                  |   2 +-
>  drivers/base/power/wakeup.c                  |  21 ++-
>  drivers/base/power/wakeup_stats.c            | 171 +++++++++++++++++++
>  fs/eventpoll.c                               |   4 +-
>  include/linux/pm_wakeup.h                    |  15 +-
>  kernel/power/autosleep.c                     |   2 +-
>  kernel/power/wakelock.c                      |  10 ++
>  kernel/time/alarmtimer.c                     |   2 +-
>  10 files changed, 294 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
>  create mode 100644 drivers/base/power/wakeup_stats.c
>
> v2:
> - Updated Documentation/ABI/, as per Greg.
> - Removed locks in attribute functions, as per Greg.
> - Lifetimes of struct wakelock and struck wakeup_source are now different due to
>   the latter embedding a refcounted kobject. Changed it so that struct wakelock
>   only has a pointer to struct wakeup_source, instead of embedding it.
> - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
>   sysfs.
>
> v3:
> Changes by Greg:
> - Reworked code to use 'struct device' instead of raw kobjects.
> - Updated documentation file.
> - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
> Changes by Tri:
> - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
>   operations. So no need to handle lifetimes in wakelock.c
>
> v4:
> - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
> - Moved new documentation to a separate file
>   Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
> - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.
>
> v5:
> - Removed CONFIG_PM_SLEEP_STATS
> - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by
>   kbuild test robot <lkp@intel.com>
> - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to
>   that device's power/wakeup file results in multiple wakeup source being
>   allocated for that device.  Changed device_wakeup_enable() to check if device
>   wakeup was previously enabled.
> Changes by Stephen:
> - Changed stats location from /sys/class/wakeup/<name>/* to
>   /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This
>   avoids name collisions in /sys/class/wakeup/ directory.
> - Added a "name" attribute to wakeup sources, and updated documentation.
> - Device registering the wakeup source is now the parent of the wakeup source.
>   Updated wakeup_source_register()'s signature and its callers accordingly.

And I really don't like these changes.  Especially having "wakeup"
twice in the path.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30  2:43 [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs Tri Vo
  2019-07-30  5:46 ` Rafael J. Wysocki
@ 2019-07-30  6:46 ` Greg KH
  2019-07-30 19:20   ` Tri Vo
  1 sibling, 1 reply; 19+ messages in thread
From: Greg KH @ 2019-07-30  6:46 UTC (permalink / raw)
  To: Tri Vo
  Cc: rjw, viresh.kumar, rafael, hridya, sspatil, kaleshsingh,
	ravisadineni, swboyd, linux-kernel, linux-pm, kernel-team,
	kbuild test robot

On Mon, Jul 29, 2019 at 07:43:09PM -0700, Tri Vo wrote:
> Userspace can use wakeup_sources debugfs node to plot history of suspend
> blocking wakeup sources over device's boot cycle. This information can
> then be used (1) for power-specific bug reporting and (2) towards
> attributing battery consumption to specific processes over a period of
> time.
> 
> However, debugfs doesn't have stable ABI. For this reason, create a
> 'struct device' to expose wakeup sources statistics in sysfs under
> /sys/class/wakeup/wakeup<ID>/*.

I agree with Rafael here, no need for the extra "wakeup" in the device
name as you are in the "wakeup" namespace already.

If you have an IDA-allocated name, there's no need for the extra
'wakeup' at all.

> +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> +{
> +	struct device *dev;
> +	int id;
> +
> +	id = ida_simple_get(&wakeup_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0)
> +		return id;

No lock needed for this ida?  Are you sure?

> +	ws->id = id;
> +
> +	dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> +					wakeup_source_groups, "wakeup%d",
> +					ws->id);
> +	if (IS_ERR(dev)) {
> +		ida_simple_remove(&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);

Again, no lock, is that ok?  I think ida's can work without a lock, but
not always, sorry, I don't remember the rules anymore given the recent
changes in that code.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30  5:46 ` Rafael J. Wysocki
@ 2019-07-30 18:39   ` Tri Vo
  2019-07-30 18:48     ` Stephen Boyd
  2019-07-30 18:49     ` Greg Kroah-Hartman
  0 siblings, 2 replies; 19+ messages in thread
From: Tri Vo @ 2019-07-30 18:39 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Stephen Boyd, Linux Kernel Mailing List,
	Linux PM, Cc: Android Kernel, kbuild test robot

On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> >
> > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > blocking wakeup sources over device's boot cycle. This information can
> > then be used (1) for power-specific bug reporting and (2) towards
> > attributing battery consumption to specific processes over a period of
> > time.
> >
> > However, debugfs doesn't have stable ABI. For this reason, create a
> > 'struct device' to expose wakeup sources statistics in sysfs under
> > /sys/class/wakeup/wakeup<ID>/*.
> >
> > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Tri Vo <trong@android.com>
> > Tested-by: Tri Vo <trong@android.com>
> > Tested-by: Kalesh Singh <kaleshsingh@google.com>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-wakeup |  76 +++++++++
> >  drivers/acpi/device_pm.c                     |   3 +-
> >  drivers/base/power/Makefile                  |   2 +-
> >  drivers/base/power/wakeup.c                  |  21 ++-
> >  drivers/base/power/wakeup_stats.c            | 171 +++++++++++++++++++
> >  fs/eventpoll.c                               |   4 +-
> >  include/linux/pm_wakeup.h                    |  15 +-
> >  kernel/power/autosleep.c                     |   2 +-
> >  kernel/power/wakelock.c                      |  10 ++
> >  kernel/time/alarmtimer.c                     |   2 +-
> >  10 files changed, 294 insertions(+), 12 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
> >  create mode 100644 drivers/base/power/wakeup_stats.c
> >
> > v2:
> > - Updated Documentation/ABI/, as per Greg.
> > - Removed locks in attribute functions, as per Greg.
> > - Lifetimes of struct wakelock and struck wakeup_source are now different due to
> >   the latter embedding a refcounted kobject. Changed it so that struct wakelock
> >   only has a pointer to struct wakeup_source, instead of embedding it.
> > - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
> >   sysfs.
> >
> > v3:
> > Changes by Greg:
> > - Reworked code to use 'struct device' instead of raw kobjects.
> > - Updated documentation file.
> > - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
> > Changes by Tri:
> > - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
> >   operations. So no need to handle lifetimes in wakelock.c
> >
> > v4:
> > - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
> > - Moved new documentation to a separate file
> >   Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
> > - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.
> >
> > v5:
> > - Removed CONFIG_PM_SLEEP_STATS
> > - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by
> >   kbuild test robot <lkp@intel.com>
> > - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to
> >   that device's power/wakeup file results in multiple wakeup source being
> >   allocated for that device.  Changed device_wakeup_enable() to check if device
> >   wakeup was previously enabled.
> > Changes by Stephen:
> > - Changed stats location from /sys/class/wakeup/<name>/* to
> >   /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This
> >   avoids name collisions in /sys/class/wakeup/ directory.
> > - Added a "name" attribute to wakeup sources, and updated documentation.
> > - Device registering the wakeup source is now the parent of the wakeup source.
> >   Updated wakeup_source_register()'s signature and its callers accordingly.
>
> And I really don't like these changes.  Especially having "wakeup"
> twice in the path.

I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?

About the other change, I think making the registering device the
parent of the wakeup source is a worthwhile change, since that way one
can associate a wakeup source sysfs entry with the device that created
it.
>
> Couldn't you find a simpler way to avoid the name collisions?

I could also simply log an error in case of a name collision instead
of failing hard. That way I can keep the old path with the wakeup
source name in it. Other than that, I can't think of a way to resolve
the directory name collisions without making that directory name
unique, i.e. generating IDs is probably the simplest way. I'm still
learning about the kernel, and I might be wrong though. What do you
think?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 18:39   ` Tri Vo
@ 2019-07-30 18:48     ` Stephen Boyd
  2019-07-30 18:51       ` Greg Kroah-Hartman
  2019-07-30 22:17       ` Rafael J. Wysocki
  2019-07-30 18:49     ` Greg Kroah-Hartman
  1 sibling, 2 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-07-30 18:48 UTC (permalink / raw)
  To: Rafael J. Wysocki, Tri Vo
  Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

Quoting Tri Vo (2019-07-30 11:39:34)
> On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > - Device registering the wakeup source is now the parent of the wakeup source.
> > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> >
> > And I really don't like these changes.  Especially having "wakeup"
> > twice in the path.
> 
> I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?

Using the same prefix for the class and the device name is quite common.
For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
class name could be changed to wakeup_source perhaps (i.e.
/sys/class/wakeup_source/wakeupN)?


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 18:39   ` Tri Vo
  2019-07-30 18:48     ` Stephen Boyd
@ 2019-07-30 18:49     ` Greg Kroah-Hartman
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-30 18:49 UTC (permalink / raw)
  To: Tri Vo
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Stephen Boyd, Linux Kernel Mailing List,
	Linux PM, Cc: Android Kernel, kbuild test robot

On Tue, Jul 30, 2019 at 11:39:34AM -0700, Tri Vo wrote:
> On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > >
> > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > blocking wakeup sources over device's boot cycle. This information can
> > > then be used (1) for power-specific bug reporting and (2) towards
> > > attributing battery consumption to specific processes over a period of
> > > time.
> > >
> > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > /sys/class/wakeup/wakeup<ID>/*.
> > >
> > > Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > Signed-off-by: Tri Vo <trong@android.com>
> > > Tested-by: Tri Vo <trong@android.com>
> > > Tested-by: Kalesh Singh <kaleshsingh@google.com>
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-wakeup |  76 +++++++++
> > >  drivers/acpi/device_pm.c                     |   3 +-
> > >  drivers/base/power/Makefile                  |   2 +-
> > >  drivers/base/power/wakeup.c                  |  21 ++-
> > >  drivers/base/power/wakeup_stats.c            | 171 +++++++++++++++++++
> > >  fs/eventpoll.c                               |   4 +-
> > >  include/linux/pm_wakeup.h                    |  15 +-
> > >  kernel/power/autosleep.c                     |   2 +-
> > >  kernel/power/wakelock.c                      |  10 ++
> > >  kernel/time/alarmtimer.c                     |   2 +-
> > >  10 files changed, 294 insertions(+), 12 deletions(-)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
> > >  create mode 100644 drivers/base/power/wakeup_stats.c
> > >
> > > v2:
> > > - Updated Documentation/ABI/, as per Greg.
> > > - Removed locks in attribute functions, as per Greg.
> > > - Lifetimes of struct wakelock and struck wakeup_source are now different due to
> > >   the latter embedding a refcounted kobject. Changed it so that struct wakelock
> > >   only has a pointer to struct wakeup_source, instead of embedding it.
> > > - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
> > >   sysfs.
> > >
> > > v3:
> > > Changes by Greg:
> > > - Reworked code to use 'struct device' instead of raw kobjects.
> > > - Updated documentation file.
> > > - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
> > > Changes by Tri:
> > > - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
> > >   operations. So no need to handle lifetimes in wakelock.c
> > >
> > > v4:
> > > - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
> > > - Moved new documentation to a separate file
> > >   Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
> > > - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.
> > >
> > > v5:
> > > - Removed CONFIG_PM_SLEEP_STATS
> > > - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by
> > >   kbuild test robot <lkp@intel.com>
> > > - Stephen reported that a call to device_init_wakeup() and writing 'enabled' to
> > >   that device's power/wakeup file results in multiple wakeup source being
> > >   allocated for that device.  Changed device_wakeup_enable() to check if device
> > >   wakeup was previously enabled.
> > > Changes by Stephen:
> > > - Changed stats location from /sys/class/wakeup/<name>/* to
> > >   /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This
> > >   avoids name collisions in /sys/class/wakeup/ directory.
> > > - Added a "name" attribute to wakeup sources, and updated documentation.
> > > - Device registering the wakeup source is now the parent of the wakeup source.
> > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> >
> > And I really don't like these changes.  Especially having "wakeup"
> > twice in the path.
> 
> I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?

Yes.

> About the other change, I think making the registering device the
> parent of the wakeup source is a worthwhile change, since that way one
> can associate a wakeup source sysfs entry with the device that created
> it.

that's fine with me.

> > Couldn't you find a simpler way to avoid the name collisions?
> 
> I could also simply log an error in case of a name collision instead
> of failing hard. That way I can keep the old path with the wakeup
> source name in it. Other than that, I can't think of a way to resolve
> the directory name collisions without making that directory name
> unique, i.e. generating IDs is probably the simplest way. I'm still
> learning about the kernel, and I might be wrong though. What do you
> think?

Uniqe ids for the class should be fine, that's all you need to have.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 18:48     ` Stephen Boyd
@ 2019-07-30 18:51       ` Greg Kroah-Hartman
  2019-07-30 22:17       ` Rafael J. Wysocki
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-30 18:51 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Tue, Jul 30, 2019 at 11:48:09AM -0700, Stephen Boyd wrote:
> Quoting Tri Vo (2019-07-30 11:39:34)
> > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> > >
> > > And I really don't like these changes.  Especially having "wakeup"
> > > twice in the path.
> > 
> > I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?
> 
> Using the same prefix for the class and the device name is quite common.
> For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> class name could be changed to wakeup_source perhaps (i.e.
> /sys/class/wakeup_source/wakeupN)?
> 

Ah, the issue is that these end up on the virtual "bus".  Yeah, sorry,
you are right, they need to have a unique prefix in order to prevent
name collisions.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30  6:46 ` Greg KH
@ 2019-07-30 19:20   ` Tri Vo
  0 siblings, 0 replies; 19+ messages in thread
From: Tri Vo @ 2019-07-30 19:20 UTC (permalink / raw)
  To: Greg KH
  Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Stephen Boyd, LKML, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Mon, Jul 29, 2019 at 11:47 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 29, 2019 at 07:43:09PM -0700, Tri Vo wrote:
> > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > blocking wakeup sources over device's boot cycle. This information can
> > then be used (1) for power-specific bug reporting and (2) towards
> > attributing battery consumption to specific processes over a period of
> > time.
> >
> > However, debugfs doesn't have stable ABI. For this reason, create a
> > 'struct device' to expose wakeup sources statistics in sysfs under
> > /sys/class/wakeup/wakeup<ID>/*.
>
> I agree with Rafael here, no need for the extra "wakeup" in the device
> name as you are in the "wakeup" namespace already.
>
> If you have an IDA-allocated name, there's no need for the extra
> 'wakeup' at all.
>
> > +int wakeup_source_sysfs_add(struct device *parent, struct wakeup_source *ws)
> > +{
> > +     struct device *dev;
> > +     int id;
> > +
> > +     id = ida_simple_get(&wakeup_ida, 0, 0, GFP_KERNEL);
> > +     if (id < 0)
> > +             return id;
>
> No lock needed for this ida?  Are you sure?
>
> > +     ws->id = id;
> > +
> > +     dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> > +                                     wakeup_source_groups, "wakeup%d",
> > +                                     ws->id);
> > +     if (IS_ERR(dev)) {
> > +             ida_simple_remove(&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);
>
> Again, no lock, is that ok?  I think ida's can work without a lock, but
> not always, sorry, I don't remember the rules anymore given the recent
> changes in that code.

Documentation says, "The IDA handles its own locking. It is safe to
call any of the IDA functions without synchronisation in your code."
https://www.kernel.org/doc/html/latest/core-api/idr.html#ida-usage

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 18:48     ` Stephen Boyd
  2019-07-30 18:51       ` Greg Kroah-Hartman
@ 2019-07-30 22:17       ` Rafael J. Wysocki
  2019-07-30 22:26         ` Stephen Boyd
  1 sibling, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-30 22:17 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> Quoting Tri Vo (2019-07-30 11:39:34)
> > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> > >
> > > And I really don't like these changes.  Especially having "wakeup"
> > > twice in the path.
> > 
> > I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?
> 
> Using the same prefix for the class and the device name is quite common.
> For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> class name could be changed to wakeup_source perhaps (i.e.
> /sys/class/wakeup_source/wakeupN)?

Alternatively /sys/class/wakeup/wsN





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 22:17       ` Rafael J. Wysocki
@ 2019-07-30 22:26         ` Stephen Boyd
  2019-07-30 23:05           ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2019-07-30 22:26 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > Quoting Tri Vo (2019-07-30 11:39:34)
> > > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> > > >
> > > > And I really don't like these changes.  Especially having "wakeup"
> > > > twice in the path.
> > > 
> > > I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?
> > 
> > Using the same prefix for the class and the device name is quite common.
> > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > class name could be changed to wakeup_source perhaps (i.e.
> > /sys/class/wakeup_source/wakeupN)?
> 
> Alternatively /sys/class/wakeup/wsN
> 

Or /sys/class/wakeup/eventN? It's your bikeshed to paint.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 22:26         ` Stephen Boyd
@ 2019-07-30 23:05           ` Rafael J. Wysocki
  2019-07-30 23:31             ` Tri Vo
  2019-07-30 23:41             ` Stephen Boyd
  0 siblings, 2 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-30 23:05 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman,
	Viresh Kumar, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > Quoting Tri Vo (2019-07-30 11:39:34)
> > > > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > > > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > > > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> > > > >
> > > > > And I really don't like these changes.  Especially having "wakeup"
> > > > > twice in the path.
> > > >
> > > > I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?
> > >
> > > Using the same prefix for the class and the device name is quite common.
> > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > > class name could be changed to wakeup_source perhaps (i.e.
> > > /sys/class/wakeup_source/wakeupN)?
> >
> > Alternatively /sys/class/wakeup/wsN
> >
>
> Or /sys/class/wakeup/eventN? It's your bikeshed to paint.

So actually the underlying problem here is that device_wakeup_enable()
tries to register a wakeup source and then attach it to the device to
avoid calling possibly sleeping functions under a spinlock.

However, it should be possible to call wakeup_source_create(name)
first, then attach the wakeup source to the device (after checking for
presence), and then invoke wakeup_source_add() (after dropping the
lock).  If the wakeup source virtual device registration is done in
wakeup_source_add(), that should avoid the problem altogether without
having to introduce extra complexity.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 23:05           ` Rafael J. Wysocki
@ 2019-07-30 23:31             ` Tri Vo
  2019-07-30 23:41             ` Stephen Boyd
  1 sibling, 0 replies; 19+ messages in thread
From: Tri Vo @ 2019-07-30 23:31 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Stephen Boyd, Rafael J. Wysocki, Greg Kroah-Hartman,
	Viresh Kumar, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Tue, Jul 30, 2019 at 4:06 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > > Quoting Tri Vo (2019-07-30 11:39:34)
> > > > > On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <trong@android.com> wrote:
> > > > > > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > > > > >   Updated wakeup_source_register()'s signature and its callers accordingly.
> > > > > >
> > > > > > And I really don't like these changes.  Especially having "wakeup"
> > > > > > twice in the path.
> > > > >
> > > > > I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?
> > > >
> > > > Using the same prefix for the class and the device name is quite common.
> > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > > > class name could be changed to wakeup_source perhaps (i.e.
> > > > /sys/class/wakeup_source/wakeupN)?
> > >
> > > Alternatively /sys/class/wakeup/wsN
> > >
> >
> > Or /sys/class/wakeup/eventN? It's your bikeshed to paint.
>
> So actually the underlying problem here is that device_wakeup_enable()
> tries to register a wakeup source and then attach it to the device to
> avoid calling possibly sleeping functions under a spinlock.
>
> However, it should be possible to call wakeup_source_create(name)
> first, then attach the wakeup source to the device (after checking for
> presence), and then invoke wakeup_source_add() (after dropping the
> lock).  If the wakeup source virtual device registration is done in
> wakeup_source_add(), that should avoid the problem altogether without
> having to introduce extra complexity.

This addresses the issue with device_wakeup_enable(), but IIUC we
still have the general problem of multiple devices having the same
name, which leads to name collisions when identifying wakeup sources
with the name only.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 23:05           ` Rafael J. Wysocki
  2019-07-30 23:31             ` Tri Vo
@ 2019-07-30 23:41             ` Stephen Boyd
  2019-07-31  8:34               ` Rafael J. Wysocki
  1 sibling, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2019-07-30 23:41 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman,
	Viresh Kumar, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

Quoting Rafael J. Wysocki (2019-07-30 16:05:55)
> On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > >
> > > > Using the same prefix for the class and the device name is quite common.
> > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > > > class name could be changed to wakeup_source perhaps (i.e.
> > > > /sys/class/wakeup_source/wakeupN)?
> > >
> > > Alternatively /sys/class/wakeup/wsN
> > >
> >
> > Or /sys/class/wakeup/eventN? It's your bikeshed to paint.
> 
> So actually the underlying problem here is that device_wakeup_enable()
> tries to register a wakeup source and then attach it to the device to
> avoid calling possibly sleeping functions under a spinlock.

Agreed, that is one problem.

> 
> However, it should be possible to call wakeup_source_create(name)
> first, then attach the wakeup source to the device (after checking for
> presence), and then invoke wakeup_source_add() (after dropping the
> lock).  If the wakeup source virtual device registration is done in
> wakeup_source_add(), that should avoid the problem altogether without
> having to introduce extra complexity.

While reordering the code to do what you describe will fix this specific
duplicate name problem, it won't fix the general problem with reusing
device names from one bus on a different bus/class. We can run into the
same problem when two buses name their devices the same name and then we
attempt to attach a wakeup source to those two devices. Or we can have a
problem where a virtual wakeup is made with the same name, and again
we'll try to make a duplicate named device. Using something like 'event'
or 'wakeup' or 'ws' as the prefix avoids this problem and keeps things
clean.

We should probably avoid letting the same virtual wakeup source be made
with the same name anyway, because userspace will be confused about what
virtual wakeup it is otherwise. I concede that using the name of the
wakeup source catches this problem without adding extra code.

Either way, I'd like to see what you outline implemented so that we
don't need to do more work than is necessary when userspace writes to
the file. I just don't want to see us need to change the name of the
wakeup device later on and then add a 'name' attribute to the class so
that we can avoid name collisions due to various buses controlling the
string we use to create the name of the wakeup device.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-30 23:41             ` Stephen Boyd
@ 2019-07-31  8:34               ` Rafael J. Wysocki
  2019-07-31 11:58                 ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-31  8:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman,
	Viresh Kumar, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-07-30 16:05:55)
> > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > > >
> > > > > Using the same prefix for the class and the device name is quite common.
> > > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > > > > class name could be changed to wakeup_source perhaps (i.e.
> > > > > /sys/class/wakeup_source/wakeupN)?
> > > >
> > > > Alternatively /sys/class/wakeup/wsN
> > > >
> > >
> > > Or /sys/class/wakeup/eventN? It's your bikeshed to paint.
> >
> > So actually the underlying problem here is that device_wakeup_enable()
> > tries to register a wakeup source and then attach it to the device to
> > avoid calling possibly sleeping functions under a spinlock.
>
> Agreed, that is one problem.
>
> >
> > However, it should be possible to call wakeup_source_create(name)
> > first, then attach the wakeup source to the device (after checking for
> > presence), and then invoke wakeup_source_add() (after dropping the
> > lock).  If the wakeup source virtual device registration is done in
> > wakeup_source_add(), that should avoid the problem altogether without
> > having to introduce extra complexity.
>
> While reordering the code to do what you describe will fix this specific
> duplicate name problem, it won't fix the general problem with reusing
> device names from one bus on a different bus/class.

Fair enough.

> We can run into the same problem when two buses name their devices the
> same name and then we attempt to attach a wakeup source to those two
> devices. Or we can have a problem where a virtual wakeup is made with
> the same name, and again we'll try to make a duplicate named device.
> Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> problem and keeps things clean.

Or suffix, like "<devname-wakeup>.

But if prefixes are used by an existing convention, I would prefer
"ws-" as it is concise enough and should not be confusing.

> We should probably avoid letting the same virtual wakeup source be made
> with the same name anyway, because userspace will be confused about what
> virtual wakeup it is otherwise. I concede that using the name of the
> wakeup source catches this problem without adding extra code.
>
> Either way, I'd like to see what you outline implemented so that we
> don't need to do more work than is necessary when userspace writes to
> the file.

Since we agree here, let's make this change first.  I can cut a patch
for that in a reasonable time frame I think if no one else beats me to
that.

> I just don't want to see us need to change the name of the
> wakeup device later on and then add a 'name' attribute to the class so
> that we can avoid name collisions due to various buses controlling the
> string we use to create the name of the wakeup device.

OK

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-31  8:34               ` Rafael J. Wysocki
@ 2019-07-31 11:58                 ` Rafael J. Wysocki
  2019-07-31 17:13                   ` Stephen Boyd
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-31 11:58 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Tri Vo, Greg Kroah-Hartman, Viresh Kumar, Hridya Valsaraju,
	Sandeep Patil, Kalesh Singh, Ravi Chandra Sadineni,
	Linux Kernel Mailing List, Linux PM, Cc: Android Kernel,
	kbuild test robot

On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-30 16:05:55)
> > > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Rafael J. Wysocki (2019-07-30 15:17:55)
> > > > > On Tuesday, July 30, 2019 8:48:09 PM CEST Stephen Boyd wrote:
> > > > > >
> > > > > > Using the same prefix for the class and the device name is quite common.
> > > > > > For example, see the input, regulator, tty, tpm, remoteproc, hwmon,
> > > > > > extcon classes. I'd prefer it was left as /sys/class/wakeup/wakeupN. The
> > > > > > class name could be changed to wakeup_source perhaps (i.e.
> > > > > > /sys/class/wakeup_source/wakeupN)?
> > > > >
> > > > > Alternatively /sys/class/wakeup/wsN
> > > > >
> > > >
> > > > Or /sys/class/wakeup/eventN? It's your bikeshed to paint.
> > >
> > > So actually the underlying problem here is that device_wakeup_enable()
> > > tries to register a wakeup source and then attach it to the device to
> > > avoid calling possibly sleeping functions under a spinlock.
> >
> > Agreed, that is one problem.
> >
> > >
> > > However, it should be possible to call wakeup_source_create(name)
> > > first, then attach the wakeup source to the device (after checking for
> > > presence), and then invoke wakeup_source_add() (after dropping the
> > > lock).  If the wakeup source virtual device registration is done in
> > > wakeup_source_add(), that should avoid the problem altogether without
> > > having to introduce extra complexity.
> >
> > While reordering the code to do what you describe will fix this specific
> > duplicate name problem, it won't fix the general problem with reusing
> > device names from one bus on a different bus/class.
> 
> Fair enough.
> 
> > We can run into the same problem when two buses name their devices the
> > same name and then we attempt to attach a wakeup source to those two
> > devices. Or we can have a problem where a virtual wakeup is made with
> > the same name, and again we'll try to make a duplicate named device.
> > Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> > problem and keeps things clean.
> 
> Or suffix, like "<devname-wakeup>.
> 
> But if prefixes are used by an existing convention, I would prefer
> "ws-" as it is concise enough and should not be confusing.
> 
> > We should probably avoid letting the same virtual wakeup source be made
> > with the same name anyway, because userspace will be confused about what
> > virtual wakeup it is otherwise. I concede that using the name of the
> > wakeup source catches this problem without adding extra code.
> >
> > Either way, I'd like to see what you outline implemented so that we
> > don't need to do more work than is necessary when userspace writes to
> > the file.
> 
> Since we agree here, let's make this change first.  I can cut a patch
> for that in a reasonable time frame I think if no one else beats me to
> that.

So maybe something like the patch below (untested).

---
 drivers/base/power/wakeup.c |   82 +++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 49 deletions(-)

Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -228,27 +228,6 @@ void wakeup_source_unregister(struct wak
 EXPORT_SYMBOL_GPL(wakeup_source_unregister);
 
 /**
- * device_wakeup_attach - Attach a wakeup source object to a device object.
- * @dev: Device to handle.
- * @ws: Wakeup source object to attach to @dev.
- *
- * This causes @dev to be treated as a wakeup device.
- */
-static int device_wakeup_attach(struct device *dev, struct wakeup_source *ws)
-{
-	spin_lock_irq(&dev->power.lock);
-	if (dev->power.wakeup) {
-		spin_unlock_irq(&dev->power.lock);
-		return -EEXIST;
-	}
-	dev->power.wakeup = ws;
-	if (dev->power.wakeirq)
-		device_wakeup_attach_irq(dev, dev->power.wakeirq);
-	spin_unlock_irq(&dev->power.lock);
-	return 0;
-}
-
-/**
  * device_wakeup_enable - Enable given device to be a wakeup source.
  * @dev: Device to handle.
  *
@@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
 	if (pm_suspend_target_state != PM_SUSPEND_ON)
 		dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
 
+	spin_lock_irq(&dev->power.lock);
+
+	if (dev->power.wakeup) {
+		spin_unlock_irq(&dev->power.lock);
+		return -EEXIST;
+	}
+	dev->power.wakeup = ERR_PTR(-EBUSY);
+
+	spin_unlock_irq(&dev->power.lock);
+
 	ws = wakeup_source_register(dev_name(dev));
 	if (!ws)
 		return -ENOMEM;
 
-	ret = device_wakeup_attach(dev, ws);
-	if (ret)
-		wakeup_source_unregister(ws);
+	spin_lock_irq(&dev->power.lock);
 
-	return ret;
+	dev->power.wakeup = ws;
+	if (dev->power.wakeirq)
+		device_wakeup_attach_irq(dev, dev->power.wakeirq);
+
+	spin_unlock_irq(&dev->power.lock);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(device_wakeup_enable);
 
@@ -294,7 +287,7 @@ void device_wakeup_attach_irq(struct dev
 	struct wakeup_source *ws;
 
 	ws = dev->power.wakeup;
-	if (!ws)
+	if (IS_ERR_OR_NULL(ws))
 		return;
 
 	if (ws->wakeirq)
@@ -316,7 +309,7 @@ void device_wakeup_detach_irq(struct dev
 	struct wakeup_source *ws;
 
 	ws = dev->power.wakeup;
-	if (ws)
+	if (!IS_ERR_OR_NULL(ws))
 		ws->wakeirq = NULL;
 }
 
@@ -353,23 +346,6 @@ void device_wakeup_disarm_wake_irqs(void
 }
 
 /**
- * device_wakeup_detach - Detach a device's wakeup source object from it.
- * @dev: Device to detach the wakeup source object from.
- *
- * After it returns, @dev will not be treated as a wakeup device any more.
- */
-static struct wakeup_source *device_wakeup_detach(struct device *dev)
-{
-	struct wakeup_source *ws;
-
-	spin_lock_irq(&dev->power.lock);
-	ws = dev->power.wakeup;
-	dev->power.wakeup = NULL;
-	spin_unlock_irq(&dev->power.lock);
-	return ws;
-}
-
-/**
  * device_wakeup_disable - Do not regard a device as a wakeup source any more.
  * @dev: Device to handle.
  *
@@ -383,8 +359,16 @@ int device_wakeup_disable(struct device
 	if (!dev || !dev->power.can_wakeup)
 		return -EINVAL;
 
-	ws = device_wakeup_detach(dev);
-	wakeup_source_unregister(ws);
+	spin_lock_irq(&dev->power.lock);
+
+	ws = dev->power.wakeup;
+	dev->power.wakeup = NULL;
+
+	spin_unlock_irq(&dev->power.lock);
+
+	if (!IS_ERR(ws))
+		wakeup_source_unregister(ws);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(device_wakeup_disable);
@@ -558,7 +542,7 @@ void __pm_stay_awake(struct wakeup_sourc
 {
 	unsigned long flags;
 
-	if (!ws)
+	if (IS_ERR_OR_NULL(ws))
 		return;
 
 	spin_lock_irqsave(&ws->lock, flags);
@@ -675,7 +659,7 @@ void __pm_relax(struct wakeup_source *ws
 {
 	unsigned long flags;
 
-	if (!ws)
+	if (IS_ERR_OR_NULL(ws))
 		return;
 
 	spin_lock_irqsave(&ws->lock, flags);
@@ -746,7 +730,7 @@ void pm_wakeup_ws_event(struct wakeup_so
 	unsigned long flags;
 	unsigned long expires;
 
-	if (!ws)
+	if (IS_ERR_OR_NULL(ws))
 		return;
 
 	spin_lock_irqsave(&ws->lock, flags);




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-31 11:58                 ` Rafael J. Wysocki
@ 2019-07-31 17:13                   ` Stephen Boyd
  2019-07-31 17:17                     ` Greg Kroah-Hartman
  2019-07-31 21:19                     ` Rafael J. Wysocki
  0 siblings, 2 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-07-31 17:13 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Tri Vo, Greg Kroah-Hartman, Viresh Kumar, Hridya Valsaraju,
	Sandeep Patil, Kalesh Singh, Ravi Chandra Sadineni,
	Linux Kernel Mailing List, Linux PM, Cc: Android Kernel,
	kbuild test robot

Quoting Rafael J. Wysocki (2019-07-31 04:58:36)
> On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> > On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > 
> > > We can run into the same problem when two buses name their devices the
> > > same name and then we attempt to attach a wakeup source to those two
> > > devices. Or we can have a problem where a virtual wakeup is made with
> > > the same name, and again we'll try to make a duplicate named device.
> > > Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> > > problem and keeps things clean.
> > 
> > Or suffix, like "<devname-wakeup>.
> > 
> > But if prefixes are used by an existing convention, I would prefer
> > "ws-" as it is concise enough and should not be confusing.

Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0

> > 
> > > We should probably avoid letting the same virtual wakeup source be made
> > > with the same name anyway, because userspace will be confused about what
> > > virtual wakeup it is otherwise. I concede that using the name of the
> > > wakeup source catches this problem without adding extra code.
> > >
> > > Either way, I'd like to see what you outline implemented so that we
> > > don't need to do more work than is necessary when userspace writes to
> > > the file.
> > 
> > Since we agree here, let's make this change first.  I can cut a patch
> > for that in a reasonable time frame I think if no one else beats me to
> > that.
> 
> So maybe something like the patch below (untested).
> 
> Index: linux-pm/drivers/base/power/wakeup.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/wakeup.c
> +++ linux-pm/drivers/base/power/wakeup.c
> @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
>         if (pm_suspend_target_state != PM_SUSPEND_ON)
>                 dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
>  
> +       spin_lock_irq(&dev->power.lock);
> +
> +       if (dev->power.wakeup) {
> +               spin_unlock_irq(&dev->power.lock);
> +               return -EEXIST;
> +       }
> +       dev->power.wakeup = ERR_PTR(-EBUSY);
> +
> +       spin_unlock_irq(&dev->power.lock);
> +
>         ws = wakeup_source_register(dev_name(dev));
>         if (!ws)
>                 return -ENOMEM;
>  

Let's say that device_wakeup_enable() is called twice at around the same
time. First thread gets to wakeup_source_register() and it fails, we
return -ENOMEM. dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
thread is at the spin_lock_irq() above, it grabs the lock and sees
dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
-EEXIST. I'd think we would want to try to create the wakeup source
instead.

    CPU0                                   CPU1
    ----                                   ----
    spin_lock_irq(&dev->power.lock)
    ...
    dev->power.wakeup = ERR_PTR(-EBUSY)
    spin_unlock_irq(&dev->power.lock)
    ws = wakeup_source_register(...)
    if (!ws)
        return -ENOMEM;                 spin_lock_irq(&dev->power.lock)
                                        if (dev->power.wakeup)
                                            return -EEXIST; // Bad


Similar problems probably exist with wakeup destruction racing with
creation. I think it might have to be a create and then publish pointer
style of code to keep the spinlock section small?

> -       ret = device_wakeup_attach(dev, ws);
> -       if (ret)
> -               wakeup_source_unregister(ws);
> +       spin_lock_irq(&dev->power.lock);
>  
> -       return ret;
> +       dev->power.wakeup = ws;
> +       if (dev->power.wakeirq)
> +               device_wakeup_attach_irq(dev, dev->power.wakeirq);
> +
> +       spin_unlock_irq(&dev->power.lock);
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(device_wakeup_enable);
>  

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-31 17:13                   ` Stephen Boyd
@ 2019-07-31 17:17                     ` Greg Kroah-Hartman
  2019-07-31 21:19                     ` Rafael J. Wysocki
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-31 17:17 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Tri Vo, Viresh Kumar, Hridya Valsaraju,
	Sandeep Patil, Kalesh Singh, Ravi Chandra Sadineni,
	Linux Kernel Mailing List, Linux PM, Cc: Android Kernel,
	kbuild test robot

On Wed, Jul 31, 2019 at 10:13:57AM -0700, Stephen Boyd wrote:
> Quoting Rafael J. Wysocki (2019-07-31 04:58:36)
> > On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> > > On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > 
> > > > We can run into the same problem when two buses name their devices the
> > > > same name and then we attempt to attach a wakeup source to those two
> > > > devices. Or we can have a problem where a virtual wakeup is made with
> > > > the same name, and again we'll try to make a duplicate named device.
> > > > Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> > > > problem and keeps things clean.
> > > 
> > > Or suffix, like "<devname-wakeup>.
> > > 
> > > But if prefixes are used by an existing convention, I would prefer
> > > "ws-" as it is concise enough and should not be confusing.
> 
> Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0

"eventX" is a prefix already used by the input subsystem, so you might
run into conflicts here :(

"wakeupX" makes sense, no namespace colisions there at all.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-31 17:13                   ` Stephen Boyd
  2019-07-31 17:17                     ` Greg Kroah-Hartman
@ 2019-07-31 21:19                     ` Rafael J. Wysocki
  2019-07-31 21:23                       ` Tri Vo
  1 sibling, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2019-07-31 21:19 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Rafael J. Wysocki, Tri Vo, Greg Kroah-Hartman, Viresh Kumar,
	Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Wed, Jul 31, 2019 at 7:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rafael J. Wysocki (2019-07-31 04:58:36)
> > On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> > > On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > >
> > > > We can run into the same problem when two buses name their devices the
> > > > same name and then we attempt to attach a wakeup source to those two
> > > > devices. Or we can have a problem where a virtual wakeup is made with
> > > > the same name, and again we'll try to make a duplicate named device.
> > > > Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> > > > problem and keeps things clean.
> > >
> > > Or suffix, like "<devname-wakeup>.
> > >
> > > But if prefixes are used by an existing convention, I would prefer
> > > "ws-" as it is concise enough and should not be confusing.
>
> Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0
>
> > >
> > > > We should probably avoid letting the same virtual wakeup source be made
> > > > with the same name anyway, because userspace will be confused about what
> > > > virtual wakeup it is otherwise. I concede that using the name of the
> > > > wakeup source catches this problem without adding extra code.
> > > >
> > > > Either way, I'd like to see what you outline implemented so that we
> > > > don't need to do more work than is necessary when userspace writes to
> > > > the file.
> > >
> > > Since we agree here, let's make this change first.  I can cut a patch
> > > for that in a reasonable time frame I think if no one else beats me to
> > > that.
> >
> > So maybe something like the patch below (untested).
> >
> > Index: linux-pm/drivers/base/power/wakeup.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/wakeup.c
> > +++ linux-pm/drivers/base/power/wakeup.c
> > @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
> >         if (pm_suspend_target_state != PM_SUSPEND_ON)
> >                 dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
> >
> > +       spin_lock_irq(&dev->power.lock);
> > +
> > +       if (dev->power.wakeup) {
> > +               spin_unlock_irq(&dev->power.lock);
> > +               return -EEXIST;
> > +       }
> > +       dev->power.wakeup = ERR_PTR(-EBUSY);
> > +
> > +       spin_unlock_irq(&dev->power.lock);
> > +
> >         ws = wakeup_source_register(dev_name(dev));
> >         if (!ws)
> >                 return -ENOMEM;
> >
>
> Let's say that device_wakeup_enable() is called twice at around the same
> time. First thread gets to wakeup_source_register() and it fails, we
> return -ENOMEM.

The return is premature.  dev->power.wakeup should be reset back to
NULL if the wakeup source creation fails.

> dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
> thread is at the spin_lock_irq() above, it grabs the lock and sees
> dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
> -EEXIST. I'd think we would want to try to create the wakeup source
> instead.
>
>     CPU0                                   CPU1
>     ----                                   ----
>     spin_lock_irq(&dev->power.lock)
>     ...
>     dev->power.wakeup = ERR_PTR(-EBUSY)
>     spin_unlock_irq(&dev->power.lock)
>     ws = wakeup_source_register(...)
>     if (!ws)
>         return -ENOMEM;                 spin_lock_irq(&dev->power.lock)
>                                         if (dev->power.wakeup)
>                                             return -EEXIST; // Bad
>
>
> Similar problems probably exist with wakeup destruction racing with
> creation. I think it might have to be a create and then publish pointer
> style of code to keep the spinlock section small?

There is a problem when there are two concurrent callers of
device_wakeup_enable() running in parallel with a caller of
device_wakeup_disable(), but that can be prevented by an extra check
in the latter.

Apart from that I missed a few if (dev->power.wakeup) checks to convert.

I'll update the patch and resend it.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
  2019-07-31 21:19                     ` Rafael J. Wysocki
@ 2019-07-31 21:23                       ` Tri Vo
  0 siblings, 0 replies; 19+ messages in thread
From: Tri Vo @ 2019-07-31 21:23 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Stephen Boyd, Rafael J. Wysocki, Greg Kroah-Hartman,
	Viresh Kumar, Hridya Valsaraju, Sandeep Patil, Kalesh Singh,
	Ravi Chandra Sadineni, Linux Kernel Mailing List, Linux PM,
	Cc: Android Kernel, kbuild test robot

On Wed, Jul 31, 2019 at 2:19 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jul 31, 2019 at 7:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-31 04:58:36)
> > > On Wednesday, July 31, 2019 10:34:11 AM CEST Rafael J. Wysocki wrote:
> > > > On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > >
> > > > > We can run into the same problem when two buses name their devices the
> > > > > same name and then we attempt to attach a wakeup source to those two
> > > > > devices. Or we can have a problem where a virtual wakeup is made with
> > > > > the same name, and again we'll try to make a duplicate named device.
> > > > > Using something like 'event' or 'wakeup' or 'ws' as the prefix avoids this
> > > > > problem and keeps things clean.
> > > >
> > > > Or suffix, like "<devname-wakeup>.
> > > >
> > > > But if prefixes are used by an existing convention, I would prefer
> > > > "ws-" as it is concise enough and should not be confusing.
> >
> > Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0
> >
> > > >
> > > > > We should probably avoid letting the same virtual wakeup source be made
> > > > > with the same name anyway, because userspace will be confused about what
> > > > > virtual wakeup it is otherwise. I concede that using the name of the
> > > > > wakeup source catches this problem without adding extra code.
> > > > >
> > > > > Either way, I'd like to see what you outline implemented so that we
> > > > > don't need to do more work than is necessary when userspace writes to
> > > > > the file.
> > > >
> > > > Since we agree here, let's make this change first.  I can cut a patch
> > > > for that in a reasonable time frame I think if no one else beats me to
> > > > that.
> > >
> > > So maybe something like the patch below (untested).
> > >
> > > Index: linux-pm/drivers/base/power/wakeup.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/power/wakeup.c
> > > +++ linux-pm/drivers/base/power/wakeup.c
> > > @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
> > >         if (pm_suspend_target_state != PM_SUSPEND_ON)
> > >                 dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);
> > >
> > > +       spin_lock_irq(&dev->power.lock);
> > > +
> > > +       if (dev->power.wakeup) {
> > > +               spin_unlock_irq(&dev->power.lock);
> > > +               return -EEXIST;
> > > +       }
> > > +       dev->power.wakeup = ERR_PTR(-EBUSY);
> > > +
> > > +       spin_unlock_irq(&dev->power.lock);
> > > +
> > >         ws = wakeup_source_register(dev_name(dev));
> > >         if (!ws)
> > >                 return -ENOMEM;
> > >
> >
> > Let's say that device_wakeup_enable() is called twice at around the same
> > time. First thread gets to wakeup_source_register() and it fails, we
> > return -ENOMEM.
>
> The return is premature.  dev->power.wakeup should be reset back to
> NULL if the wakeup source creation fails.
>
> > dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
> > thread is at the spin_lock_irq() above, it grabs the lock and sees
> > dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
> > -EEXIST. I'd think we would want to try to create the wakeup source
> > instead.
> >
> >     CPU0                                   CPU1
> >     ----                                   ----
> >     spin_lock_irq(&dev->power.lock)
> >     ...
> >     dev->power.wakeup = ERR_PTR(-EBUSY)
> >     spin_unlock_irq(&dev->power.lock)
> >     ws = wakeup_source_register(...)
> >     if (!ws)
> >         return -ENOMEM;                 spin_lock_irq(&dev->power.lock)
> >                                         if (dev->power.wakeup)
> >                                             return -EEXIST; // Bad
> >
> >
> > Similar problems probably exist with wakeup destruction racing with
> > creation. I think it might have to be a create and then publish pointer
> > style of code to keep the spinlock section small?
>
> There is a problem when there are two concurrent callers of
> device_wakeup_enable() running in parallel with a caller of
> device_wakeup_disable(), but that can be prevented by an extra check
> in the latter.
>
> Apart from that I missed a few if (dev->power.wakeup) checks to convert.
>
> I'll update the patch and resend it.

Ok thanks, I'll ignore the device_wakeup_enable() issue in this patch,
since you're addressing it in a separate patch.

IIUC checking and assigning to dev->power.wakeup must be in the same
critical section for correctness, implying that allocation of the
wakeup source must also be in that critical section (since we check
dev->power.wakeup to see whether we need a wakeup source).

Wakeup source virtual device registration can fail (it allocates
memory), in which case dev->power.wakeup need to be cleaned up.
Meaning that wakeup source virtual device registration need to be in
that same critical section.

So I'm not sure it is at all possible to satisfy these conditions at
the same time (1) avoid creating an extra wakeup source (2) not hold
the spinlock while creating/registering the wakeup source.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-07-31 21:23 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30  2:43 [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs Tri Vo
2019-07-30  5:46 ` Rafael J. Wysocki
2019-07-30 18:39   ` Tri Vo
2019-07-30 18:48     ` Stephen Boyd
2019-07-30 18:51       ` Greg Kroah-Hartman
2019-07-30 22:17       ` Rafael J. Wysocki
2019-07-30 22:26         ` Stephen Boyd
2019-07-30 23:05           ` Rafael J. Wysocki
2019-07-30 23:31             ` Tri Vo
2019-07-30 23:41             ` Stephen Boyd
2019-07-31  8:34               ` Rafael J. Wysocki
2019-07-31 11:58                 ` Rafael J. Wysocki
2019-07-31 17:13                   ` Stephen Boyd
2019-07-31 17:17                     ` Greg Kroah-Hartman
2019-07-31 21:19                     ` Rafael J. Wysocki
2019-07-31 21:23                       ` Tri Vo
2019-07-30 18:49     ` Greg Kroah-Hartman
2019-07-30  6:46 ` Greg KH
2019-07-30 19:20   ` Tri Vo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).