[PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Jeff Garzik]

Also, adds support for the first flag, LRO.

Earlier version of this patch posted about 12 hours ago.


commit 84bf82f38dacde0fa86986e23c061728e819810e
Author: Jeff Garzik <jeff@garzik.org>
Date:   Fri Aug 10 05:27:23 2007 -0400

    [ETHTOOL] Add ETHTOOL_[GS]FLAGS sub-ioctls
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 include/linux/ethtool.h   |   21 +++++++++++++++
 include/linux/netdevice.h |    1 
 net/core/ethtool.c        |   61 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+)

84bf82f38dacde0fa86986e23c061728e819810e
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 23ccea8..0e5de2f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -256,6 +256,19 @@ struct ethtool_perm_addr {
 	__u8	data[0];
 };
 
+/* boolean flags controlling per-interface behavior characteristics.
+ * When reading, the flag indicates whether or not a certain behavior
+ * is enabled/present.  When writing, the flag indicates whether
+ * or not the driver should turn on (set) or off (clear) a behavior.
+ *
+ * Some behaviors may read-only (unconditionally absent or present).
+ * If such is the case, return EINVAL in the set-flags operation if the
+ * flag differs from the read-only value.
+ */
+enum ethtool_flags {
+	ETH_FLAG_LRO		= (1 << 15),	/* LRO is enabled */
+};
+
 #ifdef __KERNEL__
 
 struct net_device;
@@ -272,6 +285,8 @@ u32 ethtool_op_get_tso(struct net_device *dev);
 int ethtool_op_set_tso(struct net_device *dev, u32 data);
 u32 ethtool_op_get_ufo(struct net_device *dev);
 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);
 
 /**
  * &ethtool_ops - Alter and report network device settings
@@ -307,6 +322,8 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data);
  * get_strings: Return a set of strings that describe the requested objects 
  * phys_id: Identify the device
  * get_stats: Return statistics about the device
+ * get_flags: get 32-bit flags bitmap
+ * set_flags: set 32-bit flags bitmap
  * 
  * Description:
  *
@@ -369,6 +386,8 @@ struct ethtool_ops {
 	void	(*complete)(struct net_device *);
 	u32     (*get_ufo)(struct net_device *);
 	int     (*set_ufo)(struct net_device *, u32);
+	u32     (*get_flags)(struct net_device *);
+	int     (*set_flags)(struct net_device *, u32);
 };
 #endif /* __KERNEL__ */
 
@@ -410,6 +429,8 @@ struct ethtool_ops {
 #define ETHTOOL_SUFO		0x00000022 /* Set UFO enable (ethtool_value) */
 #define ETHTOOL_GGSO		0x00000023 /* Get GSO enable (ethtool_value) */
 #define ETHTOOL_SGSO		0x00000024 /* Set GSO enable (ethtool_value) */
+#define ETHTOOL_GFLAGS		0x00000025 /* Get flags bitmap(ethtool_value) */
+#define ETHTOOL_SFLAGS		0x00000026 /* Set flags bitmap(ethtool_value) */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4a616d7..559a4dc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -341,6 +341,7 @@ struct net_device
 #define NETIF_F_GSO		2048	/* Enable software GSO. */
 #define NETIF_F_LLTX		4096	/* LockLess TX */
 #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
+#define NETIF_F_LRO		32768	/* large receive offload */
 
 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 2ab0a60..6e8563e 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -109,6 +109,32 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data)
 	return 0;
 }
 
+/* the following list of flags are the same as their associated
+ * NETIF_F_xxx values in include/linux/netdevice.h
+ */
+static const u32 flags_dup_features =
+	ETH_FLAG_LRO;
+
+u32 ethtool_op_get_flags(struct net_device *dev)
+{
+	/* in the future, this function will probably contain additional
+	 * handling for flags which are not so easily handled
+	 * by a simple masking operation
+	 */
+	
+	return dev->features & flags_dup_features;
+}
+
+int ethtool_op_set_flags(struct net_device *dev, u32 data)
+{
+	if (data & ETH_FLAG_LRO)
+		dev->features |= NETIF_F_LRO;
+	else
+		dev->features &= ~NETIF_F_LRO;
+
+	return 0;
+}
+
 /* Handlers for each ethtool command */
 
 static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
@@ -783,6 +809,33 @@ static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
 	return 0;
 }
 
+static int ethtool_get_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata = { ETHTOOL_GFLAGS };
+
+	if (!dev->ethtool_ops->get_flags)
+		return -EOPNOTSUPP;
+
+	edata.data = dev->ethtool_ops->get_flags(dev);
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata;
+
+	if (!dev->ethtool_ops->set_flags)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&edata, useraddr, sizeof(edata)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_flags(dev, edata.data);
+}
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct ifreq *ifr)
@@ -935,6 +988,12 @@ int dev_ethtool(struct ifreq *ifr)
 	case ETHTOOL_SGSO:
 		rc = ethtool_set_gso(dev, useraddr);
 		break;
