[PATCH] power: sysfs: Remove wakeup_abort_count attribute.

[PATCH] power: sysfs: Remove wakeup_abort_count attribute.

[Ravi Chandra Sadineni]

wakeup_abort_count and wakeup_count sysfs entries print the
same (wakeup_count) attribute. This patch removes the duplicate
wakeup_abort_count sysfs entry.

Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
---
 Documentation/ABI/testing/sysfs-devices-power | 25 ++++++-------------
 drivers/base/power/sysfs.c                    | 19 --------------
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
index 80a00f7b6667..1ca04b4f0489 100644
--- a/Documentation/ABI/testing/sysfs-devices-power
+++ b/Documentation/ABI/testing/sysfs-devices-power
@@ -81,12 +81,13 @@ What:		/sys/devices/.../power/wakeup_count
 Date:		September 2010
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
 Description:
-		The /sys/devices/.../wakeup_count attribute contains the number
-		of signaled wakeup events associated with the device.  This
-		attribute is read-only.  If the device is not capable to wake up
-		the system from sleep states, this attribute is not present.
-		If the device is not enabled to wake up the system from sleep
-		states, this attribute is empty.
+		The /sys/devices/.../wakeup_count attribute contains the
+		number of times the processing of a wakeup event associated with
+		the device might have aborted system transition into a sleep
+		state in progress. This attribute is read-only. If the device
+		is not capable to wake up the system from sleep states, this
+		attribute is not present. If the device is not enabled to wake
+		up the system from sleep states, this attribute is empty.
 
 What:		/sys/devices/.../power/wakeup_active_count
 Date:		September 2010
@@ -100,18 +101,6 @@ Description:
 		the device is not enabled to wake up the system from sleep
 		states, this attribute is empty.
 
