diff mbox series

[net-next,07/14] devlink: Implement port params registration

Message ID 20250228021227.871993-8-saeed@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series devlink, mlx5: Add new parameters for link management and SRIOV/eSwitch configurations | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl fail Generated files up to date; build failed; build has 10 warnings/errors; GEN HAS DIFF 2 files changed, 12664 deletions(-);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 523 this patch: 523
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 5 this patch: 6
netdev/source_inline success Was 0 now: 0

Commit Message

Saeed Mahameed Feb. 28, 2025, 2:12 a.m. UTC 
From: Saeed Mahameed <saeedm@nvidia.com>

Port params infrastructure is incomplete and needs a bit of plumbing to
support port params commands from netlink.

Introduce port params registration API, very similar to current devlink
params API, add the params xarray to devlink_port structure and
decouple devlink params registration routines from the devlink
structure.

Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h |  14 ++++
 net/devlink/param.c   | 150 ++++++++++++++++++++++++++++++++++--------
 net/devlink/port.c    |   3 +
 3 files changed, 140 insertions(+), 27 deletions(-)

Comments

Przemek Kitszel Feb. 28, 2025, 11:58 a.m. UTC | #1 
On 2/28/25 03:12, Saeed Mahameed wrote:
> From: Saeed Mahameed <saeedm@nvidia.com>
> 
> Port params infrastructure is incomplete and needs a bit of plumbing to
> support port params commands from netlink.
> 
> Introduce port params registration API, very similar to current devlink
> params API, add the params xarray to devlink_port structure and
> decouple devlink params registration routines from the devlink
> structure.
> 
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> ---
>   include/net/devlink.h |  14 ++++
>   net/devlink/param.c   | 150 ++++++++++++++++++++++++++++++++++--------
>   net/devlink/port.c    |   3 +
>   3 files changed, 140 insertions(+), 27 deletions(-)
For me devlink and devlink-port should be really the same, to the point
that the only difference is `bool is_port` flag inside of the
struct devlink. Then you could put special logic if really desired (to
exclude something for port).
Then for ease of driver programming you could have also a flag
"for_port" in the struct devlink_param, so developers will fill that
out statically and call it on all their devlinks (incl port).

Multiplying the APIs instead of rethinking a problem is not a good long
term solution.
Jiri Pirko Feb. 28, 2025, 12:28 p.m. UTC | #2 
Fri, Feb 28, 2025 at 12:58:38PM +0100, przemyslaw.kitszel@intel.com wrote:
>On 2/28/25 03:12, Saeed Mahameed wrote:
>> From: Saeed Mahameed <saeedm@nvidia.com>
>> 
>> Port params infrastructure is incomplete and needs a bit of plumbing to
>> support port params commands from netlink.
>> 
>> Introduce port params registration API, very similar to current devlink
>> params API, add the params xarray to devlink_port structure and
>> decouple devlink params registration routines from the devlink
>> structure.
>> 
>> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>   include/net/devlink.h |  14 ++++
>>   net/devlink/param.c   | 150 ++++++++++++++++++++++++++++++++++--------
>>   net/devlink/port.c    |   3 +
>>   3 files changed, 140 insertions(+), 27 deletions(-)
>For me devlink and devlink-port should be really the same, to the point
>that the only difference is `bool is_port` flag inside of the
>struct devlink. Then you could put special logic if really desired (to
>exclude something for port).

Why? Why other devlink objects shouldn't be the same as well. Then we
can have one union. Does not make sense to me. The only think dev and
port is sharing would be params. What else? Totally different beast.


>Then for ease of driver programming you could have also a flag
>"for_port" in the struct devlink_param, so developers will fill that
>out statically and call it on all their devlinks (incl port).
>
>Multiplying the APIs instead of rethinking a problem is not a good long
>term solution.
>
Przemek Kitszel Feb. 28, 2025, 1:23 p.m. UTC | #3 
On 2/28/25 13:28, Jiri Pirko wrote:
> Fri, Feb 28, 2025 at 12:58:38PM +0100, przemyslaw.kitszel@intel.com wrote:
>> On 2/28/25 03:12, Saeed Mahameed wrote:
>>> From: Saeed Mahameed <saeedm@nvidia.com>
>>>
>>> Port params infrastructure is incomplete and needs a bit of plumbing to
>>> support port params commands from netlink.
>>>
>>> Introduce port params registration API, very similar to current devlink
>>> params API, add the params xarray to devlink_port structure and
>>> decouple devlink params registration routines from the devlink
>>> structure.
>>>
>>> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>> ---
>>>    include/net/devlink.h |  14 ++++
>>>    net/devlink/param.c   | 150 ++++++++++++++++++++++++++++++++++--------
>>>    net/devlink/port.c    |   3 +
>>>    3 files changed, 140 insertions(+), 27 deletions(-)
>> For me devlink and devlink-port should be really the same, to the point
>> that the only difference is `bool is_port` flag inside of the
>> struct devlink. Then you could put special logic if really desired (to
>> exclude something for port).
> 
> Why? Why other devlink objects shouldn't be the same as well. Then we
> can have one union. Does not make sense to me. The only think dev and
> port is sharing would be params. What else? Totally different beast.

