Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file

From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	John Linville <linville@tuxdriver.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	Johannes Berg <johannes@sipsolutions.net>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file
Date: Thu, 4 Jul 2019 10:09:54 +0200	[thread overview]
Message-ID: <20190704080954.GH2250@nanopsycho> (raw)
In-Reply-To: <20190703143722.GN20101@unicorn.suse.cz>

Wed, Jul 03, 2019 at 04:37:22PM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 03, 2019 at 03:44:52PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:19PM CEST, mkubecek@suse.cz wrote:
>> >Introduce file net/ethtool/common.c for code shared by ioctl and netlink
>> >ethtool interface. Move name tables of features, RSS hash functions,
>> >tunables and PHY tunables into this file.
>> >
>> >Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>> >---
>> > net/ethtool/Makefile |  2 +-
>> > net/ethtool/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
>> > net/ethtool/common.h | 17 +++++++++
>> > net/ethtool/ioctl.c  | 83 ++-----------------------------------------
>> > 4 files changed, 104 insertions(+), 82 deletions(-)
>> > create mode 100644 net/ethtool/common.c
>> > create mode 100644 net/ethtool/common.h
>> >
>> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>> >index 482fdb9380fa..11782306593b 100644
>> >--- a/net/ethtool/Makefile
>> >+++ b/net/ethtool/Makefile
>> >@@ -1,6 +1,6 @@
>> > # SPDX-License-Identifier: GPL-2.0
>> > 
>> >-obj-y				+= ioctl.o
>> >+obj-y				+= ioctl.o common.o
>> > 
>> > obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
>> > 
>> >diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>> >new file mode 100644
>> >index 000000000000..b0ce420e994e
>> >--- /dev/null
>> >+++ b/net/ethtool/common.c
>> >@@ -0,0 +1,84 @@
>> >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
>> >+
>> >+#include "common.h"
>> >+
>> >+const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
>> 
>> const char *netdev_features_strings[NETDEV_FEATURE_COUNT] = {
>> ?
>> 
>> Same with the other arrays.
>
>These are not new tables, this patch only moves existing tables from
>ioctl.c (originally net/core/ethtool.c) into common.c so that they can
>be used by both ioctl and netlink code.
>
>This fixed size string array format is used by ETHTOOL_GSTRINGS ioctl
>command. So if we switch these into simple const char *table[], we can
>get rid of some complexity in strset.c and bitset.c (the "simple" vs.
>"legacy" string set mess) but we would have to convert them into the
>fixed size string array in ioctl ETHTOOL_GSTRINGS handler. And then we
>would also have to convert (or rather "index") string sets retrieved
>from NIC driver (e.g. private flags, stats, tests) - which also means an
>extra kmalloc() (or rather kmalloc_array()).
>
>It an option I'm certainly open to if we agree on it but it's not for
>free.

Got it. I don't think we need to do this now. But it would be certainly
nice to fix this later on.

>
>Michal