[PATCH net-next v2 0/3] add framework for selftests in devlink

[PATCH net-next v2 0/3] add framework for selftests in devlink

[Vikas Gupta]

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

Hi,
  This patchset adds support for selftests in the devlink framework.
  It adds a callback .selftests_show and .selftests_run in devlink_ops. 
  User can provide test(s) suite as a testmask and subsequently it is passed 
  to the driver which 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:
v1->v2:
  Addressed the changes requested by kuba@kernel.org in patch v1.
  Fixed the style issues. 


Thanks,
Vikas


Vikas Gupta (3):
  devlink: introduce framework for selftests
  bnxt_en: refactor NVM APIs
  bnxt_en: implement callbacks for devlink selftests

 .../networking/devlink/devlink-selftests.rst  |  34 +++++
 .../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                         |  30 ++++
 include/uapi/linux/devlink.h                  |  26 ++++
 net/core/devlink.c                            | 144 ++++++++++++++++++
 7 files changed, 319 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 v2 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 9524 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: To query the supported selftests
by the driver.
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  |  34 +++++
 include/net/devlink.h                         |  30 ++++
 include/uapi/linux/devlink.h                  |  26 ++++
 net/core/devlink.c                            | 144 ++++++++++++++++++
 4 files changed, 234 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..796d38f77038
--- /dev/null
+++ b/Documentation/networking/devlink/devlink-selftests.rst
@@ -0,0 +1,34 @@
+.. 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``
+     - Runs a flash test on the device.
+
+example usage
+-------------
+
+.. code:: shell
+
+    # Query selftests supported on the device
+    $ devlink dev selftests show DEV
+    # Executes selftests on the device
+    $ devlink dev selftests run DEV test {flash | all}
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 2a2a2a0c93f7..cb7c378cf720 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1215,6 +1215,18 @@ enum {
 	DEVLINK_F_RELOAD = 1UL << 0,
 };
 
