Optics (SFP) monitoring on ixgbe and igbe

Hello,

I'm looking to propose a patch for optics monitoring on igbe and ixgbe
cards which support SFP and SFP+. I have one of each card. While
searching for previous work done by others on this domain, i found the
following:

https://github.com/jelaas/bifrost-build-x86_64/tree/master/all/kernel-x86_64-3.3.0-rc2-1
(the DOM-* files)

I did not yet test this patch (i'll do in a few days), but just by
looking at the patches, do you think this could get incorporated in
the current state, provided some administrative work is done, or would
it require more architectural rework / documentation / other ?

Could you point me to the next steps required for the functionality to
be merged in a future mainline kernel/ethtool ? Is there a better
place than netdev@ to discuss this matter ?

Thanks,
-- 
Aurélien Guillaume

On Wed, 2012-11-07 at 13:27 +0100, Aurélien wrote:

Hello,

I'm looking to propose a patch for optics monitoring on igbe and ixgbe
cards which support SFP and SFP+. I have one of each card. While
searching for previous work done by others on this domain, i found the
following:

https://github.com/jelaas/bifrost-build-x86_64/tree/master/all/kernel-x86_64-3.3.0-rc2-1
(the DOM-* files)

I did not yet test this patch (i'll do in a few days), but just by
looking at the patches, do you think this could get incorporated in
the current state, provided some administrative work is done, or would
it require more architectural rework / documentation / other ?

Could you point me to the next steps required for the functionality to
be merged in a future mainline kernel/ethtool ? Is there a better
place than netdev@ to discuss this matter ?

We just added the ETHTOOL_GMODULEINFO and ETHTOOL_GMODULEEEPROM
interface for this purpose.  The diagnostics obviously aren't really an
EEPROM but I don't think that matters.

These drivers should report a type of ETH_MODULE_SFF_8472 (assuming a
module is present) and expose an 'EEPROM' with the real EEPROM in the
first 256 bytes and the diagnostic registers in the next 256 bytes.

Decoding of the diagnostic registers can be done in ethtool itself.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Wed, Nov 7, 2012 at 8:58 PM, Ben Hutchings [off-list ref] wrote:

On Wed, 2012-11-07 at 13:27 +0100, Aurélien wrote:

We just added the ETHTOOL_GMODULEINFO and ETHTOOL_GMODULEEEPROM
interface for this purpose.  The diagnostics obviously aren't really an
EEPROM but I don't think that matters.

These drivers should report a type of ETH_MODULE_SFF_8472 (assuming a
module is present) and expose an 'EEPROM' with the real EEPROM in the
first 256 bytes and the diagnostic registers in the next 256 bytes.

Decoding of the diagnostic registers can be done in ethtool itself.

Hi Ben,

Thanks for this reply.

I did not see these interfaces in my searches, but from what I gather
from the code, the missing part is the actual decoding of the data in
ethtool.

As soon as I get my card installed in a test server, I'll reboot on a
kernel issued from the davem/net-next master, and will do some tests
to see if ethtool correctly sees an SFF-8472 compliant module, and has
the correct diagnostic data after the eeprom.

Then I will port the decoding part from the kernel-side patches I to
code in ethtool, and will do some basic tests.

Thanks,
Best regards,
-- 
Aurélien Guillaume

On Thu, 2012-11-08 at 01:39 +0100, Aurélien wrote:

On Wed, Nov 7, 2012 at 8:58 PM, Ben Hutchings [off-list ref] wrote:

quoted

On Wed, 2012-11-07 at 13:27 +0100, Aurélien wrote:

We just added the ETHTOOL_GMODULEINFO and ETHTOOL_GMODULEEEPROM
interface for this purpose.  The diagnostics obviously aren't really an
EEPROM but I don't think that matters.

These drivers should report a type of ETH_MODULE_SFF_8472 (assuming a
module is present) and expose an 'EEPROM' with the real EEPROM in the
first 256 bytes and the diagnostic registers in the next 256 bytes.

Decoding of the diagnostic registers can be done in ethtool itself.

Hi Ben,

Thanks for this reply.

I did not see these interfaces in my searches, but from what I gather
from the code, the missing part is the actual decoding of the data in
ethtool.

[...]

No, the driver also needs to implement ethtool_ops::get_module_info and
ethtool_ops::get_module_eeprom.  But those should be quite easy to do.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Fri, Nov 9, 2012 at 4:08 PM, Ben Hutchings [off-list ref] wrote:

No, the driver also needs to implement ethtool_ops::get_module_info and
ethtool_ops::get_module_eeprom.  But those should be quite easy to do.

Indeed. But like you said, it should be easy. I shall receive my cards
on monday, i'll try to do the implementation next week.

Best regards,
-- 
Aurélien Guillaume
- By all means break the rules, and break them beautifully,
deliberately and well. That is one of the ends for which they exist.
-- Robert Bringhurst

On Fri, Nov 9, 2012 at 4:08 PM, Ben Hutchings [off-list ref] wrote:

quoted

No, the driver also needs to implement ethtool_ops::get_module_info and
ethtool_ops::get_module_eeprom.  But those should be quite easy to do.

Hi !

I started to implement these operations in ixgbe.

So far, the result is the attached patch, which applies on dave-m's
net-next @ 1ff05fb7114a6b4118e0f7d89fed2659f7131b0a. It's not yet
finished, and since it is my first peek at network drivers I need some
advice on:

- whether the implementation seems correct for ixgbe and all its
supported MAC/PHY combinations ?
- what would be the best way to manage SFF-8472 A0/A2 bank swapping
mechanism for reading A2h ? (it seems I need to lock the whole -
adress change sequence - read A2h - address change again - operation)
in case it's needed. I may not be able to test that, so I may add an
unsupported return code for now.
- Is the supported PHY selection correct, or should other PHYs be
supported ? What should be the rule ?

I have been able to get correct temperature readings with a patched-up
ethtool, so it seems to work correctly on at least my card (Ethernet
controller [0200]: Intel Corporation 82599EB 10-Gigabit Network
Connection [8086:10fb] (rev 01)).

About ethtool, I was thinking about making a -O option for optical
diagnostics, which would have a readable output. I will make a
function to parse the A2 register contents, so it can be reused in
other daemons/libs (SNMP, etc).

Thanks,
Best regards,
-- 
Aurélien Guillaume

On 11/15/2012 01:36 PM, Aurélien wrote:

quoted

On Fri, Nov 9, 2012 at 4:08 PM, Ben Hutchings [off-list ref] wrote:

quoted

No, the driver also needs to implement ethtool_ops::get_module_info and
ethtool_ops::get_module_eeprom.  But those should be quite easy to do.

Hi !

I started to implement these operations in ixgbe.

So far, the result is the attached patch, which applies on dave-m's
net-next @ 1ff05fb7114a6b4118e0f7d89fed2659f7131b0a. It's not yet
finished, and since it is my first peek at network drivers I need some
advice on:

- whether the implementation seems correct for ixgbe and all its
supported MAC/PHY combinations ?
- what would be the best way to manage SFF-8472 A0/A2 bank swapping
mechanism for reading A2h ? (it seems I need to lock the whole -
adress change sequence - read A2h - address change again - operation)
in case it's needed. I may not be able to test that, so I may add an
unsupported return code for now.
- Is the supported PHY selection correct, or should other PHYs be
supported ? What should be the rule ?

I have been able to get correct temperature readings with a patched-up
ethtool, so it seems to work correctly on at least my card (Ethernet
controller [0200]: Intel Corporation 82599EB 10-Gigabit Network
Connection [8086:10fb] (rev 01)).

About ethtool, I was thinking about making a -O option for optical
diagnostics, which would have a readable output. I will make a
function to parse the A2 register contents, so it can be reused in
other daemons/libs (SNMP, etc).

Thanks,
Best regards,

Can you please add me [off-list ref] to the CC on future
patches for ixgbe or ixgb, as I will be the one applying the patch to my
queue?

Thanks
Jeff

On Thu, 2012-11-15 at 14:46 -0800, Jeff Kirsher wrote:

On 11/15/2012 01:36 PM, Aurélien wrote:

quoted

quoted

On Fri, Nov 9, 2012 at 4:08 PM, Ben Hutchings [off-list ref] wrote:

quoted

No, the driver also needs to implement ethtool_ops::get_module_info and
ethtool_ops::get_module_eeprom.  But those should be quite easy to do.

Hi !

I started to implement these operations in ixgbe.

So far, the result is the attached patch, which applies on dave-m's
net-next @ 1ff05fb7114a6b4118e0f7d89fed2659f7131b0a. It's not yet
finished, and since it is my first peek at network drivers I need some
advice on:

- whether the implementation seems correct for ixgbe and all its
supported MAC/PHY combinations ?
- what would be the best way to manage SFF-8472 A0/A2 bank swapping
mechanism for reading A2h ? (it seems I need to lock the whole -
adress change sequence - read A2h - address change again - operation)
in case it's needed. I may not be able to test that, so I may add an
unsupported return code for now.
- Is the supported PHY selection correct, or should other PHYs be
supported ? What should be the rule ?

I have been able to get correct temperature readings with a patched-up
ethtool, so it seems to work correctly on at least my card (Ethernet
controller [0200]: Intel Corporation 82599EB 10-Gigabit Network
Connection [8086:10fb] (rev 01)).

About ethtool, I was thinking about making a -O option for optical
diagnostics, which would have a readable output. I will make a
function to parse the A2 register contents, so it can be reused in
other daemons/libs (SNMP, etc).

Thanks,
Best regards,

Can you please add me [off-list ref] to the CC on future
patches for ixgbe or ixgb, as I will be the one applying the patch to my
queue?

Yes, Jeff's the one you should be talking to about these drivers.  I
just look after the ethtool utility and API.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Fri, Nov 16, 2012 at 12:30 AM, Ben Hutchings
[off-list ref] wrote:

Yes, Jeff's the one you should be talking to about these drivers.  I
just look after the ethtool utility and API.

Ok, so I will discuss the ixgbe patch with Jeff :)

Ben, on the ethtool side, attached is a patch to enable the following
option and output; It's still missing externally calibrated optics
support (my current one is internally calibrated, so that's difficult
to test anything). What do you think ? Is there any other data that
could be interesting to show with -O or -m options ?

I based the text output on Juniper's, which is quite parsable by other
apps, but if someone sees a more appropriate output mode, let me know.

# ./ethtool -h
        ethtool -O|--module-optics DEVNAME	Show module optical diagnostics

# ./ethtool -O eth6
Physical interface: eth6
    Laser bias current                        :  27.378 mA
    Laser output power                        :  0.6980 mW / -1.56 dBm
    Receiver signal average optical power     :  1.0817 mW / 0.34 dBm
    Module temperature                        :  53 degrees C / 128 degrees F
    Module voltage                            :  3.2657 V
    Laser bias current high alarm             :  Off
    Laser bias current low alarm              :  Off
    Laser bias current high warning           :  Off
    Laser bias current low warning            :  Off
    Laser output power high alarm             :  Off
    Laser output power low alarm              :  Off
    Laser output power high warning           :  Off
    Laser output power low warning            :  Off
    Module temperature high alarm             :  Off
    Module temperature low alarm              :  Off
    Module temperature high warning           :  Off
    Module temperature low warning            :  Off
    Module voltage high alarm                 :  Off
    Module voltage low alarm                  :  Off
    Module voltage high warning               :  Off
    Module voltage low warning                :  Off
    Laser rx power high alarm                 :  Off
    Laser rx power low alarm                  :  Off
    Laser rx power high warning               :  Off
    Laser rx power low warning                :  Off
    Laser bias current high alarm threshold   :  100.000 mA
    Laser bias current low alarm threshold    :  2.000 mA
    Laser bias current high warning threshold :  80.000 mA
    Laser bias current low warning threshold  :  4.000 mA
    Laser output power high alarm threshold   :  2.2440 mW / 3.51 dBm
    Laser output power low alarm threshold    :  0.0792 mW / -11.01 dBm
    Laser output power high warning threshold :  1.6830 mW / 2.26 dBm
    Laser output power low warning threshold  :  0.1188 mW / -9.25 dBm
    Module temperature high alarm threshold   :  125 degrees C / 257 degrees F
    Module temperature low alarm threshold    :  -45 degrees C / -49 degrees F
    Module temperature high warning threshold :  115 degrees C / 239 degrees F
    Module temperature low warning threshold  :  -40 degrees C / -40 degrees F
    Module voltage high alarm threshold       :  3.9000 V
    Module voltage low alarm threshold        :  2.7000 V
    Module voltage high warning threshold     :  3.7000 V
    Module voltage low warning threshold      :  2.9000 V
    Laser rx power high alarm threshold       :  2.0000 mW / 3.01 dBm
    Laser rx power low alarm threshold        :  0.0158 mW / -18.01 dBm
    Laser rx power high warning threshold     :  1.5000 mW / 1.76 dBm
    Laser rx power low warning threshold      :  0.0237 mW / -16.25 dBm

Best regards,
-- 
Aurélien Guillaume

On Fri, 2012-11-16 at 03:23 +0100, Aurélien wrote:

On Fri, Nov 16, 2012 at 12:30 AM, Ben Hutchings
[off-list ref] wrote:

quoted

Yes, Jeff's the one you should be talking to about these drivers.  I
just look after the ethtool utility and API.

Ok, so I will discuss the ixgbe patch with Jeff :)

