[PATCH] platform/chrome: chromeos_tbmc : Report wake events.
Mark chromeos_tbmc as wake capable and report wake events. This helps to
abort suspend on seeing a tablet mode switch event when kernel is
suspending. This also helps identifying if chroemos_tbmc is the wake
source.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/platform/chrome/chromeos_tbmc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/platform/chrome/chromeos_tbmc.c
b/drivers/platform/chrome/chromeos_tbmc.c
index ce259ec9f990..d1cf8f3463ce 100644
--- a/drivers/platform/chrome/chromeos_tbmc.c
+++ b/drivers/platform/chrome/chromeos_tbmc.c
@@ -47,6 +47,7 @@ static __maybe_unused int chromeos_tbmc_resume(struct device
*dev)
static void chromeos_tbmc_notify(struct acpi_device *adev, u32 event)
{
+ acpi_pm_wakeup_event(>dev);
switch (event) {
case 0x80:
chromeos_tbmc_query_switch(adev, adev->driver_data);
@@ -90,6 +91,7 @@ static int chromeos_tbmc_add(struct acpi_device *adev)
dev_err(dev, "cannot register input device\n");
return ret;
}
+ device_init_wakeup(dev, true);
return 0;
}
--
2.21.0
[PATCH 0/2] Add wakeup_source symlink and update documentation.
As discussed in https://lkml.org/lkml/2019/7/23/687 this patch set creates symlink from device node to wakeup_source virtual device. This patch set also updates the documentation accordingly. Ravi Chandra Sadineni (2): power: sysfs: Add link to wakeup class device. power: sysfs: move wakeup related nodes in power dir to obselete. Documentation/ABI/obsolete/sysfs-device-power | 93 ++ Documentation/ABI/testing/sysfs-devices-power | 94 --- drivers/base/power/power.h| 2 + drivers/base/power/sysfs.c| 25 + drivers/base/power/wakeup.c | 2 + 5 files changed, 122 insertions(+), 94 deletions(-) create mode 100644 Documentation/ABI/obsolete/sysfs-device-power -- 2.20.1
[PATCH 1/2] power: sysfs: Add link to wakeup class device.
https://patchwork.kernel.org/patch/11045069/ creates a virtual device
under wakeup class for each wake capable device exposing all related
sysfs attributes. But there isn't a symlink from the actual device
node to these virtual devices. This patch creates a symlink from the
actual device to the corresponding wakeup_source device under wakeup
class.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/base/power/power.h | 2 ++
drivers/base/power/sysfs.c | 25 +
drivers/base/power/wakeup.c | 2 ++
3 files changed, 29 insertions(+)
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index c511def48b48..32b0f5c080a9 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -67,6 +67,8 @@ extern void dpm_sysfs_remove(struct device *dev);
extern void rpm_sysfs_remove(struct device *dev);
extern int wakeup_sysfs_add(struct device *dev);
extern void wakeup_sysfs_remove(struct device *dev);
+extern void wakeup_source_sysfs_link_add(struct device *dev);
+extern void wakeup_source_sysfs_link_remove(struct device *dev);
extern int pm_qos_sysfs_add_resume_latency(struct device *dev);
extern void pm_qos_sysfs_remove_resume_latency(struct device *dev);
extern int pm_qos_sysfs_add_flags(struct device *dev);
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index d713738ce796..fbbdb7b16ac5 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -95,6 +95,7 @@
const char power_group_name[] = "power";
EXPORT_SYMBOL_GPL(power_group_name);
+static const char wakeup_source_symlink_name[] = "wakeup_source";
static const char ctrl_auto[] = "auto";
static const char ctrl_on[] = "on";
@@ -679,6 +680,30 @@ int dpm_sysfs_add(struct device *dev)
return rc;
}
+void wakeup_source_sysfs_link_add(struct device *dev)
+{
+ struct wakeup_source *ws;
+ int err;
+
+ ws = dev->power.wakeup;
+ if (ws && ws->dev) {
+ err = sysfs_add_link_to_group(>kobj, power_group_name,
+ >dev->kobj, wakeup_source_symlink_name);
+ if (err) {
+ dev_err(dev,
+ "could not add %s symlink err %d\n",
+ wakeup_source_symlink_name,
+ err);
+ }
+ }
+}
+
+void wakeup_source_sysfs_link_remove(struct device *dev)
+{
+ sysfs_remove_link_from_group(>kobj, power_group_name,
+ wakeup_source_symlink_name);
+}
+
int wakeup_sysfs_add(struct device *dev)
{
return sysfs_merge_group(>kobj, _wakeup_attr_group);
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index fe779fe13a7f..87dfe401b035 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -270,6 +270,7 @@ static int device_wakeup_attach(struct device *dev, struct
wakeup_source *ws)
if (dev->power.wakeirq)
device_wakeup_attach_irq(dev, dev->power.wakeirq);
spin_unlock_irq(>power.lock);
+ wakeup_source_sysfs_link_add(dev);
return 0;
}
@@ -391,6 +392,7 @@ static struct wakeup_source *device_wakeup_detach(struct
device *dev)
ws = dev->power.wakeup;
dev->power.wakeup = NULL;
spin_unlock_irq(>power.lock);
+ wakeup_source_sysfs_link_remove(dev);
return ws;
}
--
2.20.1
[PATCH 2/2] power: sysfs: move wakeup related nodes in power dir to obselete.
As we have a new wakeup_source sub directory under power/ that exposes all the wakeup_source related attributes, move the documentation pointing to the existing attributes directly under power/ directory to obselete. Signed-off-by: Ravi Chandra Sadineni --- Documentation/ABI/obsolete/sysfs-device-power | 93 Documentation/ABI/testing/sysfs-devices-power | 102 ++ 2 files changed, 101 insertions(+), 94 deletions(-) create mode 100644 Documentation/ABI/obsolete/sysfs-device-power diff --git a/Documentation/ABI/obsolete/sysfs-device-power b/Documentation/ABI/obsolete/sysfs-device-power new file mode 100644 index ..fd780cdcf2c3 --- /dev/null +++ b/Documentation/ABI/obsolete/sysfs-device-power @@ -0,0 +1,93 @@ +What: /sys/devices/.../power/wakeup_count +Date: September 2010 +Contact: Rafael J. Wysocki +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. + +What: /sys/devices/.../power/wakeup_active_count +Date: September 2010 +Contact: Rafael J. Wysocki +Description: + The /sys/devices/.../wakeup_active_count attribute contains the + number of times the processing of wakeup events associated with + the device was completed (at the kernel level). 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_abort_count +Date: February 2012 +Contact: Rafael J. Wysocki +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 +Description: + The /sys/devices/.../wakeup_expire_count attribute contains the + number of times a wakeup event associated with the device has + been reported with a timeout that expired. 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_total_time_ms +Date: September 2010 +Contact: Rafael J. Wysocki +Description: + The /sys/devices/.../wakeup_total_time_ms attribute contains + the total time of processing wakeup events associated with the + device, in milliseconds. 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_max_time_ms +Date: September 2010 +Contact: Rafael J. Wysocki +Description: + The /sys/devices/.../wakeup_max_time_ms attribute contains + the maximum time of processing a single wakeup event associated + with the device, in milliseconds. 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_last_time_ms +Date: September 2010 +Contact: Rafael J. Wysocki +Description: + The /sys/devices/.../wakeup_last_time_ms attribute contains + the value of the monotonic clock corresponding to the time of + signaling the last wakeup event associated with the device, in + milliseconds
Re: [PATCH 0/2] power: Refactor device level sysfs.
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 wrote: > > On Tue, Jul 23, 2019 at 6:57 PM Ravi Chandra Sadineni > 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 > > wrote: > > > > > > On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni > > > 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.
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 wrote: > > On Tue, Jul 23, 2019 at 12:33 AM Ravi Chandra Sadineni > 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] power:sysfs: Expose device wakeup_event_count.
Hi Greg,
On Mon, Jul 22, 2019 at 11:22 AM Greg KH wrote:
>
> On Mon, Jul 22, 2019 at 11:02:58AM -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
> > ---
> > Documentation/ABI/testing/sysfs-devices-power | 11 ++
> > drivers/base/power/sysfs.c| 20 +++
> > 2 files changed, 31 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-devices-power
> > b/Documentation/ABI/testing/sysfs-devices-power
> > index 1ca04b4f0489..344549f4013f 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
> > +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 is empty.
>
> The attribute is not "empty" it returns just an empty line.
Corrected the description.
>
> Is that really a good thing if you are expecting a number?
This is to adhere to the convention as described in
base/power/sysfs.c. Hope this is o.k
>
> > +
> > What:/sys/devices/.../power/wakeup_active_count
> > Date:September 2010
> > Contact: Rafael J. Wysocki
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index f42044d9711c..8dc1235b9784 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -357,6 +357,25 @@ 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(>power.lock);
> > + if (dev->power.wakeup) {
> > + count = dev->power.wakeup->event_count;
> > + enabled = true;
> > + }
> > + spin_unlock_irq(>power.lock);
>
> Why do you need to lock? The state and count can change right after the
> lock, so what does this help with?
power.wakeup can be NULL (device_wakeup_detach ()) if wakeup is
disabled for a particular device.
>
> > + return enabled ? sprintf(buf, "%lu\n", count) : sprintf(buf, "\n");
>
> Use a real if statement please.
>
> > +}
> > +
> > +static DEVICE_ATTR_RO(wakeup_event_count);
> > +
> > +
> > +
>
> too many empty lines :)
Removed the empty lines.
>
> thanks,
>
> greg k-h
Thanks,
Ravi
[PATCH 1/2 V2] power: sysfs: Remove wakeup_abort_count attribute.
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
---
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
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
-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
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(>power.lock);
- if (dev->power.wakeup) {
- count = dev->power.wakeup->wakeup_count;
- enabled = true;
- }
- spin_unlock_irq(>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[] = {
_attr_wakeup.attr,
_attr_wakeup_count.attr,
_attr_wakeup_active_count.attr,
- _attr_wakeup_abort_count.attr,
_attr_wakeup_expire_count.attr,
_attr_wakeup_active.attr,
_attr_wakeup_total_time_ms.attr,
--
2.20.1
[PATCH 0/2] power: Refactor device level sysfs.
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 2/2 V2] power:sysfs: Expose device wakeup_event_count.
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
---
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
+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
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(>power.lock);
+ if (dev->power.wakeup) {
+ count = dev->power.wakeup->event_count;
+ enabled = true;
+ }
+ spin_unlock_irq(>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
_attr_wakeup.attr,
_attr_wakeup_count.attr,
+ _attr_wakeup_event_count.attr,
_attr_wakeup_active_count.attr,
_attr_wakeup_expire_count.attr,
_attr_wakeup_active.attr,
--
2.20.1
[PATCH] power:sysfs: Expose device wakeup_event_count.
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
---
Documentation/ABI/testing/sysfs-devices-power | 11 ++
drivers/base/power/sysfs.c| 20 +++
2 files changed, 31 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-devices-power
b/Documentation/ABI/testing/sysfs-devices-power
index 1ca04b4f0489..344549f4013f 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
+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 is empty.
+
What: /sys/devices/.../power/wakeup_active_count
Date: September 2010
Contact: Rafael J. Wysocki
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index f42044d9711c..8dc1235b9784 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -357,6 +357,25 @@ 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(>power.lock);
+ if (dev->power.wakeup) {
+ count = dev->power.wakeup->event_count;
+ enabled = true;
+ }
+ spin_unlock_irq(>power.lock);
+ return enabled ? sprintf(buf, "%lu\n", count) : 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 +580,7 @@ static struct attribute *wakeup_attrs[] = {
#ifdef CONFIG_PM_SLEEP
_attr_wakeup.attr,
_attr_wakeup_count.attr,
+ _attr_wakeup_event_count.attr,
_attr_wakeup_active_count.attr,
_attr_wakeup_expire_count.attr,
_attr_wakeup_active.attr,
--
2.20.1
[PATCH] power: sysfs: Remove wakeup_abort_count attribute.
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
---
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
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
-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
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(>power.lock);
- if (dev->power.wakeup) {
- count = dev->power.wakeup->wakeup_count;
- enabled = true;
- }
- spin_unlock_irq(>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[] = {
_attr_wakeup.attr,
_attr_wakeup_count.attr,
_attr_wakeup_active_count.attr,
- _attr_wakeup_abort_count.attr,
_attr_wakeup_expire_count.attr,
_attr_wakeup_active.attr,
_attr_wakeup_total_time_ms.attr,
--
2.20.1
Re: [PATCH] power: Do not clear events_check_enabled in pm_wakeup_pending()
Hi, Just wanted to check if this is o.k. Thanks, Ravi On Wed, Jun 19, 2019 at 10:52 AM Ravi Chandra Sadineni wrote: > > events_check_enabled bool is set when wakeup_count sysfs attribute > is written. User level daemon is expected to write this attribute > just before suspend. > > When this boolean is set, calls to pm_wakeup_event() will result in > increment of per device and global wakeup count that helps in > identifying the wake source. global wakeup count is also used by > pm_wakeup_pending() to identify if there are any pending events that > should result in an suspend abort. > > Currently calls to pm_wakeup_pending() also clears events_check_enabled. > This can be a problem when there are multiple wake events or when the > suspend is aborted due to an interrupt on a shared interrupt line. > For example an Mfd device can create several platform devices which > might fetch the state on resume in the driver resume method and increment > the wakeup count if needed. But if events_check_enabled is cleared before > resume methods get to execute, wakeup count will not be incremented. Thus > let us not reset the bool here. > > Note that events_check_enabled is also cleared in suspend.c/enter_state() > on every resume at the end. > > Signed-off-by: Ravi Chandra Sadineni > --- > drivers/base/power/wakeup.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index 5b2b6a05a4f3..88aade871589 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -838,7 +838,6 @@ bool pm_wakeup_pending(void) > > split_counters(, ); > ret = (cnt != saved_count || inpr > 0); > - events_check_enabled = !ret; > } > raw_spin_unlock_irqrestore(_lock, flags); > > -- > 2.20.1 >
[PATCH] power: Do not clear events_check_enabled in pm_wakeup_pending()
events_check_enabled bool is set when wakeup_count sysfs attribute is written. User level daemon is expected to write this attribute just before suspend. When this boolean is set, calls to pm_wakeup_event() will result in increment of per device and global wakeup count that helps in identifying the wake source. global wakeup count is also used by pm_wakeup_pending() to identify if there are any pending events that should result in an suspend abort. Currently calls to pm_wakeup_pending() also clears events_check_enabled. This can be a problem when there are multiple wake events or when the suspend is aborted due to an interrupt on a shared interrupt line. For example an Mfd device can create several platform devices which might fetch the state on resume in the driver resume method and increment the wakeup count if needed. But if events_check_enabled is cleared before resume methods get to execute, wakeup count will not be incremented. Thus let us not reset the bool here. Note that events_check_enabled is also cleared in suspend.c/enter_state() on every resume at the end. Signed-off-by: Ravi Chandra Sadineni --- drivers/base/power/wakeup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 5b2b6a05a4f3..88aade871589 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -838,7 +838,6 @@ bool pm_wakeup_pending(void) split_counters(, ); ret = (cnt != saved_count || inpr > 0); - events_check_enabled = !ret; } raw_spin_unlock_irqrestore(_lock, flags); -- 2.20.1
Re: [PATCH] power: Do not clear events_check_enabled in pm_wakeup_pending()
Hi, Just wanted to check if this o.k. Thanks, Ravi On Thu, Jun 6, 2019 at 5:37 PM Ravi Chandra Sadineni wrote: > > events_check_enabled bool is set when wakeup_count sysfs attribute > is written. User level daemon is expected to write this attribute > just before suspend. > > When this boolean is set, calls to pm_wakeup_event() will result in > increment of per device and global wakeup count that helps in > identifying the wake source. global wakeup count is also used by > pm_wakeup_pending() to identify if there are any pending events that > should result in an suspend abort. > > Currently calls to pm_wakeup_pending() also clears events_check_enabled. > This can be a problem when there are multiple wake events or when the > suspend is aborted due to an interrupt on a shared interrupt line. > For example an Mfd device can create several platform devices which > might fetch the state on resume in the driver resume method and increment > the wakeup count if needed. But if events_check_enabled is cleared before > resume methods get to execute, wakeup count will not be incremented. Thus > let us not reset the bool here. > > Note that events_check_enabled is also cleared in suspend.c/enter_state() > on every resume at the end. > > Signed-off-by: Ravi Chandra Sadineni > --- > drivers/base/power/wakeup.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c > index 5b2b6a05a4f3..88aade871589 100644 > --- a/drivers/base/power/wakeup.c > +++ b/drivers/base/power/wakeup.c > @@ -838,7 +838,6 @@ bool pm_wakeup_pending(void) > > split_counters(, ); > ret = (cnt != saved_count || inpr > 0); > - events_check_enabled = !ret; > } > raw_spin_unlock_irqrestore(_lock, flags); > > -- > 2.20.1 >
[PATCH] power: Do not clear events_check_enabled in pm_wakeup_pending()
events_check_enabled bool is set when wakeup_count sysfs attribute is written. User level daemon is expected to write this attribute just before suspend. When this boolean is set, calls to pm_wakeup_event() will result in increment of per device and global wakeup count that helps in identifying the wake source. global wakeup count is also used by pm_wakeup_pending() to identify if there are any pending events that should result in an suspend abort. Currently calls to pm_wakeup_pending() also clears events_check_enabled. This can be a problem when there are multiple wake events or when the suspend is aborted due to an interrupt on a shared interrupt line. For example an Mfd device can create several platform devices which might fetch the state on resume in the driver resume method and increment the wakeup count if needed. But if events_check_enabled is cleared before resume methods get to execute, wakeup count will not be incremented. Thus let us not reset the bool here. Note that events_check_enabled is also cleared in suspend.c/enter_state() on every resume at the end. Signed-off-by: Ravi Chandra Sadineni --- drivers/base/power/wakeup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c index 5b2b6a05a4f3..88aade871589 100644 --- a/drivers/base/power/wakeup.c +++ b/drivers/base/power/wakeup.c @@ -838,7 +838,6 @@ bool pm_wakeup_pending(void) split_counters(, ); ret = (cnt != saved_count || inpr > 0); - events_check_enabled = !ret; } raw_spin_unlock_irqrestore(_lock, flags); -- 2.20.1
Re: [PATCH V1] elan_i2c: Increment wakeup count if wake source.
Hi Dmitry,
On Mon, May 13, 2019 at 4:29 PM Dmitry Torokhov
wrote:
>
> Hi Ravi,
>
> On Mon, May 13, 2019 at 3:06 PM Ravi Chandra Sadineni
> wrote:
> >
> > Notify the PM core that this dev is the wake source. This helps
> > userspace daemon tracking the wake source to identify the origin of the
> > wake.
>
> I wonder if we could do that form the i2c core instead of individual drivers?
I am sorry, I don't see a way how this could be done.
>
> >
> > Signed-off-by: Ravi Chandra Sadineni
> > ---
> > drivers/input/mouse/elan_i2c_core.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > b/drivers/input/mouse/elan_i2c_core.c
> > index f9525d6f0bfe..2c0561e20b7f 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -981,6 +981,8 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
> > if (error)
> > goto out;
> >
> > + pm_wakeup_event(dev, 0);
> > +
> > switch (report[ETP_REPORT_ID_OFFSET]) {
> > case ETP_REPORT_ID:
> > elan_report_absolute(data, report);
> > --
> > 2.20.1
> >
>
> Thanks.
>
> --
> Dmitry
Thanks,
Ravi
[PATCH V1] elan_i2c: Increment wakeup count if wake source.
Notify the PM core that this dev is the wake source. This helps
userspace daemon tracking the wake source to identify the origin of the
wake.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/input/mouse/elan_i2c_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c
b/drivers/input/mouse/elan_i2c_core.c
index f9525d6f0bfe..2c0561e20b7f 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -981,6 +981,8 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
if (error)
goto out;
+ pm_wakeup_event(dev, 0);
+
switch (report[ETP_REPORT_ID_OFFSET]) {
case ETP_REPORT_ID:
elan_report_absolute(data, report);
--
2.20.1
Re: [PATCH V3] cros_ec: Expose sysfile to force battery cut-off on shutdown.
Hi Enric,
Thanks for the review.
On Thu, Mar 7, 2019 at 3:57 AM Enric Balletbo i Serra
wrote:
>
> Hi RaviChandra,
>
> Some few comments below ...
>
>
> On 5/3/19 1:53, RaviChandra Sadineni wrote:
> > On chromebooks, power_manager daemon normally shutsdown(S5) the device
> > when the battery charge falls below 4% threshold. Chromeos EC then
> > normally spends an hour in S5 before hibernating. If the battery charge
> > falls below critical threshold in the mean time, EC does a battery cutoff
>
> Be coherent using the same spelling along all the patch for cutoff.
Done.
>
>
> > instead of hibernating. On some chromebooks, S5 is optimal enough
> > resulting in EC hibernating without battery cut-off. This results in
>
> s/cut-off/cutoff/
Done.
>
> > battery deep-discharging. This is a bad user experience as battery
> > has to trickle charge before booting when the A.C is plugged in the next
> > time.
> >
> > This patch exposes a sysfs file for an userland daemon to suggest EC if it
> > has to do a battery cut-off instead of hibernating when the system enters
>
> s/cut-off/cutoff/
Done.
>
> > S5 next time.
> >
> > Signed-off-by: RaviChandra Sadineni
> > ---
> >
> > V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> > V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> >
> >
> > .../ABI/testing/sysfs-class-chromeos | 10
> > drivers/platform/chrome/cros_ec_sysfs.c | 48 +++
> > 2 files changed, 58 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-chromeos
> > b/Documentation/ABI/testing/sysfs-class-chromeos
> > new file mode 100644
> > index ..d5ab22c44977
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>
> Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
> released, so it applies cleanly.
Done.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next
>
> > @@ -0,0 +1,10 @@
> > +What: /sys/class/chromeos/cros_ec/battery-cuttoff
>
> The name should match with the attribute name, so battery_cutoff in this case
> as
> you defined the attribute name as battery_cutoff
>
> Is this a common property for all the EC devices?
>
> Note that we have
>
> /sys/class/chromeos//...
>
> where can be cros_ec|cros_pd and many others in the future
>
> On those devices that more than one EC is present we will have. E.g
>
> cros_ec/battery_cutoff and cros_pd/battery_cutoff
> cros_ec/battery_cutoff and cros_ish/battery_cutoff
>
> Which I think is not what you want. Ideally I'd like to see visible the
> attribute only on those ECs that support that feature. Is this command
> associated with an EC feature?
Yes with EC_FEATURE_BATTERY. Now checking if EC supports EC_FEATURE_BATTERY.
>
> If that's not possible I can assume writing to battery_cutoff for ECs that
> don't
> implement it would return a protocol/command error.
>
>
> > +Date: February 2019
> > +Contact:Ravi Chandra Sadineni
> > +Description:
> > +cros_ec battery cuttoff configuration. Only option
> > +currently exposed is 'at-shutdown'.
> > +
> > +'at-shutdown' sends a host command to EC requesting
> > +battery cutoff on next shutdown. If AC is plugged
> > +in before next shutdown, EC ignores the request.
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c
> > b/drivers/platform/chrome/cros_ec_sysfs.c
> > index f34a50121064..6ef6b860c818 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> > return count;
> > }
> >
> > +/* Battery cut-off control */
>
> s/cut-off/cutoff/
Done.
>
> > +static ssize_t battery_cutoff_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ec_params_battery_cutoff *param;
> > + struct cros_ec_command *msg;
> > + int ret;
> > + struct cros_ec_dev *ec =
> > + container_of(dev, struct cros_ec_dev, class_dev);
>
> use to_cros_ec_dev instead of container_of
>
> > + char *p;
> > + i
Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.
Hi Daisuke,
On Tue, Mar 5, 2019 at 9:29 AM Daisuke Nojiri wrote:
>>
>> + if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
>> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
>> + } else {
>> + kfree(msg);
>> + return -EINVAL;
>> + }
>
>
> Why don't we just let the EC decide what flag is legal or illegal? If you
> hard code 'at-shutdown', other calls to the sysfs will be blocked even if the
> EC supports other flags.
That might not be a wise option. Allowing random values can have
unintended effects. For example consider what would happen if write
random values to EC today. Any flag value other than
EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN will cause the battery to cutoff
immediately before the system shutdowns.
In future, if EC exposes more attributes, it will not be that
difficult to add support here.
>
> On Mon, Mar 4, 2019 at 5:00 PM Ravi Chandra Sadineni
> wrote:
>>
>> Hi Guenter,
>>
>> On Sun, Mar 3, 2019 at 5:13 PM Guenter Roeck wrote:
>> >
>> > On Sun, Mar 3, 2019 at 4:54 PM RaviChandra Sadineni
>> > wrote:
>> >>
>> >> On chromebooks, power_manager daemon normally shutsdown(S5) the device
>> >> when the battery charge falls below 4% threshold. Chromeos EC then
>> >> normally spends an hour in S5 before hibernating. If the battery charge
>> >> falls below critical threshold in the mean time, EC does a battery cutoff
>> >> instead of hibernating. On some chromebooks, S5 is optimal enough
>> >> resulting in EC hibernating without battery cut-off. This results in
>> >> battery deep-discharging. This is a bad user experience as battery
>> >> has to trickle charge before booting when the A.C is plugged in the next
>> >> time.
>> >>
>> >> This patch exposes a sysfs file for an userland daemon to suggest EC if it
>> >> has to do a battery cut-off instead of hibernating when the system enters
>> >> S5 next time.
>> >>
>> >> Signed-off-by: RaviChandra Sadineni
>> >> ---
>> >> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>> >>
>> >> .../ABI/testing/sysfs-class-chromeos | 8
>> >> drivers/platform/chrome/cros_ec_sysfs.c | 37 +++
>> >> 2 files changed, 45 insertions(+)
>> >> create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
>> >>
>> >> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos
>> >> b/Documentation/ABI/testing/sysfs-class-chromeos
>> >> new file mode 100644
>> >> index ..44d3cee6e7ae
>> >> --- /dev/null
>> >> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>> >> @@ -0,0 +1,8 @@
>> >> +What: /sys/class/chromeos/cros_ec/cutoff_at_shutdown
>> >> +Date: February 2019
>> >> +Contact:Ravi Chandra Sadineni
>> >> +Description:
>> >> +On writing '[Yy1]' to cutoff_at_shutdown property,
>> >> +kernel sends a host command to EC requesting battery
>> >> +cutoff on next shutdown. If AC is plugged in before
>> >> +next shutdown, EC resets this flag.
>> >> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c
>> >> b/drivers/platform/chrome/cros_ec_sysfs.c
>> >> index f34a50121064..f5168ce8bfc7 100644
>> >> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> >> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> >> @@ -322,14 +322,51 @@ static ssize_t kb_wake_angle_store(struct device
>> >> *dev,
>> >> return count;
>> >> }
>> >>
>> >> +/* Battery cut-off control */
>> >> +static ssize_t cutoff_at_shutdown_store(struct device *dev,
>> >> + struct device_attribute *attr,
>> >> + const char *buf, size_t count)
>> >> +{
>> >> + struct ec_params_battery_cutoff *param;
>> >> + struct cros_ec_command *msg;
>> >> + int ret;
>> >> + struct cros_ec_dev *ec = container_of(
>> >> + dev, struct cros_ec_dev, class_dev);
>> >> + bool cutoff_battery;
>> >> +
>> >> + if (kstrtobool(buf, _battery))
>> >> + return -EINVAL;
>> >> +
>> &g
Re: [PATCH V2] cros_ec: Expose sysfile to force battery cut-off on shutdown.
Hi Guenter,
On Sun, Mar 3, 2019 at 5:13 PM Guenter Roeck wrote:
>
> On Sun, Mar 3, 2019 at 4:54 PM RaviChandra Sadineni
> wrote:
>>
>> On chromebooks, power_manager daemon normally shutsdown(S5) the device
>> when the battery charge falls below 4% threshold. Chromeos EC then
>> normally spends an hour in S5 before hibernating. If the battery charge
>> falls below critical threshold in the mean time, EC does a battery cutoff
>> instead of hibernating. On some chromebooks, S5 is optimal enough
>> resulting in EC hibernating without battery cut-off. This results in
>> battery deep-discharging. This is a bad user experience as battery
>> has to trickle charge before booting when the A.C is plugged in the next
>> time.
>>
>> This patch exposes a sysfs file for an userland daemon to suggest EC if it
>> has to do a battery cut-off instead of hibernating when the system enters
>> S5 next time.
>>
>> Signed-off-by: RaviChandra Sadineni
>> ---
>> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>>
>> .../ABI/testing/sysfs-class-chromeos | 8
>> drivers/platform/chrome/cros_ec_sysfs.c | 37 +++
>> 2 files changed, 45 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos
>> b/Documentation/ABI/testing/sysfs-class-chromeos
>> new file mode 100644
>> index ..44d3cee6e7ae
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>> @@ -0,0 +1,8 @@
>> +What: /sys/class/chromeos/cros_ec/cutoff_at_shutdown
>> +Date: February 2019
>> +Contact:Ravi Chandra Sadineni
>> +Description:
>> +On writing '[Yy1]' to cutoff_at_shutdown property,
>> +kernel sends a host command to EC requesting battery
>> +cutoff on next shutdown. If AC is plugged in before
>> +next shutdown, EC resets this flag.
>> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c
>> b/drivers/platform/chrome/cros_ec_sysfs.c
>> index f34a50121064..f5168ce8bfc7 100644
>> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> @@ -322,14 +322,51 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>> return count;
>> }
>>
>> +/* Battery cut-off control */
>> +static ssize_t cutoff_at_shutdown_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ec_params_battery_cutoff *param;
>> + struct cros_ec_command *msg;
>> + int ret;
>> + struct cros_ec_dev *ec = container_of(
>> + dev, struct cros_ec_dev, class_dev);
>> + bool cutoff_battery;
>> +
>> + if (kstrtobool(buf, _battery))
>> + return -EINVAL;
>> +
>> + if (!cutoff_battery)
>> + return count;
>> +
>
>
> It is quite unusual to only be able to set this option, but not to be able to
> reset it. I think that warrants an explanation and reasoning. Also, if
> clearing the flag is not supported, I would expect an attempt to do so to
> return an error. There should also be an explanation in the code explaining
> why the attribute is write-only.
Added documentation for the reason behind making this flag write only.
Instead of taking a boolean value, made this attribute more generic
and took 'at-shutdown' as an option. This way it will be easy to
extend this sysfs api in the future (if EC exposes more attributes for
this host command).
>
> Guenter
>
>>
>> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + param = (struct ec_params_battery_cutoff *)msg->data;
>> + msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
>> + msg->version = 1;
>> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
>> + msg->outsize = sizeof(*param);
>> + msg->insize = 0;
>> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> + kfree(msg);
>> + if (ret < 0)
>> + return ret;
>> + return count;
>> +}
>> +
>> /* Module initialization */
>>
>> static DEVICE_ATTR_RW(reboot);
>> static DEVICE_ATTR_RO(version);
>> static DEVICE_ATTR_RO(flashinfo);
>> static DEVICE_ATTR_RW(kb_wake_angle);
>> +static DEVICE_ATTR_WO(cutoff_at_shutdown);
>>
>> static struct attribute *__ec_attrs[] = {
>> + _attr_cutoff_battery_at_shutdown.attr,
>> _attr_kb_wake_angle.attr,
>> _attr_reboot.attr,
>> _attr_version.attr,
>> --
>> 2.20.1
>>
Thanks,
Ravi
Re: [PATCH] cros_ec: Expose sysfile to force battery cut-off on shutdown.
Hi Guenter,
On Fri, Mar 1, 2019 at 1:00 PM Guenter Roeck wrote:
>
> On Fri, Mar 1, 2019 at 11:22 AM RaviChandra Sadineni
> wrote:
>>
>> On chromebooks, power_manager daemon normally shutsdown(S5) the device
>> when the battery charge falls below 4% threshold. ChromeOS EC then
>> normally spends an hour in S5 before hibernating. If the battery charge
>> falls below critical threshold in the mean time, EC does a battery cutoff
>> instead of hibernating. On some chromebooks, S5 is optimal enough
>> resulting in EC hibernating without battery cut-off. This results in
>> battery deep-discharging. This is a bad user experience as battery
>> has to trickle charge before booting when the A.C is plugged in the next
>> time.
>>
>> This patch exposes a sysfs file for an userland daemon to suggest EC if it
>> has to do a battery cut-off instead of hibernating when the system enters
>> S5 next time.
>>
>> Signed-off-by: RaviChandra Sadineni
>> ---
>> drivers/platform/chrome/cros_ec_sysfs.c | 34 +
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c
>> b/drivers/platform/chrome/cros_ec_sysfs.c
>> index f34a50121064..151c7c143941 100644
>> --- a/drivers/platform/chrome/cros_ec_sysfs.c
>> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
>> @@ -322,14 +322,48 @@ static ssize_t kb_wake_angle_store(struct device *dev,
>> return count;
>> }
>>
>> +/* Battery cut-off control */
>> +static ssize_t cutoff_battery_at_shutdown_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ec_params_battery_cutoff *param;
>> + struct cros_ec_command *msg;
>> + int ret;
>> + struct cros_ec_dev *ec = container_of(
>> + dev, struct cros_ec_dev, class_dev);
>> + uint8_t cutoff_battery;
>> +
>> + if (kstrtou8(buf, 10, _battery) || (cutoff_battery != 1))
>
>
> Excessive ( ). Also, why not kstrtobool() ?
Done.
>
>>
>> + return -EINVAL;
>> +
>>
>> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + param = (struct ec_params_battery_cutoff *)msg->data;
>> + msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
>> + msg->version = 1;
>> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
>
>
> For some reason I fail to see where cutoff_battery is used to determine what
> to send to the EC. Am I missing something ?
Currently there is no way to reset the flag. i.e EC does not expose a
console command to reset the flag once set. So I intend to accept only
true bool flag. Is there a better way to do this ?
>
>>
>> + msg->outsize = sizeof(*param);
>> + msg->insize = 0;
>> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> + kfree(msg);
>> + if (ret < 0)
>> + return ret;
>> + return count;
>> +}
>> +
>> /* Module initialization */
>>
>> static DEVICE_ATTR_RW(reboot);
>> static DEVICE_ATTR_RO(version);
>> static DEVICE_ATTR_RO(flashinfo);
>> static DEVICE_ATTR_RW(kb_wake_angle);
>> +static DEVICE_ATTR_WO(cutoff_battery_at_shutdown);
>>
>
> I personally dislike excessively long sysfs attribute names. I would prefer
> to see a shorter name and have it documented in
> Documentation/ABI/testing/sysfs-class-chromeos.
>
> Is WO really desirable and warranted here ?
Currently EC does not expose a host command to read the status.
Maintaining the state in the kernel might not work as the state can go
out of sync. For example, when AC is plugged in, EC resets the flag
and the kernel will not be aware of it.
>
>>
>> static struct attribute *__ec_attrs[] = {
>> + _attr_cutoff_battery_at_shutdown.attr,
>> _attr_kb_wake_angle.attr,
>> _attr_reboot.attr,
>> _attr_version.attr,
>> --
>> 2.20.1
>>
Thanks,
Ravi
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
hi Merek,
I tried booting a snow device and could not get it to boot it to the
console. I assume i don't have right kernel config. Can you share your
config if possible.
Thanks,
RaviOn Mon, Aug 6, 2018 at 4:05 PM Ravi Chandra Sadineni
wrote:
>
> Hi Merek,
>
> Thanks for the info. Lemme understand what's going on. Will update the
> thread once I have more info.
>
> Thanks,
> Ravi
> On Mon, Aug 6, 2018 at 12:15 AM Marek Szyprowski
> wrote:
> >
> > Hi Dmitry
> >
> > On 2018-08-06 08:16, Dmitry Torokhov wrote:
> > > On Sun, Aug 5, 2018 at 10:29 PM Marek Szyprowski
> > > wrote:
> > >> Hi Ravi,
> > >>
> > >> On 2018-08-03 18:53, Ravi Chandra Sadineni wrote:
> > >>> Understood. I am trying to reproduce this issue locally. Wanted to
> > >>> know the version of the kernel so I can give a try. Marek, can you
> > >>> please confirm the kernel version.
> > >>>
> > >> Yes, sorry for the missing context, I was in hurry writing the report and
> > >> I wanted to send it before leaving the office. I'm testing mainline on
> > >> Snow
> > >> with exynos_defconfig.
> > >>
> > >> Suspend/resume is partially broken already with mainline, but if you
> > >> disable
> > >> CPUfreq support, it works fine on Linux v4.17.
> > >>
> > >> I've posted CPUfreq related fixes here if you are interested:
> > >> https://patchwork.kernel.org/patch/10554607/
> > >> https://patchwork.kernel.org/patch/10554603/
> > >>
> > >> The issue with cros_ec_keyb patch appears first on Linux v4.18-rc1,
> > >> which is
> > >> the first release with that patch.
> > > Marek, this patch should only be in -next, I do not believe I sent it
> > > to Linus just yet. If mainline is broken for you it can't be caused by
> > > this patch.
> >
> > Aaahh. My fault then. The suspend/resume issue is cause by commit
> > 38ba34a43dbc ("Input: cros_ec_keyb - mark cros_ec_keyb driver as wake
> > enabled device."), which has been merged to v4.18-rc1.
> >
> > It looks that I've downloaded wrong patch from the patchwork to reply it
> > with a a bug report. :( I'm really sorry for the noise in the wrong thread.
> >
> > Ravi: please let me know how can I help you to debug this issue.
> >
> > >
> > >> Linux -next from 20180803, which has a few
> > >> more patches for cros_ec_keyb suffers from the same issue.
> > > Just to confirm, if you revert only this patch from -next you get
> > > suspend/resume back?
> > >
> > > Thanks.
> >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R Institute Poland
> >
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
hi Merek,
I tried booting a snow device and could not get it to boot it to the
console. I assume i don't have right kernel config. Can you share your
config if possible.
Thanks,
RaviOn Mon, Aug 6, 2018 at 4:05 PM Ravi Chandra Sadineni
wrote:
>
> Hi Merek,
>
> Thanks for the info. Lemme understand what's going on. Will update the
> thread once I have more info.
>
> Thanks,
> Ravi
> On Mon, Aug 6, 2018 at 12:15 AM Marek Szyprowski
> wrote:
> >
> > Hi Dmitry
> >
> > On 2018-08-06 08:16, Dmitry Torokhov wrote:
> > > On Sun, Aug 5, 2018 at 10:29 PM Marek Szyprowski
> > > wrote:
> > >> Hi Ravi,
> > >>
> > >> On 2018-08-03 18:53, Ravi Chandra Sadineni wrote:
> > >>> Understood. I am trying to reproduce this issue locally. Wanted to
> > >>> know the version of the kernel so I can give a try. Marek, can you
> > >>> please confirm the kernel version.
> > >>>
> > >> Yes, sorry for the missing context, I was in hurry writing the report and
> > >> I wanted to send it before leaving the office. I'm testing mainline on
> > >> Snow
> > >> with exynos_defconfig.
> > >>
> > >> Suspend/resume is partially broken already with mainline, but if you
> > >> disable
> > >> CPUfreq support, it works fine on Linux v4.17.
> > >>
> > >> I've posted CPUfreq related fixes here if you are interested:
> > >> https://patchwork.kernel.org/patch/10554607/
> > >> https://patchwork.kernel.org/patch/10554603/
> > >>
> > >> The issue with cros_ec_keyb patch appears first on Linux v4.18-rc1,
> > >> which is
> > >> the first release with that patch.
> > > Marek, this patch should only be in -next, I do not believe I sent it
> > > to Linus just yet. If mainline is broken for you it can't be caused by
> > > this patch.
> >
> > Aaahh. My fault then. The suspend/resume issue is cause by commit
> > 38ba34a43dbc ("Input: cros_ec_keyb - mark cros_ec_keyb driver as wake
> > enabled device."), which has been merged to v4.18-rc1.
> >
> > It looks that I've downloaded wrong patch from the patchwork to reply it
> > with a a bug report. :( I'm really sorry for the noise in the wrong thread.
> >
> > Ravi: please let me know how can I help you to debug this issue.
> >
> > >
> > >> Linux -next from 20180803, which has a few
> > >> more patches for cros_ec_keyb suffers from the same issue.
> > > Just to confirm, if you revert only this patch from -next you get
> > > suspend/resume back?
> > >
> > > Thanks.
> >
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R Institute Poland
> >
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Hi Merek,
Thanks for the info. Lemme understand what's going on. Will update the
thread once I have more info.
Thanks,
Ravi
On Mon, Aug 6, 2018 at 12:15 AM Marek Szyprowski
wrote:
>
> Hi Dmitry
>
> On 2018-08-06 08:16, Dmitry Torokhov wrote:
> > On Sun, Aug 5, 2018 at 10:29 PM Marek Szyprowski
> > wrote:
> >> Hi Ravi,
> >>
> >> On 2018-08-03 18:53, Ravi Chandra Sadineni wrote:
> >>> Understood. I am trying to reproduce this issue locally. Wanted to
> >>> know the version of the kernel so I can give a try. Marek, can you
> >>> please confirm the kernel version.
> >>>
> >> Yes, sorry for the missing context, I was in hurry writing the report and
> >> I wanted to send it before leaving the office. I'm testing mainline on Snow
> >> with exynos_defconfig.
> >>
> >> Suspend/resume is partially broken already with mainline, but if you
> >> disable
> >> CPUfreq support, it works fine on Linux v4.17.
> >>
> >> I've posted CPUfreq related fixes here if you are interested:
> >> https://patchwork.kernel.org/patch/10554607/
> >> https://patchwork.kernel.org/patch/10554603/
> >>
> >> The issue with cros_ec_keyb patch appears first on Linux v4.18-rc1, which
> >> is
> >> the first release with that patch.
> > Marek, this patch should only be in -next, I do not believe I sent it
> > to Linus just yet. If mainline is broken for you it can't be caused by
> > this patch.
>
> Aaahh. My fault then. The suspend/resume issue is cause by commit
> 38ba34a43dbc ("Input: cros_ec_keyb - mark cros_ec_keyb driver as wake
> enabled device."), which has been merged to v4.18-rc1.
>
> It looks that I've downloaded wrong patch from the patchwork to reply it
> with a a bug report. :( I'm really sorry for the noise in the wrong thread.
>
> Ravi: please let me know how can I help you to debug this issue.
>
> >
> >> Linux -next from 20180803, which has a few
> >> more patches for cros_ec_keyb suffers from the same issue.
> > Just to confirm, if you revert only this patch from -next you get
> > suspend/resume back?
> >
> > Thanks.
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R Institute Poland
>
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Hi Merek,
Thanks for the info. Lemme understand what's going on. Will update the
thread once I have more info.
Thanks,
Ravi
On Mon, Aug 6, 2018 at 12:15 AM Marek Szyprowski
wrote:
>
> Hi Dmitry
>
> On 2018-08-06 08:16, Dmitry Torokhov wrote:
> > On Sun, Aug 5, 2018 at 10:29 PM Marek Szyprowski
> > wrote:
> >> Hi Ravi,
> >>
> >> On 2018-08-03 18:53, Ravi Chandra Sadineni wrote:
> >>> Understood. I am trying to reproduce this issue locally. Wanted to
> >>> know the version of the kernel so I can give a try. Marek, can you
> >>> please confirm the kernel version.
> >>>
> >> Yes, sorry for the missing context, I was in hurry writing the report and
> >> I wanted to send it before leaving the office. I'm testing mainline on Snow
> >> with exynos_defconfig.
> >>
> >> Suspend/resume is partially broken already with mainline, but if you
> >> disable
> >> CPUfreq support, it works fine on Linux v4.17.
> >>
> >> I've posted CPUfreq related fixes here if you are interested:
> >> https://patchwork.kernel.org/patch/10554607/
> >> https://patchwork.kernel.org/patch/10554603/
> >>
> >> The issue with cros_ec_keyb patch appears first on Linux v4.18-rc1, which
> >> is
> >> the first release with that patch.
> > Marek, this patch should only be in -next, I do not believe I sent it
> > to Linus just yet. If mainline is broken for you it can't be caused by
> > this patch.
>
> Aaahh. My fault then. The suspend/resume issue is cause by commit
> 38ba34a43dbc ("Input: cros_ec_keyb - mark cros_ec_keyb driver as wake
> enabled device."), which has been merged to v4.18-rc1.
>
> It looks that I've downloaded wrong patch from the patchwork to reply it
> with a a bug report. :( I'm really sorry for the noise in the wrong thread.
>
> Ravi: please let me know how can I help you to debug this issue.
>
> >
> >> Linux -next from 20180803, which has a few
> >> more patches for cros_ec_keyb suffers from the same issue.
> > Just to confirm, if you revert only this patch from -next you get
> > suspend/resume back?
> >
> > Thanks.
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R Institute Poland
>
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Understood. I am trying to reproduce this issue locally. Wanted to know the version of the kernel so I can give a try. Marek, can you please confirm the kernel version. Thanks, On Fri, Aug 3, 2018 at 9:08 AM Dmitry Torokhov wrote: > On Fri, Aug 3, 2018 at 8:51 AM Ravi Chandra Sadineni > wrote: > > > > Hi Marek, > > > > Can you please give me little more context ? Snow is still on 3.8.11. > This patch is not backported to 3.8.11. Are you trying to flash a different > kernel on snow? > > > > Ravi, from upstream perspective what kernel Google uses when shipping > Snow is completely immaterial. Mainline is supposed to work equally > well on all devices. > > Thanks. > > -- > Dmitry >
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Understood. I am trying to reproduce this issue locally. Wanted to know the version of the kernel so I can give a try. Marek, can you please confirm the kernel version. Thanks, On Fri, Aug 3, 2018 at 9:08 AM Dmitry Torokhov wrote: > On Fri, Aug 3, 2018 at 8:51 AM Ravi Chandra Sadineni > wrote: > > > > Hi Marek, > > > > Can you please give me little more context ? Snow is still on 3.8.11. > This patch is not backported to 3.8.11. Are you trying to flash a different > kernel on snow? > > > > Ravi, from upstream perspective what kernel Google uses when shipping > Snow is completely immaterial. Mainline is supposed to work equally > well on all devices. > > Thanks. > > -- > Dmitry >
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Hi Marek, Can you please give me little more context ? Snow is still on 3.8.11. This patch is not backported to 3.8.11. Are you trying to flash a different kernel on snow? Thanks, Ravi On Fri, Aug 3, 2018 at 12:26 AM Marek Szyprowski wrote: > Hi All, > > On 2018-06-06 00:44, Ravi Chandra Sadineni wrote: > > Remove the unnecessary check before calling pm_wakeup_event. If the > > device is not wake enabled, this call is no-op anyway. > > > > Signed-off-by: Ravi Chandra Sadineni > > This patch breaks suspend/resume on Samsung Exynos5250 Snow Chromebook. > It looks that it is waiting forever(?) on some cros ec i2c call. With > no_console_suspend and initcall_debug kernel parameters together with > playing with magic-sysrq after suspend/resume cycle I've managed to get > following log: > > # rtcwake -s10 -mmem > rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Aug 3 06:13:42 2018 > [ 38.359219] PM: suspend entry (deep) > [ 38.362782] PM: Syncing filesystems ... done. > [ 38.379897] Freezing user space processes ... (elapsed 0.002 seconds) > done. > [ 38.382258] OOM killer disabled. > [ 38.382263] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 38.388705] calling input2+ @ 1610, parent: gpio-keys > [ 38.388847] call input2+ returned 0 after 122 usecs > [ 38.388985] calling sound+ @ 1610, parent: platform > [ 38.423739] max98095 7-0011: ASoC: PRE_PMD: LINE Left Out event > failed: -22 > [ 38.435718] call sound+ returned 0 after 45628 usecs > [ 38.435746] calling panel+ @ 1610, parent: platform > [ 38.435754] call panel+ returned 0 after 1 usecs > [ 38.435768] calling hdmi-audio-codec.4.auto+ @ 1610, parent: > 1453.hdmi > [ 38.435775] call hdmi-audio-codec.4.auto+ returned 0 after 1 usecs > [ 38.435799] calling backlight+ @ 1610, parent: backlight > [ 38.436032] call backlight+ returned 0 after 221 usecs > [ 38.436048] calling backlight+ @ 1610, parent: platform > [ 38.445536] call backlight+ returned 0 after 9257 usecs > [ 38.445670] calling mmc2:0001:3+ @ 6, parent: mmc2:0001 > [ 38.445700] call mmc2:0001:3+ returned 0 after 1 usecs > [ 38.445733] calling mmc2:0001:2+ @ 6, parent: mmc2:0001 > [ 38.445740] call mmc2:0001:2+ returned 0 after 1 usecs > [ 38.445760] calling mmc2:0001:1+ @ 6, parent: mmc2:0001 > [ 38.445767] call mmc2:0001:1+ returned 0 after 1 usecs > [ 38.445792] calling mmc2:0001+ @ 6, parent: mmc2 > [ 38.445884] calling snd-soc-dummy+ @ 1610, parent: platform > [ 38.445892] call snd-soc-dummy+ returned 0 after 1 usecs > [ 38.445988] calling cpufreq-dt+ @ 1610, parent: platform > [ 38.445997] call cpufreq-dt+ returned 0 after 1 usecs > [ 38.446071] calling tps65090-charger+ @ 1610, parent: 104-0048 > [ 38.446079] call tps65090-charger+ returned 0 after 1 usecs > [ 38.446208] calling mmc1:e624+ @ 78, parent: mmc1 > [ 38.446212] calling tps65090-pmic+ @ 1610, parent: 104-0048 > [ 38.446220] call tps65090-pmic+ returned 0 after 1 usecs > [ 38.446279] calling input1+ @ 1610, parent: > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller > [ 38.446289] call input1+ returned 0 after 3 usecs > [ 38.446305] calling > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller+ @ 1610, > parent: 104-001e > [ 38.446313] call > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller+ > returned 0 after 1 usecs > [ 38.446327] calling cros-ec-dev.3.auto+ @ 1610, parent: 104-001e > [ 38.446334] call cros-ec-dev.3.auto+ returned 0 after 1 usecs > [ 38.446370] calling 104-001e+ @ 1610, parent: i2c-104 > [ 38.448555] call mmc1:e624+ returned 0 after 2283 usecs > [ 38.450514] calling mmc0:0001+ @ 79, parent: mmc0 > [ 38.457501] call mmc2:0001+ returned 0 after 11425 usecs > [ 38.463830] wake enabled for irq 150 > [ 38.469426] calling 1-1+ @ 6, parent: usb1 > [ 38.474501] call 104-001e+ returned 0 after 27461 usecs > [ 38.490008] call 1-1+ returned 0 after 10331 usecs > [ 38.494988] calling 104-000b+ @ 1610, parent: i2c-104 > [ 38.512028] call mmc0:0001+ returned 0 after 36926 usecs > [ 38.547373] call 104-000b+ returned 0 after 51146 usecs > [ 38.670382] calling rtc1+ @ 1610, parent: 101e.rtc > [ 38.675688] call rtc1+ returned 0 after 2 usecs > [ 38.680296] calling rtc0+ @ 1610, parent: max77686-rtc > [ 38.717626] call rtc0+ returned 0 after 31302 usecs > [ 38.722578] calling input0+ @ 1610, parent: 1-0067 > [ 38.727536] call input0+ returned 0 after 3 usecs > [ 38.732368] calling usb4+ @ 78, parent: xhci-hcd.2.auto > [ 38.732790] calling usb2+ @ 1614, parent: 1212.usb > [ 38.738659] calling usb1+ @ 79, parent: 1211.u
Re: [PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Hi Marek, Can you please give me little more context ? Snow is still on 3.8.11. This patch is not backported to 3.8.11. Are you trying to flash a different kernel on snow? Thanks, Ravi On Fri, Aug 3, 2018 at 12:26 AM Marek Szyprowski wrote: > Hi All, > > On 2018-06-06 00:44, Ravi Chandra Sadineni wrote: > > Remove the unnecessary check before calling pm_wakeup_event. If the > > device is not wake enabled, this call is no-op anyway. > > > > Signed-off-by: Ravi Chandra Sadineni > > This patch breaks suspend/resume on Samsung Exynos5250 Snow Chromebook. > It looks that it is waiting forever(?) on some cros ec i2c call. With > no_console_suspend and initcall_debug kernel parameters together with > playing with magic-sysrq after suspend/resume cycle I've managed to get > following log: > > # rtcwake -s10 -mmem > rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Aug 3 06:13:42 2018 > [ 38.359219] PM: suspend entry (deep) > [ 38.362782] PM: Syncing filesystems ... done. > [ 38.379897] Freezing user space processes ... (elapsed 0.002 seconds) > done. > [ 38.382258] OOM killer disabled. > [ 38.382263] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 38.388705] calling input2+ @ 1610, parent: gpio-keys > [ 38.388847] call input2+ returned 0 after 122 usecs > [ 38.388985] calling sound+ @ 1610, parent: platform > [ 38.423739] max98095 7-0011: ASoC: PRE_PMD: LINE Left Out event > failed: -22 > [ 38.435718] call sound+ returned 0 after 45628 usecs > [ 38.435746] calling panel+ @ 1610, parent: platform > [ 38.435754] call panel+ returned 0 after 1 usecs > [ 38.435768] calling hdmi-audio-codec.4.auto+ @ 1610, parent: > 1453.hdmi > [ 38.435775] call hdmi-audio-codec.4.auto+ returned 0 after 1 usecs > [ 38.435799] calling backlight+ @ 1610, parent: backlight > [ 38.436032] call backlight+ returned 0 after 221 usecs > [ 38.436048] calling backlight+ @ 1610, parent: platform > [ 38.445536] call backlight+ returned 0 after 9257 usecs > [ 38.445670] calling mmc2:0001:3+ @ 6, parent: mmc2:0001 > [ 38.445700] call mmc2:0001:3+ returned 0 after 1 usecs > [ 38.445733] calling mmc2:0001:2+ @ 6, parent: mmc2:0001 > [ 38.445740] call mmc2:0001:2+ returned 0 after 1 usecs > [ 38.445760] calling mmc2:0001:1+ @ 6, parent: mmc2:0001 > [ 38.445767] call mmc2:0001:1+ returned 0 after 1 usecs > [ 38.445792] calling mmc2:0001+ @ 6, parent: mmc2 > [ 38.445884] calling snd-soc-dummy+ @ 1610, parent: platform > [ 38.445892] call snd-soc-dummy+ returned 0 after 1 usecs > [ 38.445988] calling cpufreq-dt+ @ 1610, parent: platform > [ 38.445997] call cpufreq-dt+ returned 0 after 1 usecs > [ 38.446071] calling tps65090-charger+ @ 1610, parent: 104-0048 > [ 38.446079] call tps65090-charger+ returned 0 after 1 usecs > [ 38.446208] calling mmc1:e624+ @ 78, parent: mmc1 > [ 38.446212] calling tps65090-pmic+ @ 1610, parent: 104-0048 > [ 38.446220] call tps65090-pmic+ returned 0 after 1 usecs > [ 38.446279] calling input1+ @ 1610, parent: > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller > [ 38.446289] call input1+ returned 0 after 3 usecs > [ 38.446305] calling > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller+ @ 1610, > parent: 104-001e > [ 38.446313] call > i2c-arbitrator:i2c@0:embedded-controller@1e:keyboard-controller+ > returned 0 after 1 usecs > [ 38.446327] calling cros-ec-dev.3.auto+ @ 1610, parent: 104-001e > [ 38.446334] call cros-ec-dev.3.auto+ returned 0 after 1 usecs > [ 38.446370] calling 104-001e+ @ 1610, parent: i2c-104 > [ 38.448555] call mmc1:e624+ returned 0 after 2283 usecs > [ 38.450514] calling mmc0:0001+ @ 79, parent: mmc0 > [ 38.457501] call mmc2:0001+ returned 0 after 11425 usecs > [ 38.463830] wake enabled for irq 150 > [ 38.469426] calling 1-1+ @ 6, parent: usb1 > [ 38.474501] call 104-001e+ returned 0 after 27461 usecs > [ 38.490008] call 1-1+ returned 0 after 10331 usecs > [ 38.494988] calling 104-000b+ @ 1610, parent: i2c-104 > [ 38.512028] call mmc0:0001+ returned 0 after 36926 usecs > [ 38.547373] call 104-000b+ returned 0 after 51146 usecs > [ 38.670382] calling rtc1+ @ 1610, parent: 101e.rtc > [ 38.675688] call rtc1+ returned 0 after 2 usecs > [ 38.680296] calling rtc0+ @ 1610, parent: max77686-rtc > [ 38.717626] call rtc0+ returned 0 after 31302 usecs > [ 38.722578] calling input0+ @ 1610, parent: 1-0067 > [ 38.727536] call input0+ returned 0 after 3 usecs > [ 38.732368] calling usb4+ @ 78, parent: xhci-hcd.2.auto > [ 38.732790] calling usb2+ @ 1614, parent: 1212.usb > [ 38.738659] calling usb1+ @ 79, parent: 1211.u
[PATCH V3] ACPI LID: increment wakeup count only when notified.
Because acpi_lid_initialize_state() is called on every system
resume and it triggers acpi_lid_notify_state() which invokes
acpi_pm_wakeup_event() for the lid device, the lid's wakeup count is
incremented even if the lid was not the source of the event that woke up
the system. That behavior confuses user space deamons using
wakeup_count to identify the potential system wakeup source. To avoid
the confusion, only trigger acpi_pm_wakeup_event() in the
acpi_button_notify() path and don't do that in the
acpi_lid_initialize_state() path.
Signed-off-by: Ravi Chandra Sadineni
---
v3: Change is_notification to signal_wakeup
V2: Increment the wakeup count only when the lid is open.
drivers/acpi/button.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5ee2dbbc..40ed3ec9fc94c 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -366,7 +363,8 @@ int acpi_lid_open(void)
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+bool signal_wakeup)
{
int state;
@@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
if (state < 0)
return state;
+ if (state && signal_wakeup)
+ acpi_pm_wakeup_event(>dev);
+
return acpi_lid_notify_state(device, state);
}
@@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device
*device)
(void)acpi_lid_notify_state(device, 1);
break;
case ACPI_BUTTON_LID_INIT_METHOD:
- (void)acpi_lid_update_state(device);
+ (void)acpi_lid_update_state(device, false);
break;
case ACPI_BUTTON_LID_INIT_IGNORE:
default:
@@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
users = button->input->users;
mutex_unlock(>input->mutex);
if (users)
- acpi_lid_update_state(device);
+ acpi_lid_update_state(device, true);
} else {
int keycode;
--
2.18.0.rc2.346.g013aa6912e-goog
[PATCH V3] ACPI LID: increment wakeup count only when notified.
Because acpi_lid_initialize_state() is called on every system
resume and it triggers acpi_lid_notify_state() which invokes
acpi_pm_wakeup_event() for the lid device, the lid's wakeup count is
incremented even if the lid was not the source of the event that woke up
the system. That behavior confuses user space deamons using
wakeup_count to identify the potential system wakeup source. To avoid
the confusion, only trigger acpi_pm_wakeup_event() in the
acpi_button_notify() path and don't do that in the
acpi_lid_initialize_state() path.
Signed-off-by: Ravi Chandra Sadineni
---
v3: Change is_notification to signal_wakeup
V2: Increment the wakeup count only when the lid is open.
drivers/acpi/button.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5ee2dbbc..40ed3ec9fc94c 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -366,7 +363,8 @@ int acpi_lid_open(void)
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+bool signal_wakeup)
{
int state;
@@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
if (state < 0)
return state;
+ if (state && signal_wakeup)
+ acpi_pm_wakeup_event(>dev);
+
return acpi_lid_notify_state(device, state);
}
@@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device
*device)
(void)acpi_lid_notify_state(device, 1);
break;
case ACPI_BUTTON_LID_INIT_METHOD:
- (void)acpi_lid_update_state(device);
+ (void)acpi_lid_update_state(device, false);
break;
case ACPI_BUTTON_LID_INIT_IGNORE:
default:
@@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
users = button->input->users;
mutex_unlock(>input->mutex);
if (users)
- acpi_lid_update_state(device);
+ acpi_lid_update_state(device, true);
} else {
int keycode;
--
2.18.0.rc2.346.g013aa6912e-goog
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
Hi Rafael, Hopefully this will clear things a bit. 1. Why is this patch needed ? Consider the following scenario. 1. User left the device idle for some time. 2. A deamon in the userland that controls suspend policy might suspend the device. The lid is still open. 3. Now the user returns and wakes the system up by pressing a key. The system resumes. 4. In the current implementation we call pm_wakeup_event() (if the lid is open) irrespective of whether we have received a notify signal. 5. The deamon in the userland tries to identify what might woken the system up based on the wakeup_count. 6. The deamon sees a wakeup_count increment for both keyboard and lid and is confused. This patch is an attempt to address the same. 2. Will it break any existing logic ? AFAIK pm_wakeup_event() serves 2 purposes. 1. Helps preventing a race between system wide suspend and wakeup event. 2. Helps identifying the device that woke the system up. As far as preventing races during suspend is concerned, in the existing implementation, we increment the wakeup_count only when there is a lid open. Thus only lid open can prevent the system from suspending (if lid is a wake enabled device). But with my previous change, we will end up incrementing the wakeup_count for both lid close and lid open. Fixed this in V2, so that we do not increment the wakeup_count when there is a lid close. And currently we cannot identify if the lid is the reason for wake anyway. So this patch can only make things better here. Thanks, Ravi On Wed, Jun 6, 2018 at 4:21 PM, Rafael J. Wysocki wrote: > On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung wrote: >> Hi Rafael, >> >> On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote: >>> > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device >>> > *device, u32 event) >>> > /* fall through */ >>> > case ACPI_BUTTON_NOTIFY_STATUS: >>> > input = button->input; >>> > + acpi_pm_wakeup_event(>dev); >>> >>> Not really. >>> >>> There already is an acpi_pm_wakeup_event() call in the else branch below. >>> >> >> Ravi removes that other call below. > > OK, I missed that, not sure why, sorry. > >> The intent for this is to call >> acpi_pm_wakeup_event() regardless if the button->type is >> ACPI_BUTTON_TYPE_LID, >> in case that event is ACPI_BUTTON_NOTIFY_STATUS. > > Well, the patch really drops the acpi_pm_wakeup_event() call from > acpi_lid_notify_state() and so it has to ensure that this function > will be called here for ACPI_BUTTON_NOTIFY_STATUS regardless of the > button->type value. > > That's fine, but still the changelog doesn't make it clear why the > acpi_pm_wakeup_event() call in acpi_lid_notify_state() is not > necessary in general. > > Thanks, > Rafael
[PATCH V2] ACPI LID: increment wakeup count only when notified.
Currently ACPI LID increments wakeup count irrespective of the wake source.
This is because we call acpi_lid_initialize_state on every resume.
Userland deamons using wakeup_count to identify the potential wake
source for the last wake will be thrown off by this. Instead increment
the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event.
Signed-off-by: Ravi Chandra Sadineni
---
V2: Increment the wakeup count only when the lid is open.
drivers/acpi/button.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5ee2dbbc..d2fe03e4faf05 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -366,7 +363,8 @@ int acpi_lid_open(void)
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+bool is_notification)
{
int state;
@@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
if (state < 0)
return state;
+ if (state && is_notification)
+ acpi_pm_wakeup_event(>dev);
+
return acpi_lid_notify_state(device, state);
}
@@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device
*device)
(void)acpi_lid_notify_state(device, 1);
break;
case ACPI_BUTTON_LID_INIT_METHOD:
- (void)acpi_lid_update_state(device);
+ (void)acpi_lid_update_state(device, false);
break;
case ACPI_BUTTON_LID_INIT_IGNORE:
default:
@@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
users = button->input->users;
mutex_unlock(>input->mutex);
if (users)
- acpi_lid_update_state(device);
+ acpi_lid_update_state(device, true);
} else {
int keycode;
--
2.18.0.rc1.242.g61856ae69a-goog
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
Hi Rafael, Hopefully this will clear things a bit. 1. Why is this patch needed ? Consider the following scenario. 1. User left the device idle for some time. 2. A deamon in the userland that controls suspend policy might suspend the device. The lid is still open. 3. Now the user returns and wakes the system up by pressing a key. The system resumes. 4. In the current implementation we call pm_wakeup_event() (if the lid is open) irrespective of whether we have received a notify signal. 5. The deamon in the userland tries to identify what might woken the system up based on the wakeup_count. 6. The deamon sees a wakeup_count increment for both keyboard and lid and is confused. This patch is an attempt to address the same. 2. Will it break any existing logic ? AFAIK pm_wakeup_event() serves 2 purposes. 1. Helps preventing a race between system wide suspend and wakeup event. 2. Helps identifying the device that woke the system up. As far as preventing races during suspend is concerned, in the existing implementation, we increment the wakeup_count only when there is a lid open. Thus only lid open can prevent the system from suspending (if lid is a wake enabled device). But with my previous change, we will end up incrementing the wakeup_count for both lid close and lid open. Fixed this in V2, so that we do not increment the wakeup_count when there is a lid close. And currently we cannot identify if the lid is the reason for wake anyway. So this patch can only make things better here. Thanks, Ravi On Wed, Jun 6, 2018 at 4:21 PM, Rafael J. Wysocki wrote: > On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung wrote: >> Hi Rafael, >> >> On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote: >>> > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device >>> > *device, u32 event) >>> > /* fall through */ >>> > case ACPI_BUTTON_NOTIFY_STATUS: >>> > input = button->input; >>> > + acpi_pm_wakeup_event(>dev); >>> >>> Not really. >>> >>> There already is an acpi_pm_wakeup_event() call in the else branch below. >>> >> >> Ravi removes that other call below. > > OK, I missed that, not sure why, sorry. > >> The intent for this is to call >> acpi_pm_wakeup_event() regardless if the button->type is >> ACPI_BUTTON_TYPE_LID, >> in case that event is ACPI_BUTTON_NOTIFY_STATUS. > > Well, the patch really drops the acpi_pm_wakeup_event() call from > acpi_lid_notify_state() and so it has to ensure that this function > will be called here for ACPI_BUTTON_NOTIFY_STATUS regardless of the > button->type value. > > That's fine, but still the changelog doesn't make it clear why the > acpi_pm_wakeup_event() call in acpi_lid_notify_state() is not > necessary in general. > > Thanks, > Rafael
[PATCH V2] ACPI LID: increment wakeup count only when notified.
Currently ACPI LID increments wakeup count irrespective of the wake source.
This is because we call acpi_lid_initialize_state on every resume.
Userland deamons using wakeup_count to identify the potential wake
source for the last wake will be thrown off by this. Instead increment
the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event.
Signed-off-by: Ravi Chandra Sadineni
---
V2: Increment the wakeup count only when the lid is open.
drivers/acpi/button.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 2345a5ee2dbbc..d2fe03e4faf05 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -366,7 +363,8 @@ int acpi_lid_open(void)
}
EXPORT_SYMBOL(acpi_lid_open);
-static int acpi_lid_update_state(struct acpi_device *device)
+static int acpi_lid_update_state(struct acpi_device *device,
+bool is_notification)
{
int state;
@@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device)
if (state < 0)
return state;
+ if (state && is_notification)
+ acpi_pm_wakeup_event(>dev);
+
return acpi_lid_notify_state(device, state);
}
@@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device
*device)
(void)acpi_lid_notify_state(device, 1);
break;
case ACPI_BUTTON_LID_INIT_METHOD:
- (void)acpi_lid_update_state(device);
+ (void)acpi_lid_update_state(device, false);
break;
case ACPI_BUTTON_LID_INIT_IGNORE:
default:
@@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
users = button->input->users;
mutex_unlock(>input->mutex);
if (users)
- acpi_lid_update_state(device);
+ acpi_lid_update_state(device, true);
} else {
int keycode;
--
2.18.0.rc1.242.g61856ae69a-goog
Re: [PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Hi Rafeal, Soft ping. Is this patch good to be merged ? Thanks, Ravi On Sun, Jun 3, 2018 at 10:14 AM, Ravi Chandra Sadineni wrote: > Hi Rafael, > > On Sun, Jun 3, 2018 at 1:05 AM, Rafael J. Wysocki wrote: >> On Sat, Jun 2, 2018 at 4:32 AM, Ravi Chandra Sadineni >> wrote: >>> Currently we show event_count instead of wakeup_count as part of per >>> device wakeup_count sysfs attribute. Change it to wakeup_count to make >>> it more meaningful. >> >> More information, please. >> >> In particular, why it is more meaningful. > Wakeup_count increments only when events_check_enabled is set. This > bool is set whenever we write current wakeup count to > /sys/power/wakeup_count from the user land. Also events_check_enabled > is cleared on every resume. My understanding is that, userland is > expected to write to this just before suspend. This way > pm_wakeup_event() when called from irqs will increment the > wakeup_count only if we are in system wide suspend resume cycle and > should give a fair approximation of how many times a device might have > caused a wake from S3/S0iX. event_count on the other hand will > increment every time pm_wakeup_event() is called irrespective of > whether we are in a suspend/resume cycle. For example when I try > doing something like this (https://lkml.org/lkml/2018/6/1/890), we see > the wakeup_count sysfs attribute for the particular device > incrementing every time there is a irq. If it is important to expose > event_count via sysfs attribute, should we create another attribute ? > Also we do expose each of these counters via > debugfs(/sys/kernel/debug/wake_sources). > > Please correct me if I am wrong or missing something. Also if there is > a better way to do this, please let me know. >> >> Thanks, >> Rafael
Re: [PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Hi Rafeal, Soft ping. Is this patch good to be merged ? Thanks, Ravi On Sun, Jun 3, 2018 at 10:14 AM, Ravi Chandra Sadineni wrote: > Hi Rafael, > > On Sun, Jun 3, 2018 at 1:05 AM, Rafael J. Wysocki wrote: >> On Sat, Jun 2, 2018 at 4:32 AM, Ravi Chandra Sadineni >> wrote: >>> Currently we show event_count instead of wakeup_count as part of per >>> device wakeup_count sysfs attribute. Change it to wakeup_count to make >>> it more meaningful. >> >> More information, please. >> >> In particular, why it is more meaningful. > Wakeup_count increments only when events_check_enabled is set. This > bool is set whenever we write current wakeup count to > /sys/power/wakeup_count from the user land. Also events_check_enabled > is cleared on every resume. My understanding is that, userland is > expected to write to this just before suspend. This way > pm_wakeup_event() when called from irqs will increment the > wakeup_count only if we are in system wide suspend resume cycle and > should give a fair approximation of how many times a device might have > caused a wake from S3/S0iX. event_count on the other hand will > increment every time pm_wakeup_event() is called irrespective of > whether we are in a suspend/resume cycle. For example when I try > doing something like this (https://lkml.org/lkml/2018/6/1/890), we see > the wakeup_count sysfs attribute for the particular device > incrementing every time there is a irq. If it is important to expose > event_count via sysfs attribute, should we create another attribute ? > Also we do expose each of these counters via > debugfs(/sys/kernel/debug/wake_sources). > > Please correct me if I am wrong or missing something. Also if there is > a better way to do this, please let me know. >> >> Thanks, >> Rafael
[PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Remove the unnecessary check before calling pm_wakeup_event. If the
device is not wake enabled, this call is no-op anyway.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/input/keyboard/cros_ec_keyb.c | 30 ++-
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 489ddd37bd4ee..c5e32544130dc 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -242,19 +242,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
u32 val;
unsigned int ev_type;
+ /*
+* If not wake enabled, discard key state changes during
+* suspend. Switches will be re-checked in
+* cros_ec_keyb_resume() to be sure nothing is lost.
+*/
+ if (queued_during_suspend && !device_may_wakeup(ckdev->dev))
+ return NOTIFY_OK;
+
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
- if (device_may_wakeup(ckdev->dev)) {
- pm_wakeup_event(ckdev->dev, 0);
- } else {
- /*
-* If keyboard is not wake enabled, discard key state
-* changes during suspend. Switches will be re-checked
-* in cros_ec_keyb_resume() to be sure nothing is lost.
-*/
- if (queued_during_suspend)
- return NOTIFY_OK;
- }
+ pm_wakeup_event(ckdev->dev, 0);
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
@@ -268,10 +266,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
break;
case EC_MKBP_EVENT_SYSRQ:
- if (device_may_wakeup(ckdev->dev))
- pm_wakeup_event(ckdev->dev, 0);
- else if (queued_during_suspend)
- return NOTIFY_OK;
+ pm_wakeup_event(ckdev->dev, 0);
val = get_unaligned_le32(>ec->event_data.data.sysrq);
dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
@@ -280,10 +275,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
- if (device_may_wakeup(ckdev->dev))
- pm_wakeup_event(ckdev->dev, 0);
- else if (queued_during_suspend)
- return NOTIFY_OK;
+ pm_wakeup_event(ckdev->dev, 0);
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
--
2.18.0.rc1.242.g61856ae69a-goog
[PATCH] Input: cros_ec_keyb: Remove check before calling pm_wakeup_event.
Remove the unnecessary check before calling pm_wakeup_event. If the
device is not wake enabled, this call is no-op anyway.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/input/keyboard/cros_ec_keyb.c | 30 ++-
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 489ddd37bd4ee..c5e32544130dc 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -242,19 +242,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
u32 val;
unsigned int ev_type;
+ /*
+* If not wake enabled, discard key state changes during
+* suspend. Switches will be re-checked in
+* cros_ec_keyb_resume() to be sure nothing is lost.
+*/
+ if (queued_during_suspend && !device_may_wakeup(ckdev->dev))
+ return NOTIFY_OK;
+
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
- if (device_may_wakeup(ckdev->dev)) {
- pm_wakeup_event(ckdev->dev, 0);
- } else {
- /*
-* If keyboard is not wake enabled, discard key state
-* changes during suspend. Switches will be re-checked
-* in cros_ec_keyb_resume() to be sure nothing is lost.
-*/
- if (queued_during_suspend)
- return NOTIFY_OK;
- }
+ pm_wakeup_event(ckdev->dev, 0);
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
@@ -268,10 +266,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
break;
case EC_MKBP_EVENT_SYSRQ:
- if (device_may_wakeup(ckdev->dev))
- pm_wakeup_event(ckdev->dev, 0);
- else if (queued_during_suspend)
- return NOTIFY_OK;
+ pm_wakeup_event(ckdev->dev, 0);
val = get_unaligned_le32(>ec->event_data.data.sysrq);
dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
@@ -280,10 +275,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
- if (device_may_wakeup(ckdev->dev))
- pm_wakeup_event(ckdev->dev, 0);
- else if (queued_during_suspend)
- return NOTIFY_OK;
+ pm_wakeup_event(ckdev->dev, 0);
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
--
2.18.0.rc1.242.g61856ae69a-goog
Re: [PATCH V2] i8042: Increment wakeup_count for the respective port.
On Tue, Jun 5, 2018 at 2:14 AM, Rafael J. Wysocki wrote: > On Mon, Jun 4, 2018 at 11:53 PM, Dmitry Torokhov > wrote: >> On Fri, Jun 01, 2018 at 06:07:08PM -0700, Ravi Chandra Sadineni wrote: >>> Call pm_wakeup_event on every irq. This should help us in identifying if >>> keyboard was a potential wake reason for the last resume. >>> >>> Signed-off-by: Ravi Chandra Sadineni >>> --- >>> V2: Increment the wakeup count only when there is a irq and not when the >>> method is called internally. >>> >>> drivers/input/serio/i8042.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c >>> index 824f4c1c1f310..2bd6f2633e29a 100644 >>> --- a/drivers/input/serio/i8042.c >>> +++ b/drivers/input/serio/i8042.c >>> @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void >>> *dev_id) >>> port = _ports[port_no]; >>> serio = port->exists ? port->serio : NULL; >>> >>> + if (irq && serio && device_may_wakeup(>dev)) >>> + pm_wakeup_event(>dev, 0); >> >> The constant checks for device_may_wakeup() before calling >> pm_wakeup_event()needed to avoid warnings in wakeup_source_activate() >> (?) are annoying. > > I'm not following you here. > > pm_wakeup_event() -> > pm_wakeup_dev_event() -> > pm_wakeup_ws_event(dev->power.wakeup, ...) > Checks if the first arg is NULL and returns quietly if so. > > I don't see why you need the device_may_wakeup() check. Just realized that device_may_wakeup check is not needed. Removed the check in V3. Thanks. > >> Rafael, can we move the check into pm_wakeup_dev_event()? > > That would be redundant, wouldn't it? > >> I am also confused when pm_wakeup_event() vs pm_wakeup_hard_event() vs >> pm_wakeup_dev_event() should be used, if any. Is there any guidance? > > First off, the "hard" variant is for when you want to abort suspends > in progress or wake up from suspend to idle regardless of whether or > not wakeup source tracking is enabled. > > Second, use pm_wakeup_dev_event() if the decision on "hard" vs "soft" > needs to be made at run time. > > Thanks, > Rafael
Re: [PATCH V2] i8042: Increment wakeup_count for the respective port.
On Tue, Jun 5, 2018 at 2:14 AM, Rafael J. Wysocki wrote: > On Mon, Jun 4, 2018 at 11:53 PM, Dmitry Torokhov > wrote: >> On Fri, Jun 01, 2018 at 06:07:08PM -0700, Ravi Chandra Sadineni wrote: >>> Call pm_wakeup_event on every irq. This should help us in identifying if >>> keyboard was a potential wake reason for the last resume. >>> >>> Signed-off-by: Ravi Chandra Sadineni >>> --- >>> V2: Increment the wakeup count only when there is a irq and not when the >>> method is called internally. >>> >>> drivers/input/serio/i8042.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c >>> index 824f4c1c1f310..2bd6f2633e29a 100644 >>> --- a/drivers/input/serio/i8042.c >>> +++ b/drivers/input/serio/i8042.c >>> @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void >>> *dev_id) >>> port = _ports[port_no]; >>> serio = port->exists ? port->serio : NULL; >>> >>> + if (irq && serio && device_may_wakeup(>dev)) >>> + pm_wakeup_event(>dev, 0); >> >> The constant checks for device_may_wakeup() before calling >> pm_wakeup_event()needed to avoid warnings in wakeup_source_activate() >> (?) are annoying. > > I'm not following you here. > > pm_wakeup_event() -> > pm_wakeup_dev_event() -> > pm_wakeup_ws_event(dev->power.wakeup, ...) > Checks if the first arg is NULL and returns quietly if so. > > I don't see why you need the device_may_wakeup() check. Just realized that device_may_wakeup check is not needed. Removed the check in V3. Thanks. > >> Rafael, can we move the check into pm_wakeup_dev_event()? > > That would be redundant, wouldn't it? > >> I am also confused when pm_wakeup_event() vs pm_wakeup_hard_event() vs >> pm_wakeup_dev_event() should be used, if any. Is there any guidance? > > First off, the "hard" variant is for when you want to abort suspends > in progress or wake up from suspend to idle regardless of whether or > not wakeup source tracking is enabled. > > Second, use pm_wakeup_dev_event() if the decision on "hard" vs "soft" > needs to be made at run time. > > Thanks, > Rafael
[PATCH V3] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if keyboard was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- V3: Remove the unnecessary device_may_wakeup check. V2: Increment the wakeup count only when there is a irq and not when the method is called internally. drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..b8bc71569349d 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (irq && serio) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.1.1185.g55be947832-goog
[PATCH V3] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if keyboard was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- V3: Remove the unnecessary device_may_wakeup check. V2: Increment the wakeup count only when there is a irq and not when the method is called internally. drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..b8bc71569349d 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (irq && serio) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.1.1185.g55be947832-goog
[PATCH] ACPI LID: increment wakeup count only when notified.
Currently ACPI LID increments wakeup count irrespective of the wake source.
This is because we call acpi_lid_initialize_state on every resume.
Userland deamons using wakeup_count to identify the potential wake
source for the last wake will be thrown off by this. Instead increment
the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/acpi/button.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index f1cc4f9d31cd9..d40fef7241f08 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
/* fall through */
case ACPI_BUTTON_NOTIFY_STATUS:
input = button->input;
+ acpi_pm_wakeup_event(>dev);
if (button->type == ACPI_BUTTON_TYPE_LID) {
mutex_lock(>input->mutex);
users = button->input->users;
@@ -426,7 +424,6 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
} else {
int keycode;
- acpi_pm_wakeup_event(>dev);
if (button->suspended)
break;
--
2.17.1.1185.g55be947832-goog
[PATCH] ACPI LID: increment wakeup count only when notified.
Currently ACPI LID increments wakeup count irrespective of the wake source.
This is because we call acpi_lid_initialize_state on every resume.
Userland deamons using wakeup_count to identify the potential wake
source for the last wake will be thrown off by this. Instead increment
the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/acpi/button.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index f1cc4f9d31cd9..d40fef7241f08 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device
*device, int state)
button->last_time = ktime_get();
}
- if (state)
- acpi_pm_wakeup_event(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_lid_notifier, state,
@@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
/* fall through */
case ACPI_BUTTON_NOTIFY_STATUS:
input = button->input;
+ acpi_pm_wakeup_event(>dev);
if (button->type == ACPI_BUTTON_TYPE_LID) {
mutex_lock(>input->mutex);
users = button->input->users;
@@ -426,7 +424,6 @@ static void acpi_button_notify(struct acpi_device *device,
u32 event)
} else {
int keycode;
- acpi_pm_wakeup_event(>dev);
if (button->suspended)
break;
--
2.17.1.1185.g55be947832-goog
Re: [PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Hi Rafael, On Sun, Jun 3, 2018 at 1:05 AM, Rafael J. Wysocki wrote: > On Sat, Jun 2, 2018 at 4:32 AM, Ravi Chandra Sadineni > wrote: >> Currently we show event_count instead of wakeup_count as part of per >> device wakeup_count sysfs attribute. Change it to wakeup_count to make >> it more meaningful. > > More information, please. > > In particular, why it is more meaningful. Wakeup_count increments only when events_check_enabled is set. This bool is set whenever we write current wakeup count to /sys/power/wakeup_count from the user land. Also events_check_enabled is cleared on every resume. My understanding is that, userland is expected to write to this just before suspend. This way pm_wakeup_event() when called from irqs will increment the wakeup_count only if we are in system wide suspend resume cycle and should give a fair approximation of how many times a device might have caused a wake from S3/S0iX. event_count on the other hand will increment every time pm_wakeup_event() is called irrespective of whether we are in a suspend/resume cycle. For example when I try doing something like this (https://lkml.org/lkml/2018/6/1/890), we see the wakeup_count sysfs attribute for the particular device incrementing every time there is a irq. If it is important to expose event_count via sysfs attribute, should we create another attribute ? Also we do expose each of these counters via debugfs(/sys/kernel/debug/wake_sources). Please correct me if I am wrong or missing something. Also if there is a better way to do this, please let me know. > > Thanks, > Rafael
Re: [PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Hi Rafael, On Sun, Jun 3, 2018 at 1:05 AM, Rafael J. Wysocki wrote: > On Sat, Jun 2, 2018 at 4:32 AM, Ravi Chandra Sadineni > wrote: >> Currently we show event_count instead of wakeup_count as part of per >> device wakeup_count sysfs attribute. Change it to wakeup_count to make >> it more meaningful. > > More information, please. > > In particular, why it is more meaningful. Wakeup_count increments only when events_check_enabled is set. This bool is set whenever we write current wakeup count to /sys/power/wakeup_count from the user land. Also events_check_enabled is cleared on every resume. My understanding is that, userland is expected to write to this just before suspend. This way pm_wakeup_event() when called from irqs will increment the wakeup_count only if we are in system wide suspend resume cycle and should give a fair approximation of how many times a device might have caused a wake from S3/S0iX. event_count on the other hand will increment every time pm_wakeup_event() is called irrespective of whether we are in a suspend/resume cycle. For example when I try doing something like this (https://lkml.org/lkml/2018/6/1/890), we see the wakeup_count sysfs attribute for the particular device incrementing every time there is a irq. If it is important to expose event_count via sysfs attribute, should we create another attribute ? Also we do expose each of these counters via debugfs(/sys/kernel/debug/wake_sources). Please correct me if I am wrong or missing something. Also if there is a better way to do this, please let me know. > > Thanks, > Rafael
[PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Currently we show event_count instead of wakeup_count as part of per
device wakeup_count sysfs attribute. Change it to wakeup_count to make
it more meaningful.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/base/power/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 0f651efc58a1a..d713738ce7967 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -353,7 +353,7 @@ static ssize_t wakeup_count_show(struct device *dev,
spin_lock_irq(>power.lock);
if (dev->power.wakeup) {
- count = dev->power.wakeup->event_count;
+ count = dev->power.wakeup->wakeup_count;
enabled = true;
}
spin_unlock_irq(>power.lock);
--
2.17.1.1185.g55be947832-goog
[PATCH] power: Print wakeup_count instead of event_count in the sysfs attribute.
Currently we show event_count instead of wakeup_count as part of per
device wakeup_count sysfs attribute. Change it to wakeup_count to make
it more meaningful.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/base/power/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 0f651efc58a1a..d713738ce7967 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -353,7 +353,7 @@ static ssize_t wakeup_count_show(struct device *dev,
spin_lock_irq(>power.lock);
if (dev->power.wakeup) {
- count = dev->power.wakeup->event_count;
+ count = dev->power.wakeup->wakeup_count;
enabled = true;
}
spin_unlock_irq(>power.lock);
--
2.17.1.1185.g55be947832-goog
[PATCH V2] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if keyboard was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- V2: Increment the wakeup count only when there is a irq and not when the method is called internally. drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..2bd6f2633e29a 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (irq && serio && device_may_wakeup(>dev)) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.1.1185.g55be947832-goog
[PATCH V2] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if keyboard was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- V2: Increment the wakeup count only when there is a irq and not when the method is called internally. drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..2bd6f2633e29a 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (irq && serio && device_may_wakeup(>dev)) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.1.1185.g55be947832-goog
[PATCH] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if i8042 port was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..96e27766d553d 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (serio && device_may_wakeup(>dev)) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.0.921.gf22659ad46-goog
[PATCH] i8042: Increment wakeup_count for the respective port.
Call pm_wakeup_event on every irq. This should help us in identifying if i8042 port was a potential wake reason for the last resume. Signed-off-by: Ravi Chandra Sadineni --- drivers/input/serio/i8042.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index 824f4c1c1f310..96e27766d553d 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id) port = _ports[port_no]; serio = port->exists ? port->serio : NULL; + if (serio && device_may_wakeup(>dev)) + pm_wakeup_event(>dev, 0); + filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n", port_no, irq, dfl & SERIO_PARITY ? ", bad parity" : "", -- 2.17.0.921.gf22659ad46-goog
[PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
related to keyboard, call pm_wakeup_event() to make sure wakeup
triggers are accounted to keyb during suspend resume path.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
V2: Marked the ckdev as wake enabled instead of input devices.
drivers/input/keyboard/cros_ec_keyb.c | 21 +
drivers/mfd/cros_ec.c | 19 +++
2 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..a7c96f0317123 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
-* If EC is not the wake source, discard key state changes
+* If Keyb is not wake enabled, discard key state changes
* during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -265,18 +270,25 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
val = get_unaligned_le32(>ec->event_data.data.sysrq);
dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
handle_sysrq(val);
+
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
break;
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
-* If EC is not the wake source, discard key state
+* If keyb is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
>ec->event_data.data.buttons);
@@ -639,6 +651,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
return err;
}
+ device_init_wakeup(ckdev->dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..36156a41499c9 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -229,7 +229,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(_dev->event_notifier,
@@ -253,21 +253,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
-* In some cases, we need to distinguish between events that occur
-* during suspend if the EC is not a wake source. For example,
-* keypresses during suspend should be discarded if it does not wake
-* the system.
-*
-* If the EC is not a wake source, drain the event queue and mark them
-* as "queued during suspend".
-*/
if (ec_dev->wake_enabled) {
disable_irq_wake(ec_dev->irq);
ec_dev->wake_enabled = 0;
- } else {
- cros_ec_drain_events(ec_dev);
}
+ /*
+* Let the mfd devices know about events that occur during
+* suspend. This way the clients know what to do with them.
+*/
+ cros_ec_report_events_during_suspend(ec_dev);
+
return 0;
}
--
2.17.0.921.gf22659ad46-goog
[PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
related to keyboard, call pm_wakeup_event() to make sure wakeup
triggers are accounted to keyb during suspend resume path.
Signed-off-by: Ravi Chandra Sadineni
---
V2: Marked the ckdev as wake enabled instead of input devices.
drivers/input/keyboard/cros_ec_keyb.c | 21 +
drivers/mfd/cros_ec.c | 19 +++
2 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..a7c96f0317123 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
-* If EC is not the wake source, discard key state changes
+* If Keyb is not wake enabled, discard key state changes
* during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -265,18 +270,25 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
val = get_unaligned_le32(>ec->event_data.data.sysrq);
dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
handle_sysrq(val);
+
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
break;
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
-* If EC is not the wake source, discard key state
+* If keyb is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
>ec->event_data.data.buttons);
@@ -639,6 +651,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
return err;
}
+ device_init_wakeup(ckdev->dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..36156a41499c9 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -229,7 +229,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(_dev->event_notifier,
@@ -253,21 +253,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
-* In some cases, we need to distinguish between events that occur
-* during suspend if the EC is not a wake source. For example,
-* keypresses during suspend should be discarded if it does not wake
-* the system.
-*
-* If the EC is not a wake source, drain the event queue and mark them
-* as "queued during suspend".
-*/
if (ec_dev->wake_enabled) {
disable_irq_wake(ec_dev->irq);
ec_dev->wake_enabled = 0;
- } else {
- cros_ec_drain_events(ec_dev);
}
+ /*
+* Let the mfd devices know about events that occur during
+* suspend. This way the clients know what to do with them.
+*/
+ cros_ec_report_events_during_suspend(ec_dev);
+
return 0;
}
--
2.17.0.921.gf22659ad46-goog
[PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device.
If the IRQ is processed during resume, increment the wakeup count to the
specific mfd device based on the event, if the mfd device is wake enabled.
This helps in identifying the specific device that caused the wake.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
drivers/input/keyboard/cros_ec_keyb.c | 20 +++-
drivers/mfd/cros_ec.c | 25 +++--
2 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..9b39289774405 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,16 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
-* If EC is not the wake source, discard key state changes
-* during suspend.
+* If keyb input device is not wake enabled, discard key
+* state changes during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->idev
+ && !device_may_wakeup(>idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->idev)
+ pm_wakeup_event(>idev->dev, 0);
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -270,13 +274,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
-* If EC is not the wake source, discard key state
+* If bs is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->bs_idev
+ && !device_may_wakeup(>bs_idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->bs_idev)
+ pm_wakeup_event(>bs_idev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
>ec->event_data.data.buttons);
@@ -522,6 +530,7 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb
*ckdev)
return ret;
}
+ device_init_wakeup(>dev, true);
return 0;
}
@@ -598,6 +607,7 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb
*ckdev)
return err;
}
+ device_init_wakeup(>dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..df9520365e54b 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -64,12 +64,14 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
* cros_ec_get_next_event() returned an error (default value for
* wake_event is true)
*/
- if (wake_event && device_may_wakeup(ec_dev->dev))
+ if (device_may_wakeup(ec_dev->dev) && wake_event
+ && ec_dev->wake_enabled)
pm_wakeup_event(ec_dev->dev, 0);
if (ret > 0)
blocking_notifier_call_chain(_dev->event_notifier,
-0, ec_dev);
+wake_event && ec_dev->wake_enabled,
+ec_dev);
return IRQ_HANDLED;
}
@@ -229,7 +231,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(_dev->event_notifier,
@@ -253,21 +255,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
-* In some cases, we need to distinguish between events that occur
-* during suspend if the EC is not a wake source. For example,
-* keypresses during suspend should be discarded if it does not wake
-* the system.
-*
-* If the EC is not a wake source, drain the event qu
[PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device.
If the IRQ is processed during resume, increment the wakeup count to the
specific mfd device based on the event, if the mfd device is wake enabled.
This helps in identifying the specific device that caused the wake.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/input/keyboard/cros_ec_keyb.c | 20 +++-
drivers/mfd/cros_ec.c | 25 +++--
2 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..9b39289774405 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,16 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
-* If EC is not the wake source, discard key state changes
-* during suspend.
+* If keyb input device is not wake enabled, discard key
+* state changes during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->idev
+ && !device_may_wakeup(>idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->idev)
+ pm_wakeup_event(>idev->dev, 0);
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -270,13 +274,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
-* If EC is not the wake source, discard key state
+* If bs is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->bs_idev
+ && !device_may_wakeup(>bs_idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->bs_idev)
+ pm_wakeup_event(>bs_idev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
>ec->event_data.data.buttons);
@@ -522,6 +530,7 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb
*ckdev)
return ret;
}
+ device_init_wakeup(>dev, true);
return 0;
}
@@ -598,6 +607,7 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb
*ckdev)
return err;
}
+ device_init_wakeup(>dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..df9520365e54b 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -64,12 +64,14 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
* cros_ec_get_next_event() returned an error (default value for
* wake_event is true)
*/
- if (wake_event && device_may_wakeup(ec_dev->dev))
+ if (device_may_wakeup(ec_dev->dev) && wake_event
+ && ec_dev->wake_enabled)
pm_wakeup_event(ec_dev->dev, 0);
if (ret > 0)
blocking_notifier_call_chain(_dev->event_notifier,
-0, ec_dev);
+wake_event && ec_dev->wake_enabled,
+ec_dev);
return IRQ_HANDLED;
}
@@ -229,7 +231,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(_dev->event_notifier,
@@ -253,21 +255,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
-* In some cases, we need to distinguish between events that occur
-* during suspend if the EC is not a wake source. For example,
-* keypresses during suspend should be discarded if it does not wake
-* the system.
-*
-* If the EC is not a wake source, drain the event queue and mark them
-* as "queued during
Re: [PATCH V5] USB: Increment wakeup count on remote wakeup.
Sure. Pushing it to the older kernels will definitely help. Thanks, Ravi On Sat, Apr 21, 2018 at 1:59 AM, Greg KH <gre...@linuxfoundation.org> wrote: > On Fri, Apr 20, 2018 at 11:08:21AM -0700, Ravi Chandra Sadineni wrote: >> On chromebooks we depend on wakeup count to identify the wakeup source. >> But currently USB devices do not increment the wakeup count when they >> trigger the remote wake. This patch addresses the same. >> >> Resume condition is reported differently on USB 2.0 and USB 3.0 devices. >> >> On USB 2.0 devices, a wake capable device, if wake enabled, drives >> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). >> The upstream facing port then sets C_PORT_SUSPEND bit and reports a >> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port >> has resumed before driving the resume signal from the host and >> C_PORT_SUSPEND is set, then the device attached to the given port might >> be the reason for the last system wakeup. Increment the wakeup count for >> the same. >> >> On USB 3.0 devices, a function may signal that it wants to exit from device >> suspend by sending a Function Wake Device Notification to the host (USB3.0 >> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the >> wakeup count. >> >> Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org> >> --- >> >> V5: Added the description of changes between different versions of patches. >> V4: Moved the wakeup count increment logic to the existing if which is >> safegaurded by hcd_root_hub_lock spinlock. >> V3: Added a gaurd to check if rh_registered is set before accessing >> root_hub pointer. >> V2: Fixed the build failure error due to uninitialized dev pointer. > > Is this needed in older kernels? Should I submit it to the stable > trees? > > thanks, > > greg k-h
Re: [PATCH V5] USB: Increment wakeup count on remote wakeup.
Sure. Pushing it to the older kernels will definitely help. Thanks, Ravi On Sat, Apr 21, 2018 at 1:59 AM, Greg KH wrote: > On Fri, Apr 20, 2018 at 11:08:21AM -0700, Ravi Chandra Sadineni wrote: >> On chromebooks we depend on wakeup count to identify the wakeup source. >> But currently USB devices do not increment the wakeup count when they >> trigger the remote wake. This patch addresses the same. >> >> Resume condition is reported differently on USB 2.0 and USB 3.0 devices. >> >> On USB 2.0 devices, a wake capable device, if wake enabled, drives >> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7). >> The upstream facing port then sets C_PORT_SUSPEND bit and reports a >> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port >> has resumed before driving the resume signal from the host and >> C_PORT_SUSPEND is set, then the device attached to the given port might >> be the reason for the last system wakeup. Increment the wakeup count for >> the same. >> >> On USB 3.0 devices, a function may signal that it wants to exit from device >> suspend by sending a Function Wake Device Notification to the host (USB3.0 >> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the >> wakeup count. >> >> Signed-off-by: Ravi Chandra Sadineni >> --- >> >> V5: Added the description of changes between different versions of patches. >> V4: Moved the wakeup count increment logic to the existing if which is >> safegaurded by hcd_root_hub_lock spinlock. >> V3: Added a gaurd to check if rh_registered is set before accessing >> root_hub pointer. >> V2: Fixed the build failure error due to uninitialized dev pointer. > > Is this needed in older kernels? Should I submit it to the stable > trees? > > thanks, > > greg k-h
Re: [PATCH V3] USB: Increment wakeup count on remote wakeup.
On Fri, Apr 20, 2018 at 10:29 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Fri, 20 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
>> ---
>
> At this point you're supposed to list the differences between this
> patch and the preceding versions.
Mentioned the changes between different versions in V5. Thanks.
>
>> drivers/usb/core/hcd.c | 2 ++
>> drivers/usb/core/hub.c | 10 +-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..b8024ae4fdcaa 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>>
>> + if (hcd->rh_registered)
>> + pm_wakeup_event(>self.root_hub->dev, 0);
>> spin_lock_irqsave (_root_hub_lock, flags);
>> if (hcd->rh_registered) {
>> set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> This isn't good enough. hcd->rh_registered can change at any time;
> it is protected by the hcd_root_hub_lock spinlock. That's why I said
> your code should be moved inside the existing "if" statement.
Sorry about this. Fixed it now. Hope this is fine. Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>> unsigned int portnum)
>> {
>> struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>> if (!hdev)
>> return;
>>
>> hub = usb_hub_to_struct_hub(hdev);
>> if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>> set_bit(portnum, hub->wakeup_bits);
>> kick_hub_wq(hub);
>> }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
Re: [PATCH V3] USB: Increment wakeup count on remote wakeup.
On Fri, Apr 20, 2018 at 10:29 AM, Alan Stern wrote:
> On Fri, 20 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni
>> ---
>
> At this point you're supposed to list the differences between this
> patch and the preceding versions.
Mentioned the changes between different versions in V5. Thanks.
>
>> drivers/usb/core/hcd.c | 2 ++
>> drivers/usb/core/hub.c | 10 +-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..b8024ae4fdcaa 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>>
>> + if (hcd->rh_registered)
>> + pm_wakeup_event(>self.root_hub->dev, 0);
>> spin_lock_irqsave (_root_hub_lock, flags);
>> if (hcd->rh_registered) {
>> set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> This isn't good enough. hcd->rh_registered can change at any time;
> it is protected by the hcd_root_hub_lock spinlock. That's why I said
> your code should be moved inside the existing "if" statement.
Sorry about this. Fixed it now. Hope this is fine. Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>> unsigned int portnum)
>> {
>> struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>> if (!hdev)
>> return;
>>
>> hub = usb_hub_to_struct_hub(hdev);
>> if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>> set_bit(portnum, hub->wakeup_bits);
>> kick_hub_wq(hub);
>> }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
[PATCH V5] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
V5: Added the description of changes between different versions of patches.
V4: Moved the wakeup count increment logic to the existing if which is
safegaurded by hcd_root_hub_lock spinlock.
V3: Added a gaurd to check if rh_registered is set before accessing
root_hub pointer.
V2: Fixed the build failure error due to uninitialized dev pointer.
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 10 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+ pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
[PATCH V5] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni
---
V5: Added the description of changes between different versions of patches.
V4: Moved the wakeup count increment logic to the existing if which is
safegaurded by hcd_root_hub_lock spinlock.
V3: Added a gaurd to check if rh_registered is set before accessing
root_hub pointer.
V2: Fixed the build failure error due to uninitialized dev pointer.
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 10 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+ pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
[PATCH V4] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 10 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+ pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
[PATCH V4] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 10 +-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+ pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
Re: [PATCH V2] USB: Increment wakeup count on remote wakeup.
On Fri, Apr 20, 2018 at 7:12 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 19 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
>> ---
>> drivers/usb/core/hcd.c | 2 ++
>> drivers/usb/core/hub.c | 10 +-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..ee0f3ec57ce49 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
>> void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>> + struct device *dev = >self.root_hub->dev;
>>
>> + pm_wakeup_event(dev, 0);
>> spin_lock_irqsave (_root_hub_lock, flags);
>> if (hcd->rh_registered) {
>> set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> In general, hcd->self.root_hub is guaranteed to be non-NULL only when
> hcd->rh_registered is set. Therefore the assignment to dev and the
> call to pm_wakeup_event() should be moved within this "if" statement.
>
> The rest of the patch looks okay.
Added the check in V3. Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>> unsigned int portnum)
>> {
>> struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>> if (!hdev)
>> return;
>>
>> hub = usb_hub_to_struct_hub(hdev);
>> if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>> set_bit(portnum, hub->wakeup_bits);
>> kick_hub_wq(hub);
>> }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
Re: [PATCH V2] USB: Increment wakeup count on remote wakeup.
On Fri, Apr 20, 2018 at 7:12 AM, Alan Stern wrote:
> On Thu, 19 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni
>> ---
>> drivers/usb/core/hcd.c | 2 ++
>> drivers/usb/core/hub.c | 10 +-
>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..ee0f3ec57ce49 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
>> void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>> + struct device *dev = >self.root_hub->dev;
>>
>> + pm_wakeup_event(dev, 0);
>> spin_lock_irqsave (_root_hub_lock, flags);
>> if (hcd->rh_registered) {
>> set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> In general, hcd->self.root_hub is guaranteed to be non-NULL only when
> hcd->rh_registered is set. Therefore the assignment to dev and the
> call to pm_wakeup_event() should be moved within this "if" statement.
>
> The rest of the patch looks okay.
Added the check in V3. Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>> unsigned int portnum)
>> {
>> struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>> if (!hdev)
>> return;
>>
>> hub = usb_hub_to_struct_hub(hdev);
>> if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>> set_bit(portnum, hub->wakeup_bits);
>> kick_hub_wq(hub);
>> }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
[PATCH V3] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
drivers/usb/core/hcd.c | 2 ++
drivers/usb/core/hub.c | 10 +-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..b8024ae4fdcaa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ if (hcd->rh_registered)
+ pm_wakeup_event(>self.root_hub->dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
[PATCH V3] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/usb/core/hcd.c | 2 ++
drivers/usb/core/hub.c | 10 +-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..b8024ae4fdcaa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ if (hcd->rh_registered)
+ pm_wakeup_event(>self.root_hub->dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.17.0.484.g0c8726318c-goog
Re: [PATCH] USB: Increment wakeup count on remote wakeup.
Hi Alan,
Thanks for reviewing the change. Appreciate your time. I tried to
address your comments in the V2 of the patch.
On Thu, Apr 19, 2018 at 8:01 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Wed, 18 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: ravisadin...@chromium.org
>> ---
>> drivers/usb/core/hcd.c | 1 +
>> drivers/usb/core/hub.c | 12 ++--
>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..79f95a878fb6e 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>>
>> + pm_wakeup_event(dev, 0);
>
> Instead of dev, you probably want to use hcd->self.sysdev. Or maybe
> hcd->self.controller, although the difference probably doesn't matter
> for your purposes.
Trying to increment the wakeup count for the roothub. So pointed dev
to >self.root_hub->dev. Hope this is fine.
>
> On the other hand, this wakeup event may already have been counted by
> the host controller's bus subsystem. Does it matter if the same wakeup
> event gets counted twice?
>
> (This is inevitable with USB devices, in any case. If a device sends a
> wakeup request, it will be counted for that device, for all the
> intermediate hubs, and for the host controller.)
We are o.k. with the wake-up count getting incremented for the
intermediate hubs. We just want to identify the leaf hid devices, if
they are cause of the remote wake. This way, we can differentiate user
triggered wake from a automatic wakes (Ex: wakes triggered by WOLAN
packets from USB ethernet adapter).
We are also o.k. with the wake-up count getting incremented twice. All
we look for is a change in the wake-up count for the interested
devices.
>
>> @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> usb_lock_port(port_dev);
>>
>> - /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + /* Skip the initial Clear-Suspend step for a remote wakeup */
>
> What is the reason for moving the comment line down after the
> hub_port_status() call?
Sorry. This was a mistake. Changed this back in V2.
>
> Alan Stern
>
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
Re: [PATCH] USB: Increment wakeup count on remote wakeup.
Hi Alan,
Thanks for reviewing the change. Appreciate your time. I tried to
address your comments in the V2 of the patch.
On Thu, Apr 19, 2018 at 8:01 AM, Alan Stern wrote:
> On Wed, 18 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: ravisadin...@chromium.org
>> ---
>> drivers/usb/core/hcd.c | 1 +
>> drivers/usb/core/hub.c | 12 ++--
>> 2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..79f95a878fb6e 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>> {
>> unsigned long flags;
>>
>> + pm_wakeup_event(dev, 0);
>
> Instead of dev, you probably want to use hcd->self.sysdev. Or maybe
> hcd->self.controller, although the difference probably doesn't matter
> for your purposes.
Trying to increment the wakeup count for the roothub. So pointed dev
to >self.root_hub->dev. Hope this is fine.
>
> On the other hand, this wakeup event may already have been counted by
> the host controller's bus subsystem. Does it matter if the same wakeup
> event gets counted twice?
>
> (This is inevitable with USB devices, in any case. If a device sends a
> wakeup request, it will be counted for that device, for all the
> intermediate hubs, and for the host controller.)
We are o.k. with the wake-up count getting incremented for the
intermediate hubs. We just want to identify the leaf hid devices, if
they are cause of the remote wake. This way, we can differentiate user
triggered wake from a automatic wakes (Ex: wakes triggered by WOLAN
packets from USB ethernet adapter).
We are also o.k. with the wake-up count getting incremented twice. All
we look for is a change in the wake-up count for the interested
devices.
>
>> @@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev,
>> pm_message_t msg)
>>
>> usb_lock_port(port_dev);
>>
>> - /* Skip the initial Clear-Suspend step for a remote wakeup */
>> status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + /* Skip the initial Clear-Suspend step for a remote wakeup */
>
> What is the reason for moving the comment line down after the
> hub_port_status() call?
Sorry. This was a mistake. Changed this back in V2.
>
> Alan Stern
>
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>> goto SuspendCleared;
>> + }
>>
>> /* see 7.1.7.7; affects power usage, but not budgeting */
>> if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
[PATCH V2] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni <ravisadin...@chromium.org>
---
drivers/usb/core/hcd.c | 2 ++
drivers/usb/core/hub.c | 10 +-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..ee0f3ec57ce49 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ struct device *dev = >self.root_hub->dev;
+ pm_wakeup_event(dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.13.5
[PATCH V2] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: Ravi Chandra Sadineni
---
drivers/usb/core/hcd.c | 2 ++
drivers/usb/core/hub.c | 10 +-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..ee0f3ec57ce49 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ struct device *dev = >self.root_hub->dev;
+ pm_wakeup_event(dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.13.5
[PATCH] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: ravisadin...@chromium.org
---
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 12 ++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..79f95a878fb6e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ pm_wakeup_event(dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..6abc5be1bcbf5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
usb_lock_port(port_dev);
- /* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ /* Skip the initial Clear-Suspend step for a remote wakeup */
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.13.5
[PATCH] USB: Increment wakeup count on remote wakeup.
On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.
Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.
On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.
Signed-off-by: ravisadin...@chromium.org
---
drivers/usb/core/hcd.c | 1 +
drivers/usb/core/hub.c | 12 ++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..79f95a878fb6e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
{
unsigned long flags;
+ pm_wakeup_event(dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..6abc5be1bcbf5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
{
struct usb_hub *hub;
+ struct usb_port *port_dev;
if (!hdev)
return;
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+ port_dev = hub->ports[portnum - 1];
+ if (port_dev && port_dev->child)
+ pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3432,10 +3437,13 @@ int usb_port_resume(struct usb_device *udev,
pm_message_t msg)
usb_lock_port(port_dev);
- /* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
- if (status == 0 && !port_is_suspended(hub, portstatus))
+ /* Skip the initial Clear-Suspend step for a remote wakeup */
+ if (status == 0 && !port_is_suspended(hub, portstatus)) {
+ if (portchange & USB_PORT_STAT_C_SUSPEND)
+ pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+ }
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
--
2.13.5
