[RFC PATCH] Ethtool style in kernel network driver configuration.

[RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

Allow network drivers to be configured by the kernel
in the same way as the userspace "ethtool" program as suggested
by Nicolas Pitre in a recent mailing list discussion.

Two methods are possible, selected by KConfig:

1) Kernel parameter (net_ethconfig.ethtool)
which accepts (most of) the same arguments as "ethtool -s"

	net_ethconfig.ethtool="eth0 speed 10 duplex full"

The wol, sopass and msglvl parameters are not (yet?) supported.

2) Programatic configuration via a new function
neteth_configure_interface() (typically from board specific setup code):

#include <linux/net-ethconfig.h>
static struct neteth_if_config force_10mbps = {
	.etool_cmd = {
		.speed = SPEED_10,
		.duplex = DUPLEX_FULL,
	},
	.set_flags = NETCONF_SET_SPEED | NETCONF_SET_DUPLEX,
};
...
neteth_configure_interface("eth0", &force_10mbps);

The programatic method may be required in certain embedded situations
(for example when different hardware revisions require different
configurations and the hardware revision can be detected by software).

Signed-off-by: Martin Fuzzey <mfuzzey@gmail.com>

---

I'm not really happy with the placement of this as it's not core,
but it's not a driver either so any better ideas?

Also not sure if I need to hold a lock while manipulating the device
from the notifier callback.

 Documentation/kernel-parameters.txt |    4 
 include/linux/net-ethconfig.h       |   30 ++
 net/Kconfig                         |   36 ++
 net/core/Makefile                   |    2 
 net/core/net-ethconfig.c            |  545 +++++++++++++++++++++++++++++++++++
 5 files changed, 616 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/net-ethconfig.h
 create mode 100644 net/core/net-ethconfig.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 600cdd7..6151f36 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1422,6 +1422,10 @@ and is between 256 and 4096 characters. It is defined in the file
 			This usage is only documented in each driver source
 			file if at all.
 
+	net_ethconfig.ethtool=	[NET] ethtool style network configuration
+			Accepts most of options of ethtool -s
+			Example net_ethconfig.ethtool="eth0 speed 10 duplex full"
+
 	nf_conntrack.acct=
 			[NETFILTER] Enable connection tracking flow accounting
 			0 to disable accounting
diff --git a/include/linux/net-ethconfig.h b/include/linux/net-ethconfig.h
new file mode 100644
index 0000000..1b50361
--- /dev/null
+++ b/include/linux/net-ethconfig.h
@@ -0,0 +1,30 @@
+/*
+ * net-config.h: Defines for programatic interface for board setup code
+ *
+ * Copyright (C) 2009 Martin Fuzzey <mfuzzey@gmail.com>
+ */
+
+#ifndef _LINUX_NET_CONFIG_H
+#define _LINUX_NET_CONFIG_H
+
+#include <linux/ethtool.h>
+
+#define NETCONF_SET_ADVERTISING		(1 << 0)
+#define NETCONF_SET_SPEED		(1 << 1)
+#define NETCONF_SET_DUPLEX		(1 << 2)
+#define NETCONF_SET_PORT		(1 << 3)
+#define NETCONF_SET_PHY_ADDR		(1 << 4)
+#define NETCONF_SET_TRANCEIVER		(1 << 5)
+#define NETCONF_SET_AUTONEG		(1 << 6)
+
+struct neteth_if_config {
+	struct ethtool_cmd etool_cmd;
+	unsigned int set_flags;
+};
+
+int neteth_configure_interface(
+		const char *name,
+		struct neteth_if_config *if_config);
+
+#endif
+
diff --git a/net/Kconfig b/net/Kconfig
index ce77db4..e1e8744 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -188,6 +188,42 @@ source "net/phonet/Kconfig"
 source "net/sched/Kconfig"
 source "net/dcb/Kconfig"
 
+menuconfig NET_ETHCONFIG
+	bool "Kernel configuration of network device parameters"
+	---help---
+	  This option allows the "ethtool" interface to be used from within
+	  the kernel or via the kernel command line. It provides support
+	  for setting such things as links speed and duplex.
+
+	  This facility is intended for embedded systems which may need to
+	  force specific settings at boot time but where the complexity of
+	  using ethtool from userspace is undesirable.
+
+if NET_ETHCONFIG
+config NET_ETHCONFIG_CMDLINE
+	bool "Enable network device configuration via kernel command line"
+	depends on NET_ETHCONFIG
+	help
+	  Saying Y here will enable the kernel parameter net_ethconfig.ethtool
+	  This parameter takes a comma seperated list of network configurations
+	  in the same format as accepted by the -s option to userspace ethtool
+	  program.
+
+config NET_ETHCONFIG_PROG
+	bool "Enable programatic network device configuration"
+	depends on NET_ETHCONFIG
+	help
+	  You can say Y here to allow board specific code to programaticaly
+	  configure the network interfaces.
+
+config NET_ETHCONFIG_DEBUG
+	bool "Enable configuration debugging"
+	depends on NET_ETHCONFIG
+	help
+	  You can say Y here to generate detailed messages regarding the network
+	  device configuration.
+endif
+
 menu "Network testing"
 
 config NET_PKTGEN
diff --git a/net/core/Makefile b/net/core/Makefile
index 796f46e..fc2b3d0 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -19,4 +19,4 @@ obj-$(CONFIG_NET_DMA) += user_dma.o
 obj-$(CONFIG_FIB_RULES) += fib_rules.o
 obj-$(CONFIG_TRACEPOINTS) += net-traces.o
 obj-$(CONFIG_NET_DROP_MONITOR) += drop_monitor.o
