[RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

Subsystems: the rest

STALE5526d

15 messages, 4 authors, 2011-09-03 · open the first message on its own page

[RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Carolyn Wyborny <hidden>
Date: 2011-09-02 20:51:13

This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
---
 ethtool.8.in |   16 +++++++++
 ethtool.c    |   98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 113 insertions(+), 1 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index 7a0bd43..ebcb233 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -134,6 +134,13 @@ ethtool \- query or control network driver and hardware settings
 .B2 rx on off
 .B2 tx on off
 .HP
+.B ethtool \-b|\-\-show\-priv\-flags
+.I ethX
+.HP
+.B ethtool \-B|\-\-set\-priv\-flags
+.I ethX
+.BN bit
+.HP
 .B ethtool \-c|\-\-show\-coalesce
 .I ethX
 .HP
@@ -342,6 +349,15 @@ Specifies whether RX pause should be enabled.
 .A2 tx on off
 Specifies whether TX pause should be enabled.
 .TP
+.B \-b \-\-show\-priv_flags
+Queries the specified network device for private flags set.
+.TP
+.B \-B \-\-set\-priv_flags
+Changes the private flags of the specified network device.
+.TP
+.BI bit \ N
+Sets the specific bit number in the private flagsg field.
+.TP
 .B \-c \-\-show\-coalesce
 Queries the specified network device for coalescing information.
 .TP
diff --git a/ethtool.c b/ethtool.c
index 943dfb7..4702d04 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -99,6 +99,8 @@ static int do_flash(int fd, struct ifreq *ifr);
 static int do_permaddr(int fd, struct ifreq *ifr);
 static int do_getfwdump(int fd, struct ifreq *ifr);
 static int do_setfwdump(int fd, struct ifreq *ifr);
+static int do_gpflags(int fd, struct ifreq *ifr);
+static int do_spflags(int fd, struct ifreq *ifr);
 
 static int send_ioctl(int fd, struct ifreq *ifr);
 
@@ -133,6 +135,8 @@ static enum {
 	MODE_PERMADDR,
 	MODE_SET_DUMP,
 	MODE_GET_DUMP,
+	MODE_GET_PFLAGS,
+	MODE_SET_PFLAGS,
 } mode = MODE_GSET;
 
 static struct option {
@@ -157,6 +161,9 @@ static struct option {
       "		[ autoneg on|off ]\n"
       "		[ rx on|off ]\n"
       "		[ tx on|off ]\n" },
+    { "-b", "--show-pflags", MODE_GET_PFLAGS, "Show private flags set." },
+    { "-B", "--set-pflags", MODE_SET_PFLAGS, "Set private flags.",
+		"		[value N | bit N]\n" },
     { "-c", "--show-coalesce", MODE_GCOALESCE, "Show coalesce options" },
     { "-C", "--coalesce", MODE_SCOALESCE, "Set coalesce options",
 		"		[adaptive-rx on|off]\n"
@@ -405,6 +412,11 @@ static int rx_class_rule_del = -1;
 static int rx_class_rule_added = 0;
 static struct ethtool_rx_flow_spec rx_rule_fs;
 
+static u32 priv_flags;
+static int priv_flags_changed;
+static u32 priv_flags_val_wanted;
+static u8 priv_flags_bit_wanted;
+
 static enum {
 	ONLINE=0,
 	OFFLINE,
@@ -552,6 +564,10 @@ static struct cmdline_info cmdline_msglvl[] = {
 	  NETIF_MSG_WOL, &msglvl_mask },
 };
 
+static struct cmdline_info cmdline_pflags[] = {
+	{ "value", CMDL_U32, &priv_flags_val_wanted, &priv_flags },
+};
+
 static long long
 get_int_range(char *str, int base, long long min, long long max)
 {
@@ -800,7 +816,9 @@ static void parse_cmdline(int argc, char **argp)
 			    (mode == MODE_FLASHDEV) ||
 			    (mode == MODE_PERMADDR) ||
 			    (mode == MODE_SET_DUMP) ||
-			    (mode == MODE_GET_DUMP)) {
+			    (mode == MODE_GET_DUMP) ||
+			    (mode == MODE_GET_PFLAGS) ||
+			    (mode == MODE_SET_PFLAGS)) {
 				devname = argp[i];
 				break;
 			}
@@ -884,6 +902,28 @@ static void parse_cmdline(int argc, char **argp)
 				i = argc;
 				break;
 			}
+			if (mode == MODE_SET_PFLAGS) {
+				if (!strcmp(argp[i], "value")) {
+					priv_flags_changed = 1;
+					i++;
+					if (i >= argc)
+						exit_bad_args();
+					priv_flags_val_wanted =
+						get_u32(argp[i], 0);
+				} else if (!strcmp(argp[i], "bit")) {
+					priv_flags_changed = 1;
+					i++;
+					if (i >= argc)
+						exit_bad_args();
+					priv_flags_bit_wanted =
+						get_int(argp[i], 0);
+					priv_flags_val_wanted =
+						(1 >> priv_flags_bit_wanted);
+				} else
+					exit_bad_args();
+				i = argc;
+				break;
+			}
 			if (mode == MODE_SCLSRULE) {
 				if (!strcmp(argp[i], "flow-type")) {
 					i += 1;
@@ -1957,6 +1997,10 @@ static int doit(void)
 		return do_getfwdump(fd, &ifr);
 	} else if (mode == MODE_SET_DUMP) {
 		return do_setfwdump(fd, &ifr);
+	} else if (mode == MODE_GET_PFLAGS) {
+		return do_gpflags(fd, &ifr);
+	} else if (mode == MODE_SET_PFLAGS) {
+		return do_spflags(fd, &ifr);
 	}
 
 	return 69;
@@ -3346,6 +3390,58 @@ static int do_setfwdump(int fd, struct ifreq *ifr)
 	return 0;
 }
 
+static int do_gpflags(int fd, struct ifreq *ifr)
+{
+	int err;
+	struct ethtool_value eval;
+
+	eval.cmd = ETHTOOL_GPFLAGS;
+	ifr->ifr_data = (caddr_t)&eval;
+	err = send_ioctl(fd, ifr);
+	if (err) {
+		perror("Cannot get device private flags");
+	} else {
+		fprintf(stdout, "	Current flags set: 0x%08x (%d)\n"
+			"			       ",
+			eval.data, eval.data);
+		print_flags(cmdline_pflags, ARRAY_SIZE(cmdline_pflags),
+			    eval.data);
+		fprintf(stdout, "\n");
+	}
+	return err;
+}
+
+static int do_spflags(int fd, struct ifreq *ifr)
+{
+	struct ethtool_value eval;
+	int err, changed = 0;
+	eval.cmd = ETHTOOL_GPFLAGS;
+	ifr->ifr_data = (caddr_t)&eval;
+	err = send_ioctl(fd, ifr);
+	if (err) {
+		perror("Cannot get device private flags");
+		return err;
+	}
+
+	priv_flags = eval.data;
+	do_generic_set(cmdline_pflags, ARRAY_SIZE(cmdline_pflags),
+		&changed);
+	if (!changed) {
+		fprintf(stderr, "No private flags have changed, aborting\n");
+			return 1;
+	} else {
+		eval.cmd = ETHTOOL_SPFLAGS;
+		eval.data = priv_flags_val_wanted;
+		ifr->ifr_data = (caddr_t)&eval;
+		err = send_ioctl(fd, ifr);
+		if (err) {
+			perror("Cannot set device private flags.\n");
+			return err;
+		}
+		return 0;
+	}
+}
+
 static int send_ioctl(int fd, struct ifreq *ifr)
 {
 	return ioctl(fd, SIOCETHTOOL, ifr);
-- 
1.7.4.4

[RFC, 2/2] igb: Implementation of ethtool priv_flags for igb driver.

From: Carolyn Wyborny <hidden>
Date: 2011-09-02 20:51:13

This patch adds igb driver support for the ethtool private flags
interface. Two features are initially configured for private flags
support as an example for use with the implementation in ethtoool
application.

Signed-off-by: Carolyn Wyborny <redacted>
---
 drivers/net/ethernet/intel/igb/igb.h         |    2 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   28 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igb/igb_main.c    |    1 +
 3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index bb47ed1..a8be3eb 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -360,6 +360,7 @@ struct igb_adapter {
 	u32 rss_queues;
 	u32 wvbr;
 	int node;
+	u32 pflags;
 };
 
 #define IGB_FLAG_HAS_MSI           (1 << 0)
@@ -367,6 +368,7 @@ struct igb_adapter {
 #define IGB_FLAG_QUAD_PORT_A       (1 << 2)
 #define IGB_FLAG_QUEUE_PAIRS       (1 << 3)
 #define IGB_FLAG_DMAC              (1 << 4)
+#define IGB_FLAG_EEE               (1 << 5)
 
 /* DMA Coalescing defines */
 #define IGB_MIN_TXPBSIZE           20408
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 64fb4ef..3a251c7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -696,6 +696,9 @@ static void igb_get_drvinfo(struct net_device *netdev,
 	drvinfo->testinfo_len = IGB_TEST_LEN;
 	drvinfo->regdump_len = igb_get_regs_len(netdev);
 	drvinfo->eedump_len = igb_get_eeprom_len(netdev);
+#ifdef ETHTOOL_GPFLAGS
+	drvinfo->n_priv_flags = 2;
+#endif
 }
 
 static void igb_get_ringparam(struct net_device *netdev,
@@ -2171,6 +2174,29 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	}
 }
 
+static int igb_set_pflags(struct net_device *netdev, u32 data)
+{
+	u32 supported_flags = IGB_FLAG_EEE;
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	if (data & supported_flags) {
+		adapter->pflags = data;
+	} else {
+		printk(KERN_INFO, "set_pflags:flag not supported..");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static u32 igb_get_pflags(struct net_device *netdev)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	return adapter->pflags;
+
+}
+
 static const struct ethtool_ops igb_ethtool_ops = {
 	.get_settings           = igb_get_settings,
 	.set_settings           = igb_set_settings,
@@ -2197,6 +2223,8 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.get_ethtool_stats      = igb_get_ethtool_stats,
 	.get_coalesce           = igb_get_coalesce,
 	.set_coalesce           = igb_set_coalesce,
+	.get_priv_flags         = igb_get_pflags,
+	.set_priv_flags         = igb_set_pflags,
 };
 
 void igb_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 289861c..a534f32 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2175,6 +2175,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 	switch (hw->mac.type) {
 	case e1000_i350:
 		igb_set_eee_i350(hw);
+		adapter->pflags |= IGB_FLAG_EEE;
 		break;
 	default:
 		break;
-- 
1.7.4.4

Re: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: David Miller <davem@davemloft.net>
Date: 2011-09-02 20:55:32

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.

RE: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Wyborny, Carolyn <hidden>
Date: 2011-09-02 21:11:21

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the implementation suggestion altogether?  I can send an updated feature implementation for the use case using DMA Coalescing if that will help.

Thanks,

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation

Re: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Michał Mirosław <hidden>
Date: 2011-09-02 21:17:37

2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the implementation suggestion altogether?  I can send an updated feature implementation for the use case using DMA Coalescing if that will help.
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags without
changing anything on userspace side.

Best Regards,
Michał Mirosław

Re: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Michał Mirosław <hidden>
Date: 2011-09-02 21:22:36

W dniu 2 września 2011 23:17 użytkownik Michał Mirosław
[off-list ref] napisał:
2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the implementation suggestion altogether?  I can send an updated feature implementation for the use case using DMA Coalescing if that will help.
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags without
changing anything on userspace side.
BTW, After pending Intel drivers get converted to ndo_set_features and
netdev->features get extended to 64 bits, it would also be possible to
use some of the unused bits there for device/driver-private flags
almost "for free".

Best Regards,
Michał Mirosław

Re: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Ben Hutchings <hidden>
Date: 2011-09-02 21:23:22

On Fri, 2011-09-02 at 16:55 -0400, David Miller wrote:
From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
I agree.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Ben Hutchings <hidden>
Date: 2011-09-02 21:27:14

On Fri, 2011-09-02 at 13:50 -0700, Carolyn Wyborny wrote:
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.
[...]

Private flags are supposed to be named (string set ETH_SS_PRIV_FLAGS).
ethtool should only support getting and setting flags by name, not
number.  That way people can actually remember what the flags do and
their scripts won't break when the driver changes flag numbers.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Wyborny, Carolyn <hidden>
Date: 2011-09-02 21:29:24

-----Original Message-----
From: Michał Mirosław [mailto:mirqus@gmail.com]
Sent: Friday, September 02, 2011 2:22 PM
To: Wyborny, Carolyn
Cc: David Miller; bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

W dniu 2 września 2011 23:17 użytkownik Michał Mirosław
[off-list ref] napisał:
quoted
2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface
for
quoted
quoted
quoted
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the
implementation suggestion altogether?  I can send an updated feature
implementation for the use case using DMA Coalescing if that will help.
quoted
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags without
changing anything on userspace side.
BTW, After pending Intel drivers get converted to ndo_set_features and
netdev->features get extended to 64 bits, it would also be possible to
use some of the unused bits there for device/driver-private flags
almost "for free".

Best Regards,
Michał Mirosław
That seems reasonable as then there would be room for them, but is it going to be OK to use those unused bits or are they going to be intended for generic features and not device specific ones?  

Is the intent then to not ever use the priv_flags partial implementation?  

Thanks,

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation

Re: [RFC, 2/2] igb: Implementation of ethtool priv_flags for igb driver.

From: Ben Hutchings <hidden>
Date: 2011-09-02 21:31:39

On Fri, 2011-09-02 at 13:50 -0700, Carolyn Wyborny wrote:
quoted hunk
This patch adds igb driver support for the ethtool private flags
interface. Two features are initially configured for private flags
support as an example for use with the implementation in ethtoool
application.

Signed-off-by: Carolyn Wyborny <redacted>
---
 drivers/net/ethernet/intel/igb/igb.h         |    2 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   28 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igb/igb_main.c    |    1 +
 3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index bb47ed1..a8be3eb 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -360,6 +360,7 @@ struct igb_adapter {
 	u32 rss_queues;
 	u32 wvbr;
 	int node;
+	u32 pflags;
 };
 
 #define IGB_FLAG_HAS_MSI           (1 << 0)
@@ -367,6 +368,7 @@ struct igb_adapter {
 #define IGB_FLAG_QUAD_PORT_A       (1 << 2)
 #define IGB_FLAG_QUEUE_PAIRS       (1 << 3)
 #define IGB_FLAG_DMAC              (1 << 4)
+#define IGB_FLAG_EEE               (1 << 5)
 
 /* DMA Coalescing defines */
 #define IGB_MIN_TXPBSIZE           20408
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 64fb4ef..3a251c7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -696,6 +696,9 @@ static void igb_get_drvinfo(struct net_device *netdev,
 	drvinfo->testinfo_len = IGB_TEST_LEN;
 	drvinfo->regdump_len = igb_get_regs_len(netdev);
 	drvinfo->eedump_len = igb_get_eeprom_len(netdev);
+#ifdef ETHTOOL_GPFLAGS
That macro is always defined,
+	drvinfo->n_priv_flags = 2;
This is not enough.  You need to support ETH_SS_PRIV_FLAGS in the
get_sset_count and get_strings operations.
quoted hunk
+#endif
 }
 
 static void igb_get_ringparam(struct net_device *netdev,
@@ -2171,6 +2174,29 @@ static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	}
 }
 
+static int igb_set_pflags(struct net_device *netdev, u32 data)
+{
+	u32 supported_flags = IGB_FLAG_EEE;
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	if (data & supported_flags) {
[...]

If the number of flags is 2, then the supported flags must be numbered
(1 << 0) and (1 << 1).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Wyborny, Carolyn <hidden>
Date: 2011-09-02 21:33:26

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Friday, September 02, 2011 2:27 PM
To: Wyborny, Carolyn
Cc: davem@davemloft.net; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

On Fri, 2011-09-02 at 13:50 -0700, Carolyn Wyborny wrote:
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.
[...]

Private flags are supposed to be named (string set ETH_SS_PRIV_FLAGS).
ethtool should only support getting and setting flags by name, not
number.  That way people can actually remember what the flags do and
their scripts won't break when the driver changes flag numbers.

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Ok, makes sense.  Do you want a private flags implementation or do you agree with Michal on extending ETHTOOL_[GS]FEATURES?

Thanks,

Carolyn

Re: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Ben Hutchings <hidden>
Date: 2011-09-02 21:34:35

On Fri, 2011-09-02 at 23:22 +0200, Michał Mirosław wrote:
W dniu 2 września 2011 23:17 użytkownik Michał Mirosław
[off-list ref] napisał:
quoted
2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the implementation suggestion altogether?  I can send an updated feature implementation for the use case using DMA Coalescing if that will help.
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags without
changing anything on userspace side.
BTW, After pending Intel drivers get converted to ndo_set_features and
netdev->features get extended to 64 bits, it would also be possible to
use some of the unused bits there for device/driver-private flags
almost "for free".
I don't really like the idea of mixing common feature flags with
driver-specific flags.  It's likely to lead to confusion if you mix
devices with different drivers in a bridge or a bond.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Wyborny, Carolyn <hidden>
Date: 2011-09-02 21:35:28

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Friday, September 02, 2011 2:35 PM
To: Michał Mirosław
Cc: Wyborny, Carolyn; David Miller; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

On Fri, 2011-09-02 at 23:22 +0200, Michał Mirosław wrote:
quoted
W dniu 2 września 2011 23:17 użytkownik Michał Mirosław
[off-list ref] napisał:
quoted
2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface
for
quoted
quoted
quoted
quoted
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the
implementation suggestion altogether?  I can send an updated feature
implementation for the use case using DMA Coalescing if that will help.
quoted
quoted
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags
without
quoted
quoted
changing anything on userspace side.
BTW, After pending Intel drivers get converted to ndo_set_features and
netdev->features get extended to 64 bits, it would also be possible to
use some of the unused bits there for device/driver-private flags
almost "for free".
I don't really like the idea of mixing common feature flags with
driver-specific flags.  It's likely to lead to confusion if you mix
devices with different drivers in a bridge or a bond.

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Ok, I'll keep working on it per your previous feedback.

Thanks,

Carolyn

RE: [RFC, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Ben Hutchings <hidden>
Date: 2011-09-02 21:36:42

On Fri, 2011-09-02 at 14:33 -0700, Wyborny, Carolyn wrote:
quoted
-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Friday, September 02, 2011 2:27 PM
To: Wyborny, Carolyn
Cc: davem@davemloft.net; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

On Fri, 2011-09-02 at 13:50 -0700, Carolyn Wyborny wrote:
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.
[...]

Private flags are supposed to be named (string set ETH_SS_PRIV_FLAGS).
ethtool should only support getting and setting flags by name, not
number.  That way people can actually remember what the flags do and
their scripts won't break when the driver changes flag numbers.

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Ok, makes sense.  Do you want a private flags implementation or do you
agree with Michal on extending ETHTOOL_[GS]FEATURES?
I'm happy to add support for private flags identified by name.

(And I still haven't worked out with Michal how we're going to handle
the new features interface in ethtool. :-/)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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, 1/2] ethtool: Implement private flags interface for ethtool application.

From: Michał Mirosław <hidden>
Date: 2011-09-03 01:39:59

W dniu 2 września 2011 23:34 użytkownik Ben Hutchings
[off-list ref] napisał:
On Fri, 2011-09-02 at 23:22 +0200, Michał Mirosław wrote:
quoted
W dniu 2 września 2011 23:17 użytkownik Michał Mirosław
[off-list ref] napisał:
quoted
2011/9/2 Wyborny, Carolyn [off-list ref]:
quoted
quoted
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Friday, September 02, 2011 1:55 PM
To: Wyborny, Carolyn
Cc: bhutchings@solarflare.com; netdev@vger.kernel.org
Subject: Re: [RFC, 1/2] ethtool: Implement private flags interface for
ethtool application.

From: Carolyn Wyborny <redacted>
Date: Fri,  2 Sep 2011 13:50:30 -0700
quoted
This patch completes the user space implementation of the private
flags inteface in ethtool. Using -b/-B options.

Signed-off-by: Carolyn Wyborny <redacted>
The only use case you show here is something generic which other
chips have, Energy Efficient Ethernet.

Making an attribute private which is present widely amonst various
networking chips makes no sense at all.

It deserved a generic ethtool flag.
Fair enough on this particular feature, but does that negate the implementation suggestion altogether?  I can send an updated feature implementation for the use case using DMA Coalescing if that will help.
I would rather see this as an extension to ETHTOOL_[GS]FEATURES. Its
semantics allow easy expanding to handle device-private flags without
changing anything on userspace side.
BTW, After pending Intel drivers get converted to ndo_set_features and
netdev->features get extended to 64 bits, it would also be possible to
use some of the unused bits there for device/driver-private flags
almost "for free".
I don't really like the idea of mixing common feature flags with
driver-specific flags.  It's likely to lead to confusion if you mix
devices with different drivers in a bridge or a bond.
I assume that device-specific flags won't ever be included in
vlan_features not be transferred in other ways to bridge or bonding.
If this holds, then it doesn't really matter how the flags are stored
and if they are included in features. I'll make a PoC implementation
to show the idea only after the feature cleanup is done, as otherwise
I would have yet another idea hanging in the queue.

Rough sketch is that there would be some number of bits in features
left after all global ones that drivers would present to userspace by
appending feature names to the string set returned for
ETH_SS_FEATURES. Those names would need to have common prefix like
'priv-' or maybe driver name prepended, and be documented as not
something that shoud be regarded as stable.

Best Regards,
Michał Mirosław
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help