netdev - Re: [PATCH 1/3] ethtool: correctly interpret bitrate of 255

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191021163551.GA7530@netronome.com>
Date:   Mon, 21 Oct 2019 18:35:53 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, linville@...driver.com, andrew@...n.ch,
        f.fainelli@...il.com
Subject: Re: [PATCH 1/3] ethtool: correctly interpret bitrate of 255

On Mon, Oct 21, 2019 at 09:09:44AM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Oct 21, 2019 at 09:40:31AM +0200, Simon Horman wrote:
> > On Fri, Oct 18, 2019 at 09:31:13PM +0100, Russell King wrote:
> > > From: Russell King <rmk+kernel@...linux.org.uk>
> > > 
> > > A bitrate of 255 is special, it means the bitrate is encoded in
> > > byte 66 in units of 250MBaud.  Add support for parsing these bit
> > > rates.
> > 
> > Hi Russell,
> > 
> > it seems from the code either that 0 is also special or its
> > handling has been optimised. Perhaps that would be worth mentioning
> > in the changelog too.
> 
> The text of SFF8472 rev 12.2:
> 
> 5.7     BR, nominal [Address A0h, Byte 12]
> The nominal bit (signaling) rate (BR, nominal) is specified in units of
> 100MBd, rounded off to the nearest 100MBd. The bit rate includes those
> bits necessary to encode and delimit the signal as well as those bits
> carrying data information. A value of FFh indicates the bit rate is
> greater than 25.0Gb/s and addresses 66 and 67 are used to determine
> bit rate. A value of 0 indicates that the bit rate is not specified and
> must be determined from the transceiver technology. The actual
> information transfer rate will depend on the encoding of the data, as
> defined by the encoding value.
> 
> 8.4    BR, max [Address A0h, Byte 66]
> If address 12 is not set to FFh, the upper bit rate limit at which the
> transceiver will still meet its specifications (BR, max) is specified
> in units of 1% above the nominal bit rate. If address 12 is set to FFh,
> the nominal bit (signaling) rate (BR, nominal) is specified in units of
> 250 MBd, rounded off to the nearest 250 MBd. A value of 00h indicates
> that this field is not specified.
> 
> 8.5    BR, min [Address A0h, Byte 67]
> If address 12 is not set to FFh, the lower bit rate limit at which the
> transceiver will still meet its specifications (BR, min) is specified in
> units of 1% below the nominal bit rate. If address 12 is set to FFh, the
> limit range of bit rates specified in units of +/- 1% around the nominal
> signaling rate. A value of zero indicates that this field is not
> specified.
> 
> So I guess you could have a br_nom == 0 (meaning it should be derived
> from other information) but max/min != 0 - which would be complex to
> implement, and means that we're doing significant interpretation of
> the contents.

Thanks Russell,

tricky indeed. My suggestion is that something like the last paragraph
be included in the changelog. But if you feel otherwise I won't push the
issue any further.

> 
> > 
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> > > ---
> > >  sfpid.c | 19 ++++++++++++++++---
> > >  1 file changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/sfpid.c b/sfpid.c
> > > index a1753d3a535f..71f0939c6282 100644
> > > --- a/sfpid.c
> > > +++ b/sfpid.c
> > > @@ -328,11 +328,24 @@ void sff8079_show_all(const __u8 *id)
> > >  {
> > >  	sff8079_show_identifier(id);
> > >  	if (((id[0] == 0x02) || (id[0] == 0x03)) && (id[1] == 0x04)) {
> > > +		unsigned int br_nom, br_min, br_max;
> > > +
> > > +		if (id[12] == 0) {
> > > +			br_nom = br_min = br_max = 0;
> > > +		} else if (id[12] == 255) {
> > > +			br_nom = id[66] * 250;
> > > +			br_max = id[67];
> > > +			br_min = id[67];
> > > +		} else {
> > > +			br_nom = id[12] * 100;
> > > +			br_max = id[66];
> > > +			br_min = id[67];
> > > +		}
> > >  		sff8079_show_ext_identifier(id);
> > >  		sff8079_show_connector(id);
> > >  		sff8079_show_transceiver(id);
> > >  		sff8079_show_encoding(id);
> > > -		sff8079_show_value_with_unit(id, 12, "BR, Nominal", 100, "MBd");
> > > +		printf("\t%-41s : %u%s\n", "BR, Nominal", br_nom, "MBd");
> > >  		sff8079_show_rate_identifier(id);
> > >  		sff8079_show_value_with_unit(id, 14,
> > >  					     "Length (SMF,km)", 1, "km");
> > > @@ -348,8 +361,8 @@ void sff8079_show_all(const __u8 *id)
> > >  		sff8079_show_ascii(id, 40, 55, "Vendor PN");
> > >  		sff8079_show_ascii(id, 56, 59, "Vendor rev");
> > >  		sff8079_show_options(id);
> > > -		sff8079_show_value_with_unit(id, 66, "BR margin, max", 1, "%");
> > > -		sff8079_show_value_with_unit(id, 67, "BR margin, min", 1, "%");
> > > +		printf("\t%-41s : %u%s\n", "BR margin, max", br_max, "%");
> > > +		printf("\t%-41s : %u%s\n", "BR margin, min", br_min, "%");
> > >  		sff8079_show_ascii(id, 68, 83, "Vendor SN");
> > >  		sff8079_show_ascii(id, 84, 91, "Date code");
> > >  	}
> > > -- 
> > > 2.7.4
> > > 
> > 
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up