[patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Motivation:

For SFs, one devlink instance per SF is created. There might be
thousands of these on a single host. When a user needs to know port
handle for specific SF, he needs to dump all devlink ports on the host
which does not scale good.

Solution:

Allow user to pass devlink handle alongside the dump command
and dump only objects which are under selected devlink instance.

Introduce new attr DEVLINK_ATTR_DUMP_SELECTOR to nest the selection
attributes. This way the userspace can use maxattr to tell if dump
selector is supported by kernel or not.

Assemble netlink policy for selector attribute. If user passes attr
unknown to kernel, netlink validation errors out.

Example:
$ devlink port show
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

$ devlink port show auxiliary/mlx5_core.eth.0
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false

$ devlink port show auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

This is done in patch #10

Dependency:

The DEVLINK_ATTR_DUMP_SELECTOR parsing is very suitable to be done
once at the beginning of the dumping. Unfortunatelly, it is not possible
to define start() and done() callbacks for netlink small ops.
So all commands that use instance iterator for dumpit are converted to
split ops. This is done in patch #1-9

Extension:

patch #11 extends the selector by port index for health reporter
dumping.

v1->v2:
- the original single patch (patch #10) was extended to a patchset

Jiri Pirko (11):
  devlink: parse linecard attr in doit() callbacks
  devlink: parse rate attrs in doit() callbacks
  devlink: introduce __devlink_nl_pre_doit() with internal flags as
    function arg
  devlink: convert port get command to split ops
  devlink: convert health reporter get command to split ops
  devlink: convert param get command to split ops
  devlink: convert trap get command to split ops
  devlink: introduce set of macros and use it for split ops definitions
  devlink: convert rest of the iterator dumpit commands to split ops
  devlink: introduce dump selector attr and use it for per-instance
    dumps
  devlink: extend health reporter dump selector by port index

 include/uapi/linux/devlink.h |   2 +
 net/devlink/devl_internal.h  |  42 +++---
 net/devlink/health.c         |  21 ++-
 net/devlink/leftover.c       | 211 ++++++++--------------------
 net/devlink/netlink.c        | 263 ++++++++++++++++++++++++++++++-----
 5 files changed, 333 insertions(+), 206 deletions(-)

-- 
2.41.0

[patch net-next v2 01/11] devlink: parse linecard attr in doit() callbacks

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

No need to give the linecards any special treatment in netlink attribute
parsing, as unlike for ports, there is only a couple of commands
benefiting from that.

Remove DEVLINK_NL_FLAG_NEED_LINECARD, make pre_doit() callback simpler
by moving the linecard attribute parsing to linecard_[gs]et_doit() ops.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  7 -------
 net/devlink/leftover.c      | 19 +++++++++++++------
 net/devlink/netlink.c       |  8 --------
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 62921b2eb0d3..44b3a69c448e 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -92,7 +92,6 @@ static inline bool devl_is_registered(struct devlink *devlink)
 #define DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT	BIT(1)
 #define DEVLINK_NL_FLAG_NEED_RATE		BIT(2)
 #define DEVLINK_NL_FLAG_NEED_RATE_NODE		BIT(3)
-#define DEVLINK_NL_FLAG_NEED_LINECARD		BIT(4)
 
 enum devlink_multicast_groups {
 	DEVLINK_MCGRP_CONFIG,
@@ -199,12 +198,6 @@ int devlink_resources_validate(struct devlink *devlink,
 			       struct devlink_resource *resource,
 			       struct genl_info *info);
 
-/* Line cards */
-struct devlink_linecard;
-
-struct devlink_linecard *
-devlink_linecard_get_from_info(struct devlink *devlink, struct genl_info *info);
-
 /* Rates */
 int devlink_rate_nodes_check(struct devlink *devlink, u16 mode,
 			     struct netlink_ext_ack *extack);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 5128b9c7eea8..dba58830ed28 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -285,7 +285,7 @@ devlink_linecard_get_from_attrs(struct devlink *devlink, struct nlattr **attrs)
 	return ERR_PTR(-EINVAL);
 }
 
-struct devlink_linecard *
+static struct devlink_linecard *
 devlink_linecard_get_from_info(struct devlink *devlink, struct genl_info *info)
 {
 	return devlink_linecard_get_from_attrs(devlink, info->attrs);
@@ -1814,11 +1814,15 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
 static int devlink_nl_cmd_linecard_get_doit(struct sk_buff *skb,
 					    struct genl_info *info)
 {
-	struct devlink_linecard *linecard = info->user_ptr[1];
-	struct devlink *devlink = linecard->devlink;
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_linecard *linecard;
 	struct sk_buff *msg;
 	int err;
 
+	linecard = devlink_linecard_get_from_info(devlink, info);
+	if (IS_ERR(linecard))
+		return PTR_ERR(linecard);
+
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
@@ -2008,10 +2012,15 @@ static int devlink_linecard_type_unset(struct devlink_linecard *linecard,
 static int devlink_nl_cmd_linecard_set_doit(struct sk_buff *skb,
 					    struct genl_info *info)
 {
-	struct devlink_linecard *linecard = info->user_ptr[1];
 	struct netlink_ext_ack *extack = info->extack;
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_linecard *linecard;
 	int err;
 
+	linecard = devlink_linecard_get_from_info(devlink, info);
+	if (IS_ERR(linecard))
+		return PTR_ERR(linecard);
+
 	if (info->attrs[DEVLINK_ATTR_LINECARD_TYPE]) {
 		const char *type;
 
@@ -6354,14 +6363,12 @@ const struct genl_small_ops devlink_nl_ops[56] = {
 		.cmd = DEVLINK_CMD_LINECARD_GET,
 		.doit = devlink_nl_cmd_linecard_get_doit,
 		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_LINECARD,
 		/* can be retrieved by unprivileged users */
 	},
 	{
 		.cmd = DEVLINK_CMD_LINECARD_SET,
 		.doit = devlink_nl_cmd_linecard_set_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_LINECARD,
 	},
 	{
 		.cmd = DEVLINK_CMD_SB_GET,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 7a332eb70f70..cd2754698478 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -112,7 +112,6 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
 static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 			       struct sk_buff *skb, struct genl_info *info)
 {
-	struct devlink_linecard *linecard;
 	struct devlink_port *devlink_port;
 	struct devlink *devlink;
 	int err;
@@ -151,13 +150,6 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 			goto unlock;
 		}
 		info->user_ptr[1] = rate_node;
-	} else if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_LINECARD) {
-		linecard = devlink_linecard_get_from_info(devlink, info);
-		if (IS_ERR(linecard)) {
-			err = PTR_ERR(linecard);
-			goto unlock;
-		}
-		info->user_ptr[1] = linecard;
 	}
 	return 0;
 
-- 
2.41.0

[patch net-next v2 02/11] devlink: parse rate attrs in doit() callbacks

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

No need to give the rate any special treatment in netlink attributes
parsing, as unlike for ports, there is only a couple of commands
benefiting from that.

Remove DEVLINK_NL_FLAG_NEED_RATE*, make pre_doit() callback simpler
by moving the rate attributes parsing to rate_*_doit() ops.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  8 +-------
 net/devlink/leftover.c      | 37 ++++++++++++++++++++++++-------------
 net/devlink/netlink.c       | 18 ------------------
 3 files changed, 25 insertions(+), 38 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 44b3a69c448e..f6e466be2310 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -90,8 +90,6 @@ static inline bool devl_is_registered(struct devlink *devlink)
 /* Netlink */
 #define DEVLINK_NL_FLAG_NEED_PORT		BIT(0)
 #define DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT	BIT(1)
-#define DEVLINK_NL_FLAG_NEED_RATE		BIT(2)
-#define DEVLINK_NL_FLAG_NEED_RATE_NODE		BIT(3)
 
 enum devlink_multicast_groups {
 	DEVLINK_MCGRP_CONFIG,
@@ -201,11 +199,7 @@ int devlink_resources_validate(struct devlink *devlink,
 /* Rates */
 int devlink_rate_nodes_check(struct devlink *devlink, u16 mode,
 			     struct netlink_ext_ack *extack);
-struct devlink_rate *
-devlink_rate_get_from_info(struct devlink *devlink, struct genl_info *info);
-struct devlink_rate *
-devlink_rate_node_get_from_info(struct devlink *devlink,
-				struct genl_info *info);
+
 /* Devlink nl cmds */
 int devlink_nl_cmd_get_doit(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index dba58830ed28..2f7130c60333 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -232,13 +232,13 @@ devlink_rate_node_get_from_attrs(struct devlink *devlink, struct nlattr **attrs)
 	return devlink_rate_node_get_by_name(devlink, rate_node_name);
 }
 
-struct devlink_rate *
+static struct devlink_rate *
 devlink_rate_node_get_from_info(struct devlink *devlink, struct genl_info *info)
 {
 	return devlink_rate_node_get_from_attrs(devlink, info->attrs);
 }
 
-struct devlink_rate *
+static struct devlink_rate *
 devlink_rate_get_from_info(struct devlink *devlink, struct genl_info *info)
 {
 	struct nlattr **attrs = info->attrs;
@@ -1041,10 +1041,15 @@ const struct devlink_cmd devl_cmd_rate_get = {
 static int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb,
 					struct genl_info *info)
 {
-	struct devlink_rate *devlink_rate = info->user_ptr[1];
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_rate *devlink_rate;
 	struct sk_buff *msg;
 	int err;
 
+	devlink_rate = devlink_rate_get_from_info(devlink, info);
+	if (IS_ERR(devlink_rate))
+		return PTR_ERR(devlink_rate);
+
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
@@ -1629,11 +1634,16 @@ static bool devlink_rate_set_ops_supported(const struct devlink_ops *ops,
 static int devlink_nl_cmd_rate_set_doit(struct sk_buff *skb,
 					struct genl_info *info)
 {
-	struct devlink_rate *devlink_rate = info->user_ptr[1];
-	struct devlink *devlink = devlink_rate->devlink;
-	const struct devlink_ops *ops = devlink->ops;
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_rate *devlink_rate;
+	const struct devlink_ops *ops;
 	int err;
 
+	devlink_rate = devlink_rate_get_from_info(devlink, info);
+	if (IS_ERR(devlink_rate))
+		return PTR_ERR(devlink_rate);
+
+	ops = devlink->ops;
 	if (!ops || !devlink_rate_set_ops_supported(ops, info, devlink_rate->type))
 		return -EOPNOTSUPP;
 
@@ -1704,18 +1714,22 @@ static int devlink_nl_cmd_rate_new_doit(struct sk_buff *skb,
 static int devlink_nl_cmd_rate_del_doit(struct sk_buff *skb,
 					struct genl_info *info)
 {
-	struct devlink_rate *rate_node = info->user_ptr[1];
-	struct devlink *devlink = rate_node->devlink;
-	const struct devlink_ops *ops = devlink->ops;
+	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_rate *rate_node;
 	int err;
 
+	rate_node = devlink_rate_node_get_from_info(devlink, info);
+	if (IS_ERR(rate_node))
+		return PTR_ERR(rate_node);
+
 	if (refcount_read(&rate_node->refcnt) > 1) {
 		NL_SET_ERR_MSG(info->extack, "Node has children. Cannot delete node.");
 		return -EBUSY;
 	}
 
 	devlink_rate_notify(rate_node, DEVLINK_CMD_RATE_DEL);
-	err = ops->rate_node_del(rate_node, rate_node->priv, info->extack);
+	err = devlink->ops->rate_node_del(rate_node, rate_node->priv,
+					  info->extack);
 	if (rate_node->parent)
 		refcount_dec(&rate_node->parent->refcnt);
 	list_del(&rate_node->list);
@@ -6314,14 +6328,12 @@ const struct genl_small_ops devlink_nl_ops[56] = {
 		.cmd = DEVLINK_CMD_RATE_GET,
 		.doit = devlink_nl_cmd_rate_get_doit,
 		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
 		/* can be retrieved by unprivileged users */
 	},
 	{
 		.cmd = DEVLINK_CMD_RATE_SET,
 		.doit = devlink_nl_cmd_rate_set_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE,
 	},
 	{
 		.cmd = DEVLINK_CMD_RATE_NEW,
@@ -6332,7 +6344,6 @@ const struct genl_small_ops devlink_nl_ops[56] = {
 		.cmd = DEVLINK_CMD_RATE_DEL,
 		.doit = devlink_nl_cmd_rate_del_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_RATE_NODE,
 	},
 	{
 		.cmd = DEVLINK_CMD_PORT_SPLIT,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index cd2754698478..336f375f9ff6 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -132,24 +132,6 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 		devlink_port = devlink_port_get_from_info(devlink, info);
 		if (!IS_ERR(devlink_port))
 			info->user_ptr[1] = devlink_port;
-	} else if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_RATE) {
-		struct devlink_rate *devlink_rate;
-
-		devlink_rate = devlink_rate_get_from_info(devlink, info);
-		if (IS_ERR(devlink_rate)) {
-			err = PTR_ERR(devlink_rate);
-			goto unlock;
-		}
-		info->user_ptr[1] = devlink_rate;
-	} else if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_RATE_NODE) {
-		struct devlink_rate *rate_node;
-
-		rate_node = devlink_rate_node_get_from_info(devlink, info);
-		if (IS_ERR(rate_node)) {
-			err = PTR_ERR(rate_node);
-			goto unlock;
-		}
-		info->user_ptr[1] = rate_node;
 	}
 	return 0;
 
-- 
2.41.0

[patch net-next v2 03/11] devlink: introduce __devlink_nl_pre_doit() with internal flags as function arg

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

To be able define pre_doit() helpers what don't rely on internal_flags
in the follow-up patches, have __devlink_nl_pre_doit() to accept the
flags as a function arg and make devlink_nl_pre_doit() a wrapper helper
function calling it.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/netlink.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 336f375f9ff6..f1a5ba0f6deb 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -109,8 +109,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
 	return ERR_PTR(-ENODEV);
 }
 
-static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
-			       struct sk_buff *skb, struct genl_info *info)
+static int __devlink_nl_pre_doit(struct sk_buff *skb, struct genl_info *info,
+				 u8 flags)
 {
 	struct devlink_port *devlink_port;
 	struct devlink *devlink;
@@ -121,14 +121,14 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 		return PTR_ERR(devlink);
 
 	info->user_ptr[0] = devlink;
-	if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_PORT) {
+	if (flags & DEVLINK_NL_FLAG_NEED_PORT) {
 		devlink_port = devlink_port_get_from_info(devlink, info);
 		if (IS_ERR(devlink_port)) {
 			err = PTR_ERR(devlink_port);
 			goto unlock;
 		}
 		info->user_ptr[1] = devlink_port;
-	} else if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT) {
+	} else if (flags & DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT) {
 		devlink_port = devlink_port_get_from_info(devlink, info);
 		if (!IS_ERR(devlink_port))
 			info->user_ptr[1] = devlink_port;
@@ -141,6 +141,12 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 	return err;
 }
 
+static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
+			       struct sk_buff *skb, struct genl_info *info)
+{
+	return __devlink_nl_pre_doit(skb, info, ops->internal_flags);
+}
+
 static void devlink_nl_post_doit(const struct genl_split_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
 {
-- 
2.41.0

[patch net-next v2 04/11] devlink: convert port get command to split ops

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Do the conversion of port get command to split ops. Introduce
devlink_nl_pre_doit_port() helper to indicate port object attribute
parsing and use it as pre_doit() callback.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  4 +++-
 net/devlink/leftover.c      | 14 +++-----------
 net/devlink/netlink.c       | 29 +++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index f6e466be2310..2870be150ee3 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -116,7 +116,7 @@ struct devlink_cmd {
 			struct netlink_callback *cb);
 };
 
-extern const struct genl_small_ops devlink_nl_ops[56];
+extern const struct genl_small_ops devlink_nl_ops[55];
 
 struct devlink *
 devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
@@ -209,6 +209,8 @@ int devlink_nl_cmd_info_get_doit(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_flash_update(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_selftests_get_doit(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_selftests_run(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_cmd_port_get_doit(struct sk_buff *skb,
+				 struct genl_info *info);
 int devlink_nl_cmd_health_reporter_get_doit(struct sk_buff *skb,
 					    struct genl_info *info);
 int devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 2f7130c60333..33f71e8fe8ee 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -1077,8 +1077,8 @@ devlink_rate_is_parent_node(struct devlink_rate *devlink_rate,
 	return false;
 }
 
-static int devlink_nl_cmd_port_get_doit(struct sk_buff *skb,
-					struct genl_info *info)
+int devlink_nl_cmd_port_get_doit(struct sk_buff *skb,
+				 struct genl_info *info)
 {
 	struct devlink_port *devlink_port = info->user_ptr[1];
 	struct sk_buff *msg;
@@ -6301,7 +6301,7 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	return devlink_trap_policer_set(devlink, policer_item, info);
 }
 
-const struct genl_small_ops devlink_nl_ops[56] = {
+const struct genl_small_ops devlink_nl_ops[55] = {
 	{
 		.cmd = DEVLINK_CMD_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6309,14 +6309,6 @@ const struct genl_small_ops devlink_nl_ops[56] = {
 		.dumpit = devlink_nl_instance_iter_dumpit,
 		/* can be retrieved by unprivileged users */
 	},
-	{
-		.cmd = DEVLINK_CMD_PORT_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_port_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_PORT_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index f1a5ba0f6deb..cd35fa637846 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -147,6 +147,12 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 	return __devlink_nl_pre_doit(skb, info, ops->internal_flags);
 }
 
+static int devlink_nl_pre_doit_port(const struct genl_split_ops *ops,
+				    struct sk_buff *skb, struct genl_info *info)
+{
+	return __devlink_nl_pre_doit(skb, info, DEVLINK_NL_FLAG_NEED_PORT);
+}
+
 static void devlink_nl_post_doit(const struct genl_split_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
 {
@@ -213,6 +219,27 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+static const struct genl_split_ops devlink_nl_split_ops[] = {
+	{
+		.cmd = DEVLINK_CMD_PORT_GET,
+		.pre_doit = devlink_nl_pre_doit_port,
+		.doit = devlink_nl_cmd_port_get_doit,
+		.post_doit = devlink_nl_post_doit,
+		.flags = GENL_CMD_CAP_DO,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
+	{
+		.cmd = DEVLINK_CMD_PORT_GET,
+		.dumpit = devlink_nl_instance_iter_dumpit,
+		.flags = GENL_CMD_CAP_DUMP,
+		.validate = GENL_DONT_VALIDATE_DUMP,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
+};
+
 struct genl_family devlink_nl_family __ro_after_init = {
 	.name		= DEVLINK_GENL_NAME,
 	.version	= DEVLINK_GENL_VERSION,
@@ -225,6 +252,8 @@ struct genl_family devlink_nl_family __ro_after_init = {
 	.module		= THIS_MODULE,
 	.small_ops	= devlink_nl_ops,
 	.n_small_ops	= ARRAY_SIZE(devlink_nl_ops),
+	.split_ops	= devlink_nl_split_ops,
+	.n_split_ops	= ARRAY_SIZE(devlink_nl_split_ops),
 	.resv_start_op	= DEVLINK_CMD_SELFTESTS_RUN + 1,
 	.mcgrps		= devlink_nl_mcgrps,
 	.n_mcgrps	= ARRAY_SIZE(devlink_nl_mcgrps),
-- 
2.41.0

[patch net-next v2 05/11] devlink: convert health reporter get command to split ops

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Do the conversion of health reporter get command to split ops.
Introduce devlink_nl_pre_doit_port_optional() helper to indicate
optional port object attribute parsing and use it
as pre_doit() callback.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  2 +-
 net/devlink/leftover.c      | 10 +---------
 net/devlink/netlink.c       | 25 +++++++++++++++++++++++++
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 2870be150ee3..948b112c1328 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -116,7 +116,7 @@ struct devlink_cmd {
 			struct netlink_callback *cb);
 };
 
-extern const struct genl_small_ops devlink_nl_ops[55];
+extern const struct genl_small_ops devlink_nl_ops[54];
 
 struct devlink *
 devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 33f71e8fe8ee..262f3e49e87b 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -6301,7 +6301,7 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	return devlink_trap_policer_set(devlink, policer_item, info);
 }
 
-const struct genl_small_ops devlink_nl_ops[55] = {
+const struct genl_small_ops devlink_nl_ops[54] = {
 	{
 		.cmd = DEVLINK_CMD_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6550,14 +6550,6 @@ const struct genl_small_ops devlink_nl_ops[55] = {
 		.dumpit = devlink_nl_instance_iter_dumpit,
 		/* can be retrieved by unprivileged users */
 	},
-	{
-		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_health_reporter_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index cd35fa637846..794370e4d04a 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -153,6 +153,13 @@ static int devlink_nl_pre_doit_port(const struct genl_split_ops *ops,
 	return __devlink_nl_pre_doit(skb, info, DEVLINK_NL_FLAG_NEED_PORT);
 }
 
+static int devlink_nl_pre_doit_port_optional(const struct genl_split_ops *ops,
+					     struct sk_buff *skb,
+					     struct genl_info *info)
+{
+	return __devlink_nl_pre_doit(skb, info, DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT);
+}
+
 static void devlink_nl_post_doit(const struct genl_split_ops *ops,
 				 struct sk_buff *skb, struct genl_info *info)
 {
@@ -238,6 +245,24 @@ static const struct genl_split_ops devlink_nl_split_ops[] = {
 		.maxattr = DEVLINK_ATTR_MAX,
 		.policy	= devlink_nl_policy,
 	},
+	{
+		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
+		.pre_doit = devlink_nl_pre_doit_port_optional,
+		.doit = devlink_nl_cmd_health_reporter_get_doit,
+		.post_doit = devlink_nl_post_doit,
+		.flags = GENL_CMD_CAP_DO,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
+	{
+		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
+		.dumpit = devlink_nl_instance_iter_dumpit,
+		.flags = GENL_CMD_CAP_DUMP,
+		.validate = GENL_DONT_VALIDATE_DUMP,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
 };
 
 struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.41.0

[patch net-next v2 06/11] devlink: convert param get command to split ops

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Do the conversion of param get command to split ops. Introduce
devlink_nl_pre_doit_simple() helper to indicate just devlink handle
attributes parsing and use it as pre_doit() callback.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  4 +++-
 net/devlink/leftover.c      | 13 +++----------
 net/devlink/netlink.c       | 24 ++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 948b112c1328..5ba3f066b3b2 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -116,7 +116,7 @@ struct devlink_cmd {
 			struct netlink_callback *cb);
 };
 
-extern const struct genl_small_ops devlink_nl_ops[54];
+extern const struct genl_small_ops devlink_nl_ops[53];
 
 struct devlink *
 devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
@@ -211,6 +211,8 @@ int devlink_nl_cmd_selftests_get_doit(struct sk_buff *skb, struct genl_info *inf
 int devlink_nl_cmd_selftests_run(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_port_get_doit(struct sk_buff *skb,
 				 struct genl_info *info);
+int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
+				  struct genl_info *info);
 int devlink_nl_cmd_health_reporter_get_doit(struct sk_buff *skb,
 					    struct genl_info *info);
 int devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 262f3e49e87b..8729dd740171 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4295,8 +4295,8 @@ devlink_param_get_from_info(struct xarray *params, struct genl_info *info)
 	return devlink_param_find_by_name(params, param_name);
 }
 
-static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
-					 struct genl_info *info)
+int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
+				  struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_param_item *param_item;
@@ -6301,7 +6301,7 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	return devlink_trap_policer_set(devlink, policer_item, info);
 }
 
-const struct genl_small_ops devlink_nl_ops[54] = {
+const struct genl_small_ops devlink_nl_ops[53] = {
 	{
 		.cmd = DEVLINK_CMD_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6489,13 +6489,6 @@ const struct genl_small_ops devlink_nl_ops[54] = {
 		.doit = devlink_nl_cmd_reload,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_PARAM_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_param_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_PARAM_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 794370e4d04a..bd128584e8c8 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -147,6 +147,12 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 	return __devlink_nl_pre_doit(skb, info, ops->internal_flags);
 }
 
+static int devlink_nl_pre_doit_simple(const struct genl_split_ops *ops,
+				      struct sk_buff *skb, struct genl_info *info)
+{
+	return __devlink_nl_pre_doit(skb, info, 0);
+}
+
 static int devlink_nl_pre_doit_port(const struct genl_split_ops *ops,
 				    struct sk_buff *skb, struct genl_info *info)
 {
@@ -245,6 +251,24 @@ static const struct genl_split_ops devlink_nl_split_ops[] = {
 		.maxattr = DEVLINK_ATTR_MAX,
 		.policy	= devlink_nl_policy,
 	},
+	{
+		.cmd = DEVLINK_CMD_PARAM_GET,
+		.pre_doit = devlink_nl_pre_doit_simple,
+		.doit = devlink_nl_cmd_param_get_doit,
+		.post_doit = devlink_nl_post_doit,
+		.flags = GENL_CMD_CAP_DO,
+		.validate = GENL_DONT_VALIDATE_STRICT,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
+	{
+		.cmd = DEVLINK_CMD_PARAM_GET,
+		.dumpit = devlink_nl_instance_iter_dumpit,
+		.flags = GENL_CMD_CAP_DUMP,
+		.validate = GENL_DONT_VALIDATE_DUMP,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
 		.pre_doit = devlink_nl_pre_doit_port_optional,
-- 
2.41.0

[patch net-next v2 07/11] devlink: convert trap get command to split ops

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Do the conversion of trap get command to split ops.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  3 ++-
 net/devlink/leftover.c      | 11 ++---------
 net/devlink/netlink.c       | 16 ++++++++++++++++
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 5ba3f066b3b2..dc655d5797a8 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -116,7 +116,7 @@ struct devlink_cmd {
 			struct netlink_callback *cb);
 };
 
-extern const struct genl_small_ops devlink_nl_ops[53];
+extern const struct genl_small_ops devlink_nl_ops[52];
 
 struct devlink *
 devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
@@ -227,3 +227,4 @@ int devlink_nl_cmd_health_reporter_dump_clear_doit(struct sk_buff *skb,
 						   struct genl_info *info);
 int devlink_nl_cmd_health_reporter_test_doit(struct sk_buff *skb,
 					     struct genl_info *info);
+int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 8729dd740171..3dbba1906ee6 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -5655,8 +5655,7 @@ static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb,
-					struct genl_info *info)
+int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
@@ -6301,7 +6300,7 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	return devlink_trap_policer_set(devlink, policer_item, info);
 }
 
-const struct genl_small_ops devlink_nl_ops[53] = {
+const struct genl_small_ops devlink_nl_ops[52] = {
 	{
 		.cmd = DEVLINK_CMD_GET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6591,12 +6590,6 @@ const struct genl_small_ops devlink_nl_ops[53] = {
 		.doit = devlink_nl_cmd_flash_update,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GET,
-		.doit = devlink_nl_cmd_trap_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_SET,
 		.doit = devlink_nl_cmd_trap_set_doit,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index bd128584e8c8..cabebff6e7a7 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -287,6 +287,22 @@ static const struct genl_split_ops devlink_nl_split_ops[] = {
 		.maxattr = DEVLINK_ATTR_MAX,
 		.policy	= devlink_nl_policy,
 	},
+	{
+		.cmd = DEVLINK_CMD_TRAP_GET,
+		.pre_doit = devlink_nl_pre_doit_simple,
+		.doit = devlink_nl_cmd_trap_get_doit,
+		.post_doit = devlink_nl_post_doit,
+		.flags = GENL_CMD_CAP_DO,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
+	{
+		.cmd = DEVLINK_CMD_TRAP_GET,
+		.dumpit = devlink_nl_instance_iter_dumpit,
+		.flags = GENL_CMD_CAP_DUMP,
+		.maxattr = DEVLINK_ATTR_MAX,
+		.policy	= devlink_nl_policy,
+	},
 };
 
 struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.41.0

[patch net-next v2 08/11] devlink: introduce set of macros and use it for split ops definitions

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The split ops structures for all commands look pretty much the same.
The are all using the same/similar callbacks.

Introduce a set of macros to make the code shorter and also avoid
possible future copy&paste mistakes and inconsistencies.

Use this macros for already converted commands.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/netlink.c | 136 ++++++++++++++++++++----------------------
 1 file changed, 66 insertions(+), 70 deletions(-)

diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index cabebff6e7a7..3dae9303cfa7 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -232,77 +232,73 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+#define __DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, _validate,	\
+			_maxattr, _policy)					\
+	{									\
+		.cmd = DEVLINK_CMD_##cmd_subname,				\
+		.pre_doit = devlink_nl_pre_doit_##pre_doit_suffix,		\
+		.doit = devlink_nl_cmd_##doit_subname##_doit,			\
+		.post_doit = devlink_nl_post_doit,				\
+		.flags = GENL_CMD_CAP_DO,					\
+		.validate = _validate,						\
+		.maxattr = _maxattr,						\
+		.policy	= _policy,						\
+	}
+
+#define __DEVL_NL_OP_DUMP(cmd_subname, _validate, _maxattr, _policy)		\
+	{									\
+		.cmd = DEVLINK_CMD_##cmd_subname,				\
+		.dumpit = devlink_nl_instance_iter_dumpit,			\
+		.flags = GENL_CMD_CAP_DUMP,					\
+		.validate = _validate,						\
+		.maxattr = _maxattr,						\
+		.policy	= _policy,						\
+	}
+
+#define __DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix,	\
+			       validate)					\
+	__DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, validate,	\
+			DEVLINK_ATTR_MAX, devlink_nl_policy)
+
+#define __DEVL_NL_OP_LEGACY_DUMP(cmd_subname, validate)				\
+	__DEVL_NL_OP_DUMP(cmd_subname, validate,				\
+			  DEVLINK_ATTR_MAX, devlink_nl_policy)
+
+#define DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix)	\
+	__DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix,	\
+			       GENL_DONT_VALIDATE_STRICT)
+
+#define DEVL_NL_OP_LEGACY_DUMP(cmd_subname)					\
+	__DEVL_NL_OP_LEGACY_DUMP(cmd_subname, GENL_DONT_VALIDATE_DUMP_STRICT)
+
+#define DEVL_NL_OP_LEGACY_STRICT_DO(cmd_subname, doit_subname, pre_doit_suffix)	\
+	__DEVL_NL_OP_LEGACY_DO(cmd_subname, doit_subname, pre_doit_suffix, 0)
+
+#define DEVL_NL_OP_LEGACY_STRICT_DUMP(cmd_subname)				\
+	__DEVL_NL_OP_LEGACY_DUMP(cmd_subname, 0)
+
+#define DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix,		\
+		      maxattr, policy)						\
+	__DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, 0,		\
+			maxattr, policy)
+
+#define DEVL_NL_OP_DUMP(cmd_subname, maxattr, policy)			\
+	__DEVL_NL_OP_DUMP(cmd_subname, 0, maxattr, policy)
+
 static const struct genl_split_ops devlink_nl_split_ops[] = {
-	{
-		.cmd = DEVLINK_CMD_PORT_GET,
-		.pre_doit = devlink_nl_pre_doit_port,
-		.doit = devlink_nl_cmd_port_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PORT_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PARAM_GET,
-		.pre_doit = devlink_nl_pre_doit_simple,
-		.doit = devlink_nl_cmd_param_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_PARAM_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
-		.pre_doit = devlink_nl_pre_doit_port_optional,
-		.doit = devlink_nl_cmd_health_reporter_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.validate = GENL_DONT_VALIDATE_STRICT,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_HEALTH_REPORTER_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.validate = GENL_DONT_VALIDATE_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GET,
-		.pre_doit = devlink_nl_pre_doit_simple,
-		.doit = devlink_nl_cmd_trap_get_doit,
-		.post_doit = devlink_nl_post_doit,
-		.flags = GENL_CMD_CAP_DO,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GET,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_CMD_CAP_DUMP,
-		.maxattr = DEVLINK_ATTR_MAX,
-		.policy	= devlink_nl_policy,
-	},
+	DEVL_NL_OP_LEGACY_DO(PORT_GET, port_get, port),
+	DEVL_NL_OP_LEGACY_DUMP(PORT_GET),
+	DEVL_NL_OP_LEGACY_DO(PARAM_GET, param_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(PARAM_GET),
+	DEVL_NL_OP_LEGACY_DO(HEALTH_REPORTER_GET, health_reporter_get,
+			     port_optional),
+	DEVL_NL_OP_LEGACY_DUMP(HEALTH_REPORTER_GET),
+	DEVL_NL_OP_LEGACY_STRICT_DO(TRAP_GET, trap_get, simple),
+	DEVL_NL_OP_LEGACY_STRICT_DUMP(TRAP_GET),
+	/* For every newly added command put above this line the set of macros
+	 * DEVL_NL_OP_DO and DEVL_NL_OP_DUMP should be used. Note that
+	 * there is an exception with non-iterator dump implementation.
+	 */
 };
 
 struct genl_family devlink_nl_family __ro_after_init = {
-- 
2.41.0

Re: [patch net-next v2 08/11] devlink: introduce set of macros and use it for split ops definitions

[Jakub Kicinski]

On Thu, 20 Jul 2023 14:18:26 +0200 Jiri Pirko wrote:
> The split ops structures for all commands look pretty much the same.
> The are all using the same/similar callbacks.
> 
> Introduce a set of macros to make the code shorter and also avoid
> possible future copy&paste mistakes and inconsistencies.
> 
> Use this macros for already converted commands.

If you want to use split ops extensively please use the nlspec
and generate the table automatically. Integrating closer with
the spec will have many benefits.

Re: [patch net-next v2 08/11] devlink: introduce set of macros and use it for split ops definitions

[Jiri Pirko]

Tue, Jul 25, 2023 at 07:38:16PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:26 +0200 Jiri Pirko wrote:
>> The split ops structures for all commands look pretty much the same.
>> The are all using the same/similar callbacks.
>> 
>> Introduce a set of macros to make the code shorter and also avoid
>> possible future copy&paste mistakes and inconsistencies.
>> 
>> Use this macros for already converted commands.
>
>If you want to use split ops extensively please use the nlspec
>and generate the table automatically. Integrating closer with
>the spec will have many benefits.

Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
check that out.

Btw, does that mean that any split-ops usage would require generated
code? If yes, could you please document that somewhere, probably near
the struct?

Thanks!

Re: [patch net-next v2 08/11] devlink: introduce set of macros and use it for split ops definitions

[Jakub Kicinski]

On Mon, 31 Jul 2023 14:21:52 +0200 Jiri Pirko wrote:
> >If you want to use split ops extensively please use the nlspec
> >and generate the table automatically. Integrating closer with
> >the spec will have many benefits.  
> 
> Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
> check that out.
> 
> Btw, does that mean that any split-ops usage would require generated
> code? If yes, could you please document that somewhere, probably near
> the struct?

I wrote it somewhere, probably the commit messages for the split ops.
The tools are not 100% ready for partial generation I don't want to
force everyone to do code gen. But the homegrown macros in every family
are a no go.

Re: [patch net-next v2 08/11] devlink: introduce set of macros and use it for split ops definitions

[Jiri Pirko]

Mon, Jul 31, 2023 at 06:57:45PM CEST, kuba@kernel.org wrote:
>On Mon, 31 Jul 2023 14:21:52 +0200 Jiri Pirko wrote:
>> >If you want to use split ops extensively please use the nlspec
>> >and generate the table automatically. Integrating closer with
>> >the spec will have many benefits.  
>> 
>> Yeah, I was thinging about it, it just didn't seem necessary. Okay, will
>> check that out.
>> 
>> Btw, does that mean that any split-ops usage would require generated
>> code? If yes, could you please document that somewhere, probably near
>> the struct?
>
>I wrote it somewhere, probably the commit messages for the split ops.

I believe we need to have it written down in actual codebase.


>The tools are not 100% ready for partial generation I don't want to
>force everyone to do code gen. But the homegrown macros in every family
>are a no go.

So you say that if I spell it out without macros, that would be okay?

[patch net-next v2 09/11] devlink: convert rest of the iterator dumpit commands to split ops

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In a similar fashion couple of example commands were converted
previously, do the conversion of the rest of the commands that use
devlink_nl_instance_iter_dumpit() callback.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  19 ++++--
 net/devlink/leftover.c      | 115 +++++-------------------------------
 net/devlink/netlink.c       |  28 ++++++++-
 3 files changed, 56 insertions(+), 106 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index dc655d5797a8..79614b45e8ac 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -116,7 +116,7 @@ struct devlink_cmd {
 			struct netlink_callback *cb);
 };
 
-extern const struct genl_small_ops devlink_nl_ops[52];
+extern const struct genl_small_ops devlink_nl_ops[40];
 
 struct devlink *
 devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
@@ -124,9 +124,6 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
 void devlink_notify_unregister(struct devlink *devlink);
 void devlink_notify_register(struct devlink *devlink);
 
-int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
-				    struct netlink_callback *cb);
-
 static inline struct devlink_nl_dump_state *
 devlink_dump_state(struct netlink_callback *cb)
 {
@@ -211,8 +208,15 @@ int devlink_nl_cmd_selftests_get_doit(struct sk_buff *skb, struct genl_info *inf
 int devlink_nl_cmd_selftests_run(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_port_get_doit(struct sk_buff *skb,
 				 struct genl_info *info);
+int devlink_nl_cmd_sb_get_doit(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_cmd_sb_pool_get_doit(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,
+					 struct genl_info *info);
+int devlink_nl_cmd_sb_tc_pool_bind_get_doit(struct sk_buff *skb,
+					    struct genl_info *info);
 int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
 				  struct genl_info *info);
+int devlink_nl_cmd_region_get_doit(struct sk_buff *skb, struct genl_info *info);
 int devlink_nl_cmd_health_reporter_get_doit(struct sk_buff *skb,
 					    struct genl_info *info);
 int devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
@@ -227,4 +231,11 @@ int devlink_nl_cmd_health_reporter_dump_clear_doit(struct sk_buff *skb,
 						   struct genl_info *info);
 int devlink_nl_cmd_health_reporter_test_doit(struct sk_buff *skb,
 					     struct genl_info *info);
+int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_cmd_linecard_get_doit(struct sk_buff *skb,
+				     struct genl_info *info);
 int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb, struct genl_info *info);
+int devlink_nl_cmd_trap_group_get_doit(struct sk_buff *skb,
+				       struct genl_info *info);
+int devlink_nl_cmd_trap_policer_get_doit(struct sk_buff *skb,
+					 struct genl_info *info);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 3dbba1906ee6..58ea1b9cc708 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -1038,8 +1038,7 @@ const struct devlink_cmd devl_cmd_rate_get = {
 	.dump_one		= devlink_nl_cmd_rate_get_dump_one,
 };
 
-static int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb,
-					struct genl_info *info)
+int devlink_nl_cmd_rate_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_rate *devlink_rate;
@@ -1825,8 +1824,8 @@ static void devlink_linecard_notify(struct devlink_linecard *linecard,
 				msg, 0, DEVLINK_MCGRP_CONFIG, GFP_KERNEL);
 }
 
-static int devlink_nl_cmd_linecard_get_doit(struct sk_buff *skb,
-					    struct genl_info *info)
+int devlink_nl_cmd_linecard_get_doit(struct sk_buff *skb,
+				     struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_linecard *linecard;
@@ -2091,8 +2090,7 @@ static int devlink_nl_sb_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_sb_get_doit(struct sk_buff *skb,
-				      struct genl_info *info)
+int devlink_nl_cmd_sb_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_sb *devlink_sb;
@@ -2194,8 +2192,7 @@ static int devlink_nl_sb_pool_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_sb_pool_get_doit(struct sk_buff *skb,
-					   struct genl_info *info)
+int devlink_nl_cmd_sb_pool_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_sb *devlink_sb;
@@ -2394,8 +2391,8 @@ static int devlink_nl_sb_port_pool_fill(struct sk_buff *msg,
 	return err;
 }
 
-static int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,
-						struct genl_info *info)
+int devlink_nl_cmd_sb_port_pool_get_doit(struct sk_buff *skb,
+					 struct genl_info *info)
 {
 	struct devlink_port *devlink_port = info->user_ptr[1];
 	struct devlink *devlink = devlink_port->devlink;
@@ -2603,8 +2600,8 @@ devlink_nl_sb_tc_pool_bind_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_sb_tc_pool_bind_get_doit(struct sk_buff *skb,
-						   struct genl_info *info)
+int devlink_nl_cmd_sb_tc_pool_bind_get_doit(struct sk_buff *skb,
+					    struct genl_info *info)
 {
 	struct devlink_port *devlink_port = info->user_ptr[1];
 	struct devlink *devlink = devlink_port->devlink;
@@ -4793,8 +4790,7 @@ static void devlink_region_snapshot_del(struct devlink_region *region,
 	kfree(snapshot);
 }
 
-static int devlink_nl_cmd_region_get_doit(struct sk_buff *skb,
-					  struct genl_info *info)
+int devlink_nl_cmd_region_get_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_port *port = NULL;
@@ -5865,8 +5861,8 @@ devlink_nl_trap_group_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_trap_group_get_doit(struct sk_buff *skb,
-					      struct genl_info *info)
+int devlink_nl_cmd_trap_group_get_doit(struct sk_buff *skb,
+				       struct genl_info *info)
 {
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
@@ -6159,8 +6155,8 @@ devlink_nl_trap_policer_fill(struct sk_buff *msg, struct devlink *devlink,
 	return -EMSGSIZE;
 }
 
-static int devlink_nl_cmd_trap_policer_get_doit(struct sk_buff *skb,
-						struct genl_info *info)
+int devlink_nl_cmd_trap_policer_get_doit(struct sk_buff *skb,
+					 struct genl_info *info)
 {
 	struct devlink_trap_policer_item *policer_item;
 	struct netlink_ext_ack *extack = info->extack;
@@ -6300,14 +6296,7 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	return devlink_trap_policer_set(devlink, policer_item, info);
 }
 
-const struct genl_small_ops devlink_nl_ops[52] = {
-	{
-		.cmd = DEVLINK_CMD_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
+const struct genl_small_ops devlink_nl_ops[40] = {
 	{
 		.cmd = DEVLINK_CMD_PORT_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6315,12 +6304,6 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
 	},
-	{
-		.cmd = DEVLINK_CMD_RATE_GET,
-		.doit = devlink_nl_cmd_rate_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_RATE_SET,
 		.doit = devlink_nl_cmd_rate_set_doit,
@@ -6361,45 +6344,17 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
 	},
-	{
-		.cmd = DEVLINK_CMD_LINECARD_GET,
-		.doit = devlink_nl_cmd_linecard_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_LINECARD_SET,
 		.doit = devlink_nl_cmd_linecard_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_SB_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_sb_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
-	{
-		.cmd = DEVLINK_CMD_SB_POOL_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_sb_pool_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_SB_POOL_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_sb_pool_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_SB_PORT_POOL_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_sb_port_pool_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_SB_PORT_POOL_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6407,14 +6362,6 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
 	},
-	{
-		.cmd = DEVLINK_CMD_SB_TC_POOL_BIND_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_sb_tc_pool_bind_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_SB_TC_POOL_BIND_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6509,13 +6456,6 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.flags = GENL_ADMIN_PERM,
 		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
 	},
-	{
-		.cmd = DEVLINK_CMD_REGION_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_region_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		.flags = GENL_ADMIN_PERM,
-	},
 	{
 		.cmd = DEVLINK_CMD_REGION_NEW,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6535,13 +6475,6 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.dumpit = devlink_nl_cmd_region_read_dumpit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_INFO_GET,
-		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
-		.doit = devlink_nl_cmd_info_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_HEALTH_REPORTER_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
@@ -6595,34 +6528,16 @@ const struct genl_small_ops devlink_nl_ops[52] = {
 		.doit = devlink_nl_cmd_trap_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_GROUP_GET,
-		.doit = devlink_nl_cmd_trap_group_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_GROUP_SET,
 		.doit = devlink_nl_cmd_trap_group_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_TRAP_POLICER_GET,
-		.doit = devlink_nl_cmd_trap_policer_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_POLICER_SET,
 		.doit = devlink_nl_cmd_trap_policer_set_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
-	{
-		.cmd = DEVLINK_CMD_SELFTESTS_GET,
-		.doit = devlink_nl_cmd_selftests_get_doit,
-		.dumpit = devlink_nl_instance_iter_dumpit,
-		/* can be retrieved by unprivileged users */
-	},
 	{
 		.cmd = DEVLINK_CMD_SELFTESTS_RUN,
 		.doit = devlink_nl_cmd_selftests_run,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 3dae9303cfa7..90497d0e1a7b 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -195,8 +195,8 @@ static const struct devlink_cmd *devl_cmds[] = {
 	[DEVLINK_CMD_SELFTESTS_GET]	= &devl_cmd_selftests_get,
 };
 
-int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
-				    struct netlink_callback *cb)
+static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
+					   struct netlink_callback *cb)
 {
 	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
 	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
@@ -286,15 +286,39 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	__DEVL_NL_OP_DUMP(cmd_subname, 0, maxattr, policy)
 
 static const struct genl_split_ops devlink_nl_split_ops[] = {
+	DEVL_NL_OP_LEGACY_DO(GET, get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(GET),
 	DEVL_NL_OP_LEGACY_DO(PORT_GET, port_get, port),
 	DEVL_NL_OP_LEGACY_DUMP(PORT_GET),
+	DEVL_NL_OP_LEGACY_DO(SB_GET, sb_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(SB_GET),
+	DEVL_NL_OP_LEGACY_DO(SB_POOL_GET, sb_pool_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(SB_POOL_GET),
+	DEVL_NL_OP_LEGACY_DO(SB_PORT_POOL_GET, sb_port_pool_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(SB_PORT_POOL_GET),
+	DEVL_NL_OP_LEGACY_DO(SB_TC_POOL_BIND_GET, sb_tc_pool_bind_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(SB_TC_POOL_BIND_GET),
 	DEVL_NL_OP_LEGACY_DO(PARAM_GET, param_get, simple),
 	DEVL_NL_OP_LEGACY_DUMP(PARAM_GET),
+	DEVL_NL_OP_LEGACY_DO(REGION_GET, region_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(REGION_GET),
+	DEVL_NL_OP_LEGACY_DO(INFO_GET, info_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(INFO_GET),
 	DEVL_NL_OP_LEGACY_DO(HEALTH_REPORTER_GET, health_reporter_get,
 			     port_optional),
 	DEVL_NL_OP_LEGACY_DUMP(HEALTH_REPORTER_GET),
 	DEVL_NL_OP_LEGACY_STRICT_DO(TRAP_GET, trap_get, simple),
 	DEVL_NL_OP_LEGACY_STRICT_DUMP(TRAP_GET),
+	DEVL_NL_OP_LEGACY_STRICT_DO(TRAP_GROUP_GET, trap_group_get, simple),
+	DEVL_NL_OP_LEGACY_STRICT_DUMP(TRAP_GROUP_GET),
+	DEVL_NL_OP_LEGACY_STRICT_DO(TRAP_POLICER_GET, trap_policer_get, simple),
+	DEVL_NL_OP_LEGACY_STRICT_DUMP(TRAP_POLICER_GET),
+	DEVL_NL_OP_LEGACY_DO(RATE_GET, rate_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(RATE_GET),
+	DEVL_NL_OP_LEGACY_DO(LINECARD_GET, linecard_get, simple),
+	DEVL_NL_OP_LEGACY_DUMP(LINECARD_GET),
+	DEVL_NL_OP_LEGACY_STRICT_DO(SELFTESTS_GET, selftests_get, simple),
+	DEVL_NL_OP_LEGACY_STRICT_DUMP(SELFTESTS_GET),
 	/* For every newly added command put above this line the set of macros
 	 * DEVL_NL_OP_DO and DEVL_NL_OP_DUMP should be used. Note that
 	 * there is an exception with non-iterator dump implementation.
-- 
2.41.0

[patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

For SFs, one devlink instance per SF is created. There might be
thousands of these on a single host. When a user needs to know port
handle for specific SF, he needs to dump all devlink ports on the host
which does not scale good.

Allow user to pass devlink handle alongside the dump command
and dump only objects which are under selected devlink instance.

Introduce new attr DEVLINK_ATTR_DUMP_SELECTOR to nest the selection
attributes. This way the userspace can use maxattr to tell if dump
selector is supported by kernel or not.

Assemble netlink policy for selector attribute. If user passes attr
unknown to kernel, netlink validation errors out.

Example:
$ devlink port show
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

$ devlink port show auxiliary/mlx5_core.eth.0
auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false

$ devlink port show auxiliary/mlx5_core.eth.1
auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- extended to patch that covers all dumpit commands
- used start() and done() callback to parse the selector attr
- changed the selector attr netlink policy to be created on fly
- changed patch description accordingly
---
 include/uapi/linux/devlink.h |  2 +
 net/devlink/devl_internal.h  |  1 +
 net/devlink/netlink.c        | 99 +++++++++++++++++++++++++++++++++++-
 3 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3782d4219ac9..8b74686512ae 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -612,6 +612,8 @@ enum devlink_attr {
 
 	DEVLINK_ATTR_REGION_DIRECT,		/* flag */
 
+	DEVLINK_ATTR_DUMP_SELECTOR,		/* nested */
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 79614b45e8ac..168d36dbc6f7 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -109,6 +109,7 @@ struct devlink_nl_dump_state {
 			u64 dump_ts;
 		};
 	};
+	struct nlattr **selector;
 };
 
 struct devlink_cmd {
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 90497d0e1a7b..c2083398bd73 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -80,6 +80,7 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_RATE_TX_PRIORITY] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_RATE_TX_WEIGHT] = { .type = NLA_U32 },
 	[DEVLINK_ATTR_REGION_DIRECT] = { .type = NLA_FLAG },
+	[DEVLINK_ATTR_DUMP_SELECTOR] = { .type = NLA_NESTED },
 };
 
 struct devlink *
@@ -195,6 +196,30 @@ static const struct devlink_cmd *devl_cmds[] = {
 	[DEVLINK_CMD_SELFTESTS_GET]	= &devl_cmd_selftests_get,
 };
 
+static int devlink_nl_instance_single_dumpit(struct sk_buff *msg,
+					     struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct nlattr **selector = state->selector;
+	const struct devlink_cmd *cmd;
+	struct devlink *devlink;
+	int err;
+
+	cmd = devl_cmds[info->op.cmd];
+
+	devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), selector);
+	if (IS_ERR(devlink))
+		return PTR_ERR(devlink);
+	err = cmd->dump_one(msg, devlink, cb);
+	devl_unlock(devlink);
+	devlink_put(devlink);
+
+	if (err != -EMSGSIZE)
+		return err;
+	return msg->len;
+}
+
 static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 					   struct netlink_callback *cb)
 {
@@ -232,6 +257,76 @@ static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 	return msg->len;
 }
 
+static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
+{
+	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
+}
+
+static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
+						 struct nla_policy *policy)
+{
+	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
+	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
+}
+
+static int devlink_nl_start(struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct nlattr **attrs = info->attrs;
+	const struct devlink_cmd *cmd;
+	struct nla_policy *policy;
+	struct nlattr **selector;
+	int err;
+
+	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
+		return 0;
+
+	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
+			   GFP_KERNEL);
+	if (!selector)
+		return -ENOMEM;
+	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
+	if (!policy) {
+		kfree(selector);
+		return -ENOMEM;
+	}
+
+	cmd = devl_cmds[info->op.cmd];
+	devlink_nl_dump_selector_policy_init(cmd, policy);
+	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
+			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
+			       policy, cb->extack);
+	kfree(policy);
+	if (err) {
+		kfree(selector);
+		return err;
+	}
+
+	state->selector = selector;
+	return 0;
+}
+
+static int devlink_nl_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	struct nlattr **selector = state->selector;
+
+	if (selector && selector[DEVLINK_ATTR_BUS_NAME] &&
+	    selector[DEVLINK_ATTR_DEV_NAME])
+		return devlink_nl_instance_single_dumpit(msg, cb);
+	else
+		return devlink_nl_instance_iter_dumpit(msg, cb);
+}
+
+static int devlink_nl_done(struct netlink_callback *cb)
+{
+	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+
+	kfree(state->selector);
+	return 0;
+}
+
 #define __DEVL_NL_OP_DO(cmd_subname, doit_subname, pre_doit_suffix, _validate,	\
 			_maxattr, _policy)					\
 	{									\
@@ -248,7 +343,9 @@ static int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
 #define __DEVL_NL_OP_DUMP(cmd_subname, _validate, _maxattr, _policy)		\
 	{									\
 		.cmd = DEVLINK_CMD_##cmd_subname,				\
-		.dumpit = devlink_nl_instance_iter_dumpit,			\
+		.start = devlink_nl_start,					\
+		.dumpit = devlink_nl_dumpit,					\
+		.done = devlink_nl_done,					\
 		.flags = GENL_CMD_CAP_DUMP,					\
 		.validate = _validate,						\
 		.maxattr = _maxattr,						\
-- 
2.41.0

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jakub Kicinski]

On Thu, 20 Jul 2023 14:18:28 +0200 Jiri Pirko wrote:
> +static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
> +{
> +	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
> +}
> +
> +static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
> +						 struct nla_policy *policy)
> +{
> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
> +}
> +
> +static int devlink_nl_start(struct netlink_callback *cb)
> +{
> +	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	struct nlattr **attrs = info->attrs;
> +	const struct devlink_cmd *cmd;
> +	struct nla_policy *policy;
> +	struct nlattr **selector;
> +	int err;
> +
> +	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
> +		return 0;
> +
> +	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
> +			   GFP_KERNEL);
> +	if (!selector)
> +		return -ENOMEM;
> +	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
> +	if (!policy) {
> +		kfree(selector);
> +		return -ENOMEM;
> +	}
> +
> +	cmd = devl_cmds[info->op.cmd];
> +	devlink_nl_dump_selector_policy_init(cmd, policy);
> +	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
> +			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> +			       policy, cb->extack);
> +	kfree(policy);
> +	if (err) {
> +		kfree(selector);
> +		return err;
> +	}
> +
> +	state->selector = selector;
> +	return 0;
> +}

Why not declare a fully nested policy with just the two attrs?

Also - do you know of any userspace which would pass garbage attrs 
to the dumps? Do we really need to accept all attributes, or can
we trim the dump policies to what's actually supported?
-- 
pw-bot: cr

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

Tue, Jul 25, 2023 at 08:40:44PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:28 +0200 Jiri Pirko wrote:
>> +static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
>> +{
>> +	memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
>> +}
>> +
>> +static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
>> +						 struct nla_policy *policy)
>> +{
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
>> +	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
>> +}
>> +
>> +static int devlink_nl_start(struct netlink_callback *cb)
>> +{
>> +	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>> +	struct nlattr **attrs = info->attrs;
>> +	const struct devlink_cmd *cmd;
>> +	struct nla_policy *policy;
>> +	struct nlattr **selector;
>> +	int err;
>> +
>> +	if (!attrs[DEVLINK_ATTR_DUMP_SELECTOR])
>> +		return 0;
>> +
>> +	selector = kzalloc(sizeof(*selector) * (DEVLINK_ATTR_MAX + 1),
>> +			   GFP_KERNEL);
>> +	if (!selector)
>> +		return -ENOMEM;
>> +	policy = kzalloc(sizeof(*policy) * (DEVLINK_ATTR_MAX + 1), GFP_KERNEL);
>> +	if (!policy) {
>> +		kfree(selector);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	cmd = devl_cmds[info->op.cmd];
>> +	devlink_nl_dump_selector_policy_init(cmd, policy);
>> +	err = nla_parse_nested(selector, DEVLINK_ATTR_MAX,
>> +			       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
>> +			       policy, cb->extack);
>> +	kfree(policy);
>> +	if (err) {
>> +		kfree(selector);
>> +		return err;
>> +	}
>> +
>> +	state->selector = selector;
>> +	return 0;
>> +}
>
>Why not declare a fully nested policy with just the two attrs?

Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
its own policy, generated by devlink_nl_dump_selector_policy_init(). I
did it this way instead of separate policy array for 2 reasons:
1) We don't have duplicate and possibly conflicting policies for devlink
   root and selector
2) It is easy for specific object type to pass attrs that are included
   in the policy initialization (see the health reporter extension later
   in this patchset). There are couple of object to benefit from this,
   for example "sb".
3) It is I think a bit nicer for specific object type to pass array of
   attrs, instead of a policy array that would be exported from netlink.c

If you insist on separate policy arrays, I can do it though. I had it
like that initially, I just decided to go this way for the 3 reasons
listed above.


>
>Also - do you know of any userspace which would pass garbage attrs 
>to the dumps? Do we really need to accept all attributes, or can
>we trim the dump policies to what's actually supported?

That's what this patch is doing. It only accepts what the kernel
understands. It gives the object types (as for example health reporter)
option to extend the attr set to accept them into selectors as well, if
they know how to handle them.


>-- 
>pw-bot: cr

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jakub Kicinski]

On Mon, 31 Jul 2023 14:47:08 +0200 Jiri Pirko wrote:
> >Why not declare a fully nested policy with just the two attrs?  
> 
> Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
> its own policy, generated by devlink_nl_dump_selector_policy_init(). I
> did it this way instead of separate policy array for 2 reasons:
> 1) We don't have duplicate and possibly conflicting policies for devlink
>    root and selector
> 2) It is easy for specific object type to pass attrs that are included
>    in the policy initialization (see the health reporter extension later
>    in this patchset). There are couple of object to benefit from this,
>    for example "sb".
> 3) It is I think a bit nicer for specific object type to pass array of
>    attrs, instead of a policy array that would be exported from netlink.c
> 
> If you insist on separate policy arrays, I can do it though.

IMO the contents of the series do not justify the complexity.

> I had it like that initially, I just decided to go this way for the 3 reasons
> listed above.
> 
> >Also - do you know of any userspace which would pass garbage attrs 
> >to the dumps? Do we really need to accept all attributes, or can
> >we trim the dump policies to what's actually supported?  
> 
> That's what this patch is doing. It only accepts what the kernel
> understands. It gives the object types (as for example health reporter)
> option to extend the attr set to accept them into selectors as well, if
> they know how to handle them.

I'm talking about the "outer" policy, the level at which
DEVLINK_ATTR_DUMP_SELECTOR is defined.

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

Mon, Jul 31, 2023 at 07:03:41PM CEST, kuba@kernel.org wrote:
>On Mon, 31 Jul 2023 14:47:08 +0200 Jiri Pirko wrote:
>> >Why not declare a fully nested policy with just the two attrs?  
>> 
>> Not sure I follow. But the nest under DEVLINK_ATTR_DUMP_SELECTOR has
>> its own policy, generated by devlink_nl_dump_selector_policy_init(). I
>> did it this way instead of separate policy array for 2 reasons:
>> 1) We don't have duplicate and possibly conflicting policies for devlink
>>    root and selector
>> 2) It is easy for specific object type to pass attrs that are included
>>    in the policy initialization (see the health reporter extension later
>>    in this patchset). There are couple of object to benefit from this,
>>    for example "sb".
>> 3) It is I think a bit nicer for specific object type to pass array of
>>    attrs, instead of a policy array that would be exported from netlink.c
>> 
>> If you insist on separate policy arrays, I can do it though.
>
>IMO the contents of the series do not justify the complexity.
>
>> I had it like that initially, I just decided to go this way for the 3 reasons
>> listed above.
>> 
>> >Also - do you know of any userspace which would pass garbage attrs 
>> >to the dumps? Do we really need to accept all attributes, or can
>> >we trim the dump policies to what's actually supported?  
>> 
>> That's what this patch is doing. It only accepts what the kernel
>> understands. It gives the object types (as for example health reporter)
>> option to extend the attr set to accept them into selectors as well, if
>> they know how to handle them.
>
>I'm talking about the "outer" policy, the level at which
>DEVLINK_ATTR_DUMP_SELECTOR is defined.

I don't follow :/ Could you please describe what exactly do you mean and
want to see? Thanks!

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jakub Kicinski]

On Tue, 1 Aug 2023 08:42:09 +0200 Jiri Pirko wrote:
> >> >Also - do you know of any userspace which would pass garbage attrs 
> >> >to the dumps? Do we really need to accept all attributes, or can
> >> >we trim the dump policies to what's actually supported?    
> >> 
> >> That's what this patch is doing. It only accepts what the kernel
> >> understands. It gives the object types (as for example health reporter)
> >> option to extend the attr set to accept them into selectors as well, if
> >> they know how to handle them.  
> >
> >I'm talking about the "outer" policy, the level at which
> >DEVLINK_ATTR_DUMP_SELECTOR is defined.  
> 
> I don't follow :/ Could you please describe what exactly do you mean and
> want to see? Thanks!

It's a bit obscured by the macros, but AFAICT you pass
devlink_nl_policy for the dumps, while the _only_ attribute
the kernel will interpret is DEVLINK_ATTR_DUMP_SELECTOR
and its insides.

Re: [patch net-next v2 10/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

Tue, Aug 01, 2023 at 05:53:01PM CEST, kuba@kernel.org wrote:
>On Tue, 1 Aug 2023 08:42:09 +0200 Jiri Pirko wrote:
>> >> >Also - do you know of any userspace which would pass garbage attrs 
>> >> >to the dumps? Do we really need to accept all attributes, or can
>> >> >we trim the dump policies to what's actually supported?    
>> >> 
>> >> That's what this patch is doing. It only accepts what the kernel
>> >> understands. It gives the object types (as for example health reporter)
>> >> option to extend the attr set to accept them into selectors as well, if
>> >> they know how to handle them.  
>> >
>> >I'm talking about the "outer" policy, the level at which
>> >DEVLINK_ATTR_DUMP_SELECTOR is defined.  
>> 
>> I don't follow :/ Could you please describe what exactly do you mean and
>> want to see? Thanks!
>
>It's a bit obscured by the macros, but AFAICT you pass
>devlink_nl_policy for the dumps, while the _only_ attribute
>the kernel will interpret is DEVLINK_ATTR_DUMP_SELECTOR
>and its insides.

True, you are correct.
Anyway with the split ops generation, this is going to be narrowed down,
so possiblem garbage is ignored.
Thanks!

[patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Introduce a possibility for devlink object to expose attributes it
supports for selection of dumped objects.

Use this by health reporter to indicate it supports port index based
selection of dump objects. Implement this selection mechanism in
devlink_nl_cmd_health_reporter_get_dump_one()

Example:
$ devlink health
pci/0000:08:00.0:
  reporter fw
    state healthy error 0 recover 0 auto_dump true
  reporter fw_fatal
    state healthy error 0 recover 0 grace_period 60000 auto_recover true auto_dump true
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32768:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32769:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32770:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.1:
  reporter fw
    state healthy error 0 recover 0 auto_dump true
  reporter fw_fatal
    state healthy error 0 recover 0 grace_period 60000 auto_recover true auto_dump true
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.1/98304:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.1/98305:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.1/98306:
  reporter vnic
    state healthy error 0 recover 0

$ devlink health show pci/0000:08:00.0
pci/0000:08:00.0:
  reporter fw
    state healthy error 0 recover 0 auto_dump true
  reporter fw_fatal
    state healthy error 0 recover 0 grace_period 60000 auto_recover true auto_dump true
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32768:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32769:
  reporter vnic
    state healthy error 0 recover 0
pci/0000:08:00.0/32770:
  reporter vnic
    state healthy error 0 recover 0

$ devlink health show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768:
  reporter vnic
    state healthy error 0 recover 0

The last command is possible because of this patch.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/devl_internal.h |  2 ++
 net/devlink/health.c        | 21 +++++++++++++++++++--
 net/devlink/netlink.c       |  8 ++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 168d36dbc6f7..510b66806341 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -115,6 +115,8 @@ struct devlink_nl_dump_state {
 struct devlink_cmd {
 	int (*dump_one)(struct sk_buff *msg, struct devlink *devlink,
 			struct netlink_callback *cb);
+	const u16 *dump_selector_attrs;
+	unsigned int dump_selector_attrs_count;
 };
 
 extern const struct genl_small_ops devlink_nl_ops[40];
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 194340a8bb86..74d322ee5b83 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -390,12 +390,21 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg,
 					    struct netlink_callback *cb)
 {
 	struct devlink_nl_dump_state *state = devlink_dump_state(cb);
+	struct nlattr **selector = state->selector;
 	struct devlink_health_reporter *reporter;
+	unsigned long port_index_end = ULONG_MAX;
+	unsigned long port_index_start = 0;
 	struct devlink_port *port;
 	unsigned long port_index;
 	int idx = 0;
 	int err;
 
+	if (selector && selector[DEVLINK_ATTR_PORT_INDEX]) {
+		port_index_start = nla_get_u32(selector[DEVLINK_ATTR_PORT_INDEX]);
+		port_index_end = port_index_start;
+		goto per_port_dump;
+	}
+
 	list_for_each_entry(reporter, &devlink->reporter_list, list) {
 		if (idx < state->idx) {
 			idx++;
@@ -412,7 +421,9 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg,
 		}
 		idx++;
 	}
-	xa_for_each(&devlink->ports, port_index, port) {
+per_port_dump:
+	xa_for_each_range(&devlink->ports, port_index, port,
+			  port_index_start, port_index_end) {
 		list_for_each_entry(reporter, &port->reporter_list, list) {
 			if (idx < state->idx) {
 				idx++;
@@ -434,8 +445,14 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg,
 	return 0;
 }
 
+static const u16 devl_cmd_health_reporter_dump_selector_attrs[] = {
+	DEVLINK_ATTR_PORT_INDEX,
+};
+
 const struct devlink_cmd devl_cmd_health_reporter_get = {
-	.dump_one		= devlink_nl_cmd_health_reporter_get_dump_one,
+	.dump_one			= devlink_nl_cmd_health_reporter_get_dump_one,
+	.dump_selector_attrs		= devl_cmd_health_reporter_dump_selector_attrs,
+	.dump_selector_attrs_count	= ARRAY_SIZE(devl_cmd_health_reporter_dump_selector_attrs),
 };
 
 int devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb,
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index c2083398bd73..bd60d229cfbe 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -265,8 +265,16 @@ static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr)
 static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd,
 						 struct nla_policy *policy)
 {
+	int i;
+
 	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME);
 	devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME);
+
+	for (i = 0; i < cmd->dump_selector_attrs_count; i++) {
+		unsigned int attr = cmd->dump_selector_attrs[i];
+
+		memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy));
+	}
 }
 
 static int devlink_nl_start(struct netlink_callback *cb)
-- 
2.41.0

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jakub Kicinski]

On Thu, 20 Jul 2023 14:18:29 +0200 Jiri Pirko wrote:
> Introduce a possibility for devlink object to expose attributes it
> supports for selection of dumped objects.
> 
> Use this by health reporter to indicate it supports port index based
> selection of dump objects. Implement this selection mechanism in
> devlink_nl_cmd_health_reporter_get_dump_one()

This patch is not very clean. IMHO implementing the filters by skipping
is not going to scale to reasonably complex filters. Isn't it better to
add a .filter callback which will look at the about-to-be-dumped object
and return true/false on whether it should be dumped?

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jiri Pirko]

Tue, Jul 25, 2023 at 08:48:03PM CEST, kuba@kernel.org wrote:
>On Thu, 20 Jul 2023 14:18:29 +0200 Jiri Pirko wrote:
>> Introduce a possibility for devlink object to expose attributes it
>> supports for selection of dumped objects.
>> 
>> Use this by health reporter to indicate it supports port index based
>> selection of dump objects. Implement this selection mechanism in
>> devlink_nl_cmd_health_reporter_get_dump_one()
>
>This patch is not very clean. IMHO implementing the filters by skipping
>is not going to scale to reasonably complex filters. Isn't it better to

I'm not sure what do you mean by skipping? There is not skipping. In
case PORT_INDEX is passed in the selector, only that specific port is
processed. No scale issues I see. Am I missing something?


>add a .filter callback which will look at the about-to-be-dumped object
>and return true/false on whether it should be dumped?

No, that would not scale. Passing the selector attrs to the dump
callback it better, as the dump callback according to the attrs can
reach only what is needed, knowing the internals. But perhaps I don't
understand correctly your suggestion.

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jakub Kicinski]

On Mon, 31 Jul 2023 14:52:44 +0200 Jiri Pirko wrote:
> >This patch is not very clean. IMHO implementing the filters by skipping
> >is not going to scale to reasonably complex filters. Isn't it better to  
> 
> I'm not sure what do you mean by skipping? There is not skipping. In
> case PORT_INDEX is passed in the selector, only that specific port is
> processed. No scale issues I see. Am I missing something?
> 
> 
> >add a .filter callback which will look at the about-to-be-dumped object
> >and return true/false on whether it should be dumped?  
> 
> No, that would not scale. Passing the selector attrs to the dump
> callback it better, as the dump callback according to the attrs can
> reach only what is needed, knowing the internals. But perhaps I don't
> understand correctly your suggestion.

for_each_obj() {
	if (obj_dump_filtered(obj, dump_info))  // < run filter
		continue;                       // < skip object

	dump_one(obj)
}

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jiri Pirko]

Mon, Jul 31, 2023 at 07:06:32PM CEST, kuba@kernel.org wrote:
>On Mon, 31 Jul 2023 14:52:44 +0200 Jiri Pirko wrote:
>> >This patch is not very clean. IMHO implementing the filters by skipping
>> >is not going to scale to reasonably complex filters. Isn't it better to  
>> 
>> I'm not sure what do you mean by skipping? There is not skipping. In
>> case PORT_INDEX is passed in the selector, only that specific port is
>> processed. No scale issues I see. Am I missing something?
>> 
>> 
>> >add a .filter callback which will look at the about-to-be-dumped object
>> >and return true/false on whether it should be dumped?  
>> 
>> No, that would not scale. Passing the selector attrs to the dump
>> callback it better, as the dump callback according to the attrs can
>> reach only what is needed, knowing the internals. But perhaps I don't
>> understand correctly your suggestion.
>
>for_each_obj() {
>	if (obj_dump_filtered(obj, dump_info))  // < run filter
>		continue;                       // < skip object
>
>	dump_one(obj)

I don't see how this would help. For example, passing PORT_INDEX, I know
exactly what object to reach, according to this PORT_INDEX. Why to
iterate over all of them and try the filter? Does not make sense to me.

Maybe we are each understanding this feature differently. This is about
passing keys which index the objects. It is always devlink handle,
sometimes port index and I see another example in shared buffer index.
That's about it. Basically user passes partial tuple of indexes.
Example:
devlink port show
the key is: bus_name/dev_name/port_index
user passes bus_name/dev_name, this is the selector, a partial key.

The sophisticated filtering is not a focus of this patchset. User can do
it putting bpf filter on the netlink socket.

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jakub Kicinski]

On Tue, 1 Aug 2023 08:49:45 +0200 Jiri Pirko wrote:
> >for_each_obj() {
> >	if (obj_dump_filtered(obj, dump_info))  // < run filter
> >		continue;                       // < skip object
> >
> >	dump_one(obj)  
> 
> I don't see how this would help. For example, passing PORT_INDEX, I know
> exactly what object to reach, according to this PORT_INDEX. Why to
> iterate over all of them and try the filter? Does not make sense to me.
> 
> Maybe we are each understanding this feature differently. This is about
> passing keys which index the objects. It is always devlink handle,
> sometimes port index and I see another example in shared buffer index.
> That's about it. Basically user passes partial tuple of indexes.
> Example:
> devlink port show
> the key is: bus_name/dev_name/port_index
> user passes bus_name/dev_name, this is the selector, a partial key.
> 
> The sophisticated filtering is not a focus of this patchset. User can do
> it putting bpf filter on the netlink socket.

Okay, I was trying to be helpful, I don't want to argue for
a particular implementation. IMO what's posted is too ugly
to be merged, please restructure it.

Re: [patch net-next v2 11/11] devlink: extend health reporter dump selector by port index

[Jiri Pirko]

Tue, Aug 01, 2023 at 05:56:44PM CEST, kuba@kernel.org wrote:
>On Tue, 1 Aug 2023 08:49:45 +0200 Jiri Pirko wrote:
>> >for_each_obj() {
>> >	if (obj_dump_filtered(obj, dump_info))  // < run filter
>> >		continue;                       // < skip object
>> >
>> >	dump_one(obj)  
>> 
>> I don't see how this would help. For example, passing PORT_INDEX, I know
>> exactly what object to reach, according to this PORT_INDEX. Why to
>> iterate over all of them and try the filter? Does not make sense to me.
>> 
>> Maybe we are each understanding this feature differently. This is about
>> passing keys which index the objects. It is always devlink handle,
>> sometimes port index and I see another example in shared buffer index.
>> That's about it. Basically user passes partial tuple of indexes.
>> Example:
>> devlink port show
>> the key is: bus_name/dev_name/port_index
>> user passes bus_name/dev_name, this is the selector, a partial key.
>> 
>> The sophisticated filtering is not a focus of this patchset. User can do
>> it putting bpf filter on the netlink socket.
>
>Okay, I was trying to be helpful, I don't want to argue for
>a particular implementation. IMO what's posted is too ugly
>to be merged, please restructure it.

Ugly in which sense? What exactly needs to be restructured?

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Petr Machata]

I'll take this through our nightly and will report back tomorrow.

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

Thu, Jul 20, 2023 at 03:55:00PM CEST, petrm@nvidia.com wrote:
>I'll take this through our nightly and will report back tomorrow.

Sure. I ran mlxsw regression with this already, no issues.

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Petr Machata]

Jiri Pirko <jiri@resnulli.us> writes:

> Thu, Jul 20, 2023 at 03:55:00PM CEST, petrm@nvidia.com wrote:
>
>>I'll take this through our nightly and will report back tomorrow.
>
> Sure. I ran mlxsw regression with this already, no issues.

You started it on one machine and it went well for a while. But it's
getting a stream of these splats right now:

INFO       - INFO       - [ 4155.564670] rcu: INFO: rcu_preempt self-detected stall on CPU 
INFO       - INFO       - [ 4155.571093] rcu: 	7-....: (99998 ticks this GP) idle=ac7c/1/0x4000000000000000 softirq=86447/86447 fqs=25001 
INFO       - INFO       - [ 4155.582077] rcu: 	(t=100015 jiffies g=289809 q=1459 ncpus=8) 
INFO       - INFO       - [ 4155.588398] CPU: 7 PID: 38940 Comm: ip Not tainted 6.5.0-rc1jiri+ #1 
INFO       - INFO       - [ 4155.595497] Hardware name: Mellanox Technologies Ltd. MSN4700/VMOD0010, BIOS 5.11 01/06/2019 
INFO       - INFO       - [ 4155.604915] RIP: 0010:__netlink_lookup+0xca/0x150 
INFO       - INFO       - [ 4155.610171] Code: 00 00 48 89 c7 48 83 cf 01 48 8b 10 48 83 e2 fe 48 0f 44 d7 f6 c2 01 75 5a 0f b7 4b 16 44 8b 44 24 08 49 89 c9 49 f7 d9 eb 08 <48> 8b 12 f6 c2 01 75 41 4a 8d 34 0a 44 39 86 e8 02 00 00 75 eb 48 
INFO       - INFO       - [ 4155.631156] RSP: 0018:ffffbea7ca41b760 EFLAGS: 00000213 
INFO       - INFO       - [ 4155.636992] RAX: ffffa048c25120b0 RBX: ffffa048c01e4000 RCX: 0000000000000400 
INFO       - INFO       - [ 4155.644964] RDX: ffffa048c6d4b400 RSI: ffffa048c6d4b000 RDI: ffffa048c25120b1 
INFO       - INFO       - [ 4155.652935] RBP: ffffa048c2512000 R08: 00000000888fe595 R09: fffffffffffffc00 
INFO       - INFO       - [ 4155.660906] R10: 00000000302e3030 R11: 0000006900030008 R12: ffffa048c9205900 
INFO       - INFO       - [ 4155.668879] R13: 00000000888fe595 R14: 0000000000000001 R15: ffffa048c01e4000 
INFO       - INFO       - [ 4155.676850] FS:  00007f2155bcf800(0000) GS:ffffa04c2fdc0000(0000) knlGS:0000000000000000 
INFO       - INFO       - [ 4155.685890] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033 
INFO       - INFO       - [ 4155.692307] CR2: 00000000004e4140 CR3: 000000014c919005 CR4: 00000000003706e0 
INFO       - INFO       - [ 4155.700279] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 
INFO       - INFO       - [ 4155.708249] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 
INFO       - INFO       - [ 4155.716220] Call Trace: 
INFO       - INFO       - [ 4155.718948]  <IRQ> 
INFO       - INFO       - [ 4155.721190]  ? rcu_dump_cpu_stacks+0xea/0x170 
INFO       - INFO       - [ 4155.726057]  ? rcu_sched_clock_irq+0x53b/0x10b0 
INFO       - INFO       - [ 4155.731116]  ? update_load_avg+0x54/0x280 
INFO       - INFO       - [ 4155.735593]  ? notifier_call_chain+0x5a/0xc0 
INFO       - INFO       - [ 4155.740361]  ? timekeeping_update+0xaf/0x280 
INFO       - INFO       - [ 4155.745130]  ? timekeeping_advance+0x374/0x590 
INFO       - INFO       - [ 4155.750093]  ? update_process_times+0x74/0xb0 
INFO       - INFO       - [ 4155.754957]  ? tick_sched_handle+0x33/0x50 
INFO       - INFO       - [ 4155.759529]  ? tick_sched_timer+0x6b/0x80 
INFO       - INFO       - [ 4155.763995]  ? tick_sched_do_timer+0x80/0x80 
INFO       - INFO       - [ 4155.768762]  ? __hrtimer_run_queues+0x10f/0x2a0 
INFO       - INFO       - [ 4155.773820]  ? hrtimer_interrupt+0xf8/0x230 
INFO       - INFO       - [ 4155.778492]  ? __sysvec_apic_timer_interrupt+0x52/0x120 
INFO       - INFO       - [ 4155.784327]  ? sysvec_apic_timer_interrupt+0x6d/0x90 
INFO       - INFO       - [ 4155.789874]  </IRQ> 
INFO       - INFO       - [ 4155.792211]  <TASK> 
INFO       - INFO       - [ 4155.794549]  ? asm_sysvec_apic_timer_interrupt+0x1a/0x20 
INFO       - INFO       - [ 4155.800485]  ? __netlink_lookup+0xca/0x150 
INFO       - INFO       - [ 4155.805059]  netlink_unicast+0x132/0x390 
INFO       - INFO       - [ 4155.809437]  rtnl_getlink+0x36d/0x410 
INFO       - INFO       - [ 4155.813532]  rtnetlink_rcv_msg+0x14f/0x3b0 
INFO       - INFO       - [ 4155.818106]  ? __alloc_pages+0x17c/0x290 
INFO       - INFO       - [ 4155.822485]  ? rtnl_calcit.isra.0+0x140/0x140 
INFO       - INFO       - [ 4155.827348]  netlink_rcv_skb+0x58/0x100 
INFO       - INFO       - [ 4155.831631]  netlink_unicast+0x23c/0x390 
INFO       - INFO       - [ 4155.836010]  netlink_sendmsg+0x214/0x470 
INFO       - INFO       - [ 4155.840390]  ? netlink_unicast+0x390/0x390 
INFO       - INFO       - [ 4155.844963]  ____sys_sendmsg+0x16a/0x260 
INFO       - INFO       - [ 4155.849345]  ___sys_sendmsg+0x9a/0xe0 
INFO       - INFO       - [ 4155.853437]  __sys_sendmsg+0x7a/0xc0 
INFO       - INFO       - [ 4155.857428]  do_syscall_64+0x38/0x80 
INFO       - INFO       - [ 4155.861419]  entry_SYSCALL_64_after_hwframe+0x63/0xcd 

BTW, while for core patches, any machine pass is usually a good
predictor of full regression pass, that's not always the case. There's
a reason we run on about 15 machines plus simulation. Even if this had
"no issues", there would be value in getting full regression run.

I'm pulling this from the nightly again.

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

I see that this patchset got moved to "changes requested" in patchwork.
Why exacly? There was no comment so far. Petr's splat is clearly not
caused by this patchset.

Should I resubmit?

Thanks!

Thu, Jul 20, 2023 at 02:18:18PM CEST, jiri@resnulli.us wrote:
>From: Jiri Pirko <jiri@nvidia.com>
>
>Motivation:
>
>For SFs, one devlink instance per SF is created. There might be
>thousands of these on a single host. When a user needs to know port
>handle for specific SF, he needs to dump all devlink ports on the host
>which does not scale good.
>
>Solution:
>
>Allow user to pass devlink handle alongside the dump command
>and dump only objects which are under selected devlink instance.
>
>Introduce new attr DEVLINK_ATTR_DUMP_SELECTOR to nest the selection
>attributes. This way the userspace can use maxattr to tell if dump
>selector is supported by kernel or not.
>
>Assemble netlink policy for selector attribute. If user passes attr
>unknown to kernel, netlink validation errors out.
>
>Example:
>$ devlink port show
>auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false
>auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false
>
>$ devlink port show auxiliary/mlx5_core.eth.0
>auxiliary/mlx5_core.eth.0/65535: type eth netdev eth2 flavour physical port 0 splittable false
>
>$ devlink port show auxiliary/mlx5_core.eth.1
>auxiliary/mlx5_core.eth.1/131071: type eth netdev eth3 flavour physical port 1 splittable false
>
>This is done in patch #10
>
>Dependency:
>
>The DEVLINK_ATTR_DUMP_SELECTOR parsing is very suitable to be done
>once at the beginning of the dumping. Unfortunatelly, it is not possible
>to define start() and done() callbacks for netlink small ops.
>So all commands that use instance iterator for dumpit are converted to
>split ops. This is done in patch #1-9
>
>Extension:
>
>patch #11 extends the selector by port index for health reporter
>dumping.
>
>v1->v2:
>- the original single patch (patch #10) was extended to a patchset
>
>Jiri Pirko (11):
>  devlink: parse linecard attr in doit() callbacks
>  devlink: parse rate attrs in doit() callbacks
>  devlink: introduce __devlink_nl_pre_doit() with internal flags as
>    function arg
>  devlink: convert port get command to split ops
>  devlink: convert health reporter get command to split ops
>  devlink: convert param get command to split ops
>  devlink: convert trap get command to split ops
>  devlink: introduce set of macros and use it for split ops definitions
>  devlink: convert rest of the iterator dumpit commands to split ops
>  devlink: introduce dump selector attr and use it for per-instance
>    dumps
>  devlink: extend health reporter dump selector by port index
>
> include/uapi/linux/devlink.h |   2 +
> net/devlink/devl_internal.h  |  42 +++---
> net/devlink/health.c         |  21 ++-
> net/devlink/leftover.c       | 211 ++++++++--------------------
> net/devlink/netlink.c        | 263 ++++++++++++++++++++++++++++++-----
> 5 files changed, 333 insertions(+), 206 deletions(-)
>
>-- 
>2.41.0
>

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Paolo Abeni]

On Tue, 2023-07-25 at 10:07 +0200, Jiri Pirko wrote:
> I see that this patchset got moved to "changes requested" in patchwork.
> Why exacly? There was no comment so far. Petr's splat is clearly not
> caused by this patchset.

Quickly skimming over the series I agree the reported splat looks
possibly more related to core netlink and/or rhashtable but it would
help if you could express your reasoning on the splat itself, possibly
with a decoded back-trace.

Thanks!

Paolo
> 

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Jiri Pirko]

Tue, Jul 25, 2023 at 10:36:31AM CEST, pabeni@redhat.com wrote:
>On Tue, 2023-07-25 at 10:07 +0200, Jiri Pirko wrote:
>> I see that this patchset got moved to "changes requested" in patchwork.
>> Why exacly? There was no comment so far. Petr's splat is clearly not
>> caused by this patchset.
>
>Quickly skimming over the series I agree the reported splat looks
>possibly more related to core netlink and/or rhashtable but it would
>help if you could express your reasoning on the splat itself, possibly
>with a decoded back-trace.
>

Well, since I'm touching only devlink, this is underlated to this
patchset. I don't see why I would need to care about the splat.
I will repost, if that is ok.


>Thanks!
>
>Paolo
>> 
>

Re: [patch net-next v2 00/11] devlink: introduce dump selector attr and use it for per-instance dumps

[Paolo Abeni]

On Tue, 2023-07-25 at 17:29 +0200, Jiri Pirko wrote:
> Tue, Jul 25, 2023 at 10:36:31AM CEST, pabeni@redhat.com wrote:
> > On Tue, 2023-07-25 at 10:07 +0200, Jiri Pirko wrote:
> > > I see that this patchset got moved to "changes requested" in patchwork.
> > > Why exacly? There was no comment so far. Petr's splat is clearly not
> > > caused by this patchset.
> > 
> > Quickly skimming over the series I agree the reported splat looks
> > possibly more related to core netlink and/or rhashtable but it would
> > help if you could express your reasoning on the splat itself, possibly
> > with a decoded back-trace.
> > 
> 
> Well, since I'm touching only devlink, this is underlated to this
> patchset. I don't see why I would need to care about the splat.
> I will repost, if that is ok.

Not needed, the series is again in 'new' status.

Cheers,

Paolo