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

[Ben Hutchings]

On Mon, 2016-06-27 at 00:02 +0200, Ben Hutchings wrote:
> On Sun, 2016-06-26 at 09:40 -0700, Vidya Sagar Ravipati wrote:
> > On Sun, Jun 26, 2016 at 2:33 AM, Ben Hutchings  wrote:
> [...]
> > > 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?
> > 
> > Diagnostic dom information in QSFP  has lot more information compared
> > to SFPs and as part of this checkin , basic dom information in qsfp which is
> > equivalent to sfp dom  is getting exposed as part of this checkin.
> > 
> > Here are list of fields (not complete)  which  are used for  debugging QSFP
> > issues, will be added for this structure in next patch sets
> > a) TX/RX output amplitude conttrol
> > b)  TX_DISABLE
> > b) TX_FAULT
> > c) TX CDR
> > d) RX CDR
> > e) RX output disable
> > f) Rate select option
> > 
> > Please let me know if it make sense to maintain the different structure
> > with above explanation  or whether it is required to be combined.
> [...]
> 
> I think there's enough information in common that it does make sense to
> use common reporting functions, and that in turn suggests that it would
> make sense to use a common structure.  You could alternately have the
> callers in sfpdiag.c and qsfp.c extract the relevant fields and pass
> them into the reporting functions.
> 
> The substantial duplication of reporting code from sfpid.c in your
> latest submission is not OK.

I mean sfpdiag.c here.  I didn't check for duplication from sfpid.c,
but I think you've avoided that.

Ben.

-- 

Ben Hutchings
Humour is the best antidote to reality.


signature.asc
Description: This is a digitally signed message part

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

[Ben Hutchings]

On Sun, 2016-06-26 at 09:40 -0700, Vidya Sagar Ravipati wrote:
> On Sun, Jun 26, 2016 at 2:33 AM, Ben Hutchings  wrote:
[...]
> > 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?
> 
> Diagnostic dom information in QSFP  has lot more information compared
> to SFPs and as part of this checkin , basic dom information in qsfp which is
> equivalent to sfp dom  is getting exposed as part of this checkin.
> 
> Here are list of fields (not complete)  which  are used for  debugging QSFP
> issues, will be added for this structure in next patch sets
> a) TX/RX output amplitude conttrol
> b)  TX_DISABLE
> b) TX_FAULT
> c) TX CDR
> d) RX CDR
> e) RX output disable
> f) Rate select option
> 
> Please let me know if it make sense to maintain the different structure
> with above explanation  or whether it is required to be combined.
[...]

I think there's enough information in common that it does make sense to
use common reporting functions, and that in turn suggests that it would
make sense to use a common structure.  You could alternately have the
callers in sfpdiag.c and qsfp.c extract the relevant fields and pass
them into the reporting functions.

The substantial duplication of reporting code from sfpid.c in your
latest submission is not OK.

Ben.

-- 

Ben Hutchings
compatible: Gracefully accepts erroneous data from any source


signature.asc
Description: This is a digitally signed message part

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

[Rami Rosen]

Hi Vidya,
Thanks a lot for your your work!

Here are my 2 cents:

> +   /* 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 */

You already had just 5 lines earlier: /* SFP voltage in 0.1mV units */
Shouldn't it be: /* SFP threshold voltage in 0.1mV units */ ?

> +   /* SFP voltage in 0.1mV units */
> +   __u16 thresh_sfp_voltage[4];


pagging should be: paging,
support_alarms should be:supports_alarms
> +* If pagging support exists, then support_alarms is marked as 1
> +*/
> +
+   if (eeprom_len == ETH_MODULE_SFF_8636_MAX_LEN) {
+   if (!(id[SFF8636_STATUS_2_OFFSET] &
+   SFF8636_STATUS_PAGE_3_PRESENT)) {
+   sd.supports_alarms = 1;
+   }
+   }


Shouldn't it be power/ TX bias current fields?  ("current" only once)
> +   /*
> +* SFF-8636/8436 spec is not clear whether RX power/ TX bias current
> +* current fields are supported or not. A valid temperature reading
> +* is used as existence for TX/RX power.

Should it be: SFF-8472/8079   (without "$")?
> + *
> + * Common utilities across SFF-8436/8636 and SFF-8472/8079$
> + * are defined in this file
> + *


Regards,
Rami Rosen

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

[Vidya Sagar Ravipati]

On Sun, Jun 26, 2016 at 2:33 AM, Ben Hutchings  wrote:
> On Mon, 2016-06-13 at 16:27 -0700, Vidya Sagar Ravipati wrote:
>> From: Vidya Sagar Ravipati 
>>
>> 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.
Will break down the 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 
>> Acked-by: Bert Kenward 
>> ---
>>  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.
>
Ack, will do
> [...]
>> --- /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.
>
Ack
>> +/* 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?

Diagnostic dom information in QSFP  has lot more information compared
to SFPs and as part of this checkin , basic dom information in qsfp which is
equivalent to sfp dom  is getting exposed as part of this checkin.

Here are list of fields (not complete)  which  are used for  debugging QSFP
issues, will be added for this structure in next patch sets
a) TX/RX output amplitude conttrol
b)  TX_DISABLE
b) TX_FAULT
c) TX CDR
d) RX CDR
e) RX output disable
f) Rate select option

Please let me know if it make 

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

[Ben Hutchings]

On Mon, 2016-06-13 at 16:27 -0700, Vidya Sagar Ravipati wrote:
> From: Vidya Sagar Ravipati 
> 
> 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 
> Acked-by: Bert Kenward 
> ---
>  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


signature.asc
Description: This is a digitally signed message part