[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6c200c61-209f-eb76-88b6-16491e2bc69c@collabora.com>
Date: Mon, 17 Aug 2020 13:31:06 +0200
From: Enric Balletbo i Serra <enric.balletbo@...labora.com>
To: Brian Norris <briannorris@...omium.org>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] platform/chrome: cros_ec_proto: check for missing
EC_CMD_HOST_EVENT_GET_WAKE_MASK
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 <briannorris@...omium.org>
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;
>
Powered by blists - more mailing lists