[RFC V2] net: core: devlink: add port_params_ops for devlink port parameters altering
From: Oleksandr Mazur <hidden>
Date: 2021-05-12 12:48:36
Also in:
lkml
Subsystem:
devlink, netdevsim, networking drivers, networking [general], the rest · Maintainers:
Jiri Pirko, Jakub Kicinski, Andrew Lunn, "David S. Miller", Eric Dumazet, Paolo Abeni, Linus Torvalds
I'd like to discuss a possibility of handling devlink port parameters
with devlink port pointer supplied.
Current design makes it impossible to distinguish which port's parameter
should get altered (set) or retrieved (get) whenever there's a single
parameter registered within a few ports.
This patch aims to show how this can be changed:
- introduce structure port_params_ops that has callbacks for
get/set/validate;
- if devlink has registered port_params_ops, then upon every devlink
port parameter get/set call invoke port parameters callback
Signed-off-by: Oleksandr Mazur <redacted>
Suggested-by: Vadym Kochan <redacted>
--
V2:
1) devlink:
- add commentary description over devlink_port_param_ops struct.
- add port param ops 'set' usage.
- remove WARN_ON upon registering devlink params in case of
get/set not set.
2) netdevsim:
- dynamically allocate port params, as well as their values.
- add base / max values for port params and devlink
params enums to ease-out iteration over them.
- separate devlink port params IDs and devlink param IDs.
- add statically allocated devlink port params array.
---
drivers/net/netdevsim/dev.c | 85 ++++++++++++++++++++++++++++++-
drivers/net/netdevsim/netdevsim.h | 2 +
include/net/devlink.h | 23 +++++++++
net/core/devlink.c | 46 +++++++++++++----
4 files changed, 145 insertions(+), 11 deletions(-)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 6189a4c0d39e..74a6a6e22119 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c@@ -39,6 +39,11 @@ static struct dentry *nsim_dev_ddir; #define NSIM_DEV_DUMMY_REGION_SIZE (1024 * 32) +static int nsim_dev_devlink_port_param_set(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx); +static int nsim_dev_devlink_port_param_get(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx); + static int nsim_dev_take_snapshot(struct devlink *devlink, const struct devlink_region_ops *ops,
@@ -339,6 +344,17 @@ static int nsim_dev_resources_register(struct devlink *devlink) enum nsim_devlink_param_id { NSIM_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, NSIM_DEVLINK_PARAM_ID_TEST1, + NSIM_DEVLINK_PARAM_ID_MAX, +}; + +#define NSIM_DEV_DEVLINK_PORT_PARAM_NUM \ + (NSIM_DEVLINK_PORT_PARAM_ID_MAX - NSIM_DEVLINK_PORT_PARAM_ID_BASE - 1) + +enum nsim_devlink_port_param_id { + NSIM_DEVLINK_PORT_PARAM_ID_BASE = NSIM_DEVLINK_PARAM_ID_MAX, + NSIM_DEVLINK_PORT_PARAM_ID_TEST_STR, + NSIM_DEVLINK_PORT_PARAM_ID_TEST_BOOL, + NSIM_DEVLINK_PORT_PARAM_ID_MAX, }; static const struct devlink_param nsim_devlink_params[] = {
@@ -892,6 +908,11 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink, return 0; } +static const struct devlink_port_param_ops nsim_dev_port_param_ops = { + .get = nsim_dev_devlink_port_param_get, + .set = nsim_dev_devlink_port_param_set, +}; + static const struct devlink_ops nsim_dev_devlink_ops = { .supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT | DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
@@ -905,6 +926,24 @@ static const struct devlink_ops nsim_dev_devlink_ops = { .trap_group_set = nsim_dev_devlink_trap_group_set, .trap_policer_set = nsim_dev_devlink_trap_policer_set, .trap_policer_counter_get = nsim_dev_devlink_trap_policer_counter_get, + .port_param_ops = &nsim_dev_port_param_ops, +}; + +struct devlink_param nsim_dev_port_params[] = { + { + .id = NSIM_DEVLINK_PORT_PARAM_ID_TEST_STR, + .name = "port_param_test_str", + .generic = false, + .type = DEVLINK_PARAM_TYPE_STRING, + .supported_cmodes = BIT(DEVLINK_PARAM_CMODE_RUNTIME), + }, + { + .id = NSIM_DEVLINK_PORT_PARAM_ID_TEST_BOOL, + .name = "port_param_test_bool", + .generic = false, + .type = DEVLINK_PARAM_TYPE_BOOL, + .supported_cmodes = BIT(DEVLINK_PARAM_CMODE_RUNTIME), + }, }; #define NSIM_DEV_MAX_MACS_DEFAULT 32
@@ -934,10 +973,15 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, if (err) goto err_port_free; - err = nsim_dev_port_debugfs_init(nsim_dev, nsim_dev_port); + err = devlink_port_params_register(devlink_port, nsim_dev_port_params, + ARRAY_SIZE(nsim_dev_port_params)); if (err) goto err_dl_port_unregister; + err = nsim_dev_port_debugfs_init(nsim_dev, nsim_dev_port); + if (err) + goto err_port_params_unregister; + nsim_dev_port->ns = nsim_create(nsim_dev, nsim_dev_port); if (IS_ERR(nsim_dev_port->ns)) { err = PTR_ERR(nsim_dev_port->ns);
@@ -951,6 +995,9 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev, err_port_debugfs_exit: nsim_dev_port_debugfs_exit(nsim_dev_port); +err_port_params_unregister: + devlink_port_params_unregister(devlink_port, nsim_dev_port_params, + ARRAY_SIZE(nsim_dev_port_params)); err_dl_port_unregister: devlink_port_unregister(devlink_port); err_port_free:
@@ -1239,6 +1286,42 @@ int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev, return err; } +static int nsim_dev_devlink_port_param_get(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx) +{ + struct nsim_dev *nsim_dev = devlink_priv(port->devlink); + struct nsim_dev_port *nsim_port = + __nsim_dev_port_lookup(nsim_dev, port->index); + + if (id == NSIM_DEVLINK_PORT_PARAM_ID_TEST_STR) { + strcpy(ctx->val.vstr, nsim_port->devlink_test_param_str); + return 0; + } else if (id == NSIM_DEVLINK_PORT_PARAM_ID_TEST_BOOL) { + ctx->val.vbool = nsim_port->devlink_test_param_bool; + return 0; + } + + return -EINVAL; +} + +static int nsim_dev_devlink_port_param_set(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx) +{ + struct nsim_dev *nsim_dev = devlink_priv(port->devlink); + struct nsim_dev_port *nsim_port = + __nsim_dev_port_lookup(nsim_dev, port->index); + + if (id == NSIM_DEVLINK_PORT_PARAM_ID_TEST_STR) { + strcpy(nsim_port->devlink_test_param_str, ctx->val.vstr); + return 0; + } else if (id == NSIM_DEVLINK_PORT_PARAM_ID_TEST_BOOL) { + nsim_port->devlink_test_param_bool = ctx->val.vbool; + return 0; + } + + return -EINVAL; +} + int nsim_dev_init(void) { nsim_dev_ddir = debugfs_create_dir(DRV_NAME, NULL);
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 7ff24e03577b..4262fd8f0a4e 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h@@ -203,6 +203,8 @@ struct nsim_dev_port { unsigned int port_index; struct dentry *ddir; struct netdevsim *ns; + char devlink_test_param_str[__DEVLINK_PARAM_MAX_STRING_VALUE]; + bool devlink_test_param_bool; }; struct nsim_dev {
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 7c984cadfec4..e9571113df59 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h@@ -1191,6 +1191,28 @@ enum devlink_trap_group_generic_id { .min_burst = _min_burst, \ } +/** + * struct devlink_port_param_ops - devlink port parameters-specific ops + * @get: get port parameter value, used for runtime and permanent + * configuration modes + * @set: set port parameter value, used for runtime and permanent + * configuration modes + * @validate: validate input value is applicable (within value range, etc.) + * + * This struct should be used by the driver register port parameters ops, + * that would be used by the devlink subsystem to fetch (get) or alter (set) + * devlink port parameter. + */ +struct devlink_port_param_ops { + int (*get)(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx); + int (*set)(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx); + int (*validate)(struct devlink_port *port, u32 id, + union devlink_param_value val, + struct netlink_ext_ack *extack); +}; + struct devlink_ops { /** * @supported_flash_update_params:
@@ -1453,6 +1475,7 @@ struct devlink_ops { struct devlink_port *port, enum devlink_port_fn_state state, struct netlink_ext_ack *extack); + const struct devlink_port_param_ops *port_param_ops; }; static inline void *devlink_priv(struct devlink *devlink)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4eb969518ee0..13cd757d879a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c@@ -3923,6 +3923,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, const struct devlink_param *param = param_item->param; struct devlink_param_gset_ctx ctx; struct nlattr *param_values_list; + struct devlink_port *dl_port; struct nlattr *param_attr; int nla_type; void *hdr;
@@ -3941,7 +3942,20 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, if (!param_item->published) continue; ctx.cmode = i; - err = devlink_param_get(devlink, param, &ctx); + if ((cmd == DEVLINK_CMD_PORT_PARAM_GET || + cmd == DEVLINK_CMD_PORT_PARAM_NEW || + cmd == DEVLINK_CMD_PORT_PARAM_DEL) && + devlink->ops->port_param_ops && + devlink->ops->port_param_ops->get) { + dl_port = devlink_port_get_by_index(devlink, + port_index); + err = devlink->ops->port_param_ops->get(dl_port, + param->id, + &ctx); + } else { + err = devlink_param_get(devlink, param, &ctx); + } + if (err) return err; param_value[i] = ctx.val;
@@ -4201,6 +4215,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, struct devlink_param_item *param_item; const struct devlink_param *param; union devlink_param_value value; + struct devlink_port *dl_port; int err = 0; param_item = devlink_param_get_from_info(param_list, info);
@@ -4234,13 +4249,26 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, param_item->driverinit_value = value; param_item->driverinit_value_valid = true; } else { - if (!param->set) - return -EOPNOTSUPP; - ctx.val = value; - ctx.cmode = cmode; - err = devlink_param_set(devlink, param, &ctx); - if (err) - return err; + if (cmd == DEVLINK_CMD_PORT_PARAM_SET && + devlink->ops->port_param_ops && + devlink->ops->port_param_ops->set) { + dl_port = devlink_port_get_by_index(devlink, + port_index); + err = devlink->ops->port_param_ops->set(dl_port, + param->id, + &ctx); + if (err) + return err; + } else { + if (!param->set) + return -EOPNOTSUPP; + + ctx.val = value; + ctx.cmode = cmode; + err = devlink_param_set(devlink, param, &ctx); + if (err) + return err; + } } devlink_param_notify(devlink, port_index, param_item, cmd);
@@ -4269,8 +4297,6 @@ static int devlink_param_register_one(struct devlink *devlink, if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT)) WARN_ON(param->get || param->set); - else - WARN_ON(!param->get || !param->set); param_item = kzalloc(sizeof(*param_item), GFP_KERNEL); if (!param_item)
--
2.17.1