-
+obj-$(CONFIG_NET_ETHCONFIG) += net-ethconfig.o
diff --git a/net/core/net-ethconfig.c b/net/core/net-ethconfig.c
new file mode 100644
index 0000000..f059fc2
--- /dev/null
+++ b/net/core/net-ethconfig.c
@@ -0,0 +1,545 @@
+/****************************************************************
+ * In kernel ethtool configuration for network devices.
+ *
+ * Copyright (c) 2009 by Martin Fuzzey <mfuzzey@gmail.com>
+
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ ****************************************************************/
+
+/*
+This module allows network drivers to be configured by the kernel
+in the same way as the userspace "ethtool" program.
+
+Two methods are possible, selected by configuration:
+
+1) Kernel parameter (net_ethconfig.ethtool)
+which accepts (most of) the same arguments as "ethtool -s"
+Eg: net_ethconfig.ethtool="eth0 speed 10 duplex full"
+
+The wol, sopass and msglvl parameters are not (yet?) supported.
+
+2) Programatic configuration via neteth_configure_interface() (typically
+from board specific setup code):
+
+#include <linux/net-ethconfig.h>
+static struct neteth_if_config force_10mbps = {
+	.etool_cmd = {
+		.speed = SPEED_10,
+		.duplex = DUPLEX_FULL,
+	},
+	.set_flags = NETCONF_SET_SPEED | NETCONF_SET_DUPLEX,
+};
+...
+neteth_configure_interface("eth0", &force_10mbps);
+*/
+
+
+#ifdef CONFIG_NET_ETHCONFIG_DEBUG
+#define DEBUG 1
+#endif
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/ethtool.h>
+#include <linux/netdevice.h>
+#include <linux/spinlock.h>
+#include <linux/net-ethconfig.h>
+
+static char module_name[] = "net-ethconfig";
+
+MODULE_AUTHOR("Martin Fuzzey <mfuzzey@gmail.com>");
+MODULE_DESCRIPTION("Ethtool based network configuration");
+MODULE_LICENSE("GPL");
+
+#define ADVERTISED_SPEEDS (ADVERTISED_10baseT_Half |\
+				ADVERTISED_10baseT_Full |\
+				ADVERTISED_100baseT_Half |\
+				ADVERTISED_100baseT_Full |\
+				ADVERTISED_1000baseT_Half |\
+				ADVERTISED_1000baseT_Full|\
+				ADVERTISED_2500baseX_Full |\
+				ADVERTISED_10000baseT_Full)
+
+struct if_config {
+	char name[IFNAMSIZ];
+	unsigned int set_flags;
+	struct ethtool_cmd cmd;
+};
+
+static unsigned int config_count;
+static struct if_config if_config[2];
+static DEFINE_SPINLOCK(if_config_lock);
+
+#ifdef CONFIG_NET_ETHCONFIG_CMDLINE
+
+struct param_def {
+	const char *name;
+	int (*parse)(const char *, struct if_config *);
+};
+
+struct choice {
+	const char *name;
+	int value;
+};
+
+
+static struct choice *find_choice(
+		const char *val, struct choice *choices, int count)
+{
+	int i;
+	struct choice *choice;
+
+	for (i = 0, choice = choices; i < count; i++, choice++) {
+		if (!strcmp(val, choice->name))
+			return choice;
+	}
+	return NULL;
+}
+
+static struct choice speed_choices[] = {
+	{"10", SPEED_10},
+	{"100", SPEED_100},
+	{"1000", SPEED_1000},
+	{"2500", SPEED_2500},
+	{"10000", SPEED_10000}
+};
+
+static int parse_speed(const char *val, struct if_config *config)
+{
+	struct choice *choice = find_choice(val,
+		speed_choices, ARRAY_SIZE(speed_choices));
+
+	if (choice == NULL)
+		return -EINVAL;
+
+	ethtool_cmd_speed_set(&config->cmd, (u32)choice->value);
+	config->set_flags |= NETCONF_SET_SPEED;
+	return 0;
+}
+
+static struct choice duplex_choices[] = {
+	{"half", DUPLEX_HALF},
+	{"full", DUPLEX_FULL}
+};
+
+static int parse_duplex(const char *val, struct if_config *config)
+{
+	struct choice *choice = find_choice(val,
+		duplex_choices, ARRAY_SIZE(duplex_choices));
+
+	if (choice == NULL)
+		return -EINVAL;
+
+	config->cmd.duplex = (u8)choice->value;
+	config->set_flags |= NETCONF_SET_DUPLEX;
+	return 0;
+}
+
+static struct choice port_choices[] = {
+	{"tp", PORT_TP},
+	{"aui", PORT_AUI},
+	{"bnc", PORT_BNC},
+	{"mii", PORT_MII},
+	{"fibre", PORT_FIBRE},
+};
+
+static int parse_port(const char *val, struct if_config *config)
+{
+	struct choice *choice = find_choice(val,
+		port_choices, ARRAY_SIZE(port_choices));
+
+	if (choice == NULL)
+		return -EINVAL;
+
+	config->cmd.port = (u8)choice->value;
+	config->set_flags |= NETCONF_SET_PORT;
+	return 0;
+}
+
+static struct choice autoneg_choices[] = {
+	{"on", AUTONEG_ENABLE},
+	{"off", AUTONEG_DISABLE}
+};
+
+static int parse_autoneg(const char *val, struct if_config *config)
+{
+	struct choice *choice = find_choice(val,
+		autoneg_choices, ARRAY_SIZE(autoneg_choices));
+
+	if (choice == NULL)
+		return -EINVAL;
+
+	config->cmd.autoneg = (u8)choice->value;
+	config->set_flags |= NETCONF_SET_AUTONEG;
+	return 0;
+}
+
+static int parse_advertise(const char *val, struct if_config *config)
+{
+	unsigned long advertising;
+
+	if (strict_strtoul(val, 16, &advertising))
+		return -EINVAL;
+
+	config->cmd.advertising = advertising;
+	config->set_flags |= NETCONF_SET_ADVERTISING;
+	return 0;
+}
+
+static int parse_phyad(const char *val, struct if_config *config)
+{
+	unsigned long phyad;
+
+	if (strict_strtoul(val, 0, &phyad))
+		return -EINVAL;
+
+	config->cmd.phy_address = (u8)phyad;
+	config->set_flags |= NETCONF_SET_PHY_ADDR;
+	return 0;
+}
+
+static struct choice xcvr_choices[] = {
+	{"internal", XCVR_INTERNAL},
+	{"external", XCVR_EXTERNAL}
+};
+
+static int parse_xcvr(const char *val, struct if_config *config)
+{
+	struct choice *choice = find_choice(val,
+		xcvr_choices, ARRAY_SIZE(xcvr_choices));
+
+	if (choice == NULL)
+		return -EINVAL;
+
+	config->cmd.transceiver = (u8)choice->value;
+	config->set_flags |= NETCONF_SET_TRANCEIVER;
+	return 0;
+}
+
+
+static struct param_def param_defs[] = {
+	{ .name = "speed", .parse = parse_speed },
+	{ .name = "duplex", .parse = parse_duplex },
+	{ .name = "port", .parse = parse_port },
+	{ .name = "autoneg", .parse = parse_autoneg },
+	{ .name = "advertise", .parse = parse_advertise },
+	{ .name = "phyad", .parse = parse_phyad },
+	{ .name = "xcvr", .parse = parse_xcvr }
+};
+
+enum parser_state {
+	INTERFACE,
+	KEY,
+	VALUE
+};
+
+static int param_set_ethtoolcmd(const char *val, struct kernel_param *kp)
+{
+	enum parser_state state = INTERFACE;
+	struct if_config *config = kp->arg;
+	char buf[64];
+	char *cur = buf, *word;
+	struct param_def *param = NULL;
+	int i, rc = 0;
+
+	strlcpy(buf, val, sizeof(buf)); /* don't mangle original */
+
+	do {
+		word = strsep(&cur, " ");
+		if (!word || *word == 0)
+			continue;
+
+		switch (state) {
+		case INTERFACE:
+			pr_debug("%s: cmdline config found for %s\n",
+				module_name, word);
+			strlcpy(config->name, word, sizeof(config->name));
+			state = KEY;
+			break;
+
+		case KEY:
+			param = param_defs;
+			for (i = 0; i < ARRAY_SIZE(param_defs); i++, param++)
+				if (!strcmp(word, param->name)) {
+					state = VALUE;
+					break;
+				}
+			if (state == KEY) {
+				pr_err("%s: Invalid key %s\n",
+					module_name, word);
+				return -EINVAL;
+			}
+			break;
+
+		case VALUE:
+			rc = param->parse(word, config);
+			if (rc < 0) {
+				pr_err("%s: Invalid value %s for %s\n",
+					module_name, word, param->name);
+				return rc;
+			}
+			state = KEY;
+			break;
+		}
+	} while (word);
+
+	if (state != KEY) {
+		pr_err("%s: %s too short\n", module_name, val);
+		rc = -EINVAL;
+	}
+	return rc;
+}
+
+static int param_get_ethtoolcmd(char *buffer, struct kernel_param *kp)
+{
+	return 0; /* no sysfs export so this should never be called */
+}
+
+module_param_array_named(ethtool, if_config, ethtoolcmd, &config_count, 0);
+
+#endif  /* CONFIG_NET_ETHCONFIG_CMDLINE */
+
+
+static struct if_config *find_config(const char *name)
+{
+	int i;
+	struct if_config *config = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&if_config_lock, flags);
+	for (i = 0; i < config_count; i++) {
+		if (!strcmp(name,  if_config[i].name)) {
+			config = &if_config[i];
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&if_config_lock, flags);
+	return config;
+}
+
+static void copy_set_fields(
+		struct ethtool_cmd *src,
+		struct ethtool_cmd *dest,
+		unsigned int set_flags)
+{
+	if (set_flags & NETCONF_SET_ADVERTISING)
+		dest->advertising = src->advertising;
+
+	if (set_flags & NETCONF_SET_SPEED) {
+		dest->speed = src->speed;
+		dest->speed_hi = src->speed_hi;
+	}
+
+	if (set_flags & NETCONF_SET_DUPLEX)
+		dest->duplex = src->duplex;
+
+	if (set_flags & NETCONF_SET_PORT)
+		dest->port = src->port;
+
+	if (set_flags & NETCONF_SET_PHY_ADDR)
+		dest->phy_address = src->phy_address;
+
+	if (set_flags & NETCONF_SET_TRANCEIVER)
+		dest->transceiver = src->transceiver;
+
+	if (set_flags & NETCONF_SET_AUTONEG)
+		dest->autoneg = src->autoneg;
+}
+
+static void dump_config(const char *header, struct ethtool_cmd *config)
+{
+	pr_debug("%s: %s "
+		"speed=%d duplex=%d "
+		"advertise=0x%08X supported=0x%08X "
+		"autoneg=%d\n",
+		module_name,
+		header,
+		config->speed, config->duplex,
+		config->advertising, config->supported,
+		config->autoneg);
+}
+
+
+#ifdef CONFIG_NET_ETHCONFIG_PROG
+
+int neteth_configure_interface(const char *name,
+		struct neteth_if_config *prog_config)
+{
+	struct if_config *config = NULL;
+	unsigned long flags;
+	int rc = 0;
+
+	pr_debug("%s: programatic configuration for %s\n",
+		module_name, name);
+
+	config = find_config(name);
+	spin_lock_irqsave(&if_config_lock, flags);
+
+	if (!config) {
+		if (config_count >= ARRAY_SIZE(if_config)) {
+			pr_err("%s: no free slots\n", module_name);
+			rc = -ENOSPC;
+			goto out;
+		}
+		config = &if_config[config_count++];
+		config->set_flags = 0;
+		strlcpy(config->name, name, sizeof(config->name));
+	}
+
+	config->set_flags |= prog_config->set_flags;
+	copy_set_fields(&prog_config->etool_cmd, &config->cmd,
+		prog_config->set_flags);
+
+out:
+	spin_unlock_irqrestore(&if_config_lock, flags);
+	return rc;
+}
+EXPORT_SYMBOL(neteth_configure_interface);
+
+#endif /* CONFIG_NET_ETHCONFIG_PROG */
+
+
+static u32 calc_advertising(u32 speed, u8 duplex)
+{
+	u32 advertising = 0;
+
+	switch (speed) {
+	case SPEED_10:
+		advertising = (duplex == DUPLEX_FULL) ?
+			ADVERTISED_10baseT_Full : ADVERTISED_10baseT_Half;
+		break;
+
+	case SPEED_100:
+		advertising = (duplex == DUPLEX_FULL) ?
+			ADVERTISED_100baseT_Full : ADVERTISED_100baseT_Half;
+		break;
+
+	case SPEED_1000:
+		advertising = (duplex == DUPLEX_FULL) ?
+			ADVERTISED_1000baseT_Full : ADVERTISED_1000baseT_Half;
+		break;
+
+	case SPEED_2500:
+		advertising = ADVERTISED_2500baseX_Full;
+		break;
+
+	case SPEED_10000:
+		advertising = ADVERTISED_10000baseT_Full;
+		break;
+	}
+	return advertising;
+}
+
+
+static int netconfig_netdev_event(struct notifier_block *this,
+		unsigned long event,
+		void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct ethtool_cmd etool_cmd;
+	struct if_config *config;
+	int rc;
+
+	if (event != NETDEV_UP)
+		goto done;
+
+	pr_debug("%s: dev=%s\n", module_name, dev->name);
+
+	config = find_config(dev->name);
+	if (!config)
+		goto done;
+
+	if (!dev->ethtool_ops ||
+			!dev->ethtool_ops->get_settings ||
+			!dev->ethtool_ops->set_settings) {
+		pr_warning(
+			"%s: %s has no ethtool support - not configuring\n",
+			module_name, dev->name);
+		goto done;
+	}
+
+	rc = dev->ethtool_ops->get_settings(dev, &etool_cmd);
+	if (rc) {
+		pr_err("%s: unable to obtain current configuration for %s "
+			"(%d)\n", module_name, dev->name, rc);
+		goto done;
+	}
+
+	dump_config("current", &etool_cmd);
+
+	if (config->set_flags & NETCONF_SET_ADVERTISING) {
+		if (config->cmd.advertising)
+			etool_cmd.advertising = config->cmd.advertising;
+		else
+			etool_cmd.advertising =
+				etool_cmd.supported & ADVERTISED_SPEEDS;
+	} else {
+		if ((config->set_flags & NETCONF_SET_SPEED) &&
+				(config->set_flags & NETCONF_SET_DUPLEX)) {
+			u32 advertising = calc_advertising(
+				ethtool_cmd_speed(&config->cmd),
+				config->cmd.duplex);
+
+			if (advertising) {
+				advertising |= (etool_cmd.advertising &
+					~ADVERTISED_SPEEDS);
+				etool_cmd.advertising = advertising;
+			}
+		}
+	}
+
+	copy_set_fields(&etool_cmd, &config->cmd,
+		config->set_flags & ~NETCONF_SET_ADVERTISING);
+
+	dump_config("new", &etool_cmd);
+
+	if (dev->ethtool_ops->begin) {
+		rc = dev->ethtool_ops->begin(dev);
+		if (rc) {
+			pr_err("%s: ethtool begin failed for %s (%d)\n",
+				module_name, dev->name, rc);
+			goto done;
+		}
+	}
+
+	rc = dev->ethtool_ops->set_settings(dev, &etool_cmd);
+	if (rc)
+		pr_err("%s: ethtool set failed for %s (%d)\n",
+			module_name, dev->name, rc);
+	else
+		pr_info("%s: configured %s\n", module_name, dev->name);
+
+	if (dev->ethtool_ops->complete)
+		dev->ethtool_ops->complete(dev);
+
+done:
+	return NOTIFY_DONE;
+}
+
+
+static struct notifier_block netconfig_netdev_notifier = {
+	.notifier_call  = netconfig_netdev_event,
+};
+
+static int __init netconfig_init(void)
+{
+	int err;
+
+	err = register_netdevice_notifier(&netconfig_netdev_notifier);
+	if (err)
+		goto fail;
+
+	pr_info("%s: %d configs\n", module_name, config_count);
+	return 0;
+
+fail:
+	return err;
+
+}
+
+module_init(netconfig_init);

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Joe Perches]

