Message ID | 20190731215514.212215-1-trong@android.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [v6] PM / wakeup: show wakeup sources stats in sysfs | expand |
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.
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
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?
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.
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); ?
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.
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?
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.
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.
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); }
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);
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 >
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.
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.
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?
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
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.
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.
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.
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).
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.
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".
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. :-)
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.
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.
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?
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".
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) {