| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20171218184911.GB18214@tuxdriver.com>
Date: Mon, 18 Dec 2017 13:49:12 -0500
From: "John W. Linville" <linville@...driver.com>
To: Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc: netdev@...r.kernel.org, oss-drivers@...ronome.com,
Roopa Prabhu <roopa@...ulusnetworks.com>,
Dustin Byford <dustin@...ulusnetworks.com>,
Vidya Sagar Ravipati <vidya.chowdary@...il.com>,
Dirk van der Merwe <dirk.vandermerwe@...ronome.com>
Subject: Re: [PATCH ethtool] ethtool: Support for FEC encoding control
On Fri, Dec 15, 2017 at 04:35:17PM -0800, Jakub Kicinski wrote:
> From: Dustin Byford <dustin@...ulusnetworks.com>
>
> As FEC settings and different FEC modes are mandatory
> and configurable across various interfaces of 25G/50G/100G/40G,
> the lack of FEC encoding control and reporting today is a source
> for interoperability issues for many vendors
>
> set-fec/show-fec option(s) are designed to provide control and report
> the FEC encoding on the link.
>
> $ethtool --set-fec swp1 encoding [off | RS | BaseR | auto]
>
> Encoding: Types of encoding
> Off : Turning off FEC
> RS : Force RS-FEC encoding
> BaseR : Force BaseR encoding
> Auto : Default FEC settings for drivers, and would represent
> asking the hardware to essentially go into a best effort mode.
>
> Here are a few examples of what we would expect if encoding=auto:
> - if autoneg is on, we are expecting FEC to be negotiated as on or off
> as long as protocol supports it
> - if the hardware is capable of detecting the FEC encoding on it's
> receiver it will reconfigure its encoder to match
> - in absence of the above, the configuration would be set to IEEE
> defaults.
>
> From our understanding, this is essentially what most hardware/driver
> combinations are doing today in the absence of a way for users to
> control the behavior.
>
> $ethtool --show-fec swp1
> FEC parameters for swp1:
> FEC encodings: RS
>
> ethtool devname output:
> $ethtool swp1
> Settings for swp1:
> root@...-7712-03:~# ethtool swp18
> Settings for swp18:
> Supported ports: [ FIBRE ]
> Supported link modes: 40000baseCR4/Full
> 40000baseSR4/Full
> 40000baseLR4/Full
> 100000baseSR4/Full
> 100000baseCR4/Full
> 100000baseLR4_ER4/Full
> Supported pause frame use: No
> Supports auto-negotiation: Yes
> Supported FEC modes: [RS | BaseR | None | Not reported]
> Advertised link modes: Not reported
> Advertised pause frame use: No
> Advertised auto-negotiation: No
> Advertised FEC modes: [RS | BaseR | None | Not reported]
> Speed: 100000Mb/s
> Duplex: Full
> Port: FIBRE
> PHYAD: 106
> Transceiver: internal
> Auto-negotiation: off
> Link detected: yes
>
> Signed-off-by: Vidya Sagar Ravipati <vidya.chowdary@...il.com>
> Signed-off-by: Dustin Byford <dustin@...ulusnetworks.com>
> [code style + man page edits + commit message update]
> Signed-off-by: Dirk van der Merwe <dirk.vandermerwe@...ronome.com>
> ---
> ethtool.8.in | 31 ++++++++++++++++
> ethtool.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 150 insertions(+)
Mostly looks good to me, but...
<snip>
> @@ -740,6 +744,26 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
> fprintf(stdout, "Yes\n");
> else
> fprintf(stdout, "No\n");
> +
> + fprintf(stdout, " %s FEC modes:", prefix);
One space after the colon saves 4 bytes of string space in the fprintf
calls below...
> + if (ethtool_link_mode_test_bit(
> + ETHTOOL_LINK_MODE_FEC_NONE_BIT, mask)) {
> + fprintf(stdout, " None");
> + fecreported = 1;
> + }
> + if (ethtool_link_mode_test_bit(
> + ETHTOOL_LINK_MODE_FEC_BASER_BIT, mask)) {
> + fprintf(stdout, " BaseR");
> + fecreported = 1;
> + }
> + if (ethtool_link_mode_test_bit(
> + ETHTOOL_LINK_MODE_FEC_RS_BIT, mask)) {
> + fprintf(stdout, " RS");
> + fecreported = 1;
> + }
> + if (!fecreported)
> + fprintf(stdout, " Not reported");
> + fprintf(stdout, "\n");
> }
> }
>
Style issue: I would prefer to see those ethtool_link_mode_test_bit
lines broken differently. The ETHTOOL_LINK_MODE_FEC_* parameters can
continue on the same line as the function name and still be within 80
columns. The mask parameter can be indented on the next line using
tabs and spaces as usual. I would prefer something similar to the
call to parse_generic_cmdline in do_sfec below:
<snip>
> +static int do_sfec(struct cmd_context *ctx)
> +{
> + char *fecmode_str = NULL;
> + struct ethtool_fecparam feccmd;
> + struct cmdline_info cmdline_fec[] = {
> + { "encoding", CMDL_STR, &fecmode_str, &feccmd.fec},
> + };
> + int changed;
> + int fecmode;
> + int rv;
> +
> + parse_generic_cmdline(ctx, &changed, cmdline_fec,
> + ARRAY_SIZE(cmdline_fec));
<snip>
Thanks,
John
--
John W. Linville Someday the world will need a hero, and you
linville@...driver.com might be all we have. Be ready.
Powered by blists - more mailing lists