+	case ETHTOOL_GFLAGS:
+		rc = ethtool_get_flags(dev, useraddr);
+		break;
+	case ETHTOOL_SFLAGS:
+		rc = ethtool_set_flags(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
@@ -960,3 +1019,5 @@ EXPORT_SYMBOL(ethtool_op_set_tx_hw_csum);
 EXPORT_SYMBOL(ethtool_op_set_tx_ipv6_csum);
 EXPORT_SYMBOL(ethtool_op_set_ufo);
 EXPORT_SYMBOL(ethtool_op_get_ufo);
+EXPORT_SYMBOL(ethtool_op_set_flags);
+EXPORT_SYMBOL(ethtool_op_get_flags);

[PATCH 2/4] ethtool: introduce get_sset_count

[Jeff Garzik]

Required prep for the priv-flags changeset (next patch), which also uses
ethtool string sets.


commit 86fe0ff220a795c82aa9dea5dc4b7359519a1366
Author: Jeff Garzik <jeff@garzik.org>
Date:   Fri Aug 10 15:49:21 2007 -0400

    [ETHTOOL] Introduce get_sset_count. Obsolete get_stats_count, self_test_count
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 include/linux/ethtool.h |    7 ++-
 net/core/ethtool.c      |   95 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 75 insertions(+), 27 deletions(-)

86fe0ff220a795c82aa9dea5dc4b7359519a1366
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 0e5de2f..f617998 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -376,11 +376,9 @@ struct ethtool_ops {
 	int	(*set_sg)(struct net_device *, u32);
 	u32	(*get_tso)(struct net_device *);
 	int	(*set_tso)(struct net_device *, u32);
-	int	(*self_test_count)(struct net_device *);
 	void	(*self_test)(struct net_device *, struct ethtool_test *, u64 *);
 	void	(*get_strings)(struct net_device *, u32 stringset, u8 *);
 	int	(*phys_id)(struct net_device *, u32);
-	int	(*get_stats_count)(struct net_device *);
 	void	(*get_ethtool_stats)(struct net_device *, struct ethtool_stats *, u64 *);
 	int	(*begin)(struct net_device *);
 	void	(*complete)(struct net_device *);
@@ -388,6 +386,11 @@ struct ethtool_ops {
 	int     (*set_ufo)(struct net_device *, u32);
 	u32     (*get_flags)(struct net_device *);
 	int     (*set_flags)(struct net_device *, u32);
+	int	(*get_sset_count)(struct net_device *, int);
+
+	/* the following hooks are obsolete */
+	int	(*self_test_count)(struct net_device *);/* use get_sset_count */
+	int	(*get_stats_count)(struct net_device *);/* use get_sset_count */
 };
 #endif /* __KERNEL__ */
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 6e8563e..2b366a5 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -179,10 +179,23 @@ static int ethtool_get_drvinfo(struct net_device *dev, void __user *useraddr)
 	info.cmd = ETHTOOL_GDRVINFO;
 	ops->get_drvinfo(dev, &info);
 
-	if (ops->self_test_count)
-		info.testinfo_len = ops->self_test_count(dev);
-	if (ops->get_stats_count)
-		info.n_stats = ops->get_stats_count(dev);
+	if (ops->get_sset_count) {
+		int rc;
+
+		rc = ops->get_sset_count(dev, ETH_SS_TEST);
+		if (rc >= 0)
+			info.testinfo_len = rc;
+		rc = ops->get_sset_count(dev, ETH_SS_STATS);
+		if (rc >= 0)
+			info.n_stats = rc;
+	} else {
+		/* code path for obsolete hooks */
+
+		if (ops->self_test_count)
+			info.testinfo_len = ops->self_test_count(dev);
+		if (ops->get_stats_count)
+			info.n_stats = ops->get_stats_count(dev);
+	}
 	if (ops->get_regs_len)
 		info.regdump_len = ops->get_regs_len(dev);
 	if (ops->get_eeprom_len)
@@ -669,16 +682,27 @@ static int ethtool_self_test(struct net_device *dev, char __user *useraddr)
 	struct ethtool_test test;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u64 *data;
-	int ret;
+	int ret, test_len;
 
-	if (!ops->self_test || !ops->self_test_count)
+	if (!ops->self_test)
+		return -EOPNOTSUPP;
+	if (!ops->get_sset_count && !ops->self_test_count)
 		return -EOPNOTSUPP;
 
+	if (ops->get_sset_count)
+		test_len = ops->get_sset_count(dev, ETH_SS_TEST);
+	else
+		/* code path for obsolete hook */
+		test_len = ops->self_test_count(dev);
+	if (test_len < 0)
+		return test_len;
+	WARN_ON(test_len == 0);
+
 	if (copy_from_user(&test, useraddr, sizeof(test)))
 		return -EFAULT;
 
-	test.len = ops->self_test_count(dev);
-	data = kmalloc(test.len * sizeof(u64), GFP_USER);
+	test.len = test_len;
+	data = kmalloc(test_len * sizeof(u64), GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
@@ -710,19 +734,29 @@ static int ethtool_get_strings(struct net_device *dev, void __user *useraddr)
 	if (copy_from_user(&gstrings, useraddr, sizeof(gstrings)))
 		return -EFAULT;
 
-	switch (gstrings.string_set) {
-	case ETH_SS_TEST:
-		if (!ops->self_test_count)
-			return -EOPNOTSUPP;
-		gstrings.len = ops->self_test_count(dev);
-		break;
-	case ETH_SS_STATS:
-		if (!ops->get_stats_count)
-			return -EOPNOTSUPP;
-		gstrings.len = ops->get_stats_count(dev);
-		break;
-	default:
-		return -EINVAL;
+	if (ops->get_sset_count) {
+		ret = ops->get_sset_count(dev, gstrings.string_set);
+		if (ret < 0)
+			return ret;
+
+		gstrings.len = ret;
+	} else {
+		/* code path for obsolete hooks */
+
+		switch (gstrings.string_set) {
+		case ETH_SS_TEST:
+			if (!ops->self_test_count)
+				return -EOPNOTSUPP;
+			gstrings.len = ops->self_test_count(dev);
+			break;
+		case ETH_SS_STATS:
+			if (!ops->get_stats_count)
+				return -EOPNOTSUPP;
+			gstrings.len = ops->get_stats_count(dev);
+			break;
+		default:
+			return -EINVAL;
+		}
 	}
 
 	data = kmalloc(gstrings.len * ETH_GSTRING_LEN, GFP_USER);
@@ -762,16 +796,27 @@ static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
 	struct ethtool_stats stats;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	u64 *data;
-	int ret;
+	int ret, n_stats;
 
-	if (!ops->get_ethtool_stats || !ops->get_stats_count)
+	if (!ops->get_ethtool_stats)
 		return -EOPNOTSUPP;
+	if (!ops->get_sset_count && !ops->get_stats_count)
+		return -EOPNOTSUPP;
+
+	if (ops->get_sset_count)
+		n_stats = ops->get_sset_count(dev, ETH_SS_STATS);
+	else
+		/* code path for obsolete hook */
+		n_stats = ops->get_stats_count(dev);
+	if (n_stats < 0)
+		return n_stats;
+	WARN_ON(n_stats == 0);
 
 	if (copy_from_user(&stats, useraddr, sizeof(stats)))
 		return -EFAULT;
 
-	stats.n_stats = ops->get_stats_count(dev);
-	data = kmalloc(stats.n_stats * sizeof(u64), GFP_USER);
+	stats.n_stats = n_stats;
+	data = kmalloc(n_stats * sizeof(u64), GFP_USER);
 	if (!data)
 		return -ENOMEM;
 

[PATCH 3/4] Add ETHTOOL_[GS]PFLAGS sub-ioctls

[Jeff Garzik]

Driver-private flags.  Driver writer's guide will follow in a reply to this
patch.

commit 4901236cec047029b970261b95e47d6be60f523e
Author: Jeff Garzik <jeff@garzik.org>
Date:   Fri Aug 10 15:52:06 2007 -0400

    [ETHTOOL] Introduce ->{get,set}_priv_flags, ETHTOOL_[GS]PFLAGS
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 include/linux/ethtool.h |    8 +++++++-
 net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

4901236cec047029b970261b95e47d6be60f523e
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f617998..71d4ada 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -39,7 +39,8 @@ struct ethtool_drvinfo {
 	char	bus_info[ETHTOOL_BUSINFO_LEN];	/* Bus info for this IF. */
 				/* For PCI devices, use pci_name(pci_dev). */
 	char	reserved1[32];
-	char	reserved2[16];
+	char	reserved2[12];
+	__u32	n_priv_flags;	/* number of flags valid in ETHTOOL_GPFLAGS */
 	__u32	n_stats;	/* number of u64's from ETHTOOL_GSTATS */
 	__u32	testinfo_len;
 	__u32	eedump_len;	/* Size of data from ETHTOOL_GEEPROM (bytes) */
@@ -219,6 +220,7 @@ struct ethtool_pauseparam {
 enum ethtool_stringset {
 	ETH_SS_TEST		= 0,
 	ETH_SS_STATS,
+	ETH_SS_PRIV_FLAGS,
 };
 
 /* for passing string sets for data tagging */
@@ -386,6 +388,8 @@ struct ethtool_ops {
 	int     (*set_ufo)(struct net_device *, u32);
 	u32     (*get_flags)(struct net_device *);
 	int     (*set_flags)(struct net_device *, u32);
+	u32     (*get_priv_flags)(struct net_device *);
+	int     (*set_priv_flags)(struct net_device *, u32);
 	int	(*get_sset_count)(struct net_device *, int);
 
 	/* the following hooks are obsolete */
@@ -434,6 +438,8 @@ struct ethtool_ops {
 #define ETHTOOL_SGSO		0x00000024 /* Set GSO enable (ethtool_value) */
 #define ETHTOOL_GFLAGS		0x00000025 /* Get flags bitmap(ethtool_value) */
 #define ETHTOOL_SFLAGS		0x00000026 /* Set flags bitmap(ethtool_value) */
+#define ETHTOOL_GPFLAGS		0x00000027 /* Get driver-private flags bitmap */
+#define ETHTOOL_SPFLAGS		0x00000028 /* Set driver-private flags bitmap */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 2b366a5..f721e38 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -188,6 +188,9 @@ static int ethtool_get_drvinfo(struct net_device *dev, void __user *useraddr)
 		rc = ops->get_sset_count(dev, ETH_SS_STATS);
 		if (rc >= 0)
 			info.n_stats = rc;
+		rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
+		if (rc >= 0)
+			info.n_priv_flags = rc;
 	} else {
 		/* code path for obsolete hooks */
 
@@ -881,6 +884,33 @@ static int ethtool_set_flags(struct net_device *dev, char __user *useraddr)
 	return dev->ethtool_ops->set_flags(dev, edata.data);
 }
 
+static int ethtool_get_priv_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata = { ETHTOOL_GPFLAGS };
+
+	if (!dev->ethtool_ops->get_priv_flags)
+		return -EOPNOTSUPP;
+
+	edata.data = dev->ethtool_ops->get_priv_flags(dev);
+
+	if (copy_to_user(useraddr, &edata, sizeof(edata)))
+		return -EFAULT;
+	return 0;
+}
+
+static int ethtool_set_priv_flags(struct net_device *dev, char __user *useraddr)
+{
+	struct ethtool_value edata;
+
+	if (!dev->ethtool_ops->set_priv_flags)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&edata, useraddr, sizeof(edata)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->set_priv_flags(dev, edata.data);
+}
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct ifreq *ifr)
@@ -915,6 +945,8 @@ int dev_ethtool(struct ifreq *ifr)
 	case ETHTOOL_GPERMADDR:
 	case ETHTOOL_GUFO:
 	case ETHTOOL_GGSO:
+	case ETHTOOL_GFLAGS:
+	case ETHTOOL_GPFLAGS:
 		break;
 	default:
 		if (!capable(CAP_NET_ADMIN))
@@ -1039,6 +1071,12 @@ int dev_ethtool(struct ifreq *ifr)
 	case ETHTOOL_SFLAGS:
 		rc = ethtool_set_flags(dev, useraddr);
 		break;
+	case ETHTOOL_GPFLAGS:
+		rc = ethtool_get_priv_flags(dev, useraddr);
+		break;
+	case ETHTOOL_SPFLAGS:
+		rc = ethtool_set_priv_flags(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}

[PATCH 4/4] ethtool: internal simplification

[Jeff Garzik]

commit 758522226b3e7758a84e721998bce8f63855caca
Author: Jeff Garzik <jeff@garzik.org>
Date:   Fri Aug 10 16:17:09 2007 -0400

    [ETHTOOL] Internal cleanup of ethtool_value-related handlers
    
    Several get/set functions can be handled by a passing the ethtool_op
    function pointer directly to a generic function.  This permits deletion
    of a fair bit of redundant code.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 net/core/ethtool.c |  199 ++++++++++-------------------------------------------
 1 file changed, 39 insertions(+), 160 deletions(-)

758522226b3e7758a84e721998bce8f63855caca
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f721e38..4559200 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -272,34 +272,6 @@ static int ethtool_set_wol(struct net_device *dev, char __user *useraddr)
 	return dev->ethtool_ops->set_wol(dev, &wol);
 }
 
-static int ethtool_get_msglevel(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GMSGLVL };
-
-	if (!dev->ethtool_ops->get_msglevel)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_msglevel(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
-static int ethtool_set_msglevel(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata;
-
-	if (!dev->ethtool_ops->set_msglevel)
-		return -EOPNOTSUPP;
-
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	dev->ethtool_ops->set_msglevel(dev, edata.data);
-	return 0;
-}
-
 static int ethtool_nway_reset(struct net_device *dev)
 {
 	if (!dev->ethtool_ops->nway_reset)
@@ -308,20 +280,6 @@ static int ethtool_nway_reset(struct net_device *dev)
 	return dev->ethtool_ops->nway_reset(dev);
 }
 
-static int ethtool_get_link(struct net_device *dev, void __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GLINK };
-
-	if (!dev->ethtool_ops->get_link)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_link(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
 static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
@@ -489,48 +447,6 @@ static int ethtool_set_pauseparam(struct net_device *dev, void __user *useraddr)
 	return dev->ethtool_ops->set_pauseparam(dev, &pauseparam);
 }
 
-static int ethtool_get_rx_csum(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GRXCSUM };
-
-	if (!dev->ethtool_ops->get_rx_csum)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_rx_csum(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
-static int ethtool_set_rx_csum(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata;
-
-	if (!dev->ethtool_ops->set_rx_csum)
-		return -EOPNOTSUPP;
-
-	if (copy_from_user(&edata, useraddr, sizeof(edata)))
-		return -EFAULT;
-
-	dev->ethtool_ops->set_rx_csum(dev, edata.data);
-	return 0;
-}
-
-static int ethtool_get_tx_csum(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GTXCSUM };
-
-	if (!dev->ethtool_ops->get_tx_csum)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_tx_csum(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
 static int __ethtool_set_sg(struct net_device *dev, u32 data)
 {
 	int err;
@@ -569,20 +485,6 @@ static int ethtool_set_tx_csum(struct net_device *dev, char __user *useraddr)
 	return dev->ethtool_ops->set_tx_csum(dev, edata.data);
 }
 
-static int ethtool_get_sg(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GSG };
-
-	if (!dev->ethtool_ops->get_sg)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_sg(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
 static int ethtool_set_sg(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_value edata;
@@ -600,20 +502,6 @@ static int ethtool_set_sg(struct net_device *dev, char __user *useraddr)
 	return __ethtool_set_sg(dev, edata.data);
 }
 
-static int ethtool_get_tso(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GTSO };
-
-	if (!dev->ethtool_ops->get_tso)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_tso(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
-	return 0;
-}
-
 static int ethtool_set_tso(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_value edata;
@@ -630,18 +518,6 @@ static int ethtool_set_tso(struct net_device *dev, char __user *useraddr)
 	return dev->ethtool_ops->set_tso(dev, edata.data);
 }
 
-static int ethtool_get_ufo(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GUFO };
-
-	if (!dev->ethtool_ops->get_ufo)
-		return -EOPNOTSUPP;
-	edata.data = dev->ethtool_ops->get_ufo(dev);
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		 return -EFAULT;
-	return 0;
-}
-
 static int ethtool_set_ufo(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_value edata;
@@ -857,58 +733,48 @@ static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
 	return 0;
 }
 
-static int ethtool_get_flags(struct net_device *dev, char __user *useraddr)
+static int ethtool_get_value(struct net_device *dev, char __user *useraddr,
+			     u32 cmd, u32 (*actor)(struct net_device *))
 {
-	struct ethtool_value edata = { ETHTOOL_GFLAGS };
+	struct ethtool_value edata = { cmd };
 
-	if (!dev->ethtool_ops->get_flags)
+	if (!actor)
 		return -EOPNOTSUPP;
 
-	edata.data = dev->ethtool_ops->get_flags(dev);
+	edata.data = actor(dev);
 
 	if (copy_to_user(useraddr, &edata, sizeof(edata)))
 		return -EFAULT;
 	return 0;
 }
 
-static int ethtool_set_flags(struct net_device *dev, char __user *useraddr)
+static int ethtool_set_value_void(struct net_device *dev, char __user *useraddr,
+			     void (*actor)(struct net_device *, u32))
 {
 	struct ethtool_value edata;
 
-	if (!dev->ethtool_ops->set_flags)
+	if (!actor)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&edata, useraddr, sizeof(edata)))
 		return -EFAULT;
 
-	return dev->ethtool_ops->set_flags(dev, edata.data);
-}
-
-static int ethtool_get_priv_flags(struct net_device *dev, char __user *useraddr)
-{
-	struct ethtool_value edata = { ETHTOOL_GPFLAGS };
-
-	if (!dev->ethtool_ops->get_priv_flags)
-		return -EOPNOTSUPP;
-
-	edata.data = dev->ethtool_ops->get_priv_flags(dev);
-
-	if (copy_to_user(useraddr, &edata, sizeof(edata)))
-		return -EFAULT;
+	actor(dev, edata.data);
 	return 0;
 }
 
-static int ethtool_set_priv_flags(struct net_device *dev, char __user *useraddr)
+static int ethtool_set_value(struct net_device *dev, char __user *useraddr,
+			     int (*actor)(struct net_device *, u32))
 {
 	struct ethtool_value edata;
 
-	if (!dev->ethtool_ops->set_priv_flags)
+	if (!actor)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&edata, useraddr, sizeof(edata)))
 		return -EFAULT;
 
-	return dev->ethtool_ops->set_priv_flags(dev, edata.data);
+	return actor(dev, edata.data);
 }
 
 /* The main entry point in this file.  Called from net/core/dev.c */
@@ -979,16 +845,19 @@ int dev_ethtool(struct ifreq *ifr)
 		rc = ethtool_set_wol(dev, useraddr);
 		break;
 	case ETHTOOL_GMSGLVL:
-		rc = ethtool_get_msglevel(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_msglevel);
 		break;
 	case ETHTOOL_SMSGLVL:
-		rc = ethtool_set_msglevel(dev, useraddr);
+		rc = ethtool_set_value_void(dev, useraddr,
+				       dev->ethtool_ops->set_msglevel);
 		break;
 	case ETHTOOL_NWAY_RST:
 		rc = ethtool_nway_reset(dev);
 		break;
 	case ETHTOOL_GLINK:
-		rc = ethtool_get_link(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_link);
 		break;
 	case ETHTOOL_GEEPROM:
 		rc = ethtool_get_eeprom(dev, useraddr);
@@ -1015,25 +884,30 @@ int dev_ethtool(struct ifreq *ifr)
 		rc = ethtool_set_pauseparam(dev, useraddr);
 		break;
 	case ETHTOOL_GRXCSUM:
-		rc = ethtool_get_rx_csum(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_rx_csum);
 		break;
 	case ETHTOOL_SRXCSUM:
-		rc = ethtool_set_rx_csum(dev, useraddr);
+		rc = ethtool_set_value(dev, useraddr,
+				       dev->ethtool_ops->set_rx_csum);
 		break;
 	case ETHTOOL_GTXCSUM:
-		rc = ethtool_get_tx_csum(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_tx_csum);
 		break;
 	case ETHTOOL_STXCSUM:
 		rc = ethtool_set_tx_csum(dev, useraddr);
 		break;
 	case ETHTOOL_GSG:
-		rc = ethtool_get_sg(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_sg);
 		break;
 	case ETHTOOL_SSG:
 		rc = ethtool_set_sg(dev, useraddr);
 		break;
 	case ETHTOOL_GTSO:
-		rc = ethtool_get_tso(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_tso);
 		break;
 	case ETHTOOL_STSO:
 		rc = ethtool_set_tso(dev, useraddr);
@@ -1054,7 +928,8 @@ int dev_ethtool(struct ifreq *ifr)
 		rc = ethtool_get_perm_addr(dev, useraddr);
 		break;
 	case ETHTOOL_GUFO:
-		rc = ethtool_get_ufo(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_ufo);
 		break;
 	case ETHTOOL_SUFO:
 		rc = ethtool_set_ufo(dev, useraddr);
@@ -1066,16 +941,20 @@ int dev_ethtool(struct ifreq *ifr)
 		rc = ethtool_set_gso(dev, useraddr);
 		break;
 	case ETHTOOL_GFLAGS:
-		rc = ethtool_get_flags(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_flags);
 		break;
 	case ETHTOOL_SFLAGS:
-		rc = ethtool_set_flags(dev, useraddr);
+		rc = ethtool_set_value(dev, useraddr,
+				       dev->ethtool_ops->set_flags);
 		break;
 	case ETHTOOL_GPFLAGS:
-		rc = ethtool_get_priv_flags(dev, useraddr);
+		rc = ethtool_get_value(dev, useraddr, ethcmd,
+				       dev->ethtool_ops->get_priv_flags);
 		break;
 	case ETHTOOL_SPFLAGS:
-		rc = ethtool_set_priv_flags(dev, useraddr);
+		rc = ethtool_set_value(dev, useraddr,
+				       dev->ethtool_ops->set_priv_flags);
 		break;
 	default:
 		rc = -EOPNOTSUPP;

Driver writer hints (was [PATCH 3/4] Add ETHTOOL_[GS]PFLAGS sub-ioctls)

[Jeff Garzik]

Jeff Garzik wrote:
> commit 4901236cec047029b970261b95e47d6be60f523e
> Author: Jeff Garzik <jeff@garzik.org>
> Date:   Fri Aug 10 15:52:06 2007 -0400
> 
>     [ETHTOOL] Introduce ->{get,set}_priv_flags, ETHTOOL_[GS]PFLAGS
>     
>     Signed-off-by: Jeff Garzik <jeff@garzik.org>
> 
>  include/linux/ethtool.h |    8 +++++++-
>  net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)


This change permits driver writers to allow userspace to enable/disable 
driver-specific boolean options on a per-interface basis.  This is best 
illustrated by describing how userland uses this:

(this is very similar to how the get-stats and self-test operations 
currently work)

1) Userland issues ETHTOOL_GDRVINFO, to obtain the n_priv_flags value.

2) Userland issues ETHTOOL_GSTRINGS (ETH_SS_PRIV_FLAGS) to obtain an 
array of strings.  Each element in this array maps to a bit in the list 
of driver-private flags.  Array element #0 is the tag (name) for bit #0. 
  Array element #5 is the tag for bit #5.  etc.

