* [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 9:02 ` Jiri Pirko
2018-12-11 8:46 ` [PATCH net-next v2 RFC 2/8] devlink: Add port param get command Vasundhara Volam
` (6 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add functions to register and unregister for the driver supported
configuration parameters table per port.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/net/devlink.h | 22 +++++++++
net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 124 insertions(+), 26 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 67f4293..98b8a66 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;
@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
return -ENOMEM;
param_item->param = param;
- list_add_tail(¶m_item->list, &devlink->param_list);
+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
devlink_port->index = port_index;
devlink_port->registered = true;
list_add_tail(&devlink_port->list, &devlink->port_list);
+ INIT_LIST_HEAD(&devlink_port->param_list);
mutex_unlock(&devlink->lock);
devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
return 0;
@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
}
EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
+static int devlink_param_verify(const struct devlink_param *param)
+{
+ if (!param || !param->name || !param->supported_cmodes)
+ return -EINVAL;
+ if (param->generic)
+ return devlink_param_generic_verify(param);
+ else
+ return devlink_param_driver_verify(param);
+}
+
/**
* devlink_params_register - register configuration parameters
*
@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
mutex_lock(&devlink->lock);
for (i = 0; i < params_count; i++, param++) {
- if (!param || !param->name || !param->supported_cmodes) {
- err = -EINVAL;
+ err = devlink_param_verify(param);
+ if (err)
goto rollback;
- }
- if (param->generic) {
- err = devlink_param_generic_verify(param);
- if (err)
- goto rollback;
- } else {
- err = devlink_param_driver_verify(param);
- if (err)
- goto rollback;
- }
- err = devlink_param_register_one(devlink, param);
+
+ err = devlink_param_register_one(devlink, &devlink->param_list,
+ param);
if (err)
goto rollback;
}
@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
if (!i)
goto unlock;
for (param--; i > 0; i--, param--)
- devlink_param_unregister_one(devlink, param);
+ devlink_param_unregister_one(devlink, &devlink->param_list,
+ param);
unlock:
mutex_unlock(&devlink->lock);
return err;
}
EXPORT_SYMBOL_GPL(devlink_params_register);
+static void __devlink_params_unregister(struct devlink *devlink,
+ struct list_head *param_list,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+ const struct devlink_param *param = params;
+ int i;
+
+ mutex_lock(&devlink->lock);
+ for (i = 0; i < params_count; i++, param++)
+ devlink_param_unregister_one(devlink, param_list, param);
+ mutex_unlock(&devlink->lock);
+}
+
/**
* devlink_params_unregister - unregister configuration parameters
* @devlink: devlink
@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
const struct devlink_param *params,
size_t params_count)
{
- const struct devlink_param *param = params;
- int i;
-
- mutex_lock(&devlink->lock);
- for (i = 0; i < params_count; i++, param++)
- devlink_param_unregister_one(devlink, param);
- mutex_unlock(&devlink->lock);
+ return __devlink_params_unregister(devlink, &devlink->param_list,
+ params, params_count);
}
EXPORT_SYMBOL_GPL(devlink_params_unregister);
@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
/**
+ * devlink_port_params_register - register port configuration parameters
+ *
+ * @devlink_port: devlink port
+ * @params: configuration parameters array
+ * @params_count: number of parameters provided
+ *
+ * Register the configuration parameters supported by the port.
+ */
+int devlink_port_params_register(struct devlink_port *devlink_port,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+ struct devlink *devlink = devlink_port->devlink;
+ const struct devlink_param *param = params;
+ int i, err;
+
+ mutex_lock(&devlink->lock);
+ for (i = 0; i < params_count; i++) {
+ err = devlink_param_verify(param);
+ if (err)
+ goto rollback;
+
+ err = devlink_param_register_one(devlink,
+ &devlink_port->param_list,
+ param);
+ if (err)
+ goto rollback;
+ }
+
+ mutex_unlock(&devlink->lock);
+ return 0;
+
+rollback:
+ if (!i)
+ goto unlock;
+ for (param--; i > 0; i--, param--)
+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
+ param);
+unlock:
+ mutex_unlock(&devlink->lock);
+ return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_register);
+
+/**
+ * devlink_port_params_unregister - unregister port configuration
+ * parameters
+ *
+ * @devlink_port: devlink port
+ * @params: configuration parameters array
+ * @params_count: number of parameters provided
+ */
+void devlink_port_params_unregister(struct devlink_port *devlink_port,
+ const struct devlink_param *params,
+ size_t params_count)
+{
+ return __devlink_params_unregister(devlink_port->devlink,
+ &devlink_port->param_list,
+ params, params_count);
+}
+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
+
+/**
* devlink_region_create - create a new address region
*
* @devlink: devlink
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-11 8:46 ` [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister Vasundhara Volam
@ 2018-12-11 9:02 ` Jiri Pirko
2018-12-11 10:00 ` Vasundhara Volam
2018-12-12 13:43 ` Vasundhara Volam
0 siblings, 2 replies; 25+ messages in thread
From: Jiri Pirko @ 2018-12-11 9:02 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, michael.chan, jiri, jakub.kicinski, netdev
Tue, Dec 11, 2018 at 09:46:41AM CET, vasundhara-v.volam@broadcom.com wrote:
>Add functions to register and unregister for the driver supported
>configuration parameters table per port.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h | 22 +++++++++
> net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
> 2 files changed, 124 insertions(+), 26 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 67f4293..98b8a66 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;
>@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
> return -ENOMEM;
> param_item->param = param;
>
>- list_add_tail(¶m_item->list, &devlink->param_list);
>+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
>@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
> devlink_port->index = port_index;
> devlink_port->registered = true;
> list_add_tail(&devlink_port->list, &devlink->port_list);
>+ INIT_LIST_HEAD(&devlink_port->param_list);
> mutex_unlock(&devlink->lock);
> devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
> return 0;
>@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
> }
> EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
>
>+static int devlink_param_verify(const struct devlink_param *param)
>+{
>+ if (!param || !param->name || !param->supported_cmodes)
>+ return -EINVAL;
>+ if (param->generic)
>+ return devlink_param_generic_verify(param);
>+ else
>+ return devlink_param_driver_verify(param);
>+}
>+
> /**
> * devlink_params_register - register configuration parameters
> *
>@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
>
> mutex_lock(&devlink->lock);
> for (i = 0; i < params_count; i++, param++) {
>- if (!param || !param->name || !param->supported_cmodes) {
>- err = -EINVAL;
>+ err = devlink_param_verify(param);
>+ if (err)
> goto rollback;
>- }
>- if (param->generic) {
>- err = devlink_param_generic_verify(param);
>- if (err)
>- goto rollback;
>- } else {
>- err = devlink_param_driver_verify(param);
>- if (err)
>- goto rollback;
>- }
>- err = devlink_param_register_one(devlink, param);
>+
>+ err = devlink_param_register_one(devlink, &devlink->param_list,
>+ param);
> if (err)
> goto rollback;
> }
>@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
> if (!i)
> goto unlock;
> for (param--; i > 0; i--, param--)
>- devlink_param_unregister_one(devlink, param);
>+ devlink_param_unregister_one(devlink, &devlink->param_list,
>+ param);
> unlock:
> mutex_unlock(&devlink->lock);
> return err;
> }
> EXPORT_SYMBOL_GPL(devlink_params_register);
>
>+static void __devlink_params_unregister(struct devlink *devlink,
>+ struct list_head *param_list,
>+ const struct devlink_param *params,
>+ size_t params_count)
>+{
>+ const struct devlink_param *param = params;
>+ int i;
>+
>+ mutex_lock(&devlink->lock);
>+ for (i = 0; i < params_count; i++, param++)
>+ devlink_param_unregister_one(devlink, param_list, param);
>+ mutex_unlock(&devlink->lock);
>+}
Please do devlink_params_register in a similar way. Have
__devlink_params_register() with list_head * arg
>+
> /**
> * devlink_params_unregister - unregister configuration parameters
> * @devlink: devlink
>@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
> const struct devlink_param *params,
> size_t params_count)
> {
>- const struct devlink_param *param = params;
>- int i;
>-
>- mutex_lock(&devlink->lock);
>- for (i = 0; i < params_count; i++, param++)
>- devlink_param_unregister_one(devlink, param);
>- mutex_unlock(&devlink->lock);
>+ return __devlink_params_unregister(devlink, &devlink->param_list,
>+ params, params_count);
> }
> EXPORT_SYMBOL_GPL(devlink_params_unregister);
>
>@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
>
> /**
>+ * devlink_port_params_register - register port configuration parameters
>+ *
>+ * @devlink_port: devlink port
>+ * @params: configuration parameters array
>+ * @params_count: number of parameters provided
>+ *
>+ * Register the configuration parameters supported by the port.
>+ */
>+int devlink_port_params_register(struct devlink_port *devlink_port,
>+ const struct devlink_param *params,
>+ size_t params_count)
>+{
>+ struct devlink *devlink = devlink_port->devlink;
>+ const struct devlink_param *param = params;
>+ int i, err;
>+
>+ mutex_lock(&devlink->lock);
>+ for (i = 0; i < params_count; i++) {
>+ err = devlink_param_verify(param);
>+ if (err)
>+ goto rollback;
>+
>+ err = devlink_param_register_one(devlink,
>+ &devlink_port->param_list,
>+ param);
>+ if (err)
>+ goto rollback;
>+ }
>+
>+ mutex_unlock(&devlink->lock);
>+ return 0;
>+
>+rollback:
>+ if (!i)
>+ goto unlock;
>+ for (param--; i > 0; i--, param--)
>+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
>+ param);
>+unlock:
>+ mutex_unlock(&devlink->lock);
>+ return err;
>+}
>+EXPORT_SYMBOL_GPL(devlink_port_params_register);
>+
>+/**
>+ * devlink_port_params_unregister - unregister port configuration
>+ * parameters
>+ *
>+ * @devlink_port: devlink port
>+ * @params: configuration parameters array
>+ * @params_count: number of parameters provided
>+ */
>+void devlink_port_params_unregister(struct devlink_port *devlink_port,
>+ const struct devlink_param *params,
>+ size_t params_count)
>+{
>+ return __devlink_params_unregister(devlink_port->devlink,
>+ &devlink_port->param_list,
>+ params, params_count);
>+}
>+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
>+
>+/**
> * devlink_region_create - create a new address region
> *
> * @devlink: devlink
>--
>1.8.3.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-11 9:02 ` Jiri Pirko
@ 2018-12-11 10:00 ` Vasundhara Volam
2018-12-12 13:43 ` Vasundhara Volam
1 sibling, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 10:00 UTC (permalink / raw)
To: Jiří Pírko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Dec 11, 2018 at 09:46:41AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h | 22 +++++++++
> > net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
> > 2 files changed, 124 insertions(+), 26 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 67f4293..98b8a66 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;
> >@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
> > return -ENOMEM;
> > param_item->param = param;
> >
> >- list_add_tail(¶m_item->list, &devlink->param_list);
> >+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
> >@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
> > devlink_port->index = port_index;
> > devlink_port->registered = true;
> > list_add_tail(&devlink_port->list, &devlink->port_list);
> >+ INIT_LIST_HEAD(&devlink_port->param_list);
> > mutex_unlock(&devlink->lock);
> > devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
> > return 0;
> >@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
> > }
> > EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
> >
> >+static int devlink_param_verify(const struct devlink_param *param)
> >+{
> >+ if (!param || !param->name || !param->supported_cmodes)
> >+ return -EINVAL;
> >+ if (param->generic)
> >+ return devlink_param_generic_verify(param);
> >+ else
> >+ return devlink_param_driver_verify(param);
> >+}
> >+
> > /**
> > * devlink_params_register - register configuration parameters
> > *
> >@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
> >
> > mutex_lock(&devlink->lock);
> > for (i = 0; i < params_count; i++, param++) {
> >- if (!param || !param->name || !param->supported_cmodes) {
> >- err = -EINVAL;
> >+ err = devlink_param_verify(param);
> >+ if (err)
> > goto rollback;
> >- }
> >- if (param->generic) {
> >- err = devlink_param_generic_verify(param);
> >- if (err)
> >- goto rollback;
> >- } else {
> >- err = devlink_param_driver_verify(param);
> >- if (err)
> >- goto rollback;
> >- }
> >- err = devlink_param_register_one(devlink, param);
> >+
> >+ err = devlink_param_register_one(devlink, &devlink->param_list,
> >+ param);
> > if (err)
> > goto rollback;
> > }
> >@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
> > if (!i)
> > goto unlock;
> > for (param--; i > 0; i--, param--)
> >- devlink_param_unregister_one(devlink, param);
> >+ devlink_param_unregister_one(devlink, &devlink->param_list,
> >+ param);
> > unlock:
> > mutex_unlock(&devlink->lock);
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_register);
> >
> >+static void __devlink_params_unregister(struct devlink *devlink,
> >+ struct list_head *param_list,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ const struct devlink_param *param = params;
> >+ int i;
> >+
> >+ mutex_lock(&devlink->lock);
> >+ for (i = 0; i < params_count; i++, param++)
> >+ devlink_param_unregister_one(devlink, param_list, param);
> >+ mutex_unlock(&devlink->lock);
> >+}
>
> Please do devlink_params_register in a similar way. Have
> __devlink_params_register() with list_head * arg
Okay, but if you see whole patchset, more arguments(nearly 6) are
needed similar to
__devlink_params_unregister().
>
>
> >+
> > /**
> > * devlink_params_unregister - unregister configuration parameters
> > * @devlink: devlink
> >@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
> > const struct devlink_param *params,
> > size_t params_count)
> > {
> >- const struct devlink_param *param = params;
> >- int i;
> >-
> >- mutex_lock(&devlink->lock);
> >- for (i = 0; i < params_count; i++, param++)
> >- devlink_param_unregister_one(devlink, param);
> >- mutex_unlock(&devlink->lock);
> >+ return __devlink_params_unregister(devlink, &devlink->param_list,
> >+ params, params_count);
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
> >
> >@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> >
> > /**
> >+ * devlink_port_params_register - register port configuration parameters
> >+ *
> >+ * @devlink_port: devlink port
> >+ * @params: configuration parameters array
> >+ * @params_count: number of parameters provided
> >+ *
> >+ * Register the configuration parameters supported by the port.
> >+ */
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ struct devlink *devlink = devlink_port->devlink;
> >+ const struct devlink_param *param = params;
> >+ int i, err;
> >+
> >+ mutex_lock(&devlink->lock);
> >+ for (i = 0; i < params_count; i++) {
> >+ err = devlink_param_verify(param);
> >+ if (err)
> >+ goto rollback;
> >+
> >+ err = devlink_param_register_one(devlink,
> >+ &devlink_port->param_list,
> >+ param);
> >+ if (err)
> >+ goto rollback;
> >+ }
> >+
> >+ mutex_unlock(&devlink->lock);
> >+ return 0;
> >+
> >+rollback:
> >+ if (!i)
> >+ goto unlock;
> >+ for (param--; i > 0; i--, param--)
> >+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
> >+ param);
> >+unlock:
> >+ mutex_unlock(&devlink->lock);
> >+ return err;
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
> >+
> >+/**
> >+ * devlink_port_params_unregister - unregister port configuration
> >+ * parameters
> >+ *
> >+ * @devlink_port: devlink port
> >+ * @params: configuration parameters array
> >+ * @params_count: number of parameters provided
> >+ */
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ return __devlink_params_unregister(devlink_port->devlink,
> >+ &devlink_port->param_list,
> >+ params, params_count);
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
> >+
> >+/**
> > * devlink_region_create - create a new address region
> > *
> > * @devlink: devlink
> >--
> >1.8.3.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-11 9:02 ` Jiri Pirko
2018-12-11 10:00 ` Vasundhara Volam
@ 2018-12-12 13:43 ` Vasundhara Volam
2018-12-13 12:54 ` Jiri Pirko
1 sibling, 1 reply; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-12 13:43 UTC (permalink / raw)
To: Jiří Pírko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Dec 11, 2018 at 09:46:41AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Add functions to register and unregister for the driver supported
> >configuration parameters table per port.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h | 22 +++++++++
> > net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
> > 2 files changed, 124 insertions(+), 26 deletions(-)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index 67f4293..98b8a66 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;
> >@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
> > return -ENOMEM;
> > param_item->param = param;
> >
> >- list_add_tail(¶m_item->list, &devlink->param_list);
> >+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
> >@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
> > devlink_port->index = port_index;
> > devlink_port->registered = true;
> > list_add_tail(&devlink_port->list, &devlink->port_list);
> >+ INIT_LIST_HEAD(&devlink_port->param_list);
> > mutex_unlock(&devlink->lock);
> > devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
> > return 0;
> >@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
> > }
> > EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
> >
> >+static int devlink_param_verify(const struct devlink_param *param)
> >+{
> >+ if (!param || !param->name || !param->supported_cmodes)
> >+ return -EINVAL;
> >+ if (param->generic)
> >+ return devlink_param_generic_verify(param);
> >+ else
> >+ return devlink_param_driver_verify(param);
> >+}
> >+
> > /**
> > * devlink_params_register - register configuration parameters
> > *
> >@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
> >
> > mutex_lock(&devlink->lock);
> > for (i = 0; i < params_count; i++, param++) {
> >- if (!param || !param->name || !param->supported_cmodes) {
> >- err = -EINVAL;
> >+ err = devlink_param_verify(param);
> >+ if (err)
> > goto rollback;
> >- }
> >- if (param->generic) {
> >- err = devlink_param_generic_verify(param);
> >- if (err)
> >- goto rollback;
> >- } else {
> >- err = devlink_param_driver_verify(param);
> >- if (err)
> >- goto rollback;
> >- }
> >- err = devlink_param_register_one(devlink, param);
> >+
> >+ err = devlink_param_register_one(devlink, &devlink->param_list,
> >+ param);
> > if (err)
> > goto rollback;
> > }
> >@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
> > if (!i)
> > goto unlock;
> > for (param--; i > 0; i--, param--)
> >- devlink_param_unregister_one(devlink, param);
> >+ devlink_param_unregister_one(devlink, &devlink->param_list,
> >+ param);
> > unlock:
> > mutex_unlock(&devlink->lock);
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_register);
> >
> >+static void __devlink_params_unregister(struct devlink *devlink,
> >+ struct list_head *param_list,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ const struct devlink_param *param = params;
> >+ int i;
> >+
> >+ mutex_lock(&devlink->lock);
> >+ for (i = 0; i < params_count; i++, param++)
> >+ devlink_param_unregister_one(devlink, param_list, param);
> >+ mutex_unlock(&devlink->lock);
> >+}
>
> Please do devlink_params_register in a similar way. Have
> __devlink_params_register() with list_head * arg
This cannot be optimised as you see the whole patch-set, unregister
also requires different command in case of failure of
param_register_one and sending 2 types of command as argument will not
simplify this code. It is better this way, as you see the final
function.
>
>
> >+
> > /**
> > * devlink_params_unregister - unregister configuration parameters
> > * @devlink: devlink
> >@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
> > const struct devlink_param *params,
> > size_t params_count)
> > {
> >- const struct devlink_param *param = params;
> >- int i;
> >-
> >- mutex_lock(&devlink->lock);
> >- for (i = 0; i < params_count; i++, param++)
> >- devlink_param_unregister_one(devlink, param);
> >- mutex_unlock(&devlink->lock);
> >+ return __devlink_params_unregister(devlink, &devlink->param_list,
> >+ params, params_count);
> > }
> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
> >
> >@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> >
> > /**
> >+ * devlink_port_params_register - register port configuration parameters
> >+ *
> >+ * @devlink_port: devlink port
> >+ * @params: configuration parameters array
> >+ * @params_count: number of parameters provided
> >+ *
> >+ * Register the configuration parameters supported by the port.
> >+ */
> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ struct devlink *devlink = devlink_port->devlink;
> >+ const struct devlink_param *param = params;
> >+ int i, err;
> >+
> >+ mutex_lock(&devlink->lock);
> >+ for (i = 0; i < params_count; i++) {
> >+ err = devlink_param_verify(param);
> >+ if (err)
> >+ goto rollback;
> >+
> >+ err = devlink_param_register_one(devlink,
> >+ &devlink_port->param_list,
> >+ param);
> >+ if (err)
> >+ goto rollback;
> >+ }
> >+
> >+ mutex_unlock(&devlink->lock);
> >+ return 0;
> >+
> >+rollback:
> >+ if (!i)
> >+ goto unlock;
> >+ for (param--; i > 0; i--, param--)
> >+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
> >+ param);
> >+unlock:
> >+ mutex_unlock(&devlink->lock);
> >+ return err;
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
> >+
> >+/**
> >+ * devlink_port_params_unregister - unregister port configuration
> >+ * parameters
> >+ *
> >+ * @devlink_port: devlink port
> >+ * @params: configuration parameters array
> >+ * @params_count: number of parameters provided
> >+ */
> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >+ const struct devlink_param *params,
> >+ size_t params_count)
> >+{
> >+ return __devlink_params_unregister(devlink_port->devlink,
> >+ &devlink_port->param_list,
> >+ params, params_count);
> >+}
> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
> >+
> >+/**
> > * devlink_region_create - create a new address region
> > *
> > * @devlink: devlink
> >--
> >1.8.3.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-12 13:43 ` Vasundhara Volam
@ 2018-12-13 12:54 ` Jiri Pirko
2018-12-14 4:52 ` Vasundhara Volam
0 siblings, 1 reply; 25+ messages in thread
From: Jiri Pirko @ 2018-12-13 12:54 UTC (permalink / raw)
To: Vasundhara Volam
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
Wed, Dec 12, 2018 at 02:43:14PM CET, vasundhara-v.volam@broadcom.com wrote:
>On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Tue, Dec 11, 2018 at 09:46:41AM CET, vasundhara-v.volam@broadcom.com wrote:
>> >Add functions to register and unregister for the driver supported
>> >configuration parameters table per port.
>> >
>> >Cc: Jiri Pirko <jiri@mellanox.com>
>> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> >---
>> > include/net/devlink.h | 22 +++++++++
>> > net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
>> > 2 files changed, 124 insertions(+), 26 deletions(-)
>> >
>> >diff --git a/include/net/devlink.h b/include/net/devlink.h
>> >index 67f4293..98b8a66 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;
>> >@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
>> > return -ENOMEM;
>> > param_item->param = param;
>> >
>> >- list_add_tail(¶m_item->list, &devlink->param_list);
>> >+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
>> >@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
>> > devlink_port->index = port_index;
>> > devlink_port->registered = true;
>> > list_add_tail(&devlink_port->list, &devlink->port_list);
>> >+ INIT_LIST_HEAD(&devlink_port->param_list);
>> > mutex_unlock(&devlink->lock);
>> > devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
>> > return 0;
>> >@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
>> > }
>> > EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
>> >
>> >+static int devlink_param_verify(const struct devlink_param *param)
>> >+{
>> >+ if (!param || !param->name || !param->supported_cmodes)
>> >+ return -EINVAL;
>> >+ if (param->generic)
>> >+ return devlink_param_generic_verify(param);
>> >+ else
>> >+ return devlink_param_driver_verify(param);
>> >+}
>> >+
>> > /**
>> > * devlink_params_register - register configuration parameters
>> > *
>> >@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
>> >
>> > mutex_lock(&devlink->lock);
>> > for (i = 0; i < params_count; i++, param++) {
>> >- if (!param || !param->name || !param->supported_cmodes) {
>> >- err = -EINVAL;
>> >+ err = devlink_param_verify(param);
>> >+ if (err)
>> > goto rollback;
>> >- }
>> >- if (param->generic) {
>> >- err = devlink_param_generic_verify(param);
>> >- if (err)
>> >- goto rollback;
>> >- } else {
>> >- err = devlink_param_driver_verify(param);
>> >- if (err)
>> >- goto rollback;
>> >- }
>> >- err = devlink_param_register_one(devlink, param);
>> >+
>> >+ err = devlink_param_register_one(devlink, &devlink->param_list,
>> >+ param);
>> > if (err)
>> > goto rollback;
>> > }
>> >@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
>> > if (!i)
>> > goto unlock;
>> > for (param--; i > 0; i--, param--)
>> >- devlink_param_unregister_one(devlink, param);
>> >+ devlink_param_unregister_one(devlink, &devlink->param_list,
>> >+ param);
>> > unlock:
>> > mutex_unlock(&devlink->lock);
>> > return err;
>> > }
>> > EXPORT_SYMBOL_GPL(devlink_params_register);
>> >
>> >+static void __devlink_params_unregister(struct devlink *devlink,
>> >+ struct list_head *param_list,
>> >+ const struct devlink_param *params,
>> >+ size_t params_count)
>> >+{
>> >+ const struct devlink_param *param = params;
>> >+ int i;
>> >+
>> >+ mutex_lock(&devlink->lock);
>> >+ for (i = 0; i < params_count; i++, param++)
>> >+ devlink_param_unregister_one(devlink, param_list, param);
>> >+ mutex_unlock(&devlink->lock);
>> >+}
>>
>> Please do devlink_params_register in a similar way. Have
>> __devlink_params_register() with list_head * arg
>
>This cannot be optimised as you see the whole patch-set, unregister
>also requires different command in case of failure of
>param_register_one and sending 2 types of command as argument will not
>simplify this code. It is better this way, as you see the final
>function.
I don't see any problem in passing 2 cmd values. The point is to avoid
code duplication and unnecessary bugs like I found below.
>
>>
>>
>> >+
>> > /**
>> > * devlink_params_unregister - unregister configuration parameters
>> > * @devlink: devlink
>> >@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
>> > const struct devlink_param *params,
>> > size_t params_count)
>> > {
>> >- const struct devlink_param *param = params;
>> >- int i;
>> >-
>> >- mutex_lock(&devlink->lock);
>> >- for (i = 0; i < params_count; i++, param++)
>> >- devlink_param_unregister_one(devlink, param);
>> >- mutex_unlock(&devlink->lock);
>> >+ return __devlink_params_unregister(devlink, &devlink->param_list,
>> >+ params, params_count);
>> > }
>> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
>> >
>> >@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
>> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
>> >
>> > /**
>> >+ * devlink_port_params_register - register port configuration parameters
>> >+ *
>> >+ * @devlink_port: devlink port
>> >+ * @params: configuration parameters array
>> >+ * @params_count: number of parameters provided
>> >+ *
>> >+ * Register the configuration parameters supported by the port.
>> >+ */
>> >+int devlink_port_params_register(struct devlink_port *devlink_port,
>> >+ const struct devlink_param *params,
>> >+ size_t params_count)
>> >+{
>> >+ struct devlink *devlink = devlink_port->devlink;
>> >+ const struct devlink_param *param = params;
>> >+ int i, err;
>> >+
>> >+ mutex_lock(&devlink->lock);
>> >+ for (i = 0; i < params_count; i++) {
You need to increment param here.
>> >+ err = devlink_param_verify(param);
>> >+ if (err)
>> >+ goto rollback;
>> >+
>> >+ err = devlink_param_register_one(devlink,
>> >+ &devlink_port->param_list,
>> >+ param);
>> >+ if (err)
>> >+ goto rollback;
>> >+ }
>> >+
>> >+ mutex_unlock(&devlink->lock);
>> >+ return 0;
>> >+
>> >+rollback:
>> >+ if (!i)
>> >+ goto unlock;
>> >+ for (param--; i > 0; i--, param--)
>> >+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
>> >+ param);
>> >+unlock:
>> >+ mutex_unlock(&devlink->lock);
>> >+ return err;
>> >+}
>> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
>> >+
>> >+/**
>> >+ * devlink_port_params_unregister - unregister port configuration
>> >+ * parameters
>> >+ *
>> >+ * @devlink_port: devlink port
>> >+ * @params: configuration parameters array
>> >+ * @params_count: number of parameters provided
>> >+ */
>> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
>> >+ const struct devlink_param *params,
>> >+ size_t params_count)
>> >+{
>> >+ return __devlink_params_unregister(devlink_port->devlink,
>> >+ &devlink_port->param_list,
>> >+ params, params_count);
>> >+}
>> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
>> >+
>> >+/**
>> > * devlink_region_create - create a new address region
>> > *
>> > * @devlink: devlink
>> >--
>> >1.8.3.1
>> >
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister
2018-12-13 12:54 ` Jiri Pirko
@ 2018-12-14 4:52 ` Vasundhara Volam
0 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-14 4:52 UTC (permalink / raw)
To: Jiří Pírko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Thu, Dec 13, 2018 at 6:32 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Dec 12, 2018 at 02:43:14PM CET, vasundhara-v.volam@broadcom.com wrote:
> >On Tue, Dec 11, 2018 at 2:39 PM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> Tue, Dec 11, 2018 at 09:46:41AM CET, vasundhara-v.volam@broadcom.com wrote:
> >> >Add functions to register and unregister for the driver supported
> >> >configuration parameters table per port.
> >> >
> >> >Cc: Jiri Pirko <jiri@mellanox.com>
> >> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> >---
> >> > include/net/devlink.h | 22 +++++++++
> >> > net/core/devlink.c | 128 ++++++++++++++++++++++++++++++++++++++++----------
> >> > 2 files changed, 124 insertions(+), 26 deletions(-)
> >> >
> >> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> >index 67f4293..98b8a66 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;
> >> >@@ -574,6 +575,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 +823,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..da7f6bd 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(&devlink->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,18 +3165,18 @@ static int devlink_param_register_one(struct devlink *devlink,
> >> > return -ENOMEM;
> >> > param_item->param = param;
> >> >
> >> >- list_add_tail(¶m_item->list, &devlink->param_list);
> >> >+ list_add_tail(¶m_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(&devlink->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(¶m_item->list);
> >> >@@ -3954,6 +3954,7 @@ int devlink_port_register(struct devlink *devlink,
> >> > devlink_port->index = port_index;
> >> > devlink_port->registered = true;
> >> > list_add_tail(&devlink_port->list, &devlink->port_list);
> >> >+ INIT_LIST_HEAD(&devlink_port->param_list);
> >> > mutex_unlock(&devlink->lock);
> >> > devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
> >> > return 0;
> >> >@@ -4471,6 +4472,16 @@ void devlink_resource_occ_get_unregister(struct devlink *devlink,
> >> > }
> >> > EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
> >> >
> >> >+static int devlink_param_verify(const struct devlink_param *param)
> >> >+{
> >> >+ if (!param || !param->name || !param->supported_cmodes)
> >> >+ return -EINVAL;
> >> >+ if (param->generic)
> >> >+ return devlink_param_generic_verify(param);
> >> >+ else
> >> >+ return devlink_param_driver_verify(param);
> >> >+}
> >> >+
> >> > /**
> >> > * devlink_params_register - register configuration parameters
> >> > *
> >> >@@ -4490,20 +4501,12 @@ int devlink_params_register(struct devlink *devlink,
> >> >
> >> > mutex_lock(&devlink->lock);
> >> > for (i = 0; i < params_count; i++, param++) {
> >> >- if (!param || !param->name || !param->supported_cmodes) {
> >> >- err = -EINVAL;
> >> >+ err = devlink_param_verify(param);
> >> >+ if (err)
> >> > goto rollback;
> >> >- }
> >> >- if (param->generic) {
> >> >- err = devlink_param_generic_verify(param);
> >> >- if (err)
> >> >- goto rollback;
> >> >- } else {
> >> >- err = devlink_param_driver_verify(param);
> >> >- if (err)
> >> >- goto rollback;
> >> >- }
> >> >- err = devlink_param_register_one(devlink, param);
> >> >+
> >> >+ err = devlink_param_register_one(devlink, &devlink->param_list,
> >> >+ param);
> >> > if (err)
> >> > goto rollback;
> >> > }
> >> >@@ -4515,13 +4518,28 @@ int devlink_params_register(struct devlink *devlink,
> >> > if (!i)
> >> > goto unlock;
> >> > for (param--; i > 0; i--, param--)
> >> >- devlink_param_unregister_one(devlink, param);
> >> >+ devlink_param_unregister_one(devlink, &devlink->param_list,
> >> >+ param);
> >> > unlock:
> >> > mutex_unlock(&devlink->lock);
> >> > return err;
> >> > }
> >> > EXPORT_SYMBOL_GPL(devlink_params_register);
> >> >
> >> >+static void __devlink_params_unregister(struct devlink *devlink,
> >> >+ struct list_head *param_list,
> >> >+ const struct devlink_param *params,
> >> >+ size_t params_count)
> >> >+{
> >> >+ const struct devlink_param *param = params;
> >> >+ int i;
> >> >+
> >> >+ mutex_lock(&devlink->lock);
> >> >+ for (i = 0; i < params_count; i++, param++)
> >> >+ devlink_param_unregister_one(devlink, param_list, param);
> >> >+ mutex_unlock(&devlink->lock);
> >> >+}
> >>
> >> Please do devlink_params_register in a similar way. Have
> >> __devlink_params_register() with list_head * arg
> >
> >This cannot be optimised as you see the whole patch-set, unregister
> >also requires different command in case of failure of
> >param_register_one and sending 2 types of command as argument will not
> >simplify this code. It is better this way, as you see the final
> >function.
>
> I don't see any problem in passing 2 cmd values. The point is to avoid
> code duplication and unnecessary bugs like I found below.
>
Okay, got your point. Since this will be easy to maintain, it is good to move to
common function. I will modify this in next version. Thank you.
>
> >
> >>
> >>
> >> >+
> >> > /**
> >> > * devlink_params_unregister - unregister configuration parameters
> >> > * @devlink: devlink
> >> >@@ -4532,13 +4550,8 @@ void devlink_params_unregister(struct devlink *devlink,
> >> > const struct devlink_param *params,
> >> > size_t params_count)
> >> > {
> >> >- const struct devlink_param *param = params;
> >> >- int i;
> >> >-
> >> >- mutex_lock(&devlink->lock);
> >> >- for (i = 0; i < params_count; i++, param++)
> >> >- devlink_param_unregister_one(devlink, param);
> >> >- mutex_unlock(&devlink->lock);
> >> >+ return __devlink_params_unregister(devlink, &devlink->param_list,
> >> >+ params, params_count);
> >> > }
> >> > EXPORT_SYMBOL_GPL(devlink_params_unregister);
> >> >
> >> >@@ -4657,6 +4670,69 @@ void devlink_param_value_str_fill(union devlink_param_value *dst_val,
> >> > EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
> >> >
> >> > /**
> >> >+ * devlink_port_params_register - register port configuration parameters
> >> >+ *
> >> >+ * @devlink_port: devlink port
> >> >+ * @params: configuration parameters array
> >> >+ * @params_count: number of parameters provided
> >> >+ *
> >> >+ * Register the configuration parameters supported by the port.
> >> >+ */
> >> >+int devlink_port_params_register(struct devlink_port *devlink_port,
> >> >+ const struct devlink_param *params,
> >> >+ size_t params_count)
> >> >+{
> >> >+ struct devlink *devlink = devlink_port->devlink;
> >> >+ const struct devlink_param *param = params;
> >> >+ int i, err;
> >> >+
> >> >+ mutex_lock(&devlink->lock);
> >> >+ for (i = 0; i < params_count; i++) {
>
> You need to increment param here.
>
>
>
> >> >+ err = devlink_param_verify(param);
> >> >+ if (err)
> >> >+ goto rollback;
> >> >+
> >> >+ err = devlink_param_register_one(devlink,
> >> >+ &devlink_port->param_list,
> >> >+ param);
> >> >+ if (err)
> >> >+ goto rollback;
> >> >+ }
> >> >+
> >> >+ mutex_unlock(&devlink->lock);
> >> >+ return 0;
> >> >+
> >> >+rollback:
> >> >+ if (!i)
> >> >+ goto unlock;
> >> >+ for (param--; i > 0; i--, param--)
> >> >+ devlink_param_unregister_one(devlink, &devlink_port->param_list,
> >> >+ param);
> >> >+unlock:
> >> >+ mutex_unlock(&devlink->lock);
> >> >+ return err;
> >> >+}
> >> >+EXPORT_SYMBOL_GPL(devlink_port_params_register);
> >> >+
> >> >+/**
> >> >+ * devlink_port_params_unregister - unregister port configuration
> >> >+ * parameters
> >> >+ *
> >> >+ * @devlink_port: devlink port
> >> >+ * @params: configuration parameters array
> >> >+ * @params_count: number of parameters provided
> >> >+ */
> >> >+void devlink_port_params_unregister(struct devlink_port *devlink_port,
> >> >+ const struct devlink_param *params,
> >> >+ size_t params_count)
> >> >+{
> >> >+ return __devlink_params_unregister(devlink_port->devlink,
> >> >+ &devlink_port->param_list,
> >> >+ params, params_count);
> >> >+}
> >> >+EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
> >> >+
> >> >+/**
> >> > * devlink_region_create - create a new address region
> >> > *
> >> > * @devlink: devlink
> >> >--
> >> >1.8.3.1
> >> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH net-next v2 RFC 2/8] devlink: Add port param get command
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 3/8] devlink: Add port param set command Vasundhara Volam
` (5 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add port param get command which gets data per parameter.
It also has option to dump the parameters data per port.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/uapi/linux/devlink.h | 2 +
net/core/devlink.c | 102 ++++++++++++++++++++++++++++++++++++++++---
2 files changed, 97 insertions(+), 7 deletions(-)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 6e52d36..448973b 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -89,6 +89,8 @@ enum devlink_command {
DEVLINK_CMD_REGION_DEL,
DEVLINK_CMD_REGION_READ,
+ DEVLINK_CMD_PORT_PARAM_GET, /* can dump */
+
/* add new commands above here */
__DEVLINK_CMD_MAX,
DEVLINK_CMD_MAX = __DEVLINK_CMD_MAX - 1
diff --git a/net/core/devlink.c b/net/core/devlink.c
index da7f6bd..798dadf 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2843,6 +2843,7 @@ static int devlink_param_set(struct devlink *devlink,
}
static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
+ unsigned int port_index,
struct devlink_param_item *param_item,
enum devlink_command cmd,
u32 portid, u32 seq, int flags)
@@ -2880,6 +2881,11 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
if (devlink_nl_put_handle(msg, devlink))
goto genlmsg_cancel;
+
+ if (cmd == DEVLINK_CMD_PORT_PARAM_GET)
+ if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, port_index))
+ goto genlmsg_cancel;
+
param_attr = nla_nest_start(msg, DEVLINK_ATTR_PARAM);
if (!param_attr)
goto genlmsg_cancel;
@@ -2933,7 +2939,7 @@ static void devlink_param_notify(struct devlink *devlink,
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return;
- err = devlink_nl_param_fill(msg, devlink, param_item, cmd, 0, 0, 0);
+ err = devlink_nl_param_fill(msg, devlink, 0, param_item, cmd, 0, 0, 0);
if (err) {
nlmsg_free(msg);
return;
@@ -2962,7 +2968,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
idx++;
continue;
}
- err = devlink_nl_param_fill(msg, devlink, param_item,
+ err = devlink_nl_param_fill(msg, devlink, 0, param_item,
DEVLINK_CMD_PARAM_GET,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq,
@@ -3051,7 +3057,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
}
static struct devlink_param_item *
-devlink_param_get_from_info(struct devlink *devlink,
+devlink_param_get_from_info(struct list_head *param_list,
struct genl_info *info)
{
char *param_name;
@@ -3060,7 +3066,7 @@ static int devlink_nl_cmd_param_get_dumpit(struct sk_buff *msg,
return NULL;
param_name = nla_data(info->attrs[DEVLINK_ATTR_PARAM_NAME]);
- return devlink_param_find_by_name(&devlink->param_list, param_name);
+ return devlink_param_find_by_name(param_list, param_name);
}
static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
@@ -3071,7 +3077,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
struct sk_buff *msg;
int err;
- param_item = devlink_param_get_from_info(devlink, info);
+ param_item = devlink_param_get_from_info(&devlink->param_list, info);
if (!param_item)
return -EINVAL;
@@ -3079,7 +3085,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
if (!msg)
return -ENOMEM;
- err = devlink_nl_param_fill(msg, devlink, param_item,
+ err = devlink_nl_param_fill(msg, devlink, 0, param_item,
DEVLINK_CMD_PARAM_GET,
info->snd_portid, info->snd_seq, 0);
if (err) {
@@ -3102,7 +3108,7 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
union devlink_param_value value;
int err = 0;
- param_item = devlink_param_get_from_info(devlink, info);
+ param_item = devlink_param_get_from_info(&devlink->param_list, info);
if (!param_item)
return -EINVAL;
param = param_item->param;
@@ -3183,6 +3189,80 @@ static void devlink_param_unregister_one(struct devlink *devlink,
kfree(param_item);
}
+static int devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
+ struct netlink_callback *cb)
+{
+ struct devlink_param_item *param_item;
+ struct devlink_port *devlink_port;
+ struct devlink *devlink;
+ int start = cb->args[0];
+ int idx = 0;
+ int err;
+
+ mutex_lock(&devlink_mutex);
+ list_for_each_entry(devlink, &devlink_list, list) {
+ if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+ continue;
+ mutex_lock(&devlink->lock);
+ list_for_each_entry(devlink_port, &devlink->port_list, list) {
+ list_for_each_entry(param_item,
+ &devlink_port->param_list, list) {
+ if (idx < start) {
+ idx++;
+ continue;
+ }
+ err = devlink_nl_param_fill(msg,
+ devlink_port->devlink,
+ devlink_port->index, param_item,
+ DEVLINK_CMD_PORT_PARAM_GET,
+ NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq,
+ NLM_F_MULTI);
+ if (err) {
+ mutex_unlock(&devlink->lock);
+ goto out;
+ }
+ idx++;
+ }
+ }
+ mutex_unlock(&devlink->lock);
+ }
+out:
+ mutex_unlock(&devlink_mutex);
+
+ cb->args[0] = idx;
+ return msg->len;
+}
+
+static int devlink_nl_cmd_port_param_get_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct devlink_port *devlink_port = info->user_ptr[0];
+ struct devlink_param_item *param_item;
+ struct sk_buff *msg;
+ int err;
+
+ param_item = devlink_param_get_from_info(&devlink_port->param_list,
+ info);
+ if (!param_item)
+ return -EINVAL;
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ err = devlink_nl_param_fill(msg, devlink_port->devlink,
+ devlink_port->index, param_item,
+ DEVLINK_CMD_PORT_PARAM_GET,
+ info->snd_portid, info->snd_seq, 0);
+ if (err) {
+ nlmsg_free(msg);
+ return err;
+ }
+
+ return genlmsg_reply(msg, info);
+}
+
static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
struct devlink *devlink,
struct devlink_snapshot *snapshot)
@@ -3821,6 +3901,14 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK,
},
{
+ .cmd = DEVLINK_CMD_PORT_PARAM_GET,
+ .doit = devlink_nl_cmd_port_param_get_doit,
+ .dumpit = devlink_nl_cmd_port_param_get_dumpit,
+ .policy = devlink_nl_policy,
+ .internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
+ /* can be retrieved by unprivileged users */
+ },
+ {
.cmd = DEVLINK_CMD_REGION_GET,
.doit = devlink_nl_cmd_region_get_doit,
.dumpit = devlink_nl_cmd_region_get_dumpit,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH net-next v2 RFC 3/8] devlink: Add port param set command
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 1/8] devlink: Add devlink_param for port register and unregister Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 2/8] devlink: Add port param get command Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 4/8] devlink: Add support for driverinit get value for devlink_port Vasundhara Volam
` (4 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add port param set command to set the value for a parameter.
Value can be set to any of the supported configuration modes.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/uapi/linux/devlink.h | 1 +
net/core/devlink.c | 37 ++++++++++++++++++++++++++++++++-----
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 448973b..3658fb2 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -90,6 +90,7 @@ enum devlink_command {
DEVLINK_CMD_REGION_READ,
DEVLINK_CMD_PORT_PARAM_GET, /* can dump */
+ DEVLINK_CMD_PORT_PARAM_SET,
/* add new commands above here */
__DEVLINK_CMD_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 798dadf..d2bd6e5 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3096,10 +3096,11 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
return genlmsg_reply(msg, info);
}
-static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
- struct genl_info *info)
+static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
+ struct list_head *param_list,
+ struct genl_info *info,
+ enum devlink_command cmd)
{
- struct devlink *devlink = info->user_ptr[0];
enum devlink_param_type param_type;
struct devlink_param_gset_ctx ctx;
enum devlink_param_cmode cmode;
@@ -3108,7 +3109,7 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
union devlink_param_value value;
int err = 0;
- param_item = devlink_param_get_from_info(&devlink->param_list, info);
+ param_item = devlink_param_get_from_info(param_list, info);
if (!param_item)
return -EINVAL;
param = param_item->param;
@@ -3148,10 +3149,19 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
return err;
}
- devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+ devlink_param_notify(devlink, param_item, cmd);
return 0;
}
+static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct devlink *devlink = info->user_ptr[0];
+
+ return __devlink_nl_cmd_param_set_doit(devlink, &devlink->param_list,
+ info, DEVLINK_CMD_PARAM_NEW);
+}
+
static int devlink_param_register_one(struct devlink *devlink,
struct list_head *param_list,
const struct devlink_param *param)
@@ -3263,6 +3273,16 @@ static int devlink_nl_cmd_port_param_get_doit(struct sk_buff *skb,
return genlmsg_reply(msg, info);
}
+static int devlink_nl_cmd_port_param_set_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct devlink_port *devlink_port = info->user_ptr[0];
+
+ return __devlink_nl_cmd_param_set_doit(devlink_port->devlink,
+ &devlink_port->param_list,
+ info, 0);
+}
+
static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
struct devlink *devlink,
struct devlink_snapshot *snapshot)
@@ -3909,6 +3929,13 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
/* can be retrieved by unprivileged users */
},
{
+ .cmd = DEVLINK_CMD_PORT_PARAM_SET,
+ .doit = devlink_nl_cmd_port_param_set_doit,
+ .policy = devlink_nl_policy,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
+ },
+ {
.cmd = DEVLINK_CMD_REGION_GET,
.doit = devlink_nl_cmd_region_get_doit,
.dumpit = devlink_nl_cmd_region_get_dumpit,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH net-next v2 RFC 4/8] devlink: Add support for driverinit get value for devlink_port
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
` (2 preceding siblings ...)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 3/8] devlink: Add port param set command Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 5/8] devlink: Add support for driverinit set " Vasundhara Volam
` (3 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add support for "driverinit" configuration mode value for devlink_port
configuration parameters. Add devlink_port_param_driverinit_value_get()
function to help the driver get the value from devlink_port.
Also, move the common code to __devlink_param_driverinit_value_get()
to be used by both device and port params.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/net/devlink.h | 8 ++++++
net/core/devlink.c | 67 ++++++++++++++++++++++++++++++++++++++-------------
2 files changed, 58 insertions(+), 17 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 98b8a66..09f3f43 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -838,6 +838,14 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
{
}
+static inline int
+devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
+ u32 param_id,
+ union devlink_param_value *init_val)
+{
+ return -EOPNOTSUPP;
+}
+
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 d2bd6e5..752ac86 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4670,26 +4670,13 @@ void devlink_params_unregister(struct devlink *devlink,
}
EXPORT_SYMBOL_GPL(devlink_params_unregister);
-/**
- * devlink_param_driverinit_value_get - get configuration parameter
- * value for driver initializing
- *
- * @devlink: devlink
- * @param_id: parameter ID
- * @init_val: value of parameter in driverinit configuration mode
- *
- * This function should be used by the driver to get driverinit
- * configuration for initialization after reload command.
- */
-int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
- union devlink_param_value *init_val)
+static int
+__devlink_param_driverinit_value_get(struct list_head *param_list, u32 param_id,
+ union devlink_param_value *init_val)
{
struct devlink_param_item *param_item;
- if (!devlink->ops || !devlink->ops->reload)
- return -EOPNOTSUPP;
-
- param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
+ param_item = devlink_param_find_by_id(param_list, param_id);
if (!param_item)
return -EINVAL;
@@ -4705,6 +4692,27 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
return 0;
}
+
+/**
+ * devlink_param_driverinit_value_get - get configuration parameter
+ * value for driver initializing
+ *
+ * @devlink: devlink
+ * @param_id: parameter ID
+ * @init_val: value of parameter in driverinit configuration mode
+ *
+ * This function should be used by the driver to get driverinit
+ * configuration for initialization after reload command.
+ */
+int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
+ union devlink_param_value *init_val)
+{
+ if (!devlink->ops || !devlink->ops->reload)
+ return -EOPNOTSUPP;
+
+ return __devlink_param_driverinit_value_get(&devlink->param_list,
+ param_id, init_val);
+}
EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
/**
@@ -4848,6 +4856,31 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
/**
+ * devlink_port_param_driverinit_value_get - get configuration parameter
+ * value for driver initializing
+ *
+ * @devlink_port: devlink_port
+ * @param_id: parameter ID
+ * @init_val: value of parameter in driverinit configuration mode
+ *
+ * This function should be used by the driver to get driverinit
+ * configuration for initialization after reload command.
+ */
+int devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
+ u32 param_id,
+ union devlink_param_value *init_val)
+{
+ struct devlink *devlink = devlink_port->devlink;
+
+ if (!devlink->ops || !devlink->ops->reload)
+ return -EOPNOTSUPP;
+
+ return __devlink_param_driverinit_value_get(&devlink_port->param_list,
+ param_id, init_val);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_get);
+
+/**
* devlink_region_create - create a new address region
*
* @devlink: devlink
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH net-next v2 RFC 5/8] devlink: Add support for driverinit set value for devlink_port
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
` (3 preceding siblings ...)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 4/8] devlink: Add support for driverinit get value for devlink_port Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 6/8] devlink: Add devlink notifications support for port params Vasundhara Volam
` (2 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add support for "driverinit" configuration mode value for devlink_port
configuration parameters. Add devlink_port_param_driverinit_value_set()
function to help the driver set the value to devlink_port.
Also, move the common code to __devlink_param_driverinit_value_set()
to be used by both device and port params.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/net/devlink.h | 11 +++++++++
net/core/devlink.c | 64 +++++++++++++++++++++++++++++++++++++++------------
2 files changed, 60 insertions(+), 15 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 09f3f43..f78cb8d 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -581,6 +581,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
void devlink_port_params_unregister(struct devlink_port *devlink_port,
const struct devlink_param *params,
size_t params_count);
+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+ u32 param_id,
+ union devlink_param_value init_val);
struct devlink_region *devlink_region_create(struct devlink *devlink,
const char *region_name,
u32 region_max_snapshots,
@@ -846,6 +849,14 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
return -EOPNOTSUPP;
}
+static inline int
+devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+ u32 param_id,
+ union devlink_param_value init_val)
+{
+ return -EOPNOTSUPP;
+}
+
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 752ac86..669d62e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4715,24 +4715,15 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
}
EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_get);
-/**
- * devlink_param_driverinit_value_set - set value of configuration
- * parameter for driverinit
- * configuration mode
- *
- * @devlink: devlink
- * @param_id: parameter ID
- * @init_val: value of parameter to set for driverinit configuration mode
- *
- * This function should be used by the driver to set driverinit
- * configuration mode default value.
- */
-int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
- union devlink_param_value init_val)
+static int
+__devlink_param_driverinit_value_set(struct devlink *devlink,
+ struct list_head *param_list, u32 param_id,
+ union devlink_param_value init_val,
+ enum devlink_command cmd)
{
struct devlink_param_item *param_item;
- param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
+ param_item = devlink_param_find_by_id(param_list, param_id);
if (!param_item)
return -EINVAL;
@@ -4749,6 +4740,27 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
return 0;
}
+
+/**
+ * devlink_param_driverinit_value_set - set value of configuration
+ * parameter for driverinit
+ * configuration mode
+ *
+ * @devlink: devlink
+ * @param_id: parameter ID
+ * @init_val: value of parameter to set for driverinit configuration mode
+ *
+ * This function should be used by the driver to set driverinit
+ * configuration mode default value.
+ */
+int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
+ union devlink_param_value init_val)
+{
+ return __devlink_param_driverinit_value_set(devlink,
+ &devlink->param_list,
+ param_id, init_val,
+ DEVLINK_CMD_PARAM_NEW);
+}
EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
/**
@@ -4881,6 +4893,28 @@ int devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_get);
/**
+ * devlink_port_param_driverinit_value_set - set value of configuration
+ * parameter for driverinit
+ * configuration mode
+ *
+ * @devlink_port: devlink_port
+ * @param_id: parameter ID
+ * @init_val: value of parameter to set for driverinit configuration mode
+ *
+ * This function should be used by the driver to set driverinit
+ * configuration mode default value.
+ */
+int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
+ u32 param_id,
+ union devlink_param_value init_val)
+{
+ return __devlink_param_driverinit_value_set(devlink_port->devlink,
+ &devlink_port->param_list,
+ param_id, init_val, 0);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_set);
+
+/**
* devlink_region_create - create a new address region
*
* @devlink: devlink
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH net-next v2 RFC 6/8] devlink: Add devlink notifications support for port params
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
` (4 preceding siblings ...)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 5/8] devlink: Add support for driverinit set " Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter Vasundhara Volam
2018-12-11 8:46 ` [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it Vasundhara Volam
7 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Add notification call for devlink port param set, register and unregister
functions.
Add devlink_port_param_value_changed() function to enable the driver notify
devlink on value change. Driver should use this function after value was
changed on any configuration mode part to driverinit.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/net/devlink.h | 8 ++++
include/uapi/linux/devlink.h | 2 +
net/core/devlink.c | 107 ++++++++++++++++++++++++++++++++-----------
3 files changed, 89 insertions(+), 28 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f78cb8d..db413d3 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -584,6 +584,8 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
u32 param_id,
union devlink_param_value init_val);
+void devlink_port_param_value_changed(struct devlink_port *devlink_port,
+ u32 param_id);
struct devlink_region *devlink_region_create(struct devlink *devlink,
const char *region_name,
u32 region_max_snapshots,
@@ -857,6 +859,12 @@ static inline bool devlink_dpipe_table_counter_enabled(struct devlink *devlink,
return -EOPNOTSUPP;
}
+static inline void
+devlink_port_param_value_changed(struct devlink_port *devlink_port,
+ u32 param_id)
+{
+}
+
static inline struct devlink_region *
devlink_region_create(struct devlink *devlink,
const char *region_name,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3658fb2..61b4447 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -91,6 +91,8 @@ enum devlink_command {
DEVLINK_CMD_PORT_PARAM_GET, /* can dump */
DEVLINK_CMD_PORT_PARAM_SET,
+ DEVLINK_CMD_PORT_PARAM_NEW,
+ DEVLINK_CMD_PORT_PARAM_DEL,
/* add new commands above here */
__DEVLINK_CMD_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 669d62e..6bd7e13 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2882,7 +2882,9 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
if (devlink_nl_put_handle(msg, devlink))
goto genlmsg_cancel;
- if (cmd == DEVLINK_CMD_PORT_PARAM_GET)
+ if (cmd == DEVLINK_CMD_PORT_PARAM_GET ||
+ cmd == DEVLINK_CMD_PORT_PARAM_NEW ||
+ cmd == DEVLINK_CMD_PORT_PARAM_DEL)
if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, port_index))
goto genlmsg_cancel;
@@ -2928,18 +2930,22 @@ static int devlink_nl_param_fill(struct sk_buff *msg, struct devlink *devlink,
}
static void devlink_param_notify(struct devlink *devlink,
+ unsigned int port_index,
struct devlink_param_item *param_item,
enum devlink_command cmd)
{
struct sk_buff *msg;
int err;
- WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL);
+ WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL &&
+ cmd != DEVLINK_CMD_PORT_PARAM_NEW &&
+ cmd != DEVLINK_CMD_PORT_PARAM_DEL);
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return;
- err = devlink_nl_param_fill(msg, devlink, 0, param_item, cmd, 0, 0, 0);
+ err = devlink_nl_param_fill(msg, devlink, port_index, param_item, cmd,
+ 0, 0, 0);
if (err) {
nlmsg_free(msg);
return;
@@ -3097,6 +3103,7 @@ static int devlink_nl_cmd_param_get_doit(struct sk_buff *skb,
}
static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
+ unsigned int port_index,
struct list_head *param_list,
struct genl_info *info,
enum devlink_command cmd)
@@ -3149,7 +3156,7 @@ static int __devlink_nl_cmd_param_set_doit(struct devlink *devlink,
return err;
}
- devlink_param_notify(devlink, param_item, cmd);
+ devlink_param_notify(devlink, port_index, param_item, cmd);
return 0;
}
@@ -3158,13 +3165,15 @@ static int devlink_nl_cmd_param_set_doit(struct sk_buff *skb,
{
struct devlink *devlink = info->user_ptr[0];
- return __devlink_nl_cmd_param_set_doit(devlink, &devlink->param_list,
+ return __devlink_nl_cmd_param_set_doit(devlink, 0, &devlink->param_list,
info, DEVLINK_CMD_PARAM_NEW);
}
static int devlink_param_register_one(struct devlink *devlink,
+ unsigned int port_index,
struct list_head *param_list,
- const struct devlink_param *param)
+ const struct devlink_param *param,
+ enum devlink_command cmd)
{
struct devlink_param_item *param_item;
@@ -3182,19 +3191,21 @@ static int devlink_param_register_one(struct devlink *devlink,
param_item->param = param;
list_add_tail(¶m_item->list, param_list);
- devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+ devlink_param_notify(devlink, port_index, param_item, cmd);
return 0;
}
static void devlink_param_unregister_one(struct devlink *devlink,
+ unsigned int port_index,
struct list_head *param_list,
- const struct devlink_param *param)
+ const struct devlink_param *param,
+ enum devlink_command cmd)
{
struct devlink_param_item *param_item;
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);
+ devlink_param_notify(devlink, port_index, param_item, cmd);
list_del(¶m_item->list);
kfree(param_item);
}
@@ -3279,8 +3290,9 @@ static int devlink_nl_cmd_port_param_set_doit(struct sk_buff *skb,
struct devlink_port *devlink_port = info->user_ptr[0];
return __devlink_nl_cmd_param_set_doit(devlink_port->devlink,
- &devlink_port->param_list,
- info, 0);
+ devlink_port->index,
+ &devlink_port->param_list, info,
+ DEVLINK_CMD_PORT_PARAM_NEW);
}
static int devlink_nl_region_snapshot_id_put(struct sk_buff *msg,
@@ -4620,8 +4632,9 @@ int devlink_params_register(struct devlink *devlink,
if (err)
goto rollback;
- err = devlink_param_register_one(devlink, &devlink->param_list,
- param);
+ err = devlink_param_register_one(devlink, 0,
+ &devlink->param_list, param,
+ DEVLINK_CMD_PARAM_NEW);
if (err)
goto rollback;
}
@@ -4633,8 +4646,8 @@ int devlink_params_register(struct devlink *devlink,
if (!i)
goto unlock;
for (param--; i > 0; i--, param--)
- devlink_param_unregister_one(devlink, &devlink->param_list,
- param);
+ devlink_param_unregister_one(devlink, 0, &devlink->param_list,
+ param, DEVLINK_CMD_PARAM_DEL);
unlock:
mutex_unlock(&devlink->lock);
return err;
@@ -4642,16 +4655,19 @@ int devlink_params_register(struct devlink *devlink,
EXPORT_SYMBOL_GPL(devlink_params_register);
static void __devlink_params_unregister(struct devlink *devlink,
+ unsigned int port_index,
struct list_head *param_list,
const struct devlink_param *params,
- size_t params_count)
+ size_t params_count,
+ enum devlink_command cmd)
{
const struct devlink_param *param = params;
int i;
mutex_lock(&devlink->lock);
for (i = 0; i < params_count; i++, param++)
- devlink_param_unregister_one(devlink, param_list, param);
+ devlink_param_unregister_one(devlink, 0, param_list, param,
+ cmd);
mutex_unlock(&devlink->lock);
}
@@ -4665,8 +4681,9 @@ void devlink_params_unregister(struct devlink *devlink,
const struct devlink_param *params,
size_t params_count)
{
- return __devlink_params_unregister(devlink, &devlink->param_list,
- params, params_count);
+ return __devlink_params_unregister(devlink, 0, &devlink->param_list,
+ params, params_count,
+ DEVLINK_CMD_PARAM_DEL);
}
EXPORT_SYMBOL_GPL(devlink_params_unregister);
@@ -4717,6 +4734,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
static int
__devlink_param_driverinit_value_set(struct devlink *devlink,
+ unsigned int port_index,
struct list_head *param_list, u32 param_id,
union devlink_param_value init_val,
enum devlink_command cmd)
@@ -4737,7 +4755,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
param_item->driverinit_value = init_val;
param_item->driverinit_value_valid = true;
- devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+ devlink_param_notify(devlink, port_index, param_item, cmd);
return 0;
}
@@ -4756,7 +4774,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
union devlink_param_value init_val)
{
- return __devlink_param_driverinit_value_set(devlink,
+ return __devlink_param_driverinit_value_set(devlink, 0,
&devlink->param_list,
param_id, init_val,
DEVLINK_CMD_PARAM_NEW);
@@ -4783,7 +4801,7 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
WARN_ON(!param_item);
- devlink_param_notify(devlink, param_item, DEVLINK_CMD_PARAM_NEW);
+ devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
}
EXPORT_SYMBOL_GPL(devlink_param_value_changed);
@@ -4827,9 +4845,10 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
if (err)
goto rollback;
- err = devlink_param_register_one(devlink,
+ err = devlink_param_register_one(devlink, devlink_port->index,
&devlink_port->param_list,
- param);
+ param,
+ DEVLINK_CMD_PORT_PARAM_NEW);
if (err)
goto rollback;
}
@@ -4841,8 +4860,9 @@ int devlink_port_params_register(struct devlink_port *devlink_port,
if (!i)
goto unlock;
for (param--; i > 0; i--, param--)
- devlink_param_unregister_one(devlink, &devlink_port->param_list,
- param);
+ devlink_param_unregister_one(devlink, devlink_port->index,
+ &devlink_port->param_list, param,
+ DEVLINK_CMD_PORT_PARAM_DEL);
unlock:
mutex_unlock(&devlink->lock);
return err;
@@ -4862,8 +4882,10 @@ void devlink_port_params_unregister(struct devlink_port *devlink_port,
size_t params_count)
{
return __devlink_params_unregister(devlink_port->devlink,
+ devlink_port->index,
&devlink_port->param_list,
- params, params_count);
+ params, params_count,
+ DEVLINK_CMD_PORT_PARAM_DEL);
}
EXPORT_SYMBOL_GPL(devlink_port_params_unregister);
@@ -4909,12 +4931,41 @@ int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
union devlink_param_value init_val)
{
return __devlink_param_driverinit_value_set(devlink_port->devlink,
+ devlink_port->index,
&devlink_port->param_list,
- param_id, init_val, 0);
+ param_id, init_val,
+ DEVLINK_CMD_PORT_PARAM_NEW);
}
EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_set);
/**
+ * devlink_port_param_value_changed - notify devlink on a parameter's value
+ * change. Should be called by the driver
+ * right after the change.
+ *
+ * @devlink_port: devlink_port
+ * @param_id: parameter ID
+ *
+ * This function should be used by the driver to notify devlink on value
+ * change, excluding driverinit configuration mode.
+ * For driverinit configuration mode driver should use the function
+ * devlink_port_param_driverinit_value_set() instead.
+ */
+void devlink_port_param_value_changed(struct devlink_port *devlink_port,
+ u32 param_id)
+{
+ struct devlink_param_item *param_item;
+
+ param_item = devlink_param_find_by_id(&devlink_port->param_list,
+ param_id);
+ WARN_ON(!param_item);
+
+ devlink_param_notify(devlink_port->devlink, devlink_port->index,
+ param_item, DEVLINK_CMD_PORT_PARAM_NEW);
+}
+EXPORT_SYMBOL_GPL(devlink_port_param_value_changed);
+
+/**
* devlink_region_create - create a new address region
*
* @devlink: devlink
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
` (5 preceding siblings ...)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 6/8] devlink: Add devlink notifications support for port params Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 10:20 ` Jiri Pirko
` (2 more replies)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it Vasundhara Volam
7 siblings, 3 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
wake-on-lan - Enables Wake on Lan for this port. If enabled,
the controller asserts a wake pin based on the wake-on-lan type.
Cc: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
include/net/devlink.h | 16 ++++++++++++++++
net/core/devlink.c | 5 +++++
2 files changed, 21 insertions(+)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index db413d3..93a1030 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -367,12 +367,25 @@ enum devlink_param_generic_id {
DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
+ DEVLINK_PARAM_GENERIC_ID_WOL,
/* add new param generic ids above here*/
__DEVLINK_PARAM_GENERIC_ID_MAX,
DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
};
+enum devlink_param_wol_types {
+ DEVLINK_PARAM_WOL_DISABLE = 0,
+ DEVLINK_PARAM_WOL_MAGIC_PKT,
+ DEVLINK_PARAM_WOL_PHY,
+ DEVLINK_PARAM_WOL_UNICAST,
+ DEVLINK_PARAM_WOL_MULTICAST,
+ DEVLINK_PARAM_WOL_BROADCAST,
+ DEVLINK_PARAM_WOL_ARP,
+ DEVLINK_PARAM_WOL_SECURE,
+ DEVLINK_PARAM_WOL_FILTERS,
+};
+
#define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
#define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
@@ -397,6 +410,9 @@ enum devlink_param_generic_id {
#define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
#define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
+#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
+#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
+
#define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \
{ \
.id = DEVLINK_PARAM_GENERIC_ID_##_id, \
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6bd7e13..cbdccb2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2697,6 +2697,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
.name = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME,
.type = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE,
},
+ {
+ .id = DEVLINK_PARAM_GENERIC_ID_WOL,
+ .name = DEVLINK_PARAM_GENERIC_WOL_NAME,
+ .type = DEVLINK_PARAM_GENERIC_WOL_TYPE,
+ },
};
static int devlink_param_generic_verify(const struct devlink_param *param)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 8:46 ` [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter Vasundhara Volam
@ 2018-12-11 10:20 ` Jiri Pirko
2018-12-11 10:34 ` Vasundhara Volam
2018-12-11 18:18 ` Jakub Kicinski
2018-12-12 9:15 ` Michal Kubecek
2 siblings, 1 reply; 25+ messages in thread
From: Jiri Pirko @ 2018-12-11 10:20 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, michael.chan, jiri, jakub.kicinski, netdev
Tue, Dec 11, 2018 at 09:46:47AM CET, vasundhara-v.volam@broadcom.com wrote:
>wake-on-lan - Enables Wake on Lan for this port. If enabled,
>the controller asserts a wake pin based on the wake-on-lan type.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> include/net/devlink.h | 16 ++++++++++++++++
> net/core/devlink.c | 5 +++++
> 2 files changed, 21 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index db413d3..93a1030 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -367,12 +367,25 @@ enum devlink_param_generic_id {
> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
> DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
>+ DEVLINK_PARAM_GENERIC_ID_WOL,
>
> /* add new param generic ids above here*/
> __DEVLINK_PARAM_GENERIC_ID_MAX,
> DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
> };
>
>+enum devlink_param_wol_types {
>+ DEVLINK_PARAM_WOL_DISABLE = 0,
>+ DEVLINK_PARAM_WOL_MAGIC_PKT,
>+ DEVLINK_PARAM_WOL_PHY,
>+ DEVLINK_PARAM_WOL_UNICAST,
>+ DEVLINK_PARAM_WOL_MULTICAST,
>+ DEVLINK_PARAM_WOL_BROADCAST,
>+ DEVLINK_PARAM_WOL_ARP,
>+ DEVLINK_PARAM_WOL_SECURE,
>+ DEVLINK_PARAM_WOL_FILTERS,
No need to put unused values to enum now. They should be added only when
used.
>+};
>+
> #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
> #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
>
>@@ -397,6 +410,9 @@ enum devlink_param_generic_id {
> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
>
>+#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
>+#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
>+
> #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \
> { \
> .id = DEVLINK_PARAM_GENERIC_ID_##_id, \
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 6bd7e13..cbdccb2 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -2697,6 +2697,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
> .name = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME,
> .type = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE,
> },
>+ {
>+ .id = DEVLINK_PARAM_GENERIC_ID_WOL,
>+ .name = DEVLINK_PARAM_GENERIC_WOL_NAME,
>+ .type = DEVLINK_PARAM_GENERIC_WOL_TYPE,
>+ },
> };
>
> static int devlink_param_generic_verify(const struct devlink_param *param)
>--
>1.8.3.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 10:20 ` Jiri Pirko
@ 2018-12-11 10:34 ` Vasundhara Volam
0 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 10:34 UTC (permalink / raw)
To: Jiří Pírko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Tue, Dec 11, 2018 at 3:58 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Dec 11, 2018 at 09:46:47AM CET, vasundhara-v.volam@broadcom.com wrote:
> >wake-on-lan - Enables Wake on Lan for this port. If enabled,
> >the controller asserts a wake pin based on the wake-on-lan type.
> >
> >Cc: Jiri Pirko <jiri@mellanox.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > include/net/devlink.h | 16 ++++++++++++++++
> > net/core/devlink.c | 5 +++++
> > 2 files changed, 21 insertions(+)
> >
> >diff --git a/include/net/devlink.h b/include/net/devlink.h
> >index db413d3..93a1030 100644
> >--- a/include/net/devlink.h
> >+++ b/include/net/devlink.h
> >@@ -367,12 +367,25 @@ enum devlink_param_generic_id {
> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
> > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
> >+ DEVLINK_PARAM_GENERIC_ID_WOL,
> >
> > /* add new param generic ids above here*/
> > __DEVLINK_PARAM_GENERIC_ID_MAX,
> > DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
> > };
> >
> >+enum devlink_param_wol_types {
> >+ DEVLINK_PARAM_WOL_DISABLE = 0,
> >+ DEVLINK_PARAM_WOL_MAGIC_PKT,
> >+ DEVLINK_PARAM_WOL_PHY,
> >+ DEVLINK_PARAM_WOL_UNICAST,
> >+ DEVLINK_PARAM_WOL_MULTICAST,
> >+ DEVLINK_PARAM_WOL_BROADCAST,
> >+ DEVLINK_PARAM_WOL_ARP,
> >+ DEVLINK_PARAM_WOL_SECURE,
> >+ DEVLINK_PARAM_WOL_FILTERS,
>
> No need to put unused values to enum now. They should be added only when
> used.
Okay, I will remove them in next version. Thanks.
>
>
> >+};
> >+
> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
> >
> >@@ -397,6 +410,9 @@ enum devlink_param_generic_id {
> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
> >
> >+#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
> >+#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
> >+
> > #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate) \
> > { \
> > .id = DEVLINK_PARAM_GENERIC_ID_##_id, \
> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >index 6bd7e13..cbdccb2 100644
> >--- a/net/core/devlink.c
> >+++ b/net/core/devlink.c
> >@@ -2697,6 +2697,11 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
> > .name = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME,
> > .type = DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE,
> > },
> >+ {
> >+ .id = DEVLINK_PARAM_GENERIC_ID_WOL,
> >+ .name = DEVLINK_PARAM_GENERIC_WOL_NAME,
> >+ .type = DEVLINK_PARAM_GENERIC_WOL_TYPE,
> >+ },
> > };
> >
> > static int devlink_param_generic_verify(const struct devlink_param *param)
> >--
> >1.8.3.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 8:46 ` [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter Vasundhara Volam
2018-12-11 10:20 ` Jiri Pirko
@ 2018-12-11 18:18 ` Jakub Kicinski
2018-12-12 7:35 ` Michael Chan
2018-12-12 9:15 ` Michal Kubecek
2 siblings, 1 reply; 25+ messages in thread
From: Jakub Kicinski @ 2018-12-11 18:18 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, michael.chan, jiri, netdev
On Tue, 11 Dec 2018 14:16:47 +0530, Vasundhara Volam wrote:
> wake-on-lan - Enables Wake on Lan for this port. If enabled,
> the controller asserts a wake pin based on the wake-on-lan type.
>
> Cc: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
As explained previously I think it's a very bad idea to add existing
configuration options to devlink, just because devlink has the ability
to persist the setting in NVM. Especially that for WoL you have to get
the link up so you potentially have all link config stuff as well. And
that n-tuple filters are one of the WoL options, meaning we'd need the
ability to persist n-tuple filters via devlink.
Your effort would be far better spent helping with migrating ethtool to
netlink, and allowing persisting there.
I have not heard any reason why devlink is a better fit. I can imagine
you're just doing it here because it's less effort for you since
ethtool is not yet migrated.
So I'm opposed, let's call it a nack.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 18:18 ` Jakub Kicinski
@ 2018-12-12 7:35 ` Michael Chan
2018-12-12 19:01 ` Jakub Kicinski
0 siblings, 1 reply; 25+ messages in thread
From: Michael Chan @ 2018-12-12 7:35 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Vasundhara Volam, David Miller, Jiri Pirko, Netdev
On Tue, Dec 11, 2018 at 10:18 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Tue, 11 Dec 2018 14:16:47 +0530, Vasundhara Volam wrote:
> > wake-on-lan - Enables Wake on Lan for this port. If enabled,
> > the controller asserts a wake pin based on the wake-on-lan type.
> >
> > Cc: Jiri Pirko <jiri@mellanox.com>
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>
> As explained previously I think it's a very bad idea to add existing
> configuration options to devlink, just because devlink has the ability
> to persist the setting in NVM. Especially that for WoL you have to get
> the link up so you potentially have all link config stuff as well. And
> that n-tuple filters are one of the WoL options, meaning we'd need the
> ability to persist n-tuple filters via devlink.
As I said before, firmware will automatically set the link to autoneg
up to the speed supported by Vaux if WoL is enabled. No special link
setting is needed as I said before. I don't think n-tuple is suitable
for the default power-up WoL setting. n-tuple requires ip address.
The ip address belongs to the system, not to the card that can move
from system to system. The n-tuple WoL packet should be a transient
setting set by the OS that won't persist a power down. The default
power-up WoL packet types should be the most basic magic packet and
other basic types. So I really don't think there is a need to persist
n-tuple WoL packet types.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-12 7:35 ` Michael Chan
@ 2018-12-12 19:01 ` Jakub Kicinski
0 siblings, 0 replies; 25+ messages in thread
From: Jakub Kicinski @ 2018-12-12 19:01 UTC (permalink / raw)
To: Michael Chan; +Cc: Vasundhara Volam, David Miller, Jiri Pirko, Netdev
On Tue, 11 Dec 2018 23:35:31 -0800, Michael Chan wrote:
> On Tue, Dec 11, 2018 at 10:18 AM Jakub Kicinski wrote:
> > On Tue, 11 Dec 2018 14:16:47 +0530, Vasundhara Volam wrote:
> > > wake-on-lan - Enables Wake on Lan for this port. If enabled,
> > > the controller asserts a wake pin based on the wake-on-lan type.
> > >
> > > Cc: Jiri Pirko <jiri@mellanox.com>
> > > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >
> > As explained previously I think it's a very bad idea to add existing
> > configuration options to devlink, just because devlink has the ability
> > to persist the setting in NVM. Especially that for WoL you have to get
> > the link up so you potentially have all link config stuff as well. And
> > that n-tuple filters are one of the WoL options, meaning we'd need the
> > ability to persist n-tuple filters via devlink.
>
> As I said before, firmware will automatically set the link to autoneg
> up to the speed supported by Vaux if WoL is enabled. No special link
> setting is needed as I said before. I don't think n-tuple is suitable
> for the default power-up WoL setting. n-tuple requires ip address.
> The ip address belongs to the system, not to the card that can move
> from system to system. The n-tuple WoL packet should be a transient
> setting set by the OS that won't persist a power down. The default
> power-up WoL packet types should be the most basic magic packet and
> other basic types. So I really don't think there is a need to persist
> n-tuple WoL packet types.
Sure your firmware sets the link to autoneg today, and your driver does
not allow n-tuple filter WoL. How about we step back and think about
the API as a whole rather than exposing ad hoc FW knobs via delink
params :/
Also what gives you the idea that n-tuple filters require an ip address?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-11 8:46 ` [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter Vasundhara Volam
2018-12-11 10:20 ` Jiri Pirko
2018-12-11 18:18 ` Jakub Kicinski
@ 2018-12-12 9:15 ` Michal Kubecek
2018-12-12 12:30 ` Vasundhara Volam
2 siblings, 1 reply; 25+ messages in thread
From: Michal Kubecek @ 2018-12-12 9:15 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, michael.chan, jiri, jakub.kicinski, netdev
On Tue, Dec 11, 2018 at 02:16:47PM +0530, Vasundhara Volam wrote:
> wake-on-lan - Enables Wake on Lan for this port. If enabled,
> the controller asserts a wake pin based on the wake-on-lan type.
>
> Cc: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> ---
> include/net/devlink.h | 16 ++++++++++++++++
> net/core/devlink.c | 5 +++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index db413d3..93a1030 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -367,12 +367,25 @@ enum devlink_param_generic_id {
> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
> DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
> DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
> + DEVLINK_PARAM_GENERIC_ID_WOL,
>
> /* add new param generic ids above here*/
> __DEVLINK_PARAM_GENERIC_ID_MAX,
> DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
> };
>
> +enum devlink_param_wol_types {
> + DEVLINK_PARAM_WOL_DISABLE = 0,
> + DEVLINK_PARAM_WOL_MAGIC_PKT,
> + DEVLINK_PARAM_WOL_PHY,
> + DEVLINK_PARAM_WOL_UNICAST,
> + DEVLINK_PARAM_WOL_MULTICAST,
> + DEVLINK_PARAM_WOL_BROADCAST,
> + DEVLINK_PARAM_WOL_ARP,
> + DEVLINK_PARAM_WOL_SECURE,
> + DEVLINK_PARAM_WOL_FILTERS,
> +};
> +
> #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
> #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
>
> @@ -397,6 +410,9 @@ enum devlink_param_generic_id {
> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
> #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
>
> +#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
Nitpick: the other two parameter names use underscores, it would be more
consistent to use them as well.
> +#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
Can you be sure it will be always sufficient to enable only one WoL type
unlike in ethtool where bitfield is used so that you can enable multiple
types?
Michal Kubecek
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-12 9:15 ` Michal Kubecek
@ 2018-12-12 12:30 ` Vasundhara Volam
2018-12-13 12:56 ` Jiri Pirko
0 siblings, 1 reply; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-12 12:30 UTC (permalink / raw)
To: mkubecek
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Wed, Dec 12, 2018 at 2:45 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, Dec 11, 2018 at 02:16:47PM +0530, Vasundhara Volam wrote:
> > wake-on-lan - Enables Wake on Lan for this port. If enabled,
> > the controller asserts a wake pin based on the wake-on-lan type.
> >
> > Cc: Jiri Pirko <jiri@mellanox.com>
> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> > ---
> > include/net/devlink.h | 16 ++++++++++++++++
> > net/core/devlink.c | 5 +++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> > index db413d3..93a1030 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -367,12 +367,25 @@ enum devlink_param_generic_id {
> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
> > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
> > + DEVLINK_PARAM_GENERIC_ID_WOL,
> >
> > /* add new param generic ids above here*/
> > __DEVLINK_PARAM_GENERIC_ID_MAX,
> > DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
> > };
> >
> > +enum devlink_param_wol_types {
> > + DEVLINK_PARAM_WOL_DISABLE = 0,
> > + DEVLINK_PARAM_WOL_MAGIC_PKT,
> > + DEVLINK_PARAM_WOL_PHY,
> > + DEVLINK_PARAM_WOL_UNICAST,
> > + DEVLINK_PARAM_WOL_MULTICAST,
> > + DEVLINK_PARAM_WOL_BROADCAST,
> > + DEVLINK_PARAM_WOL_ARP,
> > + DEVLINK_PARAM_WOL_SECURE,
> > + DEVLINK_PARAM_WOL_FILTERS,
> > +};
> > +
> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
> >
> > @@ -397,6 +410,9 @@ enum devlink_param_generic_id {
> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
> >
> > +#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
>
> Nitpick: the other two parameter names use underscores, it would be more
> consistent to use them as well.
Sure, I will fix this in next version. Thanks.
>
> > +#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
>
> Can you be sure it will be always sufficient to enable only one WoL type
> unlike in ethtool where bitfield is used so that you can enable multiple
> types?
Since this is a persistent setting, NIC may not support multiple types
of WOL upon power on.
If there is any requirement in future to add multiple types, data type
can be modified,
as other WOL types are also not added now.
>
> Michal Kubecek
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-12 12:30 ` Vasundhara Volam
@ 2018-12-13 12:56 ` Jiri Pirko
2018-12-14 4:50 ` Vasundhara Volam
0 siblings, 1 reply; 25+ messages in thread
From: Jiri Pirko @ 2018-12-13 12:56 UTC (permalink / raw)
To: Vasundhara Volam
Cc: mkubecek, David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
Wed, Dec 12, 2018 at 01:30:23PM CET, vasundhara-v.volam@broadcom.com wrote:
>On Wed, Dec 12, 2018 at 2:45 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>>
>> On Tue, Dec 11, 2018 at 02:16:47PM +0530, Vasundhara Volam wrote:
>> > wake-on-lan - Enables Wake on Lan for this port. If enabled,
>> > the controller asserts a wake pin based on the wake-on-lan type.
>> >
>> > Cc: Jiri Pirko <jiri@mellanox.com>
>> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>> > ---
>> > include/net/devlink.h | 16 ++++++++++++++++
>> > net/core/devlink.c | 5 +++++
>> > 2 files changed, 21 insertions(+)
>> >
>> > diff --git a/include/net/devlink.h b/include/net/devlink.h
>> > index db413d3..93a1030 100644
>> > --- a/include/net/devlink.h
>> > +++ b/include/net/devlink.h
>> > @@ -367,12 +367,25 @@ enum devlink_param_generic_id {
>> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
>> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
>> > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
>> > + DEVLINK_PARAM_GENERIC_ID_WOL,
>> >
>> > /* add new param generic ids above here*/
>> > __DEVLINK_PARAM_GENERIC_ID_MAX,
>> > DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
>> > };
>> >
>> > +enum devlink_param_wol_types {
>> > + DEVLINK_PARAM_WOL_DISABLE = 0,
>> > + DEVLINK_PARAM_WOL_MAGIC_PKT,
>> > + DEVLINK_PARAM_WOL_PHY,
>> > + DEVLINK_PARAM_WOL_UNICAST,
>> > + DEVLINK_PARAM_WOL_MULTICAST,
>> > + DEVLINK_PARAM_WOL_BROADCAST,
>> > + DEVLINK_PARAM_WOL_ARP,
>> > + DEVLINK_PARAM_WOL_SECURE,
>> > + DEVLINK_PARAM_WOL_FILTERS,
>> > +};
>> > +
>> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
>> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
>> >
>> > @@ -397,6 +410,9 @@ enum devlink_param_generic_id {
>> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
>> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
>> >
>> > +#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
>>
>> Nitpick: the other two parameter names use underscores, it would be more
>> consistent to use them as well.
>Sure, I will fix this in next version. Thanks.
>>
>> > +#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
>>
>> Can you be sure it will be always sufficient to enable only one WoL type
>> unlike in ethtool where bitfield is used so that you can enable multiple
>> types?
>Since this is a persistent setting, NIC may not support multiple types
>of WOL upon power on.
>If there is any requirement in future to add multiple types, data type
>can be modified,
It can't. It will become a UAPI. You need to prepare now.
>as other WOL types are also not added now.
>>
>> Michal Kubecek
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter
2018-12-13 12:56 ` Jiri Pirko
@ 2018-12-14 4:50 ` Vasundhara Volam
0 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-14 4:50 UTC (permalink / raw)
To: Jiří Pírko
Cc: mkubecek, David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Thu, Dec 13, 2018 at 6:33 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Wed, Dec 12, 2018 at 01:30:23PM CET, vasundhara-v.volam@broadcom.com wrote:
> >On Wed, Dec 12, 2018 at 2:45 PM Michal Kubecek <mkubecek@suse.cz> wrote:
> >>
> >> On Tue, Dec 11, 2018 at 02:16:47PM +0530, Vasundhara Volam wrote:
> >> > wake-on-lan - Enables Wake on Lan for this port. If enabled,
> >> > the controller asserts a wake pin based on the wake-on-lan type.
> >> >
> >> > Cc: Jiri Pirko <jiri@mellanox.com>
> >> > Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >> > ---
> >> > include/net/devlink.h | 16 ++++++++++++++++
> >> > net/core/devlink.c | 5 +++++
> >> > 2 files changed, 21 insertions(+)
> >> >
> >> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> >> > index db413d3..93a1030 100644
> >> > --- a/include/net/devlink.h
> >> > +++ b/include/net/devlink.h
> >> > @@ -367,12 +367,25 @@ enum devlink_param_generic_id {
> >> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
> >> > DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
> >> > DEVLINK_PARAM_GENERIC_ID_FW_LOAD_POLICY,
> >> > + DEVLINK_PARAM_GENERIC_ID_WOL,
> >> >
> >> > /* add new param generic ids above here*/
> >> > __DEVLINK_PARAM_GENERIC_ID_MAX,
> >> > DEVLINK_PARAM_GENERIC_ID_MAX = __DEVLINK_PARAM_GENERIC_ID_MAX - 1,
> >> > };
> >> >
> >> > +enum devlink_param_wol_types {
> >> > + DEVLINK_PARAM_WOL_DISABLE = 0,
> >> > + DEVLINK_PARAM_WOL_MAGIC_PKT,
> >> > + DEVLINK_PARAM_WOL_PHY,
> >> > + DEVLINK_PARAM_WOL_UNICAST,
> >> > + DEVLINK_PARAM_WOL_MULTICAST,
> >> > + DEVLINK_PARAM_WOL_BROADCAST,
> >> > + DEVLINK_PARAM_WOL_ARP,
> >> > + DEVLINK_PARAM_WOL_SECURE,
> >> > + DEVLINK_PARAM_WOL_FILTERS,
> >> > +};
> >> > +
> >> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_NAME "internal_error_reset"
> >> > #define DEVLINK_PARAM_GENERIC_INT_ERR_RESET_TYPE DEVLINK_PARAM_TYPE_BOOL
> >> >
> >> > @@ -397,6 +410,9 @@ enum devlink_param_generic_id {
> >> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_NAME "fw_load_policy"
> >> > #define DEVLINK_PARAM_GENERIC_FW_LOAD_POLICY_TYPE DEVLINK_PARAM_TYPE_U8
> >> >
> >> > +#define DEVLINK_PARAM_GENERIC_WOL_NAME "wake-on-lan"
> >>
> >> Nitpick: the other two parameter names use underscores, it would be more
> >> consistent to use them as well.
> >Sure, I will fix this in next version. Thanks.
> >>
> >> > +#define DEVLINK_PARAM_GENERIC_WOL_TYPE DEVLINK_PARAM_TYPE_U8
> >>
> >> Can you be sure it will be always sufficient to enable only one WoL type
> >> unlike in ethtool where bitfield is used so that you can enable multiple
> >> types?
> >Since this is a persistent setting, NIC may not support multiple types
> >of WOL upon power on.
> >If there is any requirement in future to add multiple types, data type
> >can be modified,
>
> It can't. It will become a UAPI. You need to prepare now.
>
>
Okay, I will make this definition close to ethtool WOL types.
Also rename according to ethtool.
> >as other WOL types are also not added now.
> >>
> >> Michal Kubecek
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it
2018-12-11 8:46 [PATCH net-next v2 RFC 0/8] devlink: Add configuration parameters support for devlink_port Vasundhara Volam
` (6 preceding siblings ...)
2018-12-11 8:46 ` [PATCH net-next v2 RFC 7/8] devlink: Add a generic port parameter Vasundhara Volam
@ 2018-12-11 8:46 ` Vasundhara Volam
2018-12-11 10:23 ` Jiri Pirko
7 siblings, 1 reply; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 8:46 UTC (permalink / raw)
To: davem; +Cc: michael.chan, jiri, jakub.kicinski, netdev
Register devlink_port with devlink and create initial port params
table for bnxt_en. The table consists of a generic parameter:
wake-on-lan: Enables Wake on Lan for this port when magic packet
is received with this port's MAC address using ACPI pattern.
If enabled, the controller asserts a wake pin upon reception of
WoL packet. ACPI (Advanced Configuration and Power Interface) is
an industry specification for the efficient handling of power
consumption in desktop and mobile computers.
Cc: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 +
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 44 ++++++++++++++++++++++-
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 1 +
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 3030931..1113e7c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1586,6 +1586,7 @@ struct bnxt {
/* devlink interface and vf-rep structs */
struct devlink *dl;
+ struct devlink_port dl_port;
enum devlink_eswitch_mode eswitch_mode;
struct bnxt_vf_rep **vf_reps; /* array of vf-rep ptrs */
u16 *cfa_code_map; /* cfa_code -> vf_idx map */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 140dbd6..7d084c3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -37,6 +37,8 @@ enum bnxt_dl_param_id {
NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
{BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
BNXT_NVM_SHARED_CFG, 1},
+
+ {DEVLINK_PARAM_GENERIC_ID_WOL, NVM_OFF_WOL, BNXT_NVM_PORT_CFG, 1},
};
static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
@@ -70,7 +72,8 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
bytesize = roundup(nvm_param.num_bits, BITS_PER_BYTE) / BITS_PER_BYTE;
switch (bytesize) {
case 1:
- if (nvm_param.num_bits == 1)
+ if (nvm_param.num_bits == 1 &&
+ nvm_param.id != DEVLINK_PARAM_GENERIC_ID_WOL)
buf = &val->vbool;
else
buf = &val->vu8;
@@ -164,6 +167,16 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
return 0;
}
+static int bnxt_dl_wol_validate(struct devlink *dl, u32 id,
+ union devlink_param_value val,
+ struct netlink_ext_ack *extack)
+{
+ if (val.vu8 != DEVLINK_PARAM_WOL_MAGIC_PKT ||
+ val.vu8 != DEVLINK_PARAM_WOL_DISABLE)
+ return -EINVAL;
+ return 0;
+}
+
static const struct devlink_param bnxt_dl_params[] = {
DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
BIT(DEVLINK_PARAM_CMODE_PERMANENT),
@@ -188,6 +201,12 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
NULL),
};
+static const struct devlink_param bnxt_dl_port_params[] = {
+ DEVLINK_PARAM_GENERIC(WOL, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+ bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
+ bnxt_dl_wol_validate),
+};
+
int bnxt_dl_register(struct bnxt *bp)
{
struct devlink *dl;
@@ -225,8 +244,28 @@ int bnxt_dl_register(struct bnxt *bp)
goto err_dl_unreg;
}
+ rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
+ if (rc) {
+ netdev_warn(bp->dev, "devlink_port_register failed. rc=%d",
+ rc);
+ goto err_dl_param_unreg;
+ }
+ devlink_port_type_eth_set(&bp->dl_port, bp->dev);
+
+ rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
+ ARRAY_SIZE(bnxt_dl_port_params));
+ if (rc) {
+ netdev_warn(bp->dev, "devlink_port_params_register failed. rc=%d",
+ rc);
+ goto err_dl_port_unreg;
+ }
return 0;
+err_dl_port_unreg:
+ devlink_port_unregister(&bp->dl_port);
+err_dl_param_unreg:
+ devlink_params_unregister(dl, bnxt_dl_params,
+ ARRAY_SIZE(bnxt_dl_params));
err_dl_unreg:
devlink_unregister(dl);
err_dl_free:
@@ -242,6 +281,9 @@ void bnxt_dl_unregister(struct bnxt *bp)
if (!dl)
return;
+ devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
+ ARRAY_SIZE(bnxt_dl_port_params));
+ devlink_port_unregister(&bp->dl_port);
devlink_params_unregister(dl, bnxt_dl_params,
ARRAY_SIZE(bnxt_dl_params));
devlink_unregister(dl);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
index 5b6b2c7..da065ca 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
@@ -35,6 +35,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
#define NVM_OFF_MSIX_VEC_PER_PF_MAX 108
#define NVM_OFF_MSIX_VEC_PER_PF_MIN 114
+#define NVM_OFF_WOL 152
#define NVM_OFF_IGNORE_ARI 164
#define NVM_OFF_DIS_GRE_VER_CHECK 171
#define NVM_OFF_ENABLE_SRIOV 401
--
1.8.3.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it
2018-12-11 8:46 ` [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it Vasundhara Volam
@ 2018-12-11 10:23 ` Jiri Pirko
2018-12-11 10:36 ` Vasundhara Volam
0 siblings, 1 reply; 25+ messages in thread
From: Jiri Pirko @ 2018-12-11 10:23 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, michael.chan, jiri, jakub.kicinski, netdev
Tue, Dec 11, 2018 at 09:46:48AM CET, vasundhara-v.volam@broadcom.com wrote:
>Register devlink_port with devlink and create initial port params
>table for bnxt_en. The table consists of a generic parameter:
>
>wake-on-lan: Enables Wake on Lan for this port when magic packet
>is received with this port's MAC address using ACPI pattern.
>If enabled, the controller asserts a wake pin upon reception of
>WoL packet. ACPI (Advanced Configuration and Power Interface) is
>an industry specification for the efficient handling of power
>consumption in desktop and mobile computers.
>
>Cc: Michael Chan <michael.chan@broadcom.com>
>Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
>---
> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 +
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 44 ++++++++++++++++++++++-
> drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 1 +
> 3 files changed, 45 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>index 3030931..1113e7c 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>@@ -1586,6 +1586,7 @@ struct bnxt {
>
> /* devlink interface and vf-rep structs */
> struct devlink *dl;
>+ struct devlink_port dl_port;
> enum devlink_eswitch_mode eswitch_mode;
> struct bnxt_vf_rep **vf_reps; /* array of vf-rep ptrs */
> u16 *cfa_code_map; /* cfa_code -> vf_idx map */
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>index 140dbd6..7d084c3 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
>@@ -37,6 +37,8 @@ enum bnxt_dl_param_id {
> NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
> {BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
> BNXT_NVM_SHARED_CFG, 1},
>+
>+ {DEVLINK_PARAM_GENERIC_ID_WOL, NVM_OFF_WOL, BNXT_NVM_PORT_CFG, 1},
> };
>
> static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
>@@ -70,7 +72,8 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
> bytesize = roundup(nvm_param.num_bits, BITS_PER_BYTE) / BITS_PER_BYTE;
> switch (bytesize) {
> case 1:
>- if (nvm_param.num_bits == 1)
>+ if (nvm_param.num_bits == 1 &&
>+ nvm_param.id != DEVLINK_PARAM_GENERIC_ID_WOL)
> buf = &val->vbool;
> else
> buf = &val->vu8;
>@@ -164,6 +167,16 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
> return 0;
> }
>
>+static int bnxt_dl_wol_validate(struct devlink *dl, u32 id,
>+ union devlink_param_value val,
>+ struct netlink_ext_ack *extack)
>+{
>+ if (val.vu8 != DEVLINK_PARAM_WOL_MAGIC_PKT ||
>+ val.vu8 != DEVLINK_PARAM_WOL_DISABLE)
You have extack here, please fill the message for the user so he knows
what is going on.
>+ return -EINVAL;
>+ return 0;
>+}
>+
> static const struct devlink_param bnxt_dl_params[] = {
> DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
> BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>@@ -188,6 +201,12 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
> NULL),
> };
>
>+static const struct devlink_param bnxt_dl_port_params[] = {
>+ DEVLINK_PARAM_GENERIC(WOL, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
>+ bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
>+ bnxt_dl_wol_validate),
>+};
>+
> int bnxt_dl_register(struct bnxt *bp)
> {
> struct devlink *dl;
>@@ -225,8 +244,28 @@ int bnxt_dl_register(struct bnxt *bp)
> goto err_dl_unreg;
> }
>
>+ rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
>+ if (rc) {
>+ netdev_warn(bp->dev, "devlink_port_register failed. rc=%d",
Why "warn"? This is "err".
>+ rc);
>+ goto err_dl_param_unreg;
>+ }
>+ devlink_port_type_eth_set(&bp->dl_port, bp->dev);
>+
>+ rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
>+ ARRAY_SIZE(bnxt_dl_port_params));
>+ if (rc) {
>+ netdev_warn(bp->dev, "devlink_port_params_register failed. rc=%d",
Same here. Also, no need to print "rc=x".
>+ rc);
>+ goto err_dl_port_unreg;
>+ }
> return 0;
>
>+err_dl_port_unreg:
>+ devlink_port_unregister(&bp->dl_port);
>+err_dl_param_unreg:
>+ devlink_params_unregister(dl, bnxt_dl_params,
>+ ARRAY_SIZE(bnxt_dl_params));
> err_dl_unreg:
> devlink_unregister(dl);
> err_dl_free:
>@@ -242,6 +281,9 @@ void bnxt_dl_unregister(struct bnxt *bp)
> if (!dl)
> return;
>
>+ devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
>+ ARRAY_SIZE(bnxt_dl_port_params));
>+ devlink_port_unregister(&bp->dl_port);
> devlink_params_unregister(dl, bnxt_dl_params,
> ARRAY_SIZE(bnxt_dl_params));
> devlink_unregister(dl);
>diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>index 5b6b2c7..da065ca 100644
>--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
>@@ -35,6 +35,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
>
> #define NVM_OFF_MSIX_VEC_PER_PF_MAX 108
> #define NVM_OFF_MSIX_VEC_PER_PF_MIN 114
>+#define NVM_OFF_WOL 152
> #define NVM_OFF_IGNORE_ARI 164
> #define NVM_OFF_DIS_GRE_VER_CHECK 171
> #define NVM_OFF_ENABLE_SRIOV 401
>--
>1.8.3.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH net-next v2 RFC 8/8] bnxt_en: Add bnxt_en initial port params table and register it
2018-12-11 10:23 ` Jiri Pirko
@ 2018-12-11 10:36 ` Vasundhara Volam
0 siblings, 0 replies; 25+ messages in thread
From: Vasundhara Volam @ 2018-12-11 10:36 UTC (permalink / raw)
To: Jiří Pírko
Cc: David Miller, michael.chan@broadcom.com, Jiri Pirko,
jakub.kicinski, Netdev
On Tue, Dec 11, 2018 at 4:00 PM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Tue, Dec 11, 2018 at 09:46:48AM CET, vasundhara-v.volam@broadcom.com wrote:
> >Register devlink_port with devlink and create initial port params
> >table for bnxt_en. The table consists of a generic parameter:
> >
> >wake-on-lan: Enables Wake on Lan for this port when magic packet
> >is received with this port's MAC address using ACPI pattern.
> >If enabled, the controller asserts a wake pin upon reception of
> >WoL packet. ACPI (Advanced Configuration and Power Interface) is
> >an industry specification for the efficient handling of power
> >consumption in desktop and mobile computers.
> >
> >Cc: Michael Chan <michael.chan@broadcom.com>
> >Signed-off-by: Vasundhara Volam <vasundhara-v.volam@broadcom.com>
> >---
> > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 1 +
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 44 ++++++++++++++++++++++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h | 1 +
> > 3 files changed, 45 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> >index 3030931..1113e7c 100644
> >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> >@@ -1586,6 +1586,7 @@ struct bnxt {
> >
> > /* devlink interface and vf-rep structs */
> > struct devlink *dl;
> >+ struct devlink_port dl_port;
> > enum devlink_eswitch_mode eswitch_mode;
> > struct bnxt_vf_rep **vf_reps; /* array of vf-rep ptrs */
> > u16 *cfa_code_map; /* cfa_code -> vf_idx map */
> >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> >index 140dbd6..7d084c3 100644
> >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> >@@ -37,6 +37,8 @@ enum bnxt_dl_param_id {
> > NVM_OFF_MSIX_VEC_PER_PF_MIN, BNXT_NVM_SHARED_CFG, 7},
> > {BNXT_DEVLINK_PARAM_ID_GRE_VER_CHECK, NVM_OFF_DIS_GRE_VER_CHECK,
> > BNXT_NVM_SHARED_CFG, 1},
> >+
> >+ {DEVLINK_PARAM_GENERIC_ID_WOL, NVM_OFF_WOL, BNXT_NVM_PORT_CFG, 1},
> > };
> >
> > static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
> >@@ -70,7 +72,8 @@ static int bnxt_hwrm_nvm_req(struct bnxt *bp, u32 param_id, void *msg,
> > bytesize = roundup(nvm_param.num_bits, BITS_PER_BYTE) / BITS_PER_BYTE;
> > switch (bytesize) {
> > case 1:
> >- if (nvm_param.num_bits == 1)
> >+ if (nvm_param.num_bits == 1 &&
> >+ nvm_param.id != DEVLINK_PARAM_GENERIC_ID_WOL)
> > buf = &val->vbool;
> > else
> > buf = &val->vu8;
> >@@ -164,6 +167,16 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
> > return 0;
> > }
> >
> >+static int bnxt_dl_wol_validate(struct devlink *dl, u32 id,
> >+ union devlink_param_value val,
> >+ struct netlink_ext_ack *extack)
> >+{
> >+ if (val.vu8 != DEVLINK_PARAM_WOL_MAGIC_PKT ||
> >+ val.vu8 != DEVLINK_PARAM_WOL_DISABLE)
>
> You have extack here, please fill the message for the user so he knows
> what is going on.
Okay Sure.
>
>
> >+ return -EINVAL;
> >+ return 0;
> >+}
> >+
> > static const struct devlink_param bnxt_dl_params[] = {
> > DEVLINK_PARAM_GENERIC(ENABLE_SRIOV,
> > BIT(DEVLINK_PARAM_CMODE_PERMANENT),
> >@@ -188,6 +201,12 @@ static int bnxt_dl_msix_validate(struct devlink *dl, u32 id,
> > NULL),
> > };
> >
> >+static const struct devlink_param bnxt_dl_port_params[] = {
> >+ DEVLINK_PARAM_GENERIC(WOL, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
> >+ bnxt_dl_nvm_param_get, bnxt_dl_nvm_param_set,
> >+ bnxt_dl_wol_validate),
> >+};
> >+
> > int bnxt_dl_register(struct bnxt *bp)
> > {
> > struct devlink *dl;
> >@@ -225,8 +244,28 @@ int bnxt_dl_register(struct bnxt *bp)
> > goto err_dl_unreg;
> > }
> >
> >+ rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
> >+ if (rc) {
> >+ netdev_warn(bp->dev, "devlink_port_register failed. rc=%d",
>
> Why "warn"? This is "err".
I will fix both of this below to err and remove rc=x in the next
version. Thanks.
>
>
> >+ rc);
> >+ goto err_dl_param_unreg;
> >+ }
> >+ devlink_port_type_eth_set(&bp->dl_port, bp->dev);
> >+
> >+ rc = devlink_port_params_register(&bp->dl_port, bnxt_dl_port_params,
> >+ ARRAY_SIZE(bnxt_dl_port_params));
> >+ if (rc) {
> >+ netdev_warn(bp->dev, "devlink_port_params_register failed. rc=%d",
>
> Same here. Also, no need to print "rc=x".
>
>
> >+ rc);
> >+ goto err_dl_port_unreg;
> >+ }
> > return 0;
> >
> >+err_dl_port_unreg:
> >+ devlink_port_unregister(&bp->dl_port);
> >+err_dl_param_unreg:
> >+ devlink_params_unregister(dl, bnxt_dl_params,
> >+ ARRAY_SIZE(bnxt_dl_params));
> > err_dl_unreg:
> > devlink_unregister(dl);
> > err_dl_free:
> >@@ -242,6 +281,9 @@ void bnxt_dl_unregister(struct bnxt *bp)
> > if (!dl)
> > return;
> >
> >+ devlink_port_params_unregister(&bp->dl_port, bnxt_dl_port_params,
> >+ ARRAY_SIZE(bnxt_dl_port_params));
> >+ devlink_port_unregister(&bp->dl_port);
> > devlink_params_unregister(dl, bnxt_dl_params,
> > ARRAY_SIZE(bnxt_dl_params));
> > devlink_unregister(dl);
> >diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> >index 5b6b2c7..da065ca 100644
> >--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> >+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.h
> >@@ -35,6 +35,7 @@ static inline void bnxt_link_bp_to_dl(struct bnxt *bp, struct devlink *dl)
> >
> > #define NVM_OFF_MSIX_VEC_PER_PF_MAX 108
> > #define NVM_OFF_MSIX_VEC_PER_PF_MIN 114
> >+#define NVM_OFF_WOL 152
> > #define NVM_OFF_IGNORE_ARI 164
> > #define NVM_OFF_DIS_GRE_VER_CHECK 171
> > #define NVM_OFF_ENABLE_SRIOV 401
> >--
> >1.8.3.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread