Re: [PATCH net-next v7 1/2] devlink: introduce framework for selftests

[Jiri Pirko <jiri@nvidia.com>]

Wed, Jul 27, 2022 at 08:37:18AM CEST, vikas.gupta@broadcom.com wrote:
>Add a framework for running selftests.
>Framework exposes devlink commands and test suite(s) to the user
>to execute and query the supported tests by the driver.
>
>Below are new entries in devlink_nl_ops
>devlink_nl_cmd_selftests_show_doit/dumpit: To query the supported
>selftests by the drivers.
>devlink_nl_cmd_selftests_run: To execute selftests. Users can
>provide a test mask for executing group tests or standalone tests.
>
>Documentation/networking/devlink/ path is already part of MAINTAINERS &
>the new files come under this path. Hence no update needed to the
>MAINTAINERS
>
>Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
>Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>

Note that if the patch changed since the person provided the review tag,
you should drop it. I'm pretty sure that you carry these tags from V1.

Looks good, couple small nits and I will be fine :)


>---
> .../networking/devlink/devlink-selftests.rst  |  38 +++
> include/net/devlink.h                         |  21 ++
> include/uapi/linux/devlink.h                  |  29 +++
> net/core/devlink.c                            | 216 ++++++++++++++++++
> 4 files changed, 304 insertions(+)
> create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
>
>diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst
>new file mode 100644
>index 000000000000..c0aa1f3aef0d
>--- /dev/null
>+++ b/Documentation/networking/devlink/devlink-selftests.rst
>@@ -0,0 +1,38 @@
>+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>+
>+=================
>+Devlink Selftests
>+=================
>+
>+The ``devlink-selftests`` API allows executing selftests on the device.
>+
>+Tests Mask
>+==========
>+The ``devlink-selftests`` command should be run with a mask indicating
>+the tests to be executed.
>+
>+Tests Description
>+=================
>+The following is a list of tests that drivers may execute.
>+
>+.. list-table:: List of tests
>+   :widths: 5 90
>+
>+   * - Name
>+     - Description
>+   * - ``DEVLINK_SELFTEST_FLASH``
>+     - Devices may have the firmware on non-volatile memory on the board, e.g.
>+       flash. This particular test helps to run a flash selftest on the device.
>+       Implementation of the test is left to the driver/firmware.
>+
>+example usage
>+-------------
>+
>+.. code:: shell
>+
>+    # Query selftests supported on the devlink device
>+    $ devlink dev selftests show DEV
>+    # Query selftests supported on all devlink devices
>+    $ devlink dev selftests show
>+    # Executes selftests on the device
>+    $ devlink dev selftests run DEV id flash
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 5bd3fac12e9e..b311055cc29a 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1509,6 +1509,27 @@ struct devlink_ops {
> 				    struct devlink_rate *parent,
> 				    void *priv_child, void *priv_parent,
> 				    struct netlink_ext_ack *extack);
>+	/**
>+	 * selftests_check() - queries if selftest is supported
>+	 * @devlink: devlink instance
>+	 * @test_id: test index
>+	 * @extack: extack for reporting error messages
>+	 *
>+	 * Return: true if test is supported by the driver
>+	 */
>+	bool (*selftest_check)(struct devlink *devlink, unsigned int test_id,

Should be just "id" to be consistent with the rest of the code.


>+			       struct netlink_ext_ack *extack);
>+	/**
>+	 * selftest_run() - Runs a selftest
>+	 * @devlink: devlink instance
>+	 * @test_id: test index
>+	 * @extack: extack for reporting error messages
>+	 *
>+	 * Return: status of the test
>+	 */
>+	enum devlink_selftest_status
>+	(*selftest_run)(struct devlink *devlink, unsigned int test_id,

Same here.


>+			struct netlink_ext_ack *extack);
> };
> 
> void *devlink_priv(struct devlink *devlink);
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 541321695f52..d6e2d3b76e47 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -136,6 +136,9 @@ enum devlink_command {
> 	DEVLINK_CMD_LINECARD_NEW,
> 	DEVLINK_CMD_LINECARD_DEL,
> 
>+	DEVLINK_CMD_SELFTESTS_GET,	/* can dump */
>+	DEVLINK_CMD_SELFTESTS_RUN,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -276,6 +279,30 @@ enum {
> #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> 
>+enum devlink_attr_selftest_id {
>+	DEVLINK_ATTR_SELFTEST_ID_UNSPEC,
>+	DEVLINK_ATTR_SELFTEST_ID_FLASH,	/* flag */
>+
>+	__DEVLINK_ATTR_SELFTEST_ID_MAX,
>+	DEVLINK_ATTR_SELFTEST_ID_MAX = __DEVLINK_ATTR_SELFTEST_ID_MAX - 1
>+};
>+
>+enum devlink_selftest_status {
>+	DEVLINK_SELFTEST_STATUS_SKIP,
>+	DEVLINK_SELFTEST_STATUS_PASS,
>+	DEVLINK_SELFTEST_STATUS_FAIL
>+};
>+
>+enum devlink_attr_selftest_result {
>+	DEVLINK_ATTR_SELFTEST_RESULT_UNSPEC,
>+	DEVLINK_ATTR_SELFTEST_RESULT,		/* nested */
>+	DEVLINK_ATTR_SELFTEST_RESULT_ID,	/* u32, enum devlink_attr_selftest_id */
>+	DEVLINK_ATTR_SELFTEST_RESULT_STATUS,	/* u8, enum devlink_selftest_status */
>+
>+	__DEVLINK_ATTR_SELFTEST_RESULT_MAX,
>+	DEVLINK_ATTR_SELFTEST_RESULT_MAX = __DEVLINK_ATTR_SELFTEST_RESULT_MAX - 1
>+};
>+
> /**
>  * enum devlink_trap_action - Packet trap action.
>  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
>@@ -578,6 +605,8 @@ enum devlink_attr {
> 
> 	DEVLINK_ATTR_NESTED_DEVLINK,		/* nested */
> 
>+	DEVLINK_ATTR_SELFTESTS_INFO,		/* nested */

Drop "INFO". There are no information, just data passed there and back.
"info" is odd. DEVLINK_ATTR_SELFTESTS is enough to carry the data there
and back.


>+
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 698b2d6e0ec7..a942b3c9223c 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -201,6 +201,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> 				 DEVLINK_PORT_FN_STATE_ACTIVE),
> };
> 
>+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_ATTR_SELFTEST_ID_MAX + 1] = {
>+	[DEVLINK_ATTR_SELFTEST_ID_FLASH] = { .type = NLA_FLAG },
>+};
>+
> static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> #define DEVLINK_REGISTERED XA_MARK_1
> 
>@@ -4827,6 +4831,206 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 	return ret;
> }
> 
>+static int
>+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
>+			  u32 portid, u32 seq, int flags,
>+			  struct netlink_ext_ack *extack)
>+{
>+	struct nlattr *selftests_list;
>+	void *hdr;
>+	int err;
>+	int i;
>+
>+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
>+			  DEVLINK_CMD_SELFTESTS_GET);
>+	if (!hdr)
>+		return -EMSGSIZE;
>+
>+	err = -EMSGSIZE;
>+	if (devlink_nl_put_handle(msg, devlink))
>+		goto err_cancel_msg;
>+
>+	selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+	if (!selftests_list)
>+		goto err_cancel_msg;
>+
>+	for (i = DEVLINK_ATTR_SELFTEST_ID_UNSPEC + 1;
>+	     i <= DEVLINK_ATTR_SELFTEST_ID_MAX; i++) {
>+		if (devlink->ops->selftest_check(devlink, i, extack)) {
>+			err = nla_put_flag(msg, i);
>+			if (err)
>+				goto err_cancel_msg;
>+		}
>+	}
>+
>+	nla_nest_end(msg, selftests_list);
>+	genlmsg_end(msg, hdr);
>+	return 0;
>+
>+err_cancel_msg:
>+	genlmsg_cancel(msg, hdr);
>+	return err;
>+}
>+
>+static int devlink_nl_cmd_selftests_get_doit(struct sk_buff *skb,
>+					     struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	int err;
>+
>+	if (!devlink->ops->selftest_check)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
>+					info->snd_seq, 0, info->extack);
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
>+static int devlink_nl_cmd_selftests_get_dumpit(struct sk_buff *msg,
>+					       struct netlink_callback *cb)
>+{
>+	struct devlink *devlink;
>+	int start = cb->args[0];
>+	unsigned long index;
>+	int idx = 0;
>+	int err = 0;
>+
>+	mutex_lock(&devlink_mutex);
>+	devlinks_xa_for_each_registered_get(sock_net(msg->sk), index, devlink) {
>+		if (idx < start || !devlink->ops->selftest_check)
>+			goto inc;
>+
>+		devl_lock(devlink);
>+		err = devlink_nl_selftests_fill(msg, devlink,
>+						NETLINK_CB(cb->skb).portid,
>+						cb->nlh->nlmsg_seq, NLM_F_MULTI,
>+						cb->extack);
>+		devl_unlock(devlink);
>+		if (err) {
>+			devlink_put(devlink);
>+			break;
>+		}
>+inc:
>+		idx++;
>+		devlink_put(devlink);
>+	}
>+	mutex_unlock(&devlink_mutex);
>+
>+	if (err != -EMSGSIZE)
>+		return err;
>+
>+	cb->args[0] = idx;
>+	return msg->len;
>+}
>+
>+static int devlink_selftest_result_put(struct sk_buff *skb, unsigned int test_id,
>+				       enum devlink_selftest_status test_status)
>+{
>+	struct nlattr *result_attr;
>+
>+	result_attr = nla_nest_start(skb, DEVLINK_ATTR_SELFTEST_RESULT);
>+	if (!result_attr)
>+		return -EMSGSIZE;
>+
>+	if (nla_put_u32(skb, DEVLINK_ATTR_SELFTEST_RESULT_ID, test_id) ||
>+	    nla_put_u8(skb, DEVLINK_ATTR_SELFTEST_RESULT_STATUS,
>+		       test_status))
>+		goto nla_put_failure;
>+
>+	nla_nest_end(skb, result_attr);
>+	return 0;
>+
>+nla_put_failure:
>+	nla_nest_cancel(skb, result_attr);
>+	return -EMSGSIZE;
>+}
>+
>+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
>+					struct genl_info *info)
>+{
>+	struct nlattr *tb[DEVLINK_ATTR_SELFTEST_ID_MAX + 1];
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct nlattr *attrs, *tests_info;
>+	struct sk_buff *msg;
>+	void *hdr;
>+	int err;
>+	int i;
>+
>+	if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
>+		return -EOPNOTSUPP;
>+
>+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO]) {
>+		NL_SET_ERR_MSG_MOD(info->extack, "selftest information required");

no "information"


>+		return -EINVAL;
>+	}
>+
>+	attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
>+
>+	err = nla_parse_nested(tb, DEVLINK_ATTR_SELFTEST_ID_MAX, attrs,
>+			       devlink_selftest_nl_policy, info->extack);
>+	if (err < 0)
>+		return err;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = -EMSGSIZE;
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
>+	if (!hdr)
>+		goto free_msg;
>+
>+	if (devlink_nl_put_handle(msg, devlink))
>+		goto genlmsg_cancel;
>+
>+	tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+	if (!tests_info)
>+		goto genlmsg_cancel;
>+
>+	for (i = DEVLINK_ATTR_SELFTEST_ID_UNSPEC + 1;
>+	     i <= DEVLINK_ATTR_SELFTEST_ID_MAX; i++) {
>+		enum devlink_selftest_status test_status;
>+
>+		if (nla_get_flag(tb[i])) {
>+			if (!devlink->ops->selftest_check(devlink, i,
>+							  info->extack)) {
>+				if (devlink_selftest_result_put(msg, i,
>+								DEVLINK_SELFTEST_STATUS_SKIP))
>+					goto selftests_info_nest_cancel;
>+				continue;
>+			}
>+
>+			test_status = devlink->ops->selftest_run(devlink, i,
>+								 info->extack);
>+			if (devlink_selftest_result_put(msg, i, test_status))
>+				goto selftests_info_nest_cancel;
>+		}
>+	}
>+
>+	nla_nest_end(msg, tests_info);
>+	genlmsg_end(msg, hdr);
>+	return genlmsg_reply(msg, info);
>+
>+selftests_info_nest_cancel:
>+	nla_nest_cancel(msg, tests_info);
>+genlmsg_cancel:
>+	genlmsg_cancel(msg, hdr);
>+free_msg:
>+	nlmsg_free(msg);
>+	return err;
>+}
>+
> static const struct devlink_param devlink_param_generic[] = {
> 	{
> 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
>@@ -8970,6 +9174,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> 	[DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
>+	[DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
> };
> 
> static const struct genl_small_ops devlink_nl_ops[] = {
>@@ -9329,6 +9534,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> 		.doit = devlink_nl_cmd_trap_policer_set_doit,
> 		.flags = GENL_ADMIN_PERM,
> 	},
>+	{
>+		.cmd = DEVLINK_CMD_SELFTESTS_GET,
>+		.doit = devlink_nl_cmd_selftests_get_doit,
>+		.dumpit = devlink_nl_cmd_selftests_get_dumpit
>+		/* can be retrieved by unprivileged users */
>+	},
>+	{
>+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
>+		.doit = devlink_nl_cmd_selftests_run,
>+		.flags = GENL_ADMIN_PERM,
>+	},
> };
> 
> static struct genl_family devlink_nl_family __ro_after_init = {
>-- 
>2.31.1
>