Aurélien-

As far as your driver changes go and the questions you have, Don
Skidmore (ixgbe maintainer) and I have been discussing your patch and
may have some implementation changes possibly to suggest.  I will talk
with Don more tomorrow about it before I suggest any changes.

As far as your questions regarding ixgbe PHY's, I will have Don respond
your questions.  I have CC'd Don [off-list ref] as well
on this email thread.

Cheers,
Jeff

On Fri, 2012-11-16 at 03:23 +0100, Aurélien wrote:

On Fri, Nov 16, 2012 at 12:30 AM, Ben Hutchings
[off-list ref] wrote:

quoted

Yes, Jeff's the one you should be talking to about these drivers.  I
just look after the ethtool utility and API.

Ok, so I will discuss the ixgbe patch with Jeff :)

Ben, on the ethtool side, attached is a patch to enable the following
option and output; It's still missing externally calibrated optics
support (my current one is internally calibrated, so that's difficult
to test anything). What do you think ? Is there any other data that
could be interesting to show with -O or -m options ?

[...]

This is silly; log10() and <math.h> are part of standard C and -lm is
standard on Unix.  Just use <math.h> and -lm unconditionally.

Please merge this with the existing -m option and update the
documentation to say that this covers diagnostics where available.  You
could add a long option alias like --dump-module or --module-info that
covers the two types of information.

