On Tue, 28 Jun 2022 22:12:39 +0530 Vikas Gupta 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.
>
> To execute tests, users can provide a test mask for executing
> group tests or standalone tests. API devlink_selftest_result_put() helps
> drivers to populate the test results along with their names.
>
> To query supported tests by device, API devlink_selftest_name_put()
> helps a driver to populate test names.
>
> Signed-off-by: Vikas Gupta <[email protected]>
> Reviewed-by: Michael Chan <[email protected]>
> Reviewed-by: Andy Gospodarek <[email protected]>
> ---
> .../networking/devlink/devlink-selftests.rst | 39 +++++
> include/net/devlink.h | 40 +++++
> include/uapi/linux/devlink.h | 24 +++
> net/core/devlink.c | 147 ++++++++++++++++++
> 4 files changed, 250 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..df7c8fcac9bf
> --- /dev/null
> +++ b/Documentation/networking/devlink/devlink-selftests.rst
> @@ -0,0 +1,39 @@
> +.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +
> +.. _devlink_selftests:
Why the label?
> +=================
> +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 which helps to log the health of the flash.
> + see Documentation/networking/devlink/devlink-health.rst
Quick grep of "flash" in devlink-health.rst shows nothing.
New sentence should be capitalized.
> +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}
> +
> +
Spurious nl.
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 2a2a2a0c93f7..493dab368340 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1215,6 +1215,19 @@ enum {
> DEVLINK_F_RELOAD = 1UL << 0,
> };
>
> +/**
> + * struct devlink_selftest_exec_info - Devlink selftest execution info.
> + * @name: Test name.
> + * @result: Test result.
Don't create kdoc if you have nothing to say in it.
> + */
> +struct devlink_selftest_exec_info {
> + const char *name;
> + bool result;
"result" doesn't indicate the polarity, better call it passed or failed.
int may be better to indicate pass/fail/skip?
> +};
> +
> +#define DEVLINK_SELFTEST_FLASH_TEST_NAME \
> + "flash test"
no need to break this line
> struct devlink_ops {
> /**
> * @supported_flash_update_params:
> @@ -1509,6 +1522,28 @@ 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
> + * @skb: message payload
> + * @extack: extack for reporting error messages
> + *
> + * Return: 0 on success, negative value otherwise.
> + */
> +
spurious nl
> + int (*selftests_show)(struct devlink *devlink, struct sk_buff *skb,
> + struct netlink_ext_ack *extack);
Why do we need to pass skb into this? The bits seem predefined so why
not return the mask and let the core do the formatting?
> + /**
> + * selftests_run() - Runs selftests
> + * @devlink: Devlink instance
> + * @skb: message payload
> + * @tests_mask: tests to be run
> + * @extack: extack for reporting error messages
> + *
> + * Return: 0 on success, negative value otherwise.
> + */
> + int (*selftests_run)(struct devlink *devlink, struct sk_buff *skb,
> + u32 tests_mask, struct netlink_ext_ack *extack);
Similarly it may be more convenient for the drivers to pass in an array
in which to record results rather than having to deal with outputting
to an ask.
> };
>
> void *devlink_priv(struct devlink *devlink);
> @@ -1774,6 +1809,11 @@ void
> devlink_trap_policers_unregister(struct devlink *devlink,
> const struct devlink_trap_policer *policers,
> size_t policers_count);
> +int
> +devlink_selftest_result_put(struct sk_buff *skb,
> + struct devlink_selftest_exec_info *test);
> +int
> +devlink_selftest_name_put(struct sk_buff *skb, const char *name);
>
> #if IS_ENABLED(CONFIG_NET_DEVLINK)
>
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index b3d40a5d72ff..58e2ef4010f0 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,21 @@ enum {
> #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
>
> +/* Commonly used test cases. Drivers might interpret test bit
> + * in their own way and it may map single to multiple tests.
> + */
> +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_SUPPORTED_SELFTESTS \
> + (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 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 +594,12 @@ enum devlink_attr {
> DEVLINK_ATTR_LINECARD_TYPE, /* string */
> DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
>
> + DEVLINK_ATTR_SELFTESTS_MASK, /* bitfield32 */
> + DEVLINK_ATTR_TEST_NAMES, /* nested */
> + DEVLINK_ATTR_TEST_NAME, /* string */
> + DEVLINK_ATTR_TEST_RESULTS, /* nested */
> + DEVLINK_ATTR_TEST_RESULT, /* nested */
So many nests, netlink is much lighter on nesting that say JSON.
All you need is one nest type to wrap name and result, and just
output that multiple times to the message.
> + 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..3c4c27a7dd40 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4794,6 +4794,139 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> return ret;
> }
>
> +int devlink_selftest_name_put(struct sk_buff *skb, const char *name)
> +{
> + if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
> + return -EMSGSIZE;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devlink_selftest_name_put);
> +
> +static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct devlink *devlink = info->user_ptr[0];
> + struct nlattr *names_attr;
> + struct sk_buff *msg;
> + void *hdr;
> + int err;
> +
> + 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)
Leak.
> + return -EMSGSIZE;
> +
> + if (devlink_nl_put_handle(msg, devlink))
> + goto genlmsg_cancel;
err not initialized
> + names_attr = nla_nest_start_noflag(msg, DEVLINK_ATTR_TEST_NAMES);
> + if (!names_attr)
> + goto genlmsg_cancel;
> +
> + err = devlink->ops->selftests_show(devlink, msg, info->extack);
double space
> + if (err)
> + goto names_nest_cancel;
> +
> + nla_nest_end(msg, names_attr);
> + genlmsg_end(msg, hdr);
> +
> + return genlmsg_reply(msg, info);
> +
> +names_nest_cancel:
> + nla_nest_cancel(msg, names_attr);
> +genlmsg_cancel:
> + genlmsg_cancel(msg, hdr);
> + nlmsg_free(msg);
> + return err;
> +}
> static const struct devlink_param devlink_param_generic[] = {
> {
> .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
> @@ -9000,6 +9133,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_BITFIELD32(DEVLINK_SUPPORTED_SELFTESTS),
You don't need a bitfield, use a u32 (u64?) with a mask policy.
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 <[email protected]>
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
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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 <[email protected]>
All warnings (new ones prefixed by >>):
>> Documentation/networking/devlink/devlink-selftests.rst: WARNING: document isn't included in any toctree
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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, [email protected] 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 <[email protected]>
>Reviewed-by: Michael Chan <[email protected]>
>Reviewed-by: Andy Gospodarek <[email protected]>
>---
> .../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, [email protected] wrote:
>Hi Jiri,
>
>On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
>
>> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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 <[email protected]>
>> >Reviewed-by: Michael Chan <[email protected]>
>> >Reviewed-by: Andy Gospodarek <[email protected]>
>> >---
>> > .../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 <[email protected]> wrote:
>
> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
> >Hi Jiri,
> >
> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
> >
> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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 <[email protected]>
> >> >Reviewed-by: Michael Chan <[email protected]>
> >> >Reviewed-by: Andy Gospodarek <[email protected]>
> >> >---
> >> > .../networking/devlink/devlink-selftests.rst | 34 +++++
> >> > include/net/devlink.h | 30 ++++
> >> > include/uapi/linux/devlink.h | 26 ++++
> >> > net/core/devlink.c | 144 ++++++++++++++++++
> >> > 4 files changed, 234 insertions(+)
> >> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
> >> >
> >> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst
> >> b/Documentation/networking/devlink/devlink-selftests.rst
> >> >new file mode 100644
> >> >index 000000000000..796d38f77038
> >> >--- /dev/null
> >> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
> >> >@@ -0,0 +1,34 @@
> >> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> >+
> >> >+=================
> >> >+Devlink Selftests
> >> >+=================
> >> >+
> >> >+The ``devlink-selftests`` API allows executing selftests on the device.
> >> >+
> >> >+Tests Mask
> >> >+==========
> >> >+The ``devlink-selftests`` command should be run with a mask indicating
> >> >+the tests to be executed.
> >> >+
> >> >+Tests Description
> >> >+=================
> >> >+The following is a list of tests that drivers may execute.
> >> >+
> >> >+.. list-table:: List of tests
> >> >+ :widths: 5 90
> >> >+
> >> >+ * - Name
> >> >+ - Description
> >> >+ * - ``DEVLINK_SELFTEST_FLASH``
> >> >+ - Runs a flash test on the device.
> >> >+
> >> >+example usage
> >> >+-------------
> >> >+
> >> >+.. code:: shell
> >> >+
> >> >+ # Query selftests supported on the device
> >> >+ $ devlink dev selftests show DEV
> >> >+ # Executes selftests on the device
> >> >+ $ devlink dev selftests run DEV test {flash | all}
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 2a2a2a0c93f7..cb7c378cf720 100644
> >> >--- a/include/net/devlink.h
> >> >+++ b/include/net/devlink.h
> >> >@@ -1215,6 +1215,18 @@ enum {
> >> > DEVLINK_F_RELOAD = 1UL << 0,
> >> > };
> >> >
> >> >+#define DEVLINK_SELFTEST_FLASH_TEST_NAME "flash"
> >> >+
> >> >+static inline const char *devlink_selftest_name(int test)
> >>
> >> I don't understand why this is needed. Better not to expose string to
> >> the user. Just have it as well defined attr.
> >
> >
> > OK. Will remove this function and corresponding attr
> >DEVLINK_ATTR_TEST_NAME added in this patch.
> >
> >
> >
> >
> >>
> >> >+{
> >> >+ switch (test) {
> >> >+ case DEVLINK_SELFTEST_FLASH_BIT:
> >> >+ return DEVLINK_SELFTEST_FLASH_TEST_NAME;
> >> >+ default:
> >> >+ return "unknown";
> >> >+ }
> >> >+}
> >> >+
> >> > struct devlink_ops {
> >> > /**
> >> > * @supported_flash_update_params:
> >> >@@ -1509,6 +1521,24 @@ struct devlink_ops {
> >> > struct devlink_rate *parent,
> >> > void *priv_child, void *priv_parent,
> >> > struct netlink_ext_ack *extack);
> >> >+ /**
> >> >+ * selftests_show() - Shows selftests supported by device
> >> >+ * @devlink: Devlink instance
> >> >+ * @extack: extack for reporting error messages
> >> >+ *
> >> >+ * Return: test mask supported by driver
> >> >+ */
> >> >+ u32 (*selftests_show)(struct devlink *devlink,
> >> >+ struct netlink_ext_ack *extack);
> >> >+ /**
> >> >+ * selftests_run() - Runs selftests
> >> >+ * @devlink: Devlink instance
> >> >+ * @tests_mask: tests to be run by driver
> >> >+ * @results: test results by driver
> >> >+ * @extack: extack for reporting error messages
> >> >+ */
> >> >+ void (*selftests_run)(struct devlink *devlink, u32 tests_mask,
> >> >+ u8 *results, struct netlink_ext_ack *extack);
> >> > };
> >> >
> >> > void *devlink_priv(struct devlink *devlink);
> >> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >> >index b3d40a5d72ff..1dba262328b9 100644
> >> >--- a/include/uapi/linux/devlink.h
> >> >+++ b/include/uapi/linux/devlink.h
> >> >@@ -136,6 +136,9 @@ enum devlink_command {
> >> > DEVLINK_CMD_LINECARD_NEW,
> >> > DEVLINK_CMD_LINECARD_DEL,
> >> >
> >> >+ DEVLINK_CMD_SELFTESTS_SHOW,
> >> >+ DEVLINK_CMD_SELFTESTS_RUN,
> >> >+
> >> > /* add new commands above here */
> >> > __DEVLINK_CMD_MAX,
> >> > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >> >@@ -276,6 +279,25 @@ enum {
> >> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> >> > (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> >> >
> >> >+/* Commonly used test cases */
> >> >+enum {
> >> >+ DEVLINK_SELFTEST_FLASH_BIT,
> >> >+
> >> >+ __DEVLINK_SELFTEST_MAX_BIT,
> >> >+ DEVLINK_SELFTEST_MAX_BIT = __DEVLINK_SELFTEST_MAX_BIT - 1
> >> >+};
> >> >+
> >> >+#define DEVLINK_SELFTEST_FLASH _BITUL(DEVLINK_SELFTEST_FLASH_BIT)
> >> >+
> >> >+#define DEVLINK_SELFTESTS_MASK \
> >> >+ (_BITUL(__DEVLINK_SELFTEST_MAX_BIT) - 1)
> >> >+
> >> >+enum {
> >> >+ DEVLINK_SELFTEST_SKIP,
> >> >+ DEVLINK_SELFTEST_PASS,
> >> >+ DEVLINK_SELFTEST_FAIL
> >> >+};
> >> >+
> >> > /**
> >> > * enum devlink_trap_action - Packet trap action.
> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
> >> is not
> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */
> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
> >> >
> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */
> >>
> >> I don't see why this is u32 bitset. Just have one attr per test
> >> (NLA_FLAG) in a nested attr instead.
> >>
> >
> >As per your suggestion, for an example it should be like as below
> >
> > DEVLINK_ATTR_SELFTESTS, /* nested */
> >
> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */
> >
> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */
>
> Yeah, but have the flags in separate enum, no need to pullute the
> devlink_attr enum by them.
>
>
> >
> >.... <SOME MORE TESTS>
> >
> >.....
> >
> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */
> >
> >
> >
> > If we have this way then we need to have a mapping (probably a function)
> >for drivers to tell them what tests need to be executed based on the flags
> >that are set.
> > Does this look OK?
> > The rationale behind choosing a mask is that we could directly pass the
> >mask-value to the drivers.
>
> If you have separate enum, you can use the attrs as bits internally in
> kernel. Add a helper that would help the driver to work with it.
> Pass a struct containing u32 (or u8) not to drivers. Once there are more
> tests than that, this structure can be easily extended and the helpers
> changed. This would make this scalable. No need for UAPI change or even
> internel driver api change.
As per your suggestion, selftest attributes can be declared in separate
enum as below
enum {
DEVLINK_SELFTEST_SOMETEST, /* flag */
DEVLINK_SELFTEST_SOMETEST1,
DEVLINK_SELFTEST_SOMETEST2,
....
......
__DEVLINK_SELFTEST_MAX,
DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
};
Below examples could be the flow of parameters/data from user to
kernel and vice-versa
Kernel to user for show command . Users can know what all tests are
supported by the driver. A return from kernel to user.
______
|NEST |
|_____ |TEST1|TEST4|TEST7|...
User to kernel to execute test: If user wants to execute test4, test8, test1...
______
|NEST |
|_____ |TEST4|TEST8|TEST1|...
Result Kernel to user execute test RES(u8)
______
|NEST |
|_____ |RES4|RES8|RES1|...
Results are populated in the same order as the user passed the TESTs
flags. Does the above result format from kernel to user look OK ?
Else we need to have below way to form a result format, a nest should
be made for <test_flag,
result> but since test flags are in different enum other than
devlink_attr and RES being part of devlink_attr, I believe it's not
good practice to make the below structure.
______
|NEST |
|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
Let me know if my understanding is correct.
Thanks,
Vikas
>
>
> >
> >
> >>
> >>
> >>
> >> >+ DEVLINK_ATTR_TEST_RESULT, /* nested */
> >> >+ DEVLINK_ATTR_TEST_NAME, /* string */
> >> >+ DEVLINK_ATTR_TEST_RESULT_VAL, /* u8 */
> >>
> >> Could you maintain the same "namespace" for all attrs related to
> >> selftests?
> >>
> >
> >Will fix it.
> >
> >
> >>
> >> > /* add new attributes above here, update the policy in devlink.c */
> >> >
> >> > __DEVLINK_ATTR_MAX,
> >> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> >index db61f3a341cb..0b7341ab6379 100644
> >> >--- a/net/core/devlink.c
> >> >+++ b/net/core/devlink.c
> >> >@@ -4794,6 +4794,136 @@ static int devlink_nl_cmd_flash_update(struct
> >> sk_buff *skb,
> >> > return ret;
> >> > }
> >> >
> >> >+static int devlink_selftest_name_put(struct sk_buff *skb, int test)
> >> >+{
> >> >+ const char *name = devlink_selftest_name(test);
> >> >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name))
> >> >+ return -EMSGSIZE;
> >> >+
> >> >+ return 0;
> >> >+}
> >> >+
> >> >+static int devlink_nl_cmd_selftests_show(struct sk_buff *skb,
> >> >+ struct genl_info *info)
> >> >+{
> >> >+ struct devlink *devlink = info->user_ptr[0];
> >> >+ struct sk_buff *msg;
> >> >+ unsigned long tests;
> >> >+ int err = 0;
> >> >+ void *hdr;
> >> >+ int test;
> >> >+
> >> >+ if (!devlink->ops->selftests_show)
> >> >+ return -EOPNOTSUPP;
> >> >+
> >> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >> >+ if (!msg)
> >> >+ return -ENOMEM;
> >> >+
> >> >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >> >+ &devlink_nl_family, 0,
> >> DEVLINK_CMD_SELFTESTS_SHOW);
> >> >+ if (!hdr)
> >> >+ goto free_msg;
> >> >+
> >> >+ if (devlink_nl_put_handle(msg, devlink))
> >> >+ goto genlmsg_cancel;
> >> >+
> >> >+ tests = devlink->ops->selftests_show(devlink, info->extack);
> >> >+
> >> >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
> >> >+ err = devlink_selftest_name_put(msg, test);
> >> >+ if (err)
> >> >+ goto genlmsg_cancel;
> >> >+ }
> >> >+
> >> >+ genlmsg_end(msg, hdr);
> >> >+
> >> >+ return genlmsg_reply(msg, info);
> >> >+
> >> >+genlmsg_cancel:
> >> >+ genlmsg_cancel(msg, hdr);
> >> >+free_msg:
> >> >+ nlmsg_free(msg);
> >> >+ return err;
> >> >+}
> >> >+
> >> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test,
> >> >+ u8 result)
> >> >+{
> >> >+ const char *name = devlink_selftest_name(test);
> >> >+ struct nlattr *result_attr;
> >> >+
> >> >+ result_attr = nla_nest_start_noflag(skb, DEVLINK_ATTR_TEST_RESULT);
> >> >+ if (!result_attr)
> >> >+ return -EMSGSIZE;
> >> >+
> >> >+ if (nla_put_string(skb, DEVLINK_ATTR_TEST_NAME, name) ||
> >> >+ nla_put_u8(skb, DEVLINK_ATTR_TEST_RESULT_VAL, result))
> >> >+ goto nla_put_failure;
> >> >+
> >> >+ nla_nest_end(skb, result_attr);
> >> >+
> >> >+ return 0;
> >> >+
> >> >+nla_put_failure:
> >> >+ nla_nest_cancel(skb, result_attr);
> >> >+ return -EMSGSIZE;
> >> >+}
> >> >+
> >> >+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
> >> >+ struct genl_info *info)
> >> >+{
> >> >+ u8 test_results[DEVLINK_SELFTEST_MAX_BIT + 1] = {};
> >> >+ struct devlink *devlink = info->user_ptr[0];
> >> >+ unsigned long tests;
> >> >+ struct sk_buff *msg;
> >> >+ u32 tests_mask;
> >> >+ void *hdr;
> >> >+ int err = 0;
> >> >+ int test;
> >> >+
> >> >+ if (!devlink->ops->selftests_run)
> >> >+ return -EOPNOTSUPP;
> >> >+
> >> >+ if (!info->attrs[DEVLINK_ATTR_SELFTESTS_MASK])
> >> >+ return -EINVAL;
> >> >+
> >> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >> >+ if (!msg)
> >> >+ return -ENOMEM;
> >> >+
> >> >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >> >+ &devlink_nl_family, 0,
> >> DEVLINK_CMD_SELFTESTS_RUN);
> >> >+ if (!hdr)
> >> >+ goto free_msg;
> >> >+
> >> >+ if (devlink_nl_put_handle(msg, devlink))
> >> >+ goto genlmsg_cancel;
> >> >+
> >> >+ tests_mask = nla_get_u32(info->attrs[DEVLINK_ATTR_SELFTESTS_MASK]);
> >> >+
> >> >+ devlink->ops->selftests_run(devlink, tests_mask, test_results,
> >>
> >> Why don't you run it 1 by 1 and fill up the NL message 1 by 1 too?
> >>
> >> I`ll consider it in the next patch set.
>
> Please do. This array of results returned from driver looks sloppy.
>
>
> >
> >
> >>
> >> >+ info->extack);
> >> >+ tests = tests_mask;
> >> >+
> >> >+ for_each_set_bit(test, &tests, __DEVLINK_SELFTEST_MAX_BIT) {
> >> >+ err = devlink_selftest_result_put(msg, test,
> >> >+ test_results[test]);
> >> >+ if (err)
> >> >+ goto genlmsg_cancel;
> >> >+ }
> >> >+
> >> >+ genlmsg_end(msg, hdr);
> >> >+
> >> >+ return genlmsg_reply(msg, info);
> >> >+
> >> >+genlmsg_cancel:
> >> >+ genlmsg_cancel(msg, hdr);
> >> >+free_msg:
> >> >+ nlmsg_free(msg);
> >> >+ return err;
> >> >+}
> >> >+
> >> > static const struct devlink_param devlink_param_generic[] = {
> >> > {
> >> > .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
> >> >@@ -9000,6 +9130,8 @@ static const struct nla_policy
> >> devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> >> > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> >> > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> >> > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
> >> >+ [DEVLINK_ATTR_SELFTESTS_MASK] = NLA_POLICY_MASK(NLA_U32,
> >> >+
> >> DEVLINK_SELFTESTS_MASK),
> >> > };
> >> >
> >> > static const struct genl_small_ops devlink_nl_ops[] = {
> >> >@@ -9361,6 +9493,18 @@ static const struct genl_small_ops
> >> devlink_nl_ops[] = {
> >> > .doit = devlink_nl_cmd_trap_policer_set_doit,
> >> > .flags = GENL_ADMIN_PERM,
> >> > },
> >> >+ {
> >> >+ .cmd = DEVLINK_CMD_SELFTESTS_SHOW,
> >> >+ .validate = GENL_DONT_VALIDATE_STRICT |
> >> GENL_DONT_VALIDATE_DUMP,
> >> >+ .doit = devlink_nl_cmd_selftests_show,
> >>
> >> Why don't dump?
> >>
> >
> > I`ll add a dump in the next patchset.
> >
> >Thanks,
> >Vikas
> >
> >
> >>
> >>
> >> >+ .flags = GENL_ADMIN_PERM,
> >> >+ },
> >> >+ {
> >> >+ .cmd = DEVLINK_CMD_SELFTESTS_RUN,
> >> >+ .validate = GENL_DONT_VALIDATE_STRICT |
> >> GENL_DONT_VALIDATE_DUMP,
> >> >+ .doit = devlink_nl_cmd_selftests_run,
> >> >+ .flags = GENL_ADMIN_PERM,
> >> >+ },
> >> > };
> >> >
> >> > static struct genl_family devlink_nl_family __ro_after_init = {
> >> >--
> >> >2.31.1
> >> >
> >>
> >>
> >>
>
>
Tue, Jul 12, 2022 at 06:41:49PM CEST, [email protected] wrote:
>Hi Jiri,
>
>On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <[email protected]> wrote:
>>
>> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
>> >Hi Jiri,
>> >
>> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
>> >
>> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] wrote:
[...]
>> >> > * enum devlink_trap_action - Packet trap action.
>> >> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy
>> >> is not
>> >> >@@ -576,6 +598,10 @@ enum devlink_attr {
>> >> > DEVLINK_ATTR_LINECARD_TYPE, /* string */
>> >> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
>> >> >
>> >> >+ DEVLINK_ATTR_SELFTESTS_MASK, /* u32 */
>> >>
>> >> I don't see why this is u32 bitset. Just have one attr per test
>> >> (NLA_FLAG) in a nested attr instead.
>> >>
>> >
>> >As per your suggestion, for an example it should be like as below
>> >
>> > DEVLINK_ATTR_SELFTESTS, /* nested */
>> >
>> > DEVLINK_ATTR_SELFTESTS_SOMETEST1 /* flag */
>> >
>> > DEVLINK_ATTR_SELFTESTS_SOMETEST2 /* flag */
>>
>> Yeah, but have the flags in separate enum, no need to pullute the
>> devlink_attr enum by them.
>>
>>
>> >
>> >.... <SOME MORE TESTS>
>> >
>> >.....
>> >
>> > DEVLINK_ATTR_SLEFTESTS_RESULT_VAL, /* u8 */
>> >
>> >
>> >
>> > If we have this way then we need to have a mapping (probably a function)
>> >for drivers to tell them what tests need to be executed based on the flags
>> >that are set.
>> > Does this look OK?
>> > The rationale behind choosing a mask is that we could directly pass the
>> >mask-value to the drivers.
>>
>> If you have separate enum, you can use the attrs as bits internally in
>> kernel. Add a helper that would help the driver to work with it.
>> Pass a struct containing u32 (or u8) not to drivers. Once there are more
>> tests than that, this structure can be easily extended and the helpers
>> changed. This would make this scalable. No need for UAPI change or even
>> internel driver api change.
>
>As per your suggestion, selftest attributes can be declared in separate
>enum as below
>
>enum {
>
> DEVLINK_SELFTEST_SOMETEST, /* flag */
>
> DEVLINK_SELFTEST_SOMETEST1,
>
> DEVLINK_SELFTEST_SOMETEST2,
>
>....
>
>......
>
> __DEVLINK_SELFTEST_MAX,
>
> DEVLINK_SELFTEST_MAX = __DEVLINK_SELFTEST_MAX - 1
>
>};
>Below examples could be the flow of parameters/data from user to
>kernel and vice-versa
>
>
>Kernel to user for show command . Users can know what all tests are
>supported by the driver. A return from kernel to user.
>______
>|NEST |
>|_____ |TEST1|TEST4|TEST7|...
>
>
>User to kernel to execute test: If user wants to execute test4, test8, test1...
>______
>|NEST |
>|_____ |TEST4|TEST8|TEST1|...
>
>
>Result Kernel to user execute test RES(u8)
>______
>|NEST |
>|_____ |RES4|RES8|RES1|...
Hmm, I think it is not good idea to rely on the order, a netlink library
can perhaps reorder it? Not sure here.
>
>Results are populated in the same order as the user passed the TESTs
>flags. Does the above result format from kernel to user look OK ?
>Else we need to have below way to form a result format, a nest should
>be made for <test_flag,
>result> but since test flags are in different enum other than
>devlink_attr and RES being part of devlink_attr, I believe it's not
>good practice to make the below structure.
Not a structure, no. Have it as another nest (could be the same attr as
the parent nest:
______
|NEST |
|_____ |NEST| |NEST| |NEST|
TEST4,RES4 TEST8,RES8 TEST1, RES1
also, it is flexible to add another attr if needed (like maybe result
message string containing error message? IDK).
>______
>|NEST |
>|_____ | TEST4, RES4|TEST8,RES8|TEST1,RES1|...
>
>Let me know if my understanding is correct.
[...]
Hi Jiri,
On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <[email protected]> wrote:
>
> Tue, Jul 12, 2022 at 06:41:49PM CEST, [email protected] wrote:
> >Hi Jiri,
> >
> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <[email protected]> wrote:
> >>
> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
> >> >Hi Jiri,
> >> >
> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
> >> >
> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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, [email protected] wrote:
>Hi Jiri,
>
>On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <[email protected]> wrote:
>>
>> Tue, Jul 12, 2022 at 06:41:49PM CEST, [email protected] wrote:
>> >Hi Jiri,
>> >
>> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <[email protected]> wrote:
>> >>
>> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
>> >> >Hi Jiri,
>> >> >
>> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
>> >> >
>> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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 <[email protected]> wrote:
>
> Wed, Jul 13, 2022 at 08:40:50AM CEST, [email protected] wrote:
> >Hi Jiri,
> >
> >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <[email protected]> wrote:
> >>
> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, [email protected] wrote:
> >> >Hi Jiri,
> >> >
> >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <[email protected]> wrote:
> >> >>
> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
> >> >> >Hi Jiri,
> >> >> >
> >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
> >> >> >
> >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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, [email protected] wrote:
>Hi Jiri,
>
>On Wed, Jul 13, 2022 at 12:58 PM Jiri Pirko <[email protected]> wrote:
>>
>> Wed, Jul 13, 2022 at 08:40:50AM CEST, [email protected] wrote:
>> >Hi Jiri,
>> >
>> >On Tue, Jul 12, 2022 at 11:38 PM Jiri Pirko <[email protected]> wrote:
>> >>
>> >> Tue, Jul 12, 2022 at 06:41:49PM CEST, [email protected] wrote:
>> >> >Hi Jiri,
>> >> >
>> >> >On Tue, Jul 12, 2022 at 11:58 AM Jiri Pirko <[email protected]> wrote:
>> >> >>
>> >> >> Tue, Jul 12, 2022 at 08:16:11AM CEST, [email protected] wrote:
>> >> >> >Hi Jiri,
>> >> >> >
>> >> >> >On Mon, Jul 11, 2022 at 6:10 PM Jiri Pirko <[email protected]> wrote:
>> >> >> >
>> >> >> >> Thu, Jul 07, 2022 at 08:29:48PM CEST, [email protected] 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.
>> >>
>> >> [...]
>>
>>