LinuxLists.cc - [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs

[permalink] [raw]

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

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.

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

2019-07-30 10:21:24

[permalink] [raw]

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

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

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

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

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

No lock needed for this ida? Are you sure?

> + ws->id = id;
> +
> + dev = device_create_with_groups(wakeup_class, parent, MKDEV(0, 0), ws,
> + wakeup_source_groups, "wakeup%d",
> + ws->id);
> + if (IS_ERR(dev)) {
> + ida_simple_remove(&wakeup_ida, ws->id);
> + return PTR_ERR(dev);
> + }
> +
> + ws->dev = dev;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> +
> +/**
> + * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
> + * @ws: Wakeup source to be removed from sysfs.
> + */
> +void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> +{
> + device_unregister(ws->dev);
> + ida_simple_remove(&wakeup_ida, ws->id);

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

thanks,

greg k-h

2019-07-30 18:50:04

[permalink] [raw]

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

On Tue, Jul 30, 2019 at 11:39:34AM -0700, Tri Vo wrote:
> On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki <[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?

Yes.

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

that's fine with me.

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

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

thanks,

greg k-h

2019-07-30 18:50:19

[permalink] [raw]

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

Quoting Tri Vo (2019-07-30 11:39:34)
> 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:
> > > - Device registering the wakeup source is now the parent of the wakeup source.
> > > Updated wakeup_source_register()'s signature and its callers accordingly.
> >
> > And I really don't like these changes. Especially having "wakeup"
> > twice in the path.
>
> I can trim it down to /sys/class/wakeup/<ID>/. Does that sound good?

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

2019-07-30 18:52:07

[permalink] [raw]

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

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

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

thanks,

greg k-h

2019-07-30 19:21:25

by Tri Vo

[permalink] [raw]

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

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

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

2019-07-30 22:17:23

by Tri Vo

[permalink] [raw]

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

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?

2019-07-31 00:13:14

[permalink] [raw]

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

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

Alternatively /sys/class/wakeup/wsN

2019-07-31 00:22:31

[permalink] [raw]

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

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

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

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

2019-07-31 00:23:02

[permalink] [raw]

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

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

Agreed, that is one problem.

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

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

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

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

2019-07-31 00:24:03

by Tri Vo

[permalink] [raw]

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

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

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

2019-07-31 03:15:49

[permalink] [raw]

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

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

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

2019-07-31 08:43:48

[permalink] [raw]

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

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

Fair enough.

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

Or suffix, like "<devname-wakeup>.

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

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

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

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

OK

2019-07-31 14:13:13

[permalink] [raw]

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

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

So maybe something like the patch below (untested).

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

Index: linux-pm/drivers/base/power/wakeup.c
===================================================================
--- linux-pm.orig/drivers/base/power/wakeup.c
+++ linux-pm/drivers/base/power/wakeup.c
@@ -228,27 +228,6 @@ void wakeup_source_unregister(struct wak
EXPORT_SYMBOL_GPL(wakeup_source_unregister);

/**
- * device_wakeup_attach - Attach a wakeup source object to a device object.
- * @dev: Device to handle.
- * @ws: Wakeup source object to attach to @dev.
- *
- * This causes @dev to be treated as a wakeup device.
- */
-static int device_wakeup_attach(struct device *dev, struct wakeup_source *ws)
-{
- spin_lock_irq(&dev->power.lock);
- if (dev->power.wakeup) {
- spin_unlock_irq(&dev->power.lock);
- return -EEXIST;
- }
- dev->power.wakeup = ws;
- if (dev->power.wakeirq)
- device_wakeup_attach_irq(dev, dev->power.wakeirq);
- spin_unlock_irq(&dev->power.lock);
- return 0;
-}
-
-/**
* device_wakeup_enable - Enable given device to be a wakeup source.
* @dev: Device to handle.
*
@@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
if (pm_suspend_target_state != PM_SUSPEND_ON)
dev_dbg(dev, "Suspicious %s() during system transition!\n", __func__);

+ spin_lock_irq(&dev->power.lock);
+
+ if (dev->power.wakeup) {
+ spin_unlock_irq(&dev->power.lock);
+ return -EEXIST;
+ }
+ dev->power.wakeup = ERR_PTR(-EBUSY);
+
+ spin_unlock_irq(&dev->power.lock);
+
ws = wakeup_source_register(dev_name(dev));
if (!ws)
return -ENOMEM;

- ret = device_wakeup_attach(dev, ws);
- if (ret)
- wakeup_source_unregister(ws);
+ spin_lock_irq(&dev->power.lock);

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

@@ -294,7 +287,7 @@ void device_wakeup_attach_irq(struct dev
struct wakeup_source *ws;

ws = dev->power.wakeup;
- if (!ws)
+ if (IS_ERR_OR_NULL(ws))
return;

if (ws->wakeirq)
@@ -316,7 +309,7 @@ void device_wakeup_detach_irq(struct dev
struct wakeup_source *ws;

ws = dev->power.wakeup;
- if (ws)
+ if (!IS_ERR_OR_NULL(ws))
ws->wakeirq = NULL;
}

@@ -353,23 +346,6 @@ void device_wakeup_disarm_wake_irqs(void
}

/**
- * device_wakeup_detach - Detach a device's wakeup source object from it.
- * @dev: Device to detach the wakeup source object from.
- *
- * After it returns, @dev will not be treated as a wakeup device any more.
- */
-static struct wakeup_source *device_wakeup_detach(struct device *dev)
-{
- struct wakeup_source *ws;
-
- spin_lock_irq(&dev->power.lock);
- ws = dev->power.wakeup;
- dev->power.wakeup = NULL;
- spin_unlock_irq(&dev->power.lock);
- return ws;
-}
-
-/**
* device_wakeup_disable - Do not regard a device as a wakeup source any more.
* @dev: Device to handle.
*
@@ -383,8 +359,16 @@ int device_wakeup_disable(struct device
if (!dev || !dev->power.can_wakeup)
return -EINVAL;

- ws = device_wakeup_detach(dev);
- wakeup_source_unregister(ws);
+ spin_lock_irq(&dev->power.lock);
+
+ ws = dev->power.wakeup;
+ dev->power.wakeup = NULL;
+
+ spin_unlock_irq(&dev->power.lock);
+
+ if (!IS_ERR(ws))
+ wakeup_source_unregister(ws);
+
return 0;
}
EXPORT_SYMBOL_GPL(device_wakeup_disable);
@@ -558,7 +542,7 @@ void __pm_stay_awake(struct wakeup_sourc
{
unsigned long flags;

- if (!ws)
+ if (IS_ERR_OR_NULL(ws))
return;

spin_lock_irqsave(&ws->lock, flags);
@@ -675,7 +659,7 @@ void __pm_relax(struct wakeup_source *ws
{
unsigned long flags;

- if (!ws)
+ if (IS_ERR_OR_NULL(ws))
return;

spin_lock_irqsave(&ws->lock, flags);
@@ -746,7 +730,7 @@ void pm_wakeup_ws_event(struct wakeup_so
unsigned long flags;
unsigned long expires;

- if (!ws)
+ if (IS_ERR_OR_NULL(ws))
return;

spin_lock_irqsave(&ws->lock, flags);

2019-07-31 19:24:26

[permalink] [raw]

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

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

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

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

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

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

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

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

2019-07-31 19:24:40

[permalink] [raw]

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

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

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

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

thanks,

greg k-h

2019-07-31 22:20:15