[...]

+#define SFF_A2_TEMP                       0x100 + 96
+#define SFF_A2_TEMP_HALRM                 0x100 + 0

[...]

+#define SFF_A2_ALRM_FLG                   0x100 + 112
+#define SFF_A2_WARN_FLG                   0x100 + 116

All the above offsets need parentheses around their definitions.

+struct sff8472_diags {
+
+#define MCURR 0
+#define LWARN 1
+#define HWARN 2
+#define LALRM 3
+#define HALRM 4
+
+       /* [5] tables are current, low/high warn, low/high alarm */
+       __u8 supports_dom;      /* Supports DOM */
+       __u8 supports_alarms;   /* Supports alarm/warning thold */
+       __u8 calibrated_int;    /* Is internally calibrated */
+       __u16 bias_cur[5];              /* Measured bias current in 2uA units (cur, l/h warn, l/h alarm) */
+       __u16 tx_power[5];              /* Measured TX Power in 0.1uW units (cur, warn, alarm) */
+       __u16 rx_power[5];              /* Measured RX Power (cur, warn, alarm) */
+       __u8  rx_power_type;    /* 0 = OMA, 1 = Average power */
+       __s16 sfp_temp[5];      /* SFP Temp in 0.1 Celcius (cur, warn, alarm) */
+       __u16 sfp_voltage[5];   /* SFP voltage in 0.1mV units (cur, warn, alarm) */
+
+};
+
+static struct sff8472_aw_flags {
+       const char *str;        /* Human-readable string, null at the end */
+       int offset;             /* A2-relative adress offset */

This is commented as an offset in the A2 'EEPROM' but the offsets
actually used include the 0x100 offset from the start of the
concatenated 'EEPROM'.

+       __u8 value;             /* 1-bit mask, alarm is on if offset & value != 0. */
+} sff8472_aw_flags[] =
+{
+       { "Laser bias current high alarm",   SFF_A2_ALRM_FLG, (1 << 3) },
+       { "Laser bias current low alarm",    SFF_A2_ALRM_FLG, (1 << 2) },
+       { "Laser bias current high warning", SFF_A2_WARN_FLG, (1 << 3) },
+       { "Laser bias current low warning",  SFF_A2_WARN_FLG, (1 << 2) },
+
+       { "Laser output power high alarm",   SFF_A2_ALRM_FLG, (1 << 1) },
+       { "Laser output power low alarm",    SFF_A2_ALRM_FLG, (1 << 0) },
+       { "Laser output power high warning", SFF_A2_WARN_FLG, (1 << 1) },
+       { "Laser output power low warning",  SFF_A2_WARN_FLG, (1 << 0) },
+
+       { "Module temperature high alarm",   SFF_A2_ALRM_FLG, (1 << 7) },
+       { "Module temperature low alarm",    SFF_A2_ALRM_FLG, (1 << 6) },
+       { "Module temperature high warning", SFF_A2_WARN_FLG, (1 << 7) },
+       { "Module temperature low warning",  SFF_A2_WARN_FLG, (1 << 6) },
+
+       { "Module voltage high alarm",   SFF_A2_ALRM_FLG, (1 << 5) },
+       { "Module voltage low alarm",    SFF_A2_ALRM_FLG, (1 << 4) },
+       { "Module voltage high warning", SFF_A2_WARN_FLG, (1 << 5) },
+       { "Module voltage low warning",  SFF_A2_WARN_FLG, (1 << 4) },
+
+       { "Laser rx power high alarm",   SFF_A2_ALRM_FLG + 1, (1 << 7) },
+       { "Laser rx power low alarm",    SFF_A2_ALRM_FLG + 1, (1 << 6) },
+       { "Laser rx power high warning", SFF_A2_WARN_FLG + 1, (1 << 7) },
+       { "Laser rx power low warning",  SFF_A2_WARN_FLG + 1, (1 << 6) },
+
+       { NULL, 0, 0 },
+};
+
+#ifdef HAVE_LIBM
+
+static double convert_mw_to_dbm(double mw)
+{
+       return (10.f * log10(mw / 1000.f)) + 30.f;

Why are all the literals explicitly float and not double?

+}
+
+#endif
+
+/* Externally calibrated SFP calculations */
+#define ECAL(v, s, o) (( ((double) (s>>8)) + (s & 0xFF)) * (double) v + o)     

