netdev - Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics Information Support

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: <1466933589.2504.42.camel@decadent.org.uk>
Date:	Sun, 26 Jun 2016 11:33:09 +0200
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Vidya Sagar Ravipati <vidya@...ulusnetworks.com>,
	netdev@...r.kernel.org, bwh@...nel.org, davem@...emloft.net
Cc:	bkenward@...arflare.com, daniel@...earbox.net, galp@...lanox.com,
	roopa@...ulusnetworks.com, gospo@...ulusnetworks.com,
	dustin@...ulusnetworks.com
Subject: Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics
 Information Support

On Mon, 2016-06-13 at 16:27 -0700, Vidya Sagar Ravipati wrote:
> From: Vidya Sagar Ravipati <vidya@...ulusnetworks.com>
> 
> This patch series provides following support
> a) Reorganized fields based out of SFF-8024 fields i.e. Identifier/
>    Encoding/Connector types which are common across SFP/SFP+ (SFF-8472)
>    and QSFP+/QSFP28 (SFF-8436/SFF-8636) modules into sff-common files.
> a) Support for diagnostics information for QSFP Plus/QSFP28 modules
>    based on SFF-8436/SFF-8636

Those two changes should be separate patches.

> Standards for QSFP+/QSFP28
> a) QSFP+/QSFP28 - SFF 8636 Rev 2.7 dated January 26,2016
> b) SFF-8024 Rev 4.0 dated May 31, 2016
> 
> Signed-off-by: Vidya Sagar Ravipati <vidya@...ulusnetworks.com>
> Acked-by: Bert Kenward <bkenward@...arflare.com>
> ---
>  Makefile.am  |   2 +-
>  ethtool.c    |   5 +
>  internal.h   |   3 +
>  qsfp.c       | 876 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qsfp.h       | 599 ++++++++++++++++++++++++++++++++++++++++
>  sff-common.c | 255 +++++++++++++++++
>  sff-common.h | 156 +++++++++++
>  sfpid.c      | 103 +------
>  8 files changed, 1899 insertions(+), 100 deletions(-)
>  create mode 100644 qsfp.c
>  create mode 100644 qsfp.h
>  create mode 100644 sff-common.c
>  create mode 100644 sff-common.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6814bc9..1f3d767 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -13,7 +13,7 @@ ethtool_SOURCES += \
>  		  fec_8xx.c ibm_emac.c ixgb.c ixgbe.c natsemi.c	\
>  		  pcnet32.c realtek.c tg3.c marvell.c vioc.c	\
>  		  smsc911x.c at76c50x-usb.c sfc.c stmmac.c	\
> -		  sfpid.c sfpdiag.c ixgbevf.c tse.c vmxnet3.c
> +		  sff-common.c sfpid.c sfpdiag.c ixgbevf.c tse.c vmxnet3.c qsfp.c
>  endif
>  
>  TESTS = test-cmdline test-features
> diff --git a/ethtool.c b/ethtool.c
> index 0cd0d4f..a0d48d1 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3963,6 +3963,11 @@ static int do_getmodule(struct cmd_context *ctx)
>  				sff8079_show_all(eeprom->data);
>  				sff8472_show_all(eeprom->data);
>  				break;
> +			case ETH_MODULE_SFF_8436:
> +			case ETH_MODULE_SFF_8636:
> +				sff8636_show_all(eeprom->data,
> +				      modinfo.eeprom_len);

The last line here is wrongly indented, as are many other continuation
lines.  ethtool code should be formatted just the same way David likes
kernel networking code.

[...]
> --- /dev/null
> +++ b/qsfp.c
[...]
> +/* Channel Monitoring Fields */
> +struct sff8636_channel_diags {
> +
> +	__u16 bias_cur;      /* Measured bias current in 2uA units */
> +	__u16 rx_power;      /* Measured RX Power */
> +	__u16 tx_power;      /* Measured TX Power */
> +
> +};

There's no need for blank lines in this structure definition.

> +/* Module Monitoring Fields */
> +struct sff8636_diags {
> +
> +#define MAX_CHANNEL_NUM 4
> +#define LWARN 0
> +#define HWARN 1
> +#define LALRM 2
> +#define HALRM 3
> +
> +	 /* Supports DOM */
> +	__u8 supports_dom;
> +	/* Supports alarm/warning thold */
> +	__u8 supports_alarms;
> +	/* RX Power: 0 = OMA, 1 = Average power */
> +	__u8  rx_power_type;
> +	/* TX Power: 0 = Not supported, 1 = Average power */
> +	__u8  tx_power_type;
> +	/* SFP voltage in 0.1mV units */
> +	__u16 sfp_voltage;
> +	/* SFP Temp in 16-bit signed 1/256 Celcius */
> +	__s16 sfp_temp;
> +	/* [4] tables are low/high warn, low/high alarm */
> +	/* SFP voltage in 0.1mV units */
> +	__u16 thresh_sfp_voltage[4];
> +	/* SFP Temp in 16-bit signed 1/256 Celcius */
> +	__s16 thresh_sfp_temp[4];
> +	/* Measured bias current in 2uA units */
> +	__u16 thresh_bias_cur[4];
> +	/* Measured TX Power */
> +	__u16 thresh_tx_power[4];
> +	/* Measured RX Power */
> +	__u16 thresh_rx_power[4];

This looks very similar to sff8472_diags, only with the actual values
separated from the arrays of thresholds.

Can the structure and code be combined with sfpdiag.c, with the
additional per-channel diagnostics being optional?

> +	struct sff8636_channel_diags scd[MAX_CHANNEL_NUM];
> +};
[...]
> --- /dev/null
> +++ b/sff-common.c
[...]
> +double convert_mw_to_dbm(double mw)
> +{
> +	return (10. * log10(mw / 1000.)) + 30.;
> +}
[...]

This is copied from sfpdiag.c, so you should make that file use it
rather than a duplicate definition.

Ben.

-- 

Ben Hutchings
compatible: Gracefully accepts erroneous data from any source

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)