Instead of focusing on differences try to find similarities.

health reporters per port and "toplevel",
just by grepping:
devlink_nl_sb_pool_fill()
devlink_nl_sb_port_pool_fill(),

devlink_region_create()
devlink_port_region_create()

and there is no reason to assume that someone will not want to
extend ports to have devlink resources or other thing
Jiri Pirko Feb. 28, 2025, 3:21 p.m. UTC | #4 
Fri, Feb 28, 2025 at 02:23:00PM +0100, przemyslaw.kitszel@intel.com wrote:
>On 2/28/25 13:28, Jiri Pirko wrote:
>> Fri, Feb 28, 2025 at 12:58:38PM +0100, przemyslaw.kitszel@intel.com wrote:
>> > On 2/28/25 03:12, Saeed Mahameed wrote:
>> > > From: Saeed Mahameed <saeedm@nvidia.com>
>> > > 
>> > > Port params infrastructure is incomplete and needs a bit of plumbing to
>> > > support port params commands from netlink.
>> > > 
>> > > Introduce port params registration API, very similar to current devlink
>> > > params API, add the params xarray to devlink_port structure and
>> > > decouple devlink params registration routines from the devlink
>> > > structure.
>> > > 
>> > > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>> > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> > > ---
>> > >    include/net/devlink.h |  14 ++++
>> > >    net/devlink/param.c   | 150 ++++++++++++++++++++++++++++++++++--------
>> > >    net/devlink/port.c    |   3 +
>> > >    3 files changed, 140 insertions(+), 27 deletions(-)
>> > For me devlink and devlink-port should be really the same, to the point
>> > that the only difference is `bool is_port` flag inside of the
>> > struct devlink. Then you could put special logic if really desired (to
>> > exclude something for port).
>> 
>> Why? Why other devlink objects shouldn't be the same as well. Then we
>> can have one union. Does not make sense to me. The only think dev and
>> port is sharing would be params. What else? Totally different beast.
>
>Instead of focusing on differences try to find similarities.
>
>health reporters per port and "toplevel",
>just by grepping:
>devlink_nl_sb_pool_fill()
>devlink_nl_sb_port_pool_fill(),

Sharedbuffer is separate story.

>
>devlink_region_create()
>devlink_port_region_create()

Okay, regions I missed.

>
>and there is no reason to assume that someone will not want to
>extend ports to have devlink resources or other thing

But looks at differences. They are huge.

But perhaps I'm missing the point. What you want to achieve? Just to
reduce API? That is always a tradeoff. I don't think the pros top the
cons here.
diff mbox series

Patch