On Wed, 2009-06-10 at 19:34 +0200, Martin Fuzzey wrote:
> +static struct choice port_choices[] = {
> +	{"tp", PORT_TP},
> +	{"aui", PORT_AUI},
> +	{"bnc", PORT_BNC},
> +	{"mii", PORT_MII},
> +	{"fibre", PORT_FIBRE},
> +};

perhaps add

	{"fiber", PORT_FIBRE},

for us poor spelling folk across the pond...

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Wed, 2009-06-10 at 19:34 +0200, Martin Fuzzey wrote:
> Allow network drivers to be configured by the kernel
> in the same way as the userspace "ethtool" program as suggested
> by Nicolas Pitre in a recent mailing list discussion.
> 
> Two methods are possible, selected by KConfig:
> 
> 1) Kernel parameter (net_ethconfig.ethtool)
> which accepts (most of) the same arguments as "ethtool -s"
> 
> 	net_ethconfig.ethtool="eth0 speed 10 duplex full"
> 
> The wol, sopass and msglvl parameters are not (yet?) supported.

Who needs this feature?  Why not use ethtool in an initramfs?

> 2) Programatic configuration via a new function
> neteth_configure_interface() (typically from board specific setup code):
> 
> #include <linux/net-ethconfig.h>
> static struct neteth_if_config force_10mbps = {
> 	.etool_cmd = {
> 		.speed = SPEED_10,
> 		.duplex = DUPLEX_FULL,
> 	},
> 	.set_flags = NETCONF_SET_SPEED | NETCONF_SET_DUPLEX,
> };
> ...
> neteth_configure_interface("eth0", &force_10mbps);
> 
> The programatic method may be required in certain embedded situations
> (for example when different hardware revisions require different
> configurations and the hardware revision can be detected by software).
[...]

Forcing speed and duplex is occasionally needed to work around a link
partner that doesn't implement autonegotiation correctly.  I don't see
that it should ever be needed in platform configuration.  If the driver
doesn't detect the MAC/PHY capabilities correctly then the driver should
be fixed.  Overriding the settings once will not prevent an unsupported
mode being selected later.

I can see that it may be useful to set the PHY type and address from
platform code, but I don't know how many drivers for current hardware
can cope with having those changed after initialisation.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

On Thu, 11 Jun 2009, Ben Hutchings wrote:

