Re: [PATCH net-next v8 3/9] devlink: Enable creation of the devlink-rate nodes from the driver

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 28, 2022 at 12:51:37PM CEST, michal.wilczynski@intel.com wrote:
>Intel 100G card internal firmware hierarchy for Hierarchicial QoS is very
>rigid and can't be easily removed. This requires an ability to export
>default hierarchy to allow user to modify it. Currently the driver is
>only able to create the 'leaf' nodes, which usually represent the vport.
>This is not enough for HQoS implemented in Intel hardware.
>
>Introduce new function devl_rate_node_create() that allows for creation
>of the devlink-rate nodes from the driver.
>
>Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>---
> include/net/devlink.h |  4 ++++
> net/core/devlink.c    | 49 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 929cb72ef412..9d0a424712fd 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -98,6 +98,8 @@ struct devlink_port_attrs {
> 	};
> };
> 
>+#define DEVLINK_RATE_NAME_MAX_LEN 30
>+
> struct devlink_rate {
> 	struct list_head list;
> 	enum devlink_rate_type type;
>@@ -1601,6 +1603,8 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port,
> 				   u32 controller, u16 pf, u32 sf,
> 				   bool external);
> int devl_rate_leaf_create(struct devlink_port *port, void *priv);
>+int devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name,
>+			  char *parent_name);
> void devl_rate_leaf_destroy(struct devlink_port *devlink_port);
> void devl_rate_nodes_destroy(struct devlink *devlink);
> void devlink_port_linecard_set(struct devlink_port *devlink_port,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index b97c077cf66e..08f1bbd54c43 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -10270,6 +10270,55 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
> }
> EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
> 
>+/**
>+ * devl_rate_node_create - create devlink rate node
>+ * @devlink: devlink instance
>+ * @priv: driver private data
>+ * @node_name: name of the resulting node
>+ * @parent_name: name of the parent node
>+ *
>+ * Create devlink rate object of type node
>+ */
>+int devl_rate_node_create(struct devlink *devlink, void *priv, char *node_name, char *parent_name)

Nope, this is certainly incorrect. Do not refer to kernel object by
string. You also don't have internal kernel api based on ifname to refer
to struct net_device instance.

Please have "struct devlink_rate *parent" to refer to parent node and
make this function return "struct devlink_rate *".


>+{
>+	struct devlink_rate *rate_node;
>+	struct devlink_rate *parent;
>+
>+	rate_node = devlink_rate_node_get_by_name(devlink, node_name);
>+	if (!IS_ERR(rate_node))
>+		return -EEXIST;
>+
>+	rate_node = kzalloc(sizeof(*rate_node), GFP_KERNEL);
>+	if (!rate_node)
>+		return -ENOMEM;
>+
>+	if (parent_name) {
>+		parent = devlink_rate_node_get_by_name(devlink, parent_name);
>+		if (IS_ERR(parent)) {
>+			kfree(rate_node);
>+			return -ENODEV;
>+		}
>+		rate_node->parent = parent;
>+		refcount_inc(&rate_node->parent->refcnt);
>+	}
>+
>+	rate_node->type = DEVLINK_RATE_TYPE_NODE;
>+	rate_node->devlink = devlink;
>+	rate_node->priv = priv;
>+
>+	rate_node->name = kstrndup(node_name, DEVLINK_RATE_NAME_MAX_LEN, GFP_KERNEL);

Why do you limit the name length? We don't limit the length passed from
user, I see no reason to do it for driver.


>+	if (!rate_node->name) {
>+		kfree(rate_node);
>+		return -ENOMEM;
>+	}
>+
>+	refcount_set(&rate_node->refcnt, 1);
>+	list_add(&rate_node->list, &devlink->rate_list);
>+	devlink_rate_notify(rate_node, DEVLINK_CMD_RATE_NEW);
>+	return 0;
>+}
>+EXPORT_SYMBOL_GPL(devl_rate_node_create);
>+
> /**
>  * devl_rate_leaf_create - create devlink rate leaf
>  * @devlink_port: devlink port object to create rate object on
>-- 
>2.37.2
>