The first 5 patches add Kunit tests and refactors for cros_ec_cmd_xfer().
The last 6 patches change the behavior a bit by altering return codes.
Asynchronous mechanism in EC protocol:
EC returns EC_RES_IN_PROGRESS if the host command needs more time to complete.
It saves the result into `saved_result` once the command completed[1].
By design, AP should send another command EC_CMD_RESEND_RESPONSE for getting
the result from the previous pending command[2]. The mechanism was only
designed for commands that don't need responses[3].
However, the kernel code doesn't have such logic when dealing with
EC_RES_IN_PROGRESS.
The series doesn't fix it but leave it as is. I doubt there is no existing
use case.
[1]: https://crrev.com/9b80051e01872d8cae86fff999b5d31a0bea985b/common/host_command.c#113
[2]: https://crrev.com/9b80051e01872d8cae86fff999b5d31a0bea985b/common/host_command.c#748
[3]: https://crrev.com/9b80051e01872d8cae86fff999b5d31a0bea985b/common/host_command.c#126
Return value overridden in cros_ec_send_command() if asynchronous:
By original intention, cros_ec_send_command() returns number of available
bytes of input payload.
When it falls into asynchronous path (i.e. EC_RES_IN_PROGRESS), both return
value and `msg->result` will be overridden by the subsequent
EC_CMD_GET_COMMS_STATUS.
The series doesn't fix it but leave it as is.
Tzung-Bi Shih (11):
platform/chrome: cros_ec_proto: add "cros_ec_" prefix to
send_command()
platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_cmd_xfer()
platform/chrome: cros_ec_proto: add Kunit tests for
cros_ec_send_command()
platform/chrome: cros_ec_proto: separate cros_ec_xfer_command()
platform/chrome: cros_ec_proto: separate cros_ec_wait_until_complete()
platform/chrome: cros_ec_proto: change Kunit expectation when timed
out
platform/chrome: cros_ec_proto: return -EAGAIN when retries timed out
platform/chrome: cros_ec_proto: change Kunit expectation for EC errors
platform/chrome: cros_ec_proto: return standard error codes for EC
errors
platform/chrome: cros_ec_proto: add Kunit test for empty payload
platform/chrome: cros_ec_proto: return -EPROTO if empty payload
drivers/platform/chrome/cros_ec_proto.c | 106 +++--
drivers/platform/chrome/cros_ec_proto_test.c | 443 +++++++++++++++++++
drivers/platform/chrome/cros_kunit_util.c | 20 +
drivers/platform/chrome/cros_kunit_util.h | 4 +
4 files changed, 527 insertions(+), 46 deletions(-)
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_send_command() has extra logic to handle EC_RES_IN_PROGRESS.
Separate the command transfer part into cros_ec_xfer_command() so
that other functions can re-use it.
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b02fd1414e52..0cec013be3d3 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -107,7 +107,7 @@ static int prepare_tx_legacy(struct cros_ec_device *ec_dev,
return EC_MSG_TX_PROTO_BYTES + msg->outsize;
}
-static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+static int cros_ec_xfer_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
{
int ret;
int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
@@ -123,14 +123,21 @@ static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_co
* the EC is trying to use protocol v2, on an underlying
* communication mechanism that does not support v2.
*/
- dev_err_once(ec_dev->dev,
- "missing EC transfer API, cannot send command\n");
+ dev_err_once(ec_dev->dev, "missing EC transfer API, cannot send command\n");
return -EIO;
}
trace_cros_ec_request_start(msg);
ret = (*xfer_fxn)(ec_dev, msg);
trace_cros_ec_request_done(msg, ret);
+
+ return ret;
+}
+
+static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+{
+ int ret = cros_ec_xfer_command(ec_dev, msg);
+
if (msg->result == EC_RES_IN_PROGRESS) {
int i;
struct cros_ec_command *status_msg;
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_cmd_xfer() transfers the given command and data if any. It
performs some sanity checks and calls cros_ec_send_command().
Add Kunit tests for cros_ec_cmd_xfer().
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto_test.c | 149 +++++++++++++++++++
1 file changed, 149 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 1e2a1522c288..33721607a5b9 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1535,6 +1535,151 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return0(struct kunit
}
}
+static void cros_ec_proto_test_cmd_xfer_normal(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct {
+ struct cros_ec_command msg;
+ u8 data[0x100];
+ } __packed buf;
+
+ ec_dev->max_request = 0xff;
+ ec_dev->max_response = 0xee;
+ ec_dev->max_passthru = 0xdd;
+
+ buf.msg.version = 0;
+ buf.msg.command = EC_CMD_HELLO;
+ buf.msg.insize = 4;
+ buf.msg.outsize = 2;
+ buf.data[0] = 0x55;
+ buf.data[1] = 0xaa;
+
+ {
+ u8 *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, 4);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (u8 *)mock->o_data;
+ data[0] = 0xaa;
+ data[1] = 0x55;
+ data[2] = 0xcc;
+ data[3] = 0x33;
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ KUNIT_EXPECT_EQ(test, ret, 4);
+
+ {
+ u8 *data;
+
+ mock = cros_kunit_ec_xfer_mock_next();
+ KUNIT_EXPECT_PTR_NE(test, mock, NULL);
+
+ KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
+ KUNIT_EXPECT_EQ(test, mock->msg.command, EC_CMD_HELLO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize, 4);
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 2);
+
+ data = (u8 *)mock->i_data;
+ KUNIT_EXPECT_EQ(test, data[0], 0x55);
+ KUNIT_EXPECT_EQ(test, data[1], 0xaa);
+
+ KUNIT_EXPECT_EQ(test, buf.data[0], 0xaa);
+ KUNIT_EXPECT_EQ(test, buf.data[1], 0x55);
+ KUNIT_EXPECT_EQ(test, buf.data[2], 0xcc);
+ KUNIT_EXPECT_EQ(test, buf.data[3], 0x33);
+ }
+}
+
+static void cros_ec_proto_test_cmd_xfer_excess_msg_insize(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct {
+ struct cros_ec_command msg;
+ u8 data[0x100];
+ } __packed buf;
+
+ ec_dev->max_request = 0xff;
+ ec_dev->max_response = 0xee;
+ ec_dev->max_passthru = 0xdd;
+
+ buf.msg.version = 0;
+ buf.msg.command = EC_CMD_HELLO;
+ buf.msg.insize = 0xee + 1;
+ buf.msg.outsize = 2;
+
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0xcc);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ KUNIT_EXPECT_EQ(test, ret, 0xcc);
+
+ {
+ mock = cros_kunit_ec_xfer_mock_next();
+ KUNIT_EXPECT_PTR_NE(test, mock, NULL);
+
+ KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
+ KUNIT_EXPECT_EQ(test, mock->msg.command, EC_CMD_HELLO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize, 0xee);
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 2);
+ }
+}
+
+static void cros_ec_proto_test_cmd_xfer_excess_msg_outsize_without_passthru(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct {
+ struct cros_ec_command msg;
+ u8 data[0x100];
+ } __packed buf;
+
+ ec_dev->max_request = 0xff;
+ ec_dev->max_response = 0xee;
+ ec_dev->max_passthru = 0xdd;
+
+ buf.msg.version = 0;
+ buf.msg.command = EC_CMD_HELLO;
+ buf.msg.insize = 4;
+ buf.msg.outsize = 0xff + 1;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ KUNIT_EXPECT_EQ(test, ret, -EMSGSIZE);
+}
+
+static void cros_ec_proto_test_cmd_xfer_excess_msg_outsize_with_passthru(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct {
+ struct cros_ec_command msg;
+ u8 data[0x100];
+ } __packed buf;
+
+ ec_dev->max_request = 0xff;
+ ec_dev->max_response = 0xee;
+ ec_dev->max_passthru = 0xdd;
+
+ buf.msg.version = 0;
+ buf.msg.command = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) + EC_CMD_HELLO;
+ buf.msg.insize = 4;
+ buf.msg.outsize = 0xdd + 1;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ KUNIT_EXPECT_EQ(test, ret, -EMSGSIZE);
+}
+
static void cros_ec_proto_test_release(struct device *dev)
{
}
@@ -1601,6 +1746,10 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep_return0),
KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask_return_error),
KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask_return0),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_normal),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_insize),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_outsize_without_passthru),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_outsize_with_passthru),
{}
};
--
2.37.0.rc0.161.g10f37bed90-goog
EC returns EC_RES_IN_PROGRESS if the host command needs more time to
complete. Whenever receives the return code, cros_ec_send_command()
sends EC_CMD_GET_COMMS_STATUS to query the command status.
Separate cros_ec_wait_until_complete() from cros_ec_send_command().
It sends EC_CMD_GET_COMMS_STATUS and waits until the previous command
was completed, or encountered error, or timed out.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 75 ++++++++++++-------------
1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 0cec013be3d3..466ecb063bd6 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -134,53 +134,50 @@ static int cros_ec_xfer_command(struct cros_ec_device *ec_dev, struct cros_ec_co
return ret;
}
-static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *result)
{
- int ret = cros_ec_xfer_command(ec_dev, msg);
+ struct cros_ec_command *msg;
+ struct ec_response_get_comms_status *status;
+ int ret = 0, i;
+
+ msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
- if (msg->result == EC_RES_IN_PROGRESS) {
- int i;
- struct cros_ec_command *status_msg;
- struct ec_response_get_comms_status *status;
+ msg->command = EC_CMD_GET_COMMS_STATUS;
+ msg->insize = sizeof(*status);
- status_msg = kmalloc(sizeof(*status_msg) + sizeof(*status),
- GFP_KERNEL);
- if (!status_msg)
- return -ENOMEM;
+ status = (struct ec_response_get_comms_status *)msg->data;
- status_msg->version = 0;
- status_msg->command = EC_CMD_GET_COMMS_STATUS;
- status_msg->insize = sizeof(*status);
- status_msg->outsize = 0;
+ /* Query the EC's status until it's no longer busy or we encounter an error. */
+ for (i = 0; i < EC_COMMAND_RETRIES; ++i) {
+ usleep_range(10000, 11000);
- /*
- * Query the EC's status until it's no longer busy or
- * we encounter an error.
- */
- for (i = 0; i < EC_COMMAND_RETRIES; i++) {
- usleep_range(10000, 11000);
-
- trace_cros_ec_request_start(status_msg);
- ret = (*xfer_fxn)(ec_dev, status_msg);
- trace_cros_ec_request_done(status_msg, ret);
- if (ret == -EAGAIN)
- continue;
- if (ret < 0)
- break;
-
- msg->result = status_msg->result;
- if (status_msg->result != EC_RES_SUCCESS)
- break;
-
- status = (struct ec_response_get_comms_status *)
- status_msg->data;
- if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
- break;
- }
+ ret = cros_ec_xfer_command(ec_dev, msg);
+ if (ret == -EAGAIN)
+ continue;
+ if (ret < 0)
+ break;
+
+ *result = msg->result;
+ if (msg->result != EC_RES_SUCCESS)
+ break;
- kfree(status_msg);
+ if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
+ break;
}
+ kfree(msg);
+ return ret;
+}
+
+static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+{
+ int ret = cros_ec_xfer_command(ec_dev, msg);
+
+ if (msg->result == EC_RES_IN_PROGRESS)
+ ret = cros_ec_wait_until_complete(ec_dev, &msg->result);
+
return ret;
}
--
2.37.0.rc0.161.g10f37bed90-goog
While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
cros_ec_wait_until_complete() doesn't return an error code.
Change the expectation to an error code.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto_test.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 64100fd81c6a..fbb872040711 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1870,9 +1870,7 @@ static void cros_ec_proto_test_cmd_xfer_in_progress_retries_status_processing(st
}
ret = cros_ec_cmd_xfer(ec_dev, &msg);
- KUNIT_EXPECT_EQ(test, ret, sizeof(struct ec_response_get_comms_status));
-
- KUNIT_EXPECT_EQ(test, msg.result, EC_RES_SUCCESS);
+ KUNIT_EXPECT_EQ(test, ret, -EAGAIN);
/* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 51);
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_wait_until_complete() checks `msg->result` for
EC_CMD_GET_COMMS_STATUS. However, it doesn't return standard error codes
like most of others.
Use cros_ec_map_error() to align them.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 49772a4c5353..5323edddb540 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -138,7 +138,7 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
{
struct cros_ec_command *msg;
struct ec_response_get_comms_status *status;
- int ret = 0, i;
+ int ret = 0, i, mapped;
msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
if (!msg)
@@ -160,8 +160,11 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
break;
*result = msg->result;
- if (msg->result != EC_RES_SUCCESS)
+ mapped = cros_ec_map_error(msg->result);
+ if (mapped) {
+ ret = mapped;
break;
+ }
if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
break;
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_wait_until_complete() sends EC_CMD_GET_COMMS_STATUS which expects
to receive sizeof(struct ec_response_get_comms_status) from
cros_ec_xfer_command().
Return -EPROTO if cros_ec_xfer_command() returns 0.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 5323edddb540..0c7042aa2640 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -166,6 +166,11 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
break;
}
+ if (ret == 0) {
+ ret = -EPROTO;
+ break;
+ }
+
if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
break;
}
--
2.37.0.rc0.161.g10f37bed90-goog
To be neat, add "cros_ec_" prefix to static function send_command().
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 6923ea4401e5..b02fd1414e52 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -107,8 +107,7 @@ static int prepare_tx_legacy(struct cros_ec_device *ec_dev,
return EC_MSG_TX_PROTO_BYTES + msg->outsize;
}
-static int send_command(struct cros_ec_device *ec_dev,
- struct cros_ec_command *msg)
+static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
{
int ret;
int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
@@ -255,7 +254,7 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint3
msg->command = EC_CMD_HOST_EVENT_GET_WAKE_MASK;
msg->insize = sizeof(*r);
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
if (ret < 0)
goto exit;
@@ -295,7 +294,7 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
msg->command = EC_CMD_PASSTHRU_OFFSET(devidx) | EC_CMD_GET_PROTOCOL_INFO;
msg->insize = sizeof(*info);
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
/*
* Send command once again when timeout occurred.
* Fingerprint MCU (FPMCU) is restarted during system boot which
@@ -304,7 +303,7 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
* attempt because we waited at least EC_MSG_DEADLINE_MS.
*/
if (ret == -ETIMEDOUT)
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
if (ret < 0) {
dev_dbg(ec_dev->dev,
@@ -376,7 +375,7 @@ static int cros_ec_get_proto_info_legacy(struct cros_ec_device *ec_dev)
params = (struct ec_params_hello *)msg->data;
params->in_data = 0xa0b0c0d0;
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
if (ret < 0) {
dev_dbg(ec_dev->dev, "EC failed to respond to v2 hello: %d\n", ret);
goto exit;
@@ -453,7 +452,7 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
pver = (struct ec_params_get_cmd_versions *)msg->data;
pver->cmd = cmd;
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
if (ret < 0)
goto exit;
@@ -634,7 +633,7 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
}
}
- ret = send_command(ec_dev, msg);
+ ret = cros_ec_send_command(ec_dev, msg);
mutex_unlock(&ec_dev->lock);
return ret;
--
2.37.0.rc0.161.g10f37bed90-goog
While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
cros_ec_wait_until_complete() doesn't return an error code.
Return -EAGAIN in the case instead.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 466ecb063bd6..49772a4c5353 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -167,6 +167,9 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
break;
}
+ if (i >= EC_COMMAND_RETRIES)
+ ret = -EAGAIN;
+
kfree(msg);
return ret;
}
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_wait_until_complete() checks `msg->result` for
EC_CMD_GET_COMMS_STATUS. However, it doesn't return standard error codes
like most of others.
Change the Kunit test expectation to align them.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index fbb872040711..2a6b099fbfd9 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1927,7 +1927,7 @@ static void cros_ec_proto_test_cmd_xfer_in_progress_return_error(struct kunit *t
}
ret = cros_ec_cmd_xfer(ec_dev, &msg);
- KUNIT_EXPECT_EQ(test, ret, 0);
+ KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP);
KUNIT_EXPECT_EQ(test, msg.result, EC_RES_INVALID_COMMAND);
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_wait_until_complete() sends EC_CMD_GET_COMMS_STATUS which expects
to receive sizeof(struct ec_response_get_comms_status) from
cros_ec_xfer_command().
Add Kunit test and expect to receive an error code when
cros_ec_xfer_command() returns 0.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto_test.c | 31 ++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 2a6b099fbfd9..7d45e5022221 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1934,6 +1934,36 @@ static void cros_ec_proto_test_cmd_xfer_in_progress_return_error(struct kunit *t
KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 2);
}
+static void cros_ec_proto_test_cmd_xfer_in_progress_return0(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, -EPROTO);
+
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 2);
+}
+
static void cros_ec_proto_test_release(struct device *dev)
{
}
@@ -2013,6 +2043,7 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_retries_status_processing),
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_xfer_error),
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_return_error),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_return0),
{}
};
--
2.37.0.rc0.161.g10f37bed90-goog
cros_ec_cmd_xfer() is the only exported function that calls static
function cros_ec_send_command().
Add Kunit tests for cros_ec_send_command() through calling
cros_ec_cmd_xfer().
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
drivers/platform/chrome/cros_ec_proto_test.c | 265 +++++++++++++++++++
drivers/platform/chrome/cros_kunit_util.c | 20 ++
drivers/platform/chrome/cros_kunit_util.h | 4 +
3 files changed, 289 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 33721607a5b9..64100fd81c6a 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1680,6 +1680,262 @@ static void cros_ec_proto_test_cmd_xfer_excess_msg_outsize_with_passthru(struct
KUNIT_EXPECT_EQ(test, ret, -EMSGSIZE);
}
+static void cros_ec_proto_test_cmd_xfer_protocol_v3_normal(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->proto_version = 3;
+ ec_dev->cmd_xfer = cros_kunit_ec_cmd_xfer_mock;
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_cmd_xfer_mock_called, 0);
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 1);
+}
+
+static void cros_ec_proto_test_cmd_xfer_protocol_v3_no_op(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->proto_version = 3;
+ ec_dev->cmd_xfer = cros_kunit_ec_cmd_xfer_mock;
+ ec_dev->pkt_xfer = NULL;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, -EIO);
+}
+
+static void cros_ec_proto_test_cmd_xfer_protocol_v2_normal(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->proto_version = 2;
+ ec_dev->cmd_xfer = cros_kunit_ec_cmd_xfer_mock;
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_cmd_xfer_mock_called, 1);
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 0);
+}
+
+static void cros_ec_proto_test_cmd_xfer_protocol_v2_no_op(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->proto_version = 2;
+ ec_dev->cmd_xfer = NULL;
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, -EIO);
+}
+
+static void cros_ec_proto_test_cmd_xfer_in_progress_normal(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS. */
+ {
+ struct ec_response_get_comms_status *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_get_comms_status *)mock->o_data;
+ data->flags = 0;
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, sizeof(struct ec_response_get_comms_status));
+
+ KUNIT_EXPECT_EQ(test, msg.result, EC_RES_SUCCESS);
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_next();
+ KUNIT_EXPECT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_next();
+ KUNIT_EXPECT_PTR_NE(test, mock, NULL);
+
+ KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
+ KUNIT_EXPECT_EQ(test, mock->msg.command, EC_CMD_GET_COMMS_STATUS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_comms_status));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 2);
+}
+
+static void cros_ec_proto_test_cmd_xfer_in_progress_retries_eagain(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
+ cros_kunit_ec_xfer_mock_default_ret = -EAGAIN;
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, -EAGAIN);
+
+ /* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 51);
+}
+
+static void cros_ec_proto_test_cmd_xfer_in_progress_retries_status_processing(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
+ {
+ struct ec_response_get_comms_status *data;
+ int i;
+
+ for (i = 0; i < 50; ++i) {
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_get_comms_status *)mock->o_data;
+ data->flags |= EC_COMMS_STATUS_PROCESSING;
+ }
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, sizeof(struct ec_response_get_comms_status));
+
+ KUNIT_EXPECT_EQ(test, msg.result, EC_RES_SUCCESS);
+
+ /* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 51);
+}
+
+static void cros_ec_proto_test_cmd_xfer_in_progress_xfer_error(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, -EIO, EC_RES_SUCCESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, -EIO);
+}
+
+static void cros_ec_proto_test_cmd_xfer_in_progress_return_error(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+ struct cros_ec_command msg;
+
+ memset(&msg, 0, sizeof(msg));
+
+ ec_dev->pkt_xfer = cros_kunit_ec_pkt_xfer_mock;
+
+ /* For the first host command to return EC_RES_IN_PROGRESS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_IN_PROGRESS, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For EC_CMD_GET_COMMS_STATUS. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ ret = cros_ec_cmd_xfer(ec_dev, &msg);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ KUNIT_EXPECT_EQ(test, msg.result, EC_RES_INVALID_COMMAND);
+
+ KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 2);
+}
+
static void cros_ec_proto_test_release(struct device *dev)
{
}
@@ -1750,6 +2006,15 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_insize),
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_outsize_without_passthru),
KUNIT_CASE(cros_ec_proto_test_cmd_xfer_excess_msg_outsize_with_passthru),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_protocol_v3_normal),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_protocol_v3_no_op),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_protocol_v2_normal),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_protocol_v2_no_op),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_normal),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_retries_eagain),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_retries_status_processing),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_xfer_error),
+ KUNIT_CASE(cros_ec_proto_test_cmd_xfer_in_progress_return_error),
{}
};
diff --git a/drivers/platform/chrome/cros_kunit_util.c b/drivers/platform/chrome/cros_kunit_util.c
index e031777dea87..3ede971e82ee 100644
--- a/drivers/platform/chrome/cros_kunit_util.c
+++ b/drivers/platform/chrome/cros_kunit_util.c
@@ -15,6 +15,10 @@
int cros_kunit_ec_xfer_mock_default_ret;
EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_default_ret);
+int cros_kunit_ec_cmd_xfer_mock_called;
+EXPORT_SYMBOL_GPL(cros_kunit_ec_cmd_xfer_mock_called);
+int cros_kunit_ec_pkt_xfer_mock_called;
+EXPORT_SYMBOL_GPL(cros_kunit_ec_pkt_xfer_mock_called);
static struct list_head cros_kunit_ec_xfer_mock_in;
static struct list_head cros_kunit_ec_xfer_mock_out;
@@ -46,6 +50,20 @@ int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_comman
}
EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock);
+int cros_kunit_ec_cmd_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+{
+ ++cros_kunit_ec_cmd_xfer_mock_called;
+ return cros_kunit_ec_xfer_mock(ec_dev, msg);
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_cmd_xfer_mock);
+
+int cros_kunit_ec_pkt_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+{
+ ++cros_kunit_ec_pkt_xfer_mock_called;
+ return cros_kunit_ec_xfer_mock(ec_dev, msg);
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_pkt_xfer_mock);
+
struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size)
{
return cros_kunit_ec_xfer_mock_addx(test, size, EC_RES_SUCCESS, size);
@@ -90,6 +108,8 @@ EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_next);
void cros_kunit_mock_reset(void)
{
cros_kunit_ec_xfer_mock_default_ret = 0;
+ cros_kunit_ec_cmd_xfer_mock_called = 0;
+ cros_kunit_ec_pkt_xfer_mock_called = 0;
INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_in);
INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_out);
}
diff --git a/drivers/platform/chrome/cros_kunit_util.h b/drivers/platform/chrome/cros_kunit_util.h
index 79c4525f873c..ae4080cb13f1 100644
--- a/drivers/platform/chrome/cros_kunit_util.h
+++ b/drivers/platform/chrome/cros_kunit_util.h
@@ -24,8 +24,12 @@ struct ec_xfer_mock {
};
extern int cros_kunit_ec_xfer_mock_default_ret;
+extern int cros_kunit_ec_cmd_xfer_mock_called;
+extern int cros_kunit_ec_pkt_xfer_mock_called;
int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg);
+int cros_kunit_ec_cmd_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg);
+int cros_kunit_ec_pkt_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg);
struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size);
struct ec_xfer_mock *cros_kunit_ec_xfer_mock_addx(struct kunit *test,
int ret, int result, size_t size);
--
2.37.0.rc0.161.g10f37bed90-goog
On Tue, Jun 28, 2022 at 02:49:07AM +0000, Tzung-Bi Shih wrote:
> EC returns EC_RES_IN_PROGRESS if the host command needs more time to
> complete. Whenever receives the return code, cros_ec_send_command()
> sends EC_CMD_GET_COMMS_STATUS to query the command status.
>
> Separate cros_ec_wait_until_complete() from cros_ec_send_command().
> It sends EC_CMD_GET_COMMS_STATUS and waits until the previous command
> was completed, or encountered error, or timed out.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
Could someone on the list help to review the remaining patches in the series?
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_wait_until_complete() checks `msg->result` for
> EC_CMD_GET_COMMS_STATUS. However, it doesn't return standard error codes
> like most of others.
>
> Change the Kunit test expectation to align them.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index fbb872040711..2a6b099fbfd9 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -1927,7 +1927,7 @@ static void cros_ec_proto_test_cmd_xfer_in_progress_return_error(struct kunit *t
> }
>
> ret = cros_ec_cmd_xfer(ec_dev, &msg);
> - KUNIT_EXPECT_EQ(test, ret, 0);
> + KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP);
>
> KUNIT_EXPECT_EQ(test, msg.result, EC_RES_INVALID_COMMAND);
>
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_wait_until_complete() checks `msg->result` for
> EC_CMD_GET_COMMS_STATUS. However, it doesn't return standard error codes
> like most of others.
The callers of cros_ec_send_command() do the mapping. I am not sure if
it is a good idea to change that; it may have undesired side effects
(such as changing the userspace ABI) for callers of
cros_ec_send_command() not expecting this change. It would also result
in double mapping in some situations.
Guenter
>
> Use cros_ec_map_error() to align them.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 49772a4c5353..5323edddb540 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -138,7 +138,7 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
> {
> struct cros_ec_command *msg;
> struct ec_response_get_comms_status *status;
> - int ret = 0, i;
> + int ret = 0, i, mapped;
>
> msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
> if (!msg)
> @@ -160,8 +160,11 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
> break;
>
> *result = msg->result;
> - if (msg->result != EC_RES_SUCCESS)
> + mapped = cros_ec_map_error(msg->result);
> + if (mapped) {
> + ret = mapped;
> break;
> + }
>
> if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> break;
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> cros_ec_wait_until_complete() doesn't return an error code.
>
> Return -EAGAIN in the case instead.
Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
if it is still busy (stuck ?) with executing a command and it gets
another one ?
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 466ecb063bd6..49772a4c5353 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -167,6 +167,9 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
> break;
> }
>
> + if (i >= EC_COMMAND_RETRIES)
> + ret = -EAGAIN;
> +
> kfree(msg);
> return ret;
> }
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_wait_until_complete() sends EC_CMD_GET_COMMS_STATUS which expects
> to receive sizeof(struct ec_response_get_comms_status) from
> cros_ec_xfer_command().
>
> Return -EPROTO if cros_ec_xfer_command() returns 0.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 5323edddb540..0c7042aa2640 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -166,6 +166,11 @@ static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *
> break;
> }
>
> + if (ret == 0) {
> + ret = -EPROTO;
> + break;
> + }
> +
> if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> break;
> }
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> EC returns EC_RES_IN_PROGRESS if the host command needs more time to
> complete. Whenever receives the return code, cros_ec_send_command()
> sends EC_CMD_GET_COMMS_STATUS to query the command status.
>
> Separate cros_ec_wait_until_complete() from cros_ec_send_command().
> It sends EC_CMD_GET_COMMS_STATUS and waits until the previous command
> was completed, or encountered error, or timed out.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 75 ++++++++++++-------------
> 1 file changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 0cec013be3d3..466ecb063bd6 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -134,53 +134,50 @@ static int cros_ec_xfer_command(struct cros_ec_device *ec_dev, struct cros_ec_co
> return ret;
> }
>
> -static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> +static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *result)
> {
> - int ret = cros_ec_xfer_command(ec_dev, msg);
> + struct cros_ec_command *msg;
> + struct ec_response_get_comms_status *status;
> + int ret = 0, i;
> +
> + msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
AFAICS this is always 24 bytes. I would suggest to allocate it on the
stack to reduce overhead.
>
> - if (msg->result == EC_RES_IN_PROGRESS) {
> - int i;
> - struct cros_ec_command *status_msg;
> - struct ec_response_get_comms_status *status;
> + msg->command = EC_CMD_GET_COMMS_STATUS;
> + msg->insize = sizeof(*status);
>
> - status_msg = kmalloc(sizeof(*status_msg) + sizeof(*status),
> - GFP_KERNEL);
> - if (!status_msg)
> - return -ENOMEM;
> + status = (struct ec_response_get_comms_status *)msg->data;
>
> - status_msg->version = 0;
> - status_msg->command = EC_CMD_GET_COMMS_STATUS;
> - status_msg->insize = sizeof(*status);
> - status_msg->outsize = 0;
> + /* Query the EC's status until it's no longer busy or we encounter an error. */
> + for (i = 0; i < EC_COMMAND_RETRIES; ++i) {
> + usleep_range(10000, 11000);
>
> - /*
> - * Query the EC's status until it's no longer busy or
> - * we encounter an error.
> - */
> - for (i = 0; i < EC_COMMAND_RETRIES; i++) {
> - usleep_range(10000, 11000);
> -
> - trace_cros_ec_request_start(status_msg);
> - ret = (*xfer_fxn)(ec_dev, status_msg);
> - trace_cros_ec_request_done(status_msg, ret);
> - if (ret == -EAGAIN)
> - continue;
> - if (ret < 0)
> - break;
> -
> - msg->result = status_msg->result;
> - if (status_msg->result != EC_RES_SUCCESS)
> - break;
> -
> - status = (struct ec_response_get_comms_status *)
> - status_msg->data;
> - if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> - break;
> - }
> + ret = cros_ec_xfer_command(ec_dev, msg);
> + if (ret == -EAGAIN)
> + continue;
> + if (ret < 0)
> + break;
With the command allocated on the stack, this can return immediately.
> +
> + *result = msg->result;
> + if (msg->result != EC_RES_SUCCESS)
> + break;
Again, this can return immediately if the command buffer is on the stack.
>
> - kfree(status_msg);
> + if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> + break;
Can return immediately.
> }
>
> + kfree(msg);
> + return ret;
What should this return on timeout ?
> +}
> +
> +static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> +{
> + int ret = cros_ec_xfer_command(ec_dev, msg);
> +
> + if (msg->result == EC_RES_IN_PROGRESS)
> + ret = cros_ec_wait_until_complete(ec_dev, &msg->result);
> +
> return ret;
> }
>
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
>
> While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> cros_ec_wait_until_complete() doesn't return an error code.
>
> Change the expectation to an error code.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto_test.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 64100fd81c6a..fbb872040711 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -1870,9 +1870,7 @@ static void cros_ec_proto_test_cmd_xfer_in_progress_retries_status_processing(st
> }
>
> ret = cros_ec_cmd_xfer(ec_dev, &msg);
> - KUNIT_EXPECT_EQ(test, ret, sizeof(struct ec_response_get_comms_status));
> -
> - KUNIT_EXPECT_EQ(test, msg.result, EC_RES_SUCCESS);
> + KUNIT_EXPECT_EQ(test, ret, -EAGAIN);
>
> /* For EC_CMD_GET_COMMS_STATUS EC_COMMAND_RETRIES times. */
> KUNIT_EXPECT_EQ(test, cros_kunit_ec_pkt_xfer_mock_called, 51);
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Wed, Jul 13, 2022 at 11:23:58AM -0700, Guenter Roeck wrote:
> On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > cros_ec_wait_until_complete() checks `msg->result` for
> > EC_CMD_GET_COMMS_STATUS. However, it doesn't return standard error codes
> > like most of others.
>
> The callers of cros_ec_send_command() do the mapping. I am not sure if
> it is a good idea to change that; it may have undesired side effects
> (such as changing the userspace ABI) for callers of
> cros_ec_send_command() not expecting this change. It would also result
> in double mapping in some situations.
Agreed. Let's drop the change.
On Wed, Jul 13, 2022 at 11:15:47AM -0700, Guenter Roeck wrote:
> On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> > -static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> > +static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *result)
> > {
> > - int ret = cros_ec_xfer_command(ec_dev, msg);
> > + struct cros_ec_command *msg;
> > + struct ec_response_get_comms_status *status;
> > + int ret = 0, i;
> > +
> > + msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
>
> AFAICS this is always 24 bytes. I would suggest to allocate it on the
> stack to reduce overhead.
Ack.
> > + ret = cros_ec_xfer_command(ec_dev, msg);
> > + if (ret == -EAGAIN)
> > + continue;
> > + if (ret < 0)
> > + break;
>
> With the command allocated on the stack, this can return immediately.
Nack, the function has no goto labels. `return ret` follows the loop
immediately. The `break` here doesn't make it to become too complicated.
I would prefer to keep it.
> > +
> > + *result = msg->result;
> > + if (msg->result != EC_RES_SUCCESS)
> > + break;
>
> Again, this can return immediately if the command buffer is on the stack.
Nack. See above.
> > - kfree(status_msg);
> > + if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> > + break;
>
> Can return immediately.
Nack. See above.
> > + kfree(msg);
> > + return ret;
>
> What should this return on timeout ?
It returns either:
* -EAGAIN, if cros_ec_xfer_command() returned -EAGAIN
* 0, if EC_COMMS_STATUS_PROCESSING flag was on
for EC_COMMAND_RETRIES times so far.
This is a "move" refactor. I would prefer to keep it as is and change the
behavior in later patch.
On Wed, Jul 13, 2022 at 11:18:32AM -0700, Guenter Roeck wrote:
> On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> > EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> > cros_ec_wait_until_complete() doesn't return an error code.
> >
> > Return -EAGAIN in the case instead.
>
> Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
> if it is still busy (stuck ?) with executing a command and it gets
> another one ?
AFAIK, most existing ECs use single task for host command[1][2]. As a
result, EC won't reply if it was busying on executing a host command.
Not sure if it would change after leveraging Zephyr (if enabling multi-core
support).
EC_CMD_GET_COMMS_STATUS is the only exception. EC executes the command in
interrupt context[3]. That's why AP can use EC_CMD_GET_COMMS_STATUS to query
the status while EC was busying on another host command.
I have no strong preference for the return code but tried to align to another
timeout case (when cros_ec_xfer_command() returned -EAGAIN for
EC_COMMAND_RETRIES times). Do we want to separate the cases: one for -EAGAIN
and another one for -ETIMEDOUT?
[1]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#428
[2]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/board/volteer/ec.tasklist#20
[3]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#176
On Wed, Jul 13, 2022 at 8:41 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Wed, Jul 13, 2022 at 11:18:32AM -0700, Guenter Roeck wrote:
> > On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> > >
> > > While EC_COMMS_STATUS_PROCESSING flag is still on after it tries
> > > EC_COMMAND_RETRIES times for sending EC_CMD_GET_COMMS_STATUS,
> > > cros_ec_wait_until_complete() doesn't return an error code.
> > >
> > > Return -EAGAIN in the case instead.
> >
> > Does this make sense, or should it be -ETIMEDOUT ? What does the EC do
> > if it is still busy (stuck ?) with executing a command and it gets
> > another one ?
>
> AFAIK, most existing ECs use single task for host command[1][2]. As a
> result, EC won't reply if it was busying on executing a host command.
> Not sure if it would change after leveraging Zephyr (if enabling multi-core
> support).
>
> EC_CMD_GET_COMMS_STATUS is the only exception. EC executes the command in
> interrupt context[3]. That's why AP can use EC_CMD_GET_COMMS_STATUS to query
> the status while EC was busying on another host command.
>
> I have no strong preference for the return code but tried to align to another
> timeout case (when cros_ec_xfer_command() returned -EAGAIN for
> EC_COMMAND_RETRIES times). Do we want to separate the cases: one for -EAGAIN
> and another one for -ETIMEDOUT?
If -EAGAIN is used elsewhere, let's stick with it.
Thanks,
Guenter
>
> [1]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#428
> [2]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/board/volteer/ec.tasklist#20
> [3]: https://crrev.com/4c0ae8814a68f2c2655ebb0b3b80ec4529d07cb3/common/host_command.c#176
On Wed, Jul 13, 2022 at 8:33 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Wed, Jul 13, 2022 at 11:15:47AM -0700, Guenter Roeck wrote:
> > On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> > > -static int cros_ec_send_command(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> > > +static int cros_ec_wait_until_complete(struct cros_ec_device *ec_dev, uint32_t *result)
> > > {
> > > - int ret = cros_ec_xfer_command(ec_dev, msg);
> > > + struct cros_ec_command *msg;
> > > + struct ec_response_get_comms_status *status;
> > > + int ret = 0, i;
> > > +
> > > + msg = kzalloc(sizeof(*msg) + sizeof(*status), GFP_KERNEL);
> > > + if (!msg)
> > > + return -ENOMEM;
> >
> > AFAICS this is always 24 bytes. I would suggest to allocate it on the
> > stack to reduce overhead.
>
> Ack.
>
> > > + ret = cros_ec_xfer_command(ec_dev, msg);
> > > + if (ret == -EAGAIN)
> > > + continue;
> > > + if (ret < 0)
> > > + break;
> >
> > With the command allocated on the stack, this can return immediately.
>
> Nack, the function has no goto labels. `return ret` follows the loop
> immediately. The `break` here doesn't make it to become too complicated.
> I would prefer to keep it.
Sorry, you lost me here. The code after the loop does
kfree(msg);
return ret;
If kfree() is no longer necessary, only the return statement is left. So break;
is identical to return ret;. Am I missing something ?
>
> > > +
> > > + *result = msg->result;
> > > + if (msg->result != EC_RES_SUCCESS)
> > > + break;
> >
> > Again, this can return immediately if the command buffer is on the stack.
>
> Nack. See above.
>
> > > - kfree(status_msg);
> > > + if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> > > + break;
> >
> > Can return immediately.
>
> Nack. See above.
>
Really, for those I think that
return 0;
would be better and more explicit.
> > > + kfree(msg);
> > > + return ret;
> >
> > What should this return on timeout ?
>
> It returns either:
> * -EAGAIN, if cros_ec_xfer_command() returned -EAGAIN
> * 0, if EC_COMMS_STATUS_PROCESSING flag was on
> for EC_COMMAND_RETRIES times so far.
>
> This is a "move" refactor. I would prefer to keep it as is and change the
> behavior in later patch.
Ok.
Thanks,
Guenter
On Thu, Jul 14, 2022 at 07:29:45AM -0700, Guenter Roeck wrote:
> On Wed, Jul 13, 2022 at 8:33 PM Tzung-Bi Shih <[email protected]> wrote:
> > On Wed, Jul 13, 2022 at 11:15:47AM -0700, Guenter Roeck wrote:
> > > On Mon, Jun 27, 2022 at 7:49 PM Tzung-Bi Shih <[email protected]> wrote:
> > > > + ret = cros_ec_xfer_command(ec_dev, msg);
> > > > + if (ret == -EAGAIN)
> > > > + continue;
> > > > + if (ret < 0)
> > > > + break;
> > >
> > > With the command allocated on the stack, this can return immediately.
> >
> > Nack, the function has no goto labels. `return ret` follows the loop
> > immediately. The `break` here doesn't make it to become too complicated.
> > I would prefer to keep it.
>
> Sorry, you lost me here. The code after the loop does
>
> kfree(msg);
> return ret;
>
> If kfree() is no longer necessary, only the return statement is left. So break;
> is identical to return ret;. Am I missing something ?
You are correct.
I meant personally I would prefer to use `break`:
* The loop is short so that it won't become too complicated.
* Keep the function has a single exit point.
But, anyway, let's use `return ret` to make it explicit.