linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
@ 2018-11-08  2:49 Brian Norris
  2018-11-08  2:49 ` [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup Brian Norris
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Brian Norris @ 2018-11-08  2:49 UTC (permalink / raw)
  To: Benson Leung
  Cc: Lee Jones, linux-kernel, Olof Johansson, Shawn Nematbakhsh,
	Alexandru Stan, Gwendal Grignou, Enrico Granata,
	RaviChandra Sadineni, Brian Norris

cros_ec_get_next_event() is documented to return 0 for success and
negative for errors. It currently returns negative for some errors, and
non-negative (number of bytes received) for success (including some "no
data available" responses as zero). This mostly works out OK, because the
callers were more or less ignoring the documentation, and only treating
positive values as success (and indepdently checking the modification of
'wakeup').

Let's button this up by avoiding pretending to handle event/wakeup
distinctions when no event info was retrieved (i.e., returned 0 bytes).
And fix the documentation of cros_ec_get_host_event() and
cros_ec_get_next_event() to accurately describe their behavior.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v1 -> v2:
 * don't make as many changes to the API -- just fix the documentation
   and a few corner cases instead
---
 drivers/platform/chrome/cros_ec_proto.c | 4 ++--
 include/linux/mfd/cros_ec.h             | 6 ++++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f..fff67b389c87 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
 
 	if (!ec_dev->mkbp_event_supported) {
 		ret = get_keyboard_state_event(ec_dev);
-		if (ret < 0)
+		if (ret <= 0)
 			return ret;
 
 		if (wake_event)
@@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
 	}
 
 	ret = get_next_event(ec_dev);
-	if (ret < 0)
+	if (ret <= 0)
 		return ret;
 
 	if (wake_event) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7..de8b588c8776 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
  * @wake_event: Pointer to a bool set to true upon return if the event might be
  *              treated as a wake event. Ignored if null.
  *
- * Return: 0 on success or negative error code.
+ * Return: negative error code on errors; 0 for no data; or else number of
+ * bytes received (i.e., an event was retrieved successfully). Event types are
+ * written out to @ec_dev->event_data.event_type on success.
  */
 int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
 
@@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
  * events raised and call the functions in the ec notifier. This function
  * is a helper to know which events are raised.
  *
- * Return: 0 on success or negative error code.
+ * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
  */
 u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 
-- 
2.19.1.930.g4563a0d9d0-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
  2018-11-08  2:49 [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Brian Norris
@ 2018-11-08  2:49 ` Brian Norris
  2018-11-14  5:34   ` Benson Leung
  2018-11-13  7:57 ` [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Lee Jones
  2018-11-14  5:33 ` Benson Leung
  2 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2018-11-08  2:49 UTC (permalink / raw)
  To: Benson Leung
  Cc: Lee Jones, linux-kernel, Olof Johansson, Shawn Nematbakhsh,
	Alexandru Stan, Gwendal Grignou, Enrico Granata,
	RaviChandra Sadineni, Brian Norris

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 <briannorris@chromium.org>
---
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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
  2018-11-08  2:49 [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Brian Norris
  2018-11-08  2:49 ` [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup Brian Norris
@ 2018-11-13  7:57 ` Lee Jones
  2018-11-14  5:33 ` Benson Leung
  2 siblings, 0 replies; 5+ messages in thread
From: Lee Jones @ 2018-11-13  7:57 UTC (permalink / raw)
  To: Brian Norris
  Cc: Benson Leung, linux-kernel, Olof Johansson, Shawn Nematbakhsh,
	Alexandru Stan, Gwendal Grignou, Enrico Granata,
	RaviChandra Sadineni

On Wed, 07 Nov 2018, Brian Norris wrote:

> cros_ec_get_next_event() is documented to return 0 for success and
> negative for errors. It currently returns negative for some errors, and
> non-negative (number of bytes received) for success (including some "no
> data available" responses as zero). This mostly works out OK, because the
> callers were more or less ignoring the documentation, and only treating
> positive values as success (and indepdently checking the modification of
> 'wakeup').
> 
> Let's button this up by avoiding pretending to handle event/wakeup
> distinctions when no event info was retrieved (i.e., returned 0 bytes).
> And fix the documentation of cros_ec_get_host_event() and
> cros_ec_get_next_event() to accurately describe their behavior.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> v1 -> v2:
>  * don't make as many changes to the API -- just fix the documentation
>    and a few corner cases instead
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 4 ++--
>  include/linux/mfd/cros_ec.h             | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes
  2018-11-08  2:49 [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Brian Norris
  2018-11-08  2:49 ` [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup Brian Norris
  2018-11-13  7:57 ` [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Lee Jones
@ 2018-11-14  5:33 ` Benson Leung
  2 siblings, 0 replies; 5+ messages in thread
From: Benson Leung @ 2018-11-14  5:33 UTC (permalink / raw)
  To: Brian Norris
  Cc: Benson Leung, Lee Jones, linux-kernel, Olof Johansson,
	Shawn Nematbakhsh, Alexandru Stan, Gwendal Grignou,
	Enrico Granata, RaviChandra Sadineni

[-- Attachment #1: Type: text/plain, Size: 3375 bytes --]

Hi Brian,

On Wed, Nov 07, 2018 at 06:49:38PM -0800, Brian Norris wrote:
> cros_ec_get_next_event() is documented to return 0 for success and
> negative for errors. It currently returns negative for some errors, and
> non-negative (number of bytes received) for success (including some "no
> data available" responses as zero). This mostly works out OK, because the
> callers were more or less ignoring the documentation, and only treating
> positive values as success (and indepdently checking the modification of
> 'wakeup').
> 
> Let's button this up by avoiding pretending to handle event/wakeup
> distinctions when no event info was retrieved (i.e., returned 0 bytes).
> And fix the documentation of cros_ec_get_host_event() and
> cros_ec_get_next_event() to accurately describe their behavior.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Applied.

Thanks,
Benson

> ---
> v1 -> v2:
>  * don't make as many changes to the API -- just fix the documentation
>    and a few corner cases instead
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 4 ++--
>  include/linux/mfd/cros_ec.h             | 6 ++++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f..fff67b389c87 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -580,7 +580,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
>  
>  	if (!ec_dev->mkbp_event_supported) {
>  		ret = get_keyboard_state_event(ec_dev);
> -		if (ret < 0)
> +		if (ret <= 0)
>  			return ret;
>  
>  		if (wake_event)
> @@ -590,7 +590,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event)
>  	}
>  
>  	ret = get_next_event(ec_dev);
> -	if (ret < 0)
> +	if (ret <= 0)
>  		return ret;
>  
>  	if (wake_event) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7..de8b588c8776 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -317,7 +317,9 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
>   * @wake_event: Pointer to a bool set to true upon return if the event might be
>   *              treated as a wake event. Ignored if null.
>   *
> - * Return: 0 on success or negative error code.
> + * Return: negative error code on errors; 0 for no data; or else number of
> + * bytes received (i.e., an event was retrieved successfully). Event types are
> + * written out to @ec_dev->event_data.event_type on success.
>   */
>  int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>  
> @@ -329,7 +331,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev, bool *wake_event);
>   * events raised and call the functions in the ec notifier. This function
>   * is a helper to know which events are raised.
>   *
> - * Return: 0 on success or negative error code.
> + * Return: 0 on error or non-zero bitmask of one or more EC_HOST_EVENT_*.
>   */
>  u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  
> -- 
> 2.19.1.930.g4563a0d9d0-goog
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup
  2018-11-08  2:49 ` [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup Brian Norris
@ 2018-11-14  5:34   ` Benson Leung
  0 siblings, 0 replies; 5+ messages in thread
From: Benson Leung @ 2018-11-14  5:34 UTC (permalink / raw)
  To: Brian Norris
  Cc: Benson Leung, Lee Jones, linux-kernel, Olof Johansson,
	Shawn Nematbakhsh, Alexandru Stan, Gwendal Grignou,
	Enrico Granata, RaviChandra Sadineni

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

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 <briannorris@chromium.org>

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.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-11-14  5:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-08  2:49 [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Brian Norris
2018-11-08  2:49 ` [PATCH v2 2/2] platform/chrome: don't report EC_MKBP_EVENT_SENSOR_FIFO as wakeup Brian Norris
2018-11-14  5:34   ` Benson Leung
2018-11-13  7:57 ` [PATCH v2 1/2] platform/chrome: straighten out cros_ec_get_{next,host}_event() error codes Lee Jones
2018-11-14  5:33 ` Benson Leung

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).