If we are getting (retrieving) flags:

	3) Userland issues ETHTOOL_GPFLAGS, to obtain a 32-bit bitmap

	4) Userland prints out a tag returned from ETHTOOL_GSTRINGS
	   for each bit set to one in the bitmap.  If a bit is set,
	   but there is no string to describe it, that bit is ignored.
	   (i.e. a list of 5 strings is returned, but bit 24 is set)

If we are setting flags:

	3) Userland issues ETHTOOL_GPFLAGS, to obtain 32-bit bitmap

	4) Userland parses command line, which has a series of strings
	   in the format of "+option" (set flag) and "-option" (clear
	   flag).

	5) Userland sets and clears bits in the 32-bit bitmap, by
	   matching the command line-supplied data with strings returned
	   by ETHTOOL_GSTRINGS.

	6) If the bitmap has changed, send it back to the driver using
	   ETHTOOL_SPFLAGS.

In this way, you can see that the actual bit numbers are irrelevant, and 
need not be a fixed part of the ABI.  Everything is named-based.

This name-based stuff only applies to the private flags.  The 
ETHTOOL_[GS]FLAGS operations /do/ fix the bit numbers in stone, in the 
ABI.  There is no name list associated with ETHTOOL_[GS]FLAGS].  The 
userland interface does, however, use names rather than bit numbers, for 
ease of use.

