On Mon, 18 Jul 2022 11:50:30 +0530 Vikas Gupta wrote:
> + for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
> + u8 res;
> +
> + if (nla_get_flag(tb[i])) {
> + res = devlink->ops->selftest_run(devlink, i,
Shouldn't we selftest_check() first to make sure the driver supports
given test?
> + [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
... = NLA_POLICY_NESTED(devlink_selftest_nl_policy),
Hi Jakub,
On Tue, Jul 19, 2022 at 9:03 AM Jakub Kicinski <[email protected]> wrote:
>
> On Mon, 18 Jul 2022 11:50:30 +0530 Vikas Gupta wrote:
> > + for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
> > + u8 res;
> > +
> > + if (nla_get_flag(tb[i])) {
> > + res = devlink->ops->selftest_run(devlink, i,
>
> Shouldn't we selftest_check() first to make sure the driver supports
> given test?
Yes, I`ll add selftest_check() here rather than letting the driver return SKIP.
Thanks,
Vikas
>
> > + [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
>
> ... = NLA_POLICY_NESTED(devlink_selftest_nl_policy),
Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
>selftests by the drivers.
>devlink_nl_cmd_selftests_run: To execute selftests. Users can
>provide a test mask for executing group tests or standalone tests.
>
>Documentation/networking/devlink/ path is already part of MAINTAINERS &
>the new files come under this path. Hence no update needed to the
>MAINTAINERS
>
>Signed-off-by: Vikas Gupta <[email protected]>
>Reviewed-by: Michael Chan <[email protected]>
>Reviewed-by: Andy Gospodarek <[email protected]>
>---
> .../networking/devlink/devlink-selftests.rst | 38 +++
> include/net/devlink.h | 20 ++
> include/uapi/linux/devlink.h | 29 +++
> net/core/devlink.c | 225 ++++++++++++++++++
> 4 files changed, 312 insertions(+)
> create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
>
>diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst
>new file mode 100644
>index 000000000000..0e9727895987
>--- /dev/null
>+++ b/Documentation/networking/devlink/devlink-selftests.rst
>@@ -0,0 +1,38 @@
>+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>+
>+=================
>+Devlink Selftests
>+=================
>+
>+The ``devlink-selftests`` API allows executing selftests on the device.
>+
>+Tests Mask
>+==========
>+The ``devlink-selftests`` command should be run with a mask indicating
>+the tests to be executed.
>+
>+Tests Description
>+=================
>+The following is a list of tests that drivers may execute.
>+
>+.. list-table:: List of tests
>+ :widths: 5 90
>+
>+ * - Name
>+ - Description
>+ * - ``DEVLINK_SELFTEST_FLASH``
>+ - Devices may have the firmware on non-volatile memory on the board, e.g.
>+ flash. This particular test helps to run a flash selftest on the device.
>+ Implementation of the test is left to the driver/firmware.
>+
>+example usage
>+-------------
>+
>+.. code:: shell
>+
>+ # Query selftests supported on the devlink device
>+ $ devlink dev selftests show DEV
>+ # Query selftests supported on all devlink devices
>+ $ devlink dev selftests show
>+ # Executes selftests on the device
>+ $ devlink dev selftests run DEV test flash
"test_id" to be consistend with the attr name and outputs. Please see
below. Devlink cmdline would accept "test" as well, so you can still use
this.
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 88c701b375a2..085d761f1cd3 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1509,6 +1509,26 @@ struct devlink_ops {
> struct devlink_rate *parent,
> void *priv_child, void *priv_parent,
> struct netlink_ext_ack *extack);
>+ /**
>+ * selftests_check() - queries if selftest is supported
>+ * @devlink: Devlink instance
Why capital "D"?
>+ * @test_id: test index
>+ * @extack: extack for reporting error messages
>+ *
>+ * Return: true if test is supported by the driver
>+ */
>+ bool (*selftest_check)(struct devlink *devlink, int test_id,
Why this is an "int". I would be surprised to see a negative value here.
Have this unsigned please.
>+ struct netlink_ext_ack *extack);
>+ /**
>+ * selftest_run() - Runs a selftest
>+ * @devlink: Devlink instance
>+ * @test_id: test index
>+ * @extack: extack for reporting error messages
>+ *
>+ * Return: Result of the test
>+ */
>+ u8 (*selftest_run)(struct devlink *devlink, int test_id,
There too.
>+ struct netlink_ext_ack *extack);
> };
>
> void *devlink_priv(struct devlink *devlink);
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index b3d40a5d72ff..469846f40e6d 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -136,6 +136,9 @@ enum devlink_command {
> DEVLINK_CMD_LINECARD_NEW,
> DEVLINK_CMD_LINECARD_DEL,
>
>+ DEVLINK_CMD_SELFTESTS_LIST, /* can dump */
The rest of the commands are named "_GET". Please be consistent with
them.
>+ DEVLINK_CMD_SELFTESTS_RUN,
>+
> /* add new commands above here */
> __DEVLINK_CMD_MAX,
> DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
>@@ -276,6 +279,31 @@ enum {
> #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
>
>+/* Commonly used test cases */
What do you mean by "commonly". Are there some others that are not
"common"? I don't follow.
>+enum devlink_selftest_attr {
>+ DEVLINK_SELFTEST_ATTR_UNSPEC,
>+ DEVLINK_SELFTEST_ATTR_FLASH, /* flag */
>+
>+ __DEVLINK_SELFTEST_ATTR_MAX,
>+ DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
>+};
To be consistent with the attr that caries this:
enum devlink_attr_selftest_test_id {
DEVLINK_ATTR_SELFTEST_TEST_ID_UNSPEC,
DEVLINK_ATTR_SELFTEST_TEST_ID_FLASH, /* flag */
__DEVLINK_ATTR_SELFTEST_TEST_ID_MAX,
DEVLINK_ATTR_SELFTEST_TEST_ID_MAX = __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX - 1
>+
>+enum devlink_selftest_result {
>+ DEVLINK_SELFTEST_SKIP,
>+ DEVLINK_SELFTEST_PASS,
>+ DEVLINK_SELFTEST_FAIL
It is common to have the enum name be root of names of the values.
Also, be consistent with the attr this value is carried over:
enum devlink_selftest_test_status {
DEVLINK_SELFTEST_TEST_STATUS_SKIP,
DEVLINK_SELFTEST_TEST_STATUS_PASS,
DEVLINK_SELFTEST_TEST_STATUS_FAIL
That way, it is obvious to which enum the value belongs.
>+};
>+
>+enum devlink_selftest_result_attr {
>+ DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
>+ DEVLINK_SELFTEST_ATTR_RESULT, /* nested */
>+ DEVLINK_SELFTEST_ATTR_TEST_ID, /* u32, devlink_selftest_attr */
add "enum" ?
>+ DEVLINK_SELFTEST_ATTR_TEST_STATUS, /* u8, devlink_selftest_result */
add "enum" ?
The same note as above:
enum devlink_attr_selftest_result {
DEVLINK_ATTR_SELFTEST_RESULT_UNSPEC,
DEVLINK_ATTR_SELFTEST_RESULT, /* nested */
DEVLINK_ATTR_SELFTEST_RESULT_TEST_ID, /* u32, enum devlink_selftest_attr */
DEVLINK_ATTR_SELFTEST_RESULT_TEST_STATUS, /* u8, devlink_selftest_result */
>+
>+ __DEVLINK_SELFTEST_ATTR_RES_MAX,
>+ DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
>+};
>+
> /**
> * enum devlink_trap_action - Packet trap action.
> * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
>@@ -576,6 +604,7 @@ enum devlink_attr {
> DEVLINK_ATTR_LINECARD_TYPE, /* string */
> DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
>
>+ DEVLINK_ATTR_SELFTESTS_INFO, /* nested */
> /* add new attributes above here, update the policy in devlink.c */
>
> __DEVLINK_ATTR_MAX,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index a9776ea923ae..ef9439f2502f 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> DEVLINK_PORT_FN_STATE_ACTIVE),
> };
>
>+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
>+ [DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
>+};
>+
> static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> #define DEVLINK_REGISTERED XA_MARK_1
>
>@@ -4791,6 +4795,215 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> return ret;
> }
>
>+static int
>+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
>+ u32 portid, u32 seq, int flags,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct nlattr *selftests_list;
>+ void *hdr;
>+ int err;
>+ int i;
>+
>+ hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
>+ DEVLINK_CMD_SELFTESTS_LIST);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+
>+ err = -EMSGSIZE;
>+ if (devlink_nl_put_handle(msg, devlink))
>+ goto err_cancel_msg;
>+
>+ selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+ if (!selftests_list)
>+ goto err_cancel_msg;
>+
>+ for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
**)
It is a bit odd to see "1" here. Maybe "DEVLINK_SELFTEST_ATTR_UNSPEC + 1"
would be more obvious for the reader.
also:
i < DEVLINK_SELFTEST_ATTR_MAX + 1
would be rather nicer to be:
i <= DEVLINK_SELFTEST_ATTR_MAX
>+ if (devlink->ops->selftest_check(devlink, i, extack)) {
>+ err = nla_put_flag(msg, i);
>+ if (err)
>+ goto err_cancel_msg;
>+ }
>+ }
>+
>+ nla_nest_end(msg, selftests_list);
>+
No need for this empty line.
>+ genlmsg_end(msg, hdr);
>+
No need for this empty line.
>+ return 0;
>+
>+err_cancel_msg:
>+ genlmsg_cancel(msg, hdr);
>+ return err;
>+}
>+
>+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ struct devlink *devlink = info->user_ptr[0];
>+ struct sk_buff *msg;
>+ int err;
>+
>+ if (!devlink->ops->selftest_check)
>+ return -EOPNOTSUPP;
>+
>+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+
>+ err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
>+ info->snd_seq, 0, info->extack);
>+ if (err) {
>+ nlmsg_free(msg);
>+ return err;
>+ }
>+
>+ return genlmsg_reply(msg, info);
>+}
>+
>+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
>+ struct netlink_callback *cb)
>+{
>+ struct devlink *devlink;
>+ int start = cb->args[0];
>+ unsigned long index;
>+ int idx = 0;
>+ int err = 0;
>+
>+ mutex_lock(&devlink_mutex);
>+ xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>+ if (!devlink_try_get(devlink))
>+ continue;
>+
>+ if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>+ goto retry;
>+
>+ if (idx < start || !devlink->ops->selftest_check)
>+ goto inc;
>+
>+ mutex_lock(&devlink->lock);
>+ err = devlink_nl_selftests_fill(msg, devlink,
>+ NETLINK_CB(cb->skb).portid,
>+ cb->nlh->nlmsg_seq, NLM_F_MULTI,
>+ cb->extack);
>+ mutex_unlock(&devlink->lock);
>+ if (err) {
>+ devlink_put(devlink);
>+ break;
>+ }
>+inc:
>+ idx++;
>+retry:
>+ devlink_put(devlink);
>+ }
>+ mutex_unlock(&devlink_mutex);
>+
>+ if (err != -EMSGSIZE)
>+ return err;
>+
>+ cb->args[0] = idx;
>+ return msg->len;
>+}
>+
>+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,
unsigned.
>+ u8 result)
Please be consistend and call this "test_status"
>+{
>+ struct nlattr *result_attr;
>+
>+ result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
>+ if (!result_attr)
>+ return -EMSGSIZE;
>+
>+ if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
>+ nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, result))
>+ goto nla_put_failure;
>+
>+ nla_nest_end(skb, result_attr);
>+
No need for this empty line.
>+ return 0;
>+
>+nla_put_failure:
>+ nla_nest_cancel(skb, result_attr);
>+ return -EMSGSIZE;
>+}
>+
>+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
>+ struct genl_info *info)
>+{
>+ struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
>+ struct devlink *devlink = info->user_ptr[0];
>+ struct nlattr *attrs, *tests_info;
>+ struct sk_buff *msg;
>+ void *hdr;
>+ int err;
>+ int i;
>+
>+ if (!devlink->ops->selftest_run)
>+ return -EOPNOTSUPP;
>+
>+ if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])
Fill extack message here please.
>+ return -EINVAL;
>+
>+ attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
>+
>+ err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
>+ devlink_selftest_nl_policy, info->extack);
>+ if (err < 0)
>+ return err;
>+
>+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+ if (!msg)
>+ return -ENOMEM;
>+
>+ err = -EMSGSIZE;
>+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
>+ &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
>+ if (!hdr)
>+ goto free_msg;
>+
>+ if (devlink_nl_put_handle(msg, devlink))
>+ goto genlmsg_cancel;
>+
>+ tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
>+ if (!tests_info)
>+ goto genlmsg_cancel;
>+
>+ for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
Same notes to the iteration as above. **
>+ u8 res = DEVLINK_SELFTEST_SKIP;
u8 test_status;
>+
>+ if (nla_get_flag(tb[i])) {
>+ if (devlink->ops->selftest_check &&
No need to test in every iteration. I think it is safe to assume
that driver that does not fill selftest_check() does not support
selftests at all, so please move to the beginning of this function
alongside selftest_run() check:
if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
return -EOPNOTSUPP;
>+ !devlink->ops->selftest_check(devlink, i,
>+ info->extack)) {
>+ err = devlink_selftest_result_put(msg, i, res);
Just do devlink_selftest_result_put(msg, i, .._SKIP); here and avoid
initializing "res" at the beginning.
>+ if (err)
>+ goto selftests_list_nest_cancel;
>+ continue;
>+ }
>+
>+ res = devlink->ops->selftest_run(devlink, i,
>+ info->extack);
>+ err = devlink_selftest_result_put(msg, i, res);
>+ if (err)
>+ goto selftests_list_nest_cancel;
>+ }
>+ }
>+
>+ nla_nest_end(msg, tests_info);
>+
No need for this empty line.
>+ genlmsg_end(msg, hdr);
>+
No need for this empty line.
>+ return genlmsg_reply(msg, info);
>+
>+selftests_list_nest_cancel:
>+ nla_nest_cancel(msg, tests_info);
>+genlmsg_cancel:
>+ genlmsg_cancel(msg, hdr);
>+free_msg:
>+ nlmsg_free(msg);
>+ return err;
>+}
>+
> static const struct devlink_param devlink_param_generic[] = {
> {
> .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
>@@ -8997,6 +9210,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
>+ [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
> };
>
> static const struct genl_small_ops devlink_nl_ops[] = {
>@@ -9356,6 +9570,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> .doit = devlink_nl_cmd_trap_policer_set_doit,
> .flags = GENL_ADMIN_PERM,
> },
>+ {
>+ .cmd = DEVLINK_CMD_SELFTESTS_LIST,
>+ .doit = devlink_nl_cmd_selftests_list_doit,
>+ .dumpit = devlink_nl_cmd_selftests_list_dumpit
>+ /* can be retrieved by unprivileged users */
>+ },
>+ {
>+ .cmd = DEVLINK_CMD_SELFTESTS_RUN,
>+ .doit = devlink_nl_cmd_selftests_run,
>+ .flags = GENL_ADMIN_PERM,
>+ },
> };
>
> static struct genl_family devlink_nl_family __ro_after_init = {
>--
>2.31.1
>
Thu, Jul 21, 2022 at 09:21:19AM CEST, [email protected] wrote:
[...]
>+ /**
>+ * selftest_run() - Runs a selftest
>+ * @devlink: Devlink instance
>+ * @test_id: test index
>+ * @extack: extack for reporting error messages
>+ *
>+ * Return: Result of the test
>+ */
>+ u8 (*selftest_run)(struct devlink *devlink, int test_id,
It should return enum. Make sure you changed the variable type you store
the test status in.
>+ struct netlink_ext_ack *extack);
> };
[...]
"bnxt_en: refactor NVM APIs" is very odd patch subject name. Please
change it to the summary of what the patch is aiming to do.
Thu, Jul 21, 2022 at 09:21:21AM CEST, [email protected] wrote:
>Add callbacks
>=============
>.selftest_check: returns true for flash selftest.
>.selftest_run: runs a flash selftest.
>
>Signed-off-by: Vikas Gupta <[email protected]>
>Reviewed-by: Michael Chan <[email protected]>
>Reviewed-by: Andy Gospodarek <[email protected]>
>---
> .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 6b3d4f4c2a75..927cf368d856 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>@@ -20,6 +20,8 @@
> #include "bnxt_ulp.h"
> #include "bnxt_ptp.h"
> #include "bnxt_coredump.h"
>+#include "bnxt_nvm_defs.h"
>+#include "bnxt_ethtool.h"
>
> static void __bnxt_fw_recover(struct bnxt *bp)
> {
>@@ -610,6 +612,62 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
> return rc;
> }
>
>+static bool bnxt_nvm_test(struct bnxt *bp, struct netlink_ext_ack *extack)
>+{
>+ u32 datalen;
>+ u16 index;
>+ u8 *buf;
>+
>+ if (bnxt_find_nvram_item(bp->dev, BNX_DIR_TYPE_VPD,
>+ BNX_DIR_ORDINAL_FIRST, BNX_DIR_EXT_NONE,
>+ &index, NULL, &datalen) || !datalen) {
>+ NL_SET_ERR_MSG_MOD(extack, "nvm test vpd entry error");
>+ return false;
>+ }
>+
>+ buf = kzalloc(datalen, GFP_KERNEL);
>+ if (!buf) {
>+ NL_SET_ERR_MSG_MOD(extack, "insufficient memory for nvm test");
>+ return false;
>+ }
>+
>+ if (bnxt_get_nvram_item(bp->dev, index, 0, datalen, buf)) {
>+ NL_SET_ERR_MSG_MOD(extack, "nvm test vpd read error");
>+ goto err;
>+ }
>+
>+ if (bnxt_flash_nvram(bp->dev, BNX_DIR_TYPE_VPD, BNX_DIR_ORDINAL_FIRST,
>+ BNX_DIR_EXT_NONE, 0, 0, buf, datalen)) {
>+ NL_SET_ERR_MSG_MOD(extack, "nvm test vpd write error");
>+ goto err;
>+ }
>+
>+ return true;
>+
>+err:
>+ kfree(buf);
>+ return false;
>+}
>+
>+static bool bnxt_dl_selftest_check(struct devlink *dl, int test_id,
>+ struct netlink_ext_ack *extack)
>+{
>+ return (test_id == DEVLINK_SELFTEST_ATTR_FLASH);
Return is not a function, avoid "()"
>+}
>+
>+static u8 bnxt_dl_selftest_run(struct devlink *dl, int test_id,
>+ struct netlink_ext_ack *extack)
>+{
>+ struct bnxt *bp = bnxt_get_bp_from_dl(dl);
>+
>+ if (test_id == DEVLINK_SELFTEST_ATTR_FLASH) {
>+ return (bnxt_nvm_test(bp, extack) ? DEVLINK_SELFTEST_PASS :
>+ DEVLINK_SELFTEST_FAIL);
Return is not a function, avoid "()"
>+ }
No need for "{}", it's a simple statement. I'm pretty sure checkpatch
would warn you, did you run it?
>+
>+ return DEVLINK_SELFTEST_SKIP;
>+}
>+
> static const struct devlink_ops bnxt_dl_ops = {
> #ifdef CONFIG_BNXT_SRIOV
> .eswitch_mode_set = bnxt_dl_eswitch_mode_set,
>@@ -622,6 +680,8 @@ static const struct devlink_ops bnxt_dl_ops = {
> .reload_limits = BIT(DEVLINK_RELOAD_LIMIT_NO_RESET),
> .reload_down = bnxt_dl_reload_down,
> .reload_up = bnxt_dl_reload_up,
>+ .selftest_check = bnxt_dl_selftest_check,
>+ .selftest_run = bnxt_dl_selftest_run,
> };
>
> static const struct devlink_ops bnxt_vf_dl_ops;
>--
>2.31.1
>
Hi Jiri,
On Thu, Jul 21, 2022 at 6:32 PM Jiri Pirko <[email protected]> wrote:
>
> "bnxt_en: refactor NVM APIs" is very odd patch subject name. Please
> change it to the summary of what the patch is aiming to do.
Seems like this patch might create confusion with this patch series.
Since this patch is just changing the declarations so that a flash
based test(patch# 3/3) can be supported, I would rather merge it with
the next patch(3/3).
Thanks,
Vikas
Hi Jiri,
On Thu, Jul 21, 2022 at 6:27 PM Jiri Pirko <[email protected]> wrote:
>
> Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
> >selftests by the drivers.
> >devlink_nl_cmd_selftests_run: To execute selftests. Users can
> >provide a test mask for executing group tests or standalone tests.
> >
> >Documentation/networking/devlink/ path is already part of MAINTAINERS &
> >the new files come under this path. Hence no update needed to the
> >MAINTAINERS
> >
> >Signed-off-by: Vikas Gupta <[email protected]>
> >Reviewed-by: Michael Chan <[email protected]>
> >Reviewed-by: Andy Gospodarek <[email protected]>
> >---
> > .../networking/devlink/devlink-selftests.rst | 38 +++
> > include/net/devlink.h | 20 ++
> > include/uapi/linux/devlink.h | 29 +++
> > net/core/devlink.c | 225 ++++++++++++++++++
> > 4 files changed, 312 insertions(+)
> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
> >
> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst
> >new file mode 100644
> >index 000000000000..0e9727895987
> >--- /dev/null
> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
> >@@ -0,0 +1,38 @@
> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >+
> >+=================
> >+Devlink Selftests
> >+=================
> >+
> >+The ``devlink-selftests`` API allows executing selftests on the device.
> >+
> >+Tests Mask
> >+==========
> >+The ``devlink-selftests`` command should be run with a mask indicating
> >+the tests to be executed.
> >+
> >+Tests Description
> >+=================
> >+The following is a list of tests that drivers may execute.
> >+
> >+.. list-table:: List of tests
> >+ :widths: 5 90
> >+
> >+ * - Name
> >+ - Description
> >+ * - ``DEVLINK_SELFTEST_FLASH``
> >+ - Devices may have the firmware on non-volatile memory on the board, e.g.
> >+ flash. This particular test helps to run a flash selftest on the device.
> >+ Implementation of the test is left to the driver/firmware.
> >+
> >+example usage
> >+-------------
> >+
> >+.. code:: shell
> >+
> >+ # Query selftests supported on the devlink device
> >+ $ devlink dev selftests show DEV
> >+ # Query selftests supported on all devlink devices
> >+ $ devlink dev selftests show
> >+ # Executes selftests on the device
> >+ $ devlink dev selftests run DEV test flash
>
> "test_id" to be consistend with the attr name and outputs. Please see
What is "test_id" referring to in this document? Can you please elaborate ?
> below. Devlink cmdline would accept "test" as well, so you can still use
Are you mentioning the "test" argument in the above devlink command line option?
Thanks,
Vikas
> this.
>
>
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 88c701b375a2..085d761f1cd3 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -1509,6 +1509,26 @@ struct devlink_ops {
> > struct devlink_rate *parent,
> > void *priv_child, void *priv_parent,
> > struct netlink_ext_ack *extack);
> >+ /**
> >+ * selftests_check() - queries if selftest is supported
> >+ * @devlink: Devlink instance
>
> Why capital "D"?
>
>
> >+ * @test_id: test index
> >+ * @extack: extack for reporting error messages
> >+ *
> >+ * Return: true if test is supported by the driver
> >+ */
> >+ bool (*selftest_check)(struct devlink *devlink, int test_id,
>
> Why this is an "int". I would be surprised to see a negative value here.
> Have this unsigned please.
>
>
> >+ struct netlink_ext_ack *extack);
> >+ /**
> >+ * selftest_run() - Runs a selftest
> >+ * @devlink: Devlink instance
> >+ * @test_id: test index
> >+ * @extack: extack for reporting error messages
> >+ *
> >+ * Return: Result of the test
> >+ */
> >+ u8 (*selftest_run)(struct devlink *devlink, int test_id,
>
> There too.
>
>
> >+ struct netlink_ext_ack *extack);
> > };
> >
> > void *devlink_priv(struct devlink *devlink);
> >diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >index b3d40a5d72ff..469846f40e6d 100644
> >--- a/include/uapi/linux/devlink.h
> >+++ b/include/uapi/linux/devlink.h
> >@@ -136,6 +136,9 @@ enum devlink_command {
> > DEVLINK_CMD_LINECARD_NEW,
> > DEVLINK_CMD_LINECARD_DEL,
> >
> >+ DEVLINK_CMD_SELFTESTS_LIST, /* can dump */
>
> The rest of the commands are named "_GET". Please be consistent with
> them.
>
>
> >+ DEVLINK_CMD_SELFTESTS_RUN,
> >+
> > /* add new commands above here */
> > __DEVLINK_CMD_MAX,
> > DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
> >@@ -276,6 +279,31 @@ enum {
> > #define DEVLINK_SUPPORTED_FLASH_OVERWRITE_SECTIONS \
> > (_BITUL(__DEVLINK_FLASH_OVERWRITE_MAX_BIT) - 1)
> >
> >+/* Commonly used test cases */
>
> What do you mean by "commonly". Are there some others that are not
> "common"? I don't follow.
>
>
> >+enum devlink_selftest_attr {
> >+ DEVLINK_SELFTEST_ATTR_UNSPEC,
> >+ DEVLINK_SELFTEST_ATTR_FLASH, /* flag */
> >+
> >+ __DEVLINK_SELFTEST_ATTR_MAX,
> >+ DEVLINK_SELFTEST_ATTR_MAX = __DEVLINK_SELFTEST_ATTR_MAX - 1
> >+};
>
> To be consistent with the attr that caries this:
>
> enum devlink_attr_selftest_test_id {
> DEVLINK_ATTR_SELFTEST_TEST_ID_UNSPEC,
> DEVLINK_ATTR_SELFTEST_TEST_ID_FLASH, /* flag */
>
> __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX,
> DEVLINK_ATTR_SELFTEST_TEST_ID_MAX = __DEVLINK_ATTR_SELFTEST_TEST_ID_MAX - 1
>
> >+
> >+enum devlink_selftest_result {
> >+ DEVLINK_SELFTEST_SKIP,
> >+ DEVLINK_SELFTEST_PASS,
> >+ DEVLINK_SELFTEST_FAIL
>
> It is common to have the enum name be root of names of the values.
> Also, be consistent with the attr this value is carried over:
>
> enum devlink_selftest_test_status {
> DEVLINK_SELFTEST_TEST_STATUS_SKIP,
> DEVLINK_SELFTEST_TEST_STATUS_PASS,
> DEVLINK_SELFTEST_TEST_STATUS_FAIL
>
> That way, it is obvious to which enum the value belongs.
>
>
> >+};
> >+
> >+enum devlink_selftest_result_attr {
> >+ DEVLINK_SELFTEST_ATTR_RESULT_UNSPEC,
> >+ DEVLINK_SELFTEST_ATTR_RESULT, /* nested */
> >+ DEVLINK_SELFTEST_ATTR_TEST_ID, /* u32, devlink_selftest_attr */
>
> add "enum" ?
>
> >+ DEVLINK_SELFTEST_ATTR_TEST_STATUS, /* u8, devlink_selftest_result */
>
> add "enum" ?
>
> The same note as above:
> enum devlink_attr_selftest_result {
> DEVLINK_ATTR_SELFTEST_RESULT_UNSPEC,
> DEVLINK_ATTR_SELFTEST_RESULT, /* nested */
> DEVLINK_ATTR_SELFTEST_RESULT_TEST_ID, /* u32, enum devlink_selftest_attr */
> DEVLINK_ATTR_SELFTEST_RESULT_TEST_STATUS, /* u8, devlink_selftest_result */
>
>
>
>
> >+
> >+ __DEVLINK_SELFTEST_ATTR_RES_MAX,
> >+ DEVLINK_SELFTEST_ATTR_RES_MAX = __DEVLINK_SELFTEST_ATTR_RES_MAX - 1
> >+};
> >+
> > /**
> > * enum devlink_trap_action - Packet trap action.
> > * @DEVLINK_TRAP_ACTION_DROP: Packet is dropped by the device and a copy is not
> >@@ -576,6 +604,7 @@ enum devlink_attr {
> > DEVLINK_ATTR_LINECARD_TYPE, /* string */
> > DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES, /* nested */
> >
> >+ DEVLINK_ATTR_SELFTESTS_INFO, /* nested */
> > /* add new attributes above here, update the policy in devlink.c */
> >
> > __DEVLINK_ATTR_MAX,
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index a9776ea923ae..ef9439f2502f 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -198,6 +198,10 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> > DEVLINK_PORT_FN_STATE_ACTIVE),
> > };
> >
> >+static const struct nla_policy devlink_selftest_nl_policy[DEVLINK_SELFTEST_ATTR_MAX + 1] = {
> >+ [DEVLINK_SELFTEST_ATTR_FLASH] = { .type = NLA_FLAG },
> >+};
> >+
> > static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> > #define DEVLINK_REGISTERED XA_MARK_1
> >
> >@@ -4791,6 +4795,215 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> > return ret;
> > }
> >
> >+static int
> >+devlink_nl_selftests_fill(struct sk_buff *msg, struct devlink *devlink,
> >+ u32 portid, u32 seq, int flags,
> >+ struct netlink_ext_ack *extack)
> >+{
> >+ struct nlattr *selftests_list;
> >+ void *hdr;
> >+ int err;
> >+ int i;
> >+
> >+ hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags,
> >+ DEVLINK_CMD_SELFTESTS_LIST);
> >+ if (!hdr)
> >+ return -EMSGSIZE;
> >+
> >+ err = -EMSGSIZE;
> >+ if (devlink_nl_put_handle(msg, devlink))
> >+ goto err_cancel_msg;
> >+
> >+ selftests_list = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
> >+ if (!selftests_list)
> >+ goto err_cancel_msg;
> >+
> >+ for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
>
> **)
> It is a bit odd to see "1" here. Maybe "DEVLINK_SELFTEST_ATTR_UNSPEC + 1"
> would be more obvious for the reader.
>
> also:
> i < DEVLINK_SELFTEST_ATTR_MAX + 1
> would be rather nicer to be:
> i <= DEVLINK_SELFTEST_ATTR_MAX
>
>
> >+ if (devlink->ops->selftest_check(devlink, i, extack)) {
> >+ err = nla_put_flag(msg, i);
> >+ if (err)
> >+ goto err_cancel_msg;
> >+ }
> >+ }
> >+
> >+ nla_nest_end(msg, selftests_list);
> >+
>
> No need for this empty line.
>
>
> >+ genlmsg_end(msg, hdr);
> >+
>
> No need for this empty line.
>
>
> >+ return 0;
> >+
> >+err_cancel_msg:
> >+ genlmsg_cancel(msg, hdr);
> >+ return err;
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_list_doit(struct sk_buff *skb,
> >+ struct genl_info *info)
> >+{
> >+ struct devlink *devlink = info->user_ptr[0];
> >+ struct sk_buff *msg;
> >+ int err;
> >+
> >+ if (!devlink->ops->selftest_check)
> >+ return -EOPNOTSUPP;
> >+
> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >+ if (!msg)
> >+ return -ENOMEM;
> >+
> >+ err = devlink_nl_selftests_fill(msg, devlink, info->snd_portid,
> >+ info->snd_seq, 0, info->extack);
> >+ if (err) {
> >+ nlmsg_free(msg);
> >+ return err;
> >+ }
> >+
> >+ return genlmsg_reply(msg, info);
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_list_dumpit(struct sk_buff *msg,
> >+ struct netlink_callback *cb)
> >+{
> >+ struct devlink *devlink;
> >+ int start = cb->args[0];
> >+ unsigned long index;
> >+ int idx = 0;
> >+ int err = 0;
> >+
> >+ mutex_lock(&devlink_mutex);
> >+ xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> >+ if (!devlink_try_get(devlink))
> >+ continue;
> >+
> >+ if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> >+ goto retry;
> >+
> >+ if (idx < start || !devlink->ops->selftest_check)
> >+ goto inc;
> >+
> >+ mutex_lock(&devlink->lock);
> >+ err = devlink_nl_selftests_fill(msg, devlink,
> >+ NETLINK_CB(cb->skb).portid,
> >+ cb->nlh->nlmsg_seq, NLM_F_MULTI,
> >+ cb->extack);
> >+ mutex_unlock(&devlink->lock);
> >+ if (err) {
> >+ devlink_put(devlink);
> >+ break;
> >+ }
> >+inc:
> >+ idx++;
> >+retry:
> >+ devlink_put(devlink);
> >+ }
> >+ mutex_unlock(&devlink_mutex);
> >+
> >+ if (err != -EMSGSIZE)
> >+ return err;
> >+
> >+ cb->args[0] = idx;
> >+ return msg->len;
> >+}
> >+
> >+static int devlink_selftest_result_put(struct sk_buff *skb, int test_id,
>
> unsigned.
>
> >+ u8 result)
>
> Please be consistend and call this "test_status"
>
>
> >+{
> >+ struct nlattr *result_attr;
> >+
> >+ result_attr = nla_nest_start(skb, DEVLINK_SELFTEST_ATTR_RESULT);
> >+ if (!result_attr)
> >+ return -EMSGSIZE;
> >+
> >+ if (nla_put_u32(skb, DEVLINK_SELFTEST_ATTR_TEST_ID, test_id) ||
> >+ nla_put_u8(skb, DEVLINK_SELFTEST_ATTR_TEST_STATUS, result))
> >+ goto nla_put_failure;
> >+
> >+ nla_nest_end(skb, result_attr);
> >+
>
> No need for this empty line.
>
>
> >+ return 0;
> >+
> >+nla_put_failure:
> >+ nla_nest_cancel(skb, result_attr);
> >+ return -EMSGSIZE;
> >+}
> >+
> >+static int devlink_nl_cmd_selftests_run(struct sk_buff *skb,
> >+ struct genl_info *info)
> >+{
> >+ struct nlattr *tb[DEVLINK_SELFTEST_ATTR_MAX + 1];
> >+ struct devlink *devlink = info->user_ptr[0];
> >+ struct nlattr *attrs, *tests_info;
> >+ struct sk_buff *msg;
> >+ void *hdr;
> >+ int err;
> >+ int i;
> >+
> >+ if (!devlink->ops->selftest_run)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (!info->attrs[DEVLINK_ATTR_SELFTESTS_INFO])
>
> Fill extack message here please.
>
>
> >+ return -EINVAL;
> >+
> >+ attrs = info->attrs[DEVLINK_ATTR_SELFTESTS_INFO];
> >+
> >+ err = nla_parse_nested(tb, DEVLINK_SELFTEST_ATTR_MAX, attrs,
> >+ devlink_selftest_nl_policy, info->extack);
> >+ if (err < 0)
> >+ return err;
> >+
> >+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> >+ if (!msg)
> >+ return -ENOMEM;
> >+
> >+ err = -EMSGSIZE;
> >+ hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq,
> >+ &devlink_nl_family, 0, DEVLINK_CMD_SELFTESTS_RUN);
> >+ if (!hdr)
> >+ goto free_msg;
> >+
> >+ if (devlink_nl_put_handle(msg, devlink))
> >+ goto genlmsg_cancel;
> >+
> >+ tests_info = nla_nest_start(msg, DEVLINK_ATTR_SELFTESTS_INFO);
> >+ if (!tests_info)
> >+ goto genlmsg_cancel;
> >+
> >+ for (i = 1; i < DEVLINK_SELFTEST_ATTR_MAX + 1; i++) {
>
> Same notes to the iteration as above. **
>
>
> >+ u8 res = DEVLINK_SELFTEST_SKIP;
>
> u8 test_status;
>
>
> >+
> >+ if (nla_get_flag(tb[i])) {
> >+ if (devlink->ops->selftest_check &&
>
> No need to test in every iteration. I think it is safe to assume
> that driver that does not fill selftest_check() does not support
> selftests at all, so please move to the beginning of this function
> alongside selftest_run() check:
>
> if (!devlink->ops->selftest_run || !devlink->ops->selftest_check)
> return -EOPNOTSUPP;
>
> >+ !devlink->ops->selftest_check(devlink, i,
> >+ info->extack)) {
> >+ err = devlink_selftest_result_put(msg, i, res);
>
> Just do devlink_selftest_result_put(msg, i, .._SKIP); here and avoid
> initializing "res" at the beginning.
>
>
> >+ if (err)
> >+ goto selftests_list_nest_cancel;
> >+ continue;
> >+ }
> >+
> >+ res = devlink->ops->selftest_run(devlink, i,
> >+ info->extack);
> >+ err = devlink_selftest_result_put(msg, i, res);
> >+ if (err)
> >+ goto selftests_list_nest_cancel;
> >+ }
> >+ }
> >+
> >+ nla_nest_end(msg, tests_info);
> >+
>
> No need for this empty line.
>
>
> >+ genlmsg_end(msg, hdr);
> >+
>
> No need for this empty line.
>
>
> >+ return genlmsg_reply(msg, info);
> >+
> >+selftests_list_nest_cancel:
> >+ nla_nest_cancel(msg, tests_info);
> >+genlmsg_cancel:
> >+ genlmsg_cancel(msg, hdr);
> >+free_msg:
> >+ nlmsg_free(msg);
> >+ return err;
> >+}
> >+
> > static const struct devlink_param devlink_param_generic[] = {
> > {
> > .id = DEVLINK_PARAM_GENERIC_ID_INT_ERR_RESET,
> >@@ -8997,6 +9210,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
> > [DEVLINK_ATTR_RATE_PARENT_NODE_NAME] = { .type = NLA_NUL_STRING },
> > [DEVLINK_ATTR_LINECARD_INDEX] = { .type = NLA_U32 },
> > [DEVLINK_ATTR_LINECARD_TYPE] = { .type = NLA_NUL_STRING },
> >+ [DEVLINK_ATTR_SELFTESTS_INFO] = { .type = NLA_NESTED },
> > };
> >
> > static const struct genl_small_ops devlink_nl_ops[] = {
> >@@ -9356,6 +9570,17 @@ static const struct genl_small_ops devlink_nl_ops[] = {
> > .doit = devlink_nl_cmd_trap_policer_set_doit,
> > .flags = GENL_ADMIN_PERM,
> > },
> >+ {
> >+ .cmd = DEVLINK_CMD_SELFTESTS_LIST,
> >+ .doit = devlink_nl_cmd_selftests_list_doit,
> >+ .dumpit = devlink_nl_cmd_selftests_list_dumpit
> >+ /* can be retrieved by unprivileged users */
> >+ },
> >+ {
> >+ .cmd = DEVLINK_CMD_SELFTESTS_RUN,
> >+ .doit = devlink_nl_cmd_selftests_run,
> >+ .flags = GENL_ADMIN_PERM,
> >+ },
> > };
> >
> > static struct genl_family devlink_nl_family __ro_after_init = {
> >--
> >2.31.1
> >
>
>
Thu, Jul 21, 2022 at 07:32:08PM CEST, [email protected] wrote:
>Hi Jiri,
>
>
>On Thu, Jul 21, 2022 at 6:27 PM Jiri Pirko <[email protected]> wrote:
>>
>> Thu, Jul 21, 2022 at 09:21:19AM 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_list_doit/dumpit: To query the supported
>> >selftests by the drivers.
>> >devlink_nl_cmd_selftests_run: To execute selftests. Users can
>> >provide a test mask for executing group tests or standalone tests.
>> >
>> >Documentation/networking/devlink/ path is already part of MAINTAINERS &
>> >the new files come under this path. Hence no update needed to the
>> >MAINTAINERS
>> >
>> >Signed-off-by: Vikas Gupta <[email protected]>
>> >Reviewed-by: Michael Chan <[email protected]>
>> >Reviewed-by: Andy Gospodarek <[email protected]>
>> >---
>> > .../networking/devlink/devlink-selftests.rst | 38 +++
>> > include/net/devlink.h | 20 ++
>> > include/uapi/linux/devlink.h | 29 +++
>> > net/core/devlink.c | 225 ++++++++++++++++++
>> > 4 files changed, 312 insertions(+)
>> > create mode 100644 Documentation/networking/devlink/devlink-selftests.rst
>> >
>> >diff --git a/Documentation/networking/devlink/devlink-selftests.rst b/Documentation/networking/devlink/devlink-selftests.rst
>> >new file mode 100644
>> >index 000000000000..0e9727895987
>> >--- /dev/null
>> >+++ b/Documentation/networking/devlink/devlink-selftests.rst
>> >@@ -0,0 +1,38 @@
>> >+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >+
>> >+=================
>> >+Devlink Selftests
>> >+=================
>> >+
>> >+The ``devlink-selftests`` API allows executing selftests on the device.
>> >+
>> >+Tests Mask
>> >+==========
>> >+The ``devlink-selftests`` command should be run with a mask indicating
>> >+the tests to be executed.
>> >+
>> >+Tests Description
>> >+=================
>> >+The following is a list of tests that drivers may execute.
>> >+
>> >+.. list-table:: List of tests
>> >+ :widths: 5 90
>> >+
>> >+ * - Name
>> >+ - Description
>> >+ * - ``DEVLINK_SELFTEST_FLASH``
>> >+ - Devices may have the firmware on non-volatile memory on the board, e.g.
>> >+ flash. This particular test helps to run a flash selftest on the device.
>> >+ Implementation of the test is left to the driver/firmware.
>> >+
>> >+example usage
>> >+-------------
>> >+
>> >+.. code:: shell
>> >+
>> >+ # Query selftests supported on the devlink device
>> >+ $ devlink dev selftests show DEV
>> >+ # Query selftests supported on all devlink devices
>> >+ $ devlink dev selftests show
>> >+ # Executes selftests on the device
>> >+ $ devlink dev selftests run DEV test flash
>>
>> "test_id" to be consistend with the attr name and outputs. Please see
>What is "test_id" referring to in this document? Can you please elaborate ?
"test_id" is consistent with the UAPI netlink attribute name, see
below.
>
>> below. Devlink cmdline would accept "test" as well, so you can still use
>Are you mentioning the "test" argument in the above devlink command line option?
Yes, the cmd line accepts shortened option names. So for "test_id", it
would accept:
"test_i"
"test_"
and "test"
I just wanted to say you, as a user, will be able to still use the
shortened version, but the option name "test_id" would be consistent
with UAPI, which is what we usually have.