Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

[Michal Kubecek]

On Mon, Dec 11, 2017 at 01:45:47PM -0500, David Miller wrote:
> From: Jiri Pirko 
> Date: Mon, 11 Dec 2017 19:02:19 +0100
> 
> > The discussion we had before was about flag bitfield that was there
> > *always*. In this case, that is not true. It is either ifindex or
> > ifname. Even rtnetlink has ifname as attribute.
> > 
> > The flags and info_mask is just big mystery. If it is per-command,
> > seems natural to have it as attributes.
> 
> I think flags and info_mask indeed can be moved out of this struct.
> 
> I guess, in this case, I can see your point of view especially if we
> allow ethtool operations on non-netdev entities.
> 
> So, ok, let's move forward without a base command struct and just
> use attributes.

OK, I'll rework the interface to use attributes for all data.

Michal Kubecek

Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

[David Miller]

From: Jiri Pirko 
Date: Mon, 11 Dec 2017 19:02:19 +0100

> The discussion we had before was about flag bitfield that was there
> *always*. In this case, that is not true. It is either ifindex or
> ifname. Even rtnetlink has ifname as attribute.
> 
> The flags and info_mask is just big mystery. If it is per-command,
> seems natural to have it as attributes.

I think flags and info_mask indeed can be moved out of this struct.

I guess, in this case, I can see your point of view especially if we
allow ethtool operations on non-netdev entities.

So, ok, let's move forward without a base command struct and just
use attributes.

Thanks :)

Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

[Jiri Pirko]