Another attribute is worth pointing out at this point:  ETHTOOL_SFLAGS 
and ETHTOOL_SPFLAGS both permit atomic retrieval/setting of groups of 
features all at once.  IMO this is a nice addition to ethtool as well, 
where previously you had to issue separate ioctls for each feature, 
which might result in repeated NIC resets [depending on driver 
implementation].

Get-flags and get-priv-flags are unpriveleged operations.  Set-flags and 
set-priv-flags require CAP_NET_ADMIN like other ethtool sub-ioctls.

Drivers should return -EINVAL if userland attempts to set an invariant 
(read-only) flag to something other than its constant value.

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Jeff Garzik]

All this is currently checked into the 'eflags' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git

But when everybody is happy with it, IMO we should get it into 
net-2.6.24.git, as it enables LRO.

	Jeff

Re: Driver writer hints (was [PATCH 3/4] Add ETHTOOL_[GS]PFLAGS sub-ioctls)

[Rick Jones]

> If we are getting (retrieving) flags:
> 
>     3) Userland issues ETHTOOL_GPFLAGS, to obtain a 32-bit bitmap
> 
>     4) Userland prints out a tag returned from ETHTOOL_GSTRINGS
>        for each bit set to one in the bitmap.  If a bit is set,
>        but there is no string to describe it, that bit is ignored.
>        (i.e. a list of 5 strings is returned, but bit 24 is set)