Please follow kernel coding style for spacing.  checkpatch.pl will show
you what should be changed.

+static void sff8472_parse_eeprom(const __u8 *id, struct sff8472_diags *sd)
+{
+       sd->supports_dom = id[SFF_A0_DOM] & SFF_A0_DOM_IMPL;
+       sd->supports_alarms = id[SFF_A0_OPTIONS] & SFF_A0_OPTIONS_AW;
+       sd->calibrated_int = id[SFF_A0_DOM] & SFF_A0_DOM_INTCAL;
+       sd->rx_power_type = id[SFF_A0_DOM] & SFF_A0_DOM_PWRT;
+
+
+#define OFFSET_TO_U16(offset) (id[(offset)] << 8 | id[(offset) + 1]) 
+
+       sd->bias_cur[MCURR] = OFFSET_TO_U16(SFF_A2_BIAS);
+       sd->bias_cur[HALRM] = OFFSET_TO_U16(SFF_A2_BIAS_HALRM);
+       sd->bias_cur[LALRM] = OFFSET_TO_U16(SFF_A2_BIAS_LALRM);
+       sd->bias_cur[HWARN] = OFFSET_TO_U16(SFF_A2_BIAS_HWARN);
+       sd->bias_cur[LWARN] = OFFSET_TO_U16(SFF_A2_BIAS_LWARN);
+
+       sd->sfp_voltage[MCURR] = OFFSET_TO_U16(SFF_A2_VCC);
+       sd->sfp_voltage[HALRM] = OFFSET_TO_U16(SFF_A2_VCC_HALRM);
+       sd->sfp_voltage[LALRM] = OFFSET_TO_U16(SFF_A2_VCC_LALRM);
+       sd->sfp_voltage[HWARN] = OFFSET_TO_U16(SFF_A2_VCC_HWARN);
+       sd->sfp_voltage[LWARN] = OFFSET_TO_U16(SFF_A2_VCC_LWARN);
+
+       sd->tx_power[MCURR] = OFFSET_TO_U16(SFF_A2_TX_PWR);
+       sd->tx_power[HALRM] = OFFSET_TO_U16(SFF_A2_TX_PWR_HALRM);
+       sd->tx_power[LALRM] = OFFSET_TO_U16(SFF_A2_TX_PWR_LALRM);
+       sd->tx_power[HWARN] = OFFSET_TO_U16(SFF_A2_TX_PWR_HWARN);
+       sd->tx_power[LWARN] = OFFSET_TO_U16(SFF_A2_TX_PWR_LWARN);
+
+       sd->rx_power[MCURR] = OFFSET_TO_U16(SFF_A2_RX_PWR);
+       sd->rx_power[HALRM] = OFFSET_TO_U16(SFF_A2_RX_PWR_HALRM);
+       sd->rx_power[LALRM] = OFFSET_TO_U16(SFF_A2_RX_PWR_LALRM);
+       sd->rx_power[HWARN] = OFFSET_TO_U16(SFF_A2_RX_PWR_HWARN);
+       sd->rx_power[LWARN] = OFFSET_TO_U16(SFF_A2_RX_PWR_LWARN);
+
+       /* Temperature conversions */
+#define OFFSET_TO_TEMP(offset) \
+    ((*(__s8 *)(&id[(offset)])) * 1000 + ((id[(offset) + 1] * 1000) / 256)) / 100;

This seems awfuly complicated; why not:

#define OFFSET_TO_TEMP(offset) (((s16)OFFSET_TO_U16(offset)) * 10 / 256)

But why round to tenths of a degree here and then round again to whole
degrees celsius/fahrenheit when printing?

[...]

+#define PRINT_TEMP(string, index) \
+       printf("    %-41s :  %.0f degrees C / %.0f degrees F\n", (string), \
+                  (double)(sd.sfp_temp[(index)] / 10.f), \
+                  (double)(sd.sfp_temp[(index)] / 10.f * 1.8f + 32.f));

[...]

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Hi Ben,

I've rewritten things according to your remarks.

On Fri, Nov 16, 2012 at 8:38 PM, Ben Hutchings
[off-list ref] wrote:

This is silly; log10() and <math.h> are part of standard C and -lm is
standard on Unix.  Just use <math.h> and -lm unconditionally.

Ok, I wasn't sure.

Please merge this with the existing -m option and update the
documentation to say that this covers diagnostics where available.  You
could add a long option alias like --dump-module or --module-info that
covers the two types of information.

Done that, the current output of -m has been modified so that
everything lines up correctly.
The --module-info option alias has been added.

All the above offsets need parentheses around their definitions.

[…]

This is commented as an offset in the A2 'EEPROM' but the offsets
actually used include the 0x100 offset from the start of the
concatenated 'EEPROM'.

A new SFF_A2_BASE has been added, and the OFFSET_TO macros are now
using that, so I removed the 0x100 from all the offsets, and they are
now indeed A2-relative.

Why are all the literals explicitly float and not double?

It was a keyboard/chair interface problem, now fixed :)

