On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <[email protected]> wrote: > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo <[email protected]> wrote: > > > > Userspace can use wakeup_sources debugfs node to plot history of suspend > > blocking wakeup sources over device's boot cycle. This information can > > then be used (1) for power-specific bug reporting and (2) towards > > attributing battery consumption to specific processes over a period of > > time. > > > > However, debugfs doesn't have stable ABI. For this reason, create a > > 'struct device' to expose wakeup sources statistics in sysfs under > > /sys/class/wakeup/wakeup<ID>/*. > > > > Co-developed-by: Greg Kroah-Hartman <[email protected]> > > Signed-off-by: Greg Kroah-Hartman <[email protected]> > > Co-developed-by: Stephen Boyd <[email protected]> > > Signed-off-by: Stephen Boyd <[email protected]> > > Signed-off-by: Tri Vo <[email protected]> > > Tested-by: Tri Vo <[email protected]> > > Tested-by: Kalesh Singh <[email protected]> > > Reported-by: kbuild test robot <[email protected]> > > --- > > Documentation/ABI/testing/sysfs-class-wakeup | 76 +++++++++ > > drivers/acpi/device_pm.c | 3 +- > > drivers/base/power/Makefile | 2 +- > > drivers/base/power/wakeup.c | 21 ++- > > drivers/base/power/wakeup_stats.c | 171 +++++++++++++++++++ > > fs/eventpoll.c | 4 +- > > include/linux/pm_wakeup.h | 15 +- > > kernel/power/autosleep.c | 2 +- > > kernel/power/wakelock.c | 10 ++ > > kernel/time/alarmtimer.c | 2 +- > > 10 files changed, 294 insertions(+), 12 deletions(-) > > create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup > > create mode 100644 drivers/base/power/wakeup_stats.c > > > > v2: > > - Updated Documentation/ABI/, as per Greg. > > - Removed locks in attribute functions, as per Greg. > > - Lifetimes of struct wakelock and struck wakeup_source are now different > > due to > > the latter embedding a refcounted kobject. Changed it so that struct > > wakelock > > only has a pointer to struct wakeup_source, instead of embedding it. > > - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source > > statistics in > > sysfs. > > > > v3: > > Changes by Greg: > > - Reworked code to use 'struct device' instead of raw kobjects. > > - Updated documentation file. > > - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled. > > Changes by Tri: > > - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject > > operations. So no need to handle lifetimes in wakelock.c > > > > v4: > > - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message. > > - Moved new documentation to a separate file > > Documentation/ABI/testing/sysfs-class-wakeup, as per Greg. > > - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg. > > > > v5: > > - Removed CONFIG_PM_SLEEP_STATS > > - Used PTR_ERR_OR_ZERO instead of if(IS_ERR(...)) + PTR_ERR, reported by > > kbuild test robot <[email protected]> > > - Stephen reported that a call to device_init_wakeup() and writing > > 'enabled' to > > that device's power/wakeup file results in multiple wakeup source being > > allocated for that device. Changed device_wakeup_enable() to check if > > device > > wakeup was previously enabled. > > Changes by Stephen: > > - Changed stats location from /sys/class/wakeup/<name>/* to > > /sys/class/wakeup/wakeup<ID>/*, where ID is an IDA-allocated integer. This > > avoids name collisions in /sys/class/wakeup/ directory. > > - Added a "name" attribute to wakeup sources, and updated documentation. > > - Device registering the wakeup source is now the parent of the wakeup > > source. > > Updated wakeup_source_register()'s signature and its callers accordingly. > > And I really don't like these changes. Especially having "wakeup" > twice in the path.
I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good? About the other change, I think making the registering device the parent of the wakeup source is a worthwhile change, since that way one can associate a wakeup source sysfs entry with the device that created it. > > Couldn't you find a simpler way to avoid the name collisions? I could also simply log an error in case of a name collision instead of failing hard. That way I can keep the old path with the wakeup source name in it. Other than that, I can't think of a way to resolve the directory name collisions without making that directory name unique, i.e. generating IDs is probably the simplest way. I'm still learning about the kernel, and I might be wrong though. What do you think?
