linux-kernel - [PATCH 1/2] platform/chrome: fixup cros_ec_get_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20181108013005.37362-1-briannorris@chromium.org>
Date:   Wed,  7 Nov 2018 17:30:04 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Benson Leung <bleung@...omium.org>
Cc:     Lee Jones <lee.jones@...aro.org>, <linux-kernel@...r.kernel.org>,
        Olof Johansson <olof@...om.net>,
        Shawn Nematbakhsh <shawnn@...omium.org>,
        Alexandru Stan <amstan@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Enrico Granata <egranata@...omium.org>,
        RaviChandra Sadineni <ravisadineni@...omium.org>,
        Brian Norris <briannorris@...omium.org>
Subject: [PATCH 1/2] platform/chrome: fixup cros_ec_get_next_event() error codes

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').

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 ++--
 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