From: Leon Romanovsky <[email protected]>
Devlink core exported generously the functions calls that were used
by netdevsim tests or not used at all.
Delete such APIs with one exception - devlink_alloc_ns(). That function
should be spared from deleting because it is a special form of devlink_alloc()
needed for the netdevsim.
Signed-off-by: Leon Romanovsky <[email protected]>
---
drivers/net/netdevsim/health.c | 32 -----------
include/net/devlink.h | 14 -----
net/core/devlink.c | 102 +--------------------------------
3 files changed, 3 insertions(+), 145 deletions(-)
diff --git a/drivers/net/netdevsim/health.c b/drivers/net/netdevsim/health.c
index 04aebdf85747..aa77af4a68df 100644
--- a/drivers/net/netdevsim/health.c
+++ b/drivers/net/netdevsim/health.c
@@ -110,26 +110,6 @@ static int nsim_dev_dummy_fmsg_put(struct devlink_fmsg *fmsg, u32 binary_len)
if (err)
return err;
- err = devlink_fmsg_arr_pair_nest_start(fmsg, "test_bool_array");
- if (err)
- return err;
- for (i = 0; i < 10; i++) {
- err = devlink_fmsg_bool_put(fmsg, true);
- if (err)
- return err;
- }
- err = devlink_fmsg_arr_pair_nest_end(fmsg);
- if (err)
- return err;
-
- err = devlink_fmsg_arr_pair_nest_start(fmsg, "test_u8_array");
- if (err)
- return err;
- for (i = 0; i < 10; i++) {
- err = devlink_fmsg_u8_put(fmsg, i);
- if (err)
- return err;
- }
err = devlink_fmsg_arr_pair_nest_end(fmsg);
if (err)
return err;
@@ -146,18 +126,6 @@ static int nsim_dev_dummy_fmsg_put(struct devlink_fmsg *fmsg, u32 binary_len)
if (err)
return err;
- err = devlink_fmsg_arr_pair_nest_start(fmsg, "test_u64_array");
- if (err)
- return err;
- for (i = 0; i < 10; i++) {
- err = devlink_fmsg_u64_put(fmsg, i);
- if (err)
- return err;
- }
- err = devlink_fmsg_arr_pair_nest_end(fmsg);
- if (err)
- return err;
-
err = devlink_fmsg_arr_pair_nest_start(fmsg, "test_array_of_objects");
if (err)
return err;
diff --git a/include/net/devlink.h b/include/net/devlink.h
index cd89b2dc2354..0e06b3dbbec6 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1663,18 +1663,7 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
union devlink_param_value *init_val);
int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
union devlink_param_value init_val);
-int
-devlink_port_param_driverinit_value_get(struct devlink_port *devlink_port,
- u32 param_id,
- union devlink_param_value *init_val);
-int devlink_port_param_driverinit_value_set(struct devlink_port *devlink_port,
- u32 param_id,
- union devlink_param_value init_val);
void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
-void devlink_port_param_value_changed(struct devlink_port *devlink_port,
- u32 param_id);
-void devlink_param_value_str_fill(union devlink_param_value *dst_val,
- const char *src);
struct devlink_region *
devlink_region_create(struct devlink *devlink,
const struct devlink_region_ops *ops,
@@ -1719,10 +1708,7 @@ int devlink_fmsg_binary_pair_nest_start(struct devlink_fmsg *fmsg,
const char *name);
int devlink_fmsg_binary_pair_nest_end(struct devlink_fmsg *fmsg);
-int devlink_fmsg_bool_put(struct devlink_fmsg *fmsg, bool value);
-int devlink_fmsg_u8_put(struct devlink_fmsg *fmsg, u8 value);
int devlink_fmsg_u32_put(struct devlink_fmsg *fmsg, u32 value);
-int devlink_fmsg_u64_put(struct devlink_fmsg *fmsg, u64 value);
int devlink_fmsg_string_put(struct devlink_fmsg *fmsg, const char *value);
int devlink_fmsg_binary_put(struct devlink_fmsg *fmsg, const void *value,
u16 value_len);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index f30121f07467..0f1663453ca0 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6269,23 +6269,21 @@ static int devlink_fmsg_put_value(struct devlink_fmsg *fmsg,
return 0;
}
-int devlink_fmsg_bool_put(struct devlink_fmsg *fmsg, bool value)
+static int devlink_fmsg_bool_put(struct devlink_fmsg *fmsg, bool value)
{
if (fmsg->putting_binary)
return -EINVAL;
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_FLAG);
}
-EXPORT_SYMBOL_GPL(devlink_fmsg_bool_put);
-int devlink_fmsg_u8_put(struct devlink_fmsg *fmsg, u8 value)
+static int devlink_fmsg_u8_put(struct devlink_fmsg *fmsg, u8 value)
{
if (fmsg->putting_binary)
return -EINVAL;
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U8);
}
-EXPORT_SYMBOL_GPL(devlink_fmsg_u8_put);
int devlink_fmsg_u32_put(struct devlink_fmsg *fmsg, u32 value)
{
@@ -6296,14 +6294,13 @@ int devlink_fmsg_u32_put(struct devlink_fmsg *fmsg, u32 value)
}
EXPORT_SYMBOL_GPL(devlink_fmsg_u32_put);
-int devlink_fmsg_u64_put(struct devlink_fmsg *fmsg, u64 value)
+static int devlink_fmsg_u64_put(struct devlink_fmsg *fmsg, u64 value)
{
if (fmsg->putting_binary)
return -EINVAL;
return devlink_fmsg_put_value(fmsg, &value, sizeof(value), NLA_U64);
}
-EXPORT_SYMBOL_GPL(devlink_fmsg_u64_put);
int devlink_fmsg_string_put(struct devlink_fmsg *fmsg, const char *value)
{
@@ -10257,55 +10254,6 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
}
EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
-/**
- * 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_reload_supported(devlink->ops))
- 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_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->index,
- &devlink_port->param_list,
- param_id, init_val,
- DEVLINK_CMD_PORT_PARAM_NEW);
-}
-EXPORT_SYMBOL_GPL(devlink_port_param_driverinit_value_set);
-
/**
* devlink_param_value_changed - notify devlink on a parameter's value
* change. Should be called by the driver
@@ -10329,50 +10277,6 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
}
EXPORT_SYMBOL_GPL(devlink_param_value_changed);
-/**
- * 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_param_value_str_fill - Safely fill-up the string preventing
- * from overflow of the preallocated buffer
- *
- * @dst_val: destination devlink_param_value
- * @src: source buffer
- */
-void devlink_param_value_str_fill(union devlink_param_value *dst_val,
- const char *src)
-{
- size_t len;
-
- len = strlcpy(dst_val->vstr, src, __DEVLINK_PARAM_MAX_STRING_VALUE);
- WARN_ON(len >= __DEVLINK_PARAM_MAX_STRING_VALUE);
-}
-EXPORT_SYMBOL_GPL(devlink_param_value_str_fill);
-
/**
* devlink_region_create - create a new address region
*
--
2.31.1
On Thu, Sep 16, 2021 at 06:33:18AM -0700, Jakub Kicinski wrote:
> On Thu, 16 Sep 2021 13:38:33 +0300 Leon Romanovsky wrote:
> > From: Leon Romanovsky <[email protected]>
> >
> > Devlink core exported generously the functions calls that were used
> > by netdevsim tests or not used at all.
> >
> > Delete such APIs with one exception - devlink_alloc_ns(). That function
> > should be spared from deleting because it is a special form of devlink_alloc()
> > needed for the netdevsim.
>
> Do you have a reason to do this or are you just cleaning up?
Yes for both questions. The trigger was my need to move parameter
notifications to be delayed till devlink register (like you asked). At
some point of time, I realized that devlink_*_publish() API is rubbish
and can be deleted (integrated into devlink_register). So I started to
cleanup as much as possible.
>
> The fmsg functions are not actually removed, just unexported.
> Are there out of tree drivers abusing them?
I don't know, but exported symbols pollute symbols table and the less we
have there, the better will be for everyone.
>
> The port_param functions are "symmetric" with the global param
> ones. Removing them makes the API look somewhat incomplete.
There is no value in having "complete" API that no one uses.
>
> Obviously the general guidance is that we shouldn't export
> functions which have no upstream users but that applies to
> meaningful APIs. For all practical purposes this is just a
> sliver of an API, completeness gives nice warm feelings.
It is misleading, I have much more warm feeling when I see API that is
used. Once it will be needed, the next developer will copy/paste it
pretty fast.
>
> Anyway, just curious what made you do this. I wouldn't do it
> myself but neither am I substantially opposed.
Move of devlink_register() to be last command in the devlink init flow
and removal of devlink_*_publish() calls as an outcome of that.
Thanks
On Thu, 16 Sep 2021 13:38:33 +0300 Leon Romanovsky wrote:
> From: Leon Romanovsky <[email protected]>
>
> Devlink core exported generously the functions calls that were used
> by netdevsim tests or not used at all.
>
> Delete such APIs with one exception - devlink_alloc_ns(). That function
> should be spared from deleting because it is a special form of devlink_alloc()
> needed for the netdevsim.
Do you have a reason to do this or are you just cleaning up?
The fmsg functions are not actually removed, just unexported.
Are there out of tree drivers abusing them?
The port_param functions are "symmetric" with the global param
ones. Removing them makes the API look somewhat incomplete.
Obviously the general guidance is that we shouldn't export
functions which have no upstream users but that applies to
meaningful APIs. For all practical purposes this is just a
sliver of an API, completeness gives nice warm feelings.
Anyway, just curious what made you do this. I wouldn't do it
myself but neither am I substantially opposed.
On Thu, 16 Sep 2021 16:52:02 +0300 Leon Romanovsky wrote:
> > The port_param functions are "symmetric" with the global param
> > ones. Removing them makes the API look somewhat incomplete.
>
> There is no value in having "complete" API that no one uses.
Well, for an API which we are hoping to attract vendors to, the
"completeness" could be useful. If kernel needs to be extended
some will fall back to their out of tree tools.
> > Obviously the general guidance is that we shouldn't export
> > functions which have no upstream users but that applies to
> > meaningful APIs. For all practical purposes this is just a
> > sliver of an API, completeness gives nice warm feelings.
>
> It is misleading, I have much more warm feeling when I see API that is
> used. Once it will be needed, the next developer will copy/paste it
> pretty fast.
>
> > Anyway, just curious what made you do this. I wouldn't do it
> > myself but neither am I substantially opposed.
>
> Move of devlink_register() to be last command in the devlink init flow
> and removal of devlink_*_publish() calls as an outcome of that.
Alrighty:
Acked-by: Jakub Kicinski <[email protected]>
Thu, Sep 16, 2021 at 12:38:33PM CEST, [email protected] wrote:
>From: Leon Romanovsky <[email protected]>
>
>Devlink core exported generously the functions calls that were used
>by netdevsim tests or not used at all.
>
>Delete such APIs with one exception - devlink_alloc_ns(). That function
>should be spared from deleting because it is a special form of devlink_alloc()
>needed for the netdevsim.
>
>Signed-off-by: Leon Romanovsky <[email protected]>
Reviewed-by: Jiri Pirko <[email protected]>
Hello:
This patch was applied to netdev/net-next.git (refs/heads/master):
On Thu, 16 Sep 2021 13:38:33 +0300 you wrote:
> From: Leon Romanovsky <[email protected]>
>
> Devlink core exported generously the functions calls that were used
> by netdevsim tests or not used at all.
>
> Delete such APIs with one exception - devlink_alloc_ns(). That function
> should be spared from deleting because it is a special form of devlink_alloc()
> needed for the netdevsim.
>
> [...]
Here is the summary with links:
- [net-next] devlink: Delete not-used devlink APIs
https://git.kernel.org/netdev/net-next/c/6db9350a9db3
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html