[patch 12/12] Configure out ethtool support

[patch 12/12] Configure out ethtool support

[akpm]

From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Add the CONFIG_ETHTOOL option which allows to remove support for ethtool,
not necessarly used on embedded systems.  As this is a size-reduction
option, it depends on CONFIG_EMBEDDED.  It allows to save ~6 kilobytes of
kernel code:

   text	   data	    bss	    dec	    hex	filename
1258447	 123592	 212992	1595031	 185697	vmlinux
1252147	 123592	 212992	1588731	 183dfb	vmlinux.new
  -6300       0       0   -6300   -189C +/-

Question: should we also remove ethtool-related functions from all network
drivers ?

This patch has been originally written by Matt Mackall
<mpm@selenic.com>, and is part of the Linux Tiny project.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Matt Mackall <mpm@selenic.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/ethtool.h |   16 ++++++++++++++++
 init/Kconfig            |    8 ++++++++
 net/core/Makefile       |    3 ++-
 net/core/dev.c          |    4 ++++
 4 files changed, 30 insertions(+), 1 deletion(-)

diff -puN include/linux/ethtool.h~configure-out-ethtool-support include/linux/ethtool.h
--- a/include/linux/ethtool.h~configure-out-ethtool-support
+++ a/include/linux/ethtool.h
@@ -283,6 +283,7 @@ struct ethtool_rxnfc {
 struct net_device;
 
 /* Some generic methods drivers may use in their ethtool_ops */
+#ifdef CONFIG_ETHTOOL
 u32 ethtool_op_get_link(struct net_device *dev);
 u32 ethtool_op_get_tx_csum(struct net_device *dev);
 int ethtool_op_set_tx_csum(struct net_device *dev, u32 data);
@@ -296,6 +297,21 @@ u32 ethtool_op_get_ufo(struct net_device
 int ethtool_op_set_ufo(struct net_device *dev, u32 data);
 u32 ethtool_op_get_flags(struct net_device *dev);
 int ethtool_op_set_flags(struct net_device *dev, u32 data);
+#else
+static inline u32 ethtool_op_get_link(struct net_device *dev) { return 0; }
+static inline u32 ethtool_op_get_tx_csum(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_tx_csum(struct net_device *dev, u32 data) { return 0; }
+static inline int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data) { return 0; }
+static inline int ethtool_op_set_tx_ipv6_csum(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_sg(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_sg(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_tso(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_tso(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_ufo(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_ufo(struct net_device *dev, u32 data) { return 0; }
+static inline u32 ethtool_op_get_flags(struct net_device *dev) { return 0; }
+static inline int ethtool_op_set_flags(struct net_device *dev, u32 data) { return 0; }
+#endif
 
 /**
  * &ethtool_ops - Alter and report network device settings
diff -puN init/Kconfig~configure-out-ethtool-support init/Kconfig
--- a/init/Kconfig~configure-out-ethtool-support
+++ a/init/Kconfig
@@ -732,6 +732,14 @@ config IGMP
 	  Disabling this option removes support for the Internet group
           management protocol, used for multicast. Saves about 10k.
 
+config ETHTOOL
+	bool "Enable ethtool support" if EMBEDDED
+	depends on NET
+	default y
+	help
+	  Disabling this option removes support for configuring
+	  ethernet device features via ethtool. Saves about 6k.
+
 config VM_EVENT_COUNTERS
 	default y
 	bool "Enable VM event counters for /proc/vmstat" if EMBEDDED
diff -puN net/core/Makefile~configure-out-ethtool-support net/core/Makefile
--- a/net/core/Makefile~configure-out-ethtool-support
+++ a/net/core/Makefile
@@ -7,10 +7,11 @@ obj-y := sock.o request_sock.o skbuff.o 
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
 
-obj-y		     += dev.o ethtool.o dev_mcast.o dst.o netevent.o \
+obj-y		     += dev.o dev_mcast.o dst.o netevent.o \
 			neighbour.o rtnetlink.o utils.o link_watch.o filter.o
 
 obj-$(CONFIG_XFRM) += flow.o
+obj-$(CONFIG_ETHTOOL) += ethtool.o
 obj-y += net-sysfs.o
 obj-$(CONFIG_NET_PKTGEN) += pktgen.o
 obj-$(CONFIG_NETPOLL) += netpoll.o
diff -puN net/core/dev.c~configure-out-ethtool-support net/core/dev.c
--- a/net/core/dev.c~configure-out-ethtool-support
+++ a/net/core/dev.c
@@ -3669,6 +3669,7 @@ int dev_ioctl(struct net *net, unsigned 
 			return ret;
 
 		case SIOCETHTOOL:
+#ifdef CONFIG_ETHTOOL
 			dev_load(net, ifr.ifr_name);
 			rtnl_lock();
 			ret = dev_ethtool(net, &ifr);
@@ -3681,6 +3682,9 @@ int dev_ioctl(struct net *net, unsigned 
 					ret = -EFAULT;
 			}
 			return ret;
+#else
+			return -EINVAL;
+#endif
 
 		/*
 		 *	These ioctl calls:
_

Re: [patch 12/12] Configure out ethtool support

[Stephen Hemminger]

On Wed, 30 Jul 2008 12:39:04 -0700
akpm@linux-foundation.org wrote:

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Add the CONFIG_ETHTOOL option which allows to remove support for ethtool,
> not necessarly used on embedded systems.  As this is a size-reduction
> option, it depends on CONFIG_EMBEDDED.  It allows to save ~6 kilobytes of
> kernel code:
> 
>    text	   data	    bss	    dec	    hex	filename
> 1258447	 123592	 212992	1595031	 185697	vmlinux
> 1252147	 123592	 212992	1588731	 183dfb	vmlinux.new
>   -6300       0       0   -6300   -189C +/-
> 
> Question: should we also remove ethtool-related functions from all network
> drivers ?
> 
> This patch has been originally written by Matt Mackall
> <mpm@selenic.com>, and is part of the Linux Tiny project.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jeff Garzik <jeff@garzik.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

This option needs to be forced on when bridging or bonding is selected.

Re: [patch 12/12] Configure out ethtool support

[Thomas Petazzoni]

Le Wed, 30 Jul 2008 13:37:27 -0700,
Stephen Hemminger <shemminger@vyatta.com> a écrit :

> This option needs to be forced on when bridging or bonding is
> selected.

Do you mean something like the following fixup patch ?

Thanks,

Thomas

Make sure ETHTOOL support is selected when bonding and bridging are
requested.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
 drivers/net/Kconfig |    1 +
 net/bridge/Kconfig  |    1 +
 2 files changed, 2 insertions(+)

Index: linuxdev/drivers/net/Kconfig
===================================================================
--- linuxdev.orig/drivers/net/Kconfig
+++ linuxdev/drivers/net/Kconfig
@@ -61,6 +61,7 @@
 config BONDING
 	tristate "Bonding driver support"
 	depends on INET
+	select ETHTOOL
 	---help---
 	  Say 'Y' or 'M' if you wish to be able to 'bond' multiple Ethernet
 	  Channels together. This is called 'Etherchannel' by Cisco,
Index: linuxdev/net/bridge/Kconfig
===================================================================
--- linuxdev.orig/net/bridge/Kconfig
+++ linuxdev/net/bridge/Kconfig
@@ -6,6 +6,7 @@
 	tristate "802.1d Ethernet Bridging"
 	select LLC
 	select STP
+	select ETHTOOL
 	---help---
 	  If you say Y here, then your Linux box will be able to act as an
 	  Ethernet bridge, which means that the different Ethernet segments it


-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 12/12] Configure out ethtool support

[Stephen Hemminger]

On Wed, 30 Jul 2008 22:48:49 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Le Wed, 30 Jul 2008 13:37:27 -0700,
> Stephen Hemminger <shemminger@vyatta.com> a écrit :
> 
> > This option needs to be forced on when bridging or bonding is
> > selected.
> 
> Do you mean something like the following fixup patch ?
> 
> Thanks,
> 
> Thomas
> 
> Make sure ETHTOOL support is selected when bonding and bridging are
> requested.

Yes, but still concerned that the loss of functionality. The kernel developers
have been preaching to get rid of module parameters for configuration, and use
a standard interfaces such as ethtool;then the embedded folks want to save a
meager 6K.

Re: [patch 12/12] Configure out ethtool support

[Thomas Petazzoni]

Le Wed, 30 Jul 2008 14:01:36 -0700,
Stephen Hemminger <shemminger@vyatta.com> a écrit :

> Yes, but still concerned that the loss of functionality. The kernel
> developers have been preaching to get rid of module parameters for
> configuration, and use a standard interfaces such as ethtool;then the
> embedded folks want to save a meager 6K.

I understand your point, and I understand that adding many
configuration options might not look pretty to everybody (even to me).
However, the kernel developers want to keep embedded people using recent
versions of the Linux kernel. Unfortunately, the kernel grows release
after release (see [1] for a report, for example), making the kernel
less and less usable in certain constrainted embedded contexts where
older kernels can be used. And people putting Linux in consumer
electronics devices sold several millions times really do care about
system size.

The problem is that this kernel growth is well-spread over all the code.
So yes, it's 6k here, 10k here, 7k there, but once added, it starts to
be significant.

That said, I'm well opened to suggestions on how to better fix the
kernel in that respect.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 12/12] Configure out ethtool support

[Andrew Morton]

On Wed, 30 Jul 2008 23:35:51 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Le Wed, 30 Jul 2008 14:01:36 -0700,
> Stephen Hemminger <shemminger@vyatta.com> a __crit :
> 
> > Yes, but still concerned that the loss of functionality. The kernel
> > developers have been preaching to get rid of module parameters for
> > configuration, and use a standard interfaces such as ethtool;then the
> > embedded folks want to save a meager 6K.
> 
> I understand your point, and I understand that adding many
> configuration options might not look pretty to everybody (even to me).
> However, the kernel developers want to keep embedded people using recent
> versions of the Linux kernel. Unfortunately, the kernel grows release
> after release (see [1] for a report, for example),

There is no [1].

>  making the kernel
> less and less usable in certain constrainted embedded contexts where
> older kernels can be used. And people putting Linux in consumer
> electronics devices sold several millions times really do care about
> system size.
> 
> The problem is that this kernel growth is well-spread over all the code.
> So yes, it's 6k here, 10k here, 7k there, but once added, it starts to
> be significant.

I note that everybody thinks that their bit cannot possibly be removed,
and that everyone else's can ;)

But I do think we should see some evidence that people are actually
using CONFIG_ETHTOOL=n in real setups.

Re: [patch 12/12] Configure out ethtool support

[Thomas Petazzoni]

Le Wed, 30 Jul 2008 14:48:12 -0700,
Andrew Morton <akpm@linux-foundation.org> a écrit :

> > I understand your point, and I understand that adding many
> > configuration options might not look pretty to everybody (even to
> > me). However, the kernel developers want to keep embedded people
> > using recent versions of the Linux kernel. Unfortunately, the
> > kernel grows release after release (see [1] for a report, for
> > example),
> 
> There is no [1].

Oops, sorry. [1] should have been
http://free-electrons.com/articles/conferences/elc2008-report#mackall.

«
The graph showed an increase of 28% on the kernel size over the last
two and half years. Over the last year, between 2.6.21 and 2.6.25-rc8,
the kernel size of the same allnoconfig has increased from 1.06
megabytes to 1.21 megabytes, a 14% increase.
»

Sincerly,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers and embedded Linux development,
consulting, training and support.
http://free-electrons.com

Re: [patch 12/12] Configure out ethtool support

[Andrew Morton]

On Wed, 30 Jul 2008 23:52:31 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Oops, sorry. [1] should have been
> http://free-electrons.com/articles/conferences/elc2008-report#mackall.
> 
> __
> The graph showed an increase of 28% on the kernel size over the last
> two and half years. Over the last year, between 2.6.21 and 2.6.25-rc8,
> the kernel size of the same allnoconfig has increased from 1.06
> megabytes to 1.21 megabytes, a 14% increase.

OK.

I don't really trust allnoconfig as a way of determining size changes. 
It can easily be the case that kernel A's allnoconfig happens to pull
in more stuff than kernel B's allnoconfig.

So I do think that one should dive into the details and verify that
particular size changes really are due to unavoidable code bloat,
rather than being due to some unfortunate Kconfig change.

Tht being said, yes, I'm sure that it's getting bigger, overall.

Re: [patch 12/12] Configure out ethtool support

[Roland Dreier]

 > I don't really trust allnoconfig as a way of determining size changes. 
 > It can easily be the case that kernel A's allnoconfig happens to pull
 > in more stuff than kernel B's allnoconfig.
 > 
 > So I do think that one should dive into the details and verify that
 > particular size changes really are due to unavoidable code bloat,
 > rather than being due to some unfortunate Kconfig change.

The following is rather squishy, but... when I've looked at the growth
of various bits of the kernel over time, a good bit of the growth comes
from changes like "add handling for undeniably useful feature X" or "add
handling of error condition Y."  And it's 50 bytes here, 100 bytes
there, and all the changes make sense and are too small to make
configurable.  So being able to get 6K all at once from ethtool is
actually pretty good, although probably not quite good enough to merge.

I really don't know what the right answer is to all of this.

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 30 Jul 2008 14:48:12 -0700

> But I do think we should see some evidence that people are actually
> using CONFIG_ETHTOOL=n in real setups.

Heaven help them if they want to debug any real problems that
might involve checksumming, TSO, and other offload features.
Heaven help them if they want to probe the driver or firmware
version of the card they are using.

I'm sure you can find someone who runs with CONFIG_STAND_ON_MY_HEAD=y
but that doesn't mean that supporting such an option upstream makes
any sense.

We spent years getting rid of the per-driver module parameters to
configure these things.  And now folks want to remove the common
infrastructure we created to do it.

6K is peanuts compared to the per-driver code that implements these
facilities.

Re: [patch 12/12] Configure out ethtool support

[Andrew Morton]

On Wed, 30 Jul 2008 14:57:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Wed, 30 Jul 2008 14:48:12 -0700
> 
> > But I do think we should see some evidence that people are actually
> > using CONFIG_ETHTOOL=n in real setups.
> 
> Heaven help them if they want to debug any real problems that
> might involve checksumming, TSO, and other offload features.
> Heaven help them if they want to probe the driver or firmware
> version of the card they are using.

Presumably they would enable ethtool (and a whole bunch of other
debugging things) during development.  In previous lives I've seen
development boards which have double the memory of production boards
simply so that the developers can get their work done.

> I'm sure you can find someone who runs with CONFIG_STAND_ON_MY_HEAD=y
> but that doesn't mean that supporting such an option upstream makes
> any sense.

Well yes, embedded developers do patch the hell out of their kernels
and presumably they will always do so.  The idea here is to bring
things inwards to reduce the amount of patching which these people need
to do.

If they don't do this then tens of millions of devices will be burdened
with many kilobytes of code which they will never ever need.

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Wed, 30 Jul 2008 15:13:16 -0700

> If they don't do this then tens of millions of devices will be burdened
> with many kilobytes of code which they will never ever need.

In the current tree, ethtool is even needed with just the core
IPV4 stack, in order to disable LRO when forwarding is enabled.

See dev_disable_lro() and it's callers in net/ipv4/devinet.c
et al.

As Stephen Hemminger mentioned, bridging uses this too.

So essentially, this new config option would need to depend
on CONFIG_INET, and how useful is that?

Re: [patch 12/12] Configure out ethtool support

[David Woodhouse]

On Wed, 2008-07-30 at 15:13 -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 14:57:27 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> > I'm sure you can find someone who runs with CONFIG_STAND_ON_MY_HEAD=y
> > but that doesn't mean that supporting such an option upstream makes
> > any sense.

That's not entirely consistent with Linus' observations on similar
topics. He seems to think that if people need certain things and would
be patching them in anyway, then it makes sense to merge them upstream.

> Well yes, embedded developers do patch the hell out of their kernels
> and presumably they will always do so.  The idea here is to bring
> things inwards to reduce the amount of patching which these people
> need to do.

Distro vendors used to do that too.

We need to be moving towards a situation where people don't _need_ to
patch the hell out of their kernels to make Linux viable for embedded
devices. I'm not prepared just assume that 'they will always do so', and
I'm disappointed to hear Andrew say such a thing. 

We need to do better than that.

In particular, we need not to simultaneously:
 - whine that "embedded" folks never submit their work upstream, and
 - reject it on ideological grounds when they do, citing totally bogus
   'technical' reasons.

Dave, if you won't take these patches then I will (after review) put
them in the embedded-2.6 tree with Andrew's sign-off, and ask Linus to
pull them into 2.6.28 directly from there rather than via your tree.
I think he'll take them, because it's the right thing to do.

I would prefer not to have to do that; please reconsider your refusal.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:39:36 +0100

> We need to be moving towards a situation where people don't _need_ to
> patch the hell out of their kernels to make Linux viable for embedded
> devices. I'm not prepared just assume that 'they will always do so', and
> I'm disappointed to hear Andrew say such a thing. 

It's simple economics, really.

Until they see value in upstream submission, it's always going
to be %100 invested in getting the current product out the door
or the current fire put out.

The odd I2C device hack of the day the do to one revision of their
product is likely never to be submitted upstream.  And these
embedded folks have hundreds of things on that level that they
sort out for each product, and often very little of it is reusable
in the next product.

They don't see the payback for upstreaming this stuff, and for
a lot of cases neither do I.

Re: [patch 12/12] Configure out ethtool support

[David Woodhouse]

On Thu, 2008-07-31 at 03:43 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Thu, 31 Jul 2008 11:39:36 +0100
> 
> > We need to be moving towards a situation where people don't _need_ to
> > patch the hell out of their kernels to make Linux viable for embedded
> > devices. I'm not prepared just assume that 'they will always do so', and
> > I'm disappointed to hear Andrew say such a thing. 
> 
> It's simple economics, really.
> 
> Until they see value in upstream submission, it's always going
> to be %100 invested in getting the current product out the door
> or the current fire put out.

They are starting to see value in upstream submission. Even when I was
working for my previous employer, when we first started doing embedded
Linux contracts, it was sometimes a fight to be allowed the time to
merge stuff upstream.

But they had learned for themselves, over time, that it actually helps
them in the long run if they do that. Because when they come to do a
product update, or a new product similar to and old one, it means they
don't have to start again from scratch.

That seems to be something that everyone seems to need to learn for
themselves. We can tell them about it until we're blue in the face (and
I was in Japan and Korea doing just that to CELF folks earlier this
year), but they still don't really believe it until it's bitten _them_.

> The odd I2C device hack of the day the do to one revision of their
> product is likely never to be submitted upstream.  And these
> embedded folks have hundreds of things on that level that they
> sort out for each product, and often very little of it is reusable
> in the next product.

Even that kind of stuff is getting nicer. The device-tree magic we have
for PowerPC boards means that you often don't need any code changes at
all when you bring up a new board -- you just have a device-tree which
describes the hardware. And that covers i2c devices too.

> They don't see the payback for upstreaming this stuff, and for
> a lot of cases neither do I.

They're learning. And we need to too :)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Thu, 31 Jul 2008 11:39:36 +0100

> Dave, if you won't take these patches then I will (after review) put
> them in the embedded-2.6 tree with Andrew's sign-off, and ask Linus to
> pull them into 2.6.28 directly from there rather than via your tree.
> I think he'll take them, because it's the right thing to do.

If you're going to force the issue, that's quite nice of you.  And I'm
also glad that you have the one true and only correct view of what is
"the right thing to do" in the Linux networking.

I especially appreciate that you still haven't accepted the plain fact
that CONFIG_ETHTOOL needs to be selected by CONFIG_INET.

I definitely see the next consequence of the CONFIG_ETHTOOL stuff, and
that's a set of ifdef'ola in all the drivers to config out the
per-driver ethtool support code.

Re: [patch 12/12] Configure out ethtool support

[Matt Mackall]

On Thu, 2008-07-31 at 03:47 -0700, David Miller wrote:
> I especially appreciate that you still haven't accepted the plain fact
> that CONFIG_ETHTOOL needs to be selected by CONFIG_INET.

So far the following features have been mentioned as being critically
dependent on ethtool:

- bridging
- bonding
- LRO
- netfilter (really?)
- IPv6 (really?)

And yet every single one of these is currently a config option, so your
above statement is still looking awfully dubious. At this point I'd
suggest that you've painted yourself into a corner where all these
options must also actually be mandatory, but I'm afraid you might
secretly want to do that anyway.

> I definitely see the next consequence of the CONFIG_ETHTOOL stuff, and
> that's a set of ifdef'ola in all the drivers to config out the
> per-driver ethtool support code.

If someone shows up with such a patch for a Xylinx or ARM SOC device,
you probably should take it. If the patch is instead for a 10Gb PCIe
device, I suggest you don't. (At least not until that sort of thing
becomes standard embedded kit.)

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 12/12] Configure out ethtool support

[Stephen Hemminger]

On Thu, 31 Jul 2008 10:36:31 -0500
Matt Mackall <mpm@selenic.com> wrote:

> 
> On Thu, 2008-07-31 at 03:47 -0700, David Miller wrote:
> > I especially appreciate that you still haven't accepted the plain fact
> > that CONFIG_ETHTOOL needs to be selected by CONFIG_INET.
> 
> So far the following features have been mentioned as being critically
> dependent on ethtool:
> 
> - bridging
> - bonding
> - LRO
> - netfilter (really?)
> - IPv6 (really?)
> 
> And yet every single one of these is currently a config option, so your
> above statement is still looking awfully dubious. At this point I'd
> suggest that you've painted yourself into a corner where all these
> options must also actually be mandatory, but I'm afraid you might
> secretly want to do that anyway.
> 
> > I definitely see the next consequence of the CONFIG_ETHTOOL stuff, and
> > that's a set of ifdef'ola in all the drivers to config out the
> > per-driver ethtool support code.
> 
> If someone shows up with such a patch for a Xylinx or ARM SOC device,
> you probably should take it. If the patch is instead for a 10Gb PCIe
> device, I suggest you don't. (At least not until that sort of thing
> becomes standard embedded kit.)
> 

I have no problem with an embedded CONFIG_ETHTOOL that normal users
can ignore. Just don't expect the network developers to fix things
that are broken when CONFIG_ETHTOOL is not enabled. I.e if embedded
build is broken it is your problem.

Re: [patch 12/12] Configure out ethtool support

[Matt Mackall]

On Thu, 2008-07-31 at 08:59 -0700, Stephen Hemminger wrote:
> I have no problem with an embedded CONFIG_ETHTOOL that normal users
> can ignore. Just don't expect the network developers to fix things
> that are broken when CONFIG_ETHTOOL is not enabled. I.e if embedded
> build is broken it is your problem.

menuconfig EMBEDDED
        bool "Configure standard kernel features (for small systems)"
        help
          This option allows certain base kernel options and settings
          to be disabled or tweaked. This is for specialized
          environments which can tolerate a "non-standard" kernel.
          Only use this if you really know what you are doing.

If you think that disclaimer needs to be bigger, let me know.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 12/12] Configure out ethtool support

[Evgeniy Polyakov]

Hi Matt.

On Thu, Jul 31, 2008 at 10:36:31AM -0500, Matt Mackall (mpm@selenic.com) wrote:
> > I especially appreciate that you still haven't accepted the plain fact
> > that CONFIG_ETHTOOL needs to be selected by CONFIG_INET.
> 
> So far the following features have been mentioned as being critically
> dependent on ethtool:
> 
> - bridging
> - bonding
> - LRO
> - netfilter (really?)
> - IPv6 (really?)
> 
> And yet every single one of these is currently a config option, so your
> above statement is still looking awfully dubious. At this point I'd
> suggest that you've painted yourself into a corner where all these
> options must also actually be mandatory, but I'm afraid you might
> secretly want to do that anyway.

Things not are that simple. If your hardware happend to support
scatter-gather and tx checksumming, and you forgot to add a magic ifdef
into own embedded build of the driver, then sendfile() performance will
be awful. Even more: if you hardcoded negotiation bits into the driver
and happend to plug NIC to the wrong switch, you will have no-to-bad
performance. Even more: there are quite broken switches, which just
freeze the port after some tricky packets sent by nic (for example
several autonegotiation packets). Without ethtool you will not be able
to reset NIC. You will not be able to debug any hardware trouble and/or
set needed hardware bits. If embedded folks are so much care about size,
that they will hardcode all needed parameters for their own network into
the driver, then they can apply this patch on its own tree and do not
compile ethtools.

The only sane way to try to push something like this into the tree is
obviously turned on by default and with very much all needed config
options checked first. I agree with other poster, who recommened to put
a warning into the commented syscalls.


I can recommend another major part to be removed with the same approach
you use with ethtool: netlink. Webcam does not need to install route or
configure interface, it can do it via dhcp in kernel. Embedded folks can
also create a simple module, which will do the same with smaller
overhead. Then you can turn off direct-io and readahead. Webcam does not
need to have either. One can replace scheduler algorithms with something
small and trivial. Webcam should only have init and reading software.
Actually it can do it in init (I personally wrote embedded system, where
only init existed, which was a trivial shell with dozen of commands).
You can also trim block layer by half: embedded system should not be
able to configure device queues at all. Feel free to point to any
subsystem, and there are lots of such 'non-needed' from the fist view
things. But when some troubles start or feature needed, or behaviour is
not expected, uch embedded people start to scream, that everything is so
bad in Linux and it is not suitable and so on...

-- 
	Evgeniy Polyakov

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: akpm@linux-foundation.org
Date: Wed, 30 Jul 2008 12:39:04 -0700

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> Add the CONFIG_ETHTOOL option which allows to remove support for ethtool,
> not necessarly used on embedded systems.  As this is a size-reduction
> option, it depends on CONFIG_EMBEDDED.  It allows to save ~6 kilobytes of
> kernel code:
> 
>    text	   data	    bss	    dec	    hex	filename
> 1258447	 123592	 212992	1595031	 185697	vmlinux
> 1252147	 123592	 212992	1588731	 183dfb	vmlinux.new
>   -6300       0       0   -6300   -189C +/-
> 
> Question: should we also remove ethtool-related functions from all network
> drivers ?
> 
> This patch has been originally written by Matt Mackall
> <mpm@selenic.com>, and is part of the Linux Tiny project.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jeff Garzik <jeff@garzik.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

There is no way I'm applying this, ever.

This change means you can't configure nearly all of the features on
your ethernet card.  You can't even configure the link parameters
without ethtool support.

If you want 6K back get rid of the random number generator, I hear
embedded people don't need that either :-)

Re: [patch 12/12] Configure out ethtool support

[Andrew Morton]

On Wed, 30 Jul 2008 14:35:32 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: akpm@linux-foundation.org
> Date: Wed, 30 Jul 2008 12:39:04 -0700
> 
> > From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > 
> > Add the CONFIG_ETHTOOL option which allows to remove support for ethtool,
> > not necessarly used on embedded systems.  As this is a size-reduction
> > option, it depends on CONFIG_EMBEDDED.  It allows to save ~6 kilobytes of
> > kernel code:
> > 
> >    text	   data	    bss	    dec	    hex	filename
> > 1258447	 123592	 212992	1595031	 185697	vmlinux
> > 1252147	 123592	 212992	1588731	 183dfb	vmlinux.new
> >   -6300       0       0   -6300   -189C +/-
> > 
> > Question: should we also remove ethtool-related functions from all network
> > drivers ?
> > 
> > This patch has been originally written by Matt Mackall
> > <mpm@selenic.com>, and is part of the Linux Tiny project.
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Matt Mackall <mpm@selenic.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Jeff Garzik <jeff@garzik.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> There is no way I'm applying this, ever.
> 
> This change means you can't configure nearly all of the features on
> your ethernet card.  You can't even configure the link parameters
> without ethtool support.

This has been in linux-tiny for a long time - at least since 2.6.5.

So either a) nobody sets CONFIG_ETHTOOL=n or b) you're wrong.

I don't know which of these is the case and I don't know how to find
out.

I could be that the feature was useful back in the 2.6.5 days but
is now completely useless, dunno.

> If you want 6K back get rid of the random number generator, I hear
> embedded people don't need that either :-)

They'll take all they can get.

Re: [patch 12/12] Configure out ethtool support

[Matt Mackall]

On Wed, 2008-07-30 at 14:35 -0700, David Miller wrote:
> This change means you can't configure nearly all of the features on
> your ethernet card.  You can't even configure the link parameters
> without ethtool support.

Which is big fat don't-care for the vast majority of users. Various
popular distros don't even install the tool by default.

Embedded devices care even less. You'd be fairly hard-pressed to find,
say, a BluRay player that lets you set the link duplex and checksum
offloading somewhere deep deep down in the setup menu. And good luck
trying to do that -over the wire- to a headless device. Unless the
device is a serious router, you're -never- going to see anyone go to the
trouble of exporting such functionality.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Wed, 30 Jul 2008 17:24:26 -0500

> 
> On Wed, 2008-07-30 at 14:35 -0700, David Miller wrote:
> > This change means you can't configure nearly all of the features on
> > your ethernet card.  You can't even configure the link parameters
> > without ethtool support.
> 
> Which is big fat don't-care for the vast majority of users. Various
> popular distros don't even install the tool by default.

Are you even aware that the ethtool layer gets invoked by simply
enabling IP forwarding or bridgining, in order to disable LRO
offloading which conflicts with such uses?

And I disagree with your asserion that the tool doesn't get installed
by default by distros.  I can't find one single system here that
doesn't have /usr/sbin/ethtool available.

I guess it depends upon your definition of "various".

Re: [patch 12/12] Configure out ethtool support

[Matt Mackall]

On Wed, 2008-07-30 at 15:37 -0700, David Miller wrote:
> From: Matt Mackall <mpm@selenic.com>
> Date: Wed, 30 Jul 2008 17:24:26 -0500
> 
> > 
> > On Wed, 2008-07-30 at 14:35 -0700, David Miller wrote:
> > > This change means you can't configure nearly all of the features on
> > > your ethernet card.  You can't even configure the link parameters
> > > without ethtool support.
> > 
> > Which is big fat don't-care for the vast majority of users. Various
> > popular distros don't even install the tool by default.
> 
> Are you even aware that the ethtool layer gets invoked by simply
> enabling IP forwarding or bridgining, in order to disable LRO
> offloading which conflicts with such uses?

Oh noes. How ever will my poor webcam get on the internets without that.

Again, most users don't use forwarding or bridging, nor do they have
LRO. Again, doubly so for embedded devices. Your remark about
CONFIG_INET is pure hyperbole.

> And I disagree with your asserion that the tool doesn't get installed
> by default by distros.  I can't find one single system here that
> doesn't have /usr/sbin/ethtool available.
> 
> I guess it depends upon your definition of "various".

I've got a vanilla Debian install here for starters.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Wed, 30 Jul 2008 18:05:02 -0500

> On Wed, 2008-07-30 at 15:37 -0700, David Miller wrote:
> > From: Matt Mackall <mpm@selenic.com>
> > Date: Wed, 30 Jul 2008 17:24:26 -0500
> > 
> > > 
> > > On Wed, 2008-07-30 at 14:35 -0700, David Miller wrote:
> > > > This change means you can't configure nearly all of the features on
> > > > your ethernet card.  You can't even configure the link parameters
> > > > without ethtool support.
> > > 
> > > Which is big fat don't-care for the vast majority of users. Various
> > > popular distros don't even install the tool by default.
> > 
> > Are you even aware that the ethtool layer gets invoked by simply
> > enabling IP forwarding or bridgining, in order to disable LRO
> > offloading which conflicts with such uses?
> 
> Oh noes. How ever will my poor webcam get on the internets without that.

Have any firewall rules implemented on that webcam?

It's only a matter of time before other core facilities need
to use ethtool to adjust the device's settings in one way or
another.

So what I said is anything but hyperbole.

Re: [patch 12/12] Configure out ethtool support

[Matt Mackall]

On Wed, 2008-07-30 at 16:21 -0700, David Miller wrote:
> From: Matt Mackall <mpm@selenic.com>
> Date: Wed, 30 Jul 2008 18:05:02 -0500
> 
> > On Wed, 2008-07-30 at 15:37 -0700, David Miller wrote:
> > > From: Matt Mackall <mpm@selenic.com>
> > > Date: Wed, 30 Jul 2008 17:24:26 -0500
> > > 
> > > > 
> > > > On Wed, 2008-07-30 at 14:35 -0700, David Miller wrote:
> > > > > This change means you can't configure nearly all of the features on
> > > > > your ethernet card.  You can't even configure the link parameters
> > > > > without ethtool support.
> > > > 
> > > > Which is big fat don't-care for the vast majority of users. Various
> > > > popular distros don't even install the tool by default.
> > > 
> > > Are you even aware that the ethtool layer gets invoked by simply
> > > enabling IP forwarding or bridgining, in order to disable LRO
> > > offloading which conflicts with such uses?
> > 
> > Oh noes. How ever will my poor webcam get on the internets without that.
> 
> Have any firewall rules implemented on that webcam?

Of course not. It's a webcam. Or it's a printer or game console or media
streamer or alarm clock or BluRay player, etc. Yes, I do have a
Linux-powered alarm clock (A Nokia N800) and no, it doesn't have a
firewall.

> It's only a matter of time before other core facilities need
> to use ethtool to adjust the device's settings in one way or
> another.

To which I'll respond that your LRO scheme could have just as easily
been implemented the other way around. That is, the driver could have
been written to not go into LRO mode until the core said "hey, it's safe
to turn on LRO mode now if you happen to have it". Same for
checksumming, etc. Which, in addition to being friendly to
CONFIG_ETHTOOL=N, is generally a bit more conservative.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [patch 12/12] Configure out ethtool support

[David Miller]

From: Matt Mackall <mpm@selenic.com>
Date: Wed, 30 Jul 2008 20:10:09 -0500

> Of course not. It's a webcam. Or it's a printer or game console or media
> streamer or alarm clock or BluRay player, etc. Yes, I do have a
> Linux-powered alarm clock (A Nokia N800) and no, it doesn't have a
> firewall.

Most game consoles and cell phones make use of either firewalling
or IPSEC, or both.

Looking at various kernel boot message logs of N800's posted online,
it seems to have netfilter enabled.  The ip_tables module does get
loaded.

Re: [patch 12/12] Configure out ethtool support

[Kalle Valo]

David Miller <davem@davemloft.net> writes:

> Looking at various kernel boot message logs of N800's posted online,
> it seems to have netfilter enabled.  The ip_tables module does get
> loaded.

That's true, netfilter is enabled on N800/N810. We use it to send idle
traffic timeout events to user space.

-- 
Kalle Valo