lkml.org 
[lkml]   [2018]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@chromium.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@chromium.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
>

\
 
 \ /
  Last update: 2018-11-08 03:27    [W:0.060 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site