Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister
On Thu, Dec 6, 2018 at 12:43 PM Jiri Pirko wrote:
>
> Thu, Dec 06, 2018 at 07:02:59AM CET, vasundhara-v.vo...@broadcom.com wrote:
> >Thank you reviewing the patches.
> >
> >On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko wrote:
> >>
> >> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.vo...@broadcom.com wrote:
> >> >Add functions to register and unregister for the driver supported
> >> >configuration parameters table per port.
> >> >
> >> >Cc: Jiri Pirko
> >> >Signed-off-by: Vasundhara Volam
> >> >---
> >> > include/net/devlink.h | 29 +++
> >> > net/core/devlink.c| 130
> >> > ++
> >> > 2 files changed, 151 insertions(+), 8 deletions(-)
> >> >
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 67f4293..9b4d80b 100644
> >> >--- a/include/net/devlink.h
> >> >+++ b/include/net/devlink.h
> >> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> >> >
> >> > struct devlink_port {
> >> > struct list_head list;
> >> >+ struct list_head param_list;
> >> > struct devlink *devlink;
> >> > unsigned index;
> >> > bool registered;
> >> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> >> > .validate = _validate, \
> >> > }
> >> >
> >> >+enum devlink_port_param_generic_id {
> >> >+ /* add new param generic ids above here */
> >> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> >> >+ DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> >> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> >> >+};
> >>
> >> I don't see the need for enum just for per-port params. The existing
> >> params enum should be enough.
> >In that case there won't be any differentiation between generic device
> >and port params.
>
> Yes, you don't need that.
Okay, then let me modify and refactor the patchset. Thanks.
>
> >Also, if enum is not needed, then separate struct
> >devlink_port_param_generic is also not needed.
>
> Yep.
>
> >
> >Based on this, entire patchset needs a change.
>
> [...]
Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister
Thu, Dec 06, 2018 at 07:02:59AM CET, vasundhara-v.vo...@broadcom.com wrote:
>Thank you reviewing the patches.
>
>On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko wrote:
>>
>> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.vo...@broadcom.com wrote:
>> >Add functions to register and unregister for the driver supported
>> >configuration parameters table per port.
>> >
>> >Cc: Jiri Pirko
>> >Signed-off-by: Vasundhara Volam
>> >---
>> > include/net/devlink.h | 29 +++
>> > net/core/devlink.c| 130
>> > ++
>> > 2 files changed, 151 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 67f4293..9b4d80b 100644
>> >--- a/include/net/devlink.h
>> >+++ b/include/net/devlink.h
>> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
>> >
>> > struct devlink_port {
>> > struct list_head list;
>> >+ struct list_head param_list;
>> > struct devlink *devlink;
>> > unsigned index;
>> > bool registered;
>> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
>> > .validate = _validate, \
>> > }
>> >
>> >+enum devlink_port_param_generic_id {
>> >+ /* add new param generic ids above here */
>> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
>> >+ DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
>> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
>> >+};
>>
>> I don't see the need for enum just for per-port params. The existing
>> params enum should be enough.
>In that case there won't be any differentiation between generic device
>and port params.
Yes, you don't need that.
>Also, if enum is not needed, then separate struct
>devlink_port_param_generic is also not needed.
Yep.
>
>Based on this, entire patchset needs a change.
[...]
Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister
Thank you reviewing the patches.
On Wed, Dec 5, 2018 at 5:24 PM Jiri Pirko wrote:
>
> Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.vo...@broadcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko
> >Signed-off-by: Vasundhara Volam
> >---
> > include/net/devlink.h | 29 +++
> > net/core/devlink.c| 130
> > ++
> > 2 files changed, 151 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 67f4293..9b4d80b 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -48,6 +48,7 @@ struct devlink_port_attrs {
> >
> > struct devlink_port {
> > struct list_head list;
> >+ struct list_head param_list;
> > struct devlink *devlink;
> > unsigned index;
> > bool registered;
> >@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> > .validate = _validate, \
> > }
> >
> >+enum devlink_port_param_generic_id {
> >+ /* add new param generic ids above here */
> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
> >+ DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
> >+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
> >+};
>
> I don't see the need for enum just for per-port params. The existing
> params enum should be enough.
In that case there won't be any differentiation between generic device
and port params.
Also, if enum is not needed, then separate struct
devlink_port_param_generic is also not needed.
Based on this, entire patchset needs a change.
>
>
> >+
> > struct devlink_region;
> >
> > typedef void devlink_snapshot_data_dest_t(const void *data);
> >@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink
> >*devlink, u32 param_id,
> > void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> > void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> > const char *src);
> >+int devlink_port_params_register(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);
> > struct devlink_region *devlink_region_create(struct devlink *devlink,
> >const char *region_name,
> >u32 region_max_snapshots,
> >@@ -816,6 +830,21 @@ static inline bool
> >devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> > {
> > }
> >
> >+static inline int
> >+devlink_port_params_register(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ return 0;
> >+}
> >+
> >+static inline void
> >+devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+}
> >+
> > static inline struct devlink_region *
> > devlink_region_create(struct devlink *devlink,
> > const char *region_name,
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index abb0da9..e194913 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct
> >sk_buff *skb,
> > }
> >
> > static int devlink_param_register_one(struct devlink *devlink,
> >+struct list_head *param_list,
> > const struct devlink_param *param)
> > {
> > struct devlink_param_item *param_item;
> >
> >- if (devlink_param_find_by_name(>param_list,
> >- param->name))
> >+ if (devlink_param_find_by_name(param_list, param->name))
> > return -EEXIST;
> >
> > if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
> >@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink
> >*devlink,
> > return -ENOMEM;
> > param_item->param = param;
> >
> >- list_add_tail(_item->list, >param_list);
> >+ list_add_tail(_item->list, param_list);
> > devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
> > return 0;
> > }
> >
> > static void devlink_param_unregister_one(struct devlink *devlink,
> >+ struct list_head *param_list,
> >const struct devlink_param *param)
> > {
> > struct devlink_param_item *param_item;
> >
> >- param_item = devlink_param_find_by_name(>param_list,
> >-
Re: [PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister
Wed, Dec 05, 2018 at 06:56:54AM CET, vasundhara-v.vo...@broadcom.com wrote:
>Add functions to register and unregister for the driver supported
>configuration parameters table per port.
>
>Cc: Jiri Pirko
>Signed-off-by: Vasundhara Volam
>---
> include/net/devlink.h | 29 +++
> net/core/devlink.c| 130 ++
> 2 files changed, 151 insertions(+), 8 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 67f4293..9b4d80b 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -48,6 +48,7 @@ struct devlink_port_attrs {
>
> struct devlink_port {
> struct list_head list;
>+ struct list_head param_list;
> struct devlink *devlink;
> unsigned index;
> bool registered;
>@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
> .validate = _validate, \
> }
>
>+enum devlink_port_param_generic_id {
>+ /* add new param generic ids above here */
>+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
>+ DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
>+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
>+};
I don't see the need for enum just for per-port params. The existing
params enum should be enough.
>+
> struct devlink_region;
>
> typedef void devlink_snapshot_data_dest_t(const void *data);
>@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink
>*devlink, u32 param_id,
> void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
> void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> const char *src);
>+int devlink_port_params_register(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);
> struct devlink_region *devlink_region_create(struct devlink *devlink,
>const char *region_name,
>u32 region_max_snapshots,
>@@ -816,6 +830,21 @@ static inline bool
>devlink_dpipe_table_counter_enabled(struct devlink *devlink,
> {
> }
>
>+static inline int
>+devlink_port_params_register(struct devlink_port *devlink_port,
>+ const struct devlink_param *params,
>+ size_t params_count)
>+{
>+ return 0;
>+}
>+
>+static inline void
>+devlink_port_params_unregister(struct devlink_port *devlink_port,
>+ const struct devlink_param *params,
>+ size_t params_count)
>+{
>+}
>+
> static inline struct devlink_region *
> devlink_region_create(struct devlink *devlink,
> const char *region_name,
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index abb0da9..e194913 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct
>sk_buff *skb,
> }
>
> static int devlink_param_register_one(struct devlink *devlink,
>+struct list_head *param_list,
> const struct devlink_param *param)
> {
> struct devlink_param_item *param_item;
>
>- if (devlink_param_find_by_name(>param_list,
>- param->name))
>+ if (devlink_param_find_by_name(param_list, param->name))
> return -EEXIST;
>
> if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
>@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink
>*devlink,
> return -ENOMEM;
> param_item->param = param;
>
>- list_add_tail(_item->list, >param_list);
>+ list_add_tail(_item->list, param_list);
> devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
> return 0;
> }
>
> static void devlink_param_unregister_one(struct devlink *devlink,
>+ struct list_head *param_list,
>const struct devlink_param *param)
> {
> struct devlink_param_item *param_item;
>
>- param_item = devlink_param_find_by_name(>param_list,
>- param->name);
>+ param_item = devlink_param_find_by_name(param_list, param->name);
> WARN_ON(!param_item);
> devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
> list_del(_item->list);
> kfree(param_item);
> }
>
>+static const struct devlink_param devlink_port_param_generic[] = {};
>+
>+static int devlink_port_param_generic_verify(const struct devlink_param
>*param)
>+{
>+ /* verify it matches generic parameter by id and name */
>+ if (param->id >
[PATCH net-next RFC 1/7] devlink: Add devlink_param for port register and unregister
Add functions to register and unregister for the driver supported
configuration parameters table per port.
Cc: Jiri Pirko
Signed-off-by: Vasundhara Volam
---
include/net/devlink.h | 29 +++
net/core/devlink.c| 130 ++
2 files changed, 151 insertions(+), 8 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 67f4293..9b4d80b 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -48,6 +48,7 @@ struct devlink_port_attrs {
struct devlink_port {
struct list_head list;
+ struct list_head param_list;
struct devlink *devlink;
unsigned index;
bool registered;
@@ -419,6 +420,13 @@ enum devlink_param_generic_id {
.validate = _validate, \
}
+enum devlink_port_param_generic_id {
+ /* add new param generic ids above here */
+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX,
+ DEVLINK_PORT_PARAM_GENERIC_ID_MAX =
+ __DEVLINK_PORT_PARAM_GENERIC_ID_MAX - 1,
+};
+
struct devlink_region;
typedef void devlink_snapshot_data_dest_t(const void *data);
@@ -574,6 +582,12 @@ int devlink_param_driverinit_value_set(struct devlink
*devlink, u32 param_id,
void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
void devlink_param_value_str_fill(union devlink_param_value *dst_val,
const char *src);
+int devlink_port_params_register(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);
struct devlink_region *devlink_region_create(struct devlink *devlink,
const char *region_name,
u32 region_max_snapshots,
@@ -816,6 +830,21 @@ static inline bool
devlink_dpipe_table_counter_enabled(struct devlink *devlink,
{
}
+static inline int
+devlink_port_params_register(struct devlink_port *devlink_port,
+const struct devlink_param *params,
+size_t params_count)
+{
+ return 0;
+}
+
+static inline void
+devlink_port_params_unregister(struct devlink_port *devlink_port,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+}
+
static inline struct devlink_region *
devlink_region_create(struct devlink *devlink,
const char *region_name,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index abb0da9..e194913 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3147,12 +3147,12 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff
*skb,
}
static int devlink_param_register_one(struct devlink *devlink,
+ struct list_head *param_list,
const struct devlink_param *param)
{
struct devlink_param_item *param_item;
- if (devlink_param_find_by_name(>param_list,
- param->name))
+ if (devlink_param_find_by_name(param_list, param->name))
return -EEXIST;
if (param->supported_cmodes == BIT(DEVLINK_PARAM_CMODE_DRIVERINIT))
@@ -3165,24 +3165,54 @@ static int devlink_param_register_one(struct devlink
*devlink,
return -ENOMEM;
param_item->param = param;
- list_add_tail(_item->list, >param_list);
+ list_add_tail(_item->list, param_list);
devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
return 0;
}
static void devlink_param_unregister_one(struct devlink *devlink,
+struct list_head *param_list,
const struct devlink_param *param)
{
struct devlink_param_item *param_item;
- param_item = devlink_param_find_by_name(>param_list,
- param->name);
+ param_item = devlink_param_find_by_name(param_list, param->name);
WARN_ON(!param_item);
devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_DEL);
list_del(_item->list);
kfree(param_item);
}
+static const struct devlink_param devlink_port_param_generic[] = {};
+
+static int devlink_port_param_generic_verify(const struct devlink_param *param)
+{
+ /* verify it matches generic parameter by id and name */
+ if (param->id > DEVLINK_PORT_PARAM_GENERIC_ID_MAX)
+ return -EINVAL;
+ if (strcmp(param->name, devlink_port_param_generic[param->id].name))
+ return -ENOENT;
+
+ WARN_ON(param->type != devlink_port_param_generic[param->id].type);
+
+