Is that to enable "hidden" bits?  If not I'd think that emitting some 
sort of "UNKNOWN_FLAG" might help flush-out little oopses like 
forgetting a string.

rick jones

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Jeff Garzik]

Jeff Garzik wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 4a616d7..559a4dc 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -341,6 +341,7 @@ struct net_device
>  #define NETIF_F_GSO		2048	/* Enable software GSO. */
>  #define NETIF_F_LLTX		4096	/* LockLess TX */
>  #define NETIF_F_MULTI_QUEUE	16384	/* Has multiple TX/RX queues */
> +#define NETIF_F_LRO		32768	/* large receive offload */
>  
>  	/* Segmentation offload features */
>  #define NETIF_F_GSO_SHIFT	16
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 2ab0a60..6e8563e 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -109,6 +109,32 @@ int ethtool_op_set_ufo(struct net_device *dev, u32 data)
>  	return 0;
>  }
>  
> +/* the following list of flags are the same as their associated
> + * NETIF_F_xxx values in include/linux/netdevice.h
> + */
> +static const u32 flags_dup_features =
> +	ETH_FLAG_LRO;
> +
> +u32 ethtool_op_get_flags(struct net_device *dev)
> +{
> +	/* in the future, this function will probably contain additional
> +	 * handling for flags which are not so easily handled
> +	 * by a simple masking operation
> +	 */
> +	
> +	return dev->features & flags_dup_features;
> +}
> +
> +int ethtool_op_set_flags(struct net_device *dev, u32 data)
> +{
> +	if (data & ETH_FLAG_LRO)
> +		dev->features |= NETIF_F_LRO;
> +	else
> +		dev->features &= ~NETIF_F_LRO;
> +

And, a side discussion:

This patch copies Auke in adding NETIF_F_LRO.  Is that just for 
temporary merging, or does the net core really not touch it at all?

Because, logically, if NETIF_F_LRO exists nowhere else but this patch, 
we should not add it to dev->features.  LRO knowledge can be contained 
entirely within the driver, if the net core never tests NETIF_F_LRO.

I haven't reviewed the other NETIF_F_XXX flags, but, that logic can be 
applied to any other NETIF_F_XXX flag:  if the net stack isn't using it, 
it's a piece of information specific to that driver.

	Jeff

Re: Driver writer hints (was [PATCH 3/4] Add ETHTOOL_[GS]PFLAGS sub-ioctls)

[Jeff Garzik]

Rick Jones wrote:
>> If we are getting (retrieving) flags:
>>
>>     3) Userland issues ETHTOOL_GPFLAGS, to obtain a 32-bit bitmap
>>
>>     4) Userland prints out a tag returned from ETHTOOL_GSTRINGS
>>        for each bit set to one in the bitmap.  If a bit is set,
>>        but there is no string to describe it, that bit is ignored.
>>        (i.e. a list of 5 strings is returned, but bit 24 is set)
> 
> Is that to enable "hidden" bits?  If not I'd think that emitting some 
> sort of "UNKNOWN_FLAG" might help flush-out little oopses like 
> forgetting a string.

