[PATCH net-next v7 0/2] add framework for selftests in devlink

[PATCH net-next v7 0/2] add framework for selftests in devlink

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

Hi,
  This patchset adds support for selftests in the devlink framework.
  It adds a callback .selftests_check and .selftests_run in devlink_ops.
  User can add test(s) suite which is subsequently passed to the driver 
  and driver can opt for running particular tests based on its capabilities.

  Patchset adds a flash based test for the bnxt_en driver.

  Suggested commands at user level would be as below:

changes from:
v6->v7:
  1) Changed the macros/enums name as suggested by kuba@kernel.org.
  2) Rebase with latest net-next. Dumpit impacted due to recent patches
     by jiri@nvidia.com.
  net: devlink: make sure that devlink_try_get() works with valid pointer during xarray iteration
  net: devlink: move net check into devlinks_xa_for_each_registered_get()

v5->v6:
  Addressed change in .rst file only suggested by jiri@nvidia.com in patch v5.
   
v4->v5:
  Addressed the changes requested by jiri@nvidia.com in patch v4.

v3->v4:
  Addressed the changes requested by kuba@kernel.org in patch v3.

v2->v3:
   1)
   After discussions with jiri@nvidia.com, passing a testmask from
   user to kernel is removed and a flag based arguments are adopted.
   This way we can have more than 32/64 selftests defined in the
   kernel.
   Below is the format from user to kernel and vice-versa.
   
   Kernel to user for show command . Users can know what all tests are
   supported by the driver. A return from kernel to user if driver
   supports TEST1, TEST4, and TEST7.
	______
	|NEST |
	|_____ |TEST1|TEST4|TEST7|...


    User to kernel to execute test: If user wants to execute test4, test8,
	test1...
	______
	|NEST |
	|_____ |TEST4|TEST8|TEST1|...

	After executing the tests kernel return to user.
	|NEST |
	|_____ | NEST|       |NEST|       |NEST|
	        TEST4,RES4   TEST8,RES8   TEST1, RES1
    
    2) Added dumpit in devlink for list/show command.

v1->v2:
  Addressed the changes requested by kuba@kernel.org in patch v1.
  Fixed the style issues. 


Thanks,
Vikas


Vikas Gupta (1):
  devlink: introduce framework for selftests

vikas (1):
  bnxt_en: implement callbacks for devlink selftests

 .../networking/devlink/devlink-selftests.rst  |  38 +++
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  61 +++++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  24 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  12 +
 include/net/devlink.h                         |  21 ++
 include/uapi/linux/devlink.h                  |  29 +++
 net/core/devlink.c                            | 216 ++++++++++++++++++
 7 files changed, 389 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/networking/devlink/devlink-selftests.rst

-- 
2.31.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

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

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 11838 bytes --]

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>
---
 .../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,