> On Wed, 2009-06-10 at 19:34 +0200, Martin Fuzzey wrote:
> > Allow network drivers to be configured by the kernel
> > in the same way as the userspace "ethtool" program as suggested
> > by Nicolas Pitre in a recent mailing list discussion.
[...]
> Who needs this feature?  Why not use ethtool in an initramfs?
[...]
> Forcing speed and duplex is occasionally needed to work around a link
> partner that doesn't implement autonegotiation correctly.  I don't see
> that it should ever be needed in platform configuration.  If the driver
> doesn't detect the MAC/PHY capabilities correctly then the driver should
> be fixed.  Overriding the settings once will not prevent an unsupported
> mode being selected later.

I'd suggest you have a look at the conversation that occurred in the 
thread which subject was "SMC91x: forcing speed", no later than 3 days 
ago.


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

Ben Hutchings wrote:
> Who needs this feature?  Why not use ethtool in an initramfs?
>
>   
> Forcing speed and duplex is occasionally needed to work around a link
> partner that doesn't implement autonegotiation correctly.  I don't see
> that it should ever be needed in platform configuration.  If the driver
> doesn't detect the MAC/PHY capabilities correctly then the driver should
> be fixed.  Overriding the settings once will not prevent an unsupported
> mode being selected later.
>
>   
To summarize the recent points I made in the smc91x: forcing speed thread :

1) Setting up and maintaining an initramfs can increase the complexity
for embedded systems - it's another image file to build, distribute,
update to bootloader etc.

2) While I of course agree that broken drivers should be fixed, what
about broken hardware?
I currently have this situation on one of my boards - 100Mbps doesn't
work due to electrical issues (bad routing).
This board is already in the wild - if it is fixed one day it will be a
new hardware revision and the code will have to cope with both.
Sure the "right" way is to fix the hardware but that's not always
economically or logistically possible.
I suspect such situations are not uncommon in the embedded world.


Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> Ben Hutchings wrote:
> > Who needs this feature?  Why not use ethtool in an initramfs?
> >
> >   
> > Forcing speed and duplex is occasionally needed to work around a link
> > partner that doesn't implement autonegotiation correctly.  I don't see
> > that it should ever be needed in platform configuration.  If the driver
> > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > be fixed.  Overriding the settings once will not prevent an unsupported
> > mode being selected later.
> >
> >   
> To summarize the recent points I made in the smc91x: forcing speed thread :
> 
> 1) Setting up and maintaining an initramfs can increase the complexity
> for embedded systems - it's another image file to build, distribute,
> update to bootloader etc.

This doesn't seem like a huge burden if you're net-booting.  And if
you're not net-booting, it's not critical that you override the link
mode immediately; you can do it in the regular init scripts.

> 2) While I of course agree that broken drivers should be fixed, what
> about broken hardware?

Broken hardware?  Why, I never have to deal with that. ;-)

> I currently have this situation on one of my boards - 100Mbps doesn't
> work due to electrical issues (bad routing).
> This board is already in the wild - if it is fixed one day it will be a
> new hardware revision and the code will have to cope with both.
> Sure the "right" way is to fix the hardware but that's not always
> economically or logistically possible.
> I suspect such situations are not uncommon in the embedded world.

So, as I thought, you actually want to disable some modes completely.
That is not what ethtool does.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

On Thu, 11 Jun 2009, Ben Hutchings wrote:

> On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> > Ben Hutchings wrote:
> > > Who needs this feature?  Why not use ethtool in an initramfs?
> > >
> > >   
> > > Forcing speed and duplex is occasionally needed to work around a link
> > > partner that doesn't implement autonegotiation correctly.  I don't see
> > > that it should ever be needed in platform configuration.  If the driver
> > > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > > be fixed.  Overriding the settings once will not prevent an unsupported
> > > mode being selected later.
> > >
> > >   
> > To summarize the recent points I made in the smc91x: forcing speed thread :
> > 
> > 1) Setting up and maintaining an initramfs can increase the complexity
> > for embedded systems - it's another image file to build, distribute,
> > update to bootloader etc.
> 
> This doesn't seem like a huge burden if you're net-booting.  And if
> you're not net-booting, it's not critical that you override the link
> mode immediately; you can do it in the regular init scripts.

Sure... But for some embedded setup this is actually more trouble and 
hassle than having a non-intrusive kernel based facility that can set 
defaults for you.

The same way that the kernel currently has BOOTP and DHCP in the kernel.  
This is not necessarily the preferred way on a server or workstation, 
but damn useful on some embedded targets.  If DHCP and NFS root were 
removed from the kernel then of course using ethtool from some initramfs 
(along with dhclient and 'mount -t nfs') or the like would actually be 
the "right" way.  But CONFIG_IP_PNP_DHCP and CONFIG_ROOT_NFS are still 
in the kernel and unlikely to be removed soon.

> > I currently have this situation on one of my boards - 100Mbps doesn't
> > work due to electrical issues (bad routing).
> > This board is already in the wild - if it is fixed one day it will be a
> > new hardware revision and the code will have to cope with both.
> > Sure the "right" way is to fix the hardware but that's not always
> > economically or logistically possible.
> > I suspect such situations are not uncommon in the embedded world.
> 
> So, as I thought, you actually want to disable some modes completely.
> That is not what ethtool does.

No.  What is needed is to enforce a different _default_.  If someone 
manages to run ethtool after the system is booted and select the broken 
mode then that's just too bad.

Now the mainline people are (rightly) complaining that the 
embedded people don't participate enough and communicate their needs 
with the upstream kernel.  I think this is a good example from the 
embedded world providing a clean solution to a recurring issue which 
would return again to quick hacks if the mainline reception is 
"use a cramfs" again.


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Thu, 2009-06-11 at 12:22 -0400, Nicolas Pitre wrote:
> On Thu, 11 Jun 2009, Ben Hutchings wrote:
> 
> > On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> > > Ben Hutchings wrote:
> > > > Who needs this feature?  Why not use ethtool in an initramfs?
> > > >
> > > >   
> > > > Forcing speed and duplex is occasionally needed to work around a link
> > > > partner that doesn't implement autonegotiation correctly.  I don't see
> > > > that it should ever be needed in platform configuration.  If the driver
> > > > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > > > be fixed.  Overriding the settings once will not prevent an unsupported
> > > > mode being selected later.
> > > >
> > > >   
> > > To summarize the recent points I made in the smc91x: forcing speed thread :
> > > 
> > > 1) Setting up and maintaining an initramfs can increase the complexity
> > > for embedded systems - it's another image file to build, distribute,
> > > update to bootloader etc.
> > 
> > This doesn't seem like a huge burden if you're net-booting.  And if
> > you're not net-booting, it's not critical that you override the link
> > mode immediately; you can do it in the regular init scripts.
> 
> Sure... But for some embedded setup this is actually more trouble and 
> hassle than having a non-intrusive kernel based facility that can set 
> defaults for you.

Doing it in an init script is even less intrusive!  But it seems that
the ethtool API doesn't do what you need in this case, anyway.

[...]
> > > I currently have this situation on one of my boards - 100Mbps doesn't
> > > work due to electrical issues (bad routing).
> > > This board is already in the wild - if it is fixed one day it will be a
> > > new hardware revision and the code will have to cope with both.
> > > Sure the "right" way is to fix the hardware but that's not always
> > > economically or logistically possible.
> > > I suspect such situations are not uncommon in the embedded world.
> > 
> > So, as I thought, you actually want to disable some modes completely.
> > That is not what ethtool does.
> 
> No.  What is needed is to enforce a different _default_.  If someone 
> manages to run ethtool after the system is booted and select the broken 
> mode then that's just too bad.
>
> Now the mainline people are (rightly) complaining that the 
> embedded people don't participate enough and communicate their needs 
> with the upstream kernel  I think this is a good example from the 
> embedded world providing a clean solution to a recurring issue which 
> would return again to quick hacks if the mainline reception is 
> "use a cramfs" again.

It's an example of providing a generic solution, which is definitely
more than a quick hack, but I don't see it as a "clean solution" for the
problem that certain link modes don't work on a particular board.  A
clean solution would disable those modes altogether in the driver.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

On Thu, 11 Jun 2009, Ben Hutchings wrote:

