Re: [ethtool PATCH v4 10/11] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls

Just sent v5 of the series: now only this patch left. Applied all your
suggestions.

Many thanks for the 2 bugs you caught, path was not covered by my
tests. v5 was tested on a 10G nic with: ethtool -s eth1 msglvl 0x15
speed 10000 duplex full

On Sun, Mar 13, 2016 at 6:32 PM, Ben Hutchings [off-list ref] wrote:

On Fri, 2016-03-11 at 09:58 -0800, David Decotigny wrote:
[...]

quoted

+static int parse_hex_u32_bitmap(const char *s,
+                             unsigned int nbits, u32 *result)
+{
+     const unsigned nwords = __KERNEL_DIV_ROUND_UP(nbits, 32);
+     size_t slen = strlen(s);
+     size_t i;
+
+     /* ignore optional '0x' prefix */
+     if ((slen > 2) && (
+                 (0 == memcmp(s, "0x", 2)
+                  || (0 == memcmp(s, "0X", 2))))) {

memcmp() is a really poor tool for comparing strings.  You should use
strncasecmp() here.

done.

quoted

+             slen -= 2;
+             s += 2;
+     }
+
+     if (slen > 8*nwords)  /* up to 2 digits per byte */

The '*' operator should have spaces around it.  Please run
checkpatch.pl over your ethtool patches to catch style errors like this
(there are many others in this one).

both done. a few new lines over 80 chars that I prefer to keep,
otherwise line break would make code harder to read imho.

[...]

quoted

+static void dump_link_caps(const char *prefix, const char *an_prefix,
+                        const u32 *mask, int link_mode_only)
 {
      static const struct {
              int same_line; /* print on same line as previous */
-             u32 value;
+             unsigned bit_indice;

bit_index

done.

[...]

quoted

@@ -558,51 +655,57 @@ dump_link_caps(const char *prefix, const char *an_prefix, u32 mask,
              }
      }
      if (did1 == 0)
-              fprintf(stdout, "Not reported");
+             fprintf(stdout, "Not reported");

Good catch, but whitespace fixes belong in another patch.

removed for now.

[...]

quoted

@@ -2248,10 +2360,219 @@ static int do_sfeatures(struct cmd_context *ctx)
      return 0;
 }

-static int do_gset(struct cmd_context *ctx)
+static struct ethtool_link_usettings *
+__do_ioctl_glinksettings(struct cmd_context * ctx)
+{
+     int err;
+     struct {
+             struct ethtool_link_settings req;
+             __u32 link_mode_data[3*__ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
+     } ecmd;
+     struct ethtool_link_usettings *link_usettings;
+     unsigned u32_offs;
+
+     /* handshake with kernel to determine number of words for link
+      * mode bitmaps */
+     memset(&ecmd, 0, sizeof(ecmd));
+     ecmd.req.cmd = ETHTOOL_GLINKSETTINGS;
+     err = send_ioctl(ctx, &ecmd);
+     if (err < 0)
+             return NULL;
+
+     if (ecmd.req.link_mode_masks_nwords >= 0 || ecmd.req.cmd)
+             return NULL;

Oops, I missed the kernel side of this.  Clearing the cmd field is a
very strange thing to do and inconsistent with every other ethtool
command, so I've just sent a patch to change the kernel side of that.
You'll need to change this to match.

(I also think the negative size is a bit weird, but don't feel so
strongly about it.)

updated based on your patch. sent a suggestion
http://marc.info/?l=linux-netdev&m=145806871332416 , looks like it was
not added properly to the thread, because I don't see it as a reply to
your patch in patchwork. sorry about this.

[...]

quoted

+     link_usettings = malloc(sizeof(*link_usettings));
+     if (NULL == link_usettings)

Comparison is the wrong way round.

done.

quoted

+             return NULL;
+
+     memset(link_usettings, 0, sizeof(*link_usettings));

Could use calloc() instead of malloc() + memset().

done.

quoted

+     /* keep transceiver 0 */
+     memcpy(&link_usettings->base, &ecmd.req, sizeof(link_usettings->base));
+     /* remember that ETHTOOL_GLINKSETTINGS was used */
+     link_usettings->base.cmd = ETHTOOL_GLINKSETTINGS;

This is redundant as we know that ecmd.req.cmd ==
ETHTOOL_GLINKSETTINGS.

done.

[...]

quoted

+static struct ethtool_link_usettings *
+__do_ioctl_gset(struct cmd_context * ctx)
 {

[...]

quoted

+     link_usettings->base.link_mode_masks_nwords = 1;
+     link_usettings->link_modes.supported[0] = ecmd.supported;
+     link_usettings->link_modes.advertising[0] = ecmd.advertising;
+     link_usettings->link_modes.lp_advertising[0] = ecmd.lp_advertising;
+     link_usettings->base.speed = ethtool_cmd_speed(&ecmd);
+     link_usettings->base.duplex = ecmd.duplex;
+     link_usettings->base.port = ecmd.port;
+     link_usettings->base.phy_address = ecmd.phy_address;
+     link_usettings->deprecated.transceiver = ecmd.transceiver;
+     link_usettings->base.autoneg = ecmd.autoneg;
+     link_usettings->base.mdio_support = ecmd.mdio_support;
+     /* ignored (fully deprecated): maxrxpkt, maxtxpkt */
+     link_usettings->base.eth_tp_mdix = ecmd.eth_tp_mdix;
+     link_usettings->base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl;
+     link_usettings->deprecated.transceiver = ecmd.transceiver;

Duplicate assignment.

done.

quoted

+     return link_usettings;
+}
+
+static bool ethtool_link_mode_is_backward_compatible(
+     const u32 *mask)

Don't wrap function parameter lists or statements that are already
under 80 characters.

done.

[...]

quoted

+static void
+__free_link_usettings(struct ethtool_link_usettings *link_usettings) {
+     free(link_usettings);
+}

Is this function ever likely to do more than just free()?  If not,
don't bother abstracting that.

free() is likely to stay good enough. updated.

[...]

quoted

@@ -2469,75 +2799,88 @@ static int do_sset(struct cmd_context *ctx)
              }
      }

-     if (full_advertising_wanted < 0) {
+     if (NULL == full_advertising_wanted) {
              /* User didn't supply a full advertisement bitfield:
               * construct one from the specified speed and duplex.
               */
+             int adv_bit = -1;
+
              if (speed_wanted == SPEED_10 && duplex_wanted == DUPLEX_HALF)
-                     advertising_wanted = ADVERTISED_10baseT_Half;
+                     adv_bit = ETHTOOL_LINK_MODE_10baseT_Half_BIT;
              else if (speed_wanted == SPEED_10 &&
                       duplex_wanted == DUPLEX_FULL)
-                     advertising_wanted = ADVERTISED_10baseT_Full;
+                     adv_bit = ETHTOOL_LINK_MODE_10baseT_Full_BIT;
              else if (speed_wanted == SPEED_100 &&
                       duplex_wanted == DUPLEX_HALF)
-                     advertising_wanted = ADVERTISED_100baseT_Half;
+                     adv_bit = ETHTOOL_LINK_MODE_100baseT_Half_BIT;
              else if (speed_wanted == SPEED_100 &&
                       duplex_wanted == DUPLEX_FULL)
-                     advertising_wanted = ADVERTISED_100baseT_Full;
+                     adv_bit = ETHTOOL_LINK_MODE_100baseT_Full_BIT;
              else if (speed_wanted == SPEED_1000 &&
                       duplex_wanted == DUPLEX_HALF)
-                     advertising_wanted = ADVERTISED_1000baseT_Half;
+                     adv_bit = ETHTOOL_LINK_MODE_1000baseT_Half_BIT;
              else if (speed_wanted == SPEED_1000 &&
                       duplex_wanted == DUPLEX_FULL)
-                     advertising_wanted = ADVERTISED_1000baseT_Full;
+                     adv_bit = ETHTOOL_LINK_MODE_1000baseT_Full_BIT;
              else if (speed_wanted == SPEED_2500 &&
                       duplex_wanted == DUPLEX_FULL)
-                     advertising_wanted = ADVERTISED_2500baseX_Full;
+                     adv_bit = ETHTOOL_LINK_MODE_2500baseX_Full_BIT;
              else if (speed_wanted == SPEED_10000 &&
                       duplex_wanted == DUPLEX_FULL)
-                     advertising_wanted = ADVERTISED_10000baseT_Full;
-             else
-                     /* auto negotiate without forcing,
-                      * all supported speed will be assigned below
-                      */
-                     advertising_wanted = 0;
+                     adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
+
+             if (adv_bit >= 0) {
+                     advertising_wanted = _mask_advertising_wanted;

If I'm not mistaken, _mask_advertising_wanted is uninitialised at this
point so you need to clear it before setting the one bit.

great catch! thanks.

quoted

+                     ethtool_link_mode_set_bit(
+                             adv_bit, advertising_wanted);

[...]

quoted

@@ -2549,37 +2892,56 @@ static int do_sset(struct cmd_context *ctx)
                              fprintf(stderr, "\n");
                      }
                      if (autoneg_wanted == AUTONEG_ENABLE &&
-                         advertising_wanted == 0) {
+                         NULL == advertising_wanted) {
+                             unsigned int i;
+
                              /* Auto negotiation enabled, but with
                               * unspecified speed and duplex: enable all
                               * supported speeds and duplexes.
                               */
-                             ecmd.advertising =
-                                     (ecmd.advertising &
-                                      ~ALL_ADVERTISED_MODES) |
-                                     (ALL_ADVERTISED_MODES & ecmd.supported);
+                             ethtool_link_mode_for_each_u32(i)
+                             {

Brace belongs on the previous line (everywhere you use
ethtool_link_mode_for_each_u32()).

done.

[...]

quoted

-                     } else if (advertising_wanted > 0) {
+                     } else if (NULL != advertising_wanted) {
+                             unsigned int i;
+
                              /* Enable all requested modes */
-                             ecmd.advertising =
-                                     (ecmd.advertising &
-                                      ~ALL_ADVERTISED_MODES) |
-                                     advertising_wanted;
-                     } else if (full_advertising_wanted > 0) {
-                             ecmd.advertising = full_advertising_wanted;
+                             ethtool_link_mode_for_each_u32(i)
+                             {
+                                     u32 *adv = link_usettings->link_modes.advertising + i;
+                                     *adv = ((*adv & all_advertised_modes[i])

[...]

Should be ~all_advertised_modes[i] rather than all_advertised_modes[i].

aouch! thanks.

Ben.

--
Ben Hutchings
If at first you don't succeed, you're doing about average.