Re: [PATCH net-next v7 3/9] devlink: Enable creation of the devlink-rate nodes from the driver
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: 2022-10-27 21:28:17
From: Michal Wilczynski <redacted> Date: Thu, 27 Oct 2022 15:00:43 +0200
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.
I would swap the order of paragraphs above. [...]
quoted hunk ↗ jump to hunk
@@ -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);
One space to much before `char *node_name` [...]
+/**
+ * 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)
+{
+ 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))
+ return -ENODEV;`rate_node` is leaked on error path
+ 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 = kzalloc(DEVLINK_RATE_NAME_MAX_LEN, GFP_KERNEL); + if (!rate_node->name) + return -ENOMEM; + + strscpy(rate_node->name, node_name, DEVLINK_RATE_NAME_MAX_LEN);
Again a memleak on error path. It looks also that we could use kstrndup() instead of kzalloc+strscpy combo. Finally, I would centralize memory allocation failures.
+ + 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); +
[...] --PK