> On Thu, 2009-06-11 at 12:22 -0400, Nicolas Pitre wrote:
> > On Thu, 11 Jun 2009, Ben Hutchings wrote:
> > 
> > > On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> > > > Ben Hutchings wrote:
> > > > > Who needs this feature?  Why not use ethtool in an initramfs?
> > > > >
> > > > >   
> > > > > Forcing speed and duplex is occasionally needed to work around a link
> > > > > partner that doesn't implement autonegotiation correctly.  I don't see
> > > > > that it should ever be needed in platform configuration.  If the driver
> > > > > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > > > > be fixed.  Overriding the settings once will not prevent an unsupported
> > > > > mode being selected later.
> > > > >
> > > > >   
> > > > To summarize the recent points I made in the smc91x: forcing speed thread :
> > > > 
> > > > 1) Setting up and maintaining an initramfs can increase the complexity
> > > > for embedded systems - it's another image file to build, distribute,
> > > > update to bootloader etc.
> > > 
> > > This doesn't seem like a huge burden if you're net-booting.  And if
> > > you're not net-booting, it's not critical that you override the link
> > > mode immediately; you can do it in the regular init scripts.
> > 
> > Sure... But for some embedded setup this is actually more trouble and 
> > hassle than having a non-intrusive kernel based facility that can set 
> > defaults for you.
> 
> Doing it in an init script is even less intrusive!  But it seems that
> the ethtool API doesn't do what you need in this case, anyway.

Stop thinking in terms of workstation setups please.

[...]
> It's an example of providing a generic solution, which is definitely
> more than a quick hack, but I don't see it as a "clean solution" for the
> problem that certain link modes don't work on a particular board.  A
> clean solution would disable those modes altogether in the driver.

And now you are considering the addition of special restrictions for 
some special hardware directly into the driver as less intrusive than 
what is being proposed?  And now imagine what this would look like after 
56 different embedded designs have added their own kirks into the 
driver.. because, yes, in the embedded world you do have much more than 
56 different designs sharing the same driver but each with their own 
flaws.

What people are hacking now is some #if 0 ... #endif in their own tree 
to disable the initialization of driver features that their hardware is 
not able to cope with.  There are two cleaner alternatives: changing the 
default with ethtool from user space, or using a kernel based 
ethtool-like facility to do the same as conveniently as 
CONFIG_IP_PNP_DHCP is doing already for IP autoconfig.  The later is 
what this proposal is about.


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

On Thu, Jun 11, 2009 at 6:52 PM, Ben Hutchings<bhutchings@solarflare.com> wrote:
> It's an example of providing a generic solution, which is definitely
> more than a quick hack, but I don't see it as a "clean solution" for the
> problem that certain link modes don't work on a particular board.  A
> clean solution would disable those modes altogether in the driver.
>
I don't see why this is cleaner.
It may be slightly safer as someone can't later run ethtool and mess things up.

And how do you propose disabling it in the driver?
Either we're back to quick driver specific hacks  (which is what I
initially did before writing this patch) or we need a new in kernel
interface to be implemented by all the network drivers to filter their
capabilities.

The problems with the quick hack approach are:
1) Different for each driver
2) Need to understand how each driver works to do it
3) Need to maintain the driver patch against evolving kernel

And all this work will be done again and again by everyone that has
this problem.

OTOH adding a new interface to all the drivers is a "one off" effort
but _much_ more intrusive than my patch.
As we are arguing over if its worth adding a patch which requires zero
driver modifications and has zero impact for those that chose not to
enable it I don't see how adding a new interface to all the network
drivers just to meet the needs  of a few embedded people would ever be
accepted...

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Thu, 2009-06-11 at 13:44 -0400, Nicolas Pitre wrote:
> On Thu, 11 Jun 2009, Ben Hutchings wrote:
> 
> > On Thu, 2009-06-11 at 12:22 -0400, Nicolas Pitre wrote:
> > > On Thu, 11 Jun 2009, Ben Hutchings wrote:
> > > 
> > > > On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> > > > > Ben Hutchings wrote:
> > > > > > Who needs this feature?  Why not use ethtool in an initramfs?
> > > > > >
> > > > > >   
> > > > > > Forcing speed and duplex is occasionally needed to work around a link
> > > > > > partner that doesn't implement autonegotiation correctly.  I don't see
> > > > > > that it should ever be needed in platform configuration.  If the driver
> > > > > > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > > > > > be fixed.  Overriding the settings once will not prevent an unsupported
> > > > > > mode being selected later.
> > > > > >
> > > > > >   
> > > > > To summarize the recent points I made in the smc91x: forcing speed thread :
> > > > > 
> > > > > 1) Setting up and maintaining an initramfs can increase the complexity
> > > > > for embedded systems - it's another image file to build, distribute,
> > > > > update to bootloader etc.
> > > > 
> > > > This doesn't seem like a huge burden if you're net-booting.  And if
> > > > you're not net-booting, it's not critical that you override the link
> > > > mode immediately; you can do it in the regular init scripts.
> > > 
> > > Sure... But for some embedded setup this is actually more trouble and 
> > > hassle than having a non-intrusive kernel based facility that can set 
> > > defaults for you.
> > 
> > Doing it in an init script is even less intrusive!  But it seems that
> > the ethtool API doesn't do what you need in this case, anyway.
> 
> Stop thinking in terms of workstation setups please.

Maybe you should stop thinking that an embedded system has to do things
entirely differently?  Please note, I'm talking about an init script -
which you surely must have - not an initramfs, which I recognise is
optional and unnecessary for most embedded systems.

> [...]
> > It's an example of providing a generic solution, which is definitely
> > more than a quick hack, but I don't see it as a "clean solution" for the
> > problem that certain link modes don't work on a particular board.  A
> > clean solution would disable those modes altogether in the driver.
> 
> And now you are considering the addition of special restrictions for 
> some special hardware directly into the driver as less intrusive than 
> what is being proposed?  And now imagine what this would look like after 
> 56 different embedded designs have added their own kirks into the 
> driver.. because, yes, in the embedded world you do have much more than 
> 56 different designs sharing the same driver but each with their own 
> flaws.
[...]

I was thinking that you could add it to the platform data for such
devices, not that you would put board-specific quirks in the drivers.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3358 bytes --]

On Thu, 11 Jun 2009, Ben Hutchings wrote:

> On Thu, 2009-06-11 at 13:44 -0400, Nicolas Pitre wrote:
> > On Thu, 11 Jun 2009, Ben Hutchings wrote:
> > 
> > > On Thu, 2009-06-11 at 12:22 -0400, Nicolas Pitre wrote:
> > > > On Thu, 11 Jun 2009, Ben Hutchings wrote:
> > > > 
> > > > > On Thu, 2009-06-11 at 08:47 +0200, Martin Fuzzey wrote:
> > > > > > Ben Hutchings wrote:
> > > > > > > Who needs this feature?  Why not use ethtool in an initramfs?
> > > > > > >
> > > > > > >   
> > > > > > > Forcing speed and duplex is occasionally needed to work around a link
> > > > > > > partner that doesn't implement autonegotiation correctly.  I don't see
> > > > > > > that it should ever be needed in platform configuration.  If the driver
> > > > > > > doesn't detect the MAC/PHY capabilities correctly then the driver should
> > > > > > > be fixed.  Overriding the settings once will not prevent an unsupported
> > > > > > > mode being selected later.
> > > > > > >
> > > > > > >   
> > > > > > To summarize the recent points I made in the smc91x: forcing speed thread :
> > > > > > 
> > > > > > 1) Setting up and maintaining an initramfs can increase the complexity
> > > > > > for embedded systems - it's another image file to build, distribute,
> > > > > > update to bootloader etc.
> > > > > 
> > > > > This doesn't seem like a huge burden if you're net-booting.  And if
> > > > > you're not net-booting, it's not critical that you override the link
> > > > > mode immediately; you can do it in the regular init scripts.
> > > > 
> > > > Sure... But for some embedded setup this is actually more trouble and 
> > > > hassle than having a non-intrusive kernel based facility that can set 
> > > > defaults for you.
> > > 
> > > Doing it in an init script is even less intrusive!  But it seems that
> > > the ethtool API doesn't do what you need in this case, anyway.
> > 
> > Stop thinking in terms of workstation setups please.
> 
> Maybe you should stop thinking that an embedded system has to do things
> entirely differently?