No purpose other than attempting to define sane edge case behavior.

The bitmap interface is completely invisible to the user (who sees UTF8 
strings on the command line), and given the interface I felt the easiest 
thing to do would be to define an "ignore any garbage" default policy.

Due to the way the interface is designed, each userland user must be 
aware of the precise number of valid bits in the bitmap /anyway/, thus 
making clipping/ignoring garbage bits almost a trivial side effect.

	Jeff

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Ben Greear]

Jeff Garzik wrote:

> This patch copies Auke in adding NETIF_F_LRO.  Is that just for 
> temporary merging, or does the net core really not touch it at all?
> 
> Because, logically, if NETIF_F_LRO exists nowhere else but this patch, 
> we should not add it to dev->features.  LRO knowledge can be contained 
> entirely within the driver, if the net core never tests NETIF_F_LRO.
> 
> I haven't reviewed the other NETIF_F_XXX flags, but, that logic can be 
> applied to any other NETIF_F_XXX flag:  if the net stack isn't using it, 
> it's a piece of information specific to that driver.

I believe LRO is going to have to be disabled for routing/bridging,
so the stack will probably need to become aware of it at some point...

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 10 Aug 2007 14:11:24 -0700

> Jeff Garzik wrote:
> 
> > This patch copies Auke in adding NETIF_F_LRO.  Is that just for 
> > temporary merging, or does the net core really not touch it at all?
> > 
> > Because, logically, if NETIF_F_LRO exists nowhere else but this patch, 
> > we should not add it to dev->features.  LRO knowledge can be contained 
> > entirely within the driver, if the net core never tests NETIF_F_LRO.
> > 
> > I haven't reviewed the other NETIF_F_XXX flags, but, that logic can be 
> > applied to any other NETIF_F_XXX flag:  if the net stack isn't using it, 
> > it's a piece of information specific to that driver.
> 
> I believe LRO is going to have to be disabled for routing/bridging,
> so the stack will probably need to become aware of it at some point...

