Re: [RFC] net: core: devlink: add port_params_ops for devlink port parameters altering
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.
Re: [RFC] net: core: devlink: add port_params_ops for devlink port parameters altering
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
Re: [RFC] net: core: devlink: add port_params_ops for devlink port parameters altering
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
>
> 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, );
> + err = devlink_param_get(devlink, port_index, param,
> + );
> 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, );
> + err = devlink_param_set(devlink, port_index, param, );
> if (err)
> return err;
> }
>
> >
> > This patch aims to show how this can be changed:
> >- introduce structure port_params_ops that has callbacks for
> >
Re: [RFC] net: core: devlink: add port_params_ops for devlink port parameters altering
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
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, );
+ err = devlink_param_get(devlink, port_index, param,
+ );
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, );
+ err = devlink_param_set(devlink, port_index, param, );
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
---
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
[RFC] net: core: devlink: add port_params_ops for devlink port parameters altering
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
---
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 = _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