Sorry, but like it or not, embedded systems are different.

> Please note, I'm talking about an init script -
> which you surely must have - not an initramfs, which I recognise is
> optional and unnecessary for most embedded systems.

An init script is already a luxury for some systems.  It means a 
script, which implies a shell, and of course the tool binaries (ethtool 
in this case) to be called by your shell.  Not only those do bloat your 
system (the shell + tool + script occupies way more space than the 
proposed module) and then you do have to maintain those components as 
well, but it also has impact on boot time.  Remember that we're not 
talking about systems bragging about their boot time being under 20 
seconds here, but systems that need to be operational in only a couple 
miliseconds.

> I was thinking that you could add it to the platform data for such
> devices, not that you would put board-specific quirks in the drivers.

And what if the majority of users for a driver simply don't need such a 
thing?  And how do you do that if the driver you need is for a PCI 
device?  And why would driver specific kirks be better than a generic 
module that can handle those params in a uniform way across all drivers?  
Especially if you can ignore said module if you don't need/want to use 
it?


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Thu, 2009-06-11 at 15:08 -0400, Nicolas Pitre wrote:
[...]
> > Please note, I'm talking about an init script -
> > which you surely must have - not an initramfs, which I recognise is
> > optional and unnecessary for most embedded systems.
> 
> An init script is already a luxury for some systems.

And so is an Ethernet port.  Instead of talking about hypotheticals, why
not talk about the system you need this workaround for?

> It means a 
> script, which implies a shell, and of course the tool binaries (ethtool 
> in this case) to be called by your shell.  Not only those do bloat your 
> system (the shell + tool + script occupies way more space than the 
> proposed module)

Well, if you're replacing init - which is generally a really bad idea,
by the way - you can build the ethtool API calls in there along with all
your other application-specific stuff.  The ethtool API isn't terribly
complex.

> and then you do have to maintain those components as 
> well, but it also has impact on boot time.  Remember that we're not 
> talking about systems bragging about their boot time being under 20 
> seconds here, but systems that need to be operational in only a couple 
> miliseconds.

If you want to boot that quickly you definitely don't want to have to
wait for PHY operations.

> > I was thinking that you could add it to the platform data for such
> > devices, not that you would put board-specific quirks in the drivers.
> 
> And what if the majority of users for a driver simply don't need such a 
> thing?  And how do you do that if the driver you need is for a PCI 
> device?

Any device can have platform data; it's part of struct device.

> And why would driver specific kirks be better than a generic 
> module that can handle those params in a uniform way across all drivers?  
> Especially if you can ignore said module if you don't need/want to use 
> it?

Because its raison d'etre is apparently to disable the broken link
modes, and it doesn't do that properly.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

On Thu, 11 Jun 2009, Ben Hutchings wrote:

> On Thu, 2009-06-11 at 15:08 -0400, Nicolas Pitre wrote:
> [...]
> > > Please note, I'm talking about an init script -
> > > which you surely must have - not an initramfs, which I recognise is
> > > optional and unnecessary for most embedded systems.
> > 
> > An init script is already a luxury for some systems.
> 
> And so is an Ethernet port.  Instead of talking about hypotheticals, why
> not talk about the system you need this workaround for?

Look, I personally do not need this.  And I'm rather tired arguing with 
you.  Your shortsightedness and refusal for accommodation about needs 
that aren't yours is representative of the main reason why there is 
still a disconnect between mainline people and embedded people.  If you 
can't admit it then I've failed my attempt at bridging the gap.

The issue was discussed, people objected to driver kirks already, then I 
proposed this solution to which Davem agreed if it was done cleanly and 
maintained. And here it is under the form of an actual patch. So far 
you're the only one who just can't accept that compromise.  If your 
opposition -- which so far has nothing to do with the actual 
implementation but rather on the principle -- is not overruled by others 
then the embedded disconnect, of which this is only a tiny part of, has 
no hope of ever vanishing.

Have a good day.


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Ben Hutchings]

On Thu, 2009-06-11 at 16:24 -0400, Nicolas Pitre wrote:
> On Thu, 11 Jun 2009, Ben Hutchings wrote:
> 
> > On Thu, 2009-06-11 at 15:08 -0400, Nicolas Pitre wrote:
> > [...]
> > > > Please note, I'm talking about an init script -
> > > > which you surely must have - not an initramfs, which I recognise is
> > > > optional and unnecessary for most embedded systems.
> > > 
> > > An init script is already a luxury for some systems.
> > 
> > And so is an Ethernet port.  Instead of talking about hypotheticals, why
> > not talk about the system you need this workaround for?
> 
> Look, I personally do not need this.  And I'm rather tired arguing with 
> you.  Your shortsightedness and refusal for accommodation about needs 
> that aren't yours is representative of the main reason why there is 
> still a disconnect between mainline people and embedded people.  If you 
> can't admit it then I've failed my attempt at bridging the gap.
[...]

It's not my call as to whether your needs are accommodated, and in any
case I'm not trying to dismiss them.  I'm trying to understand them and
to suggest what seems like a better solution.

I'm primarily a driver maintainer and I work with an out-of-tree module
as well as an in-tree driver.  I've had to deal with many objections in
the process of submitting that out-of-tree code, and I worked to
overcome them.  This took several iterations and it was quite
frustrating at times.  But the result was a better driver.

I don't want to spend time arguing either, so if I've not yet convinced
you to try an alternative then so be it.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

Ben Hutchings wrote:
> It's not my call as to whether your needs are accommodated, and in any
> case I'm not trying to dismiss them.  I'm trying to understand them and
> to suggest what seems like a better solution.
>   
Unfortunately the solution you are proposing (modifying the drivers) has
already been rejected.

The solution here, proposed by Nicolas and implemented by myself, has
already been accepted in principle by David Miller (subject to a clean
implementation and being maintained).

The only argument for your (much more intrusive) solution is that it
will prevent someone later shooting themselves in the foot by using
ethtool to reconfigure the interface to a non working state. But :

1) With the proposed solution there's no need to even have the ethool
binary available.
2) If you're that worried about people shooting themselves in the foot
let's modify the filesystems to refuse deleting files in /bin...

> I'm primarily a driver maintainer and I work with an out-of-tree module
> as well as an in-tree driver.  I've had to deal with many objections in
> the process of submitting that out-of-tree code, and I worked to
> overcome them.  This took several iterations and it was quite
> frustrating at times.  But the result was a better driver.
>   
Yes but it sounds like you're describing the normal review process not
an unwillingless to accept the very idea of your driver.
I have no problem undergoing several iterations (hey that's why I called
it RFC) but I had been hoping for constructive criticism of the
implementation not the principle or the need for this type of module
[which I thought settled in the original thread].

I consider it one of the great strengths of linux that  it can run on
everything from cell phones to super computers and meet the diverse
needs of all those people. I personally have no use for the majority of
the kernel code but I don't see that as as reason it shouldn't be there.

If your view prevails all I will have achieved is the replacement of my
personel 2 line #ifdef hack by 500 lines of "generic" code that no one
else will ever see. Of course in the grand scheme of things that's
insignificant but then I hope people don't complain about embedded
developpers not contributing enough. I did try, really.

Unless anyone wants to discuss the implementation this is my last post
on this subject.

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Nicolas Pitre <nico@cam.org>
Date: Thu, 11 Jun 2009 12:22:59 -0400 (EDT)

> I think this is a good example from the embedded world providing a
> clean solution to a recurring issue which would return again to
> quick hacks if the mainline reception is "use a cramfs" again.

Well initrd is the cleanest solution.

People don't want to do it because it requires some work, and there
always recurring passive-aggressive mentions of "space issues" which
frankly is totally bogus.  You can make this thing as tiny as you want
it to be.

If someone implemented it one time, then it could be shared by other
folks needing the same solution.

But I guess us upstream kernel folk just "don't understand."

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 11 Jun 2009 17:52:46 +0100

> A clean solution would disable those modes altogether in the driver.