The packet will be GSO'd on output I believe, so it won't
break anything.

Alternatively, we could make the driver only LRO accumulate if the
packet is unicast and matches one of the MAC's programmed into the
chip.

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Ben Greear]

David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>

>> I believe LRO is going to have to be disabled for routing/bridging,
>> so the stack will probably need to become aware of it at some point...
> 
> The packet will be GSO'd on output I believe, so it won't
> break anything.
> 
> Alternatively, we could make the driver only LRO accumulate if the
> packet is unicast and matches one of the MAC's programmed into the
> chip.

I think even this would fail if you are doing something clever with
NAT or other iptables stuff.  Probably we're going to have to put this
in the hands of the users..who hopefully can determine whether they
can allow LRO or not...

For GSO on output, is there a generic fallback for any driver that
does not specifically implement GSO?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 10 Aug 2007 15:40:02 -0700

> For GSO on output, is there a generic fallback for any driver that
> does not specifically implement GSO?

Absolutely, in fact that's mainly what it's there for.

I don't think there is any issue.  The knob is there via
ethtool for people who really want to disable it.

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Rick Jones]

David Miller wrote:
> From: Ben Greear <greearb@candelatech.com>
> Date: Fri, 10 Aug 2007 15:40:02 -0700
> 
> 
>>For GSO on output, is there a generic fallback for any driver that
>>does not specifically implement GSO?
> 
> 
> Absolutely, in fact that's mainly what it's there for.
> 
> I don't think there is any issue.  The knob is there via
> ethtool for people who really want to disable it.

