* [PATCH 01/13] platform/chrome: cros_ec_commands: fix compile errors
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:08 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all() Tzung-Bi Shih
` (11 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
Fix compile errors when including cros_ec_commands.h solely.
1.
cros_ec_commands.h:587:9: error: unknown type name 'uint8_t'
587 | uint8_t flags;
| ^~~~~~~
2.
cros_ec_commands.h:1105:43: error: implicit declaration of function 'BIT'
1105 | EC_COMMS_STATUS_PROCESSING = BIT(0),
| ^~~
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
include/linux/platform_data/cros_ec_commands.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 8cfa8cfca77e..a5b749a85707 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -13,8 +13,8 @@
#ifndef __CROS_EC_COMMANDS_H
#define __CROS_EC_COMMANDS_H
-
-
+#include <linux/bits.h>
+#include <linux/types.h>
#define BUILD_ASSERT(_cond)
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 01/13] platform/chrome: cros_ec_commands: fix compile errors
2022-06-06 14:10 ` [PATCH 01/13] platform/chrome: cros_ec_commands: fix compile errors Tzung-Bi Shih
@ 2022-06-06 15:08 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:08 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Fix compile errors when including cros_ec_commands.h solely.
>
> 1.
> cros_ec_commands.h:587:9: error: unknown type name 'uint8_t'
> 587 | uint8_t flags;
> | ^~~~~~~
>
> 2.
> cros_ec_commands.h:1105:43: error: implicit declaration of function 'BIT'
> 1105 | EC_COMMS_STATUS_PROCESSING = BIT(0),
> | ^~~
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> include/linux/platform_data/cros_ec_commands.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 8cfa8cfca77e..a5b749a85707 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -13,8 +13,8 @@
> #ifndef __CROS_EC_COMMANDS_H
> #define __CROS_EC_COMMANDS_H
>
> -
> -
> +#include <linux/bits.h>
> +#include <linux/types.h>
>
> #define BUILD_ASSERT(_cond)
>
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
2022-06-06 14:10 ` [PATCH 01/13] platform/chrome: cros_ec_commands: fix compile errors Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:18 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 03/13] platform/chrome: use macros for passthru indexes Tzung-Bi Shih
` (10 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
cros_ec_query_all() sends multiple host commands to EC for querying
supported protocols and settings.
Add required mock for interacting with cros_ec_query_all() and Kunit
tests.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/Kconfig | 6 +
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_proto_test.c | 738 +++++++++++++++++++
drivers/platform/chrome/cros_kunit_util.c | 98 +++
drivers/platform/chrome/cros_kunit_util.h | 36 +
5 files changed, 879 insertions(+)
create mode 100644 drivers/platform/chrome/cros_kunit_util.c
create mode 100644 drivers/platform/chrome/cros_kunit_util.h
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 4b3d2427e8dd..0b069d874845 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -268,11 +268,17 @@ config CHROMEOS_PRIVACY_SCREEN
source "drivers/platform/chrome/wilco_ec/Kconfig"
# Kunit test cases
+config CROS_KUNIT
+ tristate
+ help
+ ChromeOS Kunit.
+
config CROS_EC_PROTO_KUNIT_TEST
tristate "Kunit tests for ChromeOS EC protocol" if !KUNIT_ALL_TESTS
depends on KUNIT && CROS_EC
default KUNIT_ALL_TESTS
select CROS_EC_PROTO
+ select CROS_KUNIT
help
Kunit tests for the ChromeOS Embedded Controller protocol.
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 3c380066c6b6..a06bc56d12a8 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_CROS_USBPD_NOTIFY) += cros_usbpd_notify.o
obj-$(CONFIG_WILCO_EC) += wilco_ec/
# Kunit test cases
+obj-$(CONFIG_CROS_KUNIT) += cros_kunit_util.o
obj-$(CONFIG_CROS_EC_PROTO_KUNIT_TEST) += cros_ec_proto_test.o
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 25c4fca5c165..c5e16566a634 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -9,6 +9,7 @@
#include <linux/platform_data/cros_ec_proto.h>
#include "cros_ec.h"
+#include "cros_kunit_util.h"
#define BUFSIZE 512
@@ -172,6 +173,719 @@ static void cros_ec_proto_test_check_result(struct kunit *test)
KUNIT_EXPECT_EQ(test, ret, -EAGAIN);
}
+static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+
+ /*
+ * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
+ * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
+ * ec_dev->dev.
+ */
+ ec_dev->din = NULL;
+ ec_dev->dout = NULL;
+}
+
+static void cros_ec_proto_test_query_all_normal(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+ struct ec_xfer_mock *mock;
+ int ret;
+
+ /* For cros_ec_host_command_proto_query() 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);
+
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->protocol_versions = BIT(3) | BIT(2);
+ data->max_request_packet_size = 0xbe;
+ data->max_response_packet_size = 0xef;
+ }
+
+ /* For cros_ec_host_command_proto_query() with 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);
+
+ data = (struct ec_response_get_protocol_info *)mock->o_data;
+ data->max_request_packet_size = 0xbf;
+ }
+
+ /* 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);
+
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = BIT(6) | BIT(5);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ struct ec_response_get_cmd_versions *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = BIT(1);
+ }
+
+ /* For cros_ec_get_host_event_wake_mask(). */
+ {
+ struct ec_response_host_event_mask *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_host_event_mask *)mock->o_data;
+ data->mask = 0xbeef;
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For cros_ec_host_command_proto_query() 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);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->max_request, 0xbe - sizeof(struct ec_host_request));
+ KUNIT_EXPECT_EQ(test, ec_dev->max_response, 0xef - sizeof(struct ec_host_response));
+ KUNIT_EXPECT_EQ(test, ec_dev->proto_version, 3);
+ KUNIT_EXPECT_EQ(test, ec_dev->din_size, 0xef + EC_MAX_RESPONSE_OVERHEAD);
+ KUNIT_EXPECT_EQ(test, ec_dev->dout_size, 0xbe + EC_MAX_REQUEST_OVERHEAD);
+ }
+
+ /* For cros_ec_host_command_proto_query() 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(1) | 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, 0xbf - sizeof(struct ec_host_request));
+ }
+
+ /* 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, 7);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ 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_HOST_SLEEP_EVENT);
+
+ KUNIT_EXPECT_TRUE(test, ec_dev->host_sleep_v1);
+ }
+
+ /* For cros_ec_get_host_event_wake_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);
+
+ KUNIT_EXPECT_EQ(test, ec_dev->host_event_wake_mask, 0xbeef);
+ }
+}
+
+static void cros_ec_proto_test_query_all_no_pd(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_host_command_proto_query() 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_host_command_proto_query() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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_host_command_proto_query() 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() 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(1) | 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(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_host_command_proto_query() without passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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_host_command_proto_query() 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_no_mkbp(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 fill_protocol_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 fill_protocol_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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);
+
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = 0;
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For fill_protocol_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 fill_protocol_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(1) | 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_mkbp2(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 fill_protocol_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 fill_protocol_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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 fill_protocol_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 fill_protocol_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(1) | 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;
+ 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 fill_protocol_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 fill_protocol_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ {
+ struct ec_response_get_cmd_versions *data;
+
+ mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = 0;
+ }
+
+ cros_ec_proto_test_query_all_pretest(test);
+ ret = cros_ec_query_all(ec_dev);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* For fill_protocol_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 fill_protocol_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(1) | 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(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_host_command_proto_query() 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_host_command_proto_query() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ 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,
+ sizeof(struct ec_response_get_cmd_versions));
+ 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,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For cros_ec_get_host_event_wake_mask(). */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_host_event_mask));
+ 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_host_command_proto_query() 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() 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(1) | 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 cros_ec_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 int cros_ec_proto_test_init(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv;
@@ -188,24 +902,48 @@ static int cros_ec_proto_test_init(struct kunit *test)
ec_dev->din = (u8 *)priv->din;
ec_dev->din_size = ARRAY_SIZE(priv->din);
ec_dev->proto_version = EC_HOST_REQUEST_VERSION;
+ ec_dev->dev = kunit_kzalloc(test, sizeof(*ec_dev->dev), GFP_KERNEL);
+ if (!ec_dev->dev)
+ return -ENOMEM;
+ device_initialize(ec_dev->dev);
+ ec_dev->cmd_xfer = cros_kunit_ec_xfer_mock;
+ ec_dev->pkt_xfer = cros_kunit_ec_xfer_mock;
priv->msg = (struct cros_ec_command *)priv->_msg;
+ cros_kunit_mock_reset();
+
return 0;
}
+static void cros_ec_proto_test_exit(struct kunit *test)
+{
+ struct cros_ec_proto_test_priv *priv = test->priv;
+ struct cros_ec_device *ec_dev = &priv->ec_dev;
+
+ put_device(ec_dev->dev);
+}
+
static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_prepare_tx_legacy_normal),
KUNIT_CASE(cros_ec_proto_test_prepare_tx_legacy_bad_msg_outsize),
KUNIT_CASE(cros_ec_proto_test_prepare_tx_normal),
KUNIT_CASE(cros_ec_proto_test_prepare_tx_bad_msg_outsize),
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),
+ KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp2),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
+ KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
{}
};
static struct kunit_suite cros_ec_proto_test_suite = {
.name = "cros_ec_proto_test",
.init = cros_ec_proto_test_init,
+ .exit = cros_ec_proto_test_exit,
.test_cases = cros_ec_proto_test_cases,
};
diff --git a/drivers/platform/chrome/cros_kunit_util.c b/drivers/platform/chrome/cros_kunit_util.c
new file mode 100644
index 000000000000..e031777dea87
--- /dev/null
+++ b/drivers/platform/chrome/cros_kunit_util.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CrOS Kunit tests utilities.
+ */
+
+#include <kunit/test.h>
+
+#include <linux/list.h>
+#include <linux/minmax.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+
+#include "cros_ec.h"
+#include "cros_kunit_util.h"
+
+int cros_kunit_ec_xfer_mock_default_ret;
+EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_default_ret);
+
+static struct list_head cros_kunit_ec_xfer_mock_in;
+static struct list_head cros_kunit_ec_xfer_mock_out;
+
+int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
+{
+ struct ec_xfer_mock *mock;
+
+ mock = list_first_entry_or_null(&cros_kunit_ec_xfer_mock_in, struct ec_xfer_mock, list);
+ if (!mock)
+ return cros_kunit_ec_xfer_mock_default_ret;
+
+ list_del(&mock->list);
+
+ memcpy(&mock->msg, msg, sizeof(*msg));
+ if (msg->outsize) {
+ mock->i_data = kunit_kzalloc(mock->test, msg->outsize, GFP_KERNEL);
+ if (mock->i_data)
+ memcpy(mock->i_data, msg->data, msg->outsize);
+ }
+
+ msg->result = mock->result;
+ if (msg->insize)
+ memcpy(msg->data, mock->o_data, min(msg->insize, mock->o_data_len));
+
+ list_add_tail(&mock->list, &cros_kunit_ec_xfer_mock_out);
+
+ return mock->ret;
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock);
+
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size)
+{
+ return cros_kunit_ec_xfer_mock_addx(test, size, EC_RES_SUCCESS, size);
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_add);
+
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_addx(struct kunit *test,
+ int ret, int result, size_t size)
+{
+ struct ec_xfer_mock *mock;
+
+ mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL);
+ if (!mock)
+ return NULL;
+
+ list_add_tail(&mock->list, &cros_kunit_ec_xfer_mock_in);
+ mock->test = test;
+
+ mock->ret = ret;
+ mock->result = result;
+ mock->o_data = kunit_kzalloc(test, size, GFP_KERNEL);
+ if (!mock->o_data)
+ return NULL;
+ mock->o_data_len = size;
+
+ return mock;
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_addx);
+
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_next(void)
+{
+ struct ec_xfer_mock *mock;
+
+ mock = list_first_entry_or_null(&cros_kunit_ec_xfer_mock_out, struct ec_xfer_mock, list);
+ if (mock)
+ list_del(&mock->list);
+
+ return mock;
+}
+EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_next);
+
+void cros_kunit_mock_reset(void)
+{
+ cros_kunit_ec_xfer_mock_default_ret = 0;
+ INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_in);
+ INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_out);
+}
+EXPORT_SYMBOL_GPL(cros_kunit_mock_reset);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/chrome/cros_kunit_util.h b/drivers/platform/chrome/cros_kunit_util.h
new file mode 100644
index 000000000000..79c4525f873c
--- /dev/null
+++ b/drivers/platform/chrome/cros_kunit_util.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * CrOS Kunit tests utilities.
+ */
+
+#ifndef _CROS_KUNIT_UTIL_H_
+#define _CROS_KUNIT_UTIL_H_
+
+#include <linux/platform_data/cros_ec_proto.h>
+
+struct ec_xfer_mock {
+ struct list_head list;
+ struct kunit *test;
+
+ /* input */
+ struct cros_ec_command msg;
+ void *i_data;
+
+ /* output */
+ int ret;
+ int result;
+ void *o_data;
+ u32 o_data_len;
+};
+
+extern int cros_kunit_ec_xfer_mock_default_ret;
+
+int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg);
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size);
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_addx(struct kunit *test,
+ int ret, int result, size_t size);
+struct ec_xfer_mock *cros_kunit_ec_xfer_mock_next(void);
+
+void cros_kunit_mock_reset(void);
+
+#endif
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all()
2022-06-06 14:10 ` [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all() Tzung-Bi Shih
@ 2022-06-06 15:18 ` Guenter Roeck
2022-06-07 0:54 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:18 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> cros_ec_query_all() sends multiple host commands to EC for querying
> supported protocols and settings.
>
> Add required mock for interacting with cros_ec_query_all() and Kunit
> tests.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/Kconfig | 6 +
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_proto_test.c | 738 +++++++++++++++++++
> drivers/platform/chrome/cros_kunit_util.c | 98 +++
> drivers/platform/chrome/cros_kunit_util.h | 36 +
> 5 files changed, 879 insertions(+)
> create mode 100644 drivers/platform/chrome/cros_kunit_util.c
> create mode 100644 drivers/platform/chrome/cros_kunit_util.h
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 4b3d2427e8dd..0b069d874845 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -268,11 +268,17 @@ config CHROMEOS_PRIVACY_SCREEN
> source "drivers/platform/chrome/wilco_ec/Kconfig"
>
> # Kunit test cases
> +config CROS_KUNIT
> + tristate
> + help
> + ChromeOS Kunit.
> +
> config CROS_EC_PROTO_KUNIT_TEST
> tristate "Kunit tests for ChromeOS EC protocol" if !KUNIT_ALL_TESTS
> depends on KUNIT && CROS_EC
> default KUNIT_ALL_TESTS
> select CROS_EC_PROTO
> + select CROS_KUNIT
> help
> Kunit tests for the ChromeOS Embedded Controller protocol.
>
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 3c380066c6b6..a06bc56d12a8 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_CROS_USBPD_NOTIFY) += cros_usbpd_notify.o
> obj-$(CONFIG_WILCO_EC) += wilco_ec/
>
> # Kunit test cases
> +obj-$(CONFIG_CROS_KUNIT) += cros_kunit_util.o
> obj-$(CONFIG_CROS_EC_PROTO_KUNIT_TEST) += cros_ec_proto_test.o
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 25c4fca5c165..c5e16566a634 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -9,6 +9,7 @@
> #include <linux/platform_data/cros_ec_proto.h>
>
> #include "cros_ec.h"
> +#include "cros_kunit_util.h"
>
> #define BUFSIZE 512
>
> @@ -172,6 +173,719 @@ static void cros_ec_proto_test_check_result(struct kunit *test)
> KUNIT_EXPECT_EQ(test, ret, -EAGAIN);
> }
>
> +static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
> +{
> + struct cros_ec_proto_test_priv *priv = test->priv;
> + struct cros_ec_device *ec_dev = &priv->ec_dev;
> +
> + /*
> + * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
> + * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
> + * ec_dev->dev.
> + */
> + ec_dev->din = NULL;
> + ec_dev->dout = NULL;
> +}
> +
> +static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> +{
> + struct cros_ec_proto_test_priv *priv = test->priv;
> + struct cros_ec_device *ec_dev = &priv->ec_dev;
> + struct ec_xfer_mock *mock;
> + int ret;
> +
> + /* For cros_ec_host_command_proto_query() 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);
> +
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->protocol_versions = BIT(3) | BIT(2);
> + data->max_request_packet_size = 0xbe;
> + data->max_response_packet_size = 0xef;
> + }
> +
> + /* For cros_ec_host_command_proto_query() with 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);
> +
> + data = (struct ec_response_get_protocol_info *)mock->o_data;
> + data->max_request_packet_size = 0xbf;
> + }
> +
> + /* 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);
> +
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = BIT(6) | BIT(5);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + struct ec_response_get_cmd_versions *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = BIT(1);
> + }
> +
> + /* For cros_ec_get_host_event_wake_mask(). */
> + {
> + struct ec_response_host_event_mask *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + data = (struct ec_response_host_event_mask *)mock->o_data;
> + data->mask = 0xbeef;
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
Wouldn't it be better to implement a post_test function and have it
call devm_kfree() if it is really necessary to release ->din and
->dout here ?
Either case, I am not convinced that clearing / releasing din and dout
is really needed. The device pointer should not change, after all, and
either the next call to cros_ec_query_all() will release the pointers,
or unloading the driver will do it.
Thanks,
Guenter
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For cros_ec_host_command_proto_query() 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);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->max_request, 0xbe - sizeof(struct ec_host_request));
> + KUNIT_EXPECT_EQ(test, ec_dev->max_response, 0xef - sizeof(struct ec_host_response));
> + KUNIT_EXPECT_EQ(test, ec_dev->proto_version, 3);
> + KUNIT_EXPECT_EQ(test, ec_dev->din_size, 0xef + EC_MAX_RESPONSE_OVERHEAD);
> + KUNIT_EXPECT_EQ(test, ec_dev->dout_size, 0xbe + EC_MAX_REQUEST_OVERHEAD);
> + }
> +
> + /* For cros_ec_host_command_proto_query() 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(1) | 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, 0xbf - sizeof(struct ec_host_request));
> + }
> +
> + /* 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, 7);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + 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_HOST_SLEEP_EVENT);
> +
> + KUNIT_EXPECT_TRUE(test, ec_dev->host_sleep_v1);
> + }
> +
> + /* For cros_ec_get_host_event_wake_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);
> +
> + KUNIT_EXPECT_EQ(test, ec_dev->host_event_wake_mask, 0xbeef);
> + }
> +}
> +
> +static void cros_ec_proto_test_query_all_no_pd(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_host_command_proto_query() 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_host_command_proto_query() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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_host_command_proto_query() 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() 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(1) | 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(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_host_command_proto_query() without passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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_host_command_proto_query() 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_no_mkbp(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 fill_protocol_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 fill_protocol_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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);
> +
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = 0;
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For fill_protocol_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 fill_protocol_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(1) | 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_mkbp2(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 fill_protocol_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 fill_protocol_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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 fill_protocol_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 fill_protocol_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(1) | 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;
> + 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 fill_protocol_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 fill_protocol_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + {
> + struct ec_response_get_cmd_versions *data;
> +
> + mock = cros_kunit_ec_xfer_mock_add(test, sizeof(*data));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> +
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = 0;
> + }
> +
> + cros_ec_proto_test_query_all_pretest(test);
> + ret = cros_ec_query_all(ec_dev);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* For fill_protocol_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 fill_protocol_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(1) | 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(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_host_command_proto_query() 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_host_command_proto_query() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + 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,
> + sizeof(struct ec_response_get_cmd_versions));
> + 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,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For cros_ec_get_host_event_wake_mask(). */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_host_event_mask));
> + 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_host_command_proto_query() 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() 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(1) | 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 cros_ec_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 int cros_ec_proto_test_init(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv;
> @@ -188,24 +902,48 @@ static int cros_ec_proto_test_init(struct kunit *test)
> ec_dev->din = (u8 *)priv->din;
> ec_dev->din_size = ARRAY_SIZE(priv->din);
> ec_dev->proto_version = EC_HOST_REQUEST_VERSION;
> + ec_dev->dev = kunit_kzalloc(test, sizeof(*ec_dev->dev), GFP_KERNEL);
> + if (!ec_dev->dev)
> + return -ENOMEM;
> + device_initialize(ec_dev->dev);
> + ec_dev->cmd_xfer = cros_kunit_ec_xfer_mock;
> + ec_dev->pkt_xfer = cros_kunit_ec_xfer_mock;
>
> priv->msg = (struct cros_ec_command *)priv->_msg;
>
> + cros_kunit_mock_reset();
> +
> return 0;
> }
>
> +static void cros_ec_proto_test_exit(struct kunit *test)
> +{
> + struct cros_ec_proto_test_priv *priv = test->priv;
> + struct cros_ec_device *ec_dev = &priv->ec_dev;
> +
> + put_device(ec_dev->dev);
> +}
> +
> static struct kunit_case cros_ec_proto_test_cases[] = {
> KUNIT_CASE(cros_ec_proto_test_prepare_tx_legacy_normal),
> KUNIT_CASE(cros_ec_proto_test_prepare_tx_legacy_bad_msg_outsize),
> KUNIT_CASE(cros_ec_proto_test_prepare_tx_normal),
> KUNIT_CASE(cros_ec_proto_test_prepare_tx_bad_msg_outsize),
> 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),
> + KUNIT_CASE(cros_ec_proto_test_query_all_legacy_normal),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp2),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
> + KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
> {}
> };
>
> static struct kunit_suite cros_ec_proto_test_suite = {
> .name = "cros_ec_proto_test",
> .init = cros_ec_proto_test_init,
> + .exit = cros_ec_proto_test_exit,
> .test_cases = cros_ec_proto_test_cases,
> };
>
> diff --git a/drivers/platform/chrome/cros_kunit_util.c b/drivers/platform/chrome/cros_kunit_util.c
> new file mode 100644
> index 000000000000..e031777dea87
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_kunit_util.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CrOS Kunit tests utilities.
> + */
> +
> +#include <kunit/test.h>
> +
> +#include <linux/list.h>
> +#include <linux/minmax.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +
> +#include "cros_ec.h"
> +#include "cros_kunit_util.h"
> +
> +int cros_kunit_ec_xfer_mock_default_ret;
> +EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_default_ret);
> +
> +static struct list_head cros_kunit_ec_xfer_mock_in;
> +static struct list_head cros_kunit_ec_xfer_mock_out;
> +
> +int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> +{
> + struct ec_xfer_mock *mock;
> +
> + mock = list_first_entry_or_null(&cros_kunit_ec_xfer_mock_in, struct ec_xfer_mock, list);
> + if (!mock)
> + return cros_kunit_ec_xfer_mock_default_ret;
> +
> + list_del(&mock->list);
> +
> + memcpy(&mock->msg, msg, sizeof(*msg));
> + if (msg->outsize) {
> + mock->i_data = kunit_kzalloc(mock->test, msg->outsize, GFP_KERNEL);
> + if (mock->i_data)
> + memcpy(mock->i_data, msg->data, msg->outsize);
> + }
> +
> + msg->result = mock->result;
> + if (msg->insize)
> + memcpy(msg->data, mock->o_data, min(msg->insize, mock->o_data_len));
> +
> + list_add_tail(&mock->list, &cros_kunit_ec_xfer_mock_out);
> +
> + return mock->ret;
> +}
> +EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock);
> +
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size)
> +{
> + return cros_kunit_ec_xfer_mock_addx(test, size, EC_RES_SUCCESS, size);
> +}
> +EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_add);
> +
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_addx(struct kunit *test,
> + int ret, int result, size_t size)
> +{
> + struct ec_xfer_mock *mock;
> +
> + mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL);
> + if (!mock)
> + return NULL;
> +
> + list_add_tail(&mock->list, &cros_kunit_ec_xfer_mock_in);
> + mock->test = test;
> +
> + mock->ret = ret;
> + mock->result = result;
> + mock->o_data = kunit_kzalloc(test, size, GFP_KERNEL);
> + if (!mock->o_data)
> + return NULL;
> + mock->o_data_len = size;
> +
> + return mock;
> +}
> +EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_addx);
> +
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_next(void)
> +{
> + struct ec_xfer_mock *mock;
> +
> + mock = list_first_entry_or_null(&cros_kunit_ec_xfer_mock_out, struct ec_xfer_mock, list);
> + if (mock)
> + list_del(&mock->list);
> +
> + return mock;
> +}
> +EXPORT_SYMBOL_GPL(cros_kunit_ec_xfer_mock_next);
> +
> +void cros_kunit_mock_reset(void)
> +{
> + cros_kunit_ec_xfer_mock_default_ret = 0;
> + INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_in);
> + INIT_LIST_HEAD(&cros_kunit_ec_xfer_mock_out);
> +}
> +EXPORT_SYMBOL_GPL(cros_kunit_mock_reset);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/chrome/cros_kunit_util.h b/drivers/platform/chrome/cros_kunit_util.h
> new file mode 100644
> index 000000000000..79c4525f873c
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_kunit_util.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * CrOS Kunit tests utilities.
> + */
> +
> +#ifndef _CROS_KUNIT_UTIL_H_
> +#define _CROS_KUNIT_UTIL_H_
> +
> +#include <linux/platform_data/cros_ec_proto.h>
> +
> +struct ec_xfer_mock {
> + struct list_head list;
> + struct kunit *test;
> +
> + /* input */
> + struct cros_ec_command msg;
> + void *i_data;
> +
> + /* output */
> + int ret;
> + int result;
> + void *o_data;
> + u32 o_data_len;
> +};
> +
> +extern int cros_kunit_ec_xfer_mock_default_ret;
> +
> +int cros_kunit_ec_xfer_mock(struct cros_ec_device *ec_dev, struct cros_ec_command *msg);
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_add(struct kunit *test, size_t size);
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_addx(struct kunit *test,
> + int ret, int result, size_t size);
> +struct ec_xfer_mock *cros_kunit_ec_xfer_mock_next(void);
> +
> +void cros_kunit_mock_reset(void);
> +
> +#endif
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all()
2022-06-06 15:18 ` Guenter Roeck
@ 2022-06-07 0:54 ` Tzung-Bi Shih
2022-06-07 14:41 ` Guenter Roeck
0 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:54 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 08:18:41AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > +static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
> > +{
> > + struct cros_ec_proto_test_priv *priv = test->priv;
> > + struct cros_ec_device *ec_dev = &priv->ec_dev;
> > +
> > + /*
> > + * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
> > + * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
> > + * ec_dev->dev.
> > + */
> > + ec_dev->din = NULL;
> > + ec_dev->dout = NULL;
> > +}
> > +
> > +static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> > +{
[...]
> > + cros_ec_proto_test_query_all_pretest(test);
> > + ret = cros_ec_query_all(ec_dev);
>
> Wouldn't it be better to implement a post_test function and have it
> call devm_kfree() if it is really necessary to release ->din and
> ->dout here ?
>
> Either case, I am not convinced that clearing / releasing din and dout
> is really needed. The device pointer should not change, after all, and
> either the next call to cros_ec_query_all() will release the pointers,
> or unloading the driver will do it.
The `din` and `dout` are not managed by `ec_dev->dev` but statically
initializing in cros_ec_proto_test_init()(see below).
cros_ec_proto_test_query_all_pretest() sets them to NULL to get rid of the
following warning (as devres_destroy() in devm_kfree() returns -ENOENT):
WARNING: CPU: 0 PID: 27 at drivers/base/devres.c:1058 devm_kfree
Oops, I just realized qemu still generated yet another warning:
Device '(null)' does not have a release() function, it is broken and ...
Will fix it in next version.
[...]
> > static int cros_ec_proto_test_init(struct kunit *test)
> > {
> > struct cros_ec_proto_test_priv *priv;
> > @@ -188,24 +902,48 @@ static int cros_ec_proto_test_init(struct kunit *test)
> > ec_dev->din = (u8 *)priv->din;
> > ec_dev->din_size = ARRAY_SIZE(priv->din);
> > ec_dev->proto_version = EC_HOST_REQUEST_VERSION;
> > + ec_dev->dev = kunit_kzalloc(test, sizeof(*ec_dev->dev), GFP_KERNEL);
> > + if (!ec_dev->dev)
> > + return -ENOMEM;
> > + device_initialize(ec_dev->dev);
> > + ec_dev->cmd_xfer = cros_kunit_ec_xfer_mock;
> > + ec_dev->pkt_xfer = cros_kunit_ec_xfer_mock;
> >
> > priv->msg = (struct cros_ec_command *)priv->_msg;
> >
> > + cros_kunit_mock_reset();
> > +
> > return 0;
> > }
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all()
2022-06-07 0:54 ` Tzung-Bi Shih
@ 2022-06-07 14:41 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-07 14:41 UTC (permalink / raw)
To: Tzung-Bi Shih
Cc: Benson Leung, Guenter Roeck,
open list:CHROME HARDWARE PLATFORM SUPPORT, linux-kernel
On Mon, Jun 6, 2022 at 5:54 PM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Mon, Jun 06, 2022 at 08:18:41AM -0700, Guenter Roeck wrote:
> > On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > > +static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
> > > +{
> > > + struct cros_ec_proto_test_priv *priv = test->priv;
> > > + struct cros_ec_device *ec_dev = &priv->ec_dev;
> > > +
> > > + /*
> > > + * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
> > > + * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
> > > + * ec_dev->dev.
> > > + */
> > > + ec_dev->din = NULL;
> > > + ec_dev->dout = NULL;
> > > +}
> > > +
> > > +static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> > > +{
> [...]
> > > + cros_ec_proto_test_query_all_pretest(test);
> > > + ret = cros_ec_query_all(ec_dev);
> >
> > Wouldn't it be better to implement a post_test function and have it
> > call devm_kfree() if it is really necessary to release ->din and
> > ->dout here ?
> >
> > Either case, I am not convinced that clearing / releasing din and dout
> > is really needed. The device pointer should not change, after all, and
> > either the next call to cros_ec_query_all() will release the pointers,
> > or unloading the driver will do it.
>
> The `din` and `dout` are not managed by `ec_dev->dev` but statically
> initializing in cros_ec_proto_test_init()(see below).
>
Good point. Still I think it would be better to have a _post function
and clear ->din and ->dout after the call instead of clearing it
before the next call.
Thanks,
Guenter
> cros_ec_proto_test_query_all_pretest() sets them to NULL to get rid of the
> following warning (as devres_destroy() in devm_kfree() returns -ENOENT):
> WARNING: CPU: 0 PID: 27 at drivers/base/devres.c:1058 devm_kfree
>
> Oops, I just realized qemu still generated yet another warning:
> Device '(null)' does not have a release() function, it is broken and ...
> Will fix it in next version.
>
> [...]
> > > static int cros_ec_proto_test_init(struct kunit *test)
> > > {
> > > struct cros_ec_proto_test_priv *priv;
> > > @@ -188,24 +902,48 @@ static int cros_ec_proto_test_init(struct kunit *test)
> > > ec_dev->din = (u8 *)priv->din;
> > > ec_dev->din_size = ARRAY_SIZE(priv->din);
> > > ec_dev->proto_version = EC_HOST_REQUEST_VERSION;
> > > + ec_dev->dev = kunit_kzalloc(test, sizeof(*ec_dev->dev), GFP_KERNEL);
> > > + if (!ec_dev->dev)
> > > + return -ENOMEM;
> > > + device_initialize(ec_dev->dev);
> > > + ec_dev->cmd_xfer = cros_kunit_ec_xfer_mock;
> > > + ec_dev->pkt_xfer = cros_kunit_ec_xfer_mock;
> > >
> > > priv->msg = (struct cros_ec_command *)priv->_msg;
> > >
> > > + cros_kunit_mock_reset();
> > > +
> > > return 0;
> > > }
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 03/13] platform/chrome: use macros for passthru indexes
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
2022-06-06 14:10 ` [PATCH 01/13] platform/chrome: cros_ec_commands: fix compile errors Tzung-Bi Shih
2022-06-06 14:10 ` [PATCH 02/13] platform/chrome: cros_ec_proto: add Kunit tests for cros_ec_query_all() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:22 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 04/13] platform/chrome: cros_ec_proto: assign buffer size from protocol info Tzung-Bi Shih
` (9 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
Move passthru indexes for EC and PD devices to common header. Also use
them instead of literal constants.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec.c | 3 ---
drivers/platform/chrome/cros_ec_proto.c | 6 +++---
drivers/platform/chrome/cros_ec_proto_test.c | 18 ++++++++++++------
drivers/platform/chrome/cros_ec_trace.h | 8 ++++----
include/linux/platform_data/cros_ec_proto.h | 3 +++
5 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index b3e94cdf7d1a..e51a3f2176c7 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -19,9 +19,6 @@
#include "cros_ec.h"
-#define CROS_EC_DEV_EC_INDEX 0
-#define CROS_EC_DEV_PD_INDEX 1
-
static struct cros_ec_platform ec_p = {
.ec_name = CROS_EC_DEV_NAME,
.cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_EC_INDEX),
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 13ced9d2dd71..65191af5139c 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -433,7 +433,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
/* First try sending with proto v3. */
ec_dev->proto_version = 3;
- ret = cros_ec_host_command_proto_query(ec_dev, 0, proto_msg);
+ 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 *)
@@ -459,7 +459,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
/*
* Check for PD
*/
- ret = cros_ec_host_command_proto_query(ec_dev, 1, proto_msg);
+ 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);
@@ -609,7 +609,7 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
msg->insize = ec_dev->max_response;
}
- if (msg->command < EC_CMD_PASSTHRU_OFFSET(1)) {
+ if (msg->command < EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX)) {
if (msg->outsize > ec_dev->max_request) {
dev_err(ec_dev->dev,
"request of size %u is too big (max: %u)\n",
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index c5e16566a634..5169bf33360b 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -280,7 +280,8 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
@@ -393,7 +394,8 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
@@ -535,7 +537,8 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
@@ -623,7 +626,8 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
@@ -723,7 +727,8 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
@@ -833,7 +838,8 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
KUNIT_EXPECT_EQ(test, mock->msg.command,
- EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
+ 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);
diff --git a/drivers/platform/chrome/cros_ec_trace.h b/drivers/platform/chrome/cros_ec_trace.h
index 9bb5cd2c98b8..d7e407de88df 100644
--- a/drivers/platform/chrome/cros_ec_trace.h
+++ b/drivers/platform/chrome/cros_ec_trace.h
@@ -30,8 +30,8 @@ TRACE_EVENT(cros_ec_request_start,
),
TP_fast_assign(
__entry->version = cmd->version;
- __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
- __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
+ __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
+ __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
__entry->outsize = cmd->outsize;
__entry->insize = cmd->insize;
),
@@ -55,8 +55,8 @@ TRACE_EVENT(cros_ec_request_done,
),
TP_fast_assign(
__entry->version = cmd->version;
- __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
- __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
+ __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
+ __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
__entry->outsize = cmd->outsize;
__entry->insize = cmd->insize;
__entry->result = cmd->result;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 138fd912c808..6475a8066f00 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -21,6 +21,9 @@
#define CROS_EC_DEV_SCP_NAME "cros_scp"
#define CROS_EC_DEV_TP_NAME "cros_tp"
+#define CROS_EC_DEV_EC_INDEX 0
+#define CROS_EC_DEV_PD_INDEX 1
+
/*
* The EC is unresponsive for a time after a reboot command. Add a
* simple delay to make sure that the bus stays locked.
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 03/13] platform/chrome: use macros for passthru indexes
2022-06-06 14:10 ` [PATCH 03/13] platform/chrome: use macros for passthru indexes Tzung-Bi Shih
@ 2022-06-06 15:22 ` Guenter Roeck
2022-06-06 15:23 ` Guenter Roeck
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:22 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Move passthru indexes for EC and PD devices to common header. Also use
> them instead of literal constants.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/platform/chrome/cros_ec.c | 3 ---
> drivers/platform/chrome/cros_ec_proto.c | 6 +++---
> drivers/platform/chrome/cros_ec_proto_test.c | 18 ++++++++++++------
> drivers/platform/chrome/cros_ec_trace.h | 8 ++++----
> include/linux/platform_data/cros_ec_proto.h | 3 +++
> 5 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index b3e94cdf7d1a..e51a3f2176c7 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -19,9 +19,6 @@
>
> #include "cros_ec.h"
>
> -#define CROS_EC_DEV_EC_INDEX 0
> -#define CROS_EC_DEV_PD_INDEX 1
> -
> static struct cros_ec_platform ec_p = {
> .ec_name = CROS_EC_DEV_NAME,
> .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_EC_INDEX),
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 13ced9d2dd71..65191af5139c 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -433,7 +433,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>
> /* First try sending with proto v3. */
> ec_dev->proto_version = 3;
> - ret = cros_ec_host_command_proto_query(ec_dev, 0, proto_msg);
> + 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 *)
> @@ -459,7 +459,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> /*
> * Check for PD
> */
> - ret = cros_ec_host_command_proto_query(ec_dev, 1, proto_msg);
> + 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);
> @@ -609,7 +609,7 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> msg->insize = ec_dev->max_response;
> }
>
> - if (msg->command < EC_CMD_PASSTHRU_OFFSET(1)) {
> + if (msg->command < EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX)) {
> if (msg->outsize > ec_dev->max_request) {
> dev_err(ec_dev->dev,
> "request of size %u is too big (max: %u)\n",
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index c5e16566a634..5169bf33360b 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -280,7 +280,8 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> @@ -393,7 +394,8 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> @@ -535,7 +537,8 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> @@ -623,7 +626,8 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> @@ -723,7 +727,8 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> @@ -833,7 +838,8 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
>
> KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> KUNIT_EXPECT_EQ(test, mock->msg.command,
> - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> + 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);
> diff --git a/drivers/platform/chrome/cros_ec_trace.h b/drivers/platform/chrome/cros_ec_trace.h
> index 9bb5cd2c98b8..d7e407de88df 100644
> --- a/drivers/platform/chrome/cros_ec_trace.h
> +++ b/drivers/platform/chrome/cros_ec_trace.h
> @@ -30,8 +30,8 @@ TRACE_EVENT(cros_ec_request_start,
> ),
> TP_fast_assign(
> __entry->version = cmd->version;
> - __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
> - __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
> + __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> + __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> __entry->outsize = cmd->outsize;
> __entry->insize = cmd->insize;
> ),
> @@ -55,8 +55,8 @@ TRACE_EVENT(cros_ec_request_done,
> ),
> TP_fast_assign(
> __entry->version = cmd->version;
> - __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
> - __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
> + __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> + __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> __entry->outsize = cmd->outsize;
> __entry->insize = cmd->insize;
> __entry->result = cmd->result;
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 138fd912c808..6475a8066f00 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -21,6 +21,9 @@
> #define CROS_EC_DEV_SCP_NAME "cros_scp"
> #define CROS_EC_DEV_TP_NAME "cros_tp"
>
> +#define CROS_EC_DEV_EC_INDEX 0
> +#define CROS_EC_DEV_PD_INDEX 1
> +
> /*
> * The EC is unresponsive for a time after a reboot command. Add a
> * simple delay to make sure that the bus stays locked.
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 03/13] platform/chrome: use macros for passthru indexes
2022-06-06 15:22 ` Guenter Roeck
@ 2022-06-06 15:23 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:23 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 8:22 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > Move passthru indexes for EC and PD devices to common header. Also use
> > them instead of literal constants.
> >
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
Too much auto-typing. Trying again:
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> > ---
> > drivers/platform/chrome/cros_ec.c | 3 ---
> > drivers/platform/chrome/cros_ec_proto.c | 6 +++---
> > drivers/platform/chrome/cros_ec_proto_test.c | 18 ++++++++++++------
> > drivers/platform/chrome/cros_ec_trace.h | 8 ++++----
> > include/linux/platform_data/cros_ec_proto.h | 3 +++
> > 5 files changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> > index b3e94cdf7d1a..e51a3f2176c7 100644
> > --- a/drivers/platform/chrome/cros_ec.c
> > +++ b/drivers/platform/chrome/cros_ec.c
> > @@ -19,9 +19,6 @@
> >
> > #include "cros_ec.h"
> >
> > -#define CROS_EC_DEV_EC_INDEX 0
> > -#define CROS_EC_DEV_PD_INDEX 1
> > -
> > static struct cros_ec_platform ec_p = {
> > .ec_name = CROS_EC_DEV_NAME,
> > .cmd_offset = EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_EC_INDEX),
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 13ced9d2dd71..65191af5139c 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -433,7 +433,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> >
> > /* First try sending with proto v3. */
> > ec_dev->proto_version = 3;
> > - ret = cros_ec_host_command_proto_query(ec_dev, 0, proto_msg);
> > + 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 *)
> > @@ -459,7 +459,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > /*
> > * Check for PD
> > */
> > - ret = cros_ec_host_command_proto_query(ec_dev, 1, proto_msg);
> > + 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);
> > @@ -609,7 +609,7 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
> > msg->insize = ec_dev->max_response;
> > }
> >
> > - if (msg->command < EC_CMD_PASSTHRU_OFFSET(1)) {
> > + if (msg->command < EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX)) {
> > if (msg->outsize > ec_dev->max_request) {
> > dev_err(ec_dev->dev,
> > "request of size %u is too big (max: %u)\n",
> > diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> > index c5e16566a634..5169bf33360b 100644
> > --- a/drivers/platform/chrome/cros_ec_proto_test.c
> > +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> > @@ -280,7 +280,8 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > @@ -393,7 +394,8 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > @@ -535,7 +537,8 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > @@ -623,7 +626,8 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > @@ -723,7 +727,8 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > @@ -833,7 +838,8 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> >
> > KUNIT_EXPECT_EQ(test, mock->msg.version, 0);
> > KUNIT_EXPECT_EQ(test, mock->msg.command,
> > - EC_CMD_PASSTHRU_OFFSET(1) | EC_CMD_GET_PROTOCOL_INFO);
> > + 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);
> > diff --git a/drivers/platform/chrome/cros_ec_trace.h b/drivers/platform/chrome/cros_ec_trace.h
> > index 9bb5cd2c98b8..d7e407de88df 100644
> > --- a/drivers/platform/chrome/cros_ec_trace.h
> > +++ b/drivers/platform/chrome/cros_ec_trace.h
> > @@ -30,8 +30,8 @@ TRACE_EVENT(cros_ec_request_start,
> > ),
> > TP_fast_assign(
> > __entry->version = cmd->version;
> > - __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
> > - __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
> > + __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> > + __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> > __entry->outsize = cmd->outsize;
> > __entry->insize = cmd->insize;
> > ),
> > @@ -55,8 +55,8 @@ TRACE_EVENT(cros_ec_request_done,
> > ),
> > TP_fast_assign(
> > __entry->version = cmd->version;
> > - __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(1);
> > - __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(1);
> > + __entry->offset = cmd->command / EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> > + __entry->command = cmd->command % EC_CMD_PASSTHRU_OFFSET(CROS_EC_DEV_PD_INDEX);
> > __entry->outsize = cmd->outsize;
> > __entry->insize = cmd->insize;
> > __entry->result = cmd->result;
> > diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> > index 138fd912c808..6475a8066f00 100644
> > --- a/include/linux/platform_data/cros_ec_proto.h
> > +++ b/include/linux/platform_data/cros_ec_proto.h
> > @@ -21,6 +21,9 @@
> > #define CROS_EC_DEV_SCP_NAME "cros_scp"
> > #define CROS_EC_DEV_TP_NAME "cros_tp"
> >
> > +#define CROS_EC_DEV_EC_INDEX 0
> > +#define CROS_EC_DEV_PD_INDEX 1
> > +
> > /*
> > * The EC is unresponsive for a time after a reboot command. Add a
> > * simple delay to make sure that the bus stays locked.
> > --
> > 2.36.1.255.ge46751e96f-goog
> >
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 04/13] platform/chrome: cros_ec_proto: assign buffer size from protocol info
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (2 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 03/13] platform/chrome: use macros for passthru indexes Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:24 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 05/13] platform/chrome: cros_ec_proto: remove redundant NULL check Tzung-Bi Shih
` (8 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
`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.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 04/13] platform/chrome: cros_ec_proto: assign buffer size from protocol info
2022-06-06 14:10 ` [PATCH 04/13] platform/chrome: cros_ec_proto: assign buffer size from protocol info Tzung-Bi Shih
@ 2022-06-06 15:24 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:24 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> `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.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 05/13] platform/chrome: cros_ec_proto: remove redundant NULL check
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (3 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 04/13] platform/chrome: cros_ec_proto: assign buffer size from protocol info Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:46 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 06/13] platform/chrome: cros_ec_proto: use cros_ec_map_error() Tzung-Bi Shih
` (7 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
send_command() already checks if `ec_dev->pkt_xfer` is NULL. Remove the
redundant check.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 629dce3e6ab3..1b851dcd20c9 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -281,9 +281,6 @@ static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev,
*/
int ret;
- if (!ec_dev->pkt_xfer)
- return -EPROTONOSUPPORT;
-
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);
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 05/13] platform/chrome: cros_ec_proto: remove redundant NULL check
2022-06-06 14:10 ` [PATCH 05/13] platform/chrome: cros_ec_proto: remove redundant NULL check Tzung-Bi Shih
@ 2022-06-06 15:46 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:46 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> send_command() already checks if `ec_dev->pkt_xfer` is NULL. Remove the
> redundant check.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 629dce3e6ab3..1b851dcd20c9 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -281,9 +281,6 @@ static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev,
> */
> int ret;
>
> - if (!ec_dev->pkt_xfer)
> - return -EPROTONOSUPPORT;
> -
> 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);
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 06/13] platform/chrome: cros_ec_proto: use cros_ec_map_error()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (4 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 05/13] platform/chrome: cros_ec_proto: remove redundant NULL check Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 15:55 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info() Tzung-Bi Shih
` (6 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
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.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 06/13] platform/chrome: cros_ec_proto: use cros_ec_map_error()
2022-06-06 14:10 ` [PATCH 06/13] platform/chrome: cros_ec_proto: use cros_ec_map_error() Tzung-Bi Shih
@ 2022-06-06 15:55 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 15:55 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (5 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 06/13] platform/chrome: cros_ec_proto: use cros_ec_map_error() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:06 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy() Tzung-Bi Shih
` (5 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
Rename cros_ec_host_command_proto_query() to fill_protocol_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 <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 130 +++++++++----------
drivers/platform/chrome/cros_ec_proto_test.c | 28 ++--
2 files changed, 76 insertions(+), 82 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 71ba6a56ad7c..f57b4dba95b7 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 fill_protocol_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.
- */
+ struct cros_ec_command *msg;
+ struct ec_response_get_protocol_info *info;
int ret;
- memset(msg, 0, sizeof(*msg));
+ 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;
+
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,43 @@ 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;
}
- if (devidx > 0 && msg->result == EC_RES_INVALID_COMMAND)
- return -ENODEV;
- else if (msg->result != EC_RES_SUCCESS)
- return msg->result;
+ ret = cros_ec_map_error(msg->result);
+ if (ret)
+ goto exit;
+
+ 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 +451,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 (!fill_protocol_info(ec_dev, CROS_EC_DEV_EC_INDEX)) {
+ /* Check for PD. */
+ fill_protocol_info(ec_dev, CROS_EC_DEV_PD_INDEX);
} else {
/* Try querying with a v2 hello message. */
ec_dev->proto_version = 2;
@@ -524,8 +520,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 +551,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 5169bf33360b..cdedbdfc1885 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 fill_protocol_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 fill_protocol_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 fill_protocol_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 fill_protocol_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -348,7 +348,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For fill_protocol_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -364,7 +364,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For fill_protocol_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
sizeof(struct ec_response_get_protocol_info));
@@ -375,7 +375,7 @@ static void cros_ec_proto_test_query_all_no_pd(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 fill_protocol_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -387,7 +387,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For fill_protocol_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -411,7 +411,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
struct ec_xfer_mock *mock;
int ret;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For fill_protocol_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
sizeof(struct ec_response_get_protocol_info));
@@ -433,7 +433,7 @@ static void cros_ec_proto_test_query_all_legacy_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 fill_protocol_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -771,7 +771,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
/* Set some garbage bytes. */
ec_dev->host_event_wake_mask = U32_MAX;
- /* For cros_ec_host_command_proto_query() without passthru. */
+ /* For fill_protocol_info() without passthru. */
{
struct ec_response_get_protocol_info *data;
@@ -787,7 +787,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
data->max_response_packet_size = 0xef;
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For fill_protocol_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
sizeof(struct ec_response_get_protocol_info));
@@ -819,7 +819,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(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 fill_protocol_info() without passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -831,7 +831,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query() with passthru. */
+ /* For fill_protocol_info() with passthru. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info()
2022-06-06 14:10 ` [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info() Tzung-Bi Shih
@ 2022-06-06 16:06 ` Guenter Roeck
2022-06-07 0:54 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:06 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Rename cros_ec_host_command_proto_query() to fill_protocol_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 <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 130 +++++++++----------
> drivers/platform/chrome/cros_ec_proto_test.c | 28 ++--
> 2 files changed, 76 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 71ba6a56ad7c..f57b4dba95b7 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 fill_protocol_info(struct cros_ec_device *ec_dev, int devidx)
I would suggest to keep the cros_ec_ prefix. Also, "fill" sounds a bit
too much as if the function would fill some structure fields. "get"
(ie cros_ec_get_proto_info or similar) might be a better name,
Thanks,
Guenter
> {
> - /*
> - * 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.
> - */
> + struct cros_ec_command *msg;
> + struct ec_response_get_protocol_info *info;
> int ret;
>
> - memset(msg, 0, sizeof(*msg));
> + 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;
> +
> 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,43 @@ 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;
> }
>
> - if (devidx > 0 && msg->result == EC_RES_INVALID_COMMAND)
> - return -ENODEV;
> - else if (msg->result != EC_RES_SUCCESS)
> - return msg->result;
> + ret = cros_ec_map_error(msg->result);
> + if (ret)
> + goto exit;
> +
> + 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 +451,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 (!fill_protocol_info(ec_dev, CROS_EC_DEV_EC_INDEX)) {
> + /* Check for PD. */
> + fill_protocol_info(ec_dev, CROS_EC_DEV_PD_INDEX);
> } else {
> /* Try querying with a v2 hello message. */
> ec_dev->proto_version = 2;
> @@ -524,8 +520,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 +551,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 5169bf33360b..cdedbdfc1885 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 fill_protocol_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 fill_protocol_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 fill_protocol_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 fill_protocol_info() with passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -348,7 +348,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
> struct ec_xfer_mock *mock;
> int ret;
>
> - /* For cros_ec_host_command_proto_query() without passthru. */
> + /* For fill_protocol_info() without passthru. */
> {
> struct ec_response_get_protocol_info *data;
>
> @@ -364,7 +364,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
> data->max_response_packet_size = 0xef;
> }
>
> - /* For cros_ec_host_command_proto_query() with passthru. */
> + /* For fill_protocol_info() with passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> sizeof(struct ec_response_get_protocol_info));
> @@ -375,7 +375,7 @@ static void cros_ec_proto_test_query_all_no_pd(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 fill_protocol_info() without passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -387,7 +387,7 @@ static void cros_ec_proto_test_query_all_no_pd(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query() with passthru. */
> + /* For fill_protocol_info() with passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -411,7 +411,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
> struct ec_xfer_mock *mock;
> int ret;
>
> - /* For cros_ec_host_command_proto_query() without passthru. */
> + /* For fill_protocol_info() without passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> sizeof(struct ec_response_get_protocol_info));
> @@ -433,7 +433,7 @@ static void cros_ec_proto_test_query_all_legacy_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 fill_protocol_info() without passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -771,7 +771,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> /* Set some garbage bytes. */
> ec_dev->host_event_wake_mask = U32_MAX;
>
> - /* For cros_ec_host_command_proto_query() without passthru. */
> + /* For fill_protocol_info() without passthru. */
> {
> struct ec_response_get_protocol_info *data;
>
> @@ -787,7 +787,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> data->max_response_packet_size = 0xef;
> }
>
> - /* For cros_ec_host_command_proto_query() with passthru. */
> + /* For fill_protocol_info() with passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> sizeof(struct ec_response_get_protocol_info));
> @@ -819,7 +819,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(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 fill_protocol_info() without passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -831,7 +831,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query() with passthru. */
> + /* For fill_protocol_info() with passthru. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info()
2022-06-06 16:06 ` Guenter Roeck
@ 2022-06-07 0:54 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:54 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 09:06:03AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > -static int cros_ec_host_command_proto_query(struct cros_ec_device *ec_dev,
> > - int devidx,
> > - struct cros_ec_command *msg)
> > +static int fill_protocol_info(struct cros_ec_device *ec_dev, int devidx)
>
> I would suggest to keep the cros_ec_ prefix. Also, "fill" sounds a bit
> too much as if the function would fill some structure fields. "get"
> (ie cros_ec_get_proto_info or similar) might be a better name,
Ack, will fix in next version.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (6 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 07/13] platform/chrome: cros_ec_proto: separate fill_protocol_info() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:06 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc() Tzung-Bi Shih
` (4 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
Rename cros_ec_host_command_proto_query_v2() to
fill_protocol_info_legacy() and make it responsible for setting `ec_dev`
fields for EC protocol v2.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 72 +++++++++-----------
drivers/platform/chrome/cros_ec_proto_test.c | 4 +-
2 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index f57b4dba95b7..abb30a685567 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -344,51 +344,57 @@ static int fill_protocol_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 fill_protocol_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;
}
@@ -460,20 +466,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
fill_protocol_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 = fill_protocol_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
@@ -481,7 +475,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 cdedbdfc1885..79150bf511fb 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -418,7 +418,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For fill_protocol_info_legacy(). */
{
struct ec_response_hello *data;
@@ -445,7 +445,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_host_command_proto_query_v2(). */
+ /* For fill_protocol_info_legacy(). */
{
struct ec_params_hello *data;
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy()
2022-06-06 14:10 ` [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy() Tzung-Bi Shih
@ 2022-06-06 16:06 ` Guenter Roeck
2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:06 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Rename cros_ec_host_command_proto_query_v2() to
> fill_protocol_info_legacy() and make it responsible for setting `ec_dev`
cros_ec_get_proto_info_legacy() ?
> fields for EC protocol v2.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 72 +++++++++-----------
> drivers/platform/chrome/cros_ec_proto_test.c | 4 +-
> 2 files changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index f57b4dba95b7..abb30a685567 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -344,51 +344,57 @@ static int fill_protocol_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 fill_protocol_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;
> }
> @@ -460,20 +466,8 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> fill_protocol_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 = fill_protocol_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
> @@ -481,7 +475,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 cdedbdfc1885..79150bf511fb 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -418,7 +418,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For fill_protocol_info_legacy(). */
> {
> struct ec_response_hello *data;
>
> @@ -445,7 +445,7 @@ static void cros_ec_proto_test_query_all_legacy_normal(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_host_command_proto_query_v2(). */
> + /* For fill_protocol_info_legacy(). */
> {
> struct ec_params_hello *data;
>
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy()
2022-06-06 16:06 ` Guenter Roeck
@ 2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 09:06:35AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > Rename cros_ec_host_command_proto_query_v2() to
> > fill_protocol_info_legacy() and make it responsible for setting `ec_dev`
>
> cros_ec_get_proto_info_legacy() ?
Ack, will fix in next version.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (7 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 08/13] platform/chrome: cros_ec_proto: separate fill_protocol_info_legacy() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:04 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask() Tzung-Bi Shih
` (3 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
Use devm_krealloc() to re-allocate `din` and `dout`.
Also remove the unneeded devm_kfree() in error handling path as they are
device managed memory.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 25 ++++++--------------
drivers/platform/chrome/cros_ec_proto_test.c | 3 +--
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index abb30a685567..5f4414f05d66 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -479,21 +479,13 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
}
}
- devm_kfree(dev, ec_dev->din);
- devm_kfree(dev, ec_dev->dout);
-
- ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
- if (!ec_dev->din) {
- ret = -ENOMEM;
- goto exit;
- }
+ ec_dev->din = devm_krealloc(dev, ec_dev->din, ec_dev->din_size, GFP_KERNEL | __GFP_ZERO);
+ if (!ec_dev->din)
+ return -ENOMEM;
- ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
- if (!ec_dev->dout) {
- devm_kfree(dev, ec_dev->din);
- ret = -ENOMEM;
- goto exit;
- }
+ ec_dev->dout = devm_krealloc(dev, ec_dev->dout, ec_dev->dout_size, GFP_KERNEL | __GFP_ZERO);
+ if (!ec_dev->dout)
+ return -ENOMEM;
/* Probe if MKBP event is supported */
ret = cros_ec_get_host_command_version_mask(ec_dev,
@@ -542,10 +534,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
"failed to retrieve wake mask: %d\n", ret);
}
- ret = 0;
-
-exit:
- return ret;
+ return 0;
}
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 79150bf511fb..22f9322787f4 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -180,8 +180,7 @@ static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
/*
* cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
- * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
- * ec_dev->dev.
+ * calling devm_krealloc(). Set them to NULL as they aren't managed by ec_dev->dev.
*/
ec_dev->din = NULL;
ec_dev->dout = NULL;
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc()
2022-06-06 14:10 ` [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc() Tzung-Bi Shih
@ 2022-06-06 16:04 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:04 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> Use devm_krealloc() to re-allocate `din` and `dout`.
>
> Also remove the unneeded devm_kfree() in error handling path as they are
> device managed memory.
>
Problem with that is that the callers don't always handle error
returns from calls to cros_ec_query_all(), that the call isn't
typically from the probe function, and that the release function would
not be called after partial allocation failures until the driver is
unloaded. This would result in memory leaks, making the memory
situation even worse. I am not sure if using devm_ functions to
allocate the memory really makes sense here.
Guenter
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 25 ++++++--------------
> drivers/platform/chrome/cros_ec_proto_test.c | 3 +--
> 2 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index abb30a685567..5f4414f05d66 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -479,21 +479,13 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> }
> }
>
> - devm_kfree(dev, ec_dev->din);
> - devm_kfree(dev, ec_dev->dout);
> -
> - ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> - if (!ec_dev->din) {
> - ret = -ENOMEM;
> - goto exit;
> - }
> + ec_dev->din = devm_krealloc(dev, ec_dev->din, ec_dev->din_size, GFP_KERNEL | __GFP_ZERO);
> + if (!ec_dev->din)
> + return -ENOMEM;
>
> - ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
> - if (!ec_dev->dout) {
> - devm_kfree(dev, ec_dev->din);
> - ret = -ENOMEM;
> - goto exit;
> - }
> + ec_dev->dout = devm_krealloc(dev, ec_dev->dout, ec_dev->dout_size, GFP_KERNEL | __GFP_ZERO);
> + if (!ec_dev->dout)
> + return -ENOMEM;
>
> /* Probe if MKBP event is supported */
> ret = cros_ec_get_host_command_version_mask(ec_dev,
> @@ -542,10 +534,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> "failed to retrieve wake mask: %d\n", ret);
> }
>
> - ret = 0;
> -
> -exit:
> - return ret;
> + return 0;
> }
> 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 79150bf511fb..22f9322787f4 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -180,8 +180,7 @@ static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
>
> /*
> * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
> - * calling devm_kfree() and devm_kzalloc(). Set them to NULL as they aren't managed by
> - * ec_dev->dev.
> + * calling devm_krealloc(). Set them to NULL as they aren't managed by ec_dev->dev.
> */
> ec_dev->din = NULL;
> ec_dev->dout = NULL;
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (8 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:09 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 11/13] platform/chrome: cros_ec_proto: fix get_host_command_version_mask() returns Tzung-Bi Shih
` (2 subsequent siblings)
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
- cros_ec_get_host_command_version_mask() is a private (static) function.
Rename it to get_host_command_version_mask().
- Join multiple lines into one if it can fit in 100 columns.
- Don't show MKBP support version if it doesn't support.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 26 ++++++----------
drivers/platform/chrome/cros_ec_proto_test.c | 32 ++++++++++----------
2 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 5f4414f05d66..07b57ea105b6 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -400,7 +400,7 @@ static int fill_protocol_info_legacy(struct cros_ec_device *ec_dev)
}
/*
- * cros_ec_get_host_command_version_mask
+ * get_host_command_version_mask
*
* Get the version mask of a given command.
*
@@ -415,16 +415,14 @@ static int fill_protocol_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 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;
- msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)),
- GFP_KERNEL);
+ msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)), GFP_KERNEL);
if (!msg)
return -ENOMEM;
@@ -443,7 +441,6 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
}
kfree(msg);
-
return ret;
}
@@ -488,21 +485,16 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
return -ENOMEM;
/* Probe if MKBP event is supported */
- ret = cros_ec_get_host_command_version_mask(ec_dev,
- EC_CMD_GET_NEXT_EVENT,
- &ver_mask);
- if (ret < 0 || ver_mask == 0)
+ ret = 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
+ } else {
ec_dev->mkbp_event_supported = fls(ver_mask);
-
- dev_dbg(ec_dev->dev, "MKBP support version %u\n",
- ec_dev->mkbp_event_supported - 1);
+ dev_dbg(ec_dev->dev, "MKBP support version %u\n", ec_dev->mkbp_event_supported - 1);
+ }
/* 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);
+ ret = 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. */
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 22f9322787f4..e2c369765612 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -217,7 +217,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
data->max_request_packet_size = 0xbf;
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
struct ec_response_get_cmd_versions *data;
@@ -228,7 +228,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
data->version_mask = BIT(6) | BIT(5);
}
- /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
struct ec_response_get_cmd_versions *data;
@@ -288,7 +288,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0xbf - sizeof(struct ec_host_request));
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
struct ec_params_get_cmd_versions *data;
@@ -307,7 +307,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 7);
}
- /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
struct ec_params_get_cmd_versions *data;
@@ -502,7 +502,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
struct ec_response_get_cmd_versions *data;
@@ -543,7 +543,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_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
struct ec_params_get_cmd_versions *data;
@@ -596,7 +596,7 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
mock = cros_kunit_ec_xfer_mock_add(test, 0);
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
@@ -632,7 +632,7 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
struct ec_params_get_cmd_versions *data;
@@ -685,14 +685,14 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
mock = cros_kunit_ec_xfer_mock_add(test,
sizeof(struct ec_response_get_cmd_versions));
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
struct ec_response_get_cmd_versions *data;
@@ -733,7 +733,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_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -745,7 +745,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
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. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -793,14 +793,14 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
mock = cros_kunit_ec_xfer_mock_add(test,
sizeof(struct ec_response_get_cmd_versions));
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
mock = cros_kunit_ec_xfer_mock_add(test,
sizeof(struct ec_response_get_cmd_versions));
@@ -844,7 +844,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
}
- /* For cros_ec_get_host_command_version_mask() for MKBP. */
+ /* For get_host_command_version_mask() for MKBP. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -856,7 +856,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
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. */
+ /* For get_host_command_version_mask() for host sleep v1. */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask()
2022-06-06 14:10 ` [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask() Tzung-Bi Shih
@ 2022-06-06 16:09 ` Guenter Roeck
2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:09 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> - cros_ec_get_host_command_version_mask() is a private (static) function.
> Rename it to get_host_command_version_mask().
Personally I prefer to still have prefixes, even for static functions.
If the idea is to have a shorter function name, maybe shorten the rest
of the function name a bit.
Guenter
>
> - Join multiple lines into one if it can fit in 100 columns.
>
> - Don't show MKBP support version if it doesn't support.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 26 ++++++----------
> drivers/platform/chrome/cros_ec_proto_test.c | 32 ++++++++++----------
> 2 files changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 5f4414f05d66..07b57ea105b6 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -400,7 +400,7 @@ static int fill_protocol_info_legacy(struct cros_ec_device *ec_dev)
> }
>
> /*
> - * cros_ec_get_host_command_version_mask
> + * get_host_command_version_mask
> *
> * Get the version mask of a given command.
> *
> @@ -415,16 +415,14 @@ static int fill_protocol_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 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;
>
> - msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)),
> - GFP_KERNEL);
> + msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)), GFP_KERNEL);
> if (!msg)
> return -ENOMEM;
>
> @@ -443,7 +441,6 @@ static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
> }
>
> kfree(msg);
> -
> return ret;
> }
>
> @@ -488,21 +485,16 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> return -ENOMEM;
>
> /* Probe if MKBP event is supported */
> - ret = cros_ec_get_host_command_version_mask(ec_dev,
> - EC_CMD_GET_NEXT_EVENT,
> - &ver_mask);
> - if (ret < 0 || ver_mask == 0)
> + ret = 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
> + } else {
> ec_dev->mkbp_event_supported = fls(ver_mask);
> -
> - dev_dbg(ec_dev->dev, "MKBP support version %u\n",
> - ec_dev->mkbp_event_supported - 1);
> + dev_dbg(ec_dev->dev, "MKBP support version %u\n", ec_dev->mkbp_event_supported - 1);
> + }
>
> /* 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);
> + ret = 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. */
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 22f9322787f4..e2c369765612 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -217,7 +217,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> data->max_request_packet_size = 0xbf;
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> struct ec_response_get_cmd_versions *data;
>
> @@ -228,7 +228,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> data->version_mask = BIT(6) | BIT(5);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> struct ec_response_get_cmd_versions *data;
>
> @@ -288,7 +288,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> KUNIT_EXPECT_EQ(test, ec_dev->max_passthru, 0xbf - sizeof(struct ec_host_request));
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> struct ec_params_get_cmd_versions *data;
>
> @@ -307,7 +307,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> KUNIT_EXPECT_EQ(test, ec_dev->mkbp_event_supported, 7);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> struct ec_params_get_cmd_versions *data;
>
> @@ -502,7 +502,7 @@ static void cros_ec_proto_test_query_all_no_mkbp(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> struct ec_response_get_cmd_versions *data;
>
> @@ -543,7 +543,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_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> struct ec_params_get_cmd_versions *data;
>
> @@ -596,7 +596,7 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> mock = cros_kunit_ec_xfer_mock_add(test, 0);
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> @@ -632,7 +632,7 @@ static void cros_ec_proto_test_query_all_no_mkbp2(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> struct ec_params_get_cmd_versions *data;
>
> @@ -685,14 +685,14 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> mock = cros_kunit_ec_xfer_mock_add(test,
> sizeof(struct ec_response_get_cmd_versions));
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> struct ec_response_get_cmd_versions *data;
>
> @@ -733,7 +733,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_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -745,7 +745,7 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> 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. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -793,14 +793,14 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> mock = cros_kunit_ec_xfer_mock_add(test,
> sizeof(struct ec_response_get_cmd_versions));
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for host sleep v1. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> mock = cros_kunit_ec_xfer_mock_add(test,
> sizeof(struct ec_response_get_cmd_versions));
> @@ -844,7 +844,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, 0);
> }
>
> - /* For cros_ec_get_host_command_version_mask() for MKBP. */
> + /* For get_host_command_version_mask() for MKBP. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -856,7 +856,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> 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. */
> + /* For get_host_command_version_mask() for host sleep v1. */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask()
2022-06-06 16:09 ` Guenter Roeck
@ 2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 09:09:25AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > - cros_ec_get_host_command_version_mask() is a private (static) function.
> > Rename it to get_host_command_version_mask().
>
> Personally I prefer to still have prefixes, even for static functions.
> If the idea is to have a shorter function name, maybe shorten the rest
> of the function name a bit.
Ack, will fix in next version.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 11/13] platform/chrome: cros_ec_proto: fix get_host_command_version_mask() returns
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (9 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 10/13] platform/chrome: cros_ec_proto: arrange get_host_command_version_mask() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:16 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask() Tzung-Bi Shih
2022-06-06 14:10 ` [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns Tzung-Bi Shih
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
`mask` of get_host_command_version_mask() is valid only if it returns
positive number (i.e. sizeof(struct ec_response_get_cmd_versions)).
- Remove the unneeded `ver_mask` initialization.
- Update callers of get_host_command_version_mask() for checking correct
return values.
- Add a Kunit test: `ver_mask` has some garbage bytes from
previous EC_CMD_GET_NEXT_EVENT but there is no host sleep.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 8 +-
drivers/platform/chrome/cros_ec_proto_test.c | 109 +++++++++++++++++++
2 files changed, 113 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 07b57ea105b6..91c945c9911f 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -409,7 +409,7 @@ static int fill_protocol_info_legacy(struct cros_ec_device *ec_dev)
* @cmd: command to get the version of.
* @mask: result when function returns 0.
*
- * @return 0 on success, error code otherwise
+ * @return >0 on success, error code otherwise
*
* LOCKING:
* the caller has ec_dev->lock mutex or the caller knows there is
@@ -454,7 +454,7 @@ static int get_host_command_version_mask(struct cros_ec_device *ec_dev, u16 cmd,
int cros_ec_query_all(struct cros_ec_device *ec_dev)
{
struct device *dev = ec_dev->dev;
- u32 ver_mask = 0;
+ u32 ver_mask;
int ret;
/* First try sending with proto v3. */
@@ -486,7 +486,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
/* Probe if MKBP event is supported */
ret = get_host_command_version_mask(ec_dev, EC_CMD_GET_NEXT_EVENT, &ver_mask);
- if (ret < 0 || ver_mask == 0) {
+ if (ret <= 0 || ver_mask == 0) {
ec_dev->mkbp_event_supported = 0;
} else {
ec_dev->mkbp_event_supported = fls(ver_mask);
@@ -495,7 +495,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
/* Probe if host sleep v1 is supported for S0ix failure detection. */
ret = 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)));
+ 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 e2c369765612..7d73aeb99d1d 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -760,6 +760,114 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
}
}
+static void cros_ec_proto_test_query_all_no_host_sleep2(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 fill_protocol_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 fill_protocol_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For 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 get_host_command_version_mask(). */
+ data = (struct ec_response_get_cmd_versions *)mock->o_data;
+ data->version_mask = 0xbeef;
+ }
+
+ /* For 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 fill_protocol_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 fill_protocol_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 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 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(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv = test->priv;
@@ -941,6 +1049,7 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp2),
KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
+ KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep2),
KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
{}
};
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 11/13] platform/chrome: cros_ec_proto: fix get_host_command_version_mask() returns
2022-06-06 14:10 ` [PATCH 11/13] platform/chrome: cros_ec_proto: fix get_host_command_version_mask() returns Tzung-Bi Shih
@ 2022-06-06 16:16 ` Guenter Roeck
0 siblings, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:16 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> `mask` of get_host_command_version_mask() is valid only if it returns
> positive number (i.e. sizeof(struct ec_response_get_cmd_versions)).
>
> - Remove the unneeded `ver_mask` initialization.
>
> - Update callers of get_host_command_version_mask() for checking correct
> return values.
>
I think it would be better to have the function return a negative
error value instead of 0 if returning 0 is indeed an error. I also
wonder if the caller ever uses a return value > 0, or if the function
should just return 0 if there was no error.
Thanks,
Guenter
> - Add a Kunit test: `ver_mask` has some garbage bytes from
> previous EC_CMD_GET_NEXT_EVENT but there is no host sleep.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 8 +-
> drivers/platform/chrome/cros_ec_proto_test.c | 109 +++++++++++++++++++
> 2 files changed, 113 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 07b57ea105b6..91c945c9911f 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -409,7 +409,7 @@ static int fill_protocol_info_legacy(struct cros_ec_device *ec_dev)
> * @cmd: command to get the version of.
> * @mask: result when function returns 0.
> *
> - * @return 0 on success, error code otherwise
> + * @return >0 on success, error code otherwise
> *
> * LOCKING:
> * the caller has ec_dev->lock mutex or the caller knows there is
> @@ -454,7 +454,7 @@ static int get_host_command_version_mask(struct cros_ec_device *ec_dev, u16 cmd,
> int cros_ec_query_all(struct cros_ec_device *ec_dev)
> {
> struct device *dev = ec_dev->dev;
> - u32 ver_mask = 0;
> + u32 ver_mask;
> int ret;
>
> /* First try sending with proto v3. */
> @@ -486,7 +486,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>
> /* Probe if MKBP event is supported */
> ret = get_host_command_version_mask(ec_dev, EC_CMD_GET_NEXT_EVENT, &ver_mask);
> - if (ret < 0 || ver_mask == 0) {
> + if (ret <= 0 || ver_mask == 0) {
> ec_dev->mkbp_event_supported = 0;
> } else {
> ec_dev->mkbp_event_supported = fls(ver_mask);
> @@ -495,7 +495,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>
> /* Probe if host sleep v1 is supported for S0ix failure detection. */
> ret = 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)));
> + 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 e2c369765612..7d73aeb99d1d 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -760,6 +760,114 @@ static void cros_ec_proto_test_query_all_no_host_sleep(struct kunit *test)
> }
> }
>
> +static void cros_ec_proto_test_query_all_no_host_sleep2(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 fill_protocol_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 fill_protocol_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For 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 get_host_command_version_mask(). */
> + data = (struct ec_response_get_cmd_versions *)mock->o_data;
> + data->version_mask = 0xbeef;
> + }
> +
> + /* For 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 fill_protocol_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 fill_protocol_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 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 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(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv = test->priv;
> @@ -941,6 +1049,7 @@ static struct kunit_case cros_ec_proto_test_cases[] = {
> KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp),
> KUNIT_CASE(cros_ec_proto_test_query_all_no_mkbp2),
> KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep),
> + KUNIT_CASE(cros_ec_proto_test_query_all_no_host_sleep2),
> KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
> {}
> };
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask()
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (10 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 11/13] platform/chrome: cros_ec_proto: fix get_host_command_version_mask() returns Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:18 ` Guenter Roeck
2022-06-06 14:10 ` [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns Tzung-Bi Shih
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
- cros_ec_get_host_event_wake_mask() is a private (static) function.
Rename it to get_host_event_wake_mask().
- Join multiple lines into one if it can fit in 100 columns.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 9 ++++-----
drivers/platform/chrome/cros_ec_proto_test.c | 8 ++++----
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 91c945c9911f..1622e24747c9 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -230,7 +230,7 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
EXPORT_SYMBOL(cros_ec_check_result);
/*
- * cros_ec_get_host_event_wake_mask
+ * get_host_event_wake_mask
*
* Get the mask of host events that cause wake from suspend.
*
@@ -242,7 +242,7 @@ 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, uint32_t *mask)
+static int 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;
@@ -498,7 +498,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, &ec_dev->host_event_wake_mask);
+ ret = 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,
@@ -522,8 +522,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
* other errors.
*/
if (ret != -EOPNOTSUPP)
- dev_err(ec_dev->dev,
- "failed to retrieve wake mask: %d\n", ret);
+ dev_err(ec_dev->dev, "failed to retrieve wake mask: %d\n", ret);
}
return 0;
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index 7d73aeb99d1d..f63196289f54 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -239,7 +239,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
data->version_mask = BIT(1);
}
- /* For cros_ec_get_host_event_wake_mask(). */
+ /* For get_host_event_wake_mask(). */
{
struct ec_response_host_event_mask *data;
@@ -326,7 +326,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
KUNIT_EXPECT_TRUE(test, ec_dev->host_sleep_v1);
}
- /* For cros_ec_get_host_event_wake_mask(). */
+ /* For get_host_event_wake_mask(). */
{
mock = cros_kunit_ec_xfer_mock_next();
KUNIT_EXPECT_PTR_NE(test, mock, NULL);
@@ -915,7 +915,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_ASSERT_PTR_NE(test, mock, NULL);
}
- /* For cros_ec_get_host_event_wake_mask(). */
+ /* For get_host_event_wake_mask(). */
{
mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
sizeof(struct ec_response_host_event_mask));
@@ -976,7 +976,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
}
- /* For cros_ec_get_host_event_wake_mask(). */
+ /* For get_host_event_wake_mask(). */
{
u32 mask;
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask()
2022-06-06 14:10 ` [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask() Tzung-Bi Shih
@ 2022-06-06 16:18 ` Guenter Roeck
2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:18 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> - cros_ec_get_host_event_wake_mask() is a private (static) function.
> Rename it to get_host_event_wake_mask().
>
Same as before - I still prefer to have prefixes. Also, this is a bit
of point-of-view thing, so I'd rather leave function names alone
unless there is a good reason to change them. Otherwise we might end
up with sequences of function-name-rename patches for no good reason
besides someone mot liking the existing names.
Thanks,
Guenter
> - Join multiple lines into one if it can fit in 100 columns.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 9 ++++-----
> drivers/platform/chrome/cros_ec_proto_test.c | 8 ++++----
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 91c945c9911f..1622e24747c9 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -230,7 +230,7 @@ int cros_ec_check_result(struct cros_ec_device *ec_dev,
> EXPORT_SYMBOL(cros_ec_check_result);
>
> /*
> - * cros_ec_get_host_event_wake_mask
> + * get_host_event_wake_mask
> *
> * Get the mask of host events that cause wake from suspend.
> *
> @@ -242,7 +242,7 @@ 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, uint32_t *mask)
> +static int 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;
> @@ -498,7 +498,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, &ec_dev->host_event_wake_mask);
> + ret = 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,
> @@ -522,8 +522,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> * other errors.
> */
> if (ret != -EOPNOTSUPP)
> - dev_err(ec_dev->dev,
> - "failed to retrieve wake mask: %d\n", ret);
> + dev_err(ec_dev->dev, "failed to retrieve wake mask: %d\n", ret);
> }
>
> return 0;
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 7d73aeb99d1d..f63196289f54 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -239,7 +239,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> data->version_mask = BIT(1);
> }
>
> - /* For cros_ec_get_host_event_wake_mask(). */
> + /* For get_host_event_wake_mask(). */
> {
> struct ec_response_host_event_mask *data;
>
> @@ -326,7 +326,7 @@ static void cros_ec_proto_test_query_all_normal(struct kunit *test)
> KUNIT_EXPECT_TRUE(test, ec_dev->host_sleep_v1);
> }
>
> - /* For cros_ec_get_host_event_wake_mask(). */
> + /* For get_host_event_wake_mask(). */
> {
> mock = cros_kunit_ec_xfer_mock_next();
> KUNIT_EXPECT_PTR_NE(test, mock, NULL);
> @@ -915,7 +915,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> }
>
> - /* For cros_ec_get_host_event_wake_mask(). */
> + /* For get_host_event_wake_mask(). */
> {
> mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> sizeof(struct ec_response_host_event_mask));
> @@ -976,7 +976,7 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> KUNIT_EXPECT_EQ(test, mock->msg.outsize, sizeof(struct ec_params_get_cmd_versions));
> }
>
> - /* For cros_ec_get_host_event_wake_mask(). */
> + /* For get_host_event_wake_mask(). */
> {
> u32 mask;
>
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask()
2022-06-06 16:18 ` Guenter Roeck
@ 2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 09:18:57AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >
> > - cros_ec_get_host_event_wake_mask() is a private (static) function.
> > Rename it to get_host_event_wake_mask().
> >
> Same as before - I still prefer to have prefixes. Also, this is a bit
> of point-of-view thing, so I'd rather leave function names alone
> unless there is a good reason to change them. Otherwise we might end
> up with sequences of function-name-rename patches for no good reason
> besides someone mot liking the existing names.
Ack.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns
2022-06-06 14:10 [PATCH 00/13] platform/chrome: Kunit tests and refactor for cros_ec_query_all() Tzung-Bi Shih
` (11 preceding siblings ...)
2022-06-06 14:10 ` [PATCH 12/13] platform/chrome: cros_ec_proto: arrange get_host_event_wake_mask() Tzung-Bi Shih
@ 2022-06-06 14:10 ` Tzung-Bi Shih
2022-06-06 16:14 ` Guenter Roeck
12 siblings, 1 reply; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-06 14:10 UTC (permalink / raw)
To: bleung, groeck; +Cc: chrome-platform, tzungbi, linux-kernel
get_host_event_wake_mask() only gets valid result if send_command()
returns sizeof(struct ec_response_host_event_mask). Simplify the
code and correct the callers.
Also add a Kunit test for guarding if get_host_event_wake_mask() returns 0.
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
drivers/platform/chrome/cros_ec_proto.c | 15 +--
drivers/platform/chrome/cros_ec_proto_test.c | 131 +++++++++++++++++++
2 files changed, 137 insertions(+), 9 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 1622e24747c9..1d2399473f35 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -236,7 +236,7 @@ EXPORT_SYMBOL(cros_ec_check_result);
*
* @ec_dev: EC device to call
* @msg: message structure to use
- * @mask: result when function returns >=0.
+ * @mask: result when function returns >0.
*
* LOCKING:
* the caller has ec_dev->lock mutex, or the caller knows there is
@@ -256,19 +256,16 @@ static int get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint32_t *mas
msg->insize = sizeof(*r);
ret = send_command(ec_dev, msg);
- if (ret >= 0) {
+ if (ret > 0) {
mapped = cros_ec_map_error(msg->result);
if (mapped) {
ret = mapped;
- goto exit;
+ } else {
+ r = (struct ec_response_host_event_mask *)msg->data;
+ *mask = r->mask;
}
}
- if (ret > 0) {
- r = (struct ec_response_host_event_mask *)msg->data;
- *mask = r->mask;
- }
-exit:
kfree(msg);
return ret;
}
@@ -499,7 +496,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
/* Get host event wake mask. */
ret = get_host_event_wake_mask(ec_dev, &ec_dev->host_event_wake_mask);
- if (ret < 0) {
+ if (ret <= 0) {
/*
* If the EC doesn't support EC_CMD_HOST_EVENT_GET_WAKE_MASK,
* use a reasonable default. Note that we ignore various
diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
index f63196289f54..1ccc837b30cf 100644
--- a/drivers/platform/chrome/cros_ec_proto_test.c
+++ b/drivers/platform/chrome/cros_ec_proto_test.c
@@ -999,6 +999,136 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
}
}
+static void cros_ec_proto_test_query_all_default_wake_mask2(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 fill_protocol_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 fill_protocol_info() with passthru. */
+ {
+ mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
+ sizeof(struct ec_response_get_protocol_info));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For get_host_command_version_mask() for MKBP. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test,
+ sizeof(struct ec_response_get_cmd_versions));
+ KUNIT_ASSERT_PTR_NE(test, mock, NULL);
+ }
+
+ /* For get_host_command_version_mask() for host sleep v1. */
+ {
+ mock = cros_kunit_ec_xfer_mock_add(test,
+ sizeof(struct ec_response_get_cmd_versions));
+ 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 fill_protocol_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 fill_protocol_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 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 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 int cros_ec_proto_test_init(struct kunit *test)
{
struct cros_ec_proto_test_priv *priv;
@@ -1051,6 +1181,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_sleep2),
KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
+ KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask2),
{}
};
--
2.36.1.255.ge46751e96f-goog
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns
2022-06-06 14:10 ` [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns Tzung-Bi Shih
@ 2022-06-06 16:14 ` Guenter Roeck
2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2022-06-06 16:14 UTC (permalink / raw)
To: Tzung-Bi Shih; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> get_host_event_wake_mask() only gets valid result if send_command()
> returns sizeof(struct ec_response_host_event_mask). Simplify the
> code and correct the callers.
>
> Also add a Kunit test for guarding if get_host_event_wake_mask() returns 0.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 15 +--
> drivers/platform/chrome/cros_ec_proto_test.c | 131 +++++++++++++++++++
> 2 files changed, 137 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 1622e24747c9..1d2399473f35 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -236,7 +236,7 @@ EXPORT_SYMBOL(cros_ec_check_result);
> *
> * @ec_dev: EC device to call
> * @msg: message structure to use
> - * @mask: result when function returns >=0.
> + * @mask: result when function returns >0.
> *
> * LOCKING:
> * the caller has ec_dev->lock mutex, or the caller knows there is
> @@ -256,19 +256,16 @@ static int get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint32_t *mas
> msg->insize = sizeof(*r);
>
> ret = send_command(ec_dev, msg);
> - if (ret >= 0) {
> + if (ret > 0) {
The idea here was to (potentially) return an error if ret == 0. This
is no longer the case after this change. Instead, the caller has to
check for ret == 0 and treat it as an error. I think it would make
more sense to explicitly check for ret ==0, eg with something like
mapped = cros_ec_map_error(msg->result);
if (mapped) {
ret = mapped;
goto exit;
}
if (ret == 0) {
ret = -EPROTO;
goto exit;
}
....
> mapped = cros_ec_map_error(msg->result);
> if (mapped) {
> ret = mapped;
> - goto exit;
> + } else {
> + r = (struct ec_response_host_event_mask *)msg->data;
> + *mask = r->mask;
> }
> }
> - if (ret > 0) {
> - r = (struct ec_response_host_event_mask *)msg->data;
> - *mask = r->mask;
> - }
>
> -exit:
> kfree(msg);
> return ret;
> }
> @@ -499,7 +496,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>
> /* Get host event wake mask. */
> ret = get_host_event_wake_mask(ec_dev, &ec_dev->host_event_wake_mask);
> - if (ret < 0) {
> + if (ret <= 0) {
> /*
> * If the EC doesn't support EC_CMD_HOST_EVENT_GET_WAKE_MASK,
> * use a reasonable default. Note that we ignore various
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index f63196289f54..1ccc837b30cf 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -999,6 +999,136 @@ static void cros_ec_proto_test_query_all_default_wake_mask(struct kunit *test)
> }
> }
>
> +static void cros_ec_proto_test_query_all_default_wake_mask2(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 fill_protocol_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 fill_protocol_info() with passthru. */
> + {
> + mock = cros_kunit_ec_xfer_mock_addx(test, 0, EC_RES_INVALID_COMMAND,
> + sizeof(struct ec_response_get_protocol_info));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For get_host_command_version_mask() for MKBP. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test,
> + sizeof(struct ec_response_get_cmd_versions));
> + KUNIT_ASSERT_PTR_NE(test, mock, NULL);
> + }
> +
> + /* For get_host_command_version_mask() for host sleep v1. */
> + {
> + mock = cros_kunit_ec_xfer_mock_add(test,
> + sizeof(struct ec_response_get_cmd_versions));
> + 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 fill_protocol_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 fill_protocol_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 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 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 int cros_ec_proto_test_init(struct kunit *test)
> {
> struct cros_ec_proto_test_priv *priv;
> @@ -1051,6 +1181,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_sleep2),
> KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask),
> + KUNIT_CASE(cros_ec_proto_test_query_all_default_wake_mask2),
> {}
> };
>
> --
> 2.36.1.255.ge46751e96f-goog
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 13/13] platform/chrome: cros_ec_proto: fix get_host_event_wake_mask() returns
2022-06-06 16:14 ` Guenter Roeck
@ 2022-06-07 0:55 ` Tzung-Bi Shih
0 siblings, 0 replies; 35+ messages in thread
From: Tzung-Bi Shih @ 2022-06-07 0:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Benson Leung, Guenter Roeck, chrome-platform, linux-kernel
On Mon, Jun 06, 2022 at 09:14:26AM -0700, Guenter Roeck wrote:
> On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> > @@ -256,19 +256,16 @@ static int get_host_event_wake_mask(struct cros_ec_device *ec_dev, uint32_t *mas
> > msg->insize = sizeof(*r);
> >
> > ret = send_command(ec_dev, msg);
> > - if (ret >= 0) {
> > + if (ret > 0) {
>
> The idea here was to (potentially) return an error if ret == 0. This
> is no longer the case after this change. Instead, the caller has to
> check for ret == 0 and treat it as an error. I think it would make
> more sense to explicitly check for ret ==0, eg with something like
>
> mapped = cros_ec_map_error(msg->result);
> if (mapped) {
> ret = mapped;
> goto exit;
> }
> if (ret == 0) {
> ret = -EPROTO;
> goto exit;
> }
> ....
Ack, will fix in next version.
^ permalink raw reply [flat|nested] 35+ messages in thread