| Message ID | 20220707182950.29348-2-vikas.gupta@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add framework for selftests in devlink | expand |
On Thu, 7 Jul 2022 23:59:48 +0530 Vikas Gupta wrote: > + * - Name > + - Description > + * - ``DEVLINK_SELFTEST_FLASH`` > + - Runs a flash test on the device. A little more info on what "flash test" does would be useful. > + DEVLINK_CMD_SELFTESTS_SHOW, nit: _LIST? > /** > * enum devlink_trap_action - Packet trap action. > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not > @@ -576,6 +598,10 @@ enum devlink_attr { > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > > + DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ Can we make the uAPI field 64b just in case we need more bits? Internally we can keep using u32 doesn't matter. > + DEVLINK_ATTR_TEST_RESULT, /* nested */ > + DEVLINK_ATTR_TEST_NAME, /* string */ > + DEVLINK_ATTR_TEST_RESULT_VAL, /* u8 */ > /* add new attributes above here, update the policy in devlink.c */ > > __DEVLINK_ATTR_MAX, > diff --git a/net/core/devlink.c b/net/core/devlink.c > index db61f3a341cb..0b7341ab6379 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > return ret; > } > > +static int devlink_selftest_name_put(struct sk_buff *skb, int test) > +{ > + const char *name = devlink_selftest_name(test); empty line > + if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name)) > + return -EMSGSIZE; > + > + return 0; > +} This wrapper feels slightly unnecessary, it's only used once AFAICT. Of you want to keep it you should compress it to one stmt: static int devlink_selftest_name_put(struct sk_buff *skb, int test) { return nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, devlink_selftest_name(test))); } > +static int devlink_selftest_result_put(struct sk_buff *skb, int test, > + u8 result) > +{ > + const char *name = devlink_selftest_name(test); > + struct nlattr *result_attr; > + > + result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT); I think we can use the normal (non-_noflag) nests in new devlink code. > + if (!result_attr) > + return -EMSGSIZE; > + > + if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) || > + nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result)) > + goto nla_put_failure; > + > + nla_nest_end(skb, result_attr); > + > + return 0; > + > +nla_put_failure: > + nla_nest_cancel(skb, result_attr); > + return -EMSGSIZE; > +} > + > +static int devlink_nl_cmd_selftests_run(struct sk_buff *skb, > + struct genl_info *info) > +{ > + u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {}; > + struct devlink *devlink = info->user_ptr[0]; > + unsigned long tests; > + struct sk_buff *msg; > + u32 tests_mask; > + void *hdr; > + int err = 0; reverse xmas tree, but you probably don't want this init > + int test; > + > + if (!devlink->ops->selftests_run) > + return -EOPNOTSUPP; > + > + if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]) > + return -EINVAL; > + > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); > + if (!msg) > + return -ENOMEM; > + > + hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, > + &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN); > + if (!hdr) > + goto free_msg; err is not set here > + if (devlink_nl_put_handle(msg, devlink)) > + goto genlmsg_cancel; or here > + tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]); > + > + devlink->ops->selftests_run(devlink, tests_mask, test_results, > + info->extack); > + tests = tests_mask; > + > + for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { > + err = devlink_selftest_result_put(msg, test, > + test_results[test]); > + if (err) > + goto genlmsg_cancel; > + } > + > + genlmsg_end(msg, hdr); > + > + return genlmsg_reply(msg, info); > + > +genlmsg_cancel: > + genlmsg_cancel(msg, hdr); > +free_msg: > + nlmsg_free(msg); > + return err; > +} > + > static const struct devlink_param devlink_param_generic[] = { > { > .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET, > @@ -9000,6 +9130,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, > + [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32, > + DEVLINK_SELFTESTS_MASK), > }; > > static const struct genl_small_ops devlink_nl_ops[] = { > @@ -9361,6 +9493,18 @@ static const struct genl_small_ops devlink_nl_ops[] = { > .doit = devlink_nl_cmd_trap_policer_set_doit, > .flags = GENL_ADMIN_PERM, > }, > + { > + .cmd = DEVLINK_CMD_SELFTESTS_SHOW, > + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, I think we can validate strict for new commands, so no validation flags needed. > + .doit = devlink_nl_cmd_selftests_show, What about dump? Listing all tests on all devices? > + .flags = GENL_ADMIN_PERM, > + }, > + { > + .cmd = DEVLINK_CMD_SELFTESTS_RUN, > + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, > + .doit = devlink_nl_cmd_selftests_run, > + .flags = GENL_ADMIN_PERM, > + }, > }; > > static struct genl_family devlink_nl_family __ro_after_init = {
Hi Vikas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Vikas-Gupta/devlink-introduce-framework-for-selftests/20220708-033020
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cf21b355ccb39b0de0b6a7362532bb5584c84a80
config: arm64-allyesconfig
compiler: aarch64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/5c0b96e4473bc2ce567607d43fa8c8f27db92c17
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vikas-Gupta/devlink-introduce-framework-for-selftests/20220708-033020
git checkout 5c0b96e4473bc2ce567607d43fa8c8f27db92c17
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash net/core/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/string.h:253,
from include/linux/bitmap.h:11,
from include/linux/cpumask.h:12,
from include/linux/smp.h:13,
from arch/arm64/include/asm/arch_timer.h:18,
from arch/arm64/include/asm/timex.h:8,
from include/linux/timex.h:67,
from include/linux/time32.h:13,
from include/linux/time.h:60,
from include/linux/skbuff.h:15,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from net/core/devlink.c:10:
net/core/devlink.c: In function 'devlink_nl_cmd_selftests_show':
>> include/linux/fortify-string.h:24:45: warning: array subscript 7 is outside array bounds of 'char[6]' [-Warray-bounds]
24 | if (__builtin_constant_p(__p[__p_len]) && \
| ~~~^~~~~~~~~
include/linux/fortify-string.h:108:24: note: in expansion of macro '__compiletime_strlen'
108 | size_t p_len = __compiletime_strlen(p);
| ^~~~~~~~~~~~~~~~~~~~
net/core/devlink.c: In function 'devlink_nl_cmd_selftests_run':
>> include/linux/fortify-string.h:24:45: warning: array subscript 7 is outside array bounds of 'char[6]' [-Warray-bounds]
24 | if (__builtin_constant_p(__p[__p_len]) && \
| ~~~^~~~~~~~~
include/linux/fortify-string.h:108:24: note: in expansion of macro '__compiletime_strlen'
108 | size_t p_len = __compiletime_strlen(p);
| ^~~~~~~~~~~~~~~~~~~~
vim +24 include/linux/fortify-string.h
a28a6e860c6cf23 Francis Laniel 2021-02-25 16
3009f891bb9f328 Kees Cook 2021-08-02 17 #define __compiletime_strlen(p) \
3009f891bb9f328 Kees Cook 2021-08-02 18 ({ \
3009f891bb9f328 Kees Cook 2021-08-02 19 unsigned char *__p = (unsigned char *)(p); \
95cadae320be465 Qian Cai 2021-10-25 20 size_t __ret = (size_t)-1; \
95cadae320be465 Qian Cai 2021-10-25 21 size_t __p_size = __builtin_object_size(p, 1); \
95cadae320be465 Qian Cai 2021-10-25 22 if (__p_size != (size_t)-1) { \
95cadae320be465 Qian Cai 2021-10-25 23 size_t __p_len = __p_size - 1; \
95cadae320be465 Qian Cai 2021-10-25 @24 if (__builtin_constant_p(__p[__p_len]) && \
95cadae320be465 Qian Cai 2021-10-25 25 __p[__p_len] == '\0') \
95cadae320be465 Qian Cai 2021-10-25 26 __ret = __builtin_strlen(__p); \
3009f891bb9f328 Kees Cook 2021-08-02 27 } \
95cadae320be465 Qian Cai 2021-10-25 28 __ret; \
3009f891bb9f328 Kees Cook 2021-08-02 29 })
3009f891bb9f328 Kees Cook 2021-08-02 30
Hi Vikas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Vikas-Gupta/devlink-introduce-framework-for-selftests/20220708-033020
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git cf21b355ccb39b0de0b6a7362532bb5584c84a80
reproduce: make htmldocs
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> Documentation/networking/devlink/devlink-selftests.rst: WARNING: document isn't included in any toctree
On Thu, Jul 07, 2022 at 06:20:22PM -0700, Jakub Kicinski wrote: > On Thu, 7 Jul 2022 23:59:48 +0530 Vikas Gupta wrote: > > static const struct genl_small_ops devlink_nl_ops[] = { > > @@ -9361,6 +9493,18 @@ static const struct genl_small_ops devlink_nl_ops[] = { > > .doit = devlink_nl_cmd_trap_policer_set_doit, > > .flags = GENL_ADMIN_PERM, > > }, > > + { > > + .cmd = DEVLINK_CMD_SELFTESTS_SHOW, > > + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, > > I think we can validate strict for new commands, so no validation flags > needed. > > > + .doit = devlink_nl_cmd_selftests_show, > > What about dump? Listing all tests on all devices? > > > + .flags = GENL_ADMIN_PERM, Related to Jakub's question, is there a reason that the show command requires 'GENL_ADMIN_PERM' ? > > + }, > > + { > > + .cmd = DEVLINK_CMD_SELFTESTS_RUN, > > + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, > > + .doit = devlink_nl_cmd_selftests_run, > > + .flags = GENL_ADMIN_PERM, > > + }, > > }; > > > > static struct genl_family devlink_nl_family __ro_after_init = { >
Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: >Add a framework for running selftests. >Framework exposes devlink commands and test suite(s) to the user >to execute and query the supported tests by the driver. > >Below are new entries in devlink_nl_ops >devlink_nl_cmd_selftests_show: To query the supported selftests >by the driver. >devlink_nl_cmd_selftests_run: To execute selftests. Users can >provide a test mask for executing group tests or standalone tests. > >Documentation/networking/devlink/ path is already part of MAINTAINERS & >the new files come under this path. Hence no update needed to the >MAINTAINERS > >Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com> >Reviewed-by: Michael Chan <michael.chan@broadcom.com> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com> >--- > .../networking/devlink/devlink-selftests.rst | 34 +++++ > include/net/devlink.h | 30 ++++ > include/uapi/linux/devlink.h | 26 ++++ > net/core/devlink.c | 144 ++++++++++++++++++ > 4 files changed, 234 insertions(+) > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst > >diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst >new file mode 100644 >index 000000000000..796d38f77038 >--- /dev/null >+++ b/Documentation/networking/devlink/devlink-selftests.rst >@@ -0,0 +1,34 @@ >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >+ >+================= >+Devlink Selftests >+================= >+ >+The ``devlink-selftests`` API allows executing selftests on the device. >+ >+Tests Mask >+========== >+The ``devlink-selftests`` command should be run with a mask indicating >+the tests to be executed. >+ >+Tests Description >+================= >+The following is a list of tests that drivers may execute. >+ >+.. list-table:: List of tests >+ :widths: 5 90 >+ >+ * - Name >+ - Description >+ * - ``DEVLINK_SELFTEST_FLASH`` >+ - Runs a flash test on the device. >+ >+example usage >+------------- >+ >+.. code:: shell >+ >+ # Query selftests supported on the device >+ $ devlink dev selftests show DEV >+ # Executes selftests on the device >+ $ devlink dev selftests run DEV test {flash | all} >diff --git a/include/net/devlink.h b/include/net/devlink.h >index 2a2a2a0c93f7..cb7c378cf720 100644 >--- a/include/net/devlink.h >+++ b/include/net/devlink.h >@@ -1215,6 +1215,18 @@ enum { > DEVLINK_F_RELOAD = 1UL << 0, > }; > >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash" >+ >+static inline const char *devlink_selftest_name(int test) I don't understand why this is needed. Better not to expose string to the user. Just have it as well defined attr. >+{ >+ switch (test) { >+ case DEVLINK_SELFTEST_FLASH_BIT: >+ return DEVLINK_SELFTEST_FLASH_TEST_NAME; >+ default: >+ return "unknown"; >+ } >+} >+ > struct devlink_ops { > /** > * @supported_flash_update_params: >@@ -1509,6 +1521,24 @@ struct devlink_ops { > struct devlink_rate *parent, > void *priv_child, void *priv_parent, > struct netlink_ext_ack *extack); >+ /** >+ * selftests_show() - Shows selftests supported by device >+ * @devlink: Devlink instance >+ * @extack: extack for reporting error messages >+ * >+ * Return: test mask supported by driver >+ */ >+ u32 (*selftests_show)(struct devlink *devlink, >+ struct netlink_ext_ack *extack); >+ /** >+ * selftests_run() - Runs selftests >+ * @devlink: Devlink instance >+ * @tests_mask: tests to be run by driver >+ * @results: test results by driver >+ * @extack: extack for reporting error messages >+ */ >+ void (*selftests_run)(struct devlink *devlink, u32 tests_mask, >+ u8 *results, struct netlink_ext_ack *extack); > }; > > void *devlink_priv(struct devlink *devlink); >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >index b3d40a5d72ff..1dba262328b9 100644 >--- a/include/uapi/linux/devlink.h >+++ b/include/uapi/linux/devlink.h >@@ -136,6 +136,9 @@ enum devlink_command { > DEVLINK_CMD_LINECARD_NEW, > DEVLINK_CMD_LINECARD_DEL, > >+ DEVLINK_CMD_SELFTESTS_SHOW, >+ DEVLINK_CMD_SELFTESTS_RUN, >+ > /* add new commands above here */ > __DEVLINK_CMD_MAX, > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 >@@ -276,6 +279,25 @@ enum { > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \ > (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1) > >+/* Commonly used test cases */ >+enum { >+ DEVLINK_SELFTEST_FLASH_BIT, >+ >+ __DEVLINK_SELFTEST_MAX_BIT, >+ DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1 >+}; >+ >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT) >+ >+#define DEVLINK_SELFTESTS_MASK \ >+ (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1) >+ >+enum { >+ DEVLINK_SELFTEST_SKIP, >+ DEVLINK_SELFTEST_PASS, >+ DEVLINK_SELFTEST_FAIL >+}; >+ > /** > * enum devlink_trap_action - Packet trap action. > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not >@@ -576,6 +598,10 @@ enum devlink_attr { > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ I don't see why this is u32 bitset. Just have one attr per test (NLA_FLAG) in a nested attr instead. >+ DEVLINK_ATTR_TEST_RESULT, /* nested */ >+ DEVLINK_ATTR_TEST_NAME, /* string */ >+ DEVLINK_ATTR_TEST_RESULT_VAL, /* u8 */ Could you maintain the same "namespace" for all attrs related to selftests? > /* add new attributes above here, update the policy in devlink.c */ > > __DEVLINK_ATTR_MAX, >diff --git a/net/core/devlink.c b/net/core/devlink.c >index db61f3a341cb..0b7341ab6379 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > return ret; > } > >+static int devlink_selftest_name_put(struct sk_buff *skb, int test) >+{ >+ const char *name = devlink_selftest_name(test); >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name)) >+ return -EMSGSIZE; >+ >+ return 0; >+} >+ >+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb, >+ struct genl_info *info) >+{ >+ struct devlink *devlink = info->user_ptr[0]; >+ struct sk_buff *msg; >+ unsigned long tests; >+ int err = 0; >+ void *hdr; >+ int test; >+ >+ if (!devlink->ops->selftests_show) >+ return -EOPNOTSUPP; >+ >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); >+ if (!msg) >+ return -ENOMEM; >+ >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, >+ &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_SHOW); >+ if (!hdr) >+ goto free_msg; >+ >+ if (devlink_nl_put_handle(msg, devlink)) >+ goto genlmsg_cancel; >+ >+ tests = devlink->ops->selftests_show(devlink, info->extack); >+ >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { >+ err = devlink_selftest_name_put(msg, test); >+ if (err) >+ goto genlmsg_cancel; >+ } >+ >+ genlmsg_end(msg, hdr); >+ >+ return genlmsg_reply(msg, info); >+ >+genlmsg_cancel: >+ genlmsg_cancel(msg, hdr); >+free_msg: >+ nlmsg_free(msg); >+ return err; >+} >+ >+static int devlink_selftest_result_put(struct sk_buff *skb, int test, >+ u8 result) >+{ >+ const char *name = devlink_selftest_name(test); >+ struct nlattr *result_attr; >+ >+ result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT); >+ if (!result_attr) >+ return -EMSGSIZE; >+ >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) || >+ nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result)) >+ goto nla_put_failure; >+ >+ nla_nest_end(skb, result_attr); >+ >+ return 0; >+ >+nla_put_failure: >+ nla_nest_cancel(skb, result_attr); >+ return -EMSGSIZE; >+} >+ >+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb, >+ struct genl_info *info) >+{ >+ u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {}; >+ struct devlink *devlink = info->user_ptr[0]; >+ unsigned long tests; >+ struct sk_buff *msg; >+ u32 tests_mask; >+ void *hdr; >+ int err = 0; >+ int test; >+ >+ if (!devlink->ops->selftests_run) >+ return -EOPNOTSUPP; >+ >+ if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]) >+ return -EINVAL; >+ >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); >+ if (!msg) >+ return -ENOMEM; >+ >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, >+ &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN); >+ if (!hdr) >+ goto free_msg; >+ >+ if (devlink_nl_put_handle(msg, devlink)) >+ goto genlmsg_cancel; >+ >+ tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]); >+ >+ devlink->ops->selftests_run(devlink, tests_mask, test_results, Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too? >+ info->extack); >+ tests = tests_mask; >+ >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { >+ err = devlink_selftest_result_put(msg, test, >+ test_results[test]); >+ if (err) >+ goto genlmsg_cancel; >+ } >+ >+ genlmsg_end(msg, hdr); >+ >+ return genlmsg_reply(msg, info); >+ >+genlmsg_cancel: >+ genlmsg_cancel(msg, hdr); >+free_msg: >+ nlmsg_free(msg); >+ return err; >+} >+ > static const struct devlink_param devlink_param_generic[] = { > { > .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET, >@@ -9000,6 +9130,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, >+ [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32, >+ DEVLINK_SELFTESTS_MASK), > }; > > static const struct genl_small_ops devlink_nl_ops[] = { >@@ -9361,6 +9493,18 @@ static const struct genl_small_ops devlink_nl_ops[] = { > .doit = devlink_nl_cmd_trap_policer_set_doit, > .flags = GENL_ADMIN_PERM, > }, >+ { >+ .cmd = DEVLINK_CMD_SELFTESTS_SHOW, >+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, >+ .doit = devlink_nl_cmd_selftests_show, Why don't dump? >+ .flags = GENL_ADMIN_PERM, >+ }, >+ { >+ .cmd = DEVLINK_CMD_SELFTESTS_RUN, >+ .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, >+ .doit = devlink_nl_cmd_selftests_run, >+ .flags = GENL_ADMIN_PERM, >+ }, > }; > > static struct genl_family devlink_nl_family __ro_after_init = { >-- >2.31.1 >
Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: >Hi Jiri, > >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: > >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: >> >Add a framework for running selftests. >> >Framework exposes devlink commands and test suite(s) to the user >> >to execute and query the supported tests by the driver. >> > >> >Below are new entries in devlink_nl_ops >> >devlink_nl_cmd_selftests_show: To query the supported selftests >> >by the driver. >> >devlink_nl_cmd_selftests_run: To execute selftests. Users can >> >provide a test mask for executing group tests or standalone tests. >> > >> >Documentation/networking/devlink/ path is already part of MAINTAINERS & >> >the new files come under this path. Hence no update needed to the >> >MAINTAINERS >> > >> >Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com> >> >Reviewed-by: Michael Chan <michael.chan@broadcom.com> >> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com> >> >--- >> > .../networking/devlink/devlink-selftests.rst | 34 +++++ >> > include/net/devlink.h | 30 ++++ >> > include/uapi/linux/devlink.h | 26 ++++ >> > net/core/devlink.c | 144 ++++++++++++++++++ >> > 4 files changed, 234 insertions(+) >> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst >> > >> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst >> b/Documentation/networking/devlink/devlink-selftests.rst >> >new file mode 100644 >> >index 000000000000..796d38f77038 >> >--- /dev/null >> >+++ b/Documentation/networking/devlink/devlink-selftests.rst >> >@@ -0,0 +1,34 @@ >> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> >+ >> >+================= >> >+Devlink Selftests >> >+================= >> >+ >> >+The ``devlink-selftests`` API allows executing selftests on the device. >> >+ >> >+Tests Mask >> >+========== >> >+The ``devlink-selftests`` command should be run with a mask indicating >> >+the tests to be executed. >> >+ >> >+Tests Description >> >+================= >> >+The following is a list of tests that drivers may execute. >> >+ >> >+.. list-table:: List of tests >> >+ :widths: 5 90 >> >+ >> >+ * - Name >> >+ - Description >> >+ * - ``DEVLINK_SELFTEST_FLASH`` >> >+ - Runs a flash test on the device. >> >+ >> >+example usage >> >+------------- >> >+ >> >+.. code:: shell >> >+ >> >+ # Query selftests supported on the device >> >+ $ devlink dev selftests show DEV >> >+ # Executes selftests on the device >> >+ $ devlink dev selftests run DEV test {flash | all} >> >diff --git a/include/net/devlink.h b/include/net/devlink.h >> >index 2a2a2a0c93f7..cb7c378cf720 100644 >> >--- a/include/net/devlink.h >> >+++ b/include/net/devlink.h >> >@@ -1215,6 +1215,18 @@ enum { >> > DEVLINK_F_RELOAD = 1UL << 0, >> > }; >> > >> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash" >> >+ >> >+static inline const char *devlink_selftest_name(int test) >> >> I don't understand why this is needed. Better not to expose string to >> the user. Just have it as well defined attr. > > > OK. Will remove this function and corresponding attr >DEVLINK_ATTR_TEST_NAME added in this patch. > > > > >> >> >+{ >> >+ switch (test) { >> >+ case DEVLINK_SELFTEST_FLASH_BIT: >> >+ return DEVLINK_SELFTEST_FLASH_TEST_NAME; >> >+ default: >> >+ return "unknown"; >> >+ } >> >+} >> >+ >> > struct devlink_ops { >> > /** >> > * @supported_flash_update_params: >> >@@ -1509,6 +1521,24 @@ struct devlink_ops { >> > struct devlink_rate *parent, >> > void *priv_child, void *priv_parent, >> > struct netlink_ext_ack *extack); >> >+ /** >> >+ * selftests_show() - Shows selftests supported by device >> >+ * @devlink: Devlink instance >> >+ * @extack: extack for reporting error messages >> >+ * >> >+ * Return: test mask supported by driver >> >+ */ >> >+ u32 (*selftests_show)(struct devlink *devlink, >> >+ struct netlink_ext_ack *extack); >> >+ /** >> >+ * selftests_run() - Runs selftests >> >+ * @devlink: Devlink instance >> >+ * @tests_mask: tests to be run by driver >> >+ * @results: test results by driver >> >+ * @extack: extack for reporting error messages >> >+ */ >> >+ void (*selftests_run)(struct devlink *devlink, u32 tests_mask, >> >+ u8 *results, struct netlink_ext_ack *extack); >> > }; >> > >> > void *devlink_priv(struct devlink *devlink); >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> >index b3d40a5d72ff..1dba262328b9 100644 >> >--- a/include/uapi/linux/devlink.h >> >+++ b/include/uapi/linux/devlink.h >> >@@ -136,6 +136,9 @@ enum devlink_command { >> > DEVLINK_CMD_LINECARD_NEW, >> > DEVLINK_CMD_LINECARD_DEL, >> > >> >+ DEVLINK_CMD_SELFTESTS_SHOW, >> >+ DEVLINK_CMD_SELFTESTS_RUN, >> >+ >> > /* add new commands above here */ >> > __DEVLINK_CMD_MAX, >> > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 >> >@@ -276,6 +279,25 @@ enum { >> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \ >> > (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1) >> > >> >+/* Commonly used test cases */ >> >+enum { >> >+ DEVLINK_SELFTEST_FLASH_BIT, >> >+ >> >+ __DEVLINK_SELFTEST_MAX_BIT, >> >+ DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1 >> >+}; >> >+ >> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT) >> >+ >> >+#define DEVLINK_SELFTESTS_MASK \ >> >+ (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1) >> >+ >> >+enum { >> >+ DEVLINK_SELFTEST_SKIP, >> >+ DEVLINK_SELFTEST_PASS, >> >+ DEVLINK_SELFTEST_FAIL >> >+}; >> >+ >> > /** >> > * enum devlink_trap_action - Packet trap action. >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy >> is not >> >@@ -576,6 +598,10 @@ enum devlink_attr { >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> > >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ >> >> I don't see why this is u32 bitset. Just have one attr per test >> (NLA_FLAG) in a nested attr instead. >> > >As per your suggestion, for an example it should be like as below > > DEVLINK_ATTR_SELFTESTS, /* nested */ > > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ > > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ Yeah, but have the flags in separate enum, no need to pullute the devlink_attr enum by them. > >.... <SOME MORE TESTS> > >..... > > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ > > > > If we have this way then we need to have a mapping (probably a function) >for drivers to tell them what tests need to be executed based on the flags >that are set. > Does this look OK? > The rationale behind choosing a mask is that we could directly pass the >mask-value to the drivers. If you have separate enum, you can use the attrs as bits internally in kernel. Add a helper that would help the driver to work with it. Pass a struct containing u32 (or u8) not to drivers. Once there are more tests than that, this structure can be easily extended and the helpers changed. This would make this scalable. No need for UAPI change or even internel driver api change. > > >> >> >> >> >+ DEVLINK_ATTR_TEST_RESULT, /* nested */ >> >+ DEVLINK_ATTR_TEST_NAME, /* string */ >> >+ DEVLINK_ATTR_TEST_RESULT_VAL, /* u8 */ >> >> Could you maintain the same "namespace" for all attrs related to >> selftests? >> > >Will fix it. > > >> >> > /* add new attributes above here, update the policy in devlink.c */ >> > >> > __DEVLINK_ATTR_MAX, >> >diff --git a/net/core/devlink.c b/net/core/devlink.c >> >index db61f3a341cb..0b7341ab6379 100644 >> >--- a/net/core/devlink.c >> >+++ b/net/core/devlink.c >> >@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct >> sk_buff *skb, >> > return ret; >> > } >> > >> >+static int devlink_selftest_name_put(struct sk_buff *skb, int test) >> >+{ >> >+ const char *name = devlink_selftest_name(test); >> >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name)) >> >+ return -EMSGSIZE; >> >+ >> >+ return 0; >> >+} >> >+ >> >+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb, >> >+ struct genl_info *info) >> >+{ >> >+ struct devlink *devlink = info->user_ptr[0]; >> >+ struct sk_buff *msg; >> >+ unsigned long tests; >> >+ int err = 0; >> >+ void *hdr; >> >+ int test; >> >+ >> >+ if (!devlink->ops->selftests_show) >> >+ return -EOPNOTSUPP; >> >+ >> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); >> >+ if (!msg) >> >+ return -ENOMEM; >> >+ >> >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, >> >+ &devlink_nl_family, 0, >> DEVLINK_CMD_SELFTESTS_SHOW); >> >+ if (!hdr) >> >+ goto free_msg; >> >+ >> >+ if (devlink_nl_put_handle(msg, devlink)) >> >+ goto genlmsg_cancel; >> >+ >> >+ tests = devlink->ops->selftests_show(devlink, info->extack); >> >+ >> >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { >> >+ err = devlink_selftest_name_put(msg, test); >> >+ if (err) >> >+ goto genlmsg_cancel; >> >+ } >> >+ >> >+ genlmsg_end(msg, hdr); >> >+ >> >+ return genlmsg_reply(msg, info); >> >+ >> >+genlmsg_cancel: >> >+ genlmsg_cancel(msg, hdr); >> >+free_msg: >> >+ nlmsg_free(msg); >> >+ return err; >> >+} >> >+ >> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test, >> >+ u8 result) >> >+{ >> >+ const char *name = devlink_selftest_name(test); >> >+ struct nlattr *result_attr; >> >+ >> >+ result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT); >> >+ if (!result_attr) >> >+ return -EMSGSIZE; >> >+ >> >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) || >> >+ nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result)) >> >+ goto nla_put_failure; >> >+ >> >+ nla_nest_end(skb, result_attr); >> >+ >> >+ return 0; >> >+ >> >+nla_put_failure: >> >+ nla_nest_cancel(skb, result_attr); >> >+ return -EMSGSIZE; >> >+} >> >+ >> >+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb, >> >+ struct genl_info *info) >> >+{ >> >+ u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {}; >> >+ struct devlink *devlink = info->user_ptr[0]; >> >+ unsigned long tests; >> >+ struct sk_buff *msg; >> >+ u32 tests_mask; >> >+ void *hdr; >> >+ int err = 0; >> >+ int test; >> >+ >> >+ if (!devlink->ops->selftests_run) >> >+ return -EOPNOTSUPP; >> >+ >> >+ if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]) >> >+ return -EINVAL; >> >+ >> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); >> >+ if (!msg) >> >+ return -ENOMEM; >> >+ >> >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, >> >+ &devlink_nl_family, 0, >> DEVLINK_CMD_SELFTESTS_RUN); >> >+ if (!hdr) >> >+ goto free_msg; >> >+ >> >+ if (devlink_nl_put_handle(msg, devlink)) >> >+ goto genlmsg_cancel; >> >+ >> >+ tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]); >> >+ >> >+ devlink->ops->selftests_run(devlink, tests_mask, test_results, >> >> Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too? >> >> I`ll consider it in the next patch set. Please do. This array of results returned from driver looks sloppy. > > >> >> >+ info->extack); >> >+ tests = tests_mask; >> >+ >> >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { >> >+ err = devlink_selftest_result_put(msg, test, >> >+ test_results[test]); >> >+ if (err) >> >+ goto genlmsg_cancel; >> >+ } >> >+ >> >+ genlmsg_end(msg, hdr); >> >+ >> >+ return genlmsg_reply(msg, info); >> >+ >> >+genlmsg_cancel: >> >+ genlmsg_cancel(msg, hdr); >> >+free_msg: >> >+ nlmsg_free(msg); >> >+ return err; >> >+} >> >+ >> > static const struct devlink_param devlink_param_generic[] = { >> > { >> > .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET, >> >@@ -9000,6 +9130,8 @@ static const struct nla_policy >> devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { >> > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, >> > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, >> > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, >> >+ [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32, >> >+ >> DEVLINK_SELFTESTS_MASK), >> > }; >> > >> > static const struct genl_small_ops devlink_nl_ops[] = { >> >@@ -9361,6 +9493,18 @@ static const struct genl_small_ops >> devlink_nl_ops[] = { >> > .doit = devlink_nl_cmd_trap_policer_set_doit, >> > .flags = GENL_ADMIN_PERM, >> > }, >> >+ { >> >+ .cmd = DEVLINK_CMD_SELFTESTS_SHOW, >> >+ .validate = GENL_DONT_VALIDATE_STRICT | >> GENL_DONT_VALIDATE_DUMP, >> >+ .doit = devlink_nl_cmd_selftests_show, >> >> Why don't dump? >> > > I`ll add a dump in the next patchset. > >Thanks, >Vikas > > >> >> >> >+ .flags = GENL_ADMIN_PERM, >> >+ }, >> >+ { >> >+ .cmd = DEVLINK_CMD_SELFTESTS_RUN, >> >+ .validate = GENL_DONT_VALIDATE_STRICT | >> GENL_DONT_VALIDATE_DUMP, >> >+ .doit = devlink_nl_cmd_selftests_run, >> >+ .flags = GENL_ADMIN_PERM, >> >+ }, >> > }; >> > >> > static struct genl_family devlink_nl_family __ro_after_init = { >> >-- >> >2.31.1 >> > >> >> >>
Hi Jiri, On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: > > Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: > >Hi Jiri, > > > >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: > > > >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: > >> >Add a framework for running selftests. > >> >Framework exposes devlink commands and test suite(s) to the user > >> >to execute and query the supported tests by the driver. > >> > > >> >Below are new entries in devlink_nl_ops > >> >devlink_nl_cmd_selftests_show: To query the supported selftests > >> >by the driver. > >> >devlink_nl_cmd_selftests_run: To execute selftests. Users can > >> >provide a test mask for executing group tests or standalone tests. > >> > > >> >Documentation/networking/devlink/ path is already part of MAINTAINERS & > >> >the new files come under this path. Hence no update needed to the > >> >MAINTAINERS > >> > > >> >Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com> > >> >Reviewed-by: Michael Chan <michael.chan@broadcom.com> > >> >Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com> > >> >--- > >> > .../networking/devlink/devlink-selftests.rst | 34 +++++ > >> > include/net/devlink.h | 30 ++++ > >> > include/uapi/linux/devlink.h | 26 ++++ > >> > net/core/devlink.c | 144 ++++++++++++++++++ > >> > 4 files changed, 234 insertions(+) > >> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst > >> > > >> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst > >> b/Documentation/networking/devlink/devlink-selftests.rst > >> >new file mode 100644 > >> >index 000000000000..796d38f77038 > >> >--- /dev/null > >> >+++ b/Documentation/networking/devlink/devlink-selftests.rst > >> >@@ -0,0 +1,34 @@ > >> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >> >+ > >> >+================= > >> >+Devlink Selftests > >> >+================= > >> >+ > >> >+The ``devlink-selftests`` API allows executing selftests on the device. > >> >+ > >> >+Tests Mask > >> >+========== > >> >+The ``devlink-selftests`` command should be run with a mask indicating > >> >+the tests to be executed. > >> >+ > >> >+Tests Description > >> >+================= > >> >+The following is a list of tests that drivers may execute. > >> >+ > >> >+.. list-table:: List of tests > >> >+ :widths: 5 90 > >> >+ > >> >+ * - Name > >> >+ - Description > >> >+ * - ``DEVLINK_SELFTEST_FLASH`` > >> >+ - Runs a flash test on the device. > >> >+ > >> >+example usage > >> >+------------- > >> >+ > >> >+.. code:: shell > >> >+ > >> >+ # Query selftests supported on the device > >> >+ $ devlink dev selftests show DEV > >> >+ # Executes selftests on the device > >> >+ $ devlink dev selftests run DEV test {flash | all} > >> >diff --git a/include/net/devlink.h b/include/net/devlink.h > >> >index 2a2a2a0c93f7..cb7c378cf720 100644 > >> >--- a/include/net/devlink.h > >> >+++ b/include/net/devlink.h > >> >@@ -1215,6 +1215,18 @@ enum { > >> > DEVLINK_F_RELOAD = 1UL << 0, > >> > }; > >> > > >> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash" > >> >+ > >> >+static inline const char *devlink_selftest_name(int test) > >> > >> I don't understand why this is needed. Better not to expose string to > >> the user. Just have it as well defined attr. > > > > > > OK. Will remove this function and corresponding attr > >DEVLINK_ATTR_TEST_NAME added in this patch. > > > > > > > > > >> > >> >+{ > >> >+ switch (test) { > >> >+ case DEVLINK_SELFTEST_FLASH_BIT: > >> >+ return DEVLINK_SELFTEST_FLASH_TEST_NAME; > >> >+ default: > >> >+ return "unknown"; > >> >+ } > >> >+} > >> >+ > >> > struct devlink_ops { > >> > /** > >> > * @supported_flash_update_params: > >> >@@ -1509,6 +1521,24 @@ struct devlink_ops { > >> > struct devlink_rate *parent, > >> > void *priv_child, void *priv_parent, > >> > struct netlink_ext_ack *extack); > >> >+ /** > >> >+ * selftests_show() - Shows selftests supported by device > >> >+ * @devlink: Devlink instance > >> >+ * @extack: extack for reporting error messages > >> >+ * > >> >+ * Return: test mask supported by driver > >> >+ */ > >> >+ u32 (*selftests_show)(struct devlink *devlink, > >> >+ struct netlink_ext_ack *extack); > >> >+ /** > >> >+ * selftests_run() - Runs selftests > >> >+ * @devlink: Devlink instance > >> >+ * @tests_mask: tests to be run by driver > >> >+ * @results: test results by driver > >> >+ * @extack: extack for reporting error messages > >> >+ */ > >> >+ void (*selftests_run)(struct devlink *devlink, u32 tests_mask, > >> >+ u8 *results, struct netlink_ext_ack *extack); > >> > }; > >> > > >> > void *devlink_priv(struct devlink *devlink); > >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h > >> >index b3d40a5d72ff..1dba262328b9 100644 > >> >--- a/include/uapi/linux/devlink.h > >> >+++ b/include/uapi/linux/devlink.h > >> >@@ -136,6 +136,9 @@ enum devlink_command { > >> > DEVLINK_CMD_LINECARD_NEW, > >> > DEVLINK_CMD_LINECARD_DEL, > >> > > >> >+ DEVLINK_CMD_SELFTESTS_SHOW, > >> >+ DEVLINK_CMD_SELFTESTS_RUN, > >> >+ > >> > /* add new commands above here */ > >> > __DEVLINK_CMD_MAX, > >> > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 > >> >@@ -276,6 +279,25 @@ enum { > >> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \ > >> > (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1) > >> > > >> >+/* Commonly used test cases */ > >> >+enum { > >> >+ DEVLINK_SELFTEST_FLASH_BIT, > >> >+ > >> >+ __DEVLINK_SELFTEST_MAX_BIT, > >> >+ DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1 > >> >+}; > >> >+ > >> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT) > >> >+ > >> >+#define DEVLINK_SELFTESTS_MASK \ > >> >+ (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1) > >> >+ > >> >+enum { > >> >+ DEVLINK_SELFTEST_SKIP, > >> >+ DEVLINK_SELFTEST_PASS, > >> >+ DEVLINK_SELFTEST_FAIL > >> >+}; > >> >+ > >> > /** > >> > * enum devlink_trap_action - Packet trap action. > >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy > >> is not > >> >@@ -576,6 +598,10 @@ enum devlink_attr { > >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >> > > >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ > >> > >> I don't see why this is u32 bitset. Just have one attr per test > >> (NLA_FLAG) in a nested attr instead. > >> > > > >As per your suggestion, for an example it should be like as below > > > > DEVLINK_ATTR_SELFTESTS, /* nested */ > > > > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ > > > > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ > > Yeah, but have the flags in separate enum, no need to pullute the > devlink_attr enum by them. > > > > > >.... <SOME MORE TESTS> > > > >..... > > > > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ > > > > > > > > If we have this way then we need to have a mapping (probably a function) > >for drivers to tell them what tests need to be executed based on the flags > >that are set. > > Does this look OK? > > The rationale behind choosing a mask is that we could directly pass the > >mask-value to the drivers. > > If you have separate enum, you can use the attrs as bits internally in > kernel. Add a helper that would help the driver to work with it. > Pass a struct containing u32 (or u8) not to drivers. Once there are more > tests than that, this structure can be easily extended and the helpers > changed. This would make this scalable. No need for UAPI change or even > internel driver api change. As per your suggestion, selftest attributes can be declared in separate enum as below enum { DEVLINK_SELFTEST_SOMETEST, /* flag */ DEVLINK_SELFTEST_SOMETEST1, DEVLINK_SELFTEST_SOMETEST2, .... ...... __DEVLINK_SELFTEST_MAX, DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 }; Below examples could be the flow of parameters/data from user to kernel and vice-versa Kernel to user for show command . Users can know what all tests are supported by the driver. A return from kernel to user.
Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote: >Hi Jiri, > >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: >> >Hi Jiri, >> > >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: >> > >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: [...] >> >> > * enum devlink_trap_action - Packet trap action. >> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy >> >> is not >> >> >@@ -576,6 +598,10 @@ enum devlink_attr { >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> >> > >> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ >> >> >> >> I don't see why this is u32 bitset. Just have one attr per test >> >> (NLA_FLAG) in a nested attr instead. >> >> >> > >> >As per your suggestion, for an example it should be like as below >> > >> > DEVLINK_ATTR_SELFTESTS, /* nested */ >> > >> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ >> > >> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ >> >> Yeah, but have the flags in separate enum, no need to pullute the >> devlink_attr enum by them. >> >> >> > >> >.... <SOME MORE TESTS> >> > >> >..... >> > >> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ >> > >> > >> > >> > If we have this way then we need to have a mapping (probably a function) >> >for drivers to tell them what tests need to be executed based on the flags >> >that are set. >> > Does this look OK? >> > The rationale behind choosing a mask is that we could directly pass the >> >mask-value to the drivers. >> >> If you have separate enum, you can use the attrs as bits internally in >> kernel. Add a helper that would help the driver to work with it. >> Pass a struct containing u32 (or u8) not to drivers. Once there are more >> tests than that, this structure can be easily extended and the helpers >> changed. This would make this scalable. No need for UAPI change or even >> internel driver api change. > >As per your suggestion, selftest attributes can be declared in separate >enum as below > >enum { > > DEVLINK_SELFTEST_SOMETEST, /* flag */ > > DEVLINK_SELFTEST_SOMETEST1, > > DEVLINK_SELFTEST_SOMETEST2, > >.... > >...... > > __DEVLINK_SELFTEST_MAX, > > DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 > >}; >Below examples could be the flow of parameters/data from user to >kernel and vice-versa > > >Kernel to user for show command . Users can know what all tests are >supported by the driver. A return from kernel to user. >______ >|NEST | >|_____ |TEST1|TEST4|TEST7|... > > >User to kernel to execute test: If user wants to execute test4, test8, test1... >______ >|NEST | >|_____ |TEST4|TEST8|TEST1|... > > >Result Kernel to user execute test RES(u8) >______ >|NEST | >|_____ |RES4|RES8|RES1|... Hmm, I think it is not good idea to rely on the order, a netlink library can perhaps reorder it? Not sure here. > >Results are populated in the same order as the user passed the TESTs >flags. Does the above result format from kernel to user look OK ? >Else we need to have below way to form a result format, a nest should >be made for <test_flag, >result> but since test flags are in different enum other than >devlink_attr and RES being part of devlink_attr, I believe it's not >good practice to make the below structure. Not a structure, no. Have it as another nest (could be the same attr as the parent nest:
Hi Jiri, On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote: > >Hi Jiri, > > > >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: > >> > >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: > >> >Hi Jiri, > >> > > >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: > >> > > >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: > > [...] > > > >> >> > * enum devlink_trap_action - Packet trap action. > >> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy > >> >> is not > >> >> >@@ -576,6 +598,10 @@ enum devlink_attr { > >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >> >> > > >> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ > >> >> > >> >> I don't see why this is u32 bitset. Just have one attr per test > >> >> (NLA_FLAG) in a nested attr instead. > >> >> > >> > > >> >As per your suggestion, for an example it should be like as below > >> > > >> > DEVLINK_ATTR_SELFTESTS, /* nested */ > >> > > >> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ > >> > > >> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ > >> > >> Yeah, but have the flags in separate enum, no need to pullute the > >> devlink_attr enum by them. > >> > >> > >> > > >> >.... <SOME MORE TESTS> > >> > > >> >..... > >> > > >> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ > >> > > >> > > >> > > >> > If we have this way then we need to have a mapping (probably a function) > >> >for drivers to tell them what tests need to be executed based on the flags > >> >that are set. > >> > Does this look OK? > >> > The rationale behind choosing a mask is that we could directly pass the > >> >mask-value to the drivers. > >> > >> If you have separate enum, you can use the attrs as bits internally in > >> kernel. Add a helper that would help the driver to work with it. > >> Pass a struct containing u32 (or u8) not to drivers. Once there are more > >> tests than that, this structure can be easily extended and the helpers > >> changed. This would make this scalable. No need for UAPI change or even > >> internel driver api change. > > > >As per your suggestion, selftest attributes can be declared in separate > >enum as below > > > >enum { > > > > DEVLINK_SELFTEST_SOMETEST, /* flag */ > > > > DEVLINK_SELFTEST_SOMETEST1, > > > > DEVLINK_SELFTEST_SOMETEST2, > > > >.... > > > >...... > > > > __DEVLINK_SELFTEST_MAX, > > > > DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 > > > >}; > >Below examples could be the flow of parameters/data from user to > >kernel and vice-versa > > > > > >Kernel to user for show command . Users can know what all tests are > >supported by the driver. A return from kernel to user. > >______ > >|NEST | > >|_____ |TEST1|TEST4|TEST7|... > > > > > >User to kernel to execute test: If user wants to execute test4, test8, test1... > >______ > >|NEST | > >|_____ |TEST4|TEST8|TEST1|... > > > > > >Result Kernel to user execute test RES(u8) > >______ > >|NEST | > >|_____ |RES4|RES8|RES1|... > > Hmm, I think it is not good idea to rely on the order, a netlink library > can perhaps reorder it? Not sure here. > > > > >Results are populated in the same order as the user passed the TESTs > >flags. Does the above result format from kernel to user look OK ? > >Else we need to have below way to form a result format, a nest should > >be made for <test_flag, > >result> but since test flags are in different enum other than > >devlink_attr and RES being part of devlink_attr, I believe it's not > >good practice to make the below structure. > > Not a structure, no. Have it as another nest (could be the same attr as > the parent nest: > > ______ > |NEST | > |_____ |NEST| |NEST| |NEST| > TEST4,RES4 TEST8,RES8 TEST1, RES1 > > also, it is flexible to add another attr if needed (like maybe result > message string containing error message? IDK). For above nesting we can have the attributes defined as below Attribute in devlink_attr enum devlink_attr { .... .... DEVLINK_SELFTESTS_INFO, /* nested */ ... ... } enum devlink_selftests { DEVLINK_SELFTESTS_SOMETEST0, /* flag */ DEVLINK_SELFTESTS_SOMETEST1, DEVLINK_SELFTESTS_SOMETEST2, ... ... } enum devlink_selftest_result { DEVLINK_SELFTESTS_RESULT, /* nested */ DEVLINK_SELFTESTS_TESTNUM, /* u32 indicating the test number in devlink_selftests enum */ DEVLINK_SELFTESTS_RESULT_VAL, /* u8 skip, pass, fail.. */ ...some future attrr... } enums in devlink_selftest_result can be put in devlink_attr though. Does this look OK? Thanks, Vikas > > > > >______ > >|NEST | > >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|... > > > >Let me know if my understanding is correct. > > [...]
Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote: >Hi Jiri, > >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote: >> >Hi Jiri, >> > >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: >> >> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: >> >> >Hi Jiri, >> >> > >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: >> >> > >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: >> >> [...] >> >> >> >> >> > * enum devlink_trap_action - Packet trap action. >> >> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy >> >> >> is not >> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr { >> >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> >> >> > >> >> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ >> >> >> >> >> >> I don't see why this is u32 bitset. Just have one attr per test >> >> >> (NLA_FLAG) in a nested attr instead. >> >> >> >> >> > >> >> >As per your suggestion, for an example it should be like as below >> >> > >> >> > DEVLINK_ATTR_SELFTESTS, /* nested */ >> >> > >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ >> >> > >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ >> >> >> >> Yeah, but have the flags in separate enum, no need to pullute the >> >> devlink_attr enum by them. >> >> >> >> >> >> > >> >> >.... <SOME MORE TESTS> >> >> > >> >> >..... >> >> > >> >> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ >> >> > >> >> > >> >> > >> >> > If we have this way then we need to have a mapping (probably a function) >> >> >for drivers to tell them what tests need to be executed based on the flags >> >> >that are set. >> >> > Does this look OK? >> >> > The rationale behind choosing a mask is that we could directly pass the >> >> >mask-value to the drivers. >> >> >> >> If you have separate enum, you can use the attrs as bits internally in >> >> kernel. Add a helper that would help the driver to work with it. >> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more >> >> tests than that, this structure can be easily extended and the helpers >> >> changed. This would make this scalable. No need for UAPI change or even >> >> internel driver api change. >> > >> >As per your suggestion, selftest attributes can be declared in separate >> >enum as below >> > >> >enum { >> > >> > DEVLINK_SELFTEST_SOMETEST, /* flag */ >> > >> > DEVLINK_SELFTEST_SOMETEST1, >> > >> > DEVLINK_SELFTEST_SOMETEST2, >> > >> >.... >> > >> >...... >> > >> > __DEVLINK_SELFTEST_MAX, >> > >> > DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 >> > >> >}; >> >Below examples could be the flow of parameters/data from user to >> >kernel and vice-versa >> > >> > >> >Kernel to user for show command . Users can know what all tests are >> >supported by the driver. A return from kernel to user. >> >______ >> >|NEST | >> >|_____ |TEST1|TEST4|TEST7|... >> > >> > >> >User to kernel to execute test: If user wants to execute test4, test8, test1... >> >______ >> >|NEST | >> >|_____ |TEST4|TEST8|TEST1|... >> > >> > >> >Result Kernel to user execute test RES(u8) >> >______ >> >|NEST | >> >|_____ |RES4|RES8|RES1|... >> >> Hmm, I think it is not good idea to rely on the order, a netlink library >> can perhaps reorder it? Not sure here. >> >> > >> >Results are populated in the same order as the user passed the TESTs >> >flags. Does the above result format from kernel to user look OK ? >> >Else we need to have below way to form a result format, a nest should >> >be made for <test_flag, >> >result> but since test flags are in different enum other than >> >devlink_attr and RES being part of devlink_attr, I believe it's not >> >good practice to make the below structure. >> >> Not a structure, no. Have it as another nest (could be the same attr as >> the parent nest: >> >> ______ >> |NEST | >> |_____ |NEST| |NEST| |NEST| >> TEST4,RES4 TEST8,RES8 TEST1, RES1 >> >> also, it is flexible to add another attr if needed (like maybe result >> message string containing error message? IDK). > >For above nesting we can have the attributes defined as below > >Attribute in devlink_attr >enum devlink_attr { > .... > .... > DEVLINK_SELFTESTS_INFO, /* nested */ > ... >... >} > >enum devlink_selftests { > DEVLINK_SELFTESTS_SOMETEST0, /* flag */ > DEVLINK_SELFTESTS_SOMETEST1, > DEVLINK_SELFTESTS_SOMETEST2, > ... > ... >} > >enum devlink_selftest_result { for attrs, have "attr" in the name of the enum and "ATTR" in name of the value. > DEVLINK_SELFTESTS_RESULT, /* nested */ > DEVLINK_SELFTESTS_TESTNUM, /* u32 indicating the test You can have 1 enum, containing both these and the test flags from above. >number in devlink_selftests enum */ > DEVLINK_SELFTESTS_RESULT_VAL, /* u8 skip, pass, fail.. */ Put enum name in the comment, instead of list possible values. > ...some future attrr... > >} >enums in devlink_selftest_result can be put in devlink_attr though. You can have them separate, I think it is about the time we try to put new attrs what does not have potencial to be re-used to a separate enum. > >Does this look OK? > >Thanks, >Vikas > >> >> >> >> >______ >> >|NEST | >> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|... >> > >> >Let me know if my understanding is correct. >> >> [...]
Hi Jiri, On Wed, Jul 13, 2022 at 12:58 PM Jiri Pirko <jiri@nvidia.com> wrote: > > Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote: > >Hi Jiri, > > > >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote: > >> >Hi Jiri, > >> > > >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: > >> >> > >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: > >> >> >Hi Jiri, > >> >> > > >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: > >> >> > > >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: > >> > >> [...] > >> > >> > >> >> >> > * enum devlink_trap_action - Packet trap action. > >> >> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy > >> >> >> is not > >> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr { > >> >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ > >> >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ > >> >> >> > > >> >> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ > >> >> >> > >> >> >> I don't see why this is u32 bitset. Just have one attr per test > >> >> >> (NLA_FLAG) in a nested attr instead. > >> >> >> > >> >> > > >> >> >As per your suggestion, for an example it should be like as below > >> >> > > >> >> > DEVLINK_ATTR_SELFTESTS, /* nested */ > >> >> > > >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ > >> >> > > >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ > >> >> > >> >> Yeah, but have the flags in separate enum, no need to pullute the > >> >> devlink_attr enum by them. > >> >> > >> >> > >> >> > > >> >> >.... <SOME MORE TESTS> > >> >> > > >> >> >..... > >> >> > > >> >> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ > >> >> > > >> >> > > >> >> > > >> >> > If we have this way then we need to have a mapping (probably a function) > >> >> >for drivers to tell them what tests need to be executed based on the flags > >> >> >that are set. > >> >> > Does this look OK? > >> >> > The rationale behind choosing a mask is that we could directly pass the > >> >> >mask-value to the drivers. > >> >> > >> >> If you have separate enum, you can use the attrs as bits internally in > >> >> kernel. Add a helper that would help the driver to work with it. > >> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more > >> >> tests than that, this structure can be easily extended and the helpers > >> >> changed. This would make this scalable. No need for UAPI change or even > >> >> internel driver api change. > >> > > >> >As per your suggestion, selftest attributes can be declared in separate > >> >enum as below > >> > > >> >enum { > >> > > >> > DEVLINK_SELFTEST_SOMETEST, /* flag */ > >> > > >> > DEVLINK_SELFTEST_SOMETEST1, > >> > > >> > DEVLINK_SELFTEST_SOMETEST2, > >> > > >> >.... > >> > > >> >...... > >> > > >> > __DEVLINK_SELFTEST_MAX, > >> > > >> > DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 > >> > > >> >}; > >> >Below examples could be the flow of parameters/data from user to > >> >kernel and vice-versa > >> > > >> > > >> >Kernel to user for show command . Users can know what all tests are > >> >supported by the driver. A return from kernel to user. > >> >______ > >> >|NEST | > >> >|_____ |TEST1|TEST4|TEST7|... > >> > > >> > > >> >User to kernel to execute test: If user wants to execute test4, test8, test1... > >> >______ > >> >|NEST | > >> >|_____ |TEST4|TEST8|TEST1|... > >> > > >> > > >> >Result Kernel to user execute test RES(u8) > >> >______ > >> >|NEST | > >> >|_____ |RES4|RES8|RES1|... > >> > >> Hmm, I think it is not good idea to rely on the order, a netlink library > >> can perhaps reorder it? Not sure here. > >> > >> > > >> >Results are populated in the same order as the user passed the TESTs > >> >flags. Does the above result format from kernel to user look OK ? > >> >Else we need to have below way to form a result format, a nest should > >> >be made for <test_flag, > >> >result> but since test flags are in different enum other than > >> >devlink_attr and RES being part of devlink_attr, I believe it's not > >> >good practice to make the below structure. > >> > >> Not a structure, no. Have it as another nest (could be the same attr as > >> the parent nest: > >> > >> ______ > >> |NEST | > >> |_____ |NEST| |NEST| |NEST| > >> TEST4,RES4 TEST8,RES8 TEST1, RES1 > >> > >> also, it is flexible to add another attr if needed (like maybe result > >> message string containing error message? IDK). > > > >For above nesting we can have the attributes defined as below > > > >Attribute in devlink_attr > >enum devlink_attr { > > .... > > .... > > DEVLINK_SELFTESTS_INFO, /* nested */ > > ... > >... > >} > > > >enum devlink_selftests { > > DEVLINK_SELFTESTS_SOMETEST0, /* flag */ > > DEVLINK_SELFTESTS_SOMETEST1, > > DEVLINK_SELFTESTS_SOMETEST2, > > ... > > ... > >} > > > >enum devlink_selftest_result { > > for attrs, have "attr" in the name of the enum and "ATTR" in name of the > value. > > > DEVLINK_SELFTESTS_RESULT, /* nested */ > > DEVLINK_SELFTESTS_TESTNUM, /* u32 indicating the test > > You can have 1 enum, containing both these and the test flags from > above. I think it's better to keep enum devlink_selftests_attr (containing flags) and devlink_selftest_result_attr separately as it will have an advantage. For example, for show commands the kernel can iterate through and check with the driver if it supports a particular test. for (i = 0; i < DEVLINK_SELFTEST_ATTR_MAX, i++) { if (devlink->ops->selftest_info(devlink, i, extack)) { // supports selftest or not nla_put_flag(msg, i); } } Also flags in devlink_selftests_attr can be used as bitwise, if required. Let me know what you think. Thanks, Vikas > > > >number in devlink_selftests enum */ > > DEVLINK_SELFTESTS_RESULT_VAL, /* u8 skip, pass, fail.. */ > > Put enum name in the comment, instead of list possible values. > > > > ...some future attrr... > > > >} > >enums in devlink_selftest_result can be put in devlink_attr though. > > You can have them separate, I think it is about the time we try to put > new attrs what does not have potencial to be re-used to a separate enum. > > > > > >Does this look OK? > > > >Thanks, > >Vikas > > > >> > >> > >> > >> >______ > >> >|NEST | > >> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|... > >> > > >> >Let me know if my understanding is correct. > >> > >> [...] > >
Wed, Jul 13, 2022 at 12:16:03PM CEST, vikas.gupta@broadcom.com wrote: >Hi Jiri, > >On Wed, Jul 13, 2022 at 12:58 PM Jiri Pirko <jiri@nvidia.com> wrote: >> >> Wed, Jul 13, 2022 at 08:40:50AM CEST, vikas.gupta@broadcom.com wrote: >> >Hi Jiri, >> > >> >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, vikas.gupta@broadcom.com wrote: >> >> >Hi Jiri, >> >> > >> >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <jiri@nvidia.com> wrote: >> >> >> >> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, vikas.gupta@broadcom.com wrote: >> >> >> >Hi Jiri, >> >> >> > >> >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <jiri@nvidia.com> wrote: >> >> >> > >> >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, vikas.gupta@broadcom.com wrote: >> >> >> >> [...] >> >> >> >> >> >> >> >> > * enum devlink_trap_action - Packet trap action. >> >> >> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy >> >> >> >> is not >> >> >> >> >@@ -576,6 +598,10 @@ enum devlink_attr { >> >> >> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */ >> >> >> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ >> >> >> >> > >> >> >> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ >> >> >> >> >> >> >> >> I don't see why this is u32 bitset. Just have one attr per test >> >> >> >> (NLA_FLAG) in a nested attr instead. >> >> >> >> >> >> >> > >> >> >> >As per your suggestion, for an example it should be like as below >> >> >> > >> >> >> > DEVLINK_ATTR_SELFTESTS, /* nested */ >> >> >> > >> >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */ >> >> >> > >> >> >> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */ >> >> >> >> >> >> Yeah, but have the flags in separate enum, no need to pullute the >> >> >> devlink_attr enum by them. >> >> >> >> >> >> >> >> >> > >> >> >> >.... <SOME MORE TESTS> >> >> >> > >> >> >> >..... >> >> >> > >> >> >> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */ >> >> >> > >> >> >> > >> >> >> > >> >> >> > If we have this way then we need to have a mapping (probably a function) >> >> >> >for drivers to tell them what tests need to be executed based on the flags >> >> >> >that are set. >> >> >> > Does this look OK? >> >> >> > The rationale behind choosing a mask is that we could directly pass the >> >> >> >mask-value to the drivers. >> >> >> >> >> >> If you have separate enum, you can use the attrs as bits internally in >> >> >> kernel. Add a helper that would help the driver to work with it. >> >> >> Pass a struct containing u32 (or u8) not to drivers. Once there are more >> >> >> tests than that, this structure can be easily extended and the helpers >> >> >> changed. This would make this scalable. No need for UAPI change or even >> >> >> internel driver api change. >> >> > >> >> >As per your suggestion, selftest attributes can be declared in separate >> >> >enum as below >> >> > >> >> >enum { >> >> > >> >> > DEVLINK_SELFTEST_SOMETEST, /* flag */ >> >> > >> >> > DEVLINK_SELFTEST_SOMETEST1, >> >> > >> >> > DEVLINK_SELFTEST_SOMETEST2, >> >> > >> >> >.... >> >> > >> >> >...... >> >> > >> >> > __DEVLINK_SELFTEST_MAX, >> >> > >> >> > DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1 >> >> > >> >> >}; >> >> >Below examples could be the flow of parameters/data from user to >> >> >kernel and vice-versa >> >> > >> >> > >> >> >Kernel to user for show command . Users can know what all tests are >> >> >supported by the driver. A return from kernel to user. >> >> >______ >> >> >|NEST | >> >> >|_____ |TEST1|TEST4|TEST7|... >> >> > >> >> > >> >> >User to kernel to execute test: If user wants to execute test4, test8, test1... >> >> >______ >> >> >|NEST | >> >> >|_____ |TEST4|TEST8|TEST1|... >> >> > >> >> > >> >> >Result Kernel to user execute test RES(u8) >> >> >______ >> >> >|NEST | >> >> >|_____ |RES4|RES8|RES1|... >> >> >> >> Hmm, I think it is not good idea to rely on the order, a netlink library >> >> can perhaps reorder it? Not sure here. >> >> >> >> > >> >> >Results are populated in the same order as the user passed the TESTs >> >> >flags. Does the above result format from kernel to user look OK ? >> >> >Else we need to have below way to form a result format, a nest should >> >> >be made for <test_flag, >> >> >result> but since test flags are in different enum other than >> >> >devlink_attr and RES being part of devlink_attr, I believe it's not >> >> >good practice to make the below structure. >> >> >> >> Not a structure, no. Have it as another nest (could be the same attr as >> >> the parent nest: >> >> >> >> ______ >> >> |NEST | >> >> |_____ |NEST| |NEST| |NEST| >> >> TEST4,RES4 TEST8,RES8 TEST1, RES1 >> >> >> >> also, it is flexible to add another attr if needed (like maybe result >> >> message string containing error message? IDK). >> > >> >For above nesting we can have the attributes defined as below >> > >> >Attribute in devlink_attr >> >enum devlink_attr { >> > .... >> > .... >> > DEVLINK_SELFTESTS_INFO, /* nested */ >> > ... >> >... >> >} >> > >> >enum devlink_selftests { >> > DEVLINK_SELFTESTS_SOMETEST0, /* flag */ >> > DEVLINK_SELFTESTS_SOMETEST1, >> > DEVLINK_SELFTESTS_SOMETEST2, >> > ... >> > ... >> >} >> > >> >enum devlink_selftest_result { >> >> for attrs, have "attr" in the name of the enum and "ATTR" in name of the >> value. >> >> > DEVLINK_SELFTESTS_RESULT, /* nested */ >> > DEVLINK_SELFTESTS_TESTNUM, /* u32 indicating the test >> >> You can have 1 enum, containing both these and the test flags from >> above. > I think it's better to keep enum devlink_selftests_attr (containing >flags) and devlink_selftest_result_attr separately as it will have an >advantage. > For example, for show commands the kernel can iterate through and >check with the driver if it supports a particular test. > > for (i = 0; i < DEVLINK_SELFTEST_ATTR_MAX, i++) { > if (devlink->ops->selftest_info(devlink, i, >extack)) { // supports selftest or not > nla_put_flag(msg, i); > } > } > Also flags in devlink_selftests_attr can be used as bitwise, if required. > Let me know what you think. Okay. > >Thanks, >Vikas > >> >> >> >number in devlink_selftests enum */ >> > DEVLINK_SELFTESTS_RESULT_VAL, /* u8 skip, pass, fail.. */ >> >> Put enum name in the comment, instead of list possible values. >> >> >> > ...some future attrr... >> > >> >} >> >enums in devlink_selftest_result can be put in devlink_attr though. >> >> You can have them separate, I think it is about the time we try to put >> new attrs what does not have potencial to be re-used to a separate enum. >> >> >> > >> >Does this look OK? >> > >> >Thanks, >> >Vikas >> > >> >> >> >> >> >> >> >> >______ >> >> >|NEST | >> >> >|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|... >> >> > >> >> >Let me know if my understanding is correct. >> >> >> >> [...] >> >>
diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst new file mode 100644 index 000000000000..796d38f77038 --- /dev/null +++ b/Documentation/networking/devlink/devlink-selftests.rst @@ -0,0 +1,34 @@ +.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) + +================= +Devlink Selftests +================= + +The ``devlink-selftests`` API allows executing selftests on the device. + +Tests Mask +========== +The ``devlink-selftests`` command should be run with a mask indicating +the tests to be executed. + +Tests Description +================= +The following is a list of tests that drivers may execute. + +.. list-table:: List of tests + :widths: 5 90 + + * - Name + - Description + * - ``DEVLINK_SELFTEST_FLASH`` + - Runs a flash test on the device. + +example usage +------------- + +.. code:: shell + + # Query selftests supported on the device + $ devlink dev selftests show DEV + # Executes selftests on the device + $ devlink dev selftests run DEV test {flash | all} diff --git a/include/net/devlink.h b/include/net/devlink.h index 2a2a2a0c93f7..cb7c378cf720 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1215,6 +1215,18 @@ enum { DEVLINK_F_RELOAD = 1UL << 0, }; +#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash" + +static inline const char *devlink_selftest_name(int test) +{ + switch (test) { + case DEVLINK_SELFTEST_FLASH_BIT: + return DEVLINK_SELFTEST_FLASH_TEST_NAME; + default: + return "unknown"; + } +} + struct devlink_ops { /** * @supported_flash_update_params: @@ -1509,6 +1521,24 @@ struct devlink_ops { struct devlink_rate *parent, void *priv_child, void *priv_parent, struct netlink_ext_ack *extack); + /** + * selftests_show() - Shows selftests supported by device + * @devlink: Devlink instance + * @extack: extack for reporting error messages + * + * Return: test mask supported by driver + */ + u32 (*selftests_show)(struct devlink *devlink, + struct netlink_ext_ack *extack); + /** + * selftests_run() - Runs selftests + * @devlink: Devlink instance + * @tests_mask: tests to be run by driver + * @results: test results by driver + * @extack: extack for reporting error messages + */ + void (*selftests_run)(struct devlink *devlink, u32 tests_mask, + u8 *results, struct netlink_ext_ack *extack); }; void *devlink_priv(struct devlink *devlink); diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index b3d40a5d72ff..1dba262328b9 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -136,6 +136,9 @@ enum devlink_command { DEVLINK_CMD_LINECARD_NEW, DEVLINK_CMD_LINECARD_DEL, + DEVLINK_CMD_SELFTESTS_SHOW, + DEVLINK_CMD_SELFTESTS_RUN, + /* add new commands above here */ __DEVLINK_CMD_MAX, DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1 @@ -276,6 +279,25 @@ enum { #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \ (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1) +/* Commonly used test cases */ +enum { + DEVLINK_SELFTEST_FLASH_BIT, + + __DEVLINK_SELFTEST_MAX_BIT, + DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1 +}; + +#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT) + +#define DEVLINK_SELFTESTS_MASK \ + (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1) + +enum { + DEVLINK_SELFTEST_SKIP, + DEVLINK_SELFTEST_PASS, + DEVLINK_SELFTEST_FAIL +}; + /** * enum devlink_trap_action - Packet trap action. * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not @@ -576,6 +598,10 @@ enum devlink_attr { DEVLINK_ATTR_LINECARD_TYPE, /* string */ DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */ + DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */ + DEVLINK_ATTR_TEST_RESULT, /* nested */ + DEVLINK_ATTR_TEST_NAME, /* string */ + DEVLINK_ATTR_TEST_RESULT_VAL, /* u8 */ /* add new attributes above here, update the policy in devlink.c */ __DEVLINK_ATTR_MAX, diff --git a/net/core/devlink.c b/net/core/devlink.c index db61f3a341cb..0b7341ab6379 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, return ret; } +static int devlink_selftest_name_put(struct sk_buff *skb, int test) +{ + const char *name = devlink_selftest_name(test); + if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name)) + return -EMSGSIZE; + + return 0; +} + +static int devlink_nl_cmd_selftests_show(struct sk_buff *skb, + struct genl_info *info) +{ + struct devlink *devlink = info->user_ptr[0]; + struct sk_buff *msg; + unsigned long tests; + int err = 0; + void *hdr; + int test; + + if (!devlink->ops->selftests_show) + return -EOPNOTSUPP; + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) + return -ENOMEM; + + hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, + &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_SHOW); + if (!hdr) + goto free_msg; + + if (devlink_nl_put_handle(msg, devlink)) + goto genlmsg_cancel; + + tests = devlink->ops->selftests_show(devlink, info->extack); + + for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { + err = devlink_selftest_name_put(msg, test); + if (err) + goto genlmsg_cancel; + } + + genlmsg_end(msg, hdr); + + return genlmsg_reply(msg, info); + +genlmsg_cancel: + genlmsg_cancel(msg, hdr); +free_msg: + nlmsg_free(msg); + return err; +} + +static int devlink_selftest_result_put(struct sk_buff *skb, int test, + u8 result) +{ + const char *name = devlink_selftest_name(test); + struct nlattr *result_attr; + + result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT); + if (!result_attr) + return -EMSGSIZE; + + if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) || + nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result)) + goto nla_put_failure; + + nla_nest_end(skb, result_attr); + + return 0; + +nla_put_failure: + nla_nest_cancel(skb, result_attr); + return -EMSGSIZE; +} + +static int devlink_nl_cmd_selftests_run(struct sk_buff *skb, + struct genl_info *info) +{ + u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {}; + struct devlink *devlink = info->user_ptr[0]; + unsigned long tests; + struct sk_buff *msg; + u32 tests_mask; + void *hdr; + int err = 0; + int test; + + if (!devlink->ops->selftests_run) + return -EOPNOTSUPP; + + if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]) + return -EINVAL; + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) + return -ENOMEM; + + hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, + &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN); + if (!hdr) + goto free_msg; + + if (devlink_nl_put_handle(msg, devlink)) + goto genlmsg_cancel; + + tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]); + + devlink->ops->selftests_run(devlink, tests_mask, test_results, + info->extack); + tests = tests_mask; + + for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) { + err = devlink_selftest_result_put(msg, test, + test_results[test]); + if (err) + goto genlmsg_cancel; + } + + genlmsg_end(msg, hdr); + + return genlmsg_reply(msg, info); + +genlmsg_cancel: + genlmsg_cancel(msg, hdr); +free_msg: + nlmsg_free(msg); + return err; +} + static const struct devlink_param devlink_param_generic[] = { { .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET, @@ -9000,6 +9130,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING }, [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 }, [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING }, + [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32, + DEVLINK_SELFTESTS_MASK), }; static const struct genl_small_ops devlink_nl_ops[] = { @@ -9361,6 +9493,18 @@ static const struct genl_small_ops devlink_nl_ops[] = { .doit = devlink_nl_cmd_trap_policer_set_doit, .flags = GENL_ADMIN_PERM, }, + { + .cmd = DEVLINK_CMD_SELFTESTS_SHOW, + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, + .doit = devlink_nl_cmd_selftests_show, + .flags = GENL_ADMIN_PERM, + }, + { + .cmd = DEVLINK_CMD_SELFTESTS_RUN, + .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, + .doit = devlink_nl_cmd_selftests_run, + .flags = GENL_ADMIN_PERM, + }, }; static struct genl_family devlink_nl_family __ro_after_init = {