-What:		/sys/devices/.../power/wakeup_abort_count
-Date:		February 2012
-Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
-Description:
-		The /sys/devices/.../wakeup_abort_count attribute contains the
-		number of times the processing of a wakeup event associated with
-		the device might have aborted system transition into a sleep
-		state in progress.  This attribute is read-only.  If the device
-		is not capable to wake up the system from sleep states, this
-		attribute is not present.  If the device is not enabled to wake
-		up the system from sleep states, this attribute is empty.
-
 What:		/sys/devices/.../power/wakeup_expire_count
 Date:		February 2012
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 1b9c281cbe41..f42044d9711c 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -375,24 +375,6 @@ static ssize_t wakeup_active_count_show(struct device *dev,
 
 static DEVICE_ATTR_RO(wakeup_active_count);
 
-static ssize_t wakeup_abort_count_show(struct device *dev,
-				       struct device_attribute *attr,
-				       char *buf)
-{
-	unsigned long count = 0;
-	bool enabled = false;
-
-	spin_lock_irq(&dev->power.lock);
-	if (dev->power.wakeup) {
-		count = dev->power.wakeup->wakeup_count;
-		enabled = true;
-	}
-	spin_unlock_irq(&dev->power.lock);
-	return enabled ? sprintf(buf, "%lu\n", count) : sprintf(buf, "\n");
-}
-
-static DEVICE_ATTR_RO(wakeup_abort_count);
-
 static ssize_t wakeup_expire_count_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -580,7 +562,6 @@ static struct attribute *wakeup_attrs[] = {
 	&dev_attr_wakeup.attr,
 	&dev_attr_wakeup_count.attr,
 	&dev_attr_wakeup_active_count.attr,
-	&dev_attr_wakeup_abort_count.attr,
 	&dev_attr_wakeup_expire_count.attr,
 	&dev_attr_wakeup_active.attr,
 	&dev_attr_wakeup_total_time_ms.attr,
-- 
2.20.1

Re: [PATCH] power: sysfs: Remove wakeup_abort_count attribute.

[Greg KH]

On Mon, Jul 22, 2019 at 10:04:07AM -0700, Ravi Chandra Sadineni wrote:
> wakeup_abort_count and wakeup_count sysfs entries print the
> same (wakeup_count) attribute. This patch removes the duplicate
> wakeup_abort_count sysfs entry.
> 
> Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
> ---
>  Documentation/ABI/testing/sysfs-devices-power | 25 ++++++-------------
>  drivers/base/power/sysfs.c                    | 19 --------------
>  2 files changed, 7 insertions(+), 37 deletions(-)

Isn't this the attribute you add back in a later patch?

If you have 2 patches that touch the same file, always make a patch
series so we have a chance to guess as to what patch comes first.

thanks,

greg k-h

[PATCH 0/2] power: Refactor device level sysfs.

[Ravi Chandra Sadineni]

wakeup_abort_count and wakeup_count attributes print the
same (wakeup_count) variable. Thus this patchset removes the
duplicate wakeup_abort_count sysfs attribute. This patchset also
exposes event_count as a sysfs attribute.

Ravi Chandra Sadineni (2):
  power: sysfs: Remove wakeup_abort_count attribute.
  power:sysfs: Expose device wakeup_event_count.

 Documentation/ABI/testing/sysfs-devices-power | 36 +++++++++----------
 drivers/base/power/sysfs.c                    | 27 +++++++-------
 2 files changed, 33 insertions(+), 30 deletions(-)

-- 
2.20.1

[PATCH 1/2 V2] power: sysfs: Remove wakeup_abort_count attribute.

[Ravi Chandra Sadineni]

wakeup_abort_count and wakeup_count sysfs entries print the
same (wakeup_count) attribute. This patch removes the duplicate
wakeup_abort_count sysfs entry.

Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
---
 Documentation/ABI/testing/sysfs-devices-power | 25 ++++++-------------
 drivers/base/power/sysfs.c                    | 19 --------------
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
index 80a00f7b6667..1ca04b4f0489 100644
--- a/Documentation/ABI/testing/sysfs-devices-power
+++ b/Documentation/ABI/testing/sysfs-devices-power
@@ -81,12 +81,13 @@ What:		/sys/devices/.../power/wakeup_count
 Date:		September 2010
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
 Description:
-		The /sys/devices/.../wakeup_count attribute contains the number
-		of signaled wakeup events associated with the device.  This
-		attribute is read-only.  If the device is not capable to wake up
-		the system from sleep states, this attribute is not present.
-		If the device is not enabled to wake up the system from sleep
-		states, this attribute is empty.
+		The /sys/devices/.../wakeup_count attribute contains the
+		number of times the processing of a wakeup event associated with
+		the device might have aborted system transition into a sleep
+		state in progress. This attribute is read-only. If the device
+		is not capable to wake up the system from sleep states, this
+		attribute is not present. If the device is not enabled to wake
+		up the system from sleep states, this attribute is empty.
 
 What:		/sys/devices/.../power/wakeup_active_count
 Date:		September 2010
@@ -100,18 +101,6 @@ Description:
 		the device is not enabled to wake up the system from sleep
 		states, this attribute is empty.
 
-What:		/sys/devices/.../power/wakeup_abort_count
-Date:		February 2012
-Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
-Description:
-		The /sys/devices/.../wakeup_abort_count attribute contains the
-		number of times the processing of a wakeup event associated with
-		the device might have aborted system transition into a sleep
-		state in progress.  This attribute is read-only.  If the device
-		is not capable to wake up the system from sleep states, this
-		attribute is not present.  If the device is not enabled to wake
-		up the system from sleep states, this attribute is empty.
-
 What:		/sys/devices/.../power/wakeup_expire_count
 Date:		February 2012
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 1b9c281cbe41..f42044d9711c 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -375,24 +375,6 @@ static ssize_t wakeup_active_count_show(struct device *dev,
 
 static DEVICE_ATTR_RO(wakeup_active_count);
 
-static ssize_t wakeup_abort_count_show(struct device *dev,
-				       struct device_attribute *attr,
-				       char *buf)
-{
-	unsigned long count = 0;
-	bool enabled = false;
-
-	spin_lock_irq(&dev->power.lock);
-	if (dev->power.wakeup) {
-		count = dev->power.wakeup->wakeup_count;
-		enabled = true;
-	}
-	spin_unlock_irq(&dev->power.lock);
-	return enabled ? sprintf(buf, "%lu\n", count) : sprintf(buf, "\n");
-}
-
-static DEVICE_ATTR_RO(wakeup_abort_count);
-
 static ssize_t wakeup_expire_count_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -580,7 +562,6 @@ static struct attribute *wakeup_attrs[] = {
 	&dev_attr_wakeup.attr,
 	&dev_attr_wakeup_count.attr,
 	&dev_attr_wakeup_active_count.attr,
-	&dev_attr_wakeup_abort_count.attr,
 	&dev_attr_wakeup_expire_count.attr,
 	&dev_attr_wakeup_active.attr,
 	&dev_attr_wakeup_total_time_ms.attr,
-- 
2.20.1

Re: [PATCH 1/2 V2] power: sysfs: Remove wakeup_abort_count attribute.

[Greg KH]

On Mon, Jul 22, 2019 at 03:33:36PM -0700, Ravi Chandra Sadineni wrote:
> wakeup_abort_count and wakeup_count sysfs entries print the
> same (wakeup_count) attribute. This patch removes the duplicate
> wakeup_abort_count sysfs entry.

Are you sure that userspace is not using this "duplicate" entry?  Have
you searched userspace tools to verify this?

thanks,

greg k-h

[PATCH 2/2 V2] power:sysfs: Expose device wakeup_event_count.

[Ravi Chandra Sadineni]

Device level event_count can help user level daemon to track if a
praticular device has seen an wake interrupt during a suspend resume
cycle. Thus expose it via sysfs.

Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
---
V2: Address comments from patchset 1.

Documentation/ABI/testing/sysfs-devices-power | 11 ++++++++++
 drivers/base/power/sysfs.c                    | 21 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
index 1ca04b4f0489..abae0e8106d2 100644
--- a/Documentation/ABI/testing/sysfs-devices-power
+++ b/Documentation/ABI/testing/sysfs-devices-power
@@ -89,6 +89,17 @@ Description:
 		attribute is not present. If the device is not enabled to wake
 		up the system from sleep states, this attribute is empty.
 
+What:		/sys/devices/.../power/wakeup_event_count
+Date:		July 2019
+Contact:	Ravi Chandra sadineni <ravisadineni@chromium.org>
+Description:
+		The /sys/devices/.../wakeup_event_count attribute contains the
+		number of signaled wakeup events associated with the device.
+		This attribute is read-only. If the device is not capable to
+		wake up the system from sleep states, this attribute is not
+		present. If the device is not enabled to wake up the system
+		from sleep states, this attribute returns an empty line.
+
 What:		/sys/devices/.../power/wakeup_active_count
 Date:		September 2010
 Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index f42044d9711c..cbb9768b10e8 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -357,6 +357,26 @@ static ssize_t wakeup_count_show(struct device *dev,
 
 static DEVICE_ATTR_RO(wakeup_count);
 
+static ssize_t wakeup_event_count_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	unsigned long count = 0;
+	bool enabled = false;
+
+	spin_lock_irq(&dev->power.lock);
+	if (dev->power.wakeup) {
+		count = dev->power.wakeup->event_count;
+		enabled = true;
+	}
+	spin_unlock_irq(&dev->power.lock);
+	if (enabled)
+		return sprintf(buf, "%lu\n", count);
+	else
+		return sprintf(buf, "\n");
+}
+
+static DEVICE_ATTR_RO(wakeup_event_count);
+
 static ssize_t wakeup_active_count_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
@@ -561,6 +581,7 @@ static struct attribute *wakeup_attrs[] = {
 #ifdef CONFIG_PM_SLEEP
 	&dev_attr_wakeup.attr,
 	&dev_attr_wakeup_count.attr,
+	&dev_attr_wakeup_event_count.attr,
 	&dev_attr_wakeup_active_count.attr,
 	&dev_attr_wakeup_expire_count.attr,
 	&dev_attr_wakeup_active.attr,
-- 
2.20.1

Re: [PATCH 2/2 V2] power:sysfs: Expose device wakeup_event_count.

[Greg KH]

On Mon, Jul 22, 2019 at 03:33:37PM -0700, Ravi Chandra Sadineni wrote:
> Device level event_count can help user level daemon to track if a
> praticular device has seen an wake interrupt during a suspend resume
> cycle. Thus expose it via sysfs.
> 
> Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
> ---
> V2: Address comments from patchset 1.

You didn't address my comment about locks :(

Re: [PATCH 0/2] power: Refactor device level sysfs.

[Rafael J. Wysocki]

On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni
<ravisadineni@chromium.org> wrote:
>
> wakeup_abort_count and wakeup_count attributes print the
> same (wakeup_count) variable. Thus this patchset removes the
> duplicate wakeup_abort_count sysfs attribute. This patchset also
> exposes event_count as a sysfs attribute.
>
> Ravi Chandra Sadineni (2):
>   power: sysfs: Remove wakeup_abort_count attribute.
>   power:sysfs: Expose device wakeup_event_count.

I don't think you need this at all, because
https://patchwork.kernel.org/patch/11045069/ is exposing what you need
already.

Thanks!

Re: [PATCH 0/2] power: Refactor device level sysfs.

[Ravi Chandra Sadineni]

Hi Greg,

https://patchwork.kernel.org/patch/11045069/ seems to create a virtual
device under wakeup class with the same name as the actual device. I
don't see a way to reliably map these virtual devices to the actual
device sysfs node. For example if we have to know if a particular
input device has triggered a wake event, we have to look for a virtual
device under /sys/class/wakeup with the same name. I am afraid that
depending just on the name might be too risky as there can be multiple
devices under different buses with the same name.  Am I missing
something?

Thanks,
Ravi

On Tue, Jul 23, 2019 at 12:44 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni
> <ravisadineni@chromium.org> wrote:
> >
> > wakeup_abort_count and wakeup_count attributes print the
> > same (wakeup_count) variable. Thus this patchset removes the
> > duplicate wakeup_abort_count sysfs attribute. This patchset also
> > exposes event_count as a sysfs attribute.
> >
> > Ravi Chandra Sadineni (2):
> >   power: sysfs: Remove wakeup_abort_count attribute.
> >   power:sysfs: Expose device wakeup_event_count.
>
> I don't think you need this at all, because
> https://patchwork.kernel.org/patch/11045069/ is exposing what you need
> already.
>
> Thanks!

Re: [PATCH 0/2] power: Refactor device level sysfs.

[Rafael J. Wysocki]

On Tue, Jul 23, 2019 at 6:57 PM Ravi Chandra Sadineni
<ravisadineni@chromium.org> wrote:
>
> Hi Greg,
>
> https://patchwork.kernel.org/patch/11045069/ seems to create a virtual
> device under wakeup class with the same name as the actual device. I
> don't see a way to reliably map these virtual devices to the actual
> device sysfs node. For example if we have to know if a particular
> input device has triggered a wake event, we have to look for a virtual
> device under /sys/class/wakeup with the same name. I am afraid that
> depending just on the name might be too risky as there can be multiple
> devices under different buses with the same name.  Am I missing
> something?

There can be a symlink (say "wakeup_source") from under the actual
device to the virtual wakeup one associated with it.

Then we can advise everybody to use the symlink for the stats and
deprecate the stats attributes under the actual device going forward.
:-)

I have a plan to cut a patch to add such a symlink, but you can try to
beat me to that if you want.

> On Tue, Jul 23, 2019 at 12:44 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni
> > <ravisadineni@chromium.org> wrote:
> > >
> > > wakeup_abort_count and wakeup_count attributes print the
> > > same (wakeup_count) variable. Thus this patchset removes the
> > > duplicate wakeup_abort_count sysfs attribute. This patchset also
> > > exposes event_count as a sysfs attribute.
> > >
> > > Ravi Chandra Sadineni (2):
> > >   power: sysfs: Remove wakeup_abort_count attribute.
> > >   power:sysfs: Expose device wakeup_event_count.
> >
> > I don't think you need this at all, because
> > https://patchwork.kernel.org/patch/11045069/ is exposing what you need
> > already.
> >
> > Thanks!

Re: [PATCH 0/2] power: Refactor device level sysfs.

[Ravi Chandra Sadineni]

Thanks Rafael. I will abandon this patch set and try to create a
symlink as you suggested.

Thanks,
Ravi

On Tue, Jul 23, 2019 at 10:02 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Jul 23, 2019 at 6:57 PM Ravi Chandra Sadineni
> <ravisadineni@chromium.org> wrote:
> >
> > Hi Greg,
> >
> > https://patchwork.kernel.org/patch/11045069/ seems to create a virtual
> > device under wakeup class with the same name as the actual device. I
> > don't see a way to reliably map these virtual devices to the actual
> > device sysfs node. For example if we have to know if a particular
> > input device has triggered a wake event, we have to look for a virtual
> > device under /sys/class/wakeup with the same name. I am afraid that
> > depending just on the name might be too risky as there can be multiple
> > devices under different buses with the same name.  Am I missing
> > something?
>
> There can be a symlink (say "wakeup_source") from under the actual
> device to the virtual wakeup one associated with it.
>
> Then we can advise everybody to use the symlink for the stats and
> deprecate the stats attributes under the actual device going forward.
> :-)
>
> I have a plan to cut a patch to add such a symlink, but you can try to
> beat me to that if you want.
>
> > On Tue, Jul 23, 2019 at 12:44 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni
> > > <ravisadineni@chromium.org> wrote:
> > > >
> > > > wakeup_abort_count and wakeup_count attributes print the
> > > > same (wakeup_count) variable. Thus this patchset removes the
> > > > duplicate wakeup_abort_count sysfs attribute. This patchset also
> > > > exposes event_count as a sysfs attribute.
> > > >
> > > > Ravi Chandra Sadineni (2):
> > > >   power: sysfs: Remove wakeup_abort_count attribute.
> > > >   power:sysfs: Expose device wakeup_event_count.
> > >
> > > I don't think you need this at all, because
> > > https://patchwork.kernel.org/patch/11045069/ is exposing what you need
> > > already.
> > >
> > > Thanks!