From: Enrico Granata <[email protected]>
The ChromeOS EC has support for signaling to the host that
a single IRQ can serve multiple MKBP events.
Doing this serves an optimization purpose, as it minimizes the
number of round-trips into the interrupt handling machinery, and
it proves beneficial to sensor timestamping as it keeps the desired
synchronization of event times between the two processors.
This patch adds kernel support for this EC feature, allowing the
ec_irq to loop until all events have been served.
Signed-off-by: Enrico Granata <[email protected]>
---
drivers/mfd/cros_ec.c | 20 +++++++++++++--
drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
include/linux/mfd/cros_ec.h | 3 ++-
include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
4 files changed, 50 insertions(+), 21 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fe6f83766144f..17903a378aa1a 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
.pdata_size = sizeof(pd_p),
};
-static irqreturn_t ec_irq_thread(int irq, void *data)
+static bool ec_handle_event(struct cros_ec_device *ec_dev)
{
- struct cros_ec_device *ec_dev = data;
bool wake_event = true;
int ret;
+ bool ec_has_more_events = false;
ret = cros_ec_get_next_event(ec_dev, &wake_event);
+ if (ret > 0)
+ ec_has_more_events =
+ ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
/*
* Signal only if wake host events or any interrupt if
@@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
0, ec_dev);
+
+ return ec_has_more_events;
+}
+
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+ struct cros_ec_device *ec_dev = data;
+ bool ec_has_more_events;
+
+ do {
+ ec_has_more_events = ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
+
return IRQ_HANDLED;
}
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f3..bb126d95b2fd4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ret = cros_ec_get_host_command_version_mask(ec_dev,
EC_CMD_GET_NEXT_EVENT,
&ver_mask);
- if (ret < 0 || ver_mask == 0)
+ if (ret < 0 || ver_mask == 0) {
ec_dev->mkbp_event_supported = 0;
- else
- ec_dev->mkbp_event_supported = 1;
+ dev_info(ec_dev->dev, "MKBP not supported\n");
+ } else {
+ ec_dev->mkbp_event_supported = fls(ver_mask);
+ dev_info(ec_dev->dev, "MKBP support version %u\n",
+ ec_dev->mkbp_event_supported - 1);
+ }
/*
* Get host event wake mask, assume all events are wake events
@@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
{
u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
- static int cmd_version = 1;
- int ret;
+ const int cmd_version = ec_dev->mkbp_event_supported - 1;
if (ec_dev->suspended) {
dev_dbg(ec_dev->dev, "Device suspended.\n");
return -EHOSTDOWN;
}
- 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)
- return ret;
-
- /* Fallback to version 0 for future send attempts */
- cmd_version = 0;
- }
-
- ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+ if (cmd_version == 0)
+ return get_next_event_xfer(ec_dev, msg, 0,
sizeof(struct ec_response_get_next_event));
- return ret;
+ return get_next_event_xfer(ec_dev, msg, cmd_version,
+ sizeof(struct ec_response_get_next_event_v1));
}
static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
@@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
{
+ u32 event_type =
+ ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
u32 host_event;
BUG_ON(!ec_dev->mkbp_event_supported);
- if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
+ if (event_type != EC_MKBP_EVENT_HOST_EVENT)
return 0;
if (ec_dev->event_size != sizeof(host_event)) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7d..eb771ceeaeed1 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -152,7 +152,8 @@ struct cros_ec_device {
int (*pkt_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
struct mutex lock;
- bool mkbp_event_supported;
+ /* 0 == not supported, otherwise it supports version x - 1 */
+ u8 mkbp_event_supported;
struct blocking_notifier_head event_notifier;
struct ec_response_get_next_event_v1 event_data;
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 9a9631f0559e2..3f013593d0107 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2146,6 +2146,14 @@ struct ec_result_keyscan_seq_ctrl {
*/
#define EC_CMD_GET_NEXT_EVENT 0x67
+#define EC_MKBP_HAS_MORE_EVENTS_SHIFT 7
+
+/* EC can provide more MKBP events to host */
+#define EC_MKBP_HAS_MORE_EVENTS (1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT)
+
+/* The mask to apply to get the raw event type */
+#define EC_MKBP_EVENT_TYPE_MASK ((1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT) - 1)
+
enum ec_mkbp_event {
/* Keyboard matrix changed. The event data is the new matrix state. */
EC_MKBP_EVENT_KEY_MATRIX = 0,
@@ -2173,6 +2181,13 @@ enum ec_mkbp_event {
/* Number of MKBP events */
EC_MKBP_EVENT_COUNT,
+
+ /*
+ * Maximum possible event type
+ * The most significant bit of event type is used to indicate that
+ * the EC has multiple events for the AP to serve
+ */
+ EC_MKBP_EVENT_MAX_TYPE = EC_MKBP_EVENT_TYPE_MASK,
};
union ec_response_get_next_data {
--
2.20.0.rc0.387.gc7a69e6b6c-goog
On Thu, 29 Nov 2018, [email protected] wrote:
> From: Enrico Granata <[email protected]>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP events.
>
> Doing this serves an optimization purpose, as it minimizes the
> number of round-trips into the interrupt handling machinery, and
> it proves beneficial to sensor timestamping as it keeps the desired
> synchronization of event times between the two processors.
>
> This patch adds kernel support for this EC feature, allowing the
> ec_irq to loop until all events have been served.
>
> Signed-off-by: Enrico Granata <[email protected]>
> ---
> drivers/mfd/cros_ec.c | 20 +++++++++++++--
Looks fine, in principle:
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Enrico,
On Thu, Nov 29, 2018 at 11:55:48AM -0800, [email protected] wrote:
> From: Enrico Granata <[email protected]>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP events.
>
> Doing this serves an optimization purpose, as it minimizes the
> number of round-trips into the interrupt handling machinery, and
> it proves beneficial to sensor timestamping as it keeps the desired
> synchronization of event times between the two processors.
>
> This patch adds kernel support for this EC feature, allowing the
> ec_irq to loop until all events have been served.
Might be worth being more explicit in this commit message to say that
you're adding support for EC_CMD_GET_NEXT_EVENT version 2?
> Signed-off-by: Enrico Granata <[email protected]>
Mostly looks fine; thanks for sending. I have a few small notes (and
some of that is not necessarily something resolve in this patch), but
with at least the cros_ec.h comment fixup, feel free to add my:
Reviewed-by: Brian Norris <[email protected]>
> ---
> drivers/mfd/cros_ec.c | 20 +++++++++++++--
> drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
> include/linux/mfd/cros_ec.h | 3 ++-
> include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
> 4 files changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fe6f83766144f..17903a378aa1a 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
> .pdata_size = sizeof(pd_p),
> };
>
> -static irqreturn_t ec_irq_thread(int irq, void *data)
> +static bool ec_handle_event(struct cros_ec_device *ec_dev)
> {
> - struct cros_ec_device *ec_dev = data;
> bool wake_event = true;
> int ret;
> + bool ec_has_more_events = false;
>
> ret = cros_ec_get_next_event(ec_dev, &wake_event);
> + if (ret > 0)
> + ec_has_more_events =
> + ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
>
> /*
> * Signal only if wake host events or any interrupt if
> @@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 0, ec_dev);
> +
> + return ec_has_more_events;
> +}
> +
> +static irqreturn_t ec_irq_thread(int irq, void *data)
> +{
> + struct cros_ec_device *ec_dev = data;
> + bool ec_has_more_events;
> +
> + do {
> + ec_has_more_events = ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> +
> return IRQ_HANDLED;
> }
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f3..bb126d95b2fd4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> EC_CMD_GET_NEXT_EVENT,
> &ver_mask);
It's not exactly new here (although you're using 'ver_mask' in new
ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
right. It doesn't look at msg->result, and instead just assumes that if
we got some data back (send_command() > 0), then it must have been a
success. I don't think that's really guaranteed in general, although it
might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
IOW, to be definitely sure we're not looking at a garbage result in
'ver_mask', we should probably fixup
cros_ec_get_host_command_version_mask().
> - if (ret < 0 || ver_mask == 0)
> + if (ret < 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> - else
> - ec_dev->mkbp_event_supported = 1;
> + dev_info(ec_dev->dev, "MKBP not supported\n");
> + } else {
> + ec_dev->mkbp_event_supported = fls(ver_mask);
> + dev_info(ec_dev->dev, "MKBP support version %u\n",
> + ec_dev->mkbp_event_supported - 1);
> + }
>
> /*
> * Get host event wake mask, assume all events are wake events
> @@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> {
> u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
> struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
> - static int cmd_version = 1;
> - int ret;
> + const int cmd_version = ec_dev->mkbp_event_supported - 1;
>
> if (ec_dev->suspended) {
> dev_dbg(ec_dev->dev, "Device suspended.\n");
> return -EHOSTDOWN;
> }
>
> - 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)
> - return ret;
> -
> - /* Fallback to version 0 for future send attempts */
> - cmd_version = 0;
> - }
> -
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + if (cmd_version == 0)
> + return get_next_event_xfer(ec_dev, msg, 0,
> sizeof(struct ec_response_get_next_event));
>
> - return ret;
> + return get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event_v1));
> }
>
> static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> @@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
>
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
> {
> + u32 event_type =
> + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> u32 host_event;
>
> BUG_ON(!ec_dev->mkbp_event_supported);
>
> - if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
> + if (event_type != EC_MKBP_EVENT_HOST_EVENT)
> return 0;
>
> if (ec_dev->event_size != sizeof(host_event)) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7d..eb771ceeaeed1 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -152,7 +152,8 @@ struct cros_ec_device {
> int (*pkt_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> struct mutex lock;
> - bool mkbp_event_supported;
> + /* 0 == not supported, otherwise it supports version x - 1 */
This comment belongs in the kerneldoc, which is above the struct
definition. You're invalidating the existing comment:
* @mkbp_event_supported: True if this EC supports the MKBP event protocol.
Brian
> + u8 mkbp_event_supported;
> struct blocking_notifier_head event_notifier;
>
> struct ec_response_get_next_event_v1 event_data;
...
From: Enrico Granata <[email protected]>
The ChromeOS EC has support for signaling to the host that
a single IRQ can serve multiple MKBP events.
Doing this serves an optimization purpose, as it minimizes the
number of round-trips into the interrupt handling machinery, and
it proves beneficial to sensor timestamping as it keeps the desired
synchronization of event times between the two processors.
This patch adds kernel support for this EC feature, allowing the
ec_irq to loop until all events have been served.
Signed-off-by: Enrico Granata <[email protected]>
---
drivers/mfd/cros_ec.c | 20 +++++++++++++--
drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
include/linux/mfd/cros_ec.h | 7 ++++--
include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
4 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fe6f83766144f..17903a378aa1a 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
.pdata_size = sizeof(pd_p),
};
-static irqreturn_t ec_irq_thread(int irq, void *data)
+static bool ec_handle_event(struct cros_ec_device *ec_dev)
{
- struct cros_ec_device *ec_dev = data;
bool wake_event = true;
int ret;
+ bool ec_has_more_events = false;
ret = cros_ec_get_next_event(ec_dev, &wake_event);
+ if (ret > 0)
+ ec_has_more_events =
+ ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
/*
* Signal only if wake host events or any interrupt if
@@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
0, ec_dev);
+
+ return ec_has_more_events;
+}
+
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+ struct cros_ec_device *ec_dev = data;
+ bool ec_has_more_events;
+
+ do {
+ ec_has_more_events = ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
+
return IRQ_HANDLED;
}
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f3..bb126d95b2fd4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ret = cros_ec_get_host_command_version_mask(ec_dev,
EC_CMD_GET_NEXT_EVENT,
&ver_mask);
- if (ret < 0 || ver_mask == 0)
+ if (ret < 0 || ver_mask == 0) {
ec_dev->mkbp_event_supported = 0;
- else
- ec_dev->mkbp_event_supported = 1;
+ dev_info(ec_dev->dev, "MKBP not supported\n");
+ } else {
+ ec_dev->mkbp_event_supported = fls(ver_mask);
+ dev_info(ec_dev->dev, "MKBP support version %u\n",
+ ec_dev->mkbp_event_supported - 1);
+ }
/*
* Get host event wake mask, assume all events are wake events
@@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
{
u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
- static int cmd_version = 1;
- int ret;
+ const int cmd_version = ec_dev->mkbp_event_supported - 1;
if (ec_dev->suspended) {
dev_dbg(ec_dev->dev, "Device suspended.\n");
return -EHOSTDOWN;
}
- 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)
- return ret;
-
- /* Fallback to version 0 for future send attempts */
- cmd_version = 0;
- }
-
- ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+ if (cmd_version == 0)
+ return get_next_event_xfer(ec_dev, msg, 0,
sizeof(struct ec_response_get_next_event));
- return ret;
+ return get_next_event_xfer(ec_dev, msg, cmd_version,
+ sizeof(struct ec_response_get_next_event_v1));
}
static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
@@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
{
+ u32 event_type =
+ ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
u32 host_event;
BUG_ON(!ec_dev->mkbp_event_supported);
- if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
+ if (event_type != EC_MKBP_EVENT_HOST_EVENT)
return 0;
if (ec_dev->event_size != sizeof(host_event)) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7d..cb07ee95a6eb8 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -119,7 +119,9 @@ struct cros_ec_command {
* code.
* @pkt_xfer: Send packet to EC and get response.
* @lock: One transaction at a time.
- * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
+ * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
+ * the maximum supported version of the MKBP host event
+ * command + 1.
* @event_notifier: Interrupt event notifier for transport devices.
* @event_data: Raw payload transferred with the MKBP event.
* @event_size: Size in bytes of the event data.
@@ -152,7 +154,8 @@ struct cros_ec_device {
int (*pkt_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
struct mutex lock;
- bool mkbp_event_supported;
+ /* 0 == not supported, otherwise it supports version x - 1 */
+ u8 mkbp_event_supported;
struct blocking_notifier_head event_notifier;
struct ec_response_get_next_event_v1 event_data;
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 9a9631f0559e2..3f013593d0107 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2146,6 +2146,14 @@ struct ec_result_keyscan_seq_ctrl {
*/
#define EC_CMD_GET_NEXT_EVENT 0x67
+#define EC_MKBP_HAS_MORE_EVENTS_SHIFT 7
+
+/* EC can provide more MKBP events to host */
+#define EC_MKBP_HAS_MORE_EVENTS (1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT)
+
+/* The mask to apply to get the raw event type */
+#define EC_MKBP_EVENT_TYPE_MASK ((1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT) - 1)
+
enum ec_mkbp_event {
/* Keyboard matrix changed. The event data is the new matrix state. */
EC_MKBP_EVENT_KEY_MATRIX = 0,
@@ -2173,6 +2181,13 @@ enum ec_mkbp_event {
/* Number of MKBP events */
EC_MKBP_EVENT_COUNT,
+
+ /*
+ * Maximum possible event type
+ * The most significant bit of event type is used to indicate that
+ * the EC has multiple events for the AP to serve
+ */
+ EC_MKBP_EVENT_MAX_TYPE = EC_MKBP_EVENT_TYPE_MASK,
};
union ec_response_get_next_data {
--
2.20.1.415.g653613c723-goog
On Thu, 20 Dec 2018, [email protected] wrote:
> From: Enrico Granata <[email protected]>
>
> The ChromeOS EC has support for signaling to the host that
> a single IRQ can serve multiple MKBP events.
>
> Doing this serves an optimization purpose, as it minimizes the
> number of round-trips into the interrupt handling machinery, and
> it proves beneficial to sensor timestamping as it keeps the desired
> synchronization of event times between the two processors.
>
> This patch adds kernel support for this EC feature, allowing the
> ec_irq to loop until all events have been served.
>
> Signed-off-by: Enrico Granata <[email protected]>
> ---
What is this?
A [RESEND]? If so, you should s/[PATCH]/[RESEND/ in the subject line.
An updated patch? If so, you need a Changelog here and a [PATCH v2].
Also, submitting in-reply-to isn't scalable.
Please resubmit this patch.
> drivers/mfd/cros_ec.c | 20 +++++++++++++--
> drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
> include/linux/mfd/cros_ec.h | 7 ++++--
> include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
> 4 files changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fe6f83766144f..17903a378aa1a 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
> .pdata_size = sizeof(pd_p),
> };
>
> -static irqreturn_t ec_irq_thread(int irq, void *data)
> +static bool ec_handle_event(struct cros_ec_device *ec_dev)
> {
> - struct cros_ec_device *ec_dev = data;
> bool wake_event = true;
> int ret;
> + bool ec_has_more_events = false;
>
> ret = cros_ec_get_next_event(ec_dev, &wake_event);
> + if (ret > 0)
> + ec_has_more_events =
> + ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
>
> /*
> * Signal only if wake host events or any interrupt if
> @@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> 0, ec_dev);
> +
> + return ec_has_more_events;
> +}
> +
> +static irqreturn_t ec_irq_thread(int irq, void *data)
> +{
> + struct cros_ec_device *ec_dev = data;
> + bool ec_has_more_events;
> +
> + do {
> + ec_has_more_events = ec_handle_event(ec_dev);
> + } while (ec_has_more_events);
> +
> return IRQ_HANDLED;
> }
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b6fd4838f60f3..bb126d95b2fd4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> EC_CMD_GET_NEXT_EVENT,
> &ver_mask);
> - if (ret < 0 || ver_mask == 0)
> + if (ret < 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> - else
> - ec_dev->mkbp_event_supported = 1;
> + dev_info(ec_dev->dev, "MKBP not supported\n");
> + } else {
> + ec_dev->mkbp_event_supported = fls(ver_mask);
> + dev_info(ec_dev->dev, "MKBP support version %u\n",
> + ec_dev->mkbp_event_supported - 1);
> + }
>
> /*
> * Get host event wake mask, assume all events are wake events
> @@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> {
> u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
> struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
> - static int cmd_version = 1;
> - int ret;
> + const int cmd_version = ec_dev->mkbp_event_supported - 1;
>
> if (ec_dev->suspended) {
> dev_dbg(ec_dev->dev, "Device suspended.\n");
> return -EHOSTDOWN;
> }
>
> - 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)
> - return ret;
> -
> - /* Fallback to version 0 for future send attempts */
> - cmd_version = 0;
> - }
> -
> - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> + if (cmd_version == 0)
> + return get_next_event_xfer(ec_dev, msg, 0,
> sizeof(struct ec_response_get_next_event));
>
> - return ret;
> + return get_next_event_xfer(ec_dev, msg, cmd_version,
> + sizeof(struct ec_response_get_next_event_v1));
> }
>
> static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> @@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
>
> u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
> {
> + u32 event_type =
> + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> u32 host_event;
>
> BUG_ON(!ec_dev->mkbp_event_supported);
>
> - if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
> + if (event_type != EC_MKBP_EVENT_HOST_EVENT)
> return 0;
>
> if (ec_dev->event_size != sizeof(host_event)) {
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index e44e3ec8a9c7d..cb07ee95a6eb8 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -119,7 +119,9 @@ struct cros_ec_command {
> * code.
> * @pkt_xfer: Send packet to EC and get response.
> * @lock: One transaction at a time.
> - * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
> + * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
> + * the maximum supported version of the MKBP host event
> + * command + 1.
> * @event_notifier: Interrupt event notifier for transport devices.
> * @event_data: Raw payload transferred with the MKBP event.
> * @event_size: Size in bytes of the event data.
> @@ -152,7 +154,8 @@ struct cros_ec_device {
> int (*pkt_xfer)(struct cros_ec_device *ec,
> struct cros_ec_command *msg);
> struct mutex lock;
> - bool mkbp_event_supported;
> + /* 0 == not supported, otherwise it supports version x - 1 */
> + u8 mkbp_event_supported;
> struct blocking_notifier_head event_notifier;
>
> struct ec_response_get_next_event_v1 event_data;
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 9a9631f0559e2..3f013593d0107 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -2146,6 +2146,14 @@ struct ec_result_keyscan_seq_ctrl {
> */
> #define EC_CMD_GET_NEXT_EVENT 0x67
>
> +#define EC_MKBP_HAS_MORE_EVENTS_SHIFT 7
> +
> +/* EC can provide more MKBP events to host */
> +#define EC_MKBP_HAS_MORE_EVENTS (1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT)
> +
> +/* The mask to apply to get the raw event type */
> +#define EC_MKBP_EVENT_TYPE_MASK ((1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT) - 1)
> +
> enum ec_mkbp_event {
> /* Keyboard matrix changed. The event data is the new matrix state. */
> EC_MKBP_EVENT_KEY_MATRIX = 0,
> @@ -2173,6 +2181,13 @@ enum ec_mkbp_event {
>
> /* Number of MKBP events */
> EC_MKBP_EVENT_COUNT,
> +
> + /*
> + * Maximum possible event type
> + * The most significant bit of event type is used to indicate that
> + * the EC has multiple events for the AP to serve
> + */
> + EC_MKBP_EVENT_MAX_TYPE = EC_MKBP_EVENT_TYPE_MASK,
> };
>
> union ec_response_get_next_data {
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Enrico Granata <[email protected]>
The ChromeOS EC has support for signaling to the host that
a single IRQ can serve multiple MKBP events.
Doing this serves an optimization purpose, as it minimizes the
number of round-trips into the interrupt handling machinery, and
it proves beneficial to sensor timestamping as it keeps the desired
synchronization of event times between the two processors.
This patch adds kernel support for this EC feature, allowing the
ec_irq to loop until all events have been served.
ChangeLog:
- Cleanup incorrect comment in cros_ec.h
Signed-off-by: Enrico Granata <[email protected]>
---
drivers/mfd/cros_ec.c | 20 +++++++++++++--
drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
include/linux/mfd/cros_ec.h | 7 ++++--
include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
4 files changed, 53 insertions(+), 22 deletions(-)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fe6f83766144f..17903a378aa1a 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
.pdata_size = sizeof(pd_p),
};
-static irqreturn_t ec_irq_thread(int irq, void *data)
+static bool ec_handle_event(struct cros_ec_device *ec_dev)
{
- struct cros_ec_device *ec_dev = data;
bool wake_event = true;
int ret;
+ bool ec_has_more_events = false;
ret = cros_ec_get_next_event(ec_dev, &wake_event);
+ if (ret > 0)
+ ec_has_more_events =
+ ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
/*
* Signal only if wake host events or any interrupt if
@@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
0, ec_dev);
+
+ return ec_has_more_events;
+}
+
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+ struct cros_ec_device *ec_dev = data;
+ bool ec_has_more_events;
+
+ do {
+ ec_has_more_events = ec_handle_event(ec_dev);
+ } while (ec_has_more_events);
+
return IRQ_HANDLED;
}
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b6fd4838f60f3..bb126d95b2fd4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ret = cros_ec_get_host_command_version_mask(ec_dev,
EC_CMD_GET_NEXT_EVENT,
&ver_mask);
- if (ret < 0 || ver_mask == 0)
+ if (ret < 0 || ver_mask == 0) {
ec_dev->mkbp_event_supported = 0;
- else
- ec_dev->mkbp_event_supported = 1;
+ dev_info(ec_dev->dev, "MKBP not supported\n");
+ } else {
+ ec_dev->mkbp_event_supported = fls(ver_mask);
+ dev_info(ec_dev->dev, "MKBP support version %u\n",
+ ec_dev->mkbp_event_supported - 1);
+ }
/*
* Get host event wake mask, assume all events are wake events
@@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
{
u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
- static int cmd_version = 1;
- int ret;
+ const int cmd_version = ec_dev->mkbp_event_supported - 1;
if (ec_dev->suspended) {
dev_dbg(ec_dev->dev, "Device suspended.\n");
return -EHOSTDOWN;
}
- 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)
- return ret;
-
- /* Fallback to version 0 for future send attempts */
- cmd_version = 0;
- }
-
- ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+ if (cmd_version == 0)
+ return get_next_event_xfer(ec_dev, msg, 0,
sizeof(struct ec_response_get_next_event));
- return ret;
+ return get_next_event_xfer(ec_dev, msg, cmd_version,
+ sizeof(struct ec_response_get_next_event_v1));
}
static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
@@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
{
+ u32 event_type =
+ ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
u32 host_event;
BUG_ON(!ec_dev->mkbp_event_supported);
- if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
+ if (event_type != EC_MKBP_EVENT_HOST_EVENT)
return 0;
if (ec_dev->event_size != sizeof(host_event)) {
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index e44e3ec8a9c7d..cb07ee95a6eb8 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -119,7 +119,9 @@ struct cros_ec_command {
* code.
* @pkt_xfer: Send packet to EC and get response.
* @lock: One transaction at a time.
- * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
+ * @mkbp_event_supported: 0 if MKBP not supported. Otherwise its value is
+ * the maximum supported version of the MKBP host event
+ * command + 1.
* @event_notifier: Interrupt event notifier for transport devices.
* @event_data: Raw payload transferred with the MKBP event.
* @event_size: Size in bytes of the event data.
@@ -152,7 +154,8 @@ struct cros_ec_device {
int (*pkt_xfer)(struct cros_ec_device *ec,
struct cros_ec_command *msg);
struct mutex lock;
- bool mkbp_event_supported;
+ /* 0 == not supported, otherwise it supports version x - 1 */
+ u8 mkbp_event_supported;
struct blocking_notifier_head event_notifier;
struct ec_response_get_next_event_v1 event_data;
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 9a9631f0559e2..3f013593d0107 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2146,6 +2146,14 @@ struct ec_result_keyscan_seq_ctrl {
*/
#define EC_CMD_GET_NEXT_EVENT 0x67
+#define EC_MKBP_HAS_MORE_EVENTS_SHIFT 7
+
+/* EC can provide more MKBP events to host */
+#define EC_MKBP_HAS_MORE_EVENTS (1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT)
+
+/* The mask to apply to get the raw event type */
+#define EC_MKBP_EVENT_TYPE_MASK ((1 << EC_MKBP_HAS_MORE_EVENTS_SHIFT) - 1)
+
enum ec_mkbp_event {
/* Keyboard matrix changed. The event data is the new matrix state. */
EC_MKBP_EVENT_KEY_MATRIX = 0,
@@ -2173,6 +2181,13 @@ enum ec_mkbp_event {
/* Number of MKBP events */
EC_MKBP_EVENT_COUNT,
+
+ /*
+ * Maximum possible event type
+ * The most significant bit of event type is used to indicate that
+ * the EC has multiple events for the AP to serve
+ */
+ EC_MKBP_EVENT_MAX_TYPE = EC_MKBP_EVENT_TYPE_MASK,
};
union ec_response_get_next_data {
--
2.20.1.415.g653613c723-goog
On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <[email protected]> wrote:
>
> Hi Enrico,
>
> On Thu, Nov 29, 2018 at 11:55:48AM -0800, [email protected] wrote:
> > From: Enrico Granata <[email protected]>
> >
> > The ChromeOS EC has support for signaling to the host that
> > a single IRQ can serve multiple MKBP events.
> >
> > Doing this serves an optimization purpose, as it minimizes the
> > number of round-trips into the interrupt handling machinery, and
> > it proves beneficial to sensor timestamping as it keeps the desired
> > synchronization of event times between the two processors.
> >
> > This patch adds kernel support for this EC feature, allowing the
> > ec_irq to loop until all events have been served.
>
> Might be worth being more explicit in this commit message to say that
> you're adding support for EC_CMD_GET_NEXT_EVENT version 2?
>
> > Signed-off-by: Enrico Granata <[email protected]>
>
> Mostly looks fine; thanks for sending. I have a few small notes (and
> some of that is not necessarily something resolve in this patch), but
> with at least the cros_ec.h comment fixup, feel free to add my:
>
> Reviewed-by: Brian Norris <[email protected]>
>
> > ---
> > drivers/mfd/cros_ec.c | 20 +++++++++++++--
> > drivers/platform/chrome/cros_ec_proto.c | 33 +++++++++++--------------
> > include/linux/mfd/cros_ec.h | 3 ++-
> > include/linux/mfd/cros_ec_commands.h | 15 +++++++++++
> > 4 files changed, 50 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> > index fe6f83766144f..17903a378aa1a 100644
> > --- a/drivers/mfd/cros_ec.c
> > +++ b/drivers/mfd/cros_ec.c
> > @@ -51,13 +51,16 @@ static const struct mfd_cell ec_pd_cell = {
> > .pdata_size = sizeof(pd_p),
> > };
> >
> > -static irqreturn_t ec_irq_thread(int irq, void *data)
> > +static bool ec_handle_event(struct cros_ec_device *ec_dev)
> > {
> > - struct cros_ec_device *ec_dev = data;
> > bool wake_event = true;
> > int ret;
> > + bool ec_has_more_events = false;
> >
> > ret = cros_ec_get_next_event(ec_dev, &wake_event);
> > + if (ret > 0)
> > + ec_has_more_events =
> > + ec_dev->event_data.event_type & EC_MKBP_HAS_MORE_EVENTS;
> >
> > /*
> > * Signal only if wake host events or any interrupt if
> > @@ -70,6 +73,19 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> > if (ret > 0)
> > blocking_notifier_call_chain(&ec_dev->event_notifier,
> > 0, ec_dev);
> > +
> > + return ec_has_more_events;
> > +}
> > +
> > +static irqreturn_t ec_irq_thread(int irq, void *data)
> > +{
> > + struct cros_ec_device *ec_dev = data;
> > + bool ec_has_more_events;
> > +
> > + do {
> > + ec_has_more_events = ec_handle_event(ec_dev);
> > + } while (ec_has_more_events);
> > +
> > return IRQ_HANDLED;
> > }
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index b6fd4838f60f3..bb126d95b2fd4 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > ret = cros_ec_get_host_command_version_mask(ec_dev,
> > EC_CMD_GET_NEXT_EVENT,
> > &ver_mask);
>
> It's not exactly new here (although you're using 'ver_mask' in new
> ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
> right. It doesn't look at msg->result, and instead just assumes that if
> we got some data back (send_command() > 0), then it must have been a
> success. I don't think that's really guaranteed in general, although it
> might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can
not be greater than 0. At best it will be 0, or a negative number if
we can already qualify the error in the errno space (see
cros_ec_pkt_xfer_i2c() for instance).
Gwendal.
>
> IOW, to be definitely sure we're not looking at a garbage result in
> 'ver_mask', we should probably fixup
> cros_ec_get_host_command_version_mask().
>
> > - if (ret < 0 || ver_mask == 0)
> > + if (ret < 0 || ver_mask == 0) {
> > ec_dev->mkbp_event_supported = 0;
> > - else
> > - ec_dev->mkbp_event_supported = 1;
> > + dev_info(ec_dev->dev, "MKBP not supported\n");
> > + } else {
> > + ec_dev->mkbp_event_supported = fls(ver_mask);
> > + dev_info(ec_dev->dev, "MKBP support version %u\n",
> > + ec_dev->mkbp_event_supported - 1);
> > + }
> >
> > /*
> > * Get host event wake mask, assume all events are wake events
> > @@ -530,28 +534,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> > {
> > u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
> > struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
> > - static int cmd_version = 1;
> > - int ret;
> > + const int cmd_version = ec_dev->mkbp_event_supported - 1;
> >
> > if (ec_dev->suspended) {
> > dev_dbg(ec_dev->dev, "Device suspended.\n");
> > return -EHOSTDOWN;
> > }
> >
> > - 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)
> > - return ret;
> > -
> > - /* Fallback to version 0 for future send attempts */
> > - cmd_version = 0;
> > - }
> > -
> > - ret = get_next_event_xfer(ec_dev, msg, cmd_version,
> > + if (cmd_version == 0)
> > + return get_next_event_xfer(ec_dev, msg, 0,
> > sizeof(struct ec_response_get_next_event));
> >
> > - return ret;
> > + return get_next_event_xfer(ec_dev, msg, cmd_version,
> > + sizeof(struct ec_response_get_next_event_v1));
> > }
> >
> > static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
> > @@ -607,11 +602,13 @@ EXPORT_SYMBOL(cros_ec_get_next_event);
> >
> > u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
> > {
> > + u32 event_type =
> > + ec_dev->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK;
> > u32 host_event;
> >
> > BUG_ON(!ec_dev->mkbp_event_supported);
> >
> > - if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
> > + if (event_type != EC_MKBP_EVENT_HOST_EVENT)
> > return 0;
> >
> > if (ec_dev->event_size != sizeof(host_event)) {
> > diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> > index e44e3ec8a9c7d..eb771ceeaeed1 100644
> > --- a/include/linux/mfd/cros_ec.h
> > +++ b/include/linux/mfd/cros_ec.h
> > @@ -152,7 +152,8 @@ struct cros_ec_device {
> > int (*pkt_xfer)(struct cros_ec_device *ec,
> > struct cros_ec_command *msg);
> > struct mutex lock;
> > - bool mkbp_event_supported;
> > + /* 0 == not supported, otherwise it supports version x - 1 */
>
> This comment belongs in the kerneldoc, which is above the struct
> definition. You're invalidating the existing comment:
>
> * @mkbp_event_supported: True if this EC supports the MKBP event protocol.
>
> Brian
>
>
> > + u8 mkbp_event_supported;
> > struct blocking_notifier_head event_notifier;
> >
> > struct ec_response_get_next_event_v1 event_data;
>
> ...
Hi Gwendal,
On Mon, Jan 14, 2019 at 3:50 PM Gwendal Grignou <[email protected]> wrote:
> On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <[email protected]> wrote:
> > On Thu, Nov 29, 2018 at 11:55:48AM -0800, [email protected] wrote:
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > > ret = cros_ec_get_host_command_version_mask(ec_dev,
> > > EC_CMD_GET_NEXT_EVENT,
> > > &ver_mask);
> >
> > It's not exactly new here (although you're using 'ver_mask' in new
> > ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
> > right. It doesn't look at msg->result, and instead just assumes that if
> > we got some data back (send_command() > 0), then it must have been a
> > success. I don't think that's really guaranteed in general, although it
> > might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
> It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can
> not be greater than 0. At best it will be 0, or a negative number if
> we can already qualify the error in the errno space (see
> cros_ec_pkt_xfer_i2c() for instance).
Sorry, where do you guarantee that? The only enforcements I see in
those xfer implementation are:
(1) if result == EC_RES_IN_PROGRESS, we convert that to an errno
(2) if the expected length or checksum are bad, we turn that to an errno
So technically, the EC *could* be sending a valid, checksummed
response of the expected length, while still setting the ->result
field to something besides EC_RES_SUCCESS or EC_RES_IN_PROGRESS. And
we would treat that as a valid 'ver_mask'.
Albeit, that seems unlikely, given understanding of how the EC is
supposed to behave, but our code is not properly defensive AIUI. This
is basically why cros_ec_cmd_xfer_status() exists -- so that
sub-drivers don't get lazy and use cros_ec_cmd_xfer() without handling
the ->result field properly.
Brian
On Mon, Jan 14, 2019 at 6:04 PM Brian Norris <[email protected]> wrote:
>
> Hi Gwendal,
>
> On Mon, Jan 14, 2019 at 3:50 PM Gwendal Grignou <[email protected]> wrote:
> > On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <[email protected]> wrote:
> > > On Thu, Nov 29, 2018 at 11:55:48AM -0800, [email protected] wrote:
> > > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > > > ret = cros_ec_get_host_command_version_mask(ec_dev,
> > > > EC_CMD_GET_NEXT_EVENT,
> > > > &ver_mask);
> > >
> > > It's not exactly new here (although you're using 'ver_mask' in new
> > > ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
> > > right. It doesn't look at msg->result, and instead just assumes that if
> > > we got some data back (send_command() > 0), then it must have been a
> > > success. I don't think that's really guaranteed in general, although it
> > > might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
>
> > It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can
> > not be greater than 0. At best it will be 0, or a negative number if
> > we can already qualify the error in the errno space (see
> > T() for instance).
>
> Sorry, where do you guarantee that? The only enforcements I see in
> those xfer implementation are:
> (1) if result == EC_RES_IN_PROGRESS, we convert that to an errno
> (2) if the expected length or checksum are bad, we turn that to an errno
>
> So technically, the EC *could* be sending a valid, checksummed
> response of the expected length, while still setting the ->result
> field to something besides EC_RES_SUCCESS or EC_RES_IN_PROGRESS. And
> we would treat that as a valid 'ver_mask'.
You're right, I misread cros_ec_pkt_xfer_i2c().
> Albeit, that seems unlikely, given understanding of how the EC is
> supposed to behave, but our code is not properly defensive AIUI. This
> is basically why cros_ec_cmd_xfer_status() exists -- so that
> sub-drivers don't get lazy and use cros_ec_cmd_xfer() without handling
> the ->result field properly.
send_command is called for a very small subset of command where the
ec_dev mutex is already held. We indeed need to be careful when
calling it directly.
Gwendal.
>
> Brian