Mon, Dec 11, 2017 at 05:56:51PM CET, da...@davemloft.net wrote:
>From: Jiri Pirko 
>Date: Mon, 11 Dec 2017 17:02:21 +0100
>
>> Mon, Dec 11, 2017 at 02:53:31PM CET, mkube...@suse.cz wrote:
>>>No function implemented yet, only genetlink and module infrastructure.
>>>Register/unregister genetlink family "ethtool" and allow the module to be
>>>autoloaded by genetlink code (if built as a module, distributions would
>>>probably prefer "y").
>>>
>>>Signed-off-by: Michal Kubecek 
>> 
>> [...]
>> 
>> 
>>>+
>>>+/* identifies the device to query/set
>>>+ * - use either ifindex or ifname, not both
>>>+ * - for dumps and messages not related to a particular devices, fill 
>>>neither
>>>+ * - info_mask is a bitfield, interpretation depends on the command
>>>+ */
>>>+struct ethnlmsghdr {
>>>+__u32   ifindex;/* device ifindex */
>>>+__u16   flags;  /* request/response flags */
>>>+__u16   info_mask;  /* request/response info mask */
>>>+charifname[IFNAMSIZ];   /* device name */
>> 
>> Why do you need this header? You can have 2 attrs:
>> ETHTOOL_ATTR_IFINDEX and
>> ETHTOOL_ATTR_IFNAME
>> 
>> Why do you need per-command flags and info_mask? Could be bitfield
>> attr if needed by specific command.
>
>Jiri, we've had this discussion before :-)
>
>For elements which are common to most, if not all, requests it makes
>sense to define a base netlink message.
>
>My opinion on this matter has not changed at all since the last time
>we discussed this.

The discussion we had before was about flag bitfield that was there
*always*. In this case, that is not true. It is either ifindex or
ifname. Even rtnetlink has ifname as attribute.

The flags and info_mask is just big mystery. If it is per-command,
seems natural to have it as attributes.


>
>So unless you have new information to provide to me on this issue
>which might change my mind, please accept the result of the previous
>discussion which is that a base netlink message is not only
>appropriate but desirable.
>
>Thank you.

Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

[David Miller]

From: Jiri Pirko 
Date: Mon, 11 Dec 2017 17:02:21 +0100

> Mon, Dec 11, 2017 at 02:53:31PM CET, mkube...@suse.cz wrote:
>>No function implemented yet, only genetlink and module infrastructure.
>>Register/unregister genetlink family "ethtool" and allow the module to be
>>autoloaded by genetlink code (if built as a module, distributions would
>>probably prefer "y").
>>
>>Signed-off-by: Michal Kubecek 
> 
> [...]
> 
> 
>>+
>>+/* identifies the device to query/set
>>+ * - use either ifindex or ifname, not both
>>+ * - for dumps and messages not related to a particular devices, fill neither
>>+ * - info_mask is a bitfield, interpretation depends on the command
>>+ */
>>+struct ethnlmsghdr {
>>+ __u32   ifindex;/* device ifindex */
>>+ __u16   flags;  /* request/response flags */
>>+ __u16   info_mask;  /* request/response info mask */
>>+ charifname[IFNAMSIZ];   /* device name */
> 
> Why do you need this header? You can have 2 attrs:
> ETHTOOL_ATTR_IFINDEX and
> ETHTOOL_ATTR_IFNAME
> 
> Why do you need per-command flags and info_mask? Could be bitfield
> attr if needed by specific command.

Jiri, we've had this discussion before :-)

For elements which are common to most, if not all, requests it makes
sense to define a base netlink message.

My opinion on this matter has not changed at all since the last time
we discussed this.

So unless you have new information to provide to me on this issue
which might change my mind, please accept the result of the previous
discussion which is that a base netlink message is not only
appropriate but desirable.

Thank you.

Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

[Jiri Pirko]

Mon, Dec 11, 2017 at 02:53:31PM CET, mkube...@suse.cz wrote:
>No function implemented yet, only genetlink and module infrastructure.
>Register/unregister genetlink family "ethtool" and allow the module to be
>autoloaded by genetlink code (if built as a module, distributions would
>probably prefer "y").
>
>Signed-off-by: Michal Kubecek 

[...]


>+
>+/* identifies the device to query/set
>+ * - use either ifindex or ifname, not both
>+ * - for dumps and messages not related to a particular devices, fill neither
>+ * - info_mask is a bitfield, interpretation depends on the command
>+ */
>+struct ethnlmsghdr {
>+  __u32   ifindex;/* device ifindex */
>+  __u16   flags;  /* request/response flags */
>+  __u16   info_mask;  /* request/response info mask */
>+  charifname[IFNAMSIZ];   /* device name */

Why do you need this header? You can have 2 attrs:
ETHTOOL_ATTR_IFINDEX and
ETHTOOL_ATTR_IFNAME

Why do you need per-command flags and info_mask? Could be bitfield
attr if needed by specific command.



>+};
>+
>+#define ETHNL_HDRLEN NLMSG_ALIGN(sizeof(struct ethnlmsghdr))
>+
>+enum {
>+  ETHTOOL_CMD_NOOP,
>+
>+  __ETHTOOL_CMD_MAX,
>+  ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
>+};
>+
>+/* generic netlink info */
>+#define ETHTOOL_GENL_NAME "ethtool"
>+#define ETHTOOL_GENL_VERSION 1
>+
>+#endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/net/Kconfig b/net/Kconfig
>index 9dba2715919d..a5e3c89a2495 100644
>--- a/net/Kconfig
>+++ b/net/Kconfig
>@@ -440,6 +440,13 @@ config MAY_USE_DEVLINK
> on MAY_USE_DEVLINK to ensure they do not cause link errors when
> devlink is a loadable module and the driver using it is built-in.
> 
>+config ETHTOOL_NETLINK
>+  tristate "Netlink interface for ethtool"
>+  default m
>+  help
>+ New netlink based interface for ethtool which is going to obsolete
>+ the old ioctl based one once it provides all features.
>+
> endif   # if NET
> 
> # Used by archs to tell that they support BPF JIT compiler plus which flavour.
>diff --git a/net/core/Makefile b/net/core/Makefile
>index 1fd0a9c88b1b..617ab2abecdf 100644
>--- a/net/core/Makefile
>+++ b/net/core/Makefile
>@@ -30,3 +30,4 @@ obj-$(CONFIG_DST_CACHE) += dst_cache.o
> obj-$(CONFIG_HWBM) += hwbm.o
> obj-$(CONFIG_NET_DEVLINK) += devlink.o
> obj-$(CONFIG_GRO_CELLS) += gro_cells.o
>+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_netlink.o
>diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
>new file mode 100644
>index ..46a226bb9a2c
>--- /dev/null
>+++ b/net/core/ethtool_netlink.c
>@@ -0,0 +1,46 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+
>+#include 
>+#include 
>+#include 
>+#include 
>+#include "ethtool_common.h"
>+
>+static struct genl_family ethtool_genl_family;
>+
>+/* genetlink paperwork */

What "paperwork"? :)


>+
>+static const struct genl_ops ethtool_genl_ops[] = {
>+};
>+
>+static struct genl_family ethtool_genl_family = {
>+  .hdrsize= ETHNL_HDRLEN,
>+  .name   = ETHTOOL_GENL_NAME,
>+  .version= ETHTOOL_GENL_VERSION,
>+  .netnsok= true,
>+  .ops= ethtool_genl_ops,
>+  .n_ops  = ARRAY_SIZE(ethtool_genl_ops),
>+};
>+
>+/* module paperwork */
>+
>+static int __init ethtool_nl_init(void)
>+{
>+  return genl_register_family(_genl_family);
>+}
>+
>+static void __exit ethtool_nl_exit(void)
>+{
>+  genl_unregister_family(_genl_family);
>+}
>+
>+module_init(ethtool_nl_init);
>+module_exit(ethtool_nl_exit);
>+
>+/* this alias is for autoload */
>+MODULE_ALIAS("net-pf-" __stringify(PF_NETLINK)
>+   "-proto-" __stringify(NETLINK_GENERIC)
>+   "-family-" ETHTOOL_GENL_NAME);

You can use MODULE_ALIAS_GENL_FAMILY instead.


>+MODULE_AUTHOR("Michal Kubecek ");
>+MODULE_DESCRIPTION("Netlink interface for ethtool");
>+MODULE_LICENSE("GPL");

"GPL v2"?


>-- 
>2.15.1
>