Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 2:19 PM Rafael J. Wysocki wrote:
>
> On Wed, Jul 31, 2019 at 7:14 PM 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
> > > > 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 ".
> > > >
> > > > But if prefixes are used by an existing convention, I would prefer
> > > > "ws-" as it is concise enough and should not be confusing.
> >
> > Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0
> >
> > > >
> > > > > We should probably avoid letting the same virtual wakeup source be
> > > > > made
> > > > > with the same name anyway, because userspace will be confused about
> > > > > what
> > > > > virtual wakeup it is otherwise. I concede that using the name of the
> > > > > wakeup source catches this problem without adding extra code.
> > > > >
> > > > > Either way, I'd like to see what you outline implemented so that we
> > > > > don't need to do more work than is necessary when userspace writes to
> > > > > the file.
> > > >
> > > > Since we agree here, let's make this change first. I can cut a patch
> > > > for that in a reasonable time frame I think if no one else beats me to
> > > > that.
> > >
> > > So maybe something like the patch below (untested).
> > >
> > > Index: linux-pm/drivers/base/power/wakeup.c
> > > ===
> > > --- linux-pm.orig/drivers/base/power/wakeup.c
> > > +++ linux-pm/drivers/base/power/wakeup.c
> > > @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
> > > if (pm_suspend_target_state != PM_SUSPEND_ON)
> > > dev_dbg(dev, "Suspicious %s() during system
> > > transition!\n", __func__);
> > >
> > > + spin_lock_irq(&dev->power.lock);
> > > +
> > > + if (dev->power.wakeup) {
> > > + spin_unlock_irq(&dev->power.lock);
> > > + return -EEXIST;
> > > + }
> > > + dev->power.wakeup = ERR_PTR(-EBUSY);
> > > +
> > > + spin_unlock_irq(&dev->power.lock);
> > > +
> > > ws = wakeup_source_register(dev_name(dev));
> > > if (!ws)
> > > return -ENOMEM;
> > >
> >
> > Let's say that device_wakeup_enable() is called twice at around the same
> > time. First thread gets to wakeup_source_register() and it fails, we
> > return -ENOMEM.
>
> The return is premature. dev->power.wakeup should be reset back to
> NULL if the wakeup source creation fails.
>
> > dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
> > thread is at the spin_lock_irq() above, it grabs the lock and sees
> > dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
> > -EEXIST. I'd think we would want to try to create the wakeup source
> > instead.
> >
> > CPU0 CPU1
> >
> > spin_lock_irq(&dev->power.lock)
> > ...
> > dev->power.wakeup = ERR_PTR(-EBUSY)
> > spin_unlock_irq(&dev->power.lock)
> > ws = wakeup_source_register(...)
> > if (!ws)
> > return -ENOMEM; spin_lock_irq(&dev->power.lock)
> > if (dev->power.wakeup)
> > return -EEXIST; // Bad
> >
> >
> > Similar problems probably exist with wakeup destruction racing with
> > creation. I think it might have to be a create and then publish pointer
> > style of code to keep the spinlock section small?
>
> There is a problem when there are two concurrent callers of
> device_wakeup_enable() running in parallel with a caller of
> device_wakeup_disable(), but that can be prevented by an extra check
> in the latter.
>
> Apart from that I missed a few if (dev->power.wakeup) checks to convert.
>
> I'll update the patch and resend it.
Ok thanks, I'll ignore the device_wakeup_enable() issue in this patch,
since you're addressing it in a separate patch.
IIUC checking and assigning to dev->power.wakeup must be in the same
critical section for correctness, implying that allocation of the
wakeup source must also be in that critical section (since we check
dev->power.wakeup to see whether we need a wakeup source).
Wakeup source virtual device registration can fail (it allocates
memory), in which case dev->power.wakeup need to be cleaned up.
Meaning that wakeup source virtual device registration need to be in
that same critical section.
S
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 7:14 PM 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 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 ".
> > >
> > > But if prefixes are used by an existing convention, I would prefer
> > > "ws-" as it is concise enough and should not be confusing.
>
> Another possibility is 'eventN', so it reads as /sys/class/wakeup/event0
>
> > >
> > > > We should probably avoid letting the same virtual wakeup source be made
> > > > with the same name anyway, because userspace will be confused about what
> > > > virtual wakeup it is otherwise. I concede that using the name of the
> > > > wakeup source catches this problem without adding extra code.
> > > >
> > > > Either way, I'd like to see what you outline implemented so that we
> > > > don't need to do more work than is necessary when userspace writes to
> > > > the file.
> > >
> > > Since we agree here, let's make this change first. I can cut a patch
> > > for that in a reasonable time frame I think if no one else beats me to
> > > that.
> >
> > So maybe something like the patch below (untested).
> >
> > Index: linux-pm/drivers/base/power/wakeup.c
> > ===
> > --- linux-pm.orig/drivers/base/power/wakeup.c
> > +++ linux-pm/drivers/base/power/wakeup.c
> > @@ -265,15 +244,29 @@ int device_wakeup_enable(struct device *
> > if (pm_suspend_target_state != PM_SUSPEND_ON)
> > dev_dbg(dev, "Suspicious %s() during system transition!\n",
> > __func__);
> >
> > + spin_lock_irq(&dev->power.lock);
> > +
> > + if (dev->power.wakeup) {
> > + spin_unlock_irq(&dev->power.lock);
> > + return -EEXIST;
> > + }
> > + dev->power.wakeup = ERR_PTR(-EBUSY);
> > +
> > + spin_unlock_irq(&dev->power.lock);
> > +
> > ws = wakeup_source_register(dev_name(dev));
> > if (!ws)
> > return -ENOMEM;
> >
>
> Let's say that device_wakeup_enable() is called twice at around the same
> time. First thread gets to wakeup_source_register() and it fails, we
> return -ENOMEM.
The return is premature. dev->power.wakeup should be reset back to
NULL if the wakeup source creation fails.
> dev->power.wakeup is assigned to ERR_PTR(-EBUSY). Second
> thread is at the spin_lock_irq() above, it grabs the lock and sees
> dev->power.wakeup is ERR_PTR(-EBUSY) so it bails out with return
> -EEXIST. I'd think we would want to try to create the wakeup source
> instead.
>
> CPU0 CPU1
>
> spin_lock_irq(&dev->power.lock)
> ...
> dev->power.wakeup = ERR_PTR(-EBUSY)
> spin_unlock_irq(&dev->power.lock)
> ws = wakeup_source_register(...)
> if (!ws)
> return -ENOMEM; spin_lock_irq(&dev->power.lock)
> if (dev->power.wakeup)
> return -EEXIST; // Bad
>
>
> Similar problems probably exist with wakeup destruction racing with
> creation. I think it might have to be a create and then publish pointer
> style of code to keep the spinlock section small?
There is a problem when there are two concurrent callers of
device_wakeup_enable() running in parallel with a caller of
device_wakeup_disable(), but that can be prevented by an extra check
in the latter.
Apart from that I missed a few if (dev->power.wakeup) checks to convert.
I'll update the patch and resend it.
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 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 ". > > > > > > 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
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 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 ".
> >
> > 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);
>
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 wrote:
> >
> > Quoting Rafael J. Wysocki (2019-07-30 16:05:55)
> > > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd 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 ".
>
> 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_i
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 1:41 AM Stephen Boyd wrote: > > Quoting Rafael J. Wysocki (2019-07-30 16:05:55) > > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd 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 ". 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
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 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.
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jul 30, 2019 at 4:06 PM Rafael J. Wysocki wrote: > > On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd 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 > > > > > wrote: > > > > > > > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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.
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Wed, Jul 31, 2019 at 12:26 AM Stephen Boyd 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 > > > > wrote: > > > > > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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.
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 > > > wrote: > > > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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.
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 > > wrote: > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Mon, Jul 29, 2019 at 11:47 PM Greg KH 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/*.
>
> 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
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 > > wrote: > > > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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
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 wrote: > > > > On Tue, Jul 30, 2019 at 4:45 AM 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/*. > > > > > > Co-developed-by: Greg Kroah-Hartman > > > Signed-off-by: Greg Kroah-Hartman > > > Co-developed-by: Stephen Boyd > > > Signed-off-by: Stephen Boyd > > > Signed-off-by: Tri Vo > > > Tested-by: Tri Vo > > > Tested-by: Kalesh Singh > > > Reported-by: kbuild test robot > > > --- > > > 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 > > > - 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//* to > > > /sys/class/wakeup/wakeup/*, 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//. 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
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 wrote: > > > > On Tue, Jul 30, 2019 at 4:45 AM Tri Vo 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//. 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)?
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Mon, Jul 29, 2019 at 10:46 PM Rafael J. Wysocki wrote: > > On Tue, Jul 30, 2019 at 4:45 AM 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/*. > > > > Co-developed-by: Greg Kroah-Hartman > > Signed-off-by: Greg Kroah-Hartman > > Co-developed-by: Stephen Boyd > > Signed-off-by: Stephen Boyd > > Signed-off-by: Tri Vo > > Tested-by: Tri Vo > > Tested-by: Kalesh Singh > > Reported-by: kbuild test robot > > --- > > 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 > > - 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//* to > > /sys/class/wakeup/wakeup/*, 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//. 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?
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/*.
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
Re: [PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
On Tue, Jul 30, 2019 at 4:45 AM 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/*. > > Co-developed-by: Greg Kroah-Hartman > Signed-off-by: Greg Kroah-Hartman > Co-developed-by: Stephen Boyd > Signed-off-by: Stephen Boyd > Signed-off-by: Tri Vo > Tested-by: Tri Vo > Tested-by: Kalesh Singh > Reported-by: kbuild test robot > --- > 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 > - 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//* to > /sys/class/wakeup/wakeup/*, 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?
[PATCH v5] PM / wakeup: show wakeup sources stats in sysfs
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/*. Co-developed-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman Co-developed-by: Stephen Boyd Signed-off-by: Stephen Boyd Signed-off-by: Tri Vo Tested-by: Tri Vo Tested-by: Kalesh Singh Reported-by: kbuild test robot --- 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 - 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//* to /sys/class/wakeup/wakeup/*, where ID is an IDA-allocated integer. This avoids name collisions in /sys/class/wakeup/ directory. - Added a "name" attribute to wakeup sources, and updated documentation. - Device registering the wakeup source is now the parent of the wakeup source. Updated wakeup_source_register()'s signature and its callers accordingly. diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup new file mode 100644 index ..754aab8b6dcd --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-wakeup @@ -0,0 +1,76 @@ +What: /sys/class/wakeup/ +Date: June 2019 +Contact: Tri Vo +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 +Description: + This file contains the name of the wakeup source. + +What: /sys/class/wakeup/.../active_count +Date: June 2019 +Contact: Tri Vo +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 +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 +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 +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 +Description: + This file
