The series adds Kunit tests, refactors, and clean-ups for cros_ec_query_all().
Tzung-Bi Shih (15):
platform/chrome: cros_ec_commands: fix compile errors
-> Fixes compile errors when including cros_ec_commands.h.
platform/chrome: cros_ec_proto: add Kunit tests for
cros_ec_query_all()
-> Adds Kunit tests for cros_ec_query_all(). They are baseline tests
for the following refactor patches.
platform/chrome: use macros for passthru indexes
platform/chrome: cros_ec_proto: assign buffer size from protocol info
-> Refactors.
platform/chrome: cros_ec_proto: remove redundant NULL check
-> Clean up.
platform/chrome: cros_ec_proto: use cros_ec_map_error()
-> Changes the internal return code.
platform/chrome: cros_ec_proto: separate cros_ec_get_proto_info()
platform/chrome: cros_ec_proto: handle empty payload in getting proto
info
platform/chrome: cros_ec_proto: separate
cros_ec_get_proto_info_legacy()
platform/chrome: cros_ec_proto: handle empty payload in getting info
legacy
-> Check if send_command() returns negative integer.
Call cros_ec_map_error() to see if msg->result isn't EC_RES_SUCCESS.
Report -EPROTO if send_command() returns 0.
platform/chrome: cros_ec: don't allocate `din` and `dout` in
cros_ec_register()
-> Clean up.
platform/chrome: use krealloc() for `din` and `dout`
-> Replace devm variant to non-devm.
platform/chrome: cros_ec_proto: don't show MKBP version if unsupported
-> Minor fix up.
platform/chrome: cros_ec_proto: return 0 on getting version mask
success
platform/chrome: cros_ec_proto: return 0 on getting wake mask success
-> Conform to kernel convention: return 0 on success;
otherwise, negative integers.
drivers/platform/chrome/Kconfig | 6 +
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec.c | 17 +-
drivers/platform/chrome/cros_ec_proto.c | 318 ++--
drivers/platform/chrome/cros_ec_proto_test.c | 1400 +++++++++++++++++
drivers/platform/chrome/cros_ec_trace.h | 8 +-
drivers/platform/chrome/cros_kunit_util.c | 98 ++
drivers/platform/chrome/cros_kunit_util.h | 36 +
.../linux/platform_data/cros_ec_commands.h | 4 +-
include/linux/platform_data/cros_ec_proto.h | 3 +
10 files changed, 1713 insertions(+), 178 deletions(-)
create mode 100644 drivers/platform/chrome/cros_kunit_util.c
create mode 100644 drivers/platform/chrome/cros_kunit_util.h
Changes from v1:
(https://patchwork.kernel.org/project/chrome-platform/cover/[email protected]/)
- Fix review comments.
- Split and reorder patches.
--
2.36.1.255.ge46751e96f-goog
Rename cros_ec_host_command_proto_query_v2() to
cros_ec_get_proto_info_legacy() and make it responsible for setting
`ec_dev` fields for EC protocol v2.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Preserve the "cros_ec_" prefix.
drivers/platform/chrome/cros_ec_proto.c | 72 +++++++++-----------
drivers/platform/chrome/cros_ec_proto_test.c | 22 +++---
2 files changed, 44 insertions(+), 50 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 6f5be9e5ede4..04b9704ed302 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -351,51 +351,57 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
return ret;
}
-static int cros_ec_host_command_proto_query_v2(struct cros_ec_device *ec_dev)
+static int cros_ec_get_proto_info_legacy(struct cros_ec_device *ec_dev)
{
struct cros_ec_command *msg;
- struct ec_params_hello *hello_params;
- struct ec_response_hello *hello_response;
+ struct ec_params_hello *params;
+ struct ec_response_hello *response;
int ret;
- int len = max(sizeof(*hello_params), sizeof(*hello_response));
- msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
+ ec_dev->proto_version = 2;
+
+ msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*response)), GFP_KERNEL);
if (!msg)
return -ENOMEM;
- msg->version = 0;
msg->command = EC_CMD_HELLO;
- hello_params = (struct ec_params_hello *)msg->data;
- msg->outsize = sizeof(*hello_params);
- hello_response = (struct ec_response_hello *)msg->data;
- msg->insize = sizeof(*hello_response);
+ msg->insize = sizeof(*response);
+ msg->outsize = sizeof(*params);
- hello_params->in_data = 0xa0b0c0d0;
+ params = (struct ec_params_hello *)msg->data;
+ params->in_data = 0xa0b0c0d0;
ret = send_command(ec_dev, msg);
-
if (ret < 0) {
- dev_dbg(ec_dev->dev,
- "EC failed to respond to v2 hello: %d\n",
- ret);
+ dev_dbg(ec_dev->dev, "EC failed to respond to v2 hello: %d\n", ret);
goto exit;
- } else if (msg->result != EC_RES_SUCCESS) {
- dev_err(ec_dev->dev,
- "EC responded to v2 hello with error: %d\n",
- msg->result);
- ret = msg->result;
+ }
+
+ ret = cros_ec_map_error(msg->result);
+ if (ret) {
+ dev_err(ec_dev->dev, "EC responded to v2 hello with error: %d\n", msg->result);
goto exit;
- } else if (hello_response->out_data != 0xa1b2c3d4) {
+ }
+
+ response = (struct ec_response_hello *)msg->data;
+ if (response->out_data != 0xa1b2c3d4) {
dev_err(ec_dev->dev,
"EC responded to v2 hello with bad result: %u\n",
- hello_response->out_data);
+ response->out_data);
ret = -EBADMSG;
goto exit;
}
- ret = 0;
+ ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
+ ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
+ ec_dev->max_passthru = 0;
+ ec_dev->pkt_xfer = NULL;
+ ec_dev->din_size = EC_PROTO2_MSG_BYTES;
+ ec_dev->dout_size = EC_PROTO2_MSG_BYTES;
- exit:
+ dev_dbg(ec_dev->dev, "falling back to proto v2\n");
+ ret = 0;
+exit:
kfree(msg);
return ret;
}
@@ -467,20 +473,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
cros_ec_get_proto_info(ec_dev, CROS_EC_DEV_PD_INDEX);
} else {
/* Try querying with a v2 hello message. */
- ec_dev->proto_version = 2;
- ret = cros_ec_host_command_proto_query_v2(ec_dev);
-
- if (ret == 0) {
- /* V2 hello succeeded. */
- dev_dbg(ec_dev->dev, "falling back to proto v2\n");
-
- ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
- ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
- ec_dev->max_passthru = 0;
- ec_dev->pkt_xfer = NULL;
- ec_dev->din_size = EC_PROTO2_MSG_BYTES;
- ec_dev->dout_size = EC_PROTO2_MSG_BYTES;
- } else {
+ ret = cros_ec_get_proto_info_legacy(ec_dev);
+ if (ret) {
/*
* It's possible for a test to occur too early when
* the EC isn't listening. If this happens, we'll
@@ -488,7 +482,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
*/
ec_dev->proto_version = EC_PROTO_VERSION_UNKNOWN;
dev_dbg(ec_dev->dev, "EC query failed: %d\n", ret);
- goto exit;
+ return ret;
}
}
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 473714964cf2..9f7d9666369f 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -484,7 +484,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
struct ec_response_hello *data;
@@ -511,7 +511,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
struct ec_params_hello *data;
@@ -549,7 +549,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return0(struct kunit *
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
struct ec_response_hello *data;
@@ -576,7 +576,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return0(struct kunit *
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
struct ec_params_hello *data;
@@ -614,7 +614,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
mock = cros_kunit_ec_xfer_mock_addx(test, -EIO, EC_RES_SUCCESS, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -637,7 +637,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -662,7 +662,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -670,7 +670,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
cros_ec_proto_test_query_all_pretest(test);
ret = cros_ec_query_all(ec_dev);
- KUNIT_EXPECT_EQ(test, ret, EC_RES_INVALID_COMMAND);
+ KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP);
KUNIT_EXPECT_EQ(test, ec_dev->proto_version, EC_PROTO_VERSION_UNKNOWN);
/* For cros_ec_get_proto_info() without passthru. */
@@ -685,7 +685,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -710,7 +710,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
struct ec_response_hello *data;
@@ -738,7 +738,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For cros_ec_get_proto_info_legacy(). */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
--
2.36.1.255.ge46751e96f-goog
Use cros_ec_map_error() in cros_ec_get_host_event_wake_mask().
The behavior of cros_ec_get_host_event_wake_mask() slightly changed. It
is acceptable because the caller only needs it returns negative integers
for indicating errors. Especially, the EC_RES_INVALID_COMMAND still
maps to -EOPNOTSUPP.
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Add R-b tag.
drivers/platform/chrome/cros_ec_proto.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 1b851dcd20c9..71ba6a56ad7c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -247,7 +247,7 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev,
uint32_t *mask)
{
struct ec_response_host_event_mask *r;
- int ret;
+ int ret, mapped;
msg->command = EC_CMD_HOST_EVENT_GET_WAKE_MASK;
msg->version = 0;
@@ -256,10 +256,9 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev,
ret = send_command(ec_dev, msg);
if (ret >= 0) {
- if (msg->result == EC_RES_INVALID_COMMAND)
- return -EOPNOTSUPP;
- if (msg->result != EC_RES_SUCCESS)
- return -EPROTO;
+ mapped = cros_ec_map_error(msg->result);
+ if (mapped)
+ return mapped;
}
if (ret > 0) {
r = (struct ec_response_host_event_mask *)msg->data;
--
2.36.1.255.ge46751e96f-goog
`din_size` is calculated from `ec_dev->max_response`.
`ec_dev->max_response` is further calculated from the protocol info.
To make it clear, assign `din_size` and `dout_size` from protocol info
directly.
Reviewed-by: Guenter Roeck <[email protected]>
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Add R-b tag.
drivers/platform/chrome/cros_ec_proto.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 65191af5139c..629dce3e6ab3 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -449,12 +449,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
"using proto v%u\n",
ec_dev->proto_version);
- ec_dev->din_size = ec_dev->max_response +
- sizeof(struct ec_host_response) +
- EC_MAX_RESPONSE_OVERHEAD;
- ec_dev->dout_size = ec_dev->max_request +
- sizeof(struct ec_host_request) +
- EC_MAX_REQUEST_OVERHEAD;
+ ec_dev->din_size = proto_info->max_response_packet_size + EC_MAX_RESPONSE_OVERHEAD;
+ ec_dev->dout_size = proto_info->max_request_packet_size + EC_MAX_REQUEST_OVERHEAD;
/*
* Check for PD
--
2.36.1.255.ge46751e96f-goog
Rename cros_ec_host_command_proto_query() to cros_ec_get_proto_info()
and make it responsible for setting `ec_dev` fields according to the
response protocol info.
Also make cros_ec_get_host_event_wake_mask() allocate its own message
buffer. It was lucky that size of `struct ec_response_host_event_mask`
is less than `struct ec_response_get_protocol_info`. Thus, the buffer
wasn't overflow.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Preserve the "cros_ec_" prefix.
drivers/platform/chrome/cros_ec_proto.c | 134 +++++++++----------
drivers/platform/chrome/cros_ec_proto_test.c | 56 ++++----
2 files changed, 93 insertions(+), 97 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 71ba6a56ad7c..893b76703da6 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -242,47 +242,53 @@ EXPORT_SYMBOL(cros_ec_check_result);
* the caller has ec_dev->lock mutex, or the caller knows there is
* no other command in progress.
*/
-static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev,
- struct cros_ec_command *msg,
- uint32_t *mask)
+static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint32_t *mask)
{
+ struct cros_ec_command *msg;
struct ec_response_host_event_mask *r;
int ret, mapped;
+ msg = kzalloc(sizeof(*msg) + sizeof(*r), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
msg->command = EC_CMD_HOST_EVENT_GET_WAKE_MASK;
- msg->version = 0;
- msg->outsize = 0;
msg->insize = sizeof(*r);
ret = send_command(ec_dev, msg);
if (ret >= 0) {
mapped = cros_ec_map_error(msg->result);
- if (mapped)
- return mapped;
+ if (mapped) {
+ ret = mapped;
+ goto exit;
+ }
}
if (ret > 0) {
r = (struct ec_response_host_event_mask *)msg->data;
*mask = r->mask;
}
+exit:
+ kfree(msg);
return ret;
}
-static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev,
- int devidx,
- struct cros_ec_command *msg)
+static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
{
- /*
- * Try using v3+ to query for supported protocols. If this
- * command fails, fall back to v2. Returns the highest protocol
- * supported by the EC.
- * Also sets the max request/response/passthru size.
- */
- int ret;
+ struct cros_ec_command *msg;
+ struct ec_response_get_protocol_info *info;
+ int ret, mapped;
+
+ ec_dev->proto_version = 3;
+ if (devidx > 0)
+ ec_dev->max_passthru = 0;
+
+ msg = kzalloc(sizeof(*msg) + sizeof(*info), GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
- memset(msg, 0, sizeof(*msg));
msg->command = EC_CMD_PASSTHRU_OFFSET(devidx) | EC_CMD_GET_PROTOCOL_INFO;
- msg->insize = sizeof(struct ec_response_get_protocol_info);
+ msg->insize = sizeof(*info);
ret = send_command(ec_dev, msg);
/*
@@ -299,15 +305,45 @@ static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev,
dev_dbg(ec_dev->dev,
"failed to check for EC[%d] protocol version: %d\n",
devidx, ret);
- return ret;
+ goto exit;
+ }
+
+ mapped = cros_ec_map_error(msg->result);
+ if (mapped) {
+ ret = mapped;
+ goto exit;
}
- if (devidx > 0 && msg->result == EC_RES_INVALID_COMMAND)
- return -ENODEV;
- else if (msg->result != EC_RES_SUCCESS)
- return msg->result;
+ info = (struct ec_response_get_protocol_info *)msg->data;
+
+ switch (devidx) {
+ case CROS_EC_DEV_EC_INDEX:
+ ec_dev->max_request = info->max_request_packet_size -
+ sizeof(struct ec_host_request);
+ ec_dev->max_response = info->max_response_packet_size -
+ sizeof(struct ec_host_response);
+ ec_dev->proto_version = min(EC_HOST_REQUEST_VERSION,
+ fls(info->protocol_versions) - 1);
+ ec_dev->din_size = info->max_response_packet_size + EC_MAX_RESPONSE_OVERHEAD;
+ ec_dev->dout_size = info->max_request_packet_size + EC_MAX_REQUEST_OVERHEAD;
+
+ dev_dbg(ec_dev->dev, "using proto v%u\n", ec_dev->proto_version);
+ break;
+ case CROS_EC_DEV_PD_INDEX:
+ ec_dev->max_passthru = info->max_request_packet_size -
+ sizeof(struct ec_host_request);
+
+ dev_dbg(ec_dev->dev, "found PD chip\n");
+ break;
+ default:
+ dev_dbg(ec_dev->dev, "unknwon passthru index: %d\n", devidx);
+ break;
+ }
- return 0;
+ ret = 0;
+exit:
+ kfree(msg);
+ return ret;
}
static int cros_ec_host_command_proto_query_v2(struct cros_ec_device *ec_dev)
@@ -417,51 +453,13 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
int cros_ec_query_all(struct cros_ec_device *ec_dev)
{
struct device *dev = ec_dev->dev;
- struct cros_ec_command *proto_msg;
- struct ec_response_get_protocol_info *proto_info;
u32 ver_mask = 0;
int ret;
- proto_msg = kzalloc(sizeof(*proto_msg) + sizeof(*proto_info),
- GFP_KERNEL);
- if (!proto_msg)
- return -ENOMEM;
-
/* First try sending with proto v3. */
- ec_dev->proto_version = 3;
- ret = cros_ec_host_command_proto_query(ec_dev, CROS_EC_DEV_EC_INDEX, proto_msg);
-
- if (ret == 0) {
- proto_info = (struct ec_response_get_protocol_info *)
- proto_msg->data;
- ec_dev->max_request = proto_info->max_request_packet_size -
- sizeof(struct ec_host_request);
- ec_dev->max_response = proto_info->max_response_packet_size -
- sizeof(struct ec_host_response);
- ec_dev->proto_version =
- min(EC_HOST_REQUEST_VERSION,
- fls(proto_info->protocol_versions) - 1);
- dev_dbg(ec_dev->dev,
- "using proto v%u\n",
- ec_dev->proto_version);
-
- ec_dev->din_size = proto_info->max_response_packet_size + EC_MAX_RESPONSE_OVERHEAD;
- ec_dev->dout_size = proto_info->max_request_packet_size + EC_MAX_REQUEST_OVERHEAD;
-
- /*
- * Check for PD
- */
- ret = cros_ec_host_command_proto_query(ec_dev, CROS_EC_DEV_PD_INDEX, proto_msg);
-
- if (ret) {
- dev_dbg(ec_dev->dev, "no PD chip found: %d\n", ret);
- ec_dev->max_passthru = 0;
- } else {
- dev_dbg(ec_dev->dev, "found PD chip\n");
- ec_dev->max_passthru =
- proto_info->max_request_packet_size -
- sizeof(struct ec_host_request);
- }
+ if (!cros_ec_get_proto_info(ec_dev, CROS_EC_DEV_EC_INDEX)) {
+ /* Check for PD. */
+ cros_ec_get_proto_info(ec_dev, CROS_EC_DEV_PD_INDEX);
} else {
/* Try querying with a v2 hello message. */
ec_dev->proto_version = 2;
@@ -524,8 +522,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));
/* Get host event wake mask. */
- ret = cros_ec_get_host_event_wake_mask(ec_dev, proto_msg,
- &ec_dev->host_event_wake_mask);
+ ret = cros_ec_get_host_event_wake_mask(ec_dev, &ec_dev->host_event_wake_mask);
if (ret < 0) {
/*
* If the EC doesn't support EC_CMD_HOST_EVENT_GET_WAKE_MASK,
@@ -556,7 +553,6 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ret = 0;
exit:
- kfree(proto_msg);
return ret;
}
EXPORT_SYMBOL(cros_ec_query_all);
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 628c6582ca78..14a4441a39fc 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -194,7 +194,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -207,7 +207,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -255,7 +255,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -273,7 +273,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
KUNIT_EXPECT_EQ(test, ec_dev->dout_size, 0xbe + EC_MAX_REQUEST_OVERHEAD);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -351,7 +351,7 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
/* Set some garbage bytes. */
ec_dev->max_passthru = 0xbf;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -367,7 +367,7 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -377,7 +377,7 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -389,7 +389,7 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -413,7 +413,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -434,7 +434,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -478,7 +478,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -495,7 +495,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, ret, -EIO);
KUNIT_EXPECT_EQ(test, ec_dev->proto_version, EC_PROTO_VERSION_UNKNOWN);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -526,7 +526,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -543,7 +543,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, ret, EC_RES_INVALID_COMMAND);
KUNIT_EXPECT_EQ(test, ec_dev->proto_version, EC_PROTO_VERSION_UNKNOWN);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -574,7 +574,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -596,7 +596,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
KUNIT_EXPECT_EQ(test, ret, -EBADMSG);
KUNIT_EXPECT_EQ(test, ec_dev->proto_version, EC_PROTO_VERSION_UNKNOWN);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -630,7 +630,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
/* Set some garbage bytes. */
ec_dev->mkbp_event_supported = 0xbf;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -646,7 +646,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -667,7 +667,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -679,7 +679,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -723,7 +723,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
/* Set some garbage bytes. */
ec_dev->host_sleep_v1 = true;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -739,7 +739,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -766,7 +766,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -778,7 +778,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -829,7 +829,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
/* Set some garbage bytes. */
ec_dev->host_event_wake_mask = U32_MAX;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -845,7 +845,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -873,7 +873,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
ret = cros_ec_query_all(ec_dev);
KUNIT_EXPECT_EQ(test, ret, 0);
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For cros_ec_get_proto_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -885,7 +885,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For cros_ec_get_proto_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
--
2.36.1.255.ge46751e96f-goog
cros_ec_get_proto_info() expects to receive
sizeof(struct ec_response_get_protocol_info) from send_command(). The
payload is valid only if the return value is positive.
Add Kunit tests for returning 0 in send_command() and handle the case in
cros_ec_get_proto_info().
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
No v1. New in the series.
drivers/platform/chrome/cros_ec_proto.c | 5 +
drivers/platform/chrome/cros_ec_proto_test.c | 132 +++++++++++++++++++
2 files changed, 137 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 893b76703da6..6f5be9e5ede4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -314,6 +314,11 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
goto exit;
}
+ if (ret == 0) {
+ ret = -EPROTO;
+ goto exit;
+ }
+
info = (struct ec_response_get_protocol_info *)msg->data;
switch (devidx) {
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 14a4441a39fc..473714964cf2 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -406,6 +406,71 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
}
}
+static void cros_ec_proto_test_query_all_no_pd_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;
+
+ /* Set some garbage bytes. */
+ ec_dev->max_passthru = 0xbf;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ struct ec_response_get_protocol_info *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /*
+ * Although it doesn't check the value, provides valid sizes so that
+ * cros_ec_query_all() allocates din and dout correctly.
+ */
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
+ EC_CMD_GET_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0);
+ }
+}
+
static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv = test->priv;
@@ -471,6 +536,71 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
}
}
+static void cros_ec_proto_test_query_all_legacy_normal_v3_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;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_host_command_proto_query_v2(). */
+ {
+ struct ec_response_hello *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_hello *)mock->o_data;
+ data->out_data = 0xa1b2c3d4;
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_host_command_proto_query_v2(). */
+ {
+ struct ec_params_hello *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, sizeof(struct ec_response_hello));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
+
+ data = (struct ec_params_hello *)mock->i_data;
+ KUNIT_EXPECT_EQ(test, data->in_data, 0xa0b0c0d0);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->proto_version, 2);
+ KUNIT_EXPECT_EQ(test, ec_dev->max_request, EC_PROTO2_MAX_PARAM_SIZE);
+ KUNIT_EXPECT_EQ(test, ec_dev->max_response, EC_PROTO2_MAX_PARAM_SIZE);
+ KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0);
+ KUNIT_EXPECT_PTR_EQ(test, ec_dev->pkt_xfer, NULL);
+ KUNIT_EXPECT_EQ(test, ec_dev->din_size, EC_PROTO2_MSG_BYTES);
+ KUNIT_EXPECT_EQ(test, ec_dev->dout_size, EC_PROTO2_MSG_BYTES);
+ }
+}
+
static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv = test->priv;
@@ -998,7 +1128,9 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_check_result),
KUNIT_CASE(cros_ec_proto_test_query_all_normal),
KUNIT_CASE(cros_ec_proto_test_query_all_no_pd_return_error),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_pd_return0),
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal_v3_return_error),
+ KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal_v3_return0),
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_xfer_error),
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_return_error),
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_data_error),
--
2.36.1.255.ge46751e96f-goog
Don't allocate `din` and `dout` in cros_ec_register() as they will be
allocated soon in cros_ec_query_all().
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
No v1. New in the series.
drivers/platform/chrome/cros_ec.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index e51a3f2176c7..29d3b544dafb 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -188,14 +188,8 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
ec_dev->max_passthru = 0;
ec_dev->ec = NULL;
ec_dev->pd = NULL;
-
- ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
- if (!ec_dev->din)
- return -ENOMEM;
-
- ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
- if (!ec_dev->dout)
- return -ENOMEM;
+ ec_dev->din = NULL;
+ ec_dev->dout = NULL;
mutex_init(&ec_dev->lock);
--
2.36.1.255.ge46751e96f-goog
On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_get_proto_info() expects to receive
> sizeof(struct ec_response_get_protocol_info) from send_command(). The
> payload is valid only if the return value is positive.
>
> Add Kunit tests for returning 0 in send_command() and handle the case in
> cros_ec_get_proto_info().
>
That should be two separate patches.
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> No v1. New in the series.
>
> drivers/platform/chrome/cros_ec_proto.c | 5 +
> drivers/platform/chrome/cros_ec_proto_test.c | 132 +++++++++++++++++++
> 2 files changed, 137 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 893b76703da6..6f5be9e5ede4 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -314,6 +314,11 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
> goto exit;
> }
>
> + if (ret == 0) {
> + ret = -EPROTO;
> + goto exit;
> + }
> +
I think you can move that into the if() statement above (which already
checks for ret >=0),
making it a special case of that situation.
Thanks,
Guenter
> info = (struct ec_response_get_protocol_info *)msg->data;
>
> switch (devidx) {
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 14a4441a39fc..473714964cf2 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -406,6 +406,71 @@ static void cros_ec_proto_test_query_all_no_pd_return_error(struct kunit *test)
> }
> }
>
> +static void cros_ec_proto_test_query_all_no_pd_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;
> +
> + /* Set some garbage bytes. */
> + ec_dev->max_passthru = 0xbf;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + struct ec_response_get_protocol_info *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /*
> + * Although it doesn't check the value, provides valid sizes so that
> + * cros_ec_query_all() allocates din and dout correctly.
> + */
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
> + EC_CMD_GET_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0);
> + }
> +}
> +
> static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv = test->priv;
> @@ -471,6 +536,71 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
> }
> }
>
> +static void cros_ec_proto_test_query_all_legacy_normal_v3_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;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_host_command_proto_query_v2(). */
> + {
> + struct ec_response_hello *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + data = (struct ec_response_hello *)mock->o_data;
> + data->out_data = 0xa1b2c3d4;
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_host_command_proto_query_v2(). */
> + {
> + struct ec_params_hello *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, sizeof(struct ec_response_hello));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
> +
> + data = (struct ec_params_hello *)mock->i_data;
> + KUNIT_EXPECT_EQ(test, data->in_data, 0xa0b0c0d0);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->proto_version, 2);
> + KUNIT_EXPECT_EQ(test, ec_dev->max_request, EC_PROTO2_MAX_PARAM_SIZE);
> + KUNIT_EXPECT_EQ(test, ec_dev->max_response, EC_PROTO2_MAX_PARAM_SIZE);
> + KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0);
> + KUNIT_EXPECT_PTR_EQ(test, ec_dev->pkt_xfer, NULL);
> + KUNIT_EXPECT_EQ(test, ec_dev->din_size, EC_PROTO2_MSG_BYTES);
> + KUNIT_EXPECT_EQ(test, ec_dev->dout_size, EC_PROTO2_MSG_BYTES);
> + }
> +}
> +
> static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv = test->priv;
> @@ -998,7 +1128,9 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
> KUNIT_CASE(cros_ec_proto_test_check_result),
> KUNIT_CASE(cros_ec_proto_test_query_all_normal),
> KUNIT_CASE(cros_ec_proto_test_query_all_no_pd_return_error),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_pd_return0),
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal_v3_return_error),
> + KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal_v3_return0),
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_xfer_error),
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_return_error),
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_data_error),
> --
> 2.36.1.255.ge46751e96f-goog
>
cros_ec_get_host_command_version_mask() used to return value from
send_command() which is number of bytes for input payload on success
(i.e. sizeof(struct ec_response_get_cmd_versions)).
However, the callers don't need to know how many bytes are available.
- Fix cros_ec_get_host_command_version_mask() to return 0 on success;
negative integers on error.
- Remove the unneeded `ver_mask` initialization as the callers should
take it only if cros_ec_get_host_command_version_mask() returns 0.
- Add a Kunit test: `ver_mask` has some garbage bytes from previous
EC_CMD_GET_NEXT_EVENT but there is no host sleep to make sure the
caller checks the return values correctly.
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Return 0 on success; otherwise, negative intergers.
drivers/platform/chrome/cros_ec_proto.c | 37 ++-
drivers/platform/chrome/cros_ec_proto_test.c | 286 +++++++++++++++++++
2 files changed, 308 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index ee15a73eee38..9d96ed16244f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -428,13 +428,12 @@ static int cros_ec_get_proto_info_legacy(struct cros_ec_device *ec_dev)
* the caller has ec_dev->lock mutex or the caller knows there is
* no other command in progress.
*/
-static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
- u16 cmd, u32 *mask)
+static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev, u16 cmd, u32 *mask)
{
struct ec_params_get_cmd_versions *pver;
struct ec_response_get_cmd_versions *rver;
struct cros_ec_command *msg;
- int ret;
+ int ret, mapped;
msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)),
GFP_KERNEL);
@@ -450,13 +449,25 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
pver->cmd = cmd;
ret = send_command(ec_dev, msg);
- if (ret > 0) {
- rver = (struct ec_response_get_cmd_versions *)msg->data;
- *mask = rver->version_mask;
+ if (ret < 0)
+ goto exit;
+
+ mapped = cros_ec_map_error(msg->result);
+ if (mapped) {
+ ret = mapped;
+ goto exit;
}
- kfree(msg);
+ if (ret == 0) {
+ ret = -EPROTO;
+ goto exit;
+ }
+ rver = (struct ec_response_get_cmd_versions *)msg->data;
+ *mask = rver->version_mask;
+ ret = 0;
+exit:
+ kfree(msg);
return ret;
}
@@ -469,7 +480,7 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
*/
int cros_ec_query_all(struct cros_ec_device *ec_dev)
{
- u32 ver_mask = 0;
+ u32 ver_mask;
int ret;
u8 *din, *dout;
@@ -503,9 +514,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
ec_dev->dout = dout;
/* Probe if MKBP event is supported */
- ret = cros_ec_get_host_command_version_mask(ec_dev,
- EC_CMD_GET_NEXT_EVENT,
- &ver_mask);
+ ret = cros_ec_get_host_command_version_mask(ec_dev, EC_CMD_GET_NEXT_EVENT, &ver_mask);
if (ret < 0 || ver_mask == 0) {
ec_dev->mkbp_event_supported = 0;
} else {
@@ -515,10 +524,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
}
/* Probe if host sleep v1 is supported for S0ix failure detection. */
- ret = cros_ec_get_host_command_version_mask(ec_dev,
- EC_CMD_HOST_SLEEP_EVENT,
- &ver_mask);
- ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));
+ ret = cros_ec_get_host_command_version_mask(ec_dev, EC_CMD_HOST_SLEEP_EVENT, &ver_mask);
+ ec_dev->host_sleep_v1 = (ret == 0 && (ver_mask & EC_VER_MASK(1)));
/* Get host event wake mask. */
ret = cros_ec_get_host_event_wake_mask(ec_dev, &ec_dev->host_event_wake_mask);
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 27b81a5a9880..af69410f2978 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -890,6 +890,182 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
}
}
+static void cros_ec_proto_test_query_all_no_mkbp_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;
+
+ /* Set some garbage bytes. */
+ ec_dev->mkbp_event_supported = 0xbf;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ struct ec_response_get_protocol_info *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /*
+ * Although it doesn't check the value, provides valid sizes so that
+ * cros_ec_query_all() allocates din and dout correctly.
+ */
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
+ EC_CMD_GET_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ struct ec_params_get_cmd_versions *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_GET_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
+
+ data = (struct ec_params_get_cmd_versions *)mock->i_data;
+ KUNIT_EXPECT_EQ(test, data->cmd, EC_CMD_GET_NEXT_EVENT);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 0);
+ }
+}
+
+static void cros_ec_proto_test_query_all_no_mkbp_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;
+
+ /* Set some garbage bytes. */
+ ec_dev->mkbp_event_supported = 0xbf;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ struct ec_response_get_protocol_info *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /*
+ * Although it doesn't check the value, provides valid sizes so that
+ * cros_ec_query_all() allocates din and dout correctly.
+ */
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
+ EC_CMD_GET_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ struct ec_params_get_cmd_versions *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_GET_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
+
+ data = (struct ec_params_get_cmd_versions *)mock->i_data;
+ KUNIT_EXPECT_EQ(test, data->cmd, EC_CMD_GET_NEXT_EVENT);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 0);
+ }
+}
+
static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv = test->priv;
@@ -996,6 +1172,113 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
}
}
+static void cros_ec_proto_test_query_all_no_host_sleep_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;
+
+ /* Set some garbage bytes. */
+ ec_dev->host_sleep_v1 = true;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ struct ec_response_get_protocol_info *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /*
+ * Although it doesn't check the value, provides valid sizes so that
+ * cros_ec_query_all() allocates din and dout correctly.
+ */
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ struct ec_response_get_cmd_versions *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /* In order to pollute next cros_ec_get_host_command_version_mask(). */
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = 0xbeef;
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
+ EC_CMD_GET_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ 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_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ 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_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
+
+ KUNIT_EXPECT_FALSE(test, ec_dev->host_sleep_v1);
+ }
+}
+
static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv = test->priv;
@@ -1183,7 +1466,10 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_data_error),
KUNIT_CASE(cros_ec_proto_test_query_all_legacy_return0),
KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp_return_error),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp_return0),
KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
+ 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),
{}
};
--
2.36.1.255.ge46751e96f-goog
On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
>
> Rename cros_ec_host_command_proto_query_v2() to
> cros_ec_get_proto_info_legacy() and make it responsible for setting
> `ec_dev` fields for EC protocol v2.
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> Changes from v1:
> - Preserve the "cros_ec_" prefix.
>
> drivers/platform/chrome/cros_ec_proto.c | 72 +++++++++-----------
> drivers/platform/chrome/cros_ec_proto_test.c | 22 +++---
> 2 files changed, 44 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 6f5be9e5ede4..04b9704ed302 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -351,51 +351,57 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
> return ret;
> }
>
> -static int cros_ec_host_command_proto_query_v2(struct cros_ec_device *ec_dev)
> +static int cros_ec_get_proto_info_legacy(struct cros_ec_device *ec_dev)
> {
> struct cros_ec_command *msg;
> - struct ec_params_hello *hello_params;
> - struct ec_response_hello *hello_response;
> + struct ec_params_hello *params;
> + struct ec_response_hello *response;
> int ret;
> - int len = max(sizeof(*hello_params), sizeof(*hello_response));
>
> - msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> + ec_dev->proto_version = 2;
> +
> + msg = kzalloc(sizeof(*msg) + max(sizeof(*params), sizeof(*response)), GFP_KERNEL);
> if (!msg)
> return -ENOMEM;
>
> - msg->version = 0;
> msg->command = EC_CMD_HELLO;
> - hello_params = (struct ec_params_hello *)msg->data;
> - msg->outsize = sizeof(*hello_params);
> - hello_response = (struct ec_response_hello *)msg->data;
> - msg->insize = sizeof(*hello_response);
> + msg->insize = sizeof(*response);
> + msg->outsize = sizeof(*params);
>
> - hello_params->in_data = 0xa0b0c0d0;
> + params = (struct ec_params_hello *)msg->data;
> + params->in_data = 0xa0b0c0d0;
>
> ret = send_command(ec_dev, msg);
> -
> if (ret < 0) {
> - dev_dbg(ec_dev->dev,
> - "EC failed to respond to v2 hello: %d\n",
> - ret);
> + dev_dbg(ec_dev->dev, "EC failed to respond to v2 hello: %d\n", ret);
> goto exit;
> - } else if (msg->result != EC_RES_SUCCESS) {
> - dev_err(ec_dev->dev,
> - "EC responded to v2 hello with error: %d\n",
> - msg->result);
> - ret = msg->result;
> + }
> +
> + ret = cros_ec_map_error(msg->result);
> + if (ret) {
> + dev_err(ec_dev->dev, "EC responded to v2 hello with error: %d\n", msg->result);
> goto exit;
> - } else if (hello_response->out_data != 0xa1b2c3d4) {
> + }
> +
> + response = (struct ec_response_hello *)msg->data;
> + if (response->out_data != 0xa1b2c3d4) {
> dev_err(ec_dev->dev,
> "EC responded to v2 hello with bad result: %u\n",
> - hello_response->out_data);
> + response->out_data);
> ret = -EBADMSG;
> goto exit;
> }
>
> - ret = 0;
> + ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
> + ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
> + ec_dev->max_passthru = 0;
> + ec_dev->pkt_xfer = NULL;
> + ec_dev->din_size = EC_PROTO2_MSG_BYTES;
> + ec_dev->dout_size = EC_PROTO2_MSG_BYTES;
>
> - exit:
> + dev_dbg(ec_dev->dev, "falling back to proto v2\n");
> + ret = 0;
> +exit:
> kfree(msg);
> return ret;
> }
> @@ -467,20 +473,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> cros_ec_get_proto_info(ec_dev, CROS_EC_DEV_PD_INDEX);
> } else {
> /* Try querying with a v2 hello message. */
> - ec_dev->proto_version = 2;
> - ret = cros_ec_host_command_proto_query_v2(ec_dev);
> -
> - if (ret == 0) {
> - /* V2 hello succeeded. */
> - dev_dbg(ec_dev->dev, "falling back to proto v2\n");
> -
> - ec_dev->max_request = EC_PROTO2_MAX_PARAM_SIZE;
> - ec_dev->max_response = EC_PROTO2_MAX_PARAM_SIZE;
> - ec_dev->max_passthru = 0;
> - ec_dev->pkt_xfer = NULL;
> - ec_dev->din_size = EC_PROTO2_MSG_BYTES;
> - ec_dev->dout_size = EC_PROTO2_MSG_BYTES;
> - } else {
> + ret = cros_ec_get_proto_info_legacy(ec_dev);
> + if (ret) {
> /*
> * It's possible for a test to occur too early when
> * the EC isn't listening. If this happens, we'll
> @@ -488,7 +482,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> */
> ec_dev->proto_version = EC_PROTO_VERSION_UNKNOWN;
> dev_dbg(ec_dev->dev, "EC query failed: %d\n", ret);
> - goto exit;
> + return ret;
> }
> }
>
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 473714964cf2..9f7d9666369f 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -484,7 +484,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> struct ec_response_hello *data;
>
> @@ -511,7 +511,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return_error(struct ku
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> struct ec_params_hello *data;
>
> @@ -549,7 +549,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return0(struct kunit *
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> struct ec_response_hello *data;
>
> @@ -576,7 +576,7 @@ static void cros_ec_proto_test_query_all_legacy_normal_v3_return0(struct kunit *
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> struct ec_params_hello *data;
>
> @@ -614,7 +614,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, -EIO, EC_RES_SUCCESS, 0);
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> @@ -637,7 +637,7 @@ static void cros_ec_proto_test_query_all_legacy_xfer_error(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -662,7 +662,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> @@ -670,7 +670,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
>
> cros_ec_proto_test_query_all_pretest(test);
> ret = cros_ec_query_all(ec_dev);
> - KUNIT_EXPECT_EQ(test, ret, EC_RES_INVALID_COMMAND);
> + KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP);
> KUNIT_EXPECT_EQ(test, ec_dev->proto_version, EC_PROTO_VERSION_UNKNOWN);
>
> /* For cros_ec_get_proto_info() without passthru. */
> @@ -685,7 +685,7 @@ static void cros_ec_proto_test_query_all_legacy_return_error(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -710,7 +710,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> struct ec_response_hello *data;
>
> @@ -738,7 +738,7 @@ static void cros_ec_proto_test_query_all_legacy_data_error(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For cros_ec_get_proto_info_legacy(). */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> --
> 2.36.1.255.ge46751e96f-goog
>
cros_ec_get_host_event_wake_mask() used to return value from
send_command() which is number of bytes for input payload on success
(i.e. sizeof(struct ec_response_host_event_mask)).
However, the callers don't need to know how many bytes are available.
- Fix cros_ec_get_host_event_wake_mask() to return 0 on success;
negative integers on error.
- Add a Kunit test for guarding if send_command() returns 0 in
get_host_event_wake_mask().
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v1:
- Return 0 on success; otherwise, negative intergers.
drivers/platform/chrome/cros_ec_proto.c | 23 ++--
drivers/platform/chrome/cros_ec_proto_test.c | 128 +++++++++++++++++++
2 files changed, 142 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 9d96ed16244f..04c852aa790b 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -256,18 +256,23 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint3
msg->insize = sizeof(*r);
ret = send_command(ec_dev, msg);
- if (ret >= 0) {
- mapped = cros_ec_map_error(msg->result);
- if (mapped) {
- ret = mapped;
- goto exit;
- }
+ if (ret < 0)
+ goto exit;
+
+ mapped = cros_ec_map_error(msg->result);
+ if (mapped) {
+ ret = mapped;
+ goto exit;
}
- if (ret > 0) {
- r = (struct ec_response_host_event_mask *)msg->data;
- *mask = r->mask;
+
+ if (ret == 0) {
+ ret = -EPROTO;
+ goto exit;
}
+ r = (struct ec_response_host_event_mask *)msg->data;
+ *mask = r->mask;
+ ret = 0;
exit:
kfree(msg);
return ret;
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index af69410f2978..3ee0de337d53 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -1406,6 +1406,133 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
}
}
+static void cros_ec_proto_test_query_all_default_wake_mask_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;
+
+ /* Set some garbage bytes. */
+ ec_dev->host_event_wake_mask = U32_MAX;
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ struct ec_response_get_protocol_info *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ /*
+ * Although it doesn't check the value, provides valid sizes so that
+ * cros_ec_query_all() allocates din and dout correctly.
+ */
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For get_host_event_wake_mask(). */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test, 0);
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_get_proto_info() without passthru. */
+ {
+ 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_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_proto_info() with passthru. */
+ {
+ 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
+ EC_CMD_GET_PROTOCOL_INFO);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ {
+ 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_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ 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_CMD_VERSIONS);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
+ }
+
+ /* For get_host_event_wake_mask(). */
+ {
+ u32 mask;
+
+ 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_HOST_EVENT_GET_WAKE_MASK);
+ KUNIT_EXPECT_EQ(test, mock->msg.insize, sizeof(struct ec_response_host_event_mask));
+ KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
+
+ mask = ec_dev->host_event_wake_mask;
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_LOW), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU), 0);
+ KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_STATUS), 0);
+ }
+}
+
static void cros_ec_proto_test_release(struct device *dev)
{
}
@@ -1471,6 +1598,7 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
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),
{}
};
--
2.36.1.255.ge46751e96f-goog
On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_get_host_command_version_mask() used to return value from
> send_command() which is number of bytes for input payload on success
> (i.e. sizeof(struct ec_response_get_cmd_versions)).
>
> However, the callers don't need to know how many bytes are available.
>
> - Fix cros_ec_get_host_command_version_mask() to return 0 on success;
> negative integers on error.
>
> - Remove the unneeded `ver_mask` initialization as the callers should
> take it only if cros_ec_get_host_command_version_mask() returns 0.
>
> - Add a Kunit test: `ver_mask` has some garbage bytes from previous
> EC_CMD_GET_NEXT_EVENT but there is no host sleep to make sure the
> caller checks the return values correctly.
>
This should be separate patches.
Thanks,
Guenter
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> Changes from v1:
> - Return 0 on success; otherwise, negative intergers.
>
> drivers/platform/chrome/cros_ec_proto.c | 37 ++-
> drivers/platform/chrome/cros_ec_proto_test.c | 286 +++++++++++++++++++
> 2 files changed, 308 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index ee15a73eee38..9d96ed16244f 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -428,13 +428,12 @@ static int cros_ec_get_proto_info_legacy(struct cros_ec_device *ec_dev)
> * the caller has ec_dev->lock mutex or the caller knows there is
> * no other command in progress.
> */
> -static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
> - u16 cmd, u32 *mask)
> +static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev, u16 cmd, u32 *mask)
> {
> struct ec_params_get_cmd_versions *pver;
> struct ec_response_get_cmd_versions *rver;
> struct cros_ec_command *msg;
> - int ret;
> + int ret, mapped;
>
> msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)),
> GFP_KERNEL);
> @@ -450,13 +449,25 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
> pver->cmd = cmd;
>
> ret = send_command(ec_dev, msg);
> - if (ret > 0) {
> - rver = (struct ec_response_get_cmd_versions *)msg->data;
> - *mask = rver->version_mask;
> + if (ret < 0)
> + goto exit;
> +
> + mapped = cros_ec_map_error(msg->result);
> + if (mapped) {
> + ret = mapped;
> + goto exit;
> }
>
> - kfree(msg);
> + if (ret == 0) {
> + ret = -EPROTO;
> + goto exit;
> + }
>
> + rver = (struct ec_response_get_cmd_versions *)msg->data;
> + *mask = rver->version_mask;
> + ret = 0;
> +exit:
> + kfree(msg);
> return ret;
> }
>
> @@ -469,7 +480,7 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
> */
> int cros_ec_query_all(struct cros_ec_device *ec_dev)
> {
> - u32 ver_mask = 0;
> + u32 ver_mask;
> int ret;
> u8 *din, *dout;
>
> @@ -503,9 +514,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> ec_dev->dout = dout;
>
> /* Probe if MKBP event is supported */
> - ret = cros_ec_get_host_command_version_mask(ec_dev,
> - EC_CMD_GET_NEXT_EVENT,
> - &ver_mask);
> + ret = cros_ec_get_host_command_version_mask(ec_dev, EC_CMD_GET_NEXT_EVENT, &ver_mask);
> if (ret < 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> } else {
> @@ -515,10 +524,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> }
>
> /* Probe if host sleep v1 is supported for S0ix failure detection. */
> - ret = cros_ec_get_host_command_version_mask(ec_dev,
> - EC_CMD_HOST_SLEEP_EVENT,
> - &ver_mask);
> - ec_dev->host_sleep_v1 = (ret >= 0 && (ver_mask & EC_VER_MASK(1)));
> + ret = cros_ec_get_host_command_version_mask(ec_dev, EC_CMD_HOST_SLEEP_EVENT, &ver_mask);
> + ec_dev->host_sleep_v1 = (ret == 0 && (ver_mask & EC_VER_MASK(1)));
>
> /* Get host event wake mask. */
> ret = cros_ec_get_host_event_wake_mask(ec_dev, &ec_dev->host_event_wake_mask);
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 27b81a5a9880..af69410f2978 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -890,6 +890,182 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
> }
> }
>
> +static void cros_ec_proto_test_query_all_no_mkbp_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;
> +
> + /* Set some garbage bytes. */
> + ec_dev->mkbp_event_supported = 0xbf;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + struct ec_response_get_protocol_info *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /*
> + * Although it doesn't check the value, provides valid sizes so that
> + * cros_ec_query_all() allocates din and dout correctly.
> + */
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
> + EC_CMD_GET_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + struct ec_params_get_cmd_versions *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_GET_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
> +
> + data = (struct ec_params_get_cmd_versions *)mock->i_data;
> + KUNIT_EXPECT_EQ(test, data->cmd, EC_CMD_GET_NEXT_EVENT);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 0);
> + }
> +}
> +
> +static void cros_ec_proto_test_query_all_no_mkbp_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;
> +
> + /* Set some garbage bytes. */
> + ec_dev->mkbp_event_supported = 0xbf;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + struct ec_response_get_protocol_info *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /*
> + * Although it doesn't check the value, provides valid sizes so that
> + * cros_ec_query_all() allocates din and dout correctly.
> + */
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
> + EC_CMD_GET_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + struct ec_params_get_cmd_versions *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_GET_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(*data));
> +
> + data = (struct ec_params_get_cmd_versions *)mock->i_data;
> + KUNIT_EXPECT_EQ(test, data->cmd, EC_CMD_GET_NEXT_EVENT);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 0);
> + }
> +}
> +
> static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv = test->priv;
> @@ -996,6 +1172,113 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> }
> }
>
> +static void cros_ec_proto_test_query_all_no_host_sleep_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;
> +
> + /* Set some garbage bytes. */
> + ec_dev->host_sleep_v1 = true;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + struct ec_response_get_protocol_info *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /*
> + * Although it doesn't check the value, provides valid sizes so that
> + * cros_ec_query_all() allocates din and dout correctly.
> + */
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + struct ec_response_get_cmd_versions *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /* In order to pollute next cros_ec_get_host_command_version_mask(). */
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = 0xbeef;
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
> + EC_CMD_GET_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + 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_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + 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_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
> +
> + KUNIT_EXPECT_FALSE(test, ec_dev->host_sleep_v1);
> + }
> +}
> +
> static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv = test->priv;
> @@ -1183,7 +1466,10 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_data_error),
> KUNIT_CASE(cros_ec_proto_test_query_all_legacy_return0),
> KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp_return_error),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp_return0),
> KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
> + 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),
> {}
> };
> --
> 2.36.1.255.ge46751e96f-goog
>
On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
>
> cros_ec_get_host_event_wake_mask() used to return value from
> send_command() which is number of bytes for input payload on success
> (i.e. sizeof(struct ec_response_host_event_mask)).
>
> However, the callers don't need to know how many bytes are available.
>
> - Fix cros_ec_get_host_event_wake_mask() to return 0 on success;
> negative integers on error.
>
> - Add a Kunit test for guarding if send_command() returns 0 in
> get_host_event_wake_mask().
>
Please split into two patches.
Thanks,
Guenter
> Signed-off-by: Tzung-Bi Shih <[email protected]>
> ---
> Changes from v1:
> - Return 0 on success; otherwise, negative intergers.
>
> drivers/platform/chrome/cros_ec_proto.c | 23 ++--
> drivers/platform/chrome/cros_ec_proto_test.c | 128 +++++++++++++++++++
> 2 files changed, 142 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 9d96ed16244f..04c852aa790b 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -256,18 +256,23 @@ static int cros_ec_get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint3
> msg->insize = sizeof(*r);
>
> ret = send_command(ec_dev, msg);
> - if (ret >= 0) {
> - mapped = cros_ec_map_error(msg->result);
> - if (mapped) {
> - ret = mapped;
> - goto exit;
> - }
> + if (ret < 0)
> + goto exit;
> +
> + mapped = cros_ec_map_error(msg->result);
> + if (mapped) {
> + ret = mapped;
> + goto exit;
> }
> - if (ret > 0) {
> - r = (struct ec_response_host_event_mask *)msg->data;
> - *mask = r->mask;
> +
> + if (ret == 0) {
> + ret = -EPROTO;
> + goto exit;
> }
>
> + r = (struct ec_response_host_event_mask *)msg->data;
> + *mask = r->mask;
> + ret = 0;
> exit:
> kfree(msg);
> return ret;
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index af69410f2978..3ee0de337d53 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -1406,6 +1406,133 @@ static void cros_ec_proto_test_query_all_default_wake_mask_return_error(struct k
> }
> }
>
> +static void cros_ec_proto_test_query_all_default_wake_mask_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;
> +
> + /* Set some garbage bytes. */
> + ec_dev->host_event_wake_mask = U32_MAX;
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + struct ec_response_get_protocol_info *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + /*
> + * Although it doesn't check the value, provides valid sizes so that
> + * cros_ec_query_all() allocates din and dout correctly.
> + */
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For get_host_event_wake_mask(). */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test, 0);
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_get_proto_info() without passthru. */
> + {
> + 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_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_proto_info() with passthru. */
> + {
> + 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_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX) |
> + EC_CMD_GET_PROTOCOL_INFO);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + {
> + 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_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + 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_CMD_VERSIONS);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
> + }
> +
> + /* For get_host_event_wake_mask(). */
> + {
> + u32 mask;
> +
> + 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_HOST_EVENT_GET_WAKE_MASK);
> + KUNIT_EXPECT_EQ(test, mock->msg.insize, sizeof(struct ec_response_host_event_mask));
> + KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> +
> + mask = ec_dev->host_event_wake_mask;
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_LOW), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_CRITICAL), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU), 0);
> + KUNIT_EXPECT_EQ(test, mask & EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_STATUS), 0);
> + }
> +}
> +
> static void cros_ec_proto_test_release(struct device *dev)
> {
> }
> @@ -1471,6 +1598,7 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
> KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
> 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),
> {}
> };
>
> --
> 2.36.1.255.ge46751e96f-goog
>
On Tue, Jun 07, 2022 at 11:47:56AM -0700, Guenter Roeck wrote:
> On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
> >
> > cros_ec_get_proto_info() expects to receive
> > sizeof(struct ec_response_get_protocol_info) from send_command(). The
> > payload is valid only if the return value is positive.
> >
> > Add Kunit tests for returning 0 in send_command() and handle the case in
> > cros_ec_get_proto_info().
> >
> That should be two separate patches.
Ack, will separate them in next version. I put them together because I wrote
Kunit test first to make sure the second half takes effect (somehow TDD).
Could I still put the Kunit patch first (even if it introduces Kunit test
failure)?
>
> > Signed-off-by: Tzung-Bi Shih <[email protected]>
> > ---
> > No v1. New in the series.
> >
> > drivers/platform/chrome/cros_ec_proto.c | 5 +
> > drivers/platform/chrome/cros_ec_proto_test.c | 132 +++++++++++++++++++
> > 2 files changed, 137 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 893b76703da6..6f5be9e5ede4 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -314,6 +314,11 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
> > goto exit;
> > }
> >
> > + if (ret == 0) {
> > + ret = -EPROTO;
> > + goto exit;
> > + }
> > +
>
> I think you can move that into the if() statement above (which already
> checks for ret >=0),
> making it a special case of that situation.
Nope, there is no "ret >= 0" (you could be confusing with
cros_ec_get_host_event_wake_mask()).
The result flow roughly like:
ret = send_command(...);
if (ret < 0)
goto exit;
mapped = cros_ec_map_error(...);
if (mapped) {
...
goto exit;
}
if (ret == 0) {
...
goto exit;
}
On Tue, Jun 07, 2022 at 12:11:23PM -0700, Guenter Roeck wrote:
> On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
> >
> > cros_ec_get_host_command_version_mask() used to return value from
> > send_command() which is number of bytes for input payload on success
> > (i.e. sizeof(struct ec_response_get_cmd_versions)).
> >
> > However, the callers don't need to know how many bytes are available.
> >
> > - Fix cros_ec_get_host_command_version_mask() to return 0 on success;
> > negative integers on error.
> >
> > - Remove the unneeded `ver_mask` initialization as the callers should
> > take it only if cros_ec_get_host_command_version_mask() returns 0.
> >
> > - Add a Kunit test: `ver_mask` has some garbage bytes from previous
> > EC_CMD_GET_NEXT_EVENT but there is no host sleep to make sure the
> > caller checks the return values correctly.
> >
> This should be separate patches.
Ack. Will fix in next version.
On Tue, Jun 07, 2022 at 12:12:25PM -0700, Guenter Roeck wrote:
> On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
> >
> > cros_ec_get_host_event_wake_mask() used to return value from
> > send_command() which is number of bytes for input payload on success
> > (i.e. sizeof(struct ec_response_host_event_mask)).
> >
> > However, the callers don't need to know how many bytes are available.
> >
> > - Fix cros_ec_get_host_event_wake_mask() to return 0 on success;
> > negative integers on error.
> >
> > - Add a Kunit test for guarding if send_command() returns 0 in
> > get_host_event_wake_mask().
> >
> Please split into two patches.
Ack.
On Tue, Jun 7, 2022 at 7:17 PM Tzung-Bi Shih <[email protected]> wrote:
>
> On Tue, Jun 07, 2022 at 11:47:56AM -0700, Guenter Roeck wrote:
> > On Tue, Jun 7, 2022 at 7:57 AM Tzung-Bi Shih <[email protected]> wrote:
> > >
> > > cros_ec_get_proto_info() expects to receive
> > > sizeof(struct ec_response_get_protocol_info) from send_command(). The
> > > payload is valid only if the return value is positive.
> > >
> > > Add Kunit tests for returning 0 in send_command() and handle the case in
> > > cros_ec_get_proto_info().
> > >
> > That should be two separate patches.
>
> Ack, will separate them in next version. I put them together because I wrote
> Kunit test first to make sure the second half takes effect (somehow TDD).
>
> Could I still put the Kunit patch first (even if it introduces Kunit test
> failure)?
>
Sorry, I am running behind with e-mails.
If you want to, but why not let the fix come first ? If the unit test
patch is first, mayle add a note after --- indicating that it is
expected to fail and will be fixed with the next patch.
Thanks,
Guenter
> >
> > > Signed-off-by: Tzung-Bi Shih <[email protected]>
> > > ---
> > > No v1. New in the series.
> > >
> > > drivers/platform/chrome/cros_ec_proto.c | 5 +
> > > drivers/platform/chrome/cros_ec_proto_test.c | 132 +++++++++++++++++++
> > > 2 files changed, 137 insertions(+)
> > >
> > > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > > index 893b76703da6..6f5be9e5ede4 100644
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -314,6 +314,11 @@ static int cros_ec_get_proto_info(struct cros_ec_device *ec_dev, int devidx)
> > > goto exit;
> > > }
> > >
> > > + if (ret == 0) {
> > > + ret = -EPROTO;
> > > + goto exit;
> > > + }
> > > +
> >
> > I think you can move that into the if() statement above (which already
> > checks for ret >=0),
> > making it a special case of that situation.
>
> Nope, there is no "ret >= 0" (you could be confusing with
> cros_ec_get_host_event_wake_mask()).
>
> The result flow roughly like:
>
> ret = send_command(...);
> if (ret < 0)
> goto exit;
>
> mapped = cros_ec_map_error(...);
> if (mapped) {
> ...
> goto exit;
> }
>
> if (ret == 0) {
> ...
> goto exit;
> }