Message-ID: <Ys0UpFtcOGWjK/sZ@nanopsycho>
Date:   Tue, 12 Jul 2022 08:28:52 +0200
From:   Jiri Pirko <jiri@...dia.com>
To:     Vikas Gupta <vikas.gupta@...adcom.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>, dsahern@...nel.org,
        stephen@...workplumber.org, Eric Dumazet <edumazet@...gle.com>,
        pabeni@...hat.com, ast@...nel.org, leon@...nel.org,
        linux-doc@...r.kernel.org, corbet@....net,
        Michael Chan <michael.chan@...adcom.com>,
        Andrew Gospodarek <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v2 1/3] devlink: introduce framework for
 selftests

Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@...adcom.com wrote:
>Hi Jiri,
>
>On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@...dia.com> wrote:
>
>> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@...adcom.com wrote:
>> >Add a framework for running selftests.
>> >Framework exposes devlink commands and test suite(s) to the user
>> >to execute and query the supported tests by the driver.
>> >
>> >Below are new entries in devlink_nl_ops
>> >devlink_nl_cmd_selftests_show: To query the supported selftests
>> >by the driver.
>> >devlink_nl_cmd_selftests_run: To execute selftests. Users can
>> >provide a test mask for executing group tests or standalone tests.
>> >
>> >Documentation/networking/devlink/ path is already part of MAINTAINERS &
>> >the new files come under this path. Hence no update needed to the
>> >MAINTAINERS
>> >
>> >Signed-off-by: Vikas Gupta <vikas.gupta@...adcom.com>
>> >Reviewed-by: Michael Chan <michael.chan@...adcom.com>
>> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@...adcom.com>
>> >---
>> > .../networking/devlink/devlink-selftests.rst  |  34 +++++
>> > include/net/devlink.h                         |  30 ++++
>> > include/uapi/linux/devlink.h                  |  26 ++++
>> > net/core/devlink.c                            | 144 ++++++++++++++++++
>> > 4 files changed, 234 insertions(+)
>> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
>> >
>> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst
>> b/Documentation/networking/devlink/devlink-selftests.rst
>> >new file mode 100644
>> >index 000000000000..796d38f77038
>> >--- /dev/null
>> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
>> >@@ -0,0 +1,34 @@
>> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >+
>> >+=================
>> >+Devlink Selftests
>> >+=================
>> >+
>> >+The ``devlink-selftests`` API allows executing selftests on the device.
>> >+
>> >+Tests Mask
>> >+==========
>> >+The ``devlink-selftests`` command should be run with a mask indicating
>> >+the tests to be executed.
>> >+
>> >+Tests Description
>> >+=================
>> >+The following is a list of tests that drivers may execute.
>> >+
>> >+.. list-table:: List of tests
>> >+   :widths: 5 90
>> >+
>> >+   * - Name
>> >+     - Description
>> >+   * - ``DEVLINK_SELFTEST_FLASH``
>> >+     - Runs a flash test on the device.
>> >+
>> >+example usage
>> >+-------------
>> >+
>> >+.. code:: shell
>> >+
>> >+    # Query selftests supported on the device
>> >+    $ devlink dev selftests show DEV
>> >+    # Executes selftests on the device
>> >+    $ devlink dev selftests run DEV test {flash | all}
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 2a2a2a0c93f7..cb7c378cf720 100644
>> >--- a/include/net/devlink.h
>> >+++ b/include/net/devlink.h
>> >@@ -1215,6 +1215,18 @@ enum {
>> >       DEVLINK_F_RELOAD = 1UL << 0,
>> > };
>> >
>> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
>> >+
>> >+static inline const char *devlink_selftest_name(int test)
>>
>> I don't understand why this is needed. Better not to expose string to
>> the user. Just have it as well defined attr.
>
>
> OK. Will remove this function and corresponding attr
>DEVLINK_ATTR_TEST_NAME added in this patch.
>
>
>
>
>>
>> >+{
>> >+      switch (test) {
>> >+      case DEVLINK_SELFTEST_FLASH_BIT:
>> >+              return DEVLINK_SELFTEST_FLASH_TEST_NAME;
>> >+      default:
>> >+              return "unknown";
>> >+      }
>> >+}
>> >+
>> > struct devlink_ops {
>> >       /**
>> >        * @supported_flash_update_params:
>> >@@ -1509,6 +1521,24 @@ struct devlink_ops {
>> >                                   struct devlink_rate *parent,
>> >                                   void *priv_child, void *priv_parent,
>> >                                   struct netlink_ext_ack *extack);
>> >+      /**
>> >+       * selftests_show() - Shows selftests supported by device
>> >+       * @devlink: Devlink instance
>> >+       * @extack: extack for reporting error messages
>> >+       *
>> >+       * Return: test mask supported by driver
>> >+       */
>> >+      u32 (*selftests_show)(struct devlink *devlink,
>> >+                            struct netlink_ext_ack *extack);
>> >+      /**
>> >+       * selftests_run() - Runs selftests
>> >+       * @devlink: Devlink instance
>> >+       * @tests_mask: tests to be run by driver
>> >+       * @results: test results by driver
>> >+       * @extack: extack for reporting error messages
>> >+       */
>> >+      void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
>> >+                            u8 *results, struct netlink_ext_ack *extack);
>> > };
>> >
>> > void *devlink_priv(struct devlink *devlink);
>> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> >index b3d40a5d72ff..1dba262328b9 100644
>> >--- a/include/uapi/linux/devlink.h
>> >+++ b/include/uapi/linux/devlink.h
>> >@@ -136,6 +136,9 @@ enum devlink_command {
>> >       DEVLINK_CMD_LINECARD_NEW,
>> >       DEVLINK_CMD_LINECARD_DEL,
>> >
>> >+      DEVLINK_CMD_SELFTESTS_SHOW,
>> >+      DEVLINK_CMD_SELFTESTS_RUN,
>> >+
>> >       /* add new commands above here */
>> >       __DEVLINK_CMD_MAX,
>> >       DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>> >@@ -276,6 +279,25 @@ enum {
>> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
>> >       (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
>> >
>> >+/* Commonly used test cases */
>> >+enum {
>> >+      DEVLINK_SELFTEST_FLASH_BIT,
>> >+
>> >+      __DEVLINK_SELFTEST_MAX_BIT,
>> >+      DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
>> >+};
>> >+
>> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
>> >+
>> >+#define DEVLINK_SELFTESTS_MASK \
>> >+      (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
>> >+
>> >+enum {
>> >+      DEVLINK_SELFTEST_SKIP,
>> >+      DEVLINK_SELFTEST_PASS,
>> >+      DEVLINK_SELFTEST_FAIL
>> >+};
>> >+
>> > /**
>> >  * enum devlink_trap_action - Packet trap action.
>> >  * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> is not
>> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >       DEVLINK_ATTR_LINECARD_TYPE,             /* string */
>> >       DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES,  /* nested */
>> >
>> >+      DEVLINK_ATTR_SELFTESTS_MASK,            /* u32 */
>>
>> I don't see why this is u32 bitset. Just have one attr per test
>> (NLA_FLAG) in a nested attr instead.
>>
>
>As per your suggestion, for an example it should be like as below
>
>        DEVLINK_ATTR_SELFTESTS,                 /* nested */
>
>        DEVLINK_ATTR_SELFTESTS_SOMETEST1            /* flag */
>
>        DEVLINK_ATTR_SELFTESTS_SOMETEST2           /* flag */

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


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

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


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

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


>
>
>>
>> >+                                  info->extack);
>> >+      tests = tests_mask;
>> >+
>> >+      for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
>> >+              err = devlink_selftest_result_put(msg, test,
>> >+                                                test_results[test]);
>> >+              if (err)
>> >+                      goto genlmsg_cancel;
>> >+      }
>> >+
>> >+      genlmsg_end(msg, hdr);
>> >+
>> >+      return genlmsg_reply(msg, info);
>> >+
>> >+genlmsg_cancel:
>> >+      genlmsg_cancel(msg, hdr);
>> >+free_msg:
>> >+      nlmsg_free(msg);
>> >+      return err;
>> >+}
>> >+
>> > static const struct devlink_param devlink_param_generic[] = {
>> >       {
>> >               .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
>> >@@ -9000,6 +9130,8 @@ static const struct nla_policy
>> devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
>> >       [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
>> >       [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
>> >       [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
>> >+      [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32,
>> >+
>> DEVLINK_SELFTESTS_MASK),
>> > };
>> >
>> > static const struct genl_small_ops devlink_nl_ops[] = {
>> >@@ -9361,6 +9493,18 @@ static const struct genl_small_ops
>> devlink_nl_ops[] = {
>> >               .doit = devlink_nl_cmd_trap_policer_set_doit,
>> >               .flags = GENL_ADMIN_PERM,
>> >       },
>> >+      {
>> >+              .cmd = DEVLINK_CMD_SELFTESTS_SHOW,
>> >+              .validate = GENL_DONT_VALIDATE_STRICT |
>> GENL_DONT_VALIDATE_DUMP,
>> >+              .doit = devlink_nl_cmd_selftests_show,
>>
>> Why don't dump?
>>
>
>  I`ll add a dump in the next patchset.
>
>Thanks,
>Vikas
>
>
>>
>>
>> >+              .flags = GENL_ADMIN_PERM,
>> >+      },
>> >+      {
>> >+              .cmd = DEVLINK_CMD_SELFTESTS_RUN,
>> >+              .validate = GENL_DONT_VALIDATE_STRICT |
>> GENL_DONT_VALIDATE_DUMP,
>> >+              .doit = devlink_nl_cmd_selftests_run,
>> >+              .flags = GENL_ADMIN_PERM,
>> >+      },
>> > };
>> >
>> > static struct genl_family devlink_nl_family __ro_after_init = {
>> >--
>> >2.31.1
>> >
>>
>>
>>

Powered by blists - more mailing lists