diff --git a/include/net/devlink.h b/include/net/devlink.h
index eed1e4507d17..11f98e3a750b 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -125,6 +125,7 @@  struct devlink_port {
 	struct list_head region_list;
 	struct devlink *devlink;
 	const struct devlink_port_ops *ops;
+	struct xarray params;
 	unsigned int index;
 	spinlock_t type_lock; /* Protects type and type_eth/ib
 			       * structures consistency.
@@ -1823,6 +1824,19 @@  void devl_params_unregister(struct devlink *devlink,
 void devlink_params_unregister(struct devlink *devlink,
 			       const struct devlink_param *params,
 			       size_t params_count);
+int devl_port_params_register(struct devlink_port *devlink_port,
+			      const struct devlink_param *params,
+			      size_t params_count);
+int devlink_port_params_register(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count);
+void devl_port_params_unregister(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count);
+void devlink_port_params_unregister(struct devlink_port *devlink_port,
+				    const struct devlink_param *params,
+				    size_t params_count);
+
 int devl_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 				    union devlink_param_value *val);
 void devl_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
diff --git a/net/devlink/param.c b/net/devlink/param.c
index 2263aba85a79..719eeb5152c3 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -627,13 +627,16 @@  static int devlink_param_verify(const struct devlink_param *param)
 }
 
 static int devlink_param_register(struct devlink *devlink,
+				  struct devlink_port *devlink_port,
+				  struct xarray *params_arr,
 				  const struct devlink_param *param)
 {
 	struct devlink_param_item *param_item;
+	enum devlink_command cmd;
 	int err;
 
 	WARN_ON(devlink_param_verify(param));
-	WARN_ON(devlink_param_find_by_name(&devlink->params, param->name));
+	WARN_ON(devlink_param_find_by_name(params_arr, param->name));
 
 	if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
 		WARN_ON(param->get || param->set);
@@ -646,11 +649,13 @@  static int devlink_param_register(struct devlink *devlink,
 
 	param_item->param = param;
 
-	err = xa_insert(&devlink->params, param->id, param_item, GFP_KERNEL);
+	err = xa_insert(params_arr, param->id, param_item, GFP_KERNEL);
 	if (err)
 		goto err_xa_insert;
 
-	devlink_param_notify(devlink, NULL, param_item, DEVLINK_CMD_PARAM_NEW);
+	cmd = devlink_port ? DEVLINK_CMD_PORT_PARAM_NEW : DEVLINK_CMD_PARAM_NEW;
+	devlink_param_notify(devlink, devlink_port, param_item, cmd);
+
 	return 0;
 
 err_xa_insert:
@@ -659,30 +664,28 @@  static int devlink_param_register(struct devlink *devlink,
 }
 
 static void devlink_param_unregister(struct devlink *devlink,
+				     struct devlink_port *devlink_port,
+				     struct xarray *params_arr,
 				     const struct devlink_param *param)
 {
 	struct devlink_param_item *param_item;
+	enum devlink_command cmd;
 
-	param_item = devlink_param_find_by_id(&devlink->params, param->id);
+	param_item = devlink_param_find_by_id(params_arr, param->id);
 	if (WARN_ON(!param_item))
 		return;
-	devlink_param_notify(devlink, NULL, param_item, DEVLINK_CMD_PARAM_DEL);
-	xa_erase(&devlink->params, param->id);
+
+	cmd = devlink_port ? DEVLINK_CMD_PORT_PARAM_DEL : DEVLINK_CMD_PARAM_DEL;
+	devlink_param_notify(devlink, devlink_port, param_item, cmd);
+	xa_erase(params_arr, param->id);
 	kfree(param_item);
 }
 
-/**
- *	devl_params_register - register configuration parameters
- *
- *	@devlink: devlink
- *	@params: configuration parameters array
- *	@params_count: number of parameters provided
- *
- *	Register the configuration parameters supported by the driver.
- */
-int devl_params_register(struct devlink *devlink,
-			 const struct devlink_param *params,
-			 size_t params_count)
+static int __devlink_params_register(struct devlink *devlink,
+				     struct devlink_port *devlink_port,
+				     struct xarray *params_arr,
+				     const struct devlink_param *params,
+				     size_t params_count)
 {
 	const struct devlink_param *param = params;
 	int i, err;
@@ -690,10 +693,12 @@  int devl_params_register(struct devlink *devlink,
 	lockdep_assert_held(&devlink->lock);
 
 	for (i = 0; i < params_count; i++, param++) {
-		err = devlink_param_register(devlink, param);
+		err = devlink_param_register(devlink, devlink_port, params_arr,
+					     param);
 		if (err)
 			goto rollback;
 	}
+
 	return 0;
 
 rollback:
@@ -701,9 +706,28 @@  int devl_params_register(struct devlink *devlink,
 		return err;
 
 	for (param--; i > 0; i--, param--)
-		devlink_param_unregister(devlink, param);
+		devlink_param_unregister(devlink, devlink_port, params_arr,
+					 param);
+
 	return err;
 }
+
+/**
+ *	devl_params_register - register configuration parameters
+ *
+ *	@devlink: devlink
+ *	@params: configuration parameters array
+ *	@params_count: number of parameters provided
+ *
+ *	Register the configuration parameters supported by the driver.
+ */
+int devl_params_register(struct devlink *devlink,
+			 const struct devlink_param *params,
+			 size_t params_count)
+{
+	return __devlink_params_register(devlink, NULL, &devlink->params,
+					 params, params_count);
+}
 EXPORT_SYMBOL_GPL(devl_params_register);
 
 int devlink_params_register(struct devlink *devlink,
@@ -719,6 +743,22 @@  int devlink_params_register(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_params_register);
 
+static void __devlink_params_unregister(struct devlink *devlink,
+					struct devlink_port *devlink_port,
+					struct xarray *params_arr,
+					const struct devlink_param *params,
+					size_t params_count)
+{
+	const struct devlink_param *param = params;
+	int i;
+
+	lockdep_assert_held(&devlink->lock);
+
+	for (i = 0; i < params_count; i++, param++)
+		devlink_param_unregister(devlink, devlink_port, params_arr,
+					 param);
+}
+
 /**
  *	devl_params_unregister - unregister configuration parameters
  *	@devlink: devlink
@@ -729,13 +769,8 @@  void devl_params_unregister(struct devlink *devlink,
 			    const struct devlink_param *params,
 			    size_t params_count)
 {
-	const struct devlink_param *param = params;
-	int i;
-
-	lockdep_assert_held(&devlink->lock);
-
-	for (i = 0; i < params_count; i++, param++)
-		devlink_param_unregister(devlink, param);
+	__devlink_params_unregister(devlink, NULL, &devlink->params,
+				    params, params_count);
 }
 EXPORT_SYMBOL_GPL(devl_params_unregister);
 
@@ -749,6 +784,67 @@  void devlink_params_unregister(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_params_unregister);
 
+/**
+ *	devl_port_params_register - register configuration parameters for port
+ *
+ *	@devlink_port: devlink port
+ *	@params: configuration parameters array
+ *	@params_count: number of parameters provided
+ *
+ *	Register the configuration parameters supported by the driver for the
+ *	specific port.
+ */
+int devl_port_params_register(struct devlink_port *devlink_port,
+			      const struct devlink_param *params,
+			      size_t params_count)
+{
+	return __devlink_params_register(devlink_port->devlink,
+					 devlink_port,
+					 &devlink_port->params,
+					 params, params_count);
+}
+EXPORT_SYMBOL_GPL(devl_port_params_register);
+
+/**
+ *	devl_port_params_unregister - unregister configuration parameters for port
+ *
+ *	@devlink_port: devlink port
+ *	@params: configuration parameters to unregister
+ *	@params_count: number of parameters provided
+ */
+void devl_port_params_unregister(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count)
+{
+	__devlink_params_unregister(devlink_port->devlink, devlink_port,
+				    &devlink_port->params,
+				    params, params_count);
+}
+EXPORT_SYMBOL_GPL(devl_port_params_unregister);
+
+int devlink_port_params_register(struct devlink_port *devlink_port,
+				 const struct devlink_param *params,
+				 size_t params_count)
+{
+	int err;
+
+	devl_lock(devlink_port->devlink);
+	err = devl_port_params_register(devlink_port, params, params_count);
+	devl_unlock(devlink_port->devlink);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_register);
+
+void devlink_port_params_unregister(struct devlink_port *devlink_port,
+				    const struct devlink_param *params,
+				    size_t params_count)
+{
+	devl_lock(devlink_port->devlink);
+	devl_port_params_unregister(devlink_port, params, params_count);
+	devl_unlock(devlink_port->devlink);
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
+
 /**
  *	devl_param_driverinit_value_get - get configuration parameter
  *					  value for driver initializing
diff --git a/net/devlink/port.c b/net/devlink/port.c
index 939081a0e615..39bba3f7a1f9 100644
--- a/net/devlink/port.c
+++ b/net/devlink/port.c
@@ -1075,6 +1075,8 @@  int devl_port_register_with_ops(struct devlink *devlink,
 	devlink_port->registered = true;
 	devlink_port->index = port_index;
 	devlink_port->ops = ops ? ops : &devlink_port_dummy_ops;
+	xa_init_flags(&devlink_port->params, XA_FLAGS_ALLOC);
+
 	spin_lock_init(&devlink_port->type_lock);
 	INIT_LIST_HEAD(&devlink_port->reporter_list);
 	err = xa_insert(&devlink->ports, port_index, devlink_port, GFP_KERNEL);
@@ -1134,6 +1136,7 @@  void devl_port_unregister(struct devlink_port *devlink_port)
 	devlink_port_type_warn_cancel(devlink_port);
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
 	xa_erase(&devlink_port->devlink->ports, devlink_port->index);
+	xa_destroy(&devlink_port->params);
 	WARN_ON(!list_empty(&devlink_port->reporter_list));
 	devlink_port->registered = false;
 }