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

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

[Vikas Gupta]

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

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

  Patchset adds a flash based test for the bnxt_en driver.

  Suggested commands at user level would be as below:

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


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

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

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


Thanks,
Vikas



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

 .../networking/devlink/devlink-selftests.rst  |  38 +++
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  60 +++++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  24 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  12 +
 include/net/devlink.h                         |  20 ++
 include/uapi/linux/devlink.h                  |  29 +++
 net/core/devlink.c                            | 216 ++++++++++++++++++
 7 files changed, 387 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 v3 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 11449 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_list_doit/dumpit: To query the supported
selftests by the drivers.
devlink_nl_cmd_selftests_run: To execute selftests. Users can
provide a test mask for executing group tests or standalone tests.

Documentation/networking/devlink/ path is already part of MAINTAINERS &
the new files come under this path. Hence no update needed to the
MAINTAINERS

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 .../networking/devlink/devlink-selftests.rst  |  38 +++
 include/net/devlink.h                         |  20 ++
 include/uapi/linux/devlink.h                  |  29 +++
 net/core/devlink.c                            | 216 ++++++++++++++++++
 4 files changed, 303 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..0e9727895987
--- /dev/null
+++ b/Documentation/networking/devlink/devlink-selftests.rst
@@ -0,0 +1,38 @@
+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+=================
+Devlink Selftests
+=================
+
+The ``devlink-selftests`` API allows executing selftests on the device.
+
+Tests Mask
+==========
+The ``devlink-selftests`` command should be run with a mask indicating
+the tests to be executed.
+
+Tests Description
+=================
+The following is a list of tests that drivers may execute.
+
+.. list-table:: List of tests
+   :widths: 5 90
+
+   * - Name
+     - Description
+   * - ``DEVLINK_SELFTEST_FLASH``
+     - Devices may have the firmware on non-volatile memory on the board, e.g.
+       flash. This particular test helps to run a flash selftest on the device.
+       Implementation of the test is left to the driver/firmware.
+
+example usage
+-------------
+
+.. code:: shell
+
+    # Query selftests supported on the devlink device
+    $ devlink dev selftests show DEV
+    # Query selftests supported on all devlink devices
+    $ devlink dev selftests show
+    # Executes selftests on the device
+    $ devlink dev selftests run DEV test flash
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 88c701b375a2..9d8fc09f2c39 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1509,6 +1509,26 @@ struct devlink_ops {
 				    struct devlink_rate *parent,
 				    void *priv_child, void *priv_parent,
 				    struct netlink_ext_ack *extack);
+	/**
+	 * selftests_check() - queries if selftest is supported
+	 * @devlink: Devlink instance
+	 * @test_id: test index
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: True if test is supported by the driver
+	 */
+	bool (*selftest_check)(struct devlink *devlink, int test_id,
+			       struct netlink_ext_ack *extack);
+	/**
+	 * selftest_run() - Runs a selftest
+	 * @devlink: Devlink instance
+	 * @test_id: test index
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: Result of the test
+	 */
+	u8 (*selftest_run)(struct devlink *devlink, int test_id,
+			   struct netlink_ext_ack *extack);
 };
 
 void *devlink_priv(struct devlink *devlink);
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b3d40a5d72ff..469846f40e6d 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_LIST,	/* can dump */
+	DEVLINK_CMD_SELFTESTS_RUN,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -276,6 +279,31 @@ enum {
 #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
 
+/* Commonly used test cases */
+enum devlink_selftest_attr {
+	DEVLINK_SELFTEST_ATTR_UNSPEC,
+	DEVLINK_SELFTEST_ATTR_FLASH,		/* flag */
+
+	__DEVLINK_SELFTEST_ATTR_MAX,
+	DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
+};
+
+enum devlink_selftest_result {
+	DEVLINK_SELFTEST_SKIP,
+	DEVLINK_SELFTEST_PASS,
+	DEVLINK_SELFTEST_FAIL
+};
+
+enum devlink_selftest_result_attr {
+	DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
+	DEVLINK_SELFTEST_ATTR_RESULT,		/* nested */
+	DEVLINK_SELFTEST_ATTR_TEST_ID,		/* u32, devlink_selftest_attr */
+	DEVLINK_SELFTEST_ATTR_TEST_STATUS,	/* u8, devlink_selftest_result */
+
+	__DEVLINK_SELFTEST_ATTR_RES_MAX,
+	DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
+};
+
 /**
  * enum devlink_trap_action - Packet trap action.
  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -576,6 +604,7 @@ enum devlink_attr {
 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
 
+	DEVLINK_ATTR_SELFTESTS_INFO,		/* nested */
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index a9776ea923ae..57a9a183e1d1 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
 				 DEVLINK_PORT_FN_STATE_ACTIVE),
 };
 
+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
+	[DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
+};
+
 static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
 #define DEVLINK_REGISTERED XA_MARK_1
 
@@ -4791,6 +4795,206 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	return ret;
 }
 
