ECs that don't implement EC_CMD_HOST_EVENT_GET_WAKE_MASK should still
have some reasonable default mask -- otherwise, they'll treat a variety
of EC signals as spurious wakeups. Battery and AC events can be
especially common, for devices that have been sitting at full charge
plugged into AC for a long time, as they may cycle their charging off
and on, or their battery may start reporting failures as it ages.
Treating these as wakeups does not serve a useful purpose, and is
instead often counterproductive. And indeed, later ECs (that implement
the mask) don't include these events in their wake-mask.
Note that this patch doesn't do anything without the subsequent patch
("platform/chrome: cros_ec_proto: check for missing
EC_CMD_HOST_EVENT_GET_WAKE_MASK"), because
cros_ec_get_host_event_wake_mask() currently does not return an error if
EC_CMD_HOST_EVENT_GET_WAKE_MASK is not implemented.
Some additional notes:
While the EC typically knows not to wake the CPU for these unimportant
events once the CPU reaches a sleep state, it doesn't really have a way
to know that the CPU is "almost" asleep, unless it has support for
EC_CMD_HOST_SLEEP_EVENT. Alas, these older ECs do not support that
command either, so this solution is not 100% complete.
Signed-off-by: Brian Norris <[email protected]>
---
v2:
* more notes in commit message
---
drivers/platform/chrome/cros_ec_proto.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092..e93024b55ce8 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -469,14 +469,26 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
&ver_mask);
ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));
- /*
- * Get host event wake mask, assume all events are wake events
- * if unavailable.
- */
+ /* Get host event wake mask. */
ret = cros_ec_get_host_event_wake_mask(ec_dev, proto_msg,
&ec_dev->host_event_wake_mask);
- if (ret < 0)
- ec_dev->host_event_wake_mask = U32_MAX;
+ if (ret < 0) {
+ /*
+ * If the EC doesn't support EC_CMD_HOST_EVENT_GET_WAKE_MASK,
+ * use a reasonable default. Note that we ignore various
+ * battery, AC status, and power-state events, because (a)
+ * those can be quite common (e.g., when sitting at full
+ * charge, on AC) and (b) these are not actionable wake events;
+ * if anything, we'd like to continue suspending (to save
+ * power), not wake up.
+ */
+ ec_dev->host_event_wake_mask = U32_MAX &
+ ~(BIT(EC_HOST_EVENT_AC_DISCONNECTED) |
+ BIT(EC_HOST_EVENT_BATTERY_LOW) |
+ BIT(EC_HOST_EVENT_BATTERY_CRITICAL) |
+ BIT(EC_HOST_EVENT_PD_MCU) |
+ BIT(EC_HOST_EVENT_BATTERY_STATUS));
+ }
ret = 0;
--
2.28.0.rc0.142.g3c755180ce-goog
As with cros_ec_cmd_xfer_status(), etc., it's not enough to simply check
for the return status of send_command() -- that only covers transport or
other similarly-fatal errors. One must also check the ->result field, to
see whether the command really succeeded. If not, we can't use the data
it returns.
The caller of cros_ec_get_host_event_wake_mask() ignores this, and so
for example, on EC's where the command is not implemented, we're using
junk (or in practice, all zeros) for our wake-mask. We should be using a
non-zero default (currently, it's supposed to be all-1's).
Fix this by checking the ->result field and returning -EPROTO for
errors.
I might label this as fixing commit 29d99b966d60 ("cros_ec: Don't signal
wake event for non-wake host events"), except that this fix alone
actually may make things worse, as it now allows for a lot more spurious
wakeups. The patch "platform/chrome: cros_ec_proto: ignore battery/AC
wakeups on old ECs" helps to mitigate this.
Signed-off-by: Brian Norris <[email protected]>
---
v2:
* EOPNOTSUPP, not ENOTSUPP
---
drivers/platform/chrome/cros_ec_proto.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e93024b55ce8..31ca0af62388 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -208,6 +208,12 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev,
msg->insize = sizeof(*r);
ret = send_command(ec_dev, msg);
+ if (ret >= 0) {
+ if (msg->result == EC_RES_INVALID_COMMAND)
+ return -EOPNOTSUPP;
+ if (msg->result != EC_RES_SUCCESS)
+ return -EPROTO;
+ }
if (ret > 0) {
r = (struct ec_response_host_event_mask *)msg->data;
*mask = r->mask;
@@ -488,6 +494,13 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
BIT(EC_HOST_EVENT_BATTERY_CRITICAL) |
BIT(EC_HOST_EVENT_PD_MCU) |
BIT(EC_HOST_EVENT_BATTERY_STATUS));
+ /*
+ * Old ECs may not support this command. Complain about all
+ * other errors.
+ */
+ if (ret != -EOPNOTSUPP)
+ dev_err(ec_dev->dev,
+ "failed to retrieve wake mask: %d\n", ret);
}
ret = 0;
--
2.28.0.rc0.142.g3c755180ce-goog
Hi Brian,
On 24/7/20 21:08, Brian Norris wrote:
> As with cros_ec_cmd_xfer_status(), etc., it's not enough to simply check
> for the return status of send_command() -- that only covers transport or
> other similarly-fatal errors. One must also check the ->result field, to
> see whether the command really succeeded. If not, we can't use the data
> it returns.
>
> The caller of cros_ec_get_host_event_wake_mask() ignores this, and so
> for example, on EC's where the command is not implemented, we're using
> junk (or in practice, all zeros) for our wake-mask. We should be using a
> non-zero default (currently, it's supposed to be all-1's).
>
> Fix this by checking the ->result field and returning -EPROTO for
> errors.
>
> I might label this as fixing commit 29d99b966d60 ("cros_ec: Don't signal
> wake event for non-wake host events"), except that this fix alone
> actually may make things worse, as it now allows for a lot more spurious
> wakeups. The patch "platform/chrome: cros_ec_proto: ignore battery/AC
> wakeups on old ECs" helps to mitigate this.
>
> Signed-off-by: Brian Norris <[email protected]>
Sorry for late notice. The patch has been applied for 5.9.
> ---
> v2:
> * EOPNOTSUPP, not ENOTSUPP
> ---
> drivers/platform/chrome/cros_ec_proto.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e93024b55ce8..31ca0af62388 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -208,6 +208,12 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev,
> msg->insize = sizeof(*r);
>
> ret = send_command(ec_dev, msg);
> + if (ret >= 0) {
> + if (msg->result == EC_RES_INVALID_COMMAND)
> + return -EOPNOTSUPP;
> + if (msg->result != EC_RES_SUCCESS)
> + return -EPROTO;
> + }
> if (ret > 0) {
> r = (struct ec_response_host_event_mask *)msg->data;
> *mask = r->mask;
> @@ -488,6 +494,13 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> BIT(EC_HOST_EVENT_BATTERY_CRITICAL) |
> BIT(EC_HOST_EVENT_PD_MCU) |
> BIT(EC_HOST_EVENT_BATTERY_STATUS));
> + /*
> + * Old ECs may not support this command. Complain about all
> + * other errors.
> + */
> + if (ret != -EOPNOTSUPP)
> + dev_err(ec_dev->dev,
> + "failed to retrieve wake mask: %d\n", ret);
> }
>
> ret = 0;
>
Hi Brian,
On 24/7/20 21:08, Brian Norris wrote:
> ECs that don't implement EC_CMD_HOST_EVENT_GET_WAKE_MASK should still
> have some reasonable default mask -- otherwise, they'll treat a variety
> of EC signals as spurious wakeups. Battery and AC events can be
> especially common, for devices that have been sitting at full charge
> plugged into AC for a long time, as they may cycle their charging off
> and on, or their battery may start reporting failures as it ages.
>
> Treating these as wakeups does not serve a useful purpose, and is
> instead often counterproductive. And indeed, later ECs (that implement
> the mask) don't include these events in their wake-mask.
>
> Note that this patch doesn't do anything without the subsequent patch
> ("platform/chrome: cros_ec_proto: check for missing
> EC_CMD_HOST_EVENT_GET_WAKE_MASK"), because
> cros_ec_get_host_event_wake_mask() currently does not return an error if
> EC_CMD_HOST_EVENT_GET_WAKE_MASK is not implemented.
>
> Some additional notes:
> While the EC typically knows not to wake the CPU for these unimportant
> events once the CPU reaches a sleep state, it doesn't really have a way
> to know that the CPU is "almost" asleep, unless it has support for
> EC_CMD_HOST_SLEEP_EVENT. Alas, these older ECs do not support that
> command either, so this solution is not 100% complete.
>
> Signed-off-by: Brian Norris <[email protected]>
Sorry for late notice. The patch has been applied for 5.9.
> ---
> v2:
> * more notes in commit message
> ---
> drivers/platform/chrome/cros_ec_proto.c | 24 ++++++++++++++++++------
> 1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 3e745e0fe092..e93024b55ce8 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -469,14 +469,26 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> &ver_mask);
> ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));
>
> - /*
> - * Get host event wake mask, assume all events are wake events
> - * if unavailable.
> - */
> + /* Get host event wake mask. */
> ret = cros_ec_get_host_event_wake_mask(ec_dev, proto_msg,
> &ec_dev->host_event_wake_mask);
> - if (ret < 0)
> - ec_dev->host_event_wake_mask = U32_MAX;
> + if (ret < 0) {
> + /*
> + * If the EC doesn't support EC_CMD_HOST_EVENT_GET_WAKE_MASK,
> + * use a reasonable default. Note that we ignore various
> + * battery, AC status, and power-state events, because (a)
> + * those can be quite common (e.g., when sitting at full
> + * charge, on AC) and (b) these are not actionable wake events;
> + * if anything, we'd like to continue suspending (to save
> + * power), not wake up.
> + */
> + ec_dev->host_event_wake_mask = U32_MAX &
> + ~(BIT(EC_HOST_EVENT_AC_DISCONNECTED) |
> + BIT(EC_HOST_EVENT_BATTERY_LOW) |
> + BIT(EC_HOST_EVENT_BATTERY_CRITICAL) |
> + BIT(EC_HOST_EVENT_PD_MCU) |
> + BIT(EC_HOST_EVENT_BATTERY_STATUS));
> + }
>
> ret = 0;
>
>