Re: [PATCH] ethtool : Add option -L | --set-common to set common flags.

On Fri, Jan 14, 2011 at 1:19 PM, Ben Hutchings
[off-list ref] wrote:

On Thu, 2011-01-13 at 16:11 -0800, Mahesh Bandewar wrote:

quoted

This patch adds -L | --set-common option to add / remove common flags which
includes loopback flag. The -l | --show-common displays the current values
for these common flags.

Signed-off-by: Mahesh Bandewar <redacted>
---
 ethtool-copy.h |    1 +
 ethtool.8      |   16 ++++++++++
 ethtool.c      |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 75c3ae7..5fd18c7 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h

@@ -309,6 +309,7 @@ struct ethtool_perm_addr {

  * flag differs from the read-only value.
  */
 enum ethtool_flags {
+     ETH_FLAG_LOOPBACK       = (1 << 2),     /* Loopback enable / disable */
      ETH_FLAG_TXVLAN         = (1 << 7),     /* TX VLAN offload enabled */
      ETH_FLAG_RXVLAN         = (1 << 8),     /* RX VLAN offload enabled */
      ETH_FLAG_LRO            = (1 << 15),    /* LRO is enabled */

diff --git a/ethtool.8 b/ethtool.8
index 1760924..cf7128f 100644
--- a/ethtool.8
+++ b/ethtool.8

@@ -174,6 +174,13 @@ ethtool \- Display or change ethernet card settings

 .B2 txvlan on off
 .B2 rxhash on off

+.B ethtool \-l|\-\-show\-common
+.I ethX
+
+.B ethtool \-L|\-\-set\-common
+.I ethX
+.B2 loopback on off
+
 .B ethtool \-p|\-\-identify
 .I ethX
 .RI [ N ]

@@ -406,6 +413,15 @@ Specifies whether TX VLAN acceleration should be enabled

 .A2 rxhash on off
 Specifies whether receive hashing offload should be enabled
 .TP
+.B \-l \-\-show\-common
+Queries the specified ethernet device for common flag settings.
+.TP
+.B \-L \-\-set\-common
+Changes the common parameters of the specified ethernet device.
+.TP
+.A2 loopback on off
+Specifies whether loopback should be enabled.
+.TP

I've just gone through the manual page and changed 'ethernet device' to
'network device' for all generic operations; please follow that.  The
source for the manual page was also renamed to ethtool.8.in as it now
goes through autoconf substitution.

OK. I'll make those changes.

quoted

 .B \-p \-\-identify
 Initiates adapter-specific action intended to enable an operator to
 easily identify the adapter by sight.  Typically this involves

diff --git a/ethtool.c b/ethtool.c
index 63e0ead..1a0c10c 100644
--- a/ethtool.c
+++ b/ethtool.c

[...]

quoted

@@ -1905,6 +1932,13 @@ static int dump_offload(int rx, int tx, int sg, int tso, int ufo, int gso,

      return 0;
 }

+static int dump_common_flags(int loopback)
+{
+     fprintf(stdout, "loopback: %s\n", loopback ? "on" : "off");
+
+     return 0;
+}
+
 static int dump_rxfhash(int fhash, u64 val)
 {
      switch (fhash) {

[...]

quoted

@@ -2219,6 +2257,53 @@ static int do_scoalesce(int fd, struct ifreq *ifr)

      return 0;
 }

+static int do_gcommon(int fd, struct ifreq *ifr)
+{
+     struct ethtool_value eval;
+     int loopback = 0;
+
+     fprintf(stdout, "Common flags for %s:\n", devname);
+
+     eval.cmd = ETHTOOL_GFLAGS;
+     ifr->ifr_data = (caddr_t)&eval;
+     if (ioctl(fd, SIOCETHTOOL, ifr)) {
+             perror("Cannot get device flags");
+     } else {
+             loopback = (eval.data & ETH_FLAG_LOOPBACK) != 0;
+     }
+
+     return dump_common_flags(loopback);

Breaking up a bitmask into a list of flag parameters is fairly
pointless.  I realise do_goffload() and dump_offload() do that but I am
just waiting for Michał Mirosław's changes to offload flags to be
settled before I fix them.

quoted

+}
+
+static int do_scommon(int fd, struct ifreq *ifr)
+{
+     struct ethtool_value eval;
+
+     if (common_flags_mask) {
+             eval.cmd = ETHTOOL_GFLAGS;
+             eval.data = 0;
+             ifr->ifr_data = (caddr_t)&eval;
+             if (ioctl(fd, SIOCETHTOOL, ifr)) {
+                     perror("Cannot get device common flags");
+                     return 1;
+             }
+
+             eval.cmd = ETHTOOL_SFLAGS;
+             eval.data =
+                 ((eval.data & ~(common_flags_mask | off_flags_mask)) |
+                  (common_flags_wanted | off_flags_wanted));

Why should this use off_flags_mask and off_flags_wanted?  They should
both be 0 if this function is called.

That is right! Actually the get (ETHTOOL_GFLAGS) operation confused
me. I thought the values are fetched and *preserved* while setting the
new value. But when looked at it carefully, that is not the case.
Actually why that ioctl() with ETHTOOL_GFLAGS required?

quoted

+             if (ioctl(fd, SIOCETHTOOL, ifr)) {
+                     perror("Cannot set device common flags");
+                     return 1;
+             }
+     } else {
+             fprintf(stdout, "No common settings changed\n");
+     }
+
+     return 0;
+}
+
 static int do_goffload(int fd, struct ifreq *ifr)
 {
      struct ethtool_value eval;

@@ -2407,8 +2492,9 @@ static int do_soffload(int fd, struct ifreq *ifr)

              }

              eval.cmd = ETHTOOL_SFLAGS;
-             eval.data = ((eval.data & ~off_flags_mask) |
-                          off_flags_wanted);
+             eval.data =
+                 ((eval.data & ~(off_flags_mask | common_flags_mask)) |
+                  (off_flags_wanted | common_flags_wanted));

Similarly, why should this use common_flags_mask and
common_flags_wanted?

For the same (wrong) reason mentioned above. I'll correct it and post
the new patch.

Thanks,
--mahesh..

Ben.

quoted

              err = ioctl(fd, SIOCETHTOOL, ifr);
              if (err) {

--
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.