Please follow kernel coding style for spacing.  checkpatch.pl will show
you what should be changed.

Ran a checkpatch, and fixed everything that should be fixed.

This seems awfuly complicated; why not:

#define OFFSET_TO_TEMP(offset) (((s16)OFFSET_TO_U16(offset)) * 10 / 256)

But why round to tenths of a degree here and then round again to whole
degrees celsius/fahrenheit when printing?

I did not think a simple cast would work, but it seems to give the
right value. I also implemented externally calibrated optics in this
new version, so I now do the whole formatting in the printing, and
store the raw value in the struct.

It should be better now.

Best regards,
-- 
Aurélien Guillaume

Hi,
FYI. DOM use in Serengeti Tanzania (Bunda-Nata 60km) on solar driven low-power 
linux atom router @ 20Watt w. igb driver using the older DOM pathes. Very useful 
stuff. Yes get included in the kernel.

NATA:/# ethtool -D eth1
Ext-Calbr: Avr RX-Power: Alarm & Warn: RX_LOS: 	Wavelength: 1550 nm
Alarms, warnings in beginning of line, Ie. AH = Alarm High, WL == Warn Low etc
	Temp:  76.2 C			Thresh: Lo: -50.0/-48.0  Hi:  95.0/110.0 C
	Vcc:  3.27 V			Thresh: Lo:   2.9/3.0    Hi:   3.5/3.6   V
	Tx-Bias:  27.9 mA		Thresh: Lo:   3.0/5.0    Hi:  90.0/100.0 mA
	TX-pwr:   3.8 dBm ( 2.39 mW)	Thresh: Lo:  -5.0/-4.0   Hi:   5.0/6.0   dBm
	RX-pwr: -17.3 dBm ( 0.02 mW) 	Thresh: Lo: -40.0/-37.0  Hi:  -5.0/-3.0  dBm

