| Message ID | 20210409162247.4293-1-oleksandr.mazur@plvision.eu (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net: core: devlink: add port_params_ops for devlink port parameters altering | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 480 this patch: 482 |
| netdev/kdoc | success | Errors and warnings before: 16 this patch: 16 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis CHECK: Blank lines aren't necessary after an open brace '{' WARNING: line length of 82 exceeds 80 columns |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 672 this patch: 674 |
| netdev/header_inline | success | Link |
On 4/9/2021 9:22 AM, Oleksandr Mazur wrote: > 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. I also noticed this issue recently when trying to add port parameters and I have a patch that handles this in a different way. The ops in devlink_param struct can be updated to include port_index as an argument devlink: Fix devlink_param function pointers devlink_param function pointers are used to register device parameters as well as port parameters. So we need port_index also as an argument to enable port specific parameters. Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com> diff --git a/include/net/devlink.h b/include/net/devlink.h index 57251d12b0fc..bf55acdf98ae 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -469,12 +469,12 @@ struct devlink_param { bool generic; enum devlink_param_type type; unsigned long supported_cmodes; - int (*get)(struct devlink *devlink, u32 id, - struct devlink_param_gset_ctx *ctx); - int (*set)(struct devlink *devlink, u32 id, - struct devlink_param_gset_ctx *ctx); - int (*validate)(struct devlink *devlink, u32 id, - union devlink_param_value val, + int (*get)(struct devlink *devlink, unsigned int port_index, + u32 id, struct devlink_param_gset_ctx *ctx); + int (*set)(struct devlink *devlink, unsigned int port_index, + u32 id, struct devlink_param_gset_ctx *ctx); + int (*validate)(struct devlink *devlink, unsigned int port_index, + u32 id, union devlink_param_value val, struct netlink_ext_ack *extack); }; diff --git a/net/core/devlink.c b/net/core/devlink.c index 151f60af0c4a..65a819ead3d9 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -3826,21 +3826,23 @@ devlink_param_cmode_is_supported(const struct devlink_param *param, } static int devlink_param_get(struct devlink *devlink, + unsigned int port_index, const struct devlink_param *param, struct devlink_param_gset_ctx *ctx) { if (!param->get) return -EOPNOTSUPP; - return param->get(devlink, param->id, ctx); + return param->get(devlink, port_index, param->id, ctx); } static int devlink_param_set(struct devlink *devlink, + unsigned int port_index, const struct devlink_param *param, struct devlink_param_gset_ctx *ctx) { if (!param->set) return -EOPNOTSUPP; - return param->set(devlink, param->id, ctx); + return param->set(devlink, port_index, param->id, ctx); } static int @@ -3941,7 +3943,8 @@ 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); + err = devlink_param_get(devlink, port_index, param, + &ctx); if (err) return err; param_value[i] = ctx.val; @@ -4216,7 +4219,8 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, if (err) return err; if (param->validate) { - err = param->validate(devlink, param->id, value, info->extack); + err = param->validate(devlink, port_index, param->id, value, + info->extack); if (err) return err; } @@ -4238,7 +4242,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, return -EOPNOTSUPP; ctx.val = value; ctx.cmode = cmode; - err = devlink_param_set(devlink, param, &ctx); + err = devlink_param_set(devlink, port_index, param, &ctx); if (err) return err; } > > 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 <oleksandr.mazur@plvision.eu> > --- > drivers/net/netdevsim/dev.c | 46 +++++++++++++++++++++++++++++++ > drivers/net/netdevsim/netdevsim.h | 1 + > include/net/devlink.h | 11 ++++++++ > net/core/devlink.c | 16 ++++++++++- > 4 files changed, 73 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index 6189a4c0d39e..4f9a3104ca46 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_port_param_set(struct devlink_port *port, u32 id, > + struct devlink_param_gset_ctx *ctx); > +static int nsim_dev_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,7 @@ 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_TEST2, > }; > > static const struct devlink_param nsim_devlink_params[] = { > @@ -349,6 +355,10 @@ static const struct devlink_param nsim_devlink_params[] = { > "test1", DEVLINK_PARAM_TYPE_BOOL, > BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > NULL, NULL, NULL), > + DEVLINK_PARAM_DRIVER(NSIM_DEVLINK_PARAM_ID_TEST2, > + "test1", DEVLINK_PARAM_TYPE_U32, > + BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > + NULL, NULL, NULL), > }; > > static void nsim_devlink_set_params_init_values(struct nsim_dev *nsim_dev, > @@ -892,6 +902,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_port_param_get, > + .set = nsim_dev_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 +920,7 @@ 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, > }; > > #define NSIM_DEV_MAX_MACS_DEFAULT 32 > @@ -1239,6 +1255,36 @@ int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev, > return err; > } > > +static int nsim_dev_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_PARAM_ID_TEST2) { > + ctx->val.vu32 = nsim_port->test_parameter_value; > + return 0; > + } > + > + return -EINVAL; > +} > + > +static int nsim_dev_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_PARAM_ID_TEST2) { > + nsim_port->test_parameter_value = ctx->val.vu32; > + 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..4f5fc491c8d6 100644 > --- a/drivers/net/netdevsim/netdevsim.h > +++ b/drivers/net/netdevsim/netdevsim.h > @@ -203,6 +203,7 @@ struct nsim_dev_port { > unsigned int port_index; > struct dentry *ddir; > struct netdevsim *ns; > + u32 test_parameter_value; > }; > > struct nsim_dev { > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 853420db5d32..85a7b9970496 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -1189,6 +1189,16 @@ enum devlink_trap_group_generic_id { > .min_burst = _min_burst, \ > } > > +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: > @@ -1451,6 +1461,7 @@ struct devlink_ops { > struct devlink_port *port, > enum devlink_port_fn_state state, > struct netlink_ext_ack *extack); > + 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 737b61c2976e..20f3545f4e7b 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -3918,6 +3918,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, > enum devlink_command cmd, > u32 portid, u32 seq, int flags) > { > + struct devlink_port *dl_port; > union devlink_param_value param_value[DEVLINK_PARAM_CMODE_MAX + 1]; > bool param_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {}; > const struct devlink_param *param = param_item->param; > @@ -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) { > + > + 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;
Hi Sridhar, On Fri, Apr 09, 2021 at 09:51:13AM -0700, Samudrala, Sridhar wrote: > On 4/9/2021 9:22 AM, Oleksandr Mazur wrote: > > 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. > > I also noticed this issue recently when trying to add port parameters and > I have a patch that handles this in a different way. The ops in devlink_param > struct can be updated to include port_index as an argument > We were thinking on this direction but rather decided to have more strict cb signature which reflects that we are working with devlink_port only. > devlink: Fix devlink_param function pointers > > devlink_param function pointers are used to register device parameters > as well as port parameters. So we need port_index also as an argument > to enable port specific parameters. > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com> > > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 57251d12b0fc..bf55acdf98ae 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -469,12 +469,12 @@ struct devlink_param { > bool generic; > enum devlink_param_type type; > unsigned long supported_cmodes; > - int (*get)(struct devlink *devlink, u32 id, > - struct devlink_param_gset_ctx *ctx); > - int (*set)(struct devlink *devlink, u32 id, > - struct devlink_param_gset_ctx *ctx); > - int (*validate)(struct devlink *devlink, u32 id, > - union devlink_param_value val, > + int (*get)(struct devlink *devlink, unsigned int port_index, > + u32 id, struct devlink_param_gset_ctx *ctx); > + int (*set)(struct devlink *devlink, unsigned int port_index, > + u32 id, struct devlink_param_gset_ctx *ctx); > + int (*validate)(struct devlink *devlink, unsigned int port_index, > + u32 id, union devlink_param_value val, > struct netlink_ext_ack *extack); > }; > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 151f60af0c4a..65a819ead3d9 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -3826,21 +3826,23 @@ devlink_param_cmode_is_supported(const struct devlink_param *param, > } > > static int devlink_param_get(struct devlink *devlink, > + unsigned int port_index, > const struct devlink_param *param, > struct devlink_param_gset_ctx *ctx) > { > if (!param->get) > return -EOPNOTSUPP; > - return param->get(devlink, param->id, ctx); > + return param->get(devlink, port_index, param->id, ctx); > } > > static int devlink_param_set(struct devlink *devlink, > + unsigned int port_index, > const struct devlink_param *param, > struct devlink_param_gset_ctx *ctx) > { > if (!param->set) > return -EOPNOTSUPP; > - return param->set(devlink, param->id, ctx); > + return param->set(devlink, port_index, param->id, ctx); > } > > static int > @@ -3941,7 +3943,8 @@ 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); > + err = devlink_param_get(devlink, port_index, param, > + &ctx); > if (err) > return err; > param_value[i] = ctx.val; > @@ -4216,7 +4219,8 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, > if (err) > return err; > if (param->validate) { > - err = param->validate(devlink, param->id, value, info->extack); > + err = param->validate(devlink, port_index, param->id, value, > + info->extack); > if (err) > return err; > } > @@ -4238,7 +4242,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink, > return -EOPNOTSUPP; > ctx.val = value; > ctx.cmode = cmode; > - err = devlink_param_set(devlink, param, &ctx); > + err = devlink_param_set(devlink, port_index, param, &ctx); > if (err) > return err; > } > > > > > 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 <oleksandr.mazur@plvision.eu> > > --- > > drivers/net/netdevsim/dev.c | 46 +++++++++++++++++++++++++++++++ > > drivers/net/netdevsim/netdevsim.h | 1 + > > include/net/devlink.h | 11 ++++++++ > > net/core/devlink.c | 16 ++++++++++- > > 4 files changed, 73 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > > index 6189a4c0d39e..4f9a3104ca46 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_port_param_set(struct devlink_port *port, u32 id, > > + struct devlink_param_gset_ctx *ctx); > > +static int nsim_dev_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,7 @@ 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_TEST2, > > }; > > static const struct devlink_param nsim_devlink_params[] = { > > @@ -349,6 +355,10 @@ static const struct devlink_param nsim_devlink_params[] = { > > "test1", DEVLINK_PARAM_TYPE_BOOL, > > BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > > NULL, NULL, NULL), > > + DEVLINK_PARAM_DRIVER(NSIM_DEVLINK_PARAM_ID_TEST2, > > + "test1", DEVLINK_PARAM_TYPE_U32, > > + BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > > + NULL, NULL, NULL), > > }; > > static void nsim_devlink_set_params_init_values(struct nsim_dev *nsim_dev, > > @@ -892,6 +902,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_port_param_get, > > + .set = nsim_dev_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 +920,7 @@ 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, > > }; > > #define NSIM_DEV_MAX_MACS_DEFAULT 32 > > @@ -1239,6 +1255,36 @@ int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev, > > return err; > > } > > +static int nsim_dev_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_PARAM_ID_TEST2) { > > + ctx->val.vu32 = nsim_port->test_parameter_value; > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int nsim_dev_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_PARAM_ID_TEST2) { > > + nsim_port->test_parameter_value = ctx->val.vu32; > > + 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..4f5fc491c8d6 100644 > > --- a/drivers/net/netdevsim/netdevsim.h > > +++ b/drivers/net/netdevsim/netdevsim.h > > @@ -203,6 +203,7 @@ struct nsim_dev_port { > > unsigned int port_index; > > struct dentry *ddir; > > struct netdevsim *ns; > > + u32 test_parameter_value; > > }; > > struct nsim_dev { > > diff --git a/include/net/devlink.h b/include/net/devlink.h > > index 853420db5d32..85a7b9970496 100644 > > --- a/include/net/devlink.h > > +++ b/include/net/devlink.h > > @@ -1189,6 +1189,16 @@ enum devlink_trap_group_generic_id { > > .min_burst = _min_burst, \ > > } > > +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: > > @@ -1451,6 +1461,7 @@ struct devlink_ops { > > struct devlink_port *port, > > enum devlink_port_fn_state state, > > struct netlink_ext_ack *extack); > > + 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 737b61c2976e..20f3545f4e7b 100644 > > --- a/net/core/devlink.c > > +++ b/net/core/devlink.c > > @@ -3918,6 +3918,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, > > enum devlink_command cmd, > > u32 portid, u32 seq, int flags) > > { > > + struct devlink_port *dl_port; > > union devlink_param_value param_value[DEVLINK_PARAM_CMODE_MAX + 1]; > > bool param_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {}; > > const struct devlink_param *param = param_item->param; > > @@ -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) { > > + > > + 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; >
On Fri, 9 Apr 2021 20:01:14 +0300 Vadym Kochan wrote: > On Fri, Apr 09, 2021 at 09:51:13AM -0700, Samudrala, Sridhar wrote: > > On 4/9/2021 9:22 AM, Oleksandr Mazur wrote: > > > 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. > > > > I also noticed this issue recently when trying to add port parameters and > > I have a patch that handles this in a different way. The ops in devlink_param > > struct can be updated to include port_index as an argument > > We were thinking on this direction but rather decided to have more strict > cb signature which reflects that we are working with devlink_port only. +1 for passing the actual pointer
On 4/9/2021 11:37 AM, Jakub Kicinski wrote: > On Fri, 9 Apr 2021 20:01:14 +0300 Vadym Kochan wrote: >> On Fri, Apr 09, 2021 at 09:51:13AM -0700, Samudrala, Sridhar wrote: >>> On 4/9/2021 9:22 AM, Oleksandr Mazur wrote: >>>> 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. >>> I also noticed this issue recently when trying to add port parameters and >>> I have a patch that handles this in a different way. The ops in devlink_param >>> struct can be updated to include port_index as an argument >> We were thinking on this direction but rather decided to have more strict >> cb signature which reflects that we are working with devlink_port only. > +1 for passing the actual pointer OK. This way we don't need to change the existing users of devlink param ops.
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 6189a4c0d39e..4f9a3104ca46 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_port_param_set(struct devlink_port *port, u32 id, + struct devlink_param_gset_ctx *ctx); +static int nsim_dev_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,7 @@ 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_TEST2, }; static const struct devlink_param nsim_devlink_params[] = { @@ -349,6 +355,10 @@ static const struct devlink_param nsim_devlink_params[] = { "test1", DEVLINK_PARAM_TYPE_BOOL, BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), NULL, NULL, NULL), + DEVLINK_PARAM_DRIVER(NSIM_DEVLINK_PARAM_ID_TEST2, + "test1", DEVLINK_PARAM_TYPE_U32, + BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), + NULL, NULL, NULL), }; static void nsim_devlink_set_params_init_values(struct nsim_dev *nsim_dev, @@ -892,6 +902,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_port_param_get, + .set = nsim_dev_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 +920,7 @@ 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, }; #define NSIM_DEV_MAX_MACS_DEFAULT 32 @@ -1239,6 +1255,36 @@ int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev, return err; } +static int nsim_dev_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_PARAM_ID_TEST2) { + ctx->val.vu32 = nsim_port->test_parameter_value; + return 0; + } + + return -EINVAL; +} + +static int nsim_dev_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_PARAM_ID_TEST2) { + nsim_port->test_parameter_value = ctx->val.vu32; + 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..4f5fc491c8d6 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -203,6 +203,7 @@ struct nsim_dev_port { unsigned int port_index; struct dentry *ddir; struct netdevsim *ns; + u32 test_parameter_value; }; struct nsim_dev { diff --git a/include/net/devlink.h b/include/net/devlink.h index 853420db5d32..85a7b9970496 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1189,6 +1189,16 @@ enum devlink_trap_group_generic_id { .min_burst = _min_burst, \ } +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: @@ -1451,6 +1461,7 @@ struct devlink_ops { struct devlink_port *port, enum devlink_port_fn_state state, struct netlink_ext_ack *extack); + 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 737b61c2976e..20f3545f4e7b 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -3918,6 +3918,7 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink, enum devlink_command cmd, u32 portid, u32 seq, int flags) { + struct devlink_port *dl_port; union devlink_param_value param_value[DEVLINK_PARAM_CMODE_MAX + 1]; bool param_value_set[DEVLINK_PARAM_CMODE_MAX + 1] = {}; const struct devlink_param *param = param_item->param; @@ -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) { + + 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;
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 <oleksandr.mazur@plvision.eu> --- drivers/net/netdevsim/dev.c | 46 +++++++++++++++++++++++++++++++ drivers/net/netdevsim/netdevsim.h | 1 + include/net/devlink.h | 11 ++++++++ net/core/devlink.c | 16 ++++++++++- 4 files changed, 73 insertions(+), 1 deletion(-)