+static int
+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
+			  u32 portid, u32 seq, int flags,
+			  struct netlink_ext_ack *extack)
+{
+	struct nlattr *selftests_list;
+	void *hdr;
+	int err;
+	int i;
+
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
+			  DEVLINK_CMD_SELFTESTS_LIST);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = -EMSGSIZE;
+	if (devlink_nl_put_handle(msg, devlink))
+		goto err_cancel_msg;
+
+	selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
+	if (!selftests_list)
+		goto err_cancel_msg;
+
+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
+		if (devlink->ops->selftest_check(devlink, i, extack)) {
+			err = nla_put_flag(msg, i);
+			if (err)
+				goto err_cancel_msg;
+		}
+	}
+
+	nla_nest_end(msg, selftests_list);
+
+	genlmsg_end(msg, hdr);
+
+	return 0;
+
+err_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
+					      struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops->selftest_check)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
+					info->snd_seq, 0, info->extack);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
+						struct netlink_callback *cb)
+{
+	struct devlink *devlink;
+	int start = cb->args[0];
+	unsigned long index;
+	int idx = 0;
+	int err = 0;
+
+	mutex_lock(&devlink_mutex);
+	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
+		if (!devlink_try_get(devlink))
+			continue;
+
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			goto retry;
+
+		if (idx < start || !devlink->ops->selftest_check)
+			goto inc;
+
+		mutex_lock(&devlink->lock);
+		err = devlink_nl_selftests_fill(msg, devlink,
+						NETLINK_CB(cb->skb).portid,
+						cb->nlh->nlmsg_seq, NLM_F_MULTI,
+						cb->extack);
+		mutex_unlock(&devlink->lock);
+		if (err) {
+			devlink_put(devlink);
+			break;
+		}
+inc:
+		idx++;
+retry:
+		devlink_put(devlink);
+	}
+	mutex_unlock(&devlink_mutex);
+
+	if (err != -EMSGSIZE)
+		return err;
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,
+				       u8 result)
+{
+	struct nlattr *result_attr;
+
+	result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
+	if (!result_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
+	    nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, 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)
+{
+	struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
+	struct devlink *devlink = info->user_ptr[0];
+	struct nlattr *attrs, *tests_info;
+	struct sk_buff *msg;
+	void *hdr;
+	int err;
+	int i;
+
+	if (!devlink->ops->selftest_run)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])
+		return -EINVAL;
+
+	attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
+
+	err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
+			       devlink_selftest_nl_policy, info->extack);
+	if (err < 0)
+		return err;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = -EMSGSIZE;
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
+	if (!hdr)
+		goto free_msg;
+
+	if (devlink_nl_put_handle(msg, devlink))
+		goto genlmsg_cancel;
+
+	tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
+	if (!tests_info)
+		goto genlmsg_cancel;
+
+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
+		u8 res;
+
+		if (nla_get_flag(tb[i])) {
+			res = devlink->ops->selftest_run(devlink, i,
+							 info->extack);
+			err = devlink_selftest_result_put(msg, i, res);
+			if (err)
+				goto selftests_list_nest_cancel;
+		}
+	}
+
+	nla_nest_end(msg, tests_info);
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, info);
+
+selftests_list_nest_cancel:
+	nla_nest_cancel(msg, tests_info);
+genlmsg_cancel:
+	genlmsg_cancel(msg, hdr);
+free_msg:
+	nlmsg_free(msg);
+	return err;
+}
+
 static const struct devlink_param devlink_param_generic[] = {
 	{
 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
@@ -8997,6 +9201,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
 };
 
 static const struct genl_small_ops devlink_nl_ops[] = {
@@ -9356,6 +9561,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.doit = devlink_nl_cmd_trap_policer_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd = DEVLINK_CMD_SELFTESTS_LIST,
+		.doit = devlink_nl_cmd_selftests_list_doit,
+		.dumpit = devlink_nl_cmd_selftests_list_dumpit
+		/* can be retrieved by unprivileged users */
+	},
+	{
+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
+		.doit = devlink_nl_cmd_selftests_run,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.31.1


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

[PATCH net-next v3 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 v3 3/3] bnxt_en: implement callbacks for devlink selftests

[Vikas Gupta]

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

Add callbacks
=============
.selftest_check: returns true for flash selftest.
.selftest_run: runs 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 | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 6b3d4f4c2a75..927cf368d856 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,62 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
 	return rc;
 }
 
+static bool bnxt_nvm_test(struct bnxt *bp, struct netlink_ext_ack *extack)
+{
+	u32 datalen;
+	u16 index;
+	u8 *buf;
+
+	if (bnxt_find_nvram_item(bp->dev, BNX_DIR_TYPE_VPD,
+				 BNX_DIR_ORDINAL_FIRST, BNX_DIR_EXT_NONE,
+				 &index, NULL, &datalen) || !datalen) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd entry error");
+		return false;
+	}
+
+	buf = kzalloc(datalen, GFP_KERNEL);
+	if (!buf) {
+		NL_SET_ERR_MSG_MOD(extack, "insufficient memory for nvm test");
+		return false;
+	}
+
+	if (bnxt_get_nvram_item(bp->dev, index, 0, datalen, buf)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd read error");
+		goto err;
+	}
+
+	if (bnxt_flash_nvram(bp->dev, BNX_DIR_TYPE_VPD, BNX_DIR_ORDINAL_FIRST,
+			     BNX_DIR_EXT_NONE, 0, 0, buf, datalen)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd write error");
+		goto err;
+	}
+
+	return true;
+
+err:
+	kfree(buf);
+	return false;
+}
+
+static bool bnxt_dl_selftest_check(struct devlink *dl, int test_id,
+				   struct netlink_ext_ack *extack)
+{
+	return (test_id == DEVLINK_SELFTEST_ATTR_FLASH);
+}
+
+static u8 bnxt_dl_selftest_run(struct devlink *dl, int test_id,
+			       struct netlink_ext_ack *extack)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (test_id == DEVLINK_SELFTEST_ATTR_FLASH) {
+		return (bnxt_nvm_test(bp, extack) ? DEVLINK_SELFTEST_PASS :
+						    DEVLINK_SELFTEST_FAIL);
+	}
+
+	return DEVLINK_SELFTEST_SKIP;
+}
+
 static const struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
@@ -622,6 +680,8 @@ static const struct devlink_ops bnxt_dl_ops = {
 	.reload_limits	  = BIT(DEVLINK_RELOAD_LIMIT_NO_RESET),
 	.reload_down	  = bnxt_dl_reload_down,
 	.reload_up	  = bnxt_dl_reload_up,
+	.selftest_check	  = bnxt_dl_selftest_check,
+	.selftest_run	  = bnxt_dl_selftest_run,
 };
 
 static const struct devlink_ops bnxt_vf_dl_ops;
-- 
2.31.1


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

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

[Jakub Kicinski]

