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

[Stephen Boyd <swboyd@chromium.org>]

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);
 	}