+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
+
+static inline const char *devlink_selftest_name(int test)
+{
+	switch (test) {
+	case DEVLINK_SELFTEST_FLASH_BIT:
+		return DEVLINK_SELFTEST_FLASH_TEST_NAME;
+	default:
+		return "unknown";
+	}
+}
+
 struct devlink_ops {
 	/**
 	 * @supported_flash_update_params:
@@ -1509,6 +1521,24 @@ struct devlink_ops {
 				    struct devlink_rate *parent,
 				    void *priv_child, void *priv_parent,
 				    struct netlink_ext_ack *extack);
+	/**
+	 * selftests_show() - Shows selftests supported by device
+	 * @devlink: Devlink instance
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: test mask supported by driver
+	 */
+	u32 (*selftests_show)(struct devlink *devlink,
+			      struct netlink_ext_ack *extack);
+	/**
+	 * selftests_run() - Runs selftests
+	 * @devlink: Devlink instance
+	 * @tests_mask: tests to be run by driver
+	 * @results: test results by driver
+	 * @extack: extack for reporting error messages
+	 */
+	void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
+			      u8 *results, 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 b3d40a5d72ff..1dba262328b9 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_SHOW,
+	DEVLINK_CMD_SELFTESTS_RUN,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -276,6 +279,25 @@ enum {
 #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
 
+/* Commonly used test cases */
+enum {
+	DEVLINK_SELFTEST_FLASH_BIT,
+
+	__DEVLINK_SELFTEST_MAX_BIT,
+	DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
+};
+
+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
+
+#define DEVLINK_SELFTESTS_MASK \
+	(_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
+
+enum {
+	DEVLINK_SELFTEST_SKIP,
+	DEVLINK_SELFTEST_PASS,
+	DEVLINK_SELFTEST_FAIL
+};
+
 /**
  * enum devlink_trap_action - Packet trap action.
  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -576,6 +598,10 @@ enum devlink_attr {
 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
 
+	DEVLINK_ATTR_SELFTESTS_MASK,		/* u32 */
+	DEVLINK_ATTR_TEST_RESULT,		/* nested */
+	DEVLINK_ATTR_TEST_NAME,			/* string */
+	DEVLINK_ATTR_TEST_RESULT_VAL,		/* u8 */
 	/* 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 db61f3a341cb..0b7341ab6379 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	return ret;
 }
 
+static int devlink_selftest_name_put(struct sk_buff *skb, int test)
+{
+	const char *name = devlink_selftest_name(test);
+	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
+					 struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	unsigned long tests;
+	int err = 0;
+	void *hdr;
+	int test;
+
+	if (!devlink->ops->selftests_show)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_SHOW);
+	if (!hdr)
+		goto free_msg;
+
+	if (devlink_nl_put_handle(msg, devlink))
+		goto genlmsg_cancel;
+
+	tests = devlink->ops->selftests_show(devlink, info->extack);
+
+	for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
+		err = devlink_selftest_name_put(msg, test);
+		if (err)
+			goto genlmsg_cancel;
+	}
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, info);
+
+genlmsg_cancel:
+	genlmsg_cancel(msg, hdr);
+free_msg:
+	nlmsg_free(msg);
+	return err;
+}
+
+static int devlink_selftest_result_put(struct sk_buff *skb, int test,
+				       u8 result)
+{
+	const char *name = devlink_selftest_name(test);
+	struct nlattr *result_attr;
+
+	result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);
+	if (!result_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
+	    nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
+		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)
+{
+	u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
+	struct devlink *devlink = info->user_ptr[0];
+	unsigned long tests;
+	struct sk_buff *msg;
+	u32 tests_mask;
+	void *hdr;
+	int err = 0;
+	int test;
+
+	if (!devlink->ops->selftests_run)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
+		return -EINVAL;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	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_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
+
+	devlink->ops->selftests_run(devlink, tests_mask, test_results,
+				    info->extack);
+	tests = tests_mask;
+
+	for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
+		err = devlink_selftest_result_put(msg, test,
+						  test_results[test]);
+		if (err)
+			goto genlmsg_cancel;
+	}
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, 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,
@@ -9000,6 +9130,8 @@ 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_MASK] = NLA_POLICY_MASK(NLA_U32,
+							DEVLINK_SELFTESTS_MASK),
 };
 
 static const struct genl_small_ops devlink_nl_ops[] = {
@@ -9361,6 +9493,18 @@ 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_SHOW,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = devlink_nl_cmd_selftests_show,
+		.flags = GENL_ADMIN_PERM,
+	},
+	{
+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.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 v2 1/3] devlink: introduce framework for selftests

[Jakub Kicinski]

On Thu,  7 Jul 2022 23:59:48 +0530 Vikas Gupta wrote:
> +   * - Name
> +     - Description
> +   * - ``DEVLINK_SELFTEST_FLASH``
> +     - Runs a flash test on the device.

A little more info on what "flash test" does would be useful.

> +	DEVLINK_CMD_SELFTESTS_SHOW,

nit: _LIST?

>  /**
>   * enum devlink_trap_action - Packet trap action.
>   * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
> @@ -576,6 +598,10 @@ enum devlink_attr {
>  	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
>  	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
>  
> +	DEVLINK_ATTR_SELFTESTS_MASK,		/* u32 */

Can we make the uAPI field 64b just in case we need more bits?
Internally we can keep using u32 doesn't matter.

> +	DEVLINK_ATTR_TEST_RESULT,		/* nested */
> +	DEVLINK_ATTR_TEST_NAME,			/* string */
> +	DEVLINK_ATTR_TEST_RESULT_VAL,		/* u8 */
>  	/* 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 db61f3a341cb..0b7341ab6379 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
>  	return ret;
>  }
>  
> +static int devlink_selftest_name_put(struct sk_buff *skb, int test)
> +{
> +	const char *name = devlink_selftest_name(test);

empty line

> +	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}

This wrapper feels slightly unnecessary, it's only used once AFAICT.

Of you want to keep it you should compress it to one stmt:

static int devlink_selftest_name_put(struct sk_buff *skb, int test)
{
	return nla_put_string(skb, DEVLINK_ATTR_TEST_NAME,
			      devlink_selftest_name(test)));
}

> +static int devlink_selftest_result_put(struct sk_buff *skb, int test,
> +				       u8 result)
> +{
> +	const char *name = devlink_selftest_name(test);
> +	struct nlattr *result_attr;
> +
> +	result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);

I think we can use the normal (non-_noflag) nests in new devlink code.

> +	if (!result_attr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
> +	    nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
> +		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)
> +{
> +	u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
> +	struct devlink *devlink = info->user_ptr[0];
> +	unsigned long tests;
> +	struct sk_buff *msg;
> +	u32 tests_mask;
> +	void *hdr;
> +	int err = 0;

reverse xmas tree, but you probably don't want this init

> +	int test;
> +
> +	if (!devlink->ops->selftests_run)
> +		return -EOPNOTSUPP;
> +
> +	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
> +		return -EINVAL;
> +
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> +			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
> +	if (!hdr)
> +		goto free_msg;

err is not set here

> +	if (devlink_nl_put_handle(msg, devlink))
> +		goto genlmsg_cancel;

or here

> +	tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
> +
> +	devlink->ops->selftests_run(devlink, tests_mask, test_results,
> +				    info->extack);
> +	tests = tests_mask;
> +
> +	for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
> +		err = devlink_selftest_result_put(msg, test,
> +						  test_results[test]);
> +		if (err)
> +			goto genlmsg_cancel;
> +	}
> +
> +	genlmsg_end(msg, hdr);
> +
> +	return genlmsg_reply(msg, 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,
> @@ -9000,6 +9130,8 @@ 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_MASK] = NLA_POLICY_MASK(NLA_U32,
> +							DEVLINK_SELFTESTS_MASK),
>  };
>  
>  static const struct genl_small_ops devlink_nl_ops[] = {
> @@ -9361,6 +9493,18 @@ 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_SHOW,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,

I think we can validate strict for new commands, so no validation flags
needed.

> +		.doit = devlink_nl_cmd_selftests_show,

What about dump? Listing all tests on all devices?

> +		.flags = GENL_ADMIN_PERM,
> +	},
> +	{
> +		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = devlink_nl_cmd_selftests_run,
> +		.flags = GENL_ADMIN_PERM,
> +	},
>  };
>  
>  static struct genl_family devlink_nl_family __ro_after_init = {

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Ido Schimmel]

On Thu, Jul 07, 2022 at 06:20:22PM -0700, Jakub Kicinski wrote:
> On Thu,  7 Jul 2022 23:59:48 +0530 Vikas Gupta wrote:
> >  static const struct genl_small_ops devlink_nl_ops[] = {
> > @@ -9361,6 +9493,18 @@ 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_SHOW,
> > +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> 
> I think we can validate strict for new commands, so no validation flags
> needed.
> 
> > +		.doit = devlink_nl_cmd_selftests_show,
> 
> What about dump? Listing all tests on all devices?
> 
> > +		.flags = GENL_ADMIN_PERM,

Related to Jakub's question, is there a reason that the show command
requires 'GENL_ADMIN_PERM' ?

> > +	},
> > +	{
> > +		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
> > +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +		.doit = devlink_nl_cmd_selftests_run,
> > +		.flags = GENL_ADMIN_PERM,
> > +	},
> >  };
> >  
> >  static struct genl_family devlink_nl_family __ro_after_init = {
> 

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[kernel test robot]

Hi Vikas,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Vikas-Gupta/devlink-introduce-framework-for-selftests/20220708-033020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cf21b355ccb39b0de0b6a7362532bb5584c84a80
reproduce: make htmldocs

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> Documentation/networking/devlink/devlink-selftests.rst: WARNING: document isn't included in any toctree

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Jiri Pirko]

Thu, Jul 07, 2022 at 08:29:48PM 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: To query the supported selftests
>by the driver.
>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  |  34 +++++
> include/net/devlink.h                         |  30 ++++
> include/uapi/linux/devlink.h                  |  26 ++++
> net/core/devlink.c                            | 144 ++++++++++++++++++
> 4 files changed, 234 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..796d38f77038
>--- /dev/null
>+++ b/Documentation/networking/devlink/devlink-selftests.rst
>@@ -0,0 +1,34 @@
>+.. 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``
>+     - Runs a flash test on the device.
>+
>+example usage
>+-------------
>+
>+.. code:: shell
>+
>+    # Query selftests supported on the device
>+    $ devlink dev selftests show DEV
>+    # Executes selftests on the device
>+    $ devlink dev selftests run DEV test {flash | all}
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 2a2a2a0c93f7..cb7c378cf720 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1215,6 +1215,18 @@ enum {
> 	DEVLINK_F_RELOAD = 1UL << 0,
> };
> 
>+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
>+
>+static inline const char *devlink_selftest_name(int test)

I don't understand why this is needed. Better not to expose string to
the user. Just have it as well defined attr.


>+{
>+	switch (test) {
>+	case DEVLINK_SELFTEST_FLASH_BIT:
>+		return DEVLINK_SELFTEST_FLASH_TEST_NAME;
>+	default:
>+		return "unknown";
>+	}
>+}
>+
> struct devlink_ops {
> 	/**
> 	 * @supported_flash_update_params:
>@@ -1509,6 +1521,24 @@ struct devlink_ops {
> 				    struct devlink_rate *parent,
> 				    void *priv_child, void *priv_parent,
> 				    struct netlink_ext_ack *extack);
>+	/**
>+	 * selftests_show() - Shows selftests supported by device
>+	 * @devlink: Devlink instance
>+	 * @extack: extack for reporting error messages
>+	 *
>+	 * Return: test mask supported by driver
>+	 */
>+	u32 (*selftests_show)(struct devlink *devlink,
>+			      struct netlink_ext_ack *extack);
>+	/**
>+	 * selftests_run() - Runs selftests
>+	 * @devlink: Devlink instance
>+	 * @tests_mask: tests to be run by driver
>+	 * @results: test results by driver
>+	 * @extack: extack for reporting error messages
>+	 */
>+	void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
>+			      u8 *results, 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 b3d40a5d72ff..1dba262328b9 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_SHOW,
>+	DEVLINK_CMD_SELFTESTS_RUN,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -276,6 +279,25 @@ enum {
> #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> 
>+/* Commonly used test cases */
>+enum {
>+	DEVLINK_SELFTEST_FLASH_BIT,
>+
>+	__DEVLINK_SELFTEST_MAX_BIT,
>+	DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
>+};
>+
>+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
>+
>+#define DEVLINK_SELFTESTS_MASK \
>+	(_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
>+
>+enum {
>+	DEVLINK_SELFTEST_SKIP,
>+	DEVLINK_SELFTEST_PASS,
>+	DEVLINK_SELFTEST_FAIL
>+};
>+
> /**
>  * enum devlink_trap_action - Packet trap action.
>  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
>@@ -576,6 +598,10 @@ enum devlink_attr {
> 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
> 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
> 
>+	DEVLINK_ATTR_SELFTESTS_MASK,		/* u32 */

I don't see why this is u32 bitset. Just have one attr per test
(NLA_FLAG) in a nested attr instead.



>+	DEVLINK_ATTR_TEST_RESULT,		/* nested */
>+	DEVLINK_ATTR_TEST_NAME,			/* string */
>+	DEVLINK_ATTR_TEST_RESULT_VAL,		/* u8 */

Could you maintain the same "namespace" for all attrs related to
selftests?


> 	/* 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 db61f3a341cb..0b7341ab6379 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 	return ret;
> }
> 
>+static int devlink_selftest_name_put(struct sk_buff *skb, int test)
>+{
>+	const char *name = devlink_selftest_name(test);
>+	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
>+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
>+					 struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	unsigned long tests;
>+	int err = 0;
>+	void *hdr;
>+	int test;
>+
>+	if (!devlink->ops->selftests_show)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_SHOW);
>+	if (!hdr)
>+		goto free_msg;
>+
>+	if (devlink_nl_put_handle(msg, devlink))
>+		goto genlmsg_cancel;
>+
>+	tests = devlink->ops->selftests_show(devlink, info->extack);
>+
>+	for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
>+		err = devlink_selftest_name_put(msg, test);
>+		if (err)
>+			goto genlmsg_cancel;
>+	}
>+
>+	genlmsg_end(msg, hdr);
>+
>+	return genlmsg_reply(msg, info);
>+
>+genlmsg_cancel:
>+	genlmsg_cancel(msg, hdr);
>+free_msg:
>+	nlmsg_free(msg);
>+	return err;
>+}
>+
>+static int devlink_selftest_result_put(struct sk_buff *skb, int test,
>+				       u8 result)
>+{
>+	const char *name = devlink_selftest_name(test);
>+	struct nlattr *result_attr;
>+
>+	result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);
>+	if (!result_attr)
>+		return -EMSGSIZE;
>+
>+	if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
>+	    nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
>+		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)
>+{
>+	u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
>+	struct devlink *devlink = info->user_ptr[0];
>+	unsigned long tests;
>+	struct sk_buff *msg;
>+	u32 tests_mask;
>+	void *hdr;
>+	int err = 0;
>+	int test;
>+
>+	if (!devlink->ops->selftests_run)
>+		return -EOPNOTSUPP;
>+
>+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
>+		return -EINVAL;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	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_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
>+
>+	devlink->ops->selftests_run(devlink, tests_mask, test_results,

Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too?


>+				    info->extack);
>+	tests = tests_mask;
>+
>+	for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
>+		err = devlink_selftest_result_put(msg, test,
>+						  test_results[test]);
>+		if (err)
>+			goto genlmsg_cancel;
>+	}
>+
>+	genlmsg_end(msg, hdr);
>+
>+	return genlmsg_reply(msg, 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,
>@@ -9000,6 +9130,8 @@ 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_MASK] = NLA_POLICY_MASK(NLA_U32,
>+							DEVLINK_SELFTESTS_MASK),
> };
> 
> static const struct genl_small_ops devlink_nl_ops[] = {
>@@ -9361,6 +9493,18 @@ 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_SHOW,
>+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>+		.doit = devlink_nl_cmd_selftests_show,

Why don't dump?


>+		.flags = GENL_ADMIN_PERM,
>+	},
>+	{
>+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
>+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
>+		.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 v2 1/3] devlink: introduce framework for selftests

[Jiri Pirko]

Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
>Hi Jiri,
>
>On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
>
>> Thu, Jul 07, 2022 at 08:29:48PM 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: To query the supported selftests
>> >by the driver.
>> >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  |  34 +++++
>> > include/net/devlink.h                         |  30 ++++
>> > include/uapi/linux/devlink.h                  |  26 ++++
>> > net/core/devlink.c                            | 144 ++++++++++++++++++
>> > 4 files changed, 234 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..796d38f77038
>> >--- /dev/null
>> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
>> >@@ -0,0 +1,34 @@
>> >+.. 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``
>> >+     - Runs a flash test on the device.
>> >+
>> >+example usage
>> >+-------------
>> >+
>> >+.. code:: shell
>> >+
>> >+    # Query selftests supported on the device
>> >+    $ devlink dev selftests show DEV
>> >+    # Executes selftests on the device
>> >+    $ devlink dev selftests run DEV test {flash | all}
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 2a2a2a0c93f7..cb7c378cf720 100644
>> >--- a/include/net/devlink.h
>> >+++ b/include/net/devlink.h
>> >@@ -1215,6 +1215,18 @@ enum {
>> >       DEVLINK_F_RELOAD = 1UL << 0,
>> > };
>> >
>> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
>> >+
>> >+static inline const char *devlink_selftest_name(int test)
>>
>> I don't understand why this is needed. Better not to expose string to
>> the user. Just have it as well defined attr.
>
>
> OK. Will remove this function and corresponding attr
>DEVLINK_ATTR_TEST_NAME added in this patch.
>
>
>
>
>>
>> >+{
>> >+      switch (test) {
>> >+      case DEVLINK_SELFTEST_FLASH_BIT:
>> >+              return DEVLINK_SELFTEST_FLASH_TEST_NAME;
>> >+      default:
>> >+              return "unknown";
>> >+      }
>> >+}
>> >+
>> > struct devlink_ops {
>> >       /**
>> >        * @supported_flash_update_params:
>> >@@ -1509,6 +1521,24 @@ struct devlink_ops {
>> >                                   struct devlink_rate *parent,
>> >                                   void *priv_child, void *priv_parent,
>> >                                   struct netlink_ext_ack *extack);
>> >+      /**
>> >+       * selftests_show() - Shows selftests supported by device
>> >+       * @devlink: Devlink instance
>> >+       * @extack: extack for reporting error messages
>> >+       *
>> >+       * Return: test mask supported by driver
>> >+       */
>> >+      u32 (*selftests_show)(struct devlink *devlink,
>> >+                            struct netlink_ext_ack *extack);
>> >+      /**
>> >+       * selftests_run() - Runs selftests
>> >+       * @devlink: Devlink instance
>> >+       * @tests_mask: tests to be run by driver
>> >+       * @results: test results by driver
>> >+       * @extack: extack for reporting error messages
>> >+       */
>> >+      void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
>> >+                            u8 *results, 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 b3d40a5d72ff..1dba262328b9 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_SHOW,
>> >+      DEVLINK_CMD_SELFTESTS_RUN,
>> >+
>> >       /* add new commands above here */
>> >       __DEVLINK_CMD_MAX,
>> >       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> >@@ -276,6 +279,25 @@ enum {
>> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
>> >       (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
>> >
>> >+/* Commonly used test cases */
>> >+enum {
>> >+      DEVLINK_SELFTEST_FLASH_BIT,
>> >+
>> >+      __DEVLINK_SELFTEST_MAX_BIT,
>> >+      DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
>> >+};
>> >+
>> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
>> >+
>> >+#define DEVLINK_SELFTESTS_MASK \
>> >+      (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
>> >+
>> >+enum {
>> >+      DEVLINK_SELFTEST_SKIP,
>> >+      DEVLINK_SELFTEST_PASS,
>> >+      DEVLINK_SELFTEST_FAIL
>> >+};
>> >+
>> > /**
>> >  * enum devlink_trap_action - Packet trap action.
>> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> is not
>> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
>> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
>> >
>> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
>>
>> I don't see why this is u32 bitset. Just have one attr per test
>> (NLA_FLAG) in a nested attr instead.
>>
>
>As per your suggestion, for an example it should be like as below
>
>        DEVLINK_ATTR_SELFTESTS,                 /* nested */
>
>        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
>
>        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */

Yeah, but have the flags in separate enum, no need to pullute the
devlink_attr enum by them.


>
>....    <SOME MORE TESTS>
>
>.....
>
>        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
>
>
>
> If we have this way then we need to have a mapping (probably a function)
>for drivers to tell them what tests need to be executed based on the flags
>that are set.
> Does this look OK?
>  The rationale behind choosing a mask is that we could directly pass the
>mask-value to the drivers.

If you have separate enum, you can use the attrs as bits internally in
kernel. Add a helper that would help the driver to work with it.
Pass a struct containing u32 (or u8) not to drivers. Once there are more
tests than that, this structure can be easily extended and the helpers
changed. This would make this scalable. No need for UAPI change or even
internel driver api change.


>
>
>>
>>
>>
>> >+      DEVLINK_ATTR_TEST_RESULT,               /* nested */
>> >+      DEVLINK_ATTR_TEST_NAME,                 /* string */
>> >+      DEVLINK_ATTR_TEST_RESULT_VAL,           /* u8 */
>>
>> Could you maintain the same "namespace" for all attrs related to
>> selftests?
>>
>
>Will fix it.
>
>
>>
>> >       /* 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 db61f3a341cb..0b7341ab6379 100644
>> >--- a/net/core/devlink.c
>> >+++ b/net/core/devlink.c
>> >@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct
>> sk_buff *skb,
>> >       return ret;
>> > }
>> >
>> >+static int devlink_selftest_name_put(struct sk_buff *skb, int test)
>> >+{
>> >+      const char *name = devlink_selftest_name(test);
>> >+      if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
>> >+              return -EMSGSIZE;
>> >+
>> >+      return 0;
>> >+}
>> >+
>> >+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
>> >+                                       struct genl_info *info)
>> >+{
>> >+      struct devlink *devlink = info->user_ptr[0];
>> >+      struct sk_buff *msg;
>> >+      unsigned long tests;
>> >+      int err = 0;
>> >+      void *hdr;
>> >+      int test;
>> >+
>> >+      if (!devlink->ops->selftests_show)
>> >+              return -EOPNOTSUPP;
>> >+
>> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> >+      if (!msg)
>> >+              return -ENOMEM;
>> >+
>> >+      hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>> >+                        &devlink_nl_family, 0,
>> DEVLINK_CMD_SELFTESTS_SHOW);
>> >+      if (!hdr)
>> >+              goto free_msg;
>> >+
>> >+      if (devlink_nl_put_handle(msg, devlink))
>> >+              goto genlmsg_cancel;
>> >+
>> >+      tests = devlink->ops->selftests_show(devlink, info->extack);
>> >+
>> >+      for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
>> >+              err = devlink_selftest_name_put(msg, test);
>> >+              if (err)
>> >+                      goto genlmsg_cancel;
>> >+      }
>> >+
>> >+      genlmsg_end(msg, hdr);
>> >+
>> >+      return genlmsg_reply(msg, info);
>> >+
>> >+genlmsg_cancel:
>> >+      genlmsg_cancel(msg, hdr);
>> >+free_msg:
>> >+      nlmsg_free(msg);
>> >+      return err;
>> >+}
>> >+
>> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test,
>> >+                                     u8 result)
>> >+{
>> >+      const char *name = devlink_selftest_name(test);
>> >+      struct nlattr *result_attr;
>> >+
>> >+      result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);
>> >+      if (!result_attr)
>> >+              return -EMSGSIZE;
>> >+
>> >+      if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
>> >+          nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
>> >+              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)
>> >+{
>> >+      u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
>> >+      struct devlink *devlink = info->user_ptr[0];
>> >+      unsigned long tests;
>> >+      struct sk_buff *msg;
>> >+      u32 tests_mask;
>> >+      void *hdr;
>> >+      int err = 0;
>> >+      int test;
>> >+
>> >+      if (!devlink->ops->selftests_run)
>> >+              return -EOPNOTSUPP;
>> >+
>> >+      if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
>> >+              return -EINVAL;
>> >+
>> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> >+      if (!msg)
>> >+              return -ENOMEM;
>> >+
>> >+      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_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
>> >+
>> >+      devlink->ops->selftests_run(devlink, tests_mask, test_results,
>>
>> Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too?
>>
>>      I`ll consider it in the next patch set.

Please do. This array of results returned from driver looks sloppy.


>
>
>>
>> >+                                  info->extack);
>> >+      tests = tests_mask;
>> >+
>> >+      for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
>> >+              err = devlink_selftest_result_put(msg, test,
>> >+                                                test_results[test]);
>> >+              if (err)
>> >+                      goto genlmsg_cancel;
>> >+      }
>> >+
>> >+      genlmsg_end(msg, hdr);
>> >+
>> >+      return genlmsg_reply(msg, 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,
>> >@@ -9000,6 +9130,8 @@ 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_MASK] = NLA_POLICY_MASK(NLA_U32,
>> >+
>> DEVLINK_SELFTESTS_MASK),
>> > };
>> >
>> > static const struct genl_small_ops devlink_nl_ops[] = {
>> >@@ -9361,6 +9493,18 @@ 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_SHOW,
>> >+              .validate = GENL_DONT_VALIDATE_STRICT |
>> GENL_DONT_VALIDATE_DUMP,
>> >+              .doit = devlink_nl_cmd_selftests_show,
>>
>> Why don't dump?
>>
>
>  I`ll add a dump in the next patchset.
>
>Thanks,
>Vikas
>
>
>>
>>
>> >+              .flags = GENL_ADMIN_PERM,
>> >+      },
>> >+      {
>> >+              .cmd = DEVLINK_CMD_SELFTESTS_RUN,
>> >+              .validate = GENL_DONT_VALIDATE_STRICT |
>> GENL_DONT_VALIDATE_DUMP,
>> >+              .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 v2 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

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

Hi Jiri,

On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
>
> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
> >Hi Jiri,
> >
> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
> >
> >> Thu, Jul 07, 2022 at 08:29:48PM 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: To query the supported selftests
> >> >by the driver.
> >> >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  |  34 +++++
> >> > include/net/devlink.h                         |  30 ++++
> >> > include/uapi/linux/devlink.h                  |  26 ++++
> >> > net/core/devlink.c                            | 144 ++++++++++++++++++
> >> > 4 files changed, 234 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..796d38f77038
> >> >--- /dev/null
> >> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
> >> >@@ -0,0 +1,34 @@
> >> >+.. 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``
> >> >+     - Runs a flash test on the device.
> >> >+
> >> >+example usage
> >> >+-------------
> >> >+
> >> >+.. code:: shell
> >> >+
> >> >+    # Query selftests supported on the device
> >> >+    $ devlink dev selftests show DEV
> >> >+    # Executes selftests on the device
> >> >+    $ devlink dev selftests run DEV test {flash | all}
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 2a2a2a0c93f7..cb7c378cf720 100644
> >> >--- a/include/net/devlink.h
> >> >+++ b/include/net/devlink.h
> >> >@@ -1215,6 +1215,18 @@ enum {
> >> >       DEVLINK_F_RELOAD = 1UL << 0,
> >> > };
> >> >
> >> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
> >> >+
> >> >+static inline const char *devlink_selftest_name(int test)
> >>
> >> I don't understand why this is needed. Better not to expose string to
> >> the user. Just have it as well defined attr.
> >
> >
> > OK. Will remove this function and corresponding attr
> >DEVLINK_ATTR_TEST_NAME added in this patch.
> >
> >
> >
> >
> >>
> >> >+{
> >> >+      switch (test) {
> >> >+      case DEVLINK_SELFTEST_FLASH_BIT:
> >> >+              return DEVLINK_SELFTEST_FLASH_TEST_NAME;
> >> >+      default:
> >> >+              return "unknown";
> >> >+      }
> >> >+}
> >> >+
> >> > struct devlink_ops {
> >> >       /**
> >> >        * @supported_flash_update_params:
> >> >@@ -1509,6 +1521,24 @@ struct devlink_ops {
> >> >                                   struct devlink_rate *parent,
> >> >                                   void *priv_child, void *priv_parent,
> >> >                                   struct netlink_ext_ack *extack);
> >> >+      /**
> >> >+       * selftests_show() - Shows selftests supported by device
> >> >+       * @devlink: Devlink instance
> >> >+       * @extack: extack for reporting error messages
> >> >+       *
> >> >+       * Return: test mask supported by driver
> >> >+       */
> >> >+      u32 (*selftests_show)(struct devlink *devlink,
> >> >+                            struct netlink_ext_ack *extack);
> >> >+      /**
> >> >+       * selftests_run() - Runs selftests
> >> >+       * @devlink: Devlink instance
> >> >+       * @tests_mask: tests to be run by driver
> >> >+       * @results: test results by driver
> >> >+       * @extack: extack for reporting error messages
> >> >+       */
> >> >+      void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
> >> >+                            u8 *results, 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 b3d40a5d72ff..1dba262328b9 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_SHOW,
> >> >+      DEVLINK_CMD_SELFTESTS_RUN,
> >> >+
> >> >       /* add new commands above here */
> >> >       __DEVLINK_CMD_MAX,
> >> >       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >> >@@ -276,6 +279,25 @@ enum {
> >> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> >> >       (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> >> >
> >> >+/* Commonly used test cases */
> >> >+enum {
> >> >+      DEVLINK_SELFTEST_FLASH_BIT,
> >> >+
> >> >+      __DEVLINK_SELFTEST_MAX_BIT,
> >> >+      DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
> >> >+};
> >> >+
> >> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
> >> >+
> >> >+#define DEVLINK_SELFTESTS_MASK \
> >> >+      (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
> >> >+
> >> >+enum {
> >> >+      DEVLINK_SELFTEST_SKIP,
> >> >+      DEVLINK_SELFTEST_PASS,
> >> >+      DEVLINK_SELFTEST_FAIL
> >> >+};
> >> >+
> >> > /**
> >> >  * enum devlink_trap_action - Packet trap action.
> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
> >> is not
> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
> >> >
> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
> >>
> >> I don't see why this is u32 bitset. Just have one attr per test
> >> (NLA_FLAG) in a nested attr instead.
> >>
> >
> >As per your suggestion, for an example it should be like as below
> >
> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
> >
> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
> >
> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
>
> Yeah, but have the flags in separate enum, no need to pullute the
> devlink_attr enum by them.
>
>
> >
> >....    <SOME MORE TESTS>
> >
> >.....
> >
> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
> >
> >
> >
> > If we have this way then we need to have a mapping (probably a function)
> >for drivers to tell them what tests need to be executed based on the flags
> >that are set.
> > Does this look OK?
> >  The rationale behind choosing a mask is that we could directly pass the
> >mask-value to the drivers.
>
> If you have separate enum, you can use the attrs as bits internally in
> kernel. Add a helper that would help the driver to work with it.
> Pass a struct containing u32 (or u8) not to drivers. Once there are more
> tests than that, this structure can be easily extended and the helpers
> changed. This would make this scalable. No need for UAPI change or even
> internel driver api change.

As per your suggestion, selftest attributes can be declared in separate
enum as below

enum {

        DEVLINK_SELFTEST_SOMETEST,         /* flag */

        DEVLINK_SELFTEST_SOMETEST1,

        DEVLINK_SELFTEST_SOMETEST2,

....

......

        __DEVLINK_SELFTEST_MAX,

        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1

};
Below  examples could be the flow of parameters/data 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.
______
|NEST |
|_____ |TEST1|TEST4|TEST7|...


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


Result Kernel to user execute test RES(u8)
______
|NEST |
|_____ |RES4|RES8|RES1|...

Results are populated in the same order as the user passed the TESTs
flags. Does the above result format from kernel to user look OK ?
Else we need to have below way to form a result format, a nest should
be made for <test_flag,
result> but since test flags are in different enum other than
devlink_attr and RES being part of devlink_attr, I believe it's not
good practice to make the below structure.
______
|NEST |
|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...

Let me know if my understanding is correct.

Thanks,
Vikas

>
>
> >
> >
> >>
> >>
> >>
> >> >+      DEVLINK_ATTR_TEST_RESULT,               /* nested */
> >> >+      DEVLINK_ATTR_TEST_NAME,                 /* string */
> >> >+      DEVLINK_ATTR_TEST_RESULT_VAL,           /* u8 */
> >>
> >> Could you maintain the same "namespace" for all attrs related to
> >> selftests?
> >>
> >
> >Will fix it.
> >
> >
> >>
> >> >       /* 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 db61f3a341cb..0b7341ab6379 100644
> >> >--- a/net/core/devlink.c
> >> >+++ b/net/core/devlink.c
> >> >@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct
> >> sk_buff *skb,
> >> >       return ret;
> >> > }
> >> >
> >> >+static int devlink_selftest_name_put(struct sk_buff *skb, int test)
> >> >+{
> >> >+      const char *name = devlink_selftest_name(test);
> >> >+      if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
> >> >+              return -EMSGSIZE;
> >> >+
> >> >+      return 0;
> >> >+}
> >> >+
> >> >+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
> >> >+                                       struct genl_info *info)
> >> >+{
> >> >+      struct devlink *devlink = info->user_ptr[0];
> >> >+      struct sk_buff *msg;
> >> >+      unsigned long tests;
> >> >+      int err = 0;
> >> >+      void *hdr;
> >> >+      int test;
> >> >+
> >> >+      if (!devlink->ops->selftests_show)
> >> >+              return -EOPNOTSUPP;
> >> >+
> >> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >> >+      if (!msg)
> >> >+              return -ENOMEM;
> >> >+
> >> >+      hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >> >+                        &devlink_nl_family, 0,
> >> DEVLINK_CMD_SELFTESTS_SHOW);
> >> >+      if (!hdr)
> >> >+              goto free_msg;
> >> >+
> >> >+      if (devlink_nl_put_handle(msg, devlink))
> >> >+              goto genlmsg_cancel;
> >> >+
> >> >+      tests = devlink->ops->selftests_show(devlink, info->extack);
> >> >+
> >> >+      for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
> >> >+              err = devlink_selftest_name_put(msg, test);
> >> >+              if (err)
> >> >+                      goto genlmsg_cancel;
> >> >+      }
> >> >+
> >> >+      genlmsg_end(msg, hdr);
> >> >+
> >> >+      return genlmsg_reply(msg, info);
> >> >+
> >> >+genlmsg_cancel:
> >> >+      genlmsg_cancel(msg, hdr);
> >> >+free_msg:
> >> >+      nlmsg_free(msg);
> >> >+      return err;
> >> >+}
> >> >+
> >> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test,
> >> >+                                     u8 result)
> >> >+{
> >> >+      const char *name = devlink_selftest_name(test);
> >> >+      struct nlattr *result_attr;
> >> >+
> >> >+      result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);
> >> >+      if (!result_attr)
> >> >+              return -EMSGSIZE;
> >> >+
> >> >+      if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
> >> >+          nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
> >> >+              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)
> >> >+{
> >> >+      u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
> >> >+      struct devlink *devlink = info->user_ptr[0];
> >> >+      unsigned long tests;
> >> >+      struct sk_buff *msg;
> >> >+      u32 tests_mask;
> >> >+      void *hdr;
> >> >+      int err = 0;
> >> >+      int test;
> >> >+
> >> >+      if (!devlink->ops->selftests_run)
> >> >+              return -EOPNOTSUPP;
> >> >+
> >> >+      if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
> >> >+              return -EINVAL;
> >> >+
> >> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >> >+      if (!msg)
> >> >+              return -ENOMEM;
> >> >+
> >> >+      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_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
> >> >+
> >> >+      devlink->ops->selftests_run(devlink, tests_mask, test_results,
> >>
> >> Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too?
> >>
> >>      I`ll consider it in the next patch set.
>
> Please do. This array of results returned from driver looks sloppy.
>
>
> >
> >
> >>
> >> >+                                  info->extack);
> >> >+      tests = tests_mask;
> >> >+
> >> >+      for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
> >> >+              err = devlink_selftest_result_put(msg, test,
> >> >+                                                test_results[test]);
> >> >+              if (err)
> >> >+                      goto genlmsg_cancel;
> >> >+      }
> >> >+
> >> >+      genlmsg_end(msg, hdr);
> >> >+
> >> >+      return genlmsg_reply(msg, 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,
> >> >@@ -9000,6 +9130,8 @@ 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_MASK] = NLA_POLICY_MASK(NLA_U32,
> >> >+
> >> DEVLINK_SELFTESTS_MASK),
> >> > };
> >> >
> >> > static const struct genl_small_ops devlink_nl_ops[] = {
> >> >@@ -9361,6 +9493,18 @@ 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_SHOW,
> >> >+              .validate = GENL_DONT_VALIDATE_STRICT |
> >> GENL_DONT_VALIDATE_DUMP,
> >> >+              .doit = devlink_nl_cmd_selftests_show,
> >>
> >> Why don't dump?
> >>
> >
> >  I`ll add a dump in the next patchset.
> >
> >Thanks,
> >Vikas
> >
> >
> >>
> >>
> >> >+              .flags = GENL_ADMIN_PERM,
> >> >+      },
> >> >+      {
> >> >+              .cmd = DEVLINK_CMD_SELFTESTS_RUN,
> >> >+              .validate = GENL_DONT_VALIDATE_STRICT |
> >> GENL_DONT_VALIDATE_DUMP,
> >> >+              .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 v2 1/3] devlink: introduce framework for selftests

[Jiri Pirko]

Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote:
>Hi Jiri,
>
>On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
>>
>> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
>> >Hi Jiri,
>> >
>> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
>> >
>> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote:

[...]


>> >> >  * enum devlink_trap_action - Packet trap action.
>> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> >> is not
>> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
>> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
>> >> >
>> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
>> >>
>> >> I don't see why this is u32 bitset. Just have one attr per test
>> >> (NLA_FLAG) in a nested attr instead.
>> >>
>> >
>> >As per your suggestion, for an example it should be like as below
>> >
>> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
>> >
>> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
>> >
>> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
>>
>> Yeah, but have the flags in separate enum, no need to pullute the
>> devlink_attr enum by them.
>>
>>
>> >
>> >....    <SOME MORE TESTS>
>> >
>> >.....
>> >
>> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
>> >
>> >
>> >
>> > If we have this way then we need to have a mapping (probably a function)
>> >for drivers to tell them what tests need to be executed based on the flags
>> >that are set.
>> > Does this look OK?
>> >  The rationale behind choosing a mask is that we could directly pass the
>> >mask-value to the drivers.
>>
>> If you have separate enum, you can use the attrs as bits internally in
>> kernel. Add a helper that would help the driver to work with it.
>> Pass a struct containing u32 (or u8) not to drivers. Once there are more
>> tests than that, this structure can be easily extended and the helpers
>> changed. This would make this scalable. No need for UAPI change or even
>> internel driver api change.
>
>As per your suggestion, selftest attributes can be declared in separate
>enum as below
>
>enum {
>
>        DEVLINK_SELFTEST_SOMETEST,         /* flag */
>
>        DEVLINK_SELFTEST_SOMETEST1,
>
>        DEVLINK_SELFTEST_SOMETEST2,
>
>....
>
>......
>
>        __DEVLINK_SELFTEST_MAX,
>
>        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
>
>};
>Below  examples could be the flow of parameters/data 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.
>______
>|NEST |
>|_____ |TEST1|TEST4|TEST7|...
>
>
>User to kernel to execute test: If user wants to execute test4, test8, test1...
>______
>|NEST |
>|_____ |TEST4|TEST8|TEST1|...
>
>
>Result Kernel to user execute test RES(u8)
>______
>|NEST |
>|_____ |RES4|RES8|RES1|...

Hmm, I think it is not good idea to rely on the order, a netlink library
can perhaps reorder it? Not sure here.

>
>Results are populated in the same order as the user passed the TESTs
>flags. Does the above result format from kernel to user look OK ?
>Else we need to have below way to form a result format, a nest should
>be made for <test_flag,
>result> but since test flags are in different enum other than
>devlink_attr and RES being part of devlink_attr, I believe it's not
>good practice to make the below structure.

Not a structure, no. Have it as another nest (could be the same attr as
the parent nest:

______
|NEST |
|_____ |NEST|       |NEST|       |NEST|
        TEST4,RES4   TEST8,RES8   TEST1, RES1

also, it is flexible to add another attr if needed (like maybe result
message string containing error message? IDK).



>______
>|NEST |
>|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
>
>Let me know if my understanding is correct.

[...]

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

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

Hi Jiri,

On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote:
> >Hi Jiri,
> >
> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
> >>
> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
> >> >Hi Jiri,
> >> >
> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
> >> >
> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote:
>
> [...]
>
>
> >> >> >  * enum devlink_trap_action - Packet trap action.
> >> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
> >> >> is not
> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
> >> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
> >> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
> >> >> >
> >> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
> >> >>
> >> >> I don't see why this is u32 bitset. Just have one attr per test
> >> >> (NLA_FLAG) in a nested attr instead.
> >> >>
> >> >
> >> >As per your suggestion, for an example it should be like as below
> >> >
> >> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
> >> >
> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
> >> >
> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
> >>
> >> Yeah, but have the flags in separate enum, no need to pullute the
> >> devlink_attr enum by them.
> >>
> >>
> >> >
> >> >....    <SOME MORE TESTS>
> >> >
> >> >.....
> >> >
> >> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
> >> >
> >> >
> >> >
> >> > If we have this way then we need to have a mapping (probably a function)
> >> >for drivers to tell them what tests need to be executed based on the flags
> >> >that are set.
> >> > Does this look OK?
> >> >  The rationale behind choosing a mask is that we could directly pass the
> >> >mask-value to the drivers.
> >>
> >> If you have separate enum, you can use the attrs as bits internally in
> >> kernel. Add a helper that would help the driver to work with it.
> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more
> >> tests than that, this structure can be easily extended and the helpers
> >> changed. This would make this scalable. No need for UAPI change or even
> >> internel driver api change.
> >
> >As per your suggestion, selftest attributes can be declared in separate
> >enum as below
> >
> >enum {
> >
> >        DEVLINK_SELFTEST_SOMETEST,         /* flag */
> >
> >        DEVLINK_SELFTEST_SOMETEST1,
> >
> >        DEVLINK_SELFTEST_SOMETEST2,
> >
> >....
> >
> >......
> >
> >        __DEVLINK_SELFTEST_MAX,
> >
> >        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
> >
> >};
> >Below  examples could be the flow of parameters/data 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.
> >______
> >|NEST |
> >|_____ |TEST1|TEST4|TEST7|...
> >
> >
> >User to kernel to execute test: If user wants to execute test4, test8, test1...
> >______
> >|NEST |
> >|_____ |TEST4|TEST8|TEST1|...
> >
> >
> >Result Kernel to user execute test RES(u8)
> >______
> >|NEST |
> >|_____ |RES4|RES8|RES1|...
>
> Hmm, I think it is not good idea to rely on the order, a netlink library
> can perhaps reorder it? Not sure here.
>
> >
> >Results are populated in the same order as the user passed the TESTs
> >flags. Does the above result format from kernel to user look OK ?
> >Else we need to have below way to form a result format, a nest should
> >be made for <test_flag,
> >result> but since test flags are in different enum other than
> >devlink_attr and RES being part of devlink_attr, I believe it's not
> >good practice to make the below structure.
>
> Not a structure, no. Have it as another nest (could be the same attr as
> the parent nest:
>
> ______
> |NEST |
> |_____ |NEST|       |NEST|       |NEST|
>         TEST4,RES4   TEST8,RES8   TEST1, RES1
>
> also, it is flexible to add another attr if needed (like maybe result
> message string containing error message? IDK).

For above nesting we can have the attributes defined as below

Attribute in  devlink_attr
enum devlink_attr {
  ....
  ....
        DEVLINK_SELFTESTS_INFO, /* nested */
  ...
...
}

enum devlink_selftests {
        DEVLINK_SELFTESTS_SOMETEST0,   /* flag */
        DEVLINK_SELFTESTS_SOMETEST1,
        DEVLINK_SELFTESTS_SOMETEST2,
        ...
        ...
}

enum devlink_selftest_result {
        DEVLINK_SELFTESTS_RESULT,       /* nested */
        DEVLINK_SELFTESTS_TESTNUM,      /* u32  indicating the test
number in devlink_selftests enum */
        DEVLINK_SELFTESTS_RESULT_VAL,   /* u8  skip, pass, fail.. */
        ...some future attrr...

}
enums in devlink_selftest_result can be put in devlink_attr though.

Does this look OK?

Thanks,
Vikas

>
>
>
> >______
> >|NEST |
> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
> >
> >Let me know if my understanding is correct.
>
> [...]

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

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Jiri Pirko]

Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote:
>Hi Jiri,
>
>On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote:
>> >Hi Jiri,
>> >
>> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
>> >>
>> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
>> >> >Hi Jiri,
>> >> >
>> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
>> >> >
>> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote:
>>
>> [...]
>>
>>
>> >> >> >  * enum devlink_trap_action - Packet trap action.
>> >> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> >> >> is not
>> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
>> >> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
>> >> >> >
>> >> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
>> >> >>
>> >> >> I don't see why this is u32 bitset. Just have one attr per test
>> >> >> (NLA_FLAG) in a nested attr instead.
>> >> >>
>> >> >
>> >> >As per your suggestion, for an example it should be like as below
>> >> >
>> >> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
>> >> >
>> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
>> >> >
>> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
>> >>
>> >> Yeah, but have the flags in separate enum, no need to pullute the
>> >> devlink_attr enum by them.
>> >>
>> >>
>> >> >
>> >> >....    <SOME MORE TESTS>
>> >> >
>> >> >.....
>> >> >
>> >> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
>> >> >
>> >> >
>> >> >
>> >> > If we have this way then we need to have a mapping (probably a function)
>> >> >for drivers to tell them what tests need to be executed based on the flags
>> >> >that are set.
>> >> > Does this look OK?
>> >> >  The rationale behind choosing a mask is that we could directly pass the
>> >> >mask-value to the drivers.
>> >>
>> >> If you have separate enum, you can use the attrs as bits internally in
>> >> kernel. Add a helper that would help the driver to work with it.
>> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more
>> >> tests than that, this structure can be easily extended and the helpers
>> >> changed. This would make this scalable. No need for UAPI change or even
>> >> internel driver api change.
>> >
>> >As per your suggestion, selftest attributes can be declared in separate
>> >enum as below
>> >
>> >enum {
>> >
>> >        DEVLINK_SELFTEST_SOMETEST,         /* flag */
>> >
>> >        DEVLINK_SELFTEST_SOMETEST1,
>> >
>> >        DEVLINK_SELFTEST_SOMETEST2,
>> >
>> >....
>> >
>> >......
>> >
>> >        __DEVLINK_SELFTEST_MAX,
>> >
>> >        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
>> >
>> >};
>> >Below  examples could be the flow of parameters/data 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.
>> >______
>> >|NEST |
>> >|_____ |TEST1|TEST4|TEST7|...
>> >
>> >
>> >User to kernel to execute test: If user wants to execute test4, test8, test1...
>> >______
>> >|NEST |
>> >|_____ |TEST4|TEST8|TEST1|...
>> >
>> >
>> >Result Kernel to user execute test RES(u8)
>> >______
>> >|NEST |
>> >|_____ |RES4|RES8|RES1|...
>>
>> Hmm, I think it is not good idea to rely on the order, a netlink library
>> can perhaps reorder it? Not sure here.
>>
>> >
>> >Results are populated in the same order as the user passed the TESTs
>> >flags. Does the above result format from kernel to user look OK ?
>> >Else we need to have below way to form a result format, a nest should
>> >be made for <test_flag,
>> >result> but since test flags are in different enum other than
>> >devlink_attr and RES being part of devlink_attr, I believe it's not
>> >good practice to make the below structure.
>>
>> Not a structure, no. Have it as another nest (could be the same attr as
>> the parent nest:
>>
>> ______
>> |NEST |
>> |_____ |NEST|       |NEST|       |NEST|
>>         TEST4,RES4   TEST8,RES8   TEST1, RES1
>>
>> also, it is flexible to add another attr if needed (like maybe result
>> message string containing error message? IDK).
>
>For above nesting we can have the attributes defined as below
>
>Attribute in  devlink_attr
>enum devlink_attr {
>  ....
>  ....
>        DEVLINK_SELFTESTS_INFO, /* nested */
>  ...
>...
>}
>
>enum devlink_selftests {
>        DEVLINK_SELFTESTS_SOMETEST0,   /* flag */
>        DEVLINK_SELFTESTS_SOMETEST1,
>        DEVLINK_SELFTESTS_SOMETEST2,
>        ...
>        ...
>}
>
>enum devlink_selftest_result {

for attrs, have "attr" in the name of the enum and "ATTR" in name of the
value.

>        DEVLINK_SELFTESTS_RESULT,       /* nested */
>        DEVLINK_SELFTESTS_TESTNUM,      /* u32  indicating the test

You can have 1 enum, containing both these and the test flags from
above.


>number in devlink_selftests enum */
>        DEVLINK_SELFTESTS_RESULT_VAL,   /* u8  skip, pass, fail.. */

Put enum name in the comment, instead of list possible values.


>        ...some future attrr...
>
>}
>enums in devlink_selftest_result can be put in devlink_attr though.

You can have them separate, I think it is about the time we try to put
new attrs what does not have potencial to be re-used to a separate enum.


>
>Does this look OK?
>
>Thanks,
>Vikas
>
>>
>>
>>
>> >______
>> >|NEST |
>> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
>> >
>> >Let me know if my understanding is correct.
>>
>> [...]

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

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

Hi Jiri,

On Wed, Jul 13, 2022 at 12:58 PM Jiri Pirko <jiri@nvidia.com> wrote:
>
> Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote:
> >Hi Jiri,
> >
> >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote:
> >> >Hi Jiri,
> >> >
> >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
> >> >>
> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
> >> >> >Hi Jiri,
> >> >> >
> >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
> >> >> >
> >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote:
> >>
> >> [...]
> >>
> >>
> >> >> >> >  * enum devlink_trap_action - Packet trap action.
> >> >> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
> >> >> >> is not
> >> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
> >> >> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
> >> >> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
> >> >> >> >
> >> >> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
> >> >> >>
> >> >> >> I don't see why this is u32 bitset. Just have one attr per test
> >> >> >> (NLA_FLAG) in a nested attr instead.
> >> >> >>
> >> >> >
> >> >> >As per your suggestion, for an example it should be like as below
> >> >> >
> >> >> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
> >> >> >
> >> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
> >> >> >
> >> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
> >> >>
> >> >> Yeah, but have the flags in separate enum, no need to pullute the
> >> >> devlink_attr enum by them.
> >> >>
> >> >>
> >> >> >
> >> >> >....    <SOME MORE TESTS>
> >> >> >
> >> >> >.....
> >> >> >
> >> >> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
> >> >> >
> >> >> >
> >> >> >
> >> >> > If we have this way then we need to have a mapping (probably a function)
> >> >> >for drivers to tell them what tests need to be executed based on the flags
> >> >> >that are set.
> >> >> > Does this look OK?
> >> >> >  The rationale behind choosing a mask is that we could directly pass the
> >> >> >mask-value to the drivers.
> >> >>
> >> >> If you have separate enum, you can use the attrs as bits internally in
> >> >> kernel. Add a helper that would help the driver to work with it.
> >> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more
> >> >> tests than that, this structure can be easily extended and the helpers
> >> >> changed. This would make this scalable. No need for UAPI change or even
> >> >> internel driver api change.
> >> >
> >> >As per your suggestion, selftest attributes can be declared in separate
> >> >enum as below
> >> >
> >> >enum {
> >> >
> >> >        DEVLINK_SELFTEST_SOMETEST,         /* flag */
> >> >
> >> >        DEVLINK_SELFTEST_SOMETEST1,
> >> >
> >> >        DEVLINK_SELFTEST_SOMETEST2,
> >> >
> >> >....
> >> >
> >> >......
> >> >
> >> >        __DEVLINK_SELFTEST_MAX,
> >> >
> >> >        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
> >> >
> >> >};
> >> >Below  examples could be the flow of parameters/data 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.
> >> >______
> >> >|NEST |
> >> >|_____ |TEST1|TEST4|TEST7|...
> >> >
> >> >
> >> >User to kernel to execute test: If user wants to execute test4, test8, test1...
> >> >______
> >> >|NEST |
> >> >|_____ |TEST4|TEST8|TEST1|...
> >> >
> >> >
> >> >Result Kernel to user execute test RES(u8)
> >> >______
> >> >|NEST |
> >> >|_____ |RES4|RES8|RES1|...
> >>
> >> Hmm, I think it is not good idea to rely on the order, a netlink library
> >> can perhaps reorder it? Not sure here.
> >>
> >> >
> >> >Results are populated in the same order as the user passed the TESTs
> >> >flags. Does the above result format from kernel to user look OK ?
> >> >Else we need to have below way to form a result format, a nest should
> >> >be made for <test_flag,
> >> >result> but since test flags are in different enum other than
> >> >devlink_attr and RES being part of devlink_attr, I believe it's not
> >> >good practice to make the below structure.
> >>
> >> Not a structure, no. Have it as another nest (could be the same attr as
> >> the parent nest:
> >>
> >> ______
> >> |NEST |
> >> |_____ |NEST|       |NEST|       |NEST|
> >>         TEST4,RES4   TEST8,RES8   TEST1, RES1
> >>
> >> also, it is flexible to add another attr if needed (like maybe result
> >> message string containing error message? IDK).
> >
> >For above nesting we can have the attributes defined as below
> >
> >Attribute in  devlink_attr
> >enum devlink_attr {
> >  ....
> >  ....
> >        DEVLINK_SELFTESTS_INFO, /* nested */
> >  ...
> >...
> >}
> >
> >enum devlink_selftests {
> >        DEVLINK_SELFTESTS_SOMETEST0,   /* flag */
> >        DEVLINK_SELFTESTS_SOMETEST1,
> >        DEVLINK_SELFTESTS_SOMETEST2,
> >        ...
> >        ...
> >}
> >
> >enum devlink_selftest_result {
>
> for attrs, have "attr" in the name of the enum and "ATTR" in name of the
> value.
>
> >        DEVLINK_SELFTESTS_RESULT,       /* nested */
> >        DEVLINK_SELFTESTS_TESTNUM,      /* u32  indicating the test
>
> You can have 1 enum, containing both these and the test flags from
> above.
 I think it's better to keep enum devlink_selftests_attr (containing
flags) and devlink_selftest_result_attr separately as it will have an
advantage.
 For example, for show commands the kernel can iterate through and
check with the driver if it supports a particular test.

    for (i = 0; i < DEVLINK_SELFTEST_ATTR_MAX, i++) {
                   if (devlink->ops->selftest_info(devlink, i,
extack)) {  // supports selftest or not
                         nla_put_flag(msg, i);
                }
        }
      Also flags in devlink_selftests_attr can be used as bitwise, if required.
      Let me know what you think.

Thanks,
Vikas

>
>
> >number in devlink_selftests enum */
> >        DEVLINK_SELFTESTS_RESULT_VAL,   /* u8  skip, pass, fail.. */
>
> Put enum name in the comment, instead of list possible values.
>
>
> >        ...some future attrr...
> >
> >}
> >enums in devlink_selftest_result can be put in devlink_attr though.
>
> You can have them separate, I think it is about the time we try to put
> new attrs what does not have potencial to be re-used to a separate enum.
>
>
> >
> >Does this look OK?
> >
> >Thanks,
> >Vikas
> >
> >>
> >>
> >>
> >> >______
> >> >|NEST |
> >> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
> >> >
> >> >Let me know if my understanding is correct.
> >>
> >> [...]
>
>

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

Re: [PATCH net-next v2 1/3] devlink: introduce framework for selftests

[Jiri Pirko]

Wed, Jul 13, 2022 at 12:16:03PM CEST, vikas.gupta@broadcom.com wrote:
>Hi Jiri,
>
>On Wed, Jul 13, 2022 at 12:58 PM Jiri Pirko <jiri@nvidia.com> wrote:
>>
>> Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote:
>> >Hi Jiri,
>> >
>> >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote:
>> >>
>> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote:
>> >> >Hi Jiri,
>> >> >
>> >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote:
>> >> >>
>> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote:
>> >> >> >Hi Jiri,
>> >> >> >
>> >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote:
>> >> >> >
>> >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote:
>> >>
>> >> [...]
>> >>
>> >>
>> >> >> >> >  * enum devlink_trap_action - Packet trap action.
>> >> >> >> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> >> >> >> is not
>> >> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >> >> >> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
>> >> >> >> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
>> >> >> >> >
>> >> >> >> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
>> >> >> >>
>> >> >> >> I don't see why this is u32 bitset. Just have one attr per test
>> >> >> >> (NLA_FLAG) in a nested attr instead.
>> >> >> >>
>> >> >> >
>> >> >> >As per your suggestion, for an example it should be like as below
>> >> >> >
>> >> >> >        DEVLINK_ATTR_SELFTESTS,                 /* nested */
>> >> >> >
>> >> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
>> >> >> >
>> >> >> >        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */
>> >> >>
>> >> >> Yeah, but have the flags in separate enum, no need to pullute the
>> >> >> devlink_attr enum by them.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >....    <SOME MORE TESTS>
>> >> >> >
>> >> >> >.....
>> >> >> >
>> >> >> >        DEVLINK_ATTR_SLEFTESTS_RESULT_VAL,      /* u8 */
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > If we have this way then we need to have a mapping (probably a function)
>> >> >> >for drivers to tell them what tests need to be executed based on the flags
>> >> >> >that are set.
>> >> >> > Does this look OK?
>> >> >> >  The rationale behind choosing a mask is that we could directly pass the
>> >> >> >mask-value to the drivers.
>> >> >>
>> >> >> If you have separate enum, you can use the attrs as bits internally in
>> >> >> kernel. Add a helper that would help the driver to work with it.
>> >> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more
>> >> >> tests than that, this structure can be easily extended and the helpers
>> >> >> changed. This would make this scalable. No need for UAPI change or even
>> >> >> internel driver api change.
>> >> >
>> >> >As per your suggestion, selftest attributes can be declared in separate
>> >> >enum as below
>> >> >
>> >> >enum {
>> >> >
>> >> >        DEVLINK_SELFTEST_SOMETEST,         /* flag */
>> >> >
>> >> >        DEVLINK_SELFTEST_SOMETEST1,
>> >> >
>> >> >        DEVLINK_SELFTEST_SOMETEST2,
>> >> >
>> >> >....
>> >> >
>> >> >......
>> >> >
>> >> >        __DEVLINK_SELFTEST_MAX,
>> >> >
>> >> >        DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
>> >> >
>> >> >};
>> >> >Below  examples could be the flow of parameters/data 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.
>> >> >______
>> >> >|NEST |
>> >> >|_____ |TEST1|TEST4|TEST7|...
>> >> >
>> >> >
>> >> >User to kernel to execute test: If user wants to execute test4, test8, test1...
>> >> >______
>> >> >|NEST |
>> >> >|_____ |TEST4|TEST8|TEST1|...
>> >> >
>> >> >
>> >> >Result Kernel to user execute test RES(u8)
>> >> >______
>> >> >|NEST |
>> >> >|_____ |RES4|RES8|RES1|...
>> >>
>> >> Hmm, I think it is not good idea to rely on the order, a netlink library
>> >> can perhaps reorder it? Not sure here.
>> >>
>> >> >
>> >> >Results are populated in the same order as the user passed the TESTs
>> >> >flags. Does the above result format from kernel to user look OK ?
>> >> >Else we need to have below way to form a result format, a nest should
>> >> >be made for <test_flag,
>> >> >result> but since test flags are in different enum other than
>> >> >devlink_attr and RES being part of devlink_attr, I believe it's not
>> >> >good practice to make the below structure.
>> >>
>> >> Not a structure, no. Have it as another nest (could be the same attr as
>> >> the parent nest:
>> >>
>> >> ______
>> >> |NEST |
>> >> |_____ |NEST|       |NEST|       |NEST|
>> >>         TEST4,RES4   TEST8,RES8   TEST1, RES1
>> >>
>> >> also, it is flexible to add another attr if needed (like maybe result
>> >> message string containing error message? IDK).
>> >
>> >For above nesting we can have the attributes defined as below
>> >
>> >Attribute in  devlink_attr
>> >enum devlink_attr {
>> >  ....
>> >  ....
>> >        DEVLINK_SELFTESTS_INFO, /* nested */
>> >  ...
>> >...
>> >}
>> >
>> >enum devlink_selftests {
>> >        DEVLINK_SELFTESTS_SOMETEST0,   /* flag */
>> >        DEVLINK_SELFTESTS_SOMETEST1,
>> >        DEVLINK_SELFTESTS_SOMETEST2,
>> >        ...
>> >        ...
>> >}
>> >
>> >enum devlink_selftest_result {
>>
>> for attrs, have "attr" in the name of the enum and "ATTR" in name of the
>> value.
>>
>> >        DEVLINK_SELFTESTS_RESULT,       /* nested */
>> >        DEVLINK_SELFTESTS_TESTNUM,      /* u32  indicating the test
>>
>> You can have 1 enum, containing both these and the test flags from
>> above.
> I think it's better to keep enum devlink_selftests_attr (containing
>flags) and devlink_selftest_result_attr separately as it will have an
>advantage.
> For example, for show commands the kernel can iterate through and
>check with the driver if it supports a particular test.
>
>    for (i = 0; i < DEVLINK_SELFTEST_ATTR_MAX, i++) {
>                   if (devlink->ops->selftest_info(devlink, i,
>extack)) {  // supports selftest or not
>                         nla_put_flag(msg, i);
>                }
>        }
>      Also flags in devlink_selftests_attr can be used as bitwise, if required.
>      Let me know what you think.

Okay.


>
>Thanks,
>Vikas
>
>>
>>
>> >number in devlink_selftests enum */
>> >        DEVLINK_SELFTESTS_RESULT_VAL,   /* u8  skip, pass, fail.. */
>>
>> Put enum name in the comment, instead of list possible values.
>>
>>
>> >        ...some future attrr...
>> >
>> >}
>> >enums in devlink_selftest_result can be put in devlink_attr though.
>>
>> You can have them separate, I think it is about the time we try to put
>> new attrs what does not have potencial to be re-used to a separate enum.
>>
>>
>> >
>> >Does this look OK?
>> >
>> >Thanks,
>> >Vikas
>> >
>> >>
>> >>
>> >>
>> >> >______
>> >> >|NEST |
>> >> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
>> >> >
>> >> >Let me know if my understanding is correct.
>> >>
>> >> [...]
>>
>>

[PATCH net-next v2 2/3] bnxt_en: refactor NVM APIs

[Vikas Gupta]

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

modify declaration for NVM APIs so that they can be
used with devlink and ethtool both.

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>
---
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 24 +++++++++----------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 12 ++++++++++
 2 files changed, 24 insertions(+), 12 deletions(-)

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 --]

[PATCH net-next v2 3/3] bnxt_en: implement callbacks for devlink selftests

[Vikas Gupta]

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

Add callbacks
=============
.selftests_show: populates flash test name.
.selftests_run: implements a flash selftest.

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>
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 3528ce9849e6..750034b45049 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 u32 bnxt_dl_selftests_show(struct devlink *dl,
+				  struct netlink_ext_ack *extack)
+{
+	return DEVLINK_SELFTEST_FLASH;
+}
+
+static void bnxt_dl_selftests_run(struct devlink *dl, u32 test_mask,
+				  u8 *results, struct netlink_ext_ack *extack)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+	bool res;
+
+	if (test_mask & DEVLINK_SELFTEST_FLASH) {
+		res = bnxt_nvm_test(bp, extack);
+		results[DEVLINK_SELFTEST_FLASH_BIT] = res ?
+							DEVLINK_SELFTEST_PASS :
+							DEVLINK_SELFTEST_FAIL;
+	}
+}
+
 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,
+	.selftests_show	  = bnxt_dl_selftests_show,
+	.selftests_run	  = bnxt_dl_selftests_run,
 };
 
 static const struct devlink_ops bnxt_vf_dl_ops;
-- 
2.31.1


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