On Mon, 18 Jul 2022 11:50:30 +0530 Vikas Gupta wrote:
> +	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
> +		u8 res;
> +
> +		if (nla_get_flag(tb[i])) {
> +			res = devlink->ops->selftest_run(devlink, i,

Shouldn't we selftest_check() first to make sure the driver supports
given test?

> +	[DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },

	... = NLA_POLICY_NESTED(devlink_selftest_nl_policy),

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

[Vikas Gupta]

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

Hi Jakub,

On Tue, Jul 19, 2022 at 9:03 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 18 Jul 2022 11:50:30 +0530 Vikas Gupta wrote:
> > +     for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
> > +             u8 res;
> > +
> > +             if (nla_get_flag(tb[i])) {
> > +                     res = devlink->ops->selftest_run(devlink, i,
>
> Shouldn't we selftest_check() first to make sure the driver supports
> given test?
 Yes, I`ll add selftest_check() here rather than letting the driver return SKIP.

Thanks,
Vikas

>
> > +     [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
>
>         ... = NLA_POLICY_NESTED(devlink_selftest_nl_policy),

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

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

[Vikas Gupta]

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

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

  Patchset adds a flash based test for the bnxt_en driver.

  Suggested commands at user level would be as below:

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

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


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

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

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


Thanks,
Vikas

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

 .../networking/devlink/devlink-selftests.rst  |  38 +++
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  60 +++++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  24 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h |  12 +
 include/net/devlink.h                         |  20 ++
 include/uapi/linux/devlink.h                  |  29 +++
 net/core/devlink.c                            | 225 ++++++++++++++++++
 7 files changed, 396 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 v4 1/3] devlink: introduce framework for selftests

[Vikas Gupta]

[-- Attachment #1: Type: text/plain, Size: 11719 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_list_doit/dumpit: To query the supported
selftests by the drivers.
devlink_nl_cmd_selftests_run: To execute selftests. Users can
provide a test mask for executing group tests or standalone tests.

Documentation/networking/devlink/ path is already part of MAINTAINERS &
the new files come under this path. Hence no update needed to the
MAINTAINERS

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 .../networking/devlink/devlink-selftests.rst  |  38 +++
 include/net/devlink.h                         |  20 ++
 include/uapi/linux/devlink.h                  |  29 +++
 net/core/devlink.c                            | 225 ++++++++++++++++++
 4 files changed, 312 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..0e9727895987
--- /dev/null
+++ b/Documentation/networking/devlink/devlink-selftests.rst
@@ -0,0 +1,38 @@
+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+=================
+Devlink Selftests
+=================
+
+The ``devlink-selftests`` API allows executing selftests on the device.
+
+Tests Mask
+==========
+The ``devlink-selftests`` command should be run with a mask indicating
+the tests to be executed.
+
+Tests Description
+=================
+The following is a list of tests that drivers may execute.
+
+.. list-table:: List of tests
+   :widths: 5 90
+
+   * - Name
+     - Description
+   * - ``DEVLINK_SELFTEST_FLASH``
+     - Devices may have the firmware on non-volatile memory on the board, e.g.
+       flash. This particular test helps to run a flash selftest on the device.
+       Implementation of the test is left to the driver/firmware.
+
+example usage
+-------------
+
+.. code:: shell
+
+    # Query selftests supported on the devlink device
+    $ devlink dev selftests show DEV
+    # Query selftests supported on all devlink devices
+    $ devlink dev selftests show
+    # Executes selftests on the device
+    $ devlink dev selftests run DEV test flash
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 88c701b375a2..085d761f1cd3 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1509,6 +1509,26 @@ struct devlink_ops {
 				    struct devlink_rate *parent,
 				    void *priv_child, void *priv_parent,
 				    struct netlink_ext_ack *extack);
+	/**
+	 * selftests_check() - queries if selftest is supported
+	 * @devlink: Devlink instance
+	 * @test_id: test index
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: true if test is supported by the driver
+	 */
+	bool (*selftest_check)(struct devlink *devlink, int test_id,
+			       struct netlink_ext_ack *extack);
+	/**
+	 * selftest_run() - Runs a selftest
+	 * @devlink: Devlink instance
+	 * @test_id: test index
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: Result of the test
+	 */
+	u8 (*selftest_run)(struct devlink *devlink, int test_id,
+			   struct netlink_ext_ack *extack);
 };
 
 void *devlink_priv(struct devlink *devlink);
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b3d40a5d72ff..469846f40e6d 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_LIST,	/* can dump */
+	DEVLINK_CMD_SELFTESTS_RUN,
+
 	/* add new commands above here */
 	__DEVLINK_CMD_MAX,
 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
@@ -276,6 +279,31 @@ enum {
 #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
 
+/* Commonly used test cases */
+enum devlink_selftest_attr {
+	DEVLINK_SELFTEST_ATTR_UNSPEC,
+	DEVLINK_SELFTEST_ATTR_FLASH,		/* flag */
+
+	__DEVLINK_SELFTEST_ATTR_MAX,
+	DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
+};
+
+enum devlink_selftest_result {
+	DEVLINK_SELFTEST_SKIP,
+	DEVLINK_SELFTEST_PASS,
+	DEVLINK_SELFTEST_FAIL
+};
+
+enum devlink_selftest_result_attr {
+	DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
+	DEVLINK_SELFTEST_ATTR_RESULT,		/* nested */
+	DEVLINK_SELFTEST_ATTR_TEST_ID,		/* u32, devlink_selftest_attr */
+	DEVLINK_SELFTEST_ATTR_TEST_STATUS,	/* u8, devlink_selftest_result */
+
+	__DEVLINK_SELFTEST_ATTR_RES_MAX,
+	DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
+};
+
 /**
  * enum devlink_trap_action - Packet trap action.
  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
@@ -576,6 +604,7 @@ enum devlink_attr {
 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
 
+	DEVLINK_ATTR_SELFTESTS_INFO,		/* nested */
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index a9776ea923ae..ef9439f2502f 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
 				 DEVLINK_PORT_FN_STATE_ACTIVE),
 };
 
+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
+	[DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
+};
+
 static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
 #define DEVLINK_REGISTERED XA_MARK_1
 
@@ -4791,6 +4795,215 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
 	return ret;
 }
 
+static int
+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
+			  u32 portid, u32 seq, int flags,
+			  struct netlink_ext_ack *extack)
+{
+	struct nlattr *selftests_list;
+	void *hdr;
+	int err;
+	int i;
+
+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
+			  DEVLINK_CMD_SELFTESTS_LIST);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	err = -EMSGSIZE;
+	if (devlink_nl_put_handle(msg, devlink))
+		goto err_cancel_msg;
+
+	selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
+	if (!selftests_list)
+		goto err_cancel_msg;
+
+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
+		if (devlink->ops->selftest_check(devlink, i, extack)) {
+			err = nla_put_flag(msg, i);
+			if (err)
+				goto err_cancel_msg;
+		}
+	}
+
+	nla_nest_end(msg, selftests_list);
+
+	genlmsg_end(msg, hdr);
+
+	return 0;
+
+err_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
+					      struct genl_info *info)
+{
+	struct devlink *devlink = info->user_ptr[0];
+	struct sk_buff *msg;
+	int err;
+
+	if (!devlink->ops->selftest_check)
+		return -EOPNOTSUPP;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
+					info->snd_seq, 0, info->extack);
+	if (err) {
+		nlmsg_free(msg);
+		return err;
+	}
+
+	return genlmsg_reply(msg, info);
+}
+
+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
+						struct netlink_callback *cb)
+{
+	struct devlink *devlink;
+	int start = cb->args[0];
+	unsigned long index;
+	int idx = 0;
+	int err = 0;
+
+	mutex_lock(&devlink_mutex);
+	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
+		if (!devlink_try_get(devlink))
+			continue;
+
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			goto retry;
+
+		if (idx < start || !devlink->ops->selftest_check)
+			goto inc;
+
+		mutex_lock(&devlink->lock);
+		err = devlink_nl_selftests_fill(msg, devlink,
+						NETLINK_CB(cb->skb).portid,
+						cb->nlh->nlmsg_seq, NLM_F_MULTI,
+						cb->extack);
+		mutex_unlock(&devlink->lock);
+		if (err) {
+			devlink_put(devlink);
+			break;
+		}
+inc:
+		idx++;
+retry:
+		devlink_put(devlink);
+	}
+	mutex_unlock(&devlink_mutex);
+
+	if (err != -EMSGSIZE)
+		return err;
+
+	cb->args[0] = idx;
+	return msg->len;
+}
+
+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,
+				       u8 result)
+{
+	struct nlattr *result_attr;
+
+	result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
+	if (!result_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
+	    nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, 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)
+{
+	struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
+	struct devlink *devlink = info->user_ptr[0];
+	struct nlattr *attrs, *tests_info;
+	struct sk_buff *msg;
+	void *hdr;
+	int err;
+	int i;
+
+	if (!devlink->ops->selftest_run)
+		return -EOPNOTSUPP;
+
+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])
+		return -EINVAL;
+
+	attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
+
+	err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
+			       devlink_selftest_nl_policy, info->extack);
+	if (err < 0)
+		return err;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	err = -EMSGSIZE;
+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
+	if (!hdr)
+		goto free_msg;
+
+	if (devlink_nl_put_handle(msg, devlink))
+		goto genlmsg_cancel;
+
+	tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
+	if (!tests_info)
+		goto genlmsg_cancel;
+
+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
+		u8 res = DEVLINK_SELFTEST_SKIP;
+
+		if (nla_get_flag(tb[i])) {
+			if (devlink->ops->selftest_check &&
+			    !devlink->ops->selftest_check(devlink, i,
+							  info->extack)) {
+				err = devlink_selftest_result_put(msg, i, res);
+				if (err)
+					goto selftests_list_nest_cancel;
+				continue;
+			}
+
+			res = devlink->ops->selftest_run(devlink, i,
+							 info->extack);
+			err = devlink_selftest_result_put(msg, i, res);
+			if (err)
+				goto selftests_list_nest_cancel;
+		}
+	}
+
+	nla_nest_end(msg, tests_info);
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, info);
+
+selftests_list_nest_cancel:
+	nla_nest_cancel(msg, tests_info);
+genlmsg_cancel:
+	genlmsg_cancel(msg, hdr);
+free_msg:
+	nlmsg_free(msg);
+	return err;
+}
+
 static const struct devlink_param devlink_param_generic[] = {
 	{
 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
@@ -8997,6 +9210,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
 	[DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
+	[DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
 };
 
 static const struct genl_small_ops devlink_nl_ops[] = {
@@ -9356,6 +9570,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
 		.doit = devlink_nl_cmd_trap_policer_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd = DEVLINK_CMD_SELFTESTS_LIST,
+		.doit = devlink_nl_cmd_selftests_list_doit,
+		.dumpit = devlink_nl_cmd_selftests_list_dumpit
+		/* can be retrieved by unprivileged users */
+	},
+	{
+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
+		.doit = devlink_nl_cmd_selftests_run,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.31.1


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

[PATCH net-next v4 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 v4 3/3] bnxt_en: implement callbacks for devlink selftests

[Vikas Gupta]

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

Add callbacks
=============
.selftest_check: returns true for flash selftest.
.selftest_run: runs 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 | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 6b3d4f4c2a75..927cf368d856 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,62 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
 	return rc;
 }
 
+static bool bnxt_nvm_test(struct bnxt *bp, struct netlink_ext_ack *extack)
+{
+	u32 datalen;
+	u16 index;
+	u8 *buf;
+
+	if (bnxt_find_nvram_item(bp->dev, BNX_DIR_TYPE_VPD,
+				 BNX_DIR_ORDINAL_FIRST, BNX_DIR_EXT_NONE,
+				 &index, NULL, &datalen) || !datalen) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd entry error");
+		return false;
+	}
+
+	buf = kzalloc(datalen, GFP_KERNEL);
+	if (!buf) {
+		NL_SET_ERR_MSG_MOD(extack, "insufficient memory for nvm test");
+		return false;
+	}
+
+	if (bnxt_get_nvram_item(bp->dev, index, 0, datalen, buf)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd read error");
+		goto err;
+	}
+
+	if (bnxt_flash_nvram(bp->dev, BNX_DIR_TYPE_VPD, BNX_DIR_ORDINAL_FIRST,
+			     BNX_DIR_EXT_NONE, 0, 0, buf, datalen)) {
+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd write error");
+		goto err;
+	}
+
+	return true;
+
+err:
+	kfree(buf);
+	return false;
+}
+
+static bool bnxt_dl_selftest_check(struct devlink *dl, int test_id,
+				   struct netlink_ext_ack *extack)
+{
+	return (test_id == DEVLINK_SELFTEST_ATTR_FLASH);
+}
+
+static u8 bnxt_dl_selftest_run(struct devlink *dl, int test_id,
+			       struct netlink_ext_ack *extack)
+{
+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
+
+	if (test_id == DEVLINK_SELFTEST_ATTR_FLASH) {
+		return (bnxt_nvm_test(bp, extack) ? DEVLINK_SELFTEST_PASS :
+						    DEVLINK_SELFTEST_FAIL);
+	}
+
+	return DEVLINK_SELFTEST_SKIP;
+}
+
 static const struct devlink_ops bnxt_dl_ops = {
 #ifdef CONFIG_BNXT_SRIOV
 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
@@ -622,6 +680,8 @@ static const struct devlink_ops bnxt_dl_ops = {
 	.reload_limits	  = BIT(DEVLINK_RELOAD_LIMIT_NO_RESET),
 	.reload_down	  = bnxt_dl_reload_down,
 	.reload_up	  = bnxt_dl_reload_up,
+	.selftest_check	  = bnxt_dl_selftest_check,
+	.selftest_run	  = bnxt_dl_selftest_run,
 };
 
 static const struct devlink_ops bnxt_vf_dl_ops;
-- 
2.31.1


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

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

[Jiri Pirko]

Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
>selftests by the drivers.
>devlink_nl_cmd_selftests_run: To execute selftests. Users can
>provide a test mask for executing group tests or standalone tests.
>
>Documentation/networking/devlink/ path is already part of MAINTAINERS &
>the new files come under this path. Hence no update needed to the
>MAINTAINERS
>
>Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
>Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
>---
> .../networking/devlink/devlink-selftests.rst  |  38 +++
> include/net/devlink.h                         |  20 ++
> include/uapi/linux/devlink.h                  |  29 +++
> net/core/devlink.c                            | 225 ++++++++++++++++++
> 4 files changed, 312 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..0e9727895987
>--- /dev/null
>+++ b/Documentation/networking/devlink/devlink-selftests.rst
>@@ -0,0 +1,38 @@
>+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>+
>+=================
>+Devlink Selftests
>+=================
>+
>+The ``devlink-selftests`` API allows executing selftests on the device.
>+
>+Tests Mask
>+==========
>+The ``devlink-selftests`` command should be run with a mask indicating
>+the tests to be executed.
>+
>+Tests Description
>+=================
>+The following is a list of tests that drivers may execute.
>+
>+.. list-table:: List of tests
>+   :widths: 5 90
>+
>+   * - Name
>+     - Description
>+   * - ``DEVLINK_SELFTEST_FLASH``
>+     - Devices may have the firmware on non-volatile memory on the board, e.g.
>+       flash. This particular test helps to run a flash selftest on the device.
>+       Implementation of the test is left to the driver/firmware.
>+
>+example usage
>+-------------
>+
>+.. code:: shell
>+
>+    # Query selftests supported on the devlink device
>+    $ devlink dev selftests show DEV
>+    # Query selftests supported on all devlink devices
>+    $ devlink dev selftests show
>+    # Executes selftests on the device
>+    $ devlink dev selftests run DEV test flash

"test_id" to be consistend with the attr name and outputs. Please see
below. Devlink cmdline would accept "test" as well, so you can still use
this.


>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 88c701b375a2..085d761f1cd3 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1509,6 +1509,26 @@ struct devlink_ops {
> 				    struct devlink_rate *parent,
> 				    void *priv_child, void *priv_parent,
> 				    struct netlink_ext_ack *extack);
>+	/**
>+	 * selftests_check() - queries if selftest is supported
>+	 * @devlink: Devlink instance

Why capital "D"?


>+	 * @test_id: test index
>+	 * @extack: extack for reporting error messages
>+	 *
>+	 * Return: true if test is supported by the driver
>+	 */
>+	bool (*selftest_check)(struct devlink *devlink, int test_id,

Why this is an "int". I would be surprised to see a negative value here.
Have this unsigned please.


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

There too.


>+			   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..469846f40e6d 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_LIST,	/* can dump */

The rest of the commands are named "_GET". Please be consistent with
them.


>+	DEVLINK_CMD_SELFTESTS_RUN,
>+
> 	/* add new commands above here */
> 	__DEVLINK_CMD_MAX,
> 	DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -276,6 +279,31 @@ enum {
> #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> 	(_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> 
>+/* Commonly used test cases */

What do you mean by "commonly". Are there some others that are not
"common"? I don't follow.


>+enum devlink_selftest_attr {
>+	DEVLINK_SELFTEST_ATTR_UNSPEC,
>+	DEVLINK_SELFTEST_ATTR_FLASH,		/* flag */
>+
>+	__DEVLINK_SELFTEST_ATTR_MAX,
>+	DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
>+};

To be consistent with the attr that caries this:

enum devlink_attr_selftest_test_id {
	DEVLINK_ATTR_SELFTEST_TEST_ID_UNSPEC,
	DEVLINK_ATTR_SELFTEST_TEST_ID_FLASH,		/* flag */

	__DEVLINK_ATTR_SELFTEST_TEST_ID_MAX,
	DEVLINK_ATTR_SELFTEST_TEST_ID_MAX = __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX - 1

>+
>+enum devlink_selftest_result {
>+	DEVLINK_SELFTEST_SKIP,
>+	DEVLINK_SELFTEST_PASS,
>+	DEVLINK_SELFTEST_FAIL

It is common to have the enum name be root of names of the values.
Also, be consistent with the attr this value is carried over:

enum devlink_selftest_test_status {
	DEVLINK_SELFTEST_TEST_STATUS_SKIP,
	DEVLINK_SELFTEST_TEST_STATUS_PASS,
	DEVLINK_SELFTEST_TEST_STATUS_FAIL

That way, it is obvious to which enum the value belongs.


>+};
>+
>+enum devlink_selftest_result_attr {
>+	DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
>+	DEVLINK_SELFTEST_ATTR_RESULT,		/* nested */
>+	DEVLINK_SELFTEST_ATTR_TEST_ID,		/* u32, devlink_selftest_attr */

add "enum" ?

>+	DEVLINK_SELFTEST_ATTR_TEST_STATUS,	/* u8, devlink_selftest_result */

add "enum" ?

The same note as above:
enum devlink_attr_selftest_result {
	DEVLINK_ATTR_SELFTEST_RESULT_UNSPEC,
	DEVLINK_ATTR_SELFTEST_RESULT,		  /* nested */
	DEVLINK_ATTR_SELFTEST_RESULT_TEST_ID,	  /* u32, enum devlink_selftest_attr */
	DEVLINK_ATTR_SELFTEST_RESULT_TEST_STATUS, /* u8, devlink_selftest_result */




>+
>+	__DEVLINK_SELFTEST_ATTR_RES_MAX,
>+	DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
>+};
>+
> /**
>  * enum devlink_trap_action - Packet trap action.
>  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
>@@ -576,6 +604,7 @@ enum devlink_attr {
> 	DEVLINK_ATTR_LINECARD_TYPE,		/* string */
> 	DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,	/* nested */
> 
>+	DEVLINK_ATTR_SELFTESTS_INFO,		/* nested */
> 	/* add new attributes above here, update the policy in devlink.c */
> 
> 	__DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index a9776ea923ae..ef9439f2502f 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> 				 DEVLINK_PORT_FN_STATE_ACTIVE),
> };
> 
>+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
>+	[DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
>+};
>+
> static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> #define DEVLINK_REGISTERED XA_MARK_1
> 
>@@ -4791,6 +4795,215 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> 	return ret;
> }
> 
>+static int
>+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
>+			  u32 portid, u32 seq, int flags,
>+			  struct netlink_ext_ack *extack)
>+{
>+	struct nlattr *selftests_list;
>+	void *hdr;
>+	int err;
>+	int i;
>+
>+	hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
>+			  DEVLINK_CMD_SELFTESTS_LIST);
>+	if (!hdr)
>+		return -EMSGSIZE;
>+
>+	err = -EMSGSIZE;
>+	if (devlink_nl_put_handle(msg, devlink))
>+		goto err_cancel_msg;
>+
>+	selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+	if (!selftests_list)
>+		goto err_cancel_msg;
>+
>+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {

**)
It is a bit odd to see "1" here. Maybe "DEVLINK_SELFTEST_ATTR_UNSPEC + 1"
would be more obvious for the reader.

also:
i < DEVLINK_SELFTEST_ATTR_MAX + 1
would be rather nicer to be:
i <= DEVLINK_SELFTEST_ATTR_MAX


>+		if (devlink->ops->selftest_check(devlink, i, extack)) {
>+			err = nla_put_flag(msg, i);
>+			if (err)
>+				goto err_cancel_msg;
>+		}
>+	}
>+
>+	nla_nest_end(msg, selftests_list);
>+

No need for this empty line.


>+	genlmsg_end(msg, hdr);
>+

No need for this empty line.


>+	return 0;
>+
>+err_cancel_msg:
>+	genlmsg_cancel(msg, hdr);
>+	return err;
>+}
>+
>+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
>+					      struct genl_info *info)
>+{
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct sk_buff *msg;
>+	int err;
>+
>+	if (!devlink->ops->selftest_check)
>+		return -EOPNOTSUPP;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
>+					info->snd_seq, 0, info->extack);
>+	if (err) {
>+		nlmsg_free(msg);
>+		return err;
>+	}
>+
>+	return genlmsg_reply(msg, info);
>+}
>+
>+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
>+						struct netlink_callback *cb)
>+{
>+	struct devlink *devlink;
>+	int start = cb->args[0];
>+	unsigned long index;
>+	int idx = 0;
>+	int err = 0;
>+
>+	mutex_lock(&devlink_mutex);
>+	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>+		if (!devlink_try_get(devlink))
>+			continue;
>+
>+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>+			goto retry;
>+
>+		if (idx < start || !devlink->ops->selftest_check)
>+			goto inc;
>+
>+		mutex_lock(&devlink->lock);
>+		err = devlink_nl_selftests_fill(msg, devlink,
>+						NETLINK_CB(cb->skb).portid,
>+						cb->nlh->nlmsg_seq, NLM_F_MULTI,
>+						cb->extack);
>+		mutex_unlock(&devlink->lock);
>+		if (err) {
>+			devlink_put(devlink);
>+			break;
>+		}
>+inc:
>+		idx++;
>+retry:
>+		devlink_put(devlink);
>+	}
>+	mutex_unlock(&devlink_mutex);
>+
>+	if (err != -EMSGSIZE)
>+		return err;
>+
>+	cb->args[0] = idx;
>+	return msg->len;
>+}
>+
>+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,

unsigned.

>+				       u8 result)

Please be consistend and call this "test_status"


>+{
>+	struct nlattr *result_attr;
>+
>+	result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
>+	if (!result_attr)
>+		return -EMSGSIZE;
>+
>+	if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
>+	    nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, result))
>+		goto nla_put_failure;
>+
>+	nla_nest_end(skb, result_attr);
>+

No need for this empty line.


>+	return 0;
>+
>+nla_put_failure:
>+	nla_nest_cancel(skb, result_attr);
>+	return -EMSGSIZE;
>+}
>+
>+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
>+					struct genl_info *info)
>+{
>+	struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
>+	struct devlink *devlink = info->user_ptr[0];
>+	struct nlattr *attrs, *tests_info;
>+	struct sk_buff *msg;
>+	void *hdr;
>+	int err;
>+	int i;
>+
>+	if (!devlink->ops->selftest_run)
>+		return -EOPNOTSUPP;
>+
>+	if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])

Fill extack message here please.


>+		return -EINVAL;
>+
>+	attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
>+
>+	err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
>+			       devlink_selftest_nl_policy, info->extack);
>+	if (err < 0)
>+		return err;
>+
>+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+	if (!msg)
>+		return -ENOMEM;
>+
>+	err = -EMSGSIZE;
>+	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+			  &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
>+	if (!hdr)
>+		goto free_msg;
>+
>+	if (devlink_nl_put_handle(msg, devlink))
>+		goto genlmsg_cancel;
>+
>+	tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+	if (!tests_info)
>+		goto genlmsg_cancel;
>+
>+	for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {

Same notes to the iteration as above. **


>+		u8 res = DEVLINK_SELFTEST_SKIP;

u8 test_status;


>+
>+		if (nla_get_flag(tb[i])) {
>+			if (devlink->ops->selftest_check &&

No need to test in every iteration. I think it is safe to assume
that driver that does not fill selftest_check() does not support
selftests at all, so please move to the beginning of this function
alongside selftest_run() check:

	if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
		return -EOPNOTSUPP;

>+			    !devlink->ops->selftest_check(devlink, i,
>+							  info->extack)) {
>+				err = devlink_selftest_result_put(msg, i, res);

Just do devlink_selftest_result_put(msg, i, .._SKIP); here and avoid
initializing "res" at the beginning.


>+				if (err)
>+					goto selftests_list_nest_cancel;
>+				continue;
>+			}
>+
>+			res = devlink->ops->selftest_run(devlink, i,
>+							 info->extack);
>+			err = devlink_selftest_result_put(msg, i, res);
>+			if (err)
>+				goto selftests_list_nest_cancel;
>+		}
>+	}
>+
>+	nla_nest_end(msg, tests_info);
>+

No need for this empty line.


>+	genlmsg_end(msg, hdr);
>+

No need for this empty line.


>+	return genlmsg_reply(msg, info);
>+
>+selftests_list_nest_cancel:
>+	nla_nest_cancel(msg, tests_info);
>+genlmsg_cancel:
>+	genlmsg_cancel(msg, hdr);
>+free_msg:
>+	nlmsg_free(msg);
>+	return err;
>+}
>+
> static const struct devlink_param devlink_param_generic[] = {
> 	{
> 		.id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
>@@ -8997,6 +9210,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> 	[DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> 	[DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
>+	[DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
> };
> 
> static const struct genl_small_ops devlink_nl_ops[] = {
>@@ -9356,6 +9570,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> 		.doit = devlink_nl_cmd_trap_policer_set_doit,
> 		.flags = GENL_ADMIN_PERM,
> 	},
>+	{
>+		.cmd = DEVLINK_CMD_SELFTESTS_LIST,
>+		.doit = devlink_nl_cmd_selftests_list_doit,
>+		.dumpit = devlink_nl_cmd_selftests_list_dumpit
>+		/* can be retrieved by unprivileged users */
>+	},
>+	{
>+		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
>+		.doit = devlink_nl_cmd_selftests_run,
>+		.flags = GENL_ADMIN_PERM,
>+	},
> };
> 
> static struct genl_family devlink_nl_family __ro_after_init = {
>-- 
>2.31.1
>

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

[Jiri Pirko]

Thu, Jul 21, 2022 at 09:21:21AM CEST, vikas.gupta@broadcom.com wrote:
>Add callbacks
>=============
>.selftest_check: returns true for flash selftest.
>.selftest_run: runs 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 | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 6b3d4f4c2a75..927cf368d856 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,62 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
> 	return rc;
> }
> 
>+static bool bnxt_nvm_test(struct bnxt *bp, struct netlink_ext_ack *extack)
>+{
>+	u32 datalen;
>+	u16 index;
>+	u8 *buf;
>+
>+	if (bnxt_find_nvram_item(bp->dev, BNX_DIR_TYPE_VPD,
>+				 BNX_DIR_ORDINAL_FIRST, BNX_DIR_EXT_NONE,
>+				 &index, NULL, &datalen) || !datalen) {
>+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd entry error");
>+		return false;
>+	}
>+
>+	buf = kzalloc(datalen, GFP_KERNEL);
>+	if (!buf) {
>+		NL_SET_ERR_MSG_MOD(extack, "insufficient memory for nvm test");
>+		return false;
>+	}
>+
>+	if (bnxt_get_nvram_item(bp->dev, index, 0, datalen, buf)) {
>+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd read error");
>+		goto err;
>+	}
>+
>+	if (bnxt_flash_nvram(bp->dev, BNX_DIR_TYPE_VPD, BNX_DIR_ORDINAL_FIRST,
>+			     BNX_DIR_EXT_NONE, 0, 0, buf, datalen)) {
>+		NL_SET_ERR_MSG_MOD(extack, "nvm test vpd write error");
>+		goto err;
>+	}
>+
>+	return true;
>+
>+err:
>+	kfree(buf);
>+	return false;
>+}
>+
>+static bool bnxt_dl_selftest_check(struct devlink *dl, int test_id,
>+				   struct netlink_ext_ack *extack)
>+{
>+	return (test_id == DEVLINK_SELFTEST_ATTR_FLASH);

Return is not a function, avoid "()"


>+}
>+
>+static u8 bnxt_dl_selftest_run(struct devlink *dl, int test_id,
>+			       struct netlink_ext_ack *extack)
>+{
>+	struct bnxt *bp = bnxt_get_bp_from_dl(dl);
>+
>+	if (test_id == DEVLINK_SELFTEST_ATTR_FLASH) {
>+		return (bnxt_nvm_test(bp, extack) ? DEVLINK_SELFTEST_PASS :
>+						    DEVLINK_SELFTEST_FAIL);

Return is not a function, avoid "()"

>+	}

No need for "{}", it's a simple statement. I'm pretty sure checkpatch
would warn you, did you run it?


>+
>+	return DEVLINK_SELFTEST_SKIP;
>+}
>+
> static const struct devlink_ops bnxt_dl_ops = {
> #ifdef CONFIG_BNXT_SRIOV
> 	.eswitch_mode_set = bnxt_dl_eswitch_mode_set,
>@@ -622,6 +680,8 @@ static const struct devlink_ops bnxt_dl_ops = {
> 	.reload_limits	  = BIT(DEVLINK_RELOAD_LIMIT_NO_RESET),
> 	.reload_down	  = bnxt_dl_reload_down,
> 	.reload_up	  = bnxt_dl_reload_up,
>+	.selftest_check	  = bnxt_dl_selftest_check,
>+	.selftest_run	  = bnxt_dl_selftest_run,
> };
> 
> static const struct devlink_ops bnxt_vf_dl_ops;
>-- 
>2.31.1
>

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

[Jiri Pirko]

Thu, Jul 21, 2022 at 09:21:19AM CEST, vikas.gupta@broadcom.com wrote:

[...]

>+	/**
>+	 * selftest_run() - Runs a selftest
>+	 * @devlink: Devlink instance
>+	 * @test_id: test index
>+	 * @extack: extack for reporting error messages
>+	 *
>+	 * Return: Result of the test
>+	 */
>+	u8 (*selftest_run)(struct devlink *devlink, int test_id,

It should return enum. Make sure you changed the variable type you store
the test status in.


>+			   struct netlink_ext_ack *extack);
> };

[...]

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

[Jiri Pirko]

"bnxt_en: refactor NVM APIs" is very odd patch subject name. Please
change it to the summary of what the patch is aiming to do.

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

[Vikas Gupta]

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

Hi Jiri,

On Thu, Jul 21, 2022 at 6:32 PM Jiri Pirko <jiri@nvidia.com> wrote:
>
> "bnxt_en: refactor NVM APIs" is very odd patch subject name. Please
> change it to the summary of what the patch is aiming to do.

Seems like this patch might create confusion with this patch series.
Since this patch is just changing the declarations so that a flash
based test(patch# 3/3) can be supported, I would rather merge it with
the next patch(3/3).

Thanks,
Vikas

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

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

[Vikas Gupta]

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

Hi Jiri,


On Thu, Jul 21, 2022 at 6:27 PM Jiri Pirko <jiri@nvidia.com> wrote:
>
> Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
> >selftests by the drivers.
> >devlink_nl_cmd_selftests_run: To execute selftests. Users can
> >provide a test mask for executing group tests or standalone tests.
> >
> >Documentation/networking/devlink/ path is already part of MAINTAINERS &
> >the new files come under this path. Hence no update needed to the
> >MAINTAINERS
> >
> >Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
> >Reviewed-by: Michael Chan <michael.chan@broadcom.com>
> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> >---
> > .../networking/devlink/devlink-selftests.rst  |  38 +++
> > include/net/devlink.h                         |  20 ++
> > include/uapi/linux/devlink.h                  |  29 +++
> > net/core/devlink.c                            | 225 ++++++++++++++++++
> > 4 files changed, 312 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..0e9727895987
> >--- /dev/null
> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
> >@@ -0,0 +1,38 @@
> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >+
> >+=================
> >+Devlink Selftests
> >+=================
> >+
> >+The ``devlink-selftests`` API allows executing selftests on the device.
> >+
> >+Tests Mask
> >+==========
> >+The ``devlink-selftests`` command should be run with a mask indicating
> >+the tests to be executed.
> >+
> >+Tests Description
> >+=================
> >+The following is a list of tests that drivers may execute.
> >+
> >+.. list-table:: List of tests
> >+   :widths: 5 90
> >+
> >+   * - Name
> >+     - Description
> >+   * - ``DEVLINK_SELFTEST_FLASH``
> >+     - Devices may have the firmware on non-volatile memory on the board, e.g.
> >+       flash. This particular test helps to run a flash selftest on the device.
> >+       Implementation of the test is left to the driver/firmware.
> >+
> >+example usage
> >+-------------
> >+
> >+.. code:: shell
> >+
> >+    # Query selftests supported on the devlink device
> >+    $ devlink dev selftests show DEV
> >+    # Query selftests supported on all devlink devices
> >+    $ devlink dev selftests show
> >+    # Executes selftests on the device
> >+    $ devlink dev selftests run DEV test flash
>
> "test_id" to be consistend with the attr name and outputs. Please see
What is "test_id" referring to in this document? Can you please elaborate ?

> below. Devlink cmdline would accept "test" as well, so you can still use
Are you mentioning the "test" argument in the above devlink command line option?

Thanks,
Vikas
> this.

>
>
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 88c701b375a2..085d761f1cd3 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -1509,6 +1509,26 @@ struct devlink_ops {
> >                                   struct devlink_rate *parent,
> >                                   void *priv_child, void *priv_parent,
> >                                   struct netlink_ext_ack *extack);
> >+      /**
> >+       * selftests_check() - queries if selftest is supported
> >+       * @devlink: Devlink instance
>
> Why capital "D"?
>
>
> >+       * @test_id: test index
> >+       * @extack: extack for reporting error messages
> >+       *
> >+       * Return: true if test is supported by the driver
> >+       */
> >+      bool (*selftest_check)(struct devlink *devlink, int test_id,
>
> Why this is an "int". I would be surprised to see a negative value here.
> Have this unsigned please.
>
>
> >+                             struct netlink_ext_ack *extack);
> >+      /**
> >+       * selftest_run() - Runs a selftest
> >+       * @devlink: Devlink instance
> >+       * @test_id: test index
> >+       * @extack: extack for reporting error messages
> >+       *
> >+       * Return: Result of the test
> >+       */
> >+      u8 (*selftest_run)(struct devlink *devlink, int test_id,
>
> There too.
>
>
> >+                         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..469846f40e6d 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_LIST,     /* can dump */
>
> The rest of the commands are named "_GET". Please be consistent with
> them.
>
>
> >+      DEVLINK_CMD_SELFTESTS_RUN,
> >+
> >       /* add new commands above here */
> >       __DEVLINK_CMD_MAX,
> >       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >@@ -276,6 +279,31 @@ enum {
> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> >       (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> >
> >+/* Commonly used test cases */
>
> What do you mean by "commonly". Are there some others that are not
> "common"? I don't follow.
>
>
> >+enum devlink_selftest_attr {
> >+      DEVLINK_SELFTEST_ATTR_UNSPEC,
> >+      DEVLINK_SELFTEST_ATTR_FLASH,            /* flag */
> >+
> >+      __DEVLINK_SELFTEST_ATTR_MAX,
> >+      DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
> >+};
>
> To be consistent with the attr that caries this:
>
> enum devlink_attr_selftest_test_id {
>         DEVLINK_ATTR_SELFTEST_TEST_ID_UNSPEC,
>         DEVLINK_ATTR_SELFTEST_TEST_ID_FLASH,            /* flag */
>
>         __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX,
>         DEVLINK_ATTR_SELFTEST_TEST_ID_MAX = __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX - 1
>
> >+
> >+enum devlink_selftest_result {
> >+      DEVLINK_SELFTEST_SKIP,
> >+      DEVLINK_SELFTEST_PASS,
> >+      DEVLINK_SELFTEST_FAIL
>
> It is common to have the enum name be root of names of the values.
> Also, be consistent with the attr this value is carried over:
>
> enum devlink_selftest_test_status {
>         DEVLINK_SELFTEST_TEST_STATUS_SKIP,
>         DEVLINK_SELFTEST_TEST_STATUS_PASS,
>         DEVLINK_SELFTEST_TEST_STATUS_FAIL
>
> That way, it is obvious to which enum the value belongs.
>
>
> >+};
> >+
> >+enum devlink_selftest_result_attr {
> >+      DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
> >+      DEVLINK_SELFTEST_ATTR_RESULT,           /* nested */
> >+      DEVLINK_SELFTEST_ATTR_TEST_ID,          /* u32, devlink_selftest_attr */
>
> add "enum" ?
>
> >+      DEVLINK_SELFTEST_ATTR_TEST_STATUS,      /* u8, devlink_selftest_result */
>
> add "enum" ?
>
> The same note as above:
> enum devlink_attr_selftest_result {
>         DEVLINK_ATTR_SELFTEST_RESULT_UNSPEC,
>         DEVLINK_ATTR_SELFTEST_RESULT,             /* nested */
>         DEVLINK_ATTR_SELFTEST_RESULT_TEST_ID,     /* u32, enum devlink_selftest_attr */
>         DEVLINK_ATTR_SELFTEST_RESULT_TEST_STATUS, /* u8, devlink_selftest_result */
>
>
>
>
> >+
> >+      __DEVLINK_SELFTEST_ATTR_RES_MAX,
> >+      DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
> >+};
> >+
> > /**
> >  * enum devlink_trap_action - Packet trap action.
> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
> >@@ -576,6 +604,7 @@ enum devlink_attr {
> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
> >
> >+      DEVLINK_ATTR_SELFTESTS_INFO,            /* nested */
> >       /* add new attributes above here, update the policy in devlink.c */
> >
> >       __DEVLINK_ATTR_MAX,
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index a9776ea923ae..ef9439f2502f 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> >                                DEVLINK_PORT_FN_STATE_ACTIVE),
> > };
> >
> >+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
> >+      [DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
> >+};
> >+
> > static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> > #define DEVLINK_REGISTERED XA_MARK_1
> >
> >@@ -4791,6 +4795,215 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> >       return ret;
> > }
> >
> >+static int
> >+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
> >+                        u32 portid, u32 seq, int flags,
> >+                        struct netlink_ext_ack *extack)
> >+{
> >+      struct nlattr *selftests_list;
> >+      void *hdr;
> >+      int err;
> >+      int i;
> >+
> >+      hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
> >+                        DEVLINK_CMD_SELFTESTS_LIST);
> >+      if (!hdr)
> >+              return -EMSGSIZE;
> >+
> >+      err = -EMSGSIZE;
> >+      if (devlink_nl_put_handle(msg, devlink))
> >+              goto err_cancel_msg;
> >+
> >+      selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
> >+      if (!selftests_list)
> >+              goto err_cancel_msg;
> >+
> >+      for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
>
> **)
> It is a bit odd to see "1" here. Maybe "DEVLINK_SELFTEST_ATTR_UNSPEC + 1"
> would be more obvious for the reader.
>
> also:
> i < DEVLINK_SELFTEST_ATTR_MAX + 1
> would be rather nicer to be:
> i <= DEVLINK_SELFTEST_ATTR_MAX
>
>
> >+              if (devlink->ops->selftest_check(devlink, i, extack)) {
> >+                      err = nla_put_flag(msg, i);
> >+                      if (err)
> >+                              goto err_cancel_msg;
> >+              }
> >+      }
> >+
> >+      nla_nest_end(msg, selftests_list);
> >+
>
> No need for this empty line.
>
>
> >+      genlmsg_end(msg, hdr);
> >+
>
> No need for this empty line.
>
>
> >+      return 0;
> >+
> >+err_cancel_msg:
> >+      genlmsg_cancel(msg, hdr);
> >+      return err;
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
> >+                                            struct genl_info *info)
> >+{
> >+      struct devlink *devlink = info->user_ptr[0];
> >+      struct sk_buff *msg;
> >+      int err;
> >+
> >+      if (!devlink->ops->selftest_check)
> >+              return -EOPNOTSUPP;
> >+
> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >+      if (!msg)
> >+              return -ENOMEM;
> >+
> >+      err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
> >+                                      info->snd_seq, 0, info->extack);
> >+      if (err) {
> >+              nlmsg_free(msg);
> >+              return err;
> >+      }
> >+
> >+      return genlmsg_reply(msg, info);
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
> >+                                              struct netlink_callback *cb)
> >+{
> >+      struct devlink *devlink;
> >+      int start = cb->args[0];
> >+      unsigned long index;
> >+      int idx = 0;
> >+      int err = 0;
> >+
> >+      mutex_lock(&devlink_mutex);
> >+      xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> >+              if (!devlink_try_get(devlink))
> >+                      continue;
> >+
> >+              if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> >+                      goto retry;
> >+
> >+              if (idx < start || !devlink->ops->selftest_check)
> >+                      goto inc;
> >+
> >+              mutex_lock(&devlink->lock);
> >+              err = devlink_nl_selftests_fill(msg, devlink,
> >+                                              NETLINK_CB(cb->skb).portid,
> >+                                              cb->nlh->nlmsg_seq, NLM_F_MULTI,
> >+                                              cb->extack);
> >+              mutex_unlock(&devlink->lock);
> >+              if (err) {
> >+                      devlink_put(devlink);
> >+                      break;
> >+              }
> >+inc:
> >+              idx++;
> >+retry:
> >+              devlink_put(devlink);
> >+      }
> >+      mutex_unlock(&devlink_mutex);
> >+
> >+      if (err != -EMSGSIZE)
> >+              return err;
> >+
> >+      cb->args[0] = idx;
> >+      return msg->len;
> >+}
> >+
> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,
>
> unsigned.
>
> >+                                     u8 result)
>
> Please be consistend and call this "test_status"
>
>
> >+{
> >+      struct nlattr *result_attr;
> >+
> >+      result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
> >+      if (!result_attr)
> >+              return -EMSGSIZE;
> >+
> >+      if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
> >+          nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, result))
> >+              goto nla_put_failure;
> >+
> >+      nla_nest_end(skb, result_attr);
> >+
>
> No need for this empty line.
>
>
> >+      return 0;
> >+
> >+nla_put_failure:
> >+      nla_nest_cancel(skb, result_attr);
> >+      return -EMSGSIZE;
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
> >+                                      struct genl_info *info)
> >+{
> >+      struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
> >+      struct devlink *devlink = info->user_ptr[0];
> >+      struct nlattr *attrs, *tests_info;
> >+      struct sk_buff *msg;
> >+      void *hdr;
> >+      int err;
> >+      int i;
> >+
> >+      if (!devlink->ops->selftest_run)
> >+              return -EOPNOTSUPP;
> >+
> >+      if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])
>
> Fill extack message here please.
>
>
> >+              return -EINVAL;
> >+
> >+      attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
> >+
> >+      err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
> >+                             devlink_selftest_nl_policy, info->extack);
> >+      if (err < 0)
> >+              return err;
> >+
> >+      msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >+      if (!msg)
> >+              return -ENOMEM;
> >+
> >+      err = -EMSGSIZE;
> >+      hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >+                        &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
> >+      if (!hdr)
> >+              goto free_msg;
> >+
> >+      if (devlink_nl_put_handle(msg, devlink))
> >+              goto genlmsg_cancel;
> >+
> >+      tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
> >+      if (!tests_info)
> >+              goto genlmsg_cancel;
> >+
> >+      for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
>
> Same notes to the iteration as above. **
>
>
> >+              u8 res = DEVLINK_SELFTEST_SKIP;
>
> u8 test_status;
>
>
> >+
> >+              if (nla_get_flag(tb[i])) {
> >+                      if (devlink->ops->selftest_check &&
>
> No need to test in every iteration. I think it is safe to assume
> that driver that does not fill selftest_check() does not support
> selftests at all, so please move to the beginning of this function
> alongside selftest_run() check:
>
>         if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
>                 return -EOPNOTSUPP;
>
> >+                          !devlink->ops->selftest_check(devlink, i,
> >+                                                        info->extack)) {
> >+                              err = devlink_selftest_result_put(msg, i, res);
>
> Just do devlink_selftest_result_put(msg, i, .._SKIP); here and avoid
> initializing "res" at the beginning.
>
>
> >+                              if (err)
> >+                                      goto selftests_list_nest_cancel;
> >+                              continue;
> >+                      }
> >+
> >+                      res = devlink->ops->selftest_run(devlink, i,
> >+                                                       info->extack);
> >+                      err = devlink_selftest_result_put(msg, i, res);
> >+                      if (err)
> >+                              goto selftests_list_nest_cancel;
> >+              }
> >+      }
> >+
> >+      nla_nest_end(msg, tests_info);
> >+
>
> No need for this empty line.
>
>
> >+      genlmsg_end(msg, hdr);
> >+
>
> No need for this empty line.
>
>
> >+      return genlmsg_reply(msg, info);
> >+
> >+selftests_list_nest_cancel:
> >+      nla_nest_cancel(msg, tests_info);
> >+genlmsg_cancel:
> >+      genlmsg_cancel(msg, hdr);
> >+free_msg:
> >+      nlmsg_free(msg);
> >+      return err;
> >+}
> >+
> > static const struct devlink_param devlink_param_generic[] = {
> >       {
> >               .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
> >@@ -8997,6 +9210,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> >       [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> >       [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> >       [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
> >+      [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
> > };
> >
> > static const struct genl_small_ops devlink_nl_ops[] = {
> >@@ -9356,6 +9570,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> >               .doit = devlink_nl_cmd_trap_policer_set_doit,
> >               .flags = GENL_ADMIN_PERM,
> >       },
> >+      {
> >+              .cmd = DEVLINK_CMD_SELFTESTS_LIST,
> >+              .doit = devlink_nl_cmd_selftests_list_doit,
> >+              .dumpit = devlink_nl_cmd_selftests_list_dumpit
> >+              /* can be retrieved by unprivileged users */
> >+      },
> >+      {
> >+              .cmd = DEVLINK_CMD_SELFTESTS_RUN,
> >+              .doit = devlink_nl_cmd_selftests_run,
> >+              .flags = GENL_ADMIN_PERM,
> >+      },
> > };
> >
> > static struct genl_family devlink_nl_family __ro_after_init = {
> >--
> >2.31.1
> >
>
>

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

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

[Jiri Pirko]

Thu, Jul 21, 2022 at 07:32:08PM CEST, vikas.gupta@broadcom.com wrote:
>Hi Jiri,
>
>
>On Thu, Jul 21, 2022 at 6:27 PM Jiri Pirko <jiri@nvidia.com> wrote:
>>
>> Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
>> >selftests by the drivers.
>> >devlink_nl_cmd_selftests_run: To execute selftests. Users can
>> >provide a test mask for executing group tests or standalone tests.
>> >
>> >Documentation/networking/devlink/ path is already part of MAINTAINERS &
>> >the new files come under this path. Hence no update needed to the
>> >MAINTAINERS
>> >
>> >Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
>> >Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
>> >---
>> > .../networking/devlink/devlink-selftests.rst  |  38 +++
>> > include/net/devlink.h                         |  20 ++
>> > include/uapi/linux/devlink.h                  |  29 +++
>> > net/core/devlink.c                            | 225 ++++++++++++++++++
>> > 4 files changed, 312 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..0e9727895987
>> >--- /dev/null
>> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
>> >@@ -0,0 +1,38 @@
>> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >+
>> >+=================
>> >+Devlink Selftests
>> >+=================
>> >+
>> >+The ``devlink-selftests`` API allows executing selftests on the device.
>> >+
>> >+Tests Mask
>> >+==========
>> >+The ``devlink-selftests`` command should be run with a mask indicating
>> >+the tests to be executed.
>> >+
>> >+Tests Description
>> >+=================
>> >+The following is a list of tests that drivers may execute.
>> >+
>> >+.. list-table:: List of tests
>> >+   :widths: 5 90
>> >+
>> >+   * - Name
>> >+     - Description
>> >+   * - ``DEVLINK_SELFTEST_FLASH``
>> >+     - Devices may have the firmware on non-volatile memory on the board, e.g.
>> >+       flash. This particular test helps to run a flash selftest on the device.
>> >+       Implementation of the test is left to the driver/firmware.
>> >+
>> >+example usage
>> >+-------------
>> >+
>> >+.. code:: shell
>> >+
>> >+    # Query selftests supported on the devlink device
>> >+    $ devlink dev selftests show DEV
>> >+    # Query selftests supported on all devlink devices
>> >+    $ devlink dev selftests show
>> >+    # Executes selftests on the device
>> >+    $ devlink dev selftests run DEV test flash
>>
>> "test_id" to be consistend with the attr name and outputs. Please see
>What is "test_id" referring to in this document? Can you please elaborate ?

"test_id" is consistent with the UAPI netlink attribute name, see
below.

>
>> below. Devlink cmdline would accept "test" as well, so you can still use
>Are you mentioning the "test" argument in the above devlink command line option?

Yes, the cmd line accepts shortened option names. So for "test_id", it
would accept:
"test_i"
"test_"
and "test"

I just wanted to say you, as a user, will be able to still use the
shortened version, but the option name "test_id" would be consistent
with UAPI, which is what we usually have.