+			       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,
+			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 */
+
 	/* 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");
+		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


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

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

[Jiri Pirko]

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
>

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

[Andy Gospodarek]

[-- Attachment #1: Type: text/plain, Size: 13923 bytes --]

On Wed, Jul 27, 2022 at 09:26:48AM +0200, Jiri Pirko wrote:
> 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.

To give Vikas credit on this, I think most (if not all) were reviewed
internally before sending out to the list, so it wasn't a total cut and
paste of these for each revision.  :)

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

Great!  Thanks for the detailed review.

> 
> 
> >---
> > .../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
> >
> 
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]

[PATCH net-next v7 2/2] bnxt_en: implement callbacks for devlink selftests

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 6382 bytes --]

From: vikas <vikas.gupta@broadcom.com>

Add callbacks
=============
.selftest_check: returns true for flash selftest.
.selftest_run: runs a flash selftest.

Also, refactor NVM APIs so that they can be
used with devlink and ethtool both.

Signed-off-by: vikas <vikas.gupta@broadcom.com>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 61 +++++++++++++++++++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 24 ++++----
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 12 ++++
 3 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 6b3d4f4c2a75..ec22b8da9f6f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -20,6 +20,8 @@
 #include "bnxt_ulp.h"
 #include "bnxt_ptp.h"
 #include "bnxt_coredump.h"
+#include "bnxt_nvm_defs.h"
+#include "bnxt_ethtool.h"
 
 static void __bnxt_fw_recover(struct bnxt *bp)
 {
@@ -610,6 +612,63 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
 	return rc;
 }
 
+static bool bnxt_nvm_test(struct bnxt *bp, struct netlink_ext_ack *extack)
+{
+	u32 datalen;
+	u16 index;
+	u8 *buf;
+
+	if (bnxt_find_nvram_item(bp->dev, BNX_DIR_TYPE_VPD,
+				 BNX_DIR_ORDINAL_FIRST, BNX_DIR_EXT_NONE,
+				 &index, NULL, &datalen) || !datalen) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd entry error");
+		return false;
+	}
+
+	buf = kzalloc(datalen, GFP_KERNEL);
+	if (!buf) {
+		NL_SET_ERR_MSG_MOD(extack, "insufficient memory for nvm test");
+		return false;
+	}
+
+	if (bnxt_get_nvram_item(bp->dev, index, 0, datalen, buf)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd read error");
+		goto err;
+	}
+
+	if (bnxt_flash_nvram(bp->dev, BNX_DIR_TYPE_VPD, BNX_DIR_ORDINAL_FIRST,
+			     BNX_DIR_EXT_NONE, 0, 0, buf, datalen)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd write error");
+		goto err;
+	}
+
+	return true;
+
+err:
+	kfree(buf);
+	return false;
+}
+
+static bool bnxt_dl_selftest_check(struct devlink *dl, unsigned int test_id,
+				   struct netlink_ext_ack *extack)
+{
+	return test_id == DEVLINK_ATTR_SELFTEST_ID_FLASH;
+}
+
+static enum devlink_selftest_status bnxt_dl_selftest_run(struct devlink *dl,
+							 unsigned int test_id,
+							 struct netlink_ext_ack *extack)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (test_id == DEVLINK_ATTR_SELFTEST_ID_FLASH)
+		return bnxt_nvm_test(bp, extack) ?
+				DEVLINK_SELFTEST_STATUS_PASS :
+				DEVLINK_SELFTEST_STATUS_FAIL;
+
+	return DEVLINK_SELFTEST_STATUS_SKIP;
+}
+
 static const struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
@@ -622,6 +681,8 @@ static const struct devlink_ops bnxt_dl_ops = {
 	.reload_limits	  = BIT(DEVLINK_RELOAD_LIMIT_NO_RESET),
 	.reload_down	  = bnxt_dl_reload_down,
 	.reload_up	  = bnxt_dl_reload_up,
+	.selftest_check	  = bnxt_dl_selftest_check,
+	.selftest_run	  = bnxt_dl_selftest_run,
 };
 
 static const struct devlink_ops bnxt_vf_dl_ops;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 7191e5d74208..87eb5362ad70 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2176,14 +2176,14 @@ static void bnxt_print_admin_err(struct bnxt *bp)
 	netdev_info(bp->dev, "PF does not have admin privileges to flash or reset the device\n");
 }
 
-static int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
-				u16 ext, u16 *index, u32 *item_length,
-				u32 *data_length);
+int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
+			 u16 ext, u16 *index, u32 *item_length,
+			 u32 *data_length);
 
-static int bnxt_flash_nvram(struct net_device *dev, u16 dir_type,
-			    u16 dir_ordinal, u16 dir_ext, u16 dir_attr,
-			    u32 dir_item_len, const u8 *data,
-			    size_t data_len)
+int bnxt_flash_nvram(struct net_device *dev, u16 dir_type,
+		     u16 dir_ordinal, u16 dir_ext, u16 dir_attr,
+		     u32 dir_item_len, const u8 *data,
+		     size_t data_len)
 {
 	struct bnxt *bp = netdev_priv(dev);
 	struct hwrm_nvm_write_input *req;
@@ -2836,8 +2836,8 @@ static int bnxt_get_nvram_directory(struct net_device *dev, u32 len, u8 *data)
 	return rc;
 }
 
-static int bnxt_get_nvram_item(struct net_device *dev, u32 index, u32 offset,
-			       u32 length, u8 *data)
+int bnxt_get_nvram_item(struct net_device *dev, u32 index, u32 offset,
+			u32 length, u8 *data)
 {
 	struct bnxt *bp = netdev_priv(dev);
 	int rc;
@@ -2871,9 +2871,9 @@ static int bnxt_get_nvram_item(struct net_device *dev, u32 index, u32 offset,
 	return rc;
 }
 
-static int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
-				u16 ext, u16 *index, u32 *item_length,
-				u32 *data_length)
+int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
+			 u16 ext, u16 *index, u32 *item_length,
+			 u32 *data_length)
 {
 	struct hwrm_nvm_find_dir_entry_output *output;
 	struct hwrm_nvm_find_dir_entry_input *req;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
index a59284215e78..a8ecef8ab82c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h
@@ -58,5 +58,17 @@ int bnxt_flash_package_from_fw_obj(struct net_device *dev, const struct firmware
 int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size);
 void bnxt_ethtool_init(struct bnxt *bp);
 void bnxt_ethtool_free(struct bnxt *bp);
+int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
+			 u16 ext, u16 *index, u32 *item_length,
+			 u32 *data_length);
+int bnxt_find_nvram_item(struct net_device *dev, u16 type, u16 ordinal,
+			 u16 ext, u16 *index, u32 *item_length,
+			 u32 *data_length);
+int bnxt_flash_nvram(struct net_device *dev, u16 dir_type,
+		     u16 dir_ordinal, u16 dir_ext, u16 dir_attr,
+		     u32 dir_item_len, const u8 *data,
+		     size_t data_len);
+int bnxt_get_nvram_item(struct net_device *dev, u32 index, u32 offset,
+			u32 length, u8 *data);
 
 #endif
-- 
2.31.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]