Absolutely agreed, in this specific case.  If that part of the
chip or the PHY is non-functional, it should be disabled in
the driver entirely.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Martin Fuzzey <mfuzzey@gmail.com>
Date: Thu, 11 Jun 2009 19:45:07 +0200

> The problems with the quick hack approach are:
> 1) Different for each driver
> 2) Need to understand how each driver works to do it
> 3) Need to maintain the driver patch against evolving kernel

Hardware specific solutions for hardware specific problems.

You can instantiate a platform_device that the driver matches
and uses to acquire board-specific-juju such as link modes
which are non-functional.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Nicolas Pitre <nico@cam.org>
Date: Thu, 11 Jun 2009 16:24:05 -0400 (EDT)

> Your shortsightedness and refusal for accommodation about needs 
> that aren't yours is representative of the main reason why there is 
> still a disconnect between mainline people and embedded people.

Our main beef with the embedded folks is short-sightedness and a
desire for quick solutions rather than clean ones with long term
considerations in mind.

You exemplified that here.

And now you're desire parachute out of the discussion only confirms
it even more.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Nicolas Pitre]

On Thu, 11 Jun 2009, David Miller wrote:

> From: Nicolas Pitre <nico@cam.org>
> Date: Thu, 11 Jun 2009 16:24:05 -0400 (EDT)
> 
> > Your shortsightedness and refusal for accommodation about needs 
> > that aren't yours is representative of the main reason why there is 
> > still a disconnect between mainline people and embedded people.
> 
> Our main beef with the embedded folks is short-sightedness and a
> desire for quick solutions rather than clean ones with long term
> considerations in mind.

Let me quote Martin again:

| If your view prevails all I will have achieved is the replacement of 
| my personel 2 line #ifdef hack by 500 lines of "generic" code that no 
| one else will ever see. Of course in the grand scheme of things that's
| insignificant but then I hope people don't complain about embedded
| developpers not contributing enough. I did try, really.

Wasn't his patch looking like a clean solution with long term 
considerations?  If no then I don't think we have much else left to 
discuss.

And no, as absurd and strange that might sound to you, maintaining extra 
components to access the ethtool interface from user space (whether or 
not it is for broken PHY or whatnot) in an embedded product is often not 
worth the bother.  Those systems are not workstations where all that you 
have to do is fire an editor and type one line of command in a script 
because the default environment makes everything already available.  

Compare that to the quick solution that the one line hack represents.


Nicolas

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Nicolas Pitre <nico@cam.org>
Date: Thu, 11 Jun 2009 20:38:43 -0400 (EDT)

> Compare that to the quick solution that the one line hack represents.

I can't wait until the kernel is full of those.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Mark Brown]

On Thu, Jun 11, 2009 at 05:09:48PM -0700, David Miller wrote:

> You can instantiate a platform_device that the driver matches
> and uses to acquire board-specific-juju such as link modes
> which are non-functional.

How would you handle PCI devices in that scheme?  Last time I looked at
this (a while ago, so I may be out of date) there didn't appear to be a
sensible generic way of getting platform data to PCI devices.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Fri, 12 Jun 2009 11:50:20 +0100

> On Thu, Jun 11, 2009 at 05:09:48PM -0700, David Miller wrote:
> 
>> You can instantiate a platform_device that the driver matches
>> and uses to acquire board-specific-juju such as link modes
>> which are non-functional.
> 
> How would you handle PCI devices in that scheme?  Last time I looked at
> this (a while ago, so I may be out of date) there didn't appear to be a
> sensible generic way of getting platform data to PCI devices.

It's irrelevant in this discussion because in the contexts where
this is wanted, people are adding driver-wide hacks to make
these settings.

So a dummy platform_device would serve handily.

For PCI, in general, the VPD or openfirmware descriptions could
be used to describe such issues.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

On Fri, Jun 12, 2009 at 2:09 AM, David Miller<davem@davemloft.net> wrote:

> You can instantiate a platform_device that the driver matches
> and uses to acquire board-specific-juju such as link modes
> which are non-functional.
>
Just to make sure I'm understanding you here - that would still
require modifying each and every network driver to use and interpret
the said platform_device right?
This proposal doesn't require _any_ changes to the network device drivers.
It also doesn't have _any_ impact on the kernel for those that don't enable it.

The smc9x driver contains this comment :
/*
 * The internal workings of the driver.  If you are changing anything
 * here with the SMC stuff, you should have the datasheet and know
 * what you are doing.
 */
Before writing this patch I had to disregard this to do the hacked version.
I wasn't touching the driver to improve it or fix it - in which case
it would be quite reasonable to expect me to have read the datasheet
but just to turn off a bit of functionality to work around broken
hardware.

Why should people have to dabble in driver internals just to say
"please use this mode"?
The proposed patch allows you to do this in a portable way so that
when you work on another board with a different network chip you don't
need to do it all again.

I thought I'd done everything right;I posted a question on the mailing
list, _before_ writing any code, discussion ensued and we seemed to
reach a consensus as when Nicolas proposed this solution you said:

>And you could even write it and maintain it somewhere if you
>like for other embedded people to use. :-)
>Once full featured, I'd be happy to include it.

So I went off and implemented it.
I'm not claiming  it's perfect but we're not even discussing the
implementation and how to improve it but have gone back to discussing
if we should even be doing it. This is _very_ fusttrating

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Mark Brown]

On Fri, Jun 12, 2009 at 04:33:50AM -0700, David Miller wrote:
> From: Mark Brown <broonie@opensource.wolfsonmicro.com>

> > How would you handle PCI devices in that scheme?  Last time I looked at
> > this (a while ago, so I may be out of date) there didn't appear to be a
> > sensible generic way of getting platform data to PCI devices.

> It's irrelevant in this discussion because in the contexts where
> this is wanted, people are adding driver-wide hacks to make
> these settings.

...which work well enough locally have problems getting upstream.

> For PCI, in general, the VPD or openfirmware descriptions could
> be used to describe such issues.

Where this is needed for PCI devices the VPD isn't useful - the hardware
often won't have the SEPROM (or whatever) that the device is intended to
load configuration from for cost or production reasons, the data being
provided elsewhere through something like OpenFirmware.

OpenFirmware works well enough on the platforms that use it but it's far
from universal.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Martin Fuzzey <mfuzzey@gmail.com>
Date: Fri, 12 Jun 2009 14:19:11 +0200

> The smc9x driver contains this comment :
> /*
>  * The internal workings of the driver.  If you are changing anything
>  * here with the SMC stuff, you should have the datasheet and know
>  * what you are doing.
>  */
> Before writing this patch I had to disregard this to do the hacked version.
> I wasn't touching the driver to improve it or fix it - in which case
> it would be quite reasonable to expect me to have read the datasheet
> but just to turn off a bit of functionality to work around broken
> hardware.

I don't expect anyone to be required to read a data-sheet just to
prevent some link mode settings.

If the driver isn't clean enough to make a change like that easy,
that's a bug.

This is just a scarecrow argument.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Fri, 12 Jun 2009 13:24:32 +0100

> OpenFirmware works well enough on the platforms that use it but it's far
> from universal.

Sounds like an argument for it being more-so :-)

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

David Miller wrote:
> I don't expect anyone to be required to read a data-sheet just to
> prevent some link mode settings.
>
> If the driver isn't clean enough to make a change like that easy,
> that's a bug.
>
> This is just a scarecrow argument.
>   
Ok if you like.

But please reply to the rest of that message, in particular explain why,
having publicly stated you would accept an implementation of Nicolas'
idea, you are now back pedaling. I'm quite prepared to be told that the
code itself is not good enough yet (with indications of how to improve
it) but we're not even discussing the code.

The impression (and I sincerely hope I am wrong) this gives me is that
when you accepted the idea in principle you didn't think anyone would
actually step up and write the code [presumably because us embedded
people are too busy working on our quick hacks]. Well I did and now I'm
calling upon you to keep your word.

At the end of the day you are the networking maintainer and I'm a no
name so you can tell me to go jump in the lake. But think a little about
the message that would send first. To the best of my knowledge I
respected the linux development process by discussing first and
apparently reaching a consensus - it's not like I jumped in with a chunk
of code demanding it be merged. If I misunderstood the process please
tell me too.

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Martin Fuzzey <mfuzzey@gmail.com>
Date: Sat, 13 Jun 2009 09:00:10 +0200

