| Message ID | 20220721072121.43648-2-vikas.gupta@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add framework for selftests in devlink | expand |
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 >
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); > }; [...]
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 > > > >
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.
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 = {