Message-ID: <CA+ASDXNJdbUNw3FX8HL7V2OFwkE8eqLnhHOt+CWELP2a_RzY8Q@mail.gmail.com>
Date:   Wed, 7 Nov 2018 18:26:45 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     bleung@...omium.org
Cc:     Lee Jones <lee.jones@...aro.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>, olof@...om.net,
        Shawn N <shawnn@...omium.org>,
        Alexandru M Stan <amstan@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>, egranata@...omium.org,
        ravisadineni@...omium.org
Subject: Re: [PATCH 1/2] platform/chrome: fixup cros_ec_get_next_event() error codes

On Wed, Nov 7, 2018 at 5:30 PM Brian Norris <briannorris@...omium.org> 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 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').

Hmm, I realized there's a problem with this: I'd need to change
get_keyboard_state_event() too, if I want to do this right.

I think maybe it'll be better to leave this alone, and just fix up the
documentation and a few corner cases.

> This is difficult to reason about though, and it makes it harder to
> refactor cros_ec_get_next_event() without breaking its users. So
> instead, I
> (a) make cros_ec_get_next_event() match its documentation and
> (b) change the users to treat non-negative as success.
>
> This change tries to match a roughly similar pattern to
> cros_ec_cmd_xfer_status(), where most non-zero result codes are treated
> as an error -- in this case, all protocol errors (including
> EC_RES_UNAVAILABLE) are treated as "no data available."
>
> Signed-off-by: Brian Norris <briannorris@...omium.org>
> ---
> This barely touches MFD code, but overall this series should probably go
> through the platform/chrome maintenance, IMO.
> ---
>  drivers/mfd/cros_ec.c                   |  4 ++--

I may end up dropping the MFD changes from the next version, so I
might drop Lee from the list too.

Everyone else...stay tuned for v2.

Thanks,
Brian

>  drivers/platform/chrome/cros_ec_lpc.c   |  2 +-
>  drivers/platform/chrome/cros_ec_proto.c | 13 ++++++++++---
>  3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fe6f83766144..807a5870198d 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -67,7 +67,7 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
>         if (wake_event && device_may_wakeup(ec_dev->dev))
>                 pm_wakeup_event(ec_dev->dev, 0);
>
> -       if (ret > 0)
> +       if (ret >= 0)
>                 blocking_notifier_call_chain(&ec_dev->event_notifier,
>                                              0, ec_dev);
>         return IRQ_HANDLED;
> @@ -219,7 +219,7 @@ EXPORT_SYMBOL(cros_ec_suspend);
>  static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
>  {
>         while (ec_dev->mkbp_event_supported &&
> -              cros_ec_get_next_event(ec_dev, NULL) > 0)
> +              cros_ec_get_next_event(ec_dev, NULL) >= 0)



>                 blocking_notifier_call_chain(&ec_dev->event_notifier,
>                                              1, ec_dev);
>  }
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index e1b75775cd4a..292408eefe43 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -235,7 +235,7 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
>         struct cros_ec_device *ec_dev = data;
>
>         if (ec_dev->mkbp_event_supported &&
> -           cros_ec_get_next_event(ec_dev, NULL) > 0)
> +           cros_ec_get_next_event(ec_dev, NULL) >= 0)
>                 blocking_notifier_call_chain(&ec_dev->event_notifier, 0,
>                                              ec_dev);
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f..49625836fdd6 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -541,8 +541,12 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>         if (cmd_version == 1) {
>                 ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>                                 sizeof(struct ec_response_get_next_event_v1));
> -               if (ret < 0 || msg->result != EC_RES_INVALID_VERSION)
> +               if (ret < 0)
>                         return ret;
> +               if (msg->result == EC_RES_SUCCESS)
> +                       return 0;
> +               if (msg->result != EC_RES_INVALID_VERSION)
> +                       return -ENODATA;
>
>                 /* Fallback to version 0 for future send attempts */
>                 cmd_version = 0;
> @@ -550,8 +554,11 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>
>         ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>                                   sizeof(struct ec_response_get_next_event));
> -
> -       return ret;
> +       if (ret < 0)
> +               return ret;
> +       if (msg->result != EC_RES_SUCCESS)
> +               return -ENODATA;
> +       return 0;
>  }
>
>  static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> --
> 2.19.1.930.g4563a0d9d0-goog
>

Powered by blists - more mailing lists