http://herjulf.se/robert/tanzania-2012/Nata-installation-2.jpg

						--ro

On Sun, 2012-11-18 at 22:35 +0100, Aurélien wrote:

Hi Ben,

I've rewritten things according to your remarks.

On Fri, Nov 16, 2012 at 8:38 PM, Ben Hutchings
[off-list ref] wrote:

quoted

This is silly; log10() and <math.h> are part of standard C and -lm is
standard on Unix.  Just use <math.h> and -lm unconditionally.

Ok, I wasn't sure.

This version drops the -lm completely, so it doesn't link.  Maybe you
edited the generated Makefile or Makefile.in?

quoted

Please merge this with the existing -m option and update the
documentation to say that this covers diagnostics where available.  You
could add a long option alias like --dump-module or --module-info that
covers the two types of information.

Done that, the current output of -m has been modified so that
everything lines up correctly.
The --module-info option alias has been added.

The option alias should be included in the manual page and in a
(trivial) test case in test-cmdline.c.

[...]

quoted

Please follow kernel coding style for spacing.  checkpatch.pl will show
you what should be changed.

Ran a checkpatch, and fixed everything that should be fixed.

The indentation is still weird, though:

[...]

[...]

+struct sff8472_diags {
+
+#define MCURR 0
+#define LWARN 1
+#define HWARN 2
+#define LALRM 3
+#define HALRM 4
+
+       /* [5] tables are current, low/high warn, low/high alarm */
+       __u8 supports_dom;      /* Supports DOM */
+       __u8 supports_alarms;   /* Supports alarm/warning thold */
+       __u8 calibrated_ext;    /* Is externally calibrated */
+       __u16 bias_cur[5];              /* Measured bias current in 2uA units */
+       __u16 tx_power[5];              /* Measured TX Power in 0.1uW units */
+       __u16 rx_power[5];              /* Measured RX Power */

These comments should be lined up vertically.

[...]

+/* Converts to a float from a big-endian 4-byte source buffer. */
+static float befloattoh(const __u32 *source)
+{
+       union {
+               __u32 src;
+               float dst;
+       } converter;
+
+       converter.src = be32toh(*source);

be32toh() is non-standard and was apparently added to glibc relatively
recently (version 2.9).  Therefore please use the equivalent ntohl()
instead.

[...]

+static void sff8472_parse_eeprom(const __u8 *id, struct sff8472_diags *sd)
+{
+       sd->supports_dom = id[SFF_A0_DOM] & SFF_A0_DOM_IMPL;
+       sd->supports_alarms = id[SFF_A0_OPTIONS] & SFF_A0_OPTIONS_AW;
+       sd->calibrated_ext = id[SFF_A0_DOM] & SFF_A0_DOM_EXTCAL;
+       sd->rx_power_type = id[SFF_A0_DOM] & SFF_A0_DOM_PWRT;
+
+       sff8472_dom_parse(id, sd);
+
+       /*
+        * If the SFP is externally calibrated, we need to read calibration data
+        * and compensate the already stored readings.
+        */
+       if (sd->calibrated_ext)
+               sff8472_calibration(id, sd);
+}
+
+
+

One line between functions is enough.

+void sff8472_show_all(const __u8 *id)
+{
+       struct sff8472_diags sd;
+       char *rx_power_string = NULL;
+       int i;
+
+       sff8472_parse_eeprom(id, &sd);
+
+       if (!sd.supports_dom) {
+               printf("\t%-41s : No\n", "Optical diagnostics support");
+               return ;
+       }
+       printf("\t%-41s : Yes\n", "Optical diagnostics support");
+
+#define PRINT_BIAS(string, index) \
+       printf("\t%-41s : %.3f mA\n", (string), \
+                  (double)(sd.bias_cur[(index)] / 500.));
+
+# define PRINT_xX_PWR(string, var, index) \
+       printf("\t%-41s : %.4f mW / %.2f dBm\n", (string), \
+                  (double)((var)[(index)] / 10000.), \
+                  convert_mw_to_dbm((double)((var)[(index)] / 10000.)));
+
+#define PRINT_TEMP(string, index) \
+       printf("\t%-41s : %.2f degrees C / %.2f degrees F\n", (string), \
+                  (double)(sd.sfp_temp[(index)] / 256.), \
+                  (double)(sd.sfp_temp[(index)] / 256. * 1.8 + 32.));
+
+#define PRINT_VCC(string, index) \
+       printf("\t%-41s : %.4f V\n", (string), \
+                  (double)(sd.sfp_voltage[(index)] / 10000.));

Function-like macros generally shouldn't be defined with a trailing
semi-colon, as that will be added at the point of use.

The backslashes should be lined up on the right, and continuation lines
within parentheses should be indented so they begin just to the right of
the opening parenthesis, e.g.:

#define PRINT_VCC(string, index)				\
	printf("\t%-41s : %.4f V\n", (string),			\
	       (double)(sd.sfp_voltage[(index)] / 10000.))

[...]

+               PRINT_xX_PWR("Laser output power high alarm threshold",
+                                        sd.tx_power, HALRM);
+               PRINT_xX_PWR("Laser output power low alarm threshold",
+                                        sd.tx_power, LALRM);
+               PRINT_xX_PWR("Laser output power high warning threshold",
+                                        sd.tx_power, HWARN);
+               PRINT_xX_PWR("Laser output power low warning threshold",
+                                        sd.tx_power, LWARN);

The continuation lines are over-indented here.

[...]

+               PRINT_xX_PWR("Laser rx power high alarm threshold",
+                                        sd.rx_power, HALRM);
+               PRINT_xX_PWR("Laser rx power low alarm threshold",
+                                        sd.rx_power, LALRM);
+               PRINT_xX_PWR("Laser rx power high warning threshold",
+                                        sd.rx_power, HWARN);
+               PRINT_xX_PWR("Laser rx power low warning threshold",
+                                        sd.rx_power, LWARN);
+       }

Same here.

[...]

If you want to change these labels, do that in a separate patch.

[...]

These changes are unnecessary.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Hi Ben,

Thanks for your review. Here's a fixed-up version, according to your remarks.

On Sat, Dec 1, 2012 at 5:18 AM, Ben Hutchings [off-list ref] wrote:

This version drops the -lm completely, so it doesn't link.  Maybe you
edited the generated Makefile or Makefile.in?

No, I just stupidly forgot to make distclean & autogen after removing
all libm checks. Re-added AC_CHECK_LIB to link with it.

The option alias should be included in the manual page and in a
(trivial) test case in test-cmdline.c.

Included in the man page, along with a modified option description,
and a test equivalent to --dump-module-eeprom, which passes.

The indentation is still weird, though:

[...]

These comments should be lined up vertically.

Yup, I had a mixup between tab/whitespaces, and my Vim config did not
help. Fixed.

be32toh() is non-standard and was apparently added to glibc relatively
recently (version 2.9).  Therefore please use the equivalent ntohl()
instead.

Did that, it indeed works just fine. I should have used that from the start.

Function-like macros generally shouldn't be defined with a trailing
semi-colon, as that will be added at the point of use.

That was a copy/paste typo, fixed.

The backslashes should be lined up on the right, and continuation lines
within parentheses should be indented so they begin just to the right of
the opening parenthesis, e.g.:

#define PRINT_VCC(string, index)                                \
        printf("\t%-41s : %.4f V\n", (string),                  \
               (double)(sd.sfp_voltage[(index)] / 10000.))

[...]

quoted

+               PRINT_xX_PWR("Laser output power low warning threshold",
+                                        sd.tx_power, LWARN);

The continuation lines are over-indented here.

Fixed (was using wrong tabstop width at 4).

quoted

-               printf("\tActive Cu cmplnce.  : 0x%02x", id[60]);
+               printf("\t%-41s : 0x%02x", "Active copper compliance", id[60]);

If you want to change these labels, do that in a separate patch.

There's no real need, so I'll leave those alone, just changing the
alignment like for the other labels.

[...]

quoted

-                                            "Length (62.5um)", 10, "m");
+                               "Length (62.5um)", 10, "m");