Just to be paranoid (who me?) we are then at a point where what happened 
a couple months ago with forwarding between 10G and IPoIB won't happen 
again - where things failed because a 10G NIC had LRO enabled by default?

rick jones

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[Kok, Auke]

Rick Jones wrote:
> David Miller wrote:
>> From: Ben Greear <greearb@candelatech.com>
>> Date: Fri, 10 Aug 2007 15:40:02 -0700
>>
>>
>>> For GSO on output, is there a generic fallback for any driver that
>>> does not specifically implement GSO?
>>
>> Absolutely, in fact that's mainly what it's there for.
>>
>> I don't think there is any issue.  The knob is there via
>> ethtool for people who really want to disable it.
> 
> Just to be paranoid (who me?) we are then at a point where what happened 
> a couple months ago with forwarding between 10G and IPoIB won't happen 
> again - where things failed because a 10G NIC had LRO enabled by default?

we still have the NETIF_F_LRO flag which Jeff will keep around. Perhaps the 
IPoIB code can force this to _off_ when setting it up? (or at least warn about it).

Auke

Re: [PATCH 1/4] Add ETHTOOL_[GS]FLAGS sub-ioctls

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 10 Aug 2007 16:56:17 -0400

> All this is currently checked into the 'eflags' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/netdev-2.6.git
> 
> But when everybody is happy with it, IMO we should get it into 
> net-2.6.24.git, as it enables LRO.

I've backed out the ETHTOOL LRO stuff from Auke, and applied your
patches to net-2.6.24

We can get rid of that NETIF_F_LRO thing, since as you observed
it is indeed superfluous.