Re: [PATCH 2/2] platform/chrome: cros_ec_proto: check for missing EC_CMD_HOST_EVENT_GET_WAKE_MASK

[Enric Balletbo i Serra]

Hi Brian,

Thank you for your patch, I'll take a look soon but I'd like to ask if you can
join the discussion with this patchset [1], specially this one [2]. We're trying
to match EC errors with standard linux kernel errors because we think can be
helpful.

[1] https://lore.kernel.org/patchwork/cover/1276734/
[2] https://lore.kernel.org/patchwork/patch/1276738/

Thanks,
 Enric

On 22/7/20 3:57, 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@xxxxxxxxxxxx>
> ---
>  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..01a74abe4191 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 -ENOTSUPP;
> +		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 != -ENOTSUPP)
> +			dev_err(ec_dev->dev,
> +				"failed to retrieve wake mask: %d\n", ret);
>  	}
>  
>  	ret = 0;
>