> But please reply to the rest of that message, in particular explain why,
> having publicly stated you would accept an implementation of Nicolas'
> idea, you are now back pedaling.

I misunderstood the problem, thanks for asking me to clarify
this.

I thought the issue was the "in the environment where his devices
would be used" he wanted to force a certain link speed and duplex
setting.  In that case, forcing the link via ethtool is appropriate
because it's an attribute of where the device is connected.

But the situation is different, the physical hardware has a limitation
and know of this belongs, or should be described, in the driver
somehow.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

David Miller wrote:
> I misunderstood the problem, thanks for asking me to clarify
> this.
>
> I thought the issue was the "in the environment where his devices
> would be used" he wanted to force a certain link speed and duplex
> setting.  In that case, forcing the link via ethtool is appropriate
> because it's an attribute of where the device is connected.
>   
Well I thought my initial post (in the smc91x force speed thread before
this patch was written) was quite unambiguous as to my motivation :

I am using the SMC91x driver on a based ARM board that has a hardware
problem causing 100Mbps mode not to work (even though the PHY
negotiates to that speed).


That being said even if this is _my_ use case the facility is equally
useful for the use case you understood (and yes you could use ethtool
from userspace/initramfs/initscript to do that but you've already
accepted the idea of an in kernel ethtool for the "environment" case).
> But the situation is different, the physical hardware has a limitation
> and know of this belongs, or should be described, in the driver
> somehow.
>   
Theoretically I agree. However the only practical advantage I can see of
doing it in the driver is that it is then impossible to later re-enable
broken modes. The proposed patch would allow you to later mess up
networking but requires :
1) ethtool installed on the device
2) root access to the device

If you have root access there are all manner of ways you can mess up the
system and breaking networking probably comes quite a way down the list.

If you want to do it in the driver for this usecase either it's back to
everyone for himself quick hacks or we need to add an interface to _all_
the drivers. How long would that take? How long did ethtool take? Given
the trouble this very non intrusive patch is having would you really
accept a patch to all the network drivers for this? I for one wouldn't
do it as I don't have all that hardware to test.

Even if we did go the "purist" route and add an interface to all the
drivers this patch would still be useful for the other usecase (the one
you understood).

So my argument is that while this patch is not the purist solution for
my usecase I consider it an acceptable solution (better than hacks) and
a good solution for other usecases.

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[David Miller]

From: Martin Fuzzey <mfuzzey@gmail.com>
Date: Sat, 13 Jun 2009 09:51:38 +0200

> David Miller wrote:
>> But the situation is different, the physical hardware has a limitation
>> and know of this belongs, or should be described, in the driver
>> somehow.
>>   
> Theoretically I agree. However the only practical advantage I can see of
> doing it in the driver is that it is then impossible to later re-enable
> broken modes.

The bug is a hardware limitation.

The kernel programs and knows the hardware.

Therefore knowledge of the limitation belongs in the kernel.

In no other situation would we say "this aspect of this chip doesn't
work, so we'll block usage of that in some high level configuration
framework"

No, we'd always deal with HW problems in the driver itself.

You'd only re-enable the broken modes later when you know they
are working properly, in which case you can update the driver as
appropriate.  And this is what we'd do in other similar cases too.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

David Miller wrote:
> From: Martin Fuzzey <mfuzzey@gmail.com>
> Date: Sat, 13 Jun 2009 09:51:38 +0200
>   
> The bug is a hardware limitation.
>
> The kernel programs and knows the hardware.
>
> Therefore knowledge of the limitation belongs in the kernel.
>   
Yes indeed and that's why I included the programatic interface to let
the board specific code in the kernel do it in a non intrusive way with
zero driver modifications.
For me the command line method is for the "environment" case you mention
and the programatic interface is for the broken hardware case. The patch
supports both (or just one however you decide to KConfig it) so if you
just want to work around broken hardware you don't have to pay the
overhead of the command line parsing code.
> In no other situation would we say "this aspect of this chip doesn't
> work, so we'll block usage of that in some high level configuration
> framework"
>
> No, we'd always deal with HW problems in the driver itself.
>   
Yes I agree but this isn't about the _chip_ being broken but the _board_
being broken.
IMHO the drivers are supposed to be  chip (or even chip family) specific
not board specific.
The driver in question is fine and doesn't need fixing - even the board
schematic is fine - it's the board layout causing signal corruption at
100MBps...

Martin

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Mark Brown]

On Fri, Jun 12, 2009 at 05:01:57PM -0700, David Miller wrote:
> From: Mark Brown <broonie@opensource.wolfsonmicro.com>

> > OpenFirmware works well enough on the platforms that use it but it's far
> > from universal.

> Sounds like an argument for it being more-so :-)

It'd be nice if it were; one of my problems with OpenFirmware right now
is that it just makes for more work since it just winds up meaning we
have to do platform data two ways.  If we made a decision to move to
using OpenFirmware more consistently this would be less of an issue.

Re: [RFC PATCH] Ethtool style in kernel network driver configuration.

[Martin Fuzzey]

David Miller wrote:
> From: Martin Fuzzey <mfuzzey@gmail.com>
> Date: Sat, 13 Jun 2009 09:00:10 +0200
>
>   
>> But please reply to the rest of that message, in particular explain why,
>> having publicly stated you would accept an implementation of Nicolas'
>> idea, you are now back pedaling.
>>     
>
> I misunderstood the problem, thanks for asking me to clarify
> this.
>
> I thought the issue was the "in the environment where his devices
> would be used" he wanted to force a certain link speed and duplex
> setting.  In that case, forcing the link via ethtool is appropriate
> because it's an attribute of where the device is connected.
>
> But the situation is different, the physical hardware has a limitation
> and know of this belongs, or should be described, in the driver
> somehow.
>   
Actually it's both.
I think you are mixing up the problem _I_ am having (on my broken board)
and the range of problems _this patch_ is intended to address.

As is explained in the patch header the patch allows unmodified network
drivers to be configured using their ethtool interface in _two_ manners :

1) From the kernel command line
2) From kernel code (typically the board setup code)

Maybe however I didn't make the different use cases of these two methods
sufficiently clear.

_Command line configuration_

This is _not_ intended to be used to work around broken hardware but
indeed to be used to configure the device for the network environment in
which it will be used when autonegotiation cannot be used for some
reason. This is thus the same use case as userspace ethtool.

The kernel already has command line configuration for the IP layer
(static IP address, DHCP etc) so this can just be viewed as extending
this to a lower layer in the network stack.

All of this (both the link layer and the IP configuration) _could _ be
done from userspace initram FS or whatever but in embedded situations it
really is very useful to be able to do it from the kernel command line.
While I have never used this facility on workstations or servers I use
it on embedded boards _every day_ (and I suspect the same holds form
many / most embedded developpers). Note that were command line
configuration for the IP layer not available the NFS root + static IP
case would be particularly awkward to handle (as it would require a
different initramfs for every board to ensure unique IPs).

A concrete usecase for this is when a (fully working) device supporting
with 10/100 link speeds needs to be forced to a lower speed for testing
purposes [for example because it will be deployed to a site only having
a 10Mbps network but it needs to be tested in the lab with a simple
10/100 non manageable switch].

The users of this facility are not necesarilly developpers but could be
testers etc (as all they need to do is change a parameter in the boot
loader).


_ programmatic interface _

This method is intended to work around broken _boards_ (not drivers -
which should of course be fixed, nor _chips_ which should be
disactivated in the driver).

It enables the developper of the board setup code to force certain link
parameters.
In this manner the board setup code centralises knowledge about the
board (GPIOs, chip addresses, and any networking workarounds required).

Since it is called from code:

1) It is indeed a case of kernel _code_ not configuration doing the work
around
2) The activation of the workaround may be conditional (for example
based on reading the board revision number from hardware)


The patch allows the kernel to be configured with just one or both of
these methods as desired. For example on my broken board I am only using
the programmatic method at the moment (after testing the command line
method of course).

Hope this clarifies things.

Martin