Re: [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
Hi Brian,
On Wed, Nov 07, 2018 at 06:49:39PM -0800, Brian Norris wrote:
> EC_MKBP_EVENT_SENSOR_FIFO events can be triggered for a variety of
> reasons, and there are very few cases in which they should be treated as
> wakeup interrupts (particularly, when a certain
> MOTIONSENSE_MODULE_FLAG_* is set, but this is not even supported in the
> mainline cros_ec_sensor driver yet). Most of the time, they are benign
> sensor readings. In any case, the top-level cros_ec device doesn't know
> enough to determine that they should wake the system, and so it should
> not report the event. This would be the job of the cros_ec_sensors
> driver to parse.
>
> This patch adds checks to cros_ec_get_next_event() such that it doesn't
> signal 'wakeup' for events of type EC_MKBP_EVENT_SENSOR_FIFO.
>
> This patch is particularly relevant on devices like Scarlet (Rockchip
> RK3399 tablet, known as Acer Chromebook Tab 10), where the EC firmware
> reports sensor events much more frequently. This was causing
> /sys/power/wakeup_count to increase very frequently, often needlessly
> interrupting our ability to suspend the system.
>
> Signed-off-by: Brian Norris
Applied. Thanks!
Benson
> ---
> v1 -> v2:
> * no change
> ---
> drivers/platform/chrome/cros_ec_proto.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> b/drivers/platform/chrome/cros_ec_proto.c
> index fff67b389c87..cc7baf0ecb3c 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -575,6 +575,7 @@ static int get_keyboard_state_event(struct cros_ec_device
> *ec_dev)
>
> int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
> {
> + u8 event_type;
> u32 host_event;
> int ret;
>
> @@ -594,11 +595,22 @@ int cros_ec_get_next_event(struct cros_ec_device
> *ec_dev, bool *wake_event)
> return ret;
>
> if (wake_event) {
> + event_type = ec_dev->event_data.event_type;
> host_event = cros_ec_get_host_event(ec_dev);
>
> - /* Consider non-host_event as wake event */
> - *wake_event = !host_event ||
> - !!(host_event & ec_dev->host_event_wake_mask);
> + /*
> + * Sensor events need to be parsed by the sensor sub-device.
> + * Defer them, and don't report the wakeup here.
> + */
> + if (event_type == EC_MKBP_EVENT_SENSOR_FIFO)
> + *wake_event = false;
> + /* Masked host-events should not count as wake events. */
> + else if (host_event &&
> + !(host_event & ec_dev->host_event_wake_mask))
> + *wake_event = false;
> + /* Consider all other events as wake events. */
> + else
> + *wake_event = true;
> }
>
> return ret;
> --
> 2.19.1.930.g4563a0d9d0-goog
>
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
Hi Brian,
On Wed, Nov 07, 2018 at 06:49:39PM -0800, Brian Norris wrote:
> EC_MKBP_EVENT_SENSOR_FIFO events can be triggered for a variety of
> reasons, and there are very few cases in which they should be treated as
> wakeup interrupts (particularly, when a certain
> MOTIONSENSE_MODULE_FLAG_* is set, but this is not even supported in the
> mainline cros_ec_sensor driver yet). Most of the time, they are benign
> sensor readings. In any case, the top-level cros_ec device doesn't know
> enough to determine that they should wake the system, and so it should
> not report the event. This would be the job of the cros_ec_sensors
> driver to parse.
>
> This patch adds checks to cros_ec_get_next_event() such that it doesn't
> signal 'wakeup' for events of type EC_MKBP_EVENT_SENSOR_FIFO.
>
> This patch is particularly relevant on devices like Scarlet (Rockchip
> RK3399 tablet, known as Acer Chromebook Tab 10), where the EC firmware
> reports sensor events much more frequently. This was causing
> /sys/power/wakeup_count to increase very frequently, often needlessly
> interrupting our ability to suspend the system.
>
> Signed-off-by: Brian Norris
Applied. Thanks!
Benson
> ---
> v1 -> v2:
> * no change
> ---
> drivers/platform/chrome/cros_ec_proto.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c
> b/drivers/platform/chrome/cros_ec_proto.c
> index fff67b389c87..cc7baf0ecb3c 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -575,6 +575,7 @@ static int get_keyboard_state_event(struct cros_ec_device
> *ec_dev)
>
> int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
> {
> + u8 event_type;
> u32 host_event;
> int ret;
>
> @@ -594,11 +595,22 @@ int cros_ec_get_next_event(struct cros_ec_device
> *ec_dev, bool *wake_event)
> return ret;
>
> if (wake_event) {
> + event_type = ec_dev->event_data.event_type;
> host_event = cros_ec_get_host_event(ec_dev);
>
> - /* Consider non-host_event as wake event */
> - *wake_event = !host_event ||
> - !!(host_event & ec_dev->host_event_wake_mask);
> + /*
> + * Sensor events need to be parsed by the sensor sub-device.
> + * Defer them, and don't report the wakeup here.
> + */
> + if (event_type == EC_MKBP_EVENT_SENSOR_FIFO)
> + *wake_event = false;
> + /* Masked host-events should not count as wake events. */
> + else if (host_event &&
> + !(host_event & ec_dev->host_event_wake_mask))
> + *wake_event = false;
> + /* Consider all other events as wake events. */
> + else
> + *wake_event = true;
> }
>
> return ret;
> --
> 2.19.1.930.g4563a0d9d0-goog
>
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
[PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
EC_MKBP_EVENT_SENSOR_FIFO events can be triggered for a variety of
reasons, and there are very few cases in which they should be treated as
wakeup interrupts (particularly, when a certain
MOTIONSENSE_MODULE_FLAG_* is set, but this is not even supported in the
mainline cros_ec_sensor driver yet). Most of the time, they are benign
sensor readings. In any case, the top-level cros_ec device doesn't know
enough to determine that they should wake the system, and so it should
not report the event. This would be the job of the cros_ec_sensors
driver to parse.
This patch adds checks to cros_ec_get_next_event() such that it doesn't
signal 'wakeup' for events of type EC_MKBP_EVENT_SENSOR_FIFO.
This patch is particularly relevant on devices like Scarlet (Rockchip
RK3399 tablet, known as Acer Chromebook Tab 10), where the EC firmware
reports sensor events much more frequently. This was causing
/sys/power/wakeup_count to increase very frequently, often needlessly
interrupting our ability to suspend the system.
Signed-off-by: Brian Norris
---
v1 -> v2:
* no change
---
drivers/platform/chrome/cros_ec_proto.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c
b/drivers/platform/chrome/cros_ec_proto.c
index fff67b389c87..cc7baf0ecb3c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -575,6 +575,7 @@ static int get_keyboard_state_event(struct cros_ec_device
*ec_dev)
int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
{
+ u8 event_type;
u32 host_event;
int ret;
@@ -594,11 +595,22 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
return ret;
if (wake_event) {
+ event_type = ec_dev->event_data.event_type;
host_event = cros_ec_get_host_event(ec_dev);
- /* Consider non-host_event as wake event */
- *wake_event = !host_event ||
- !!(host_event & ec_dev->host_event_wake_mask);
+ /*
+* Sensor events need to be parsed by the sensor sub-device.
+* Defer them, and don't report the wakeup here.
+*/
+ if (event_type == EC_MKBP_EVENT_SENSOR_FIFO)
+ *wake_event = false;
+ /* Masked host-events should not count as wake events. */
+ else if (host_event &&
+!(host_event & ec_dev->host_event_wake_mask))
+ *wake_event = false;
+ /* Consider all other events as wake events. */
+ else
+ *wake_event = true;
}
return ret;
--
2.19.1.930.g4563a0d9d0-goog
[PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
EC_MKBP_EVENT_SENSOR_FIFO events can be triggered for a variety of
reasons, and there are very few cases in which they should be treated as
wakeup interrupts (particularly, when a certain
MOTIONSENSE_MODULE_FLAG_* is set, but this is not even supported in the
mainline cros_ec_sensor driver yet). Most of the time, they are benign
sensor readings. In any case, the top-level cros_ec device doesn't know
enough to determine that they should wake the system, and so it should
not report the event. This would be the job of the cros_ec_sensors
driver to parse.
This patch adds checks to cros_ec_get_next_event() such that it doesn't
signal 'wakeup' for events of type EC_MKBP_EVENT_SENSOR_FIFO.
This patch is particularly relevant on devices like Scarlet (Rockchip
RK3399 tablet, known as Acer Chromebook Tab 10), where the EC firmware
reports sensor events much more frequently. This was causing
/sys/power/wakeup_count to increase very frequently, often needlessly
interrupting our ability to suspend the system.
Signed-off-by: Brian Norris
---
v1 -> v2:
* no change
---
drivers/platform/chrome/cros_ec_proto.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c
b/drivers/platform/chrome/cros_ec_proto.c
index fff67b389c87..cc7baf0ecb3c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -575,6 +575,7 @@ static int get_keyboard_state_event(struct cros_ec_device
*ec_dev)
int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
{
+ u8 event_type;
u32 host_event;
int ret;
@@ -594,11 +595,22 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
bool *wake_event)
return ret;
if (wake_event) {
+ event_type = ec_dev->event_data.event_type;
host_event = cros_ec_get_host_event(ec_dev);
- /* Consider non-host_event as wake event */
- *wake_event = !host_event ||
- !!(host_event & ec_dev->host_event_wake_mask);
+ /*
+* Sensor events need to be parsed by the sensor sub-device.
+* Defer them, and don't report the wakeup here.
+*/
+ if (event_type == EC_MKBP_EVENT_SENSOR_FIFO)
+ *wake_event = false;
+ /* Masked host-events should not count as wake events. */
+ else if (host_event &&
+!(host_event & ec_dev->host_event_wake_mask))
+ *wake_event = false;
+ /* Consider all other events as wake events. */
+ else
+ *wake_event = true;
}
return ret;
--
2.19.1.930.g4563a0d9d0-goog
