Re: [PATCH ethtool] ethtool: support combinations of FEC modes

I was won

+
+/* Takes a comma-separated list of FEC modes, returns the bitwise OR of
their
+ * corresponding ETHTOOL_FEC_* constants.
+ * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,').
+ */
+static int parse_fecmode(const char *str)
+{
        int fecmode = 0;
+       char buf[6];
        if (!str)
-               return fecmode;
-
-       if (!strcasecmp(str, "auto"))
-               fecmode |= ETHTOOL_FEC_AUTO;
-       else if (!strcasecmp(str, "off"))
-               fecmode |= ETHTOOL_FEC_OFF;
-       else if (!strcasecmp(str, "rs"))
-               fecmode |= ETHTOOL_FEC_RS;
-       else if (!strcasecmp(str, "baser"))
-               fecmode |= ETHTOOL_FEC_BASER;
+               return 0;
+       while (*str) {
+               size_t next;
+               int mode;
  +             next = strcspn(str, ",");
+               if (next >= 6) /* Bad mode, longest name is 5 chars */
+                       return 0;
+               /* Copy into temp buffer and nul-terminate */
+               memcpy(buf, str, next);
+               buf[next] = 0;
+               mode = fecmode_str_to_type(buf);
+               if (!mode) /* Bad mode encountered */
+                       return 0;
+               fecmode |= mode;
+               str += next;
+               /* Skip over ',' (but not nul) */
+               if (*str)
+                       str++;
+       }
        return fecmode;

I would like to apologize for my late response.

I find the ability to set off, auto and specific FEC mode in the same
command confusing.
Here are some examples

1. What is the expected result of 'off' & other FEC mode such as 'RS'?
  -'off'?
  -'RS'?
  -automatic selection {'off','RS'}? w/o setting of auto?

2. What is the expected result of 'off', 'RS' and 'auto'?
  - automatic selection from the set of {'RS','off'}
    - if that is the case, what is the different from 'off' and 'RS'
with out 'auto'?
  - allowing the device to use all three modes
    - automatic selection {auto, rs, off}. what is the meaning of auto of auto?

I think that we shall have some mutual configuration limitation :

1. if 'auto' was set, any other configuration from within the set
{'off', 'RS', 'base-r'}
    will imply the set of configuration to be selected by auto mode
    i.e. 'auto', 'RS' and 'off' configuration will result with
automatic selection between {'off', 'RS'}
2. if 'auto' was not set, only one configuration from within {'off',
'RS', 'base-r'} can
    be set (and from that, 'off' cannot be set with other configuration)

Thanks
Ariel Almog
Mellanox technologies

On 26/09/18 09:47, Ariel Almog wrote:

I was won

Truncated sentence?  ("... wondering"?)

I find the ability to set off, auto and specific FEC mode in the same
command confusing.

I didn't try to define semantics here since each driver currently does
 something slightly different.  Probably the configuration space that's
 meaningful is different for each piece of hardware anyway.

Here are some examples

1. What is the expected result of 'off' & other FEC mode such as 'RS'?
  -'off'?
  -'RS'?
  -automatic selection {'off','RS'}? w/o setting of auto?

In sfc, 'off' overrides everything else.

The meaning (again, in sfc) of a combination of 'auto' and a specific mode
 (e.g. 'rs') is "prefer the specified mode, but fall back to autoneg if
 it's not supported".  The combination {'rs', 'baser'} (with or without
 'auto') means "use the strongest FEC supported", i.e. it will attempt to
 negotiate FEC even if the cable & link partner don't request it (e.g. a
 short cable).

For us, those semantics make sense (our HW has a notion of 'supported'
 and 'requested' bits for each FEC type for each of local-device, cable
 and link-partner, and uses the strongest FEC mode that's supported by
 everyone and requested by anyone); but if something else is a better fit
 for your hardware I wouldn't worry too much about the inconsistency —
 people using this functionality will hopefully have read the hardware's
 user manual...

-Ed