These changes are unnecessary.

Agreed, removed from the patch.

Best regards,
-- 
Aurélien Guillaume

On Sun, Dec 2, 2012 at 10:47 PM, Aurélien [off-list ref] wrote:

quoted

This version drops the -lm completely, so it doesn't link.  Maybe you
edited the generated Makefile or Makefile.in?

No, I just stupidly forgot to make distclean & autogen after removing
all libm checks. Re-added AC_CHECK_LIB to link with it.

Just after re-reading this, I thought it was silly; I instead edited
Makefile.am and reverted the configure.ac change. Here's a new
full-patch with that fix along with the rest.

Sorry for the noise.
-- 
Aurélien Guillaume

On Sun, 2012-12-02 at 23:00 +0100, Aurélien wrote:

On Sun, Dec 2, 2012 at 10:47 PM, Aurélien [off-list ref] wrote:

quoted

quoted

This version drops the -lm completely, so it doesn't link.  Maybe you
edited the generated Makefile or Makefile.in?

No, I just stupidly forgot to make distclean & autogen after removing
all libm checks. Re-added AC_CHECK_LIB to link with it.

Just after re-reading this, I thought it was silly; I instead edited
Makefile.am and reverted the configure.ac change. Here's a new
full-patch with that fix along with the rest.

Sorry for the noise.

Applied, thanks.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.