For us, those semantics make sense (our HW has a notion of 'supported'
 and 'requested' bits for each FEC type for each of local-device, cable
 and link-partner, and uses the strongest FEC mode that's supported by
 everyone and requested by anyone); but if something else is a better fit
 for your hardware I wouldn't worry too much about the inconsistency —
 people using this functionality will hopefully have read the hardware's
 user manual...

I wonder how true that will be in 5 years time, about reading the
manual? SFP sockets are starting to appear in consumer devices. There
are some Marvell SoC reference boards with SFP and SFP+. Broadcom also
have some boards with SFP. With time, SFP will move out of the data
centre and comms rack and into more everyday systems. In such context,
reading the manual becomes less likely. It would be nice to avoid a
future inconsistent mess caused be this sentiment now.

      Andrew

On 28/09/18 16:39, Andrew Lunn wrote:

I wonder how true that will be in 5 years time, about reading the
manual? SFP sockets are starting to appear in consumer devices. There
are some Marvell SoC reference boards with SFP and SFP+. Broadcom also
have some boards with SFP. With time, SFP will move out of the data
centre and comms rack and into more everyday systems. In such context,
reading the manual becomes less likely. It would be nice to avoid a
future inconsistent mess caused be this sentiment now.

      Andrew

I see where you're coming from, but if people start needing to manually
 configure FEC on their consumer devices, possibly we have bigger
 problems.
Ethtool FEC control is for those situations where autoneg, autodetect,
 autoconfigure etc. don't work (e.g. owing to out-of-spec switches, or
 just a user wanting to disable FEC to save a few hundred nanos).  I
 would hope that FEC won't show up in consumer gear until these kinds
 of problems have settled down somewhat.

Perhaps we can add something to the man page saying that not only can
 the semantics vary from NIC to NIC, but that the semantics for a given
 NIC might change in the future?  Then if in five years' time we know
 what the Right Thing™ is, we can move everyone over to that (with
 appropriately *loud* release-notes).

I think the alternative, of finding a set of semantics that fits
 everyone's hardware and covers everyone's requirements, is likely to
 be difficult (and probably require changing the ethtool API).

-Ed

I see where you're coming from, but if people start needing to manually
 configure FEC on their consumer devices, possibly we have bigger
 problems.

Yes, i agree with that. For the consumer market, SFPs needs to grow up
and start doing full and reliable auto-neg, just like copper Ethernet.

However, there is often an intermediate step after the really niche
market like TOR routers. Industrial applications start using this
stuff. There are a lot of planes flying today using SFPs for the
inflight entertainment systems. Fibre weights less than copper. It is
a somewhat specialist market, so you probably can still force them to
read the hardware manual, but i think they would prefer not to. And
i'm sure they are not the only industrial users. There are likely to
be more industrial users than TOR users.

In general, it is hard to know which APIs are going to remain Unix
Wizard level, and which are going to be used by mere mortals. So
ideally, we want consistency everywhere.

I think the alternative, of finding a set of semantics that fits
 everyone's hardware and covers everyone's requirements, is likely to
 be difficult (and probably require changing the ethtool API).

Now is a good time to change the API, since we are moving to a netlink
socket. Which is why these questions were asked in the first place...

	Andrew

On 28/09/18 17:45, Andrew Lunn wrote:

Now is a good time to change the API, since we are moving to a netlink
socket. Which is why these questions were asked in the first place...

OK, well, I've posted sfc's semantics and view-from-the-hardware*; now
 patiently waiting for other NIC vendors to chime in so we can try to
 converge on something consistent.
Then again, since they've been CCed since the original patch three weeks
 ago, we might be waiting a while :-(

Regarding Ariel Almog's suggested semantics, it seems like they have the
 'auto' bit just encoding 'more than one non-auto bit', which is
 redundant (i.e. off|rs is always off|rs|auto, whereas rs is never
 rs|auto).  I don't see how that would be useful.

-Ed

* One complication I left out: we actually have _three_ pairs of sup/req
  bits, because we separate 'BaseR for 10G/40G/100G' from 'BaseR for
  25G/50G'.  I don't know the details of why our HW does this (or why
  100G isn't lumped in with the other 25ers) but I think it has to do
  with Horrific Ethernet Spec Arcana Man Was Not Meant To Know™.