Re: Optics (SFP) monitoring on ixgbe and igbe

[Aurélien <footplus@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Sun, Dec 2, 2012 at 10:47 PM, Aurélien <footplus@gmail.com> 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.
>

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

[-- Attachment #2: 0001-Implemented-basic-optics-diagnostics-for-SFF-8472.patch --]
[-- Type: application/octet-stream, Size: 22479 bytes --]

From c6e58988c0c30123f78e0ae83730697bb2b159d0 Mon Sep 17 00:00:00 2001
From: Aurelien Guillaume <aurelien@iwi.me>
Date: Sun, 2 Dec 2012 21:21:01 +0100
Subject: [PATCH] Implemented basic optics diagnostics for SFF-8472


Signed-off-by: Aurelien Guillaume <aurelien@iwi.me>
---
 Makefile.am    |    3 +-
 ethtool.8.in   |    8 +-
 ethtool.c      |   17 +++-
 internal.h     |    3 +
 sfpdiag.c      |  362 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 sfpid.c        |   28 +++--
 test-cmdline.c |    2 +
 7 files changed, 404 insertions(+), 19 deletions(-)
 create mode 100644 sfpdiag.c

diff --git a/Makefile.am b/Makefile.am
index e33f71f..ba1faa6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,4 +1,5 @@
 AM_CFLAGS = -Wall
+LDADD = -lm
 
 man_MANS = ethtool.8
 EXTRA_DIST = LICENSE ethtool.8 ethtool.spec.in aclocal.m4 ChangeLog autogen.sh
@@ -9,7 +10,7 @@ ethtool_SOURCES = ethtool.c ethtool-copy.h internal.h net_tstamp-copy.h \
 		  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	\
-		  rxclass.c sfpid.c
+		  rxclass.c sfpid.c sfpdiag.c
 
 TESTS = test-cmdline test-features
 check_PROGRAMS = test-cmdline test-features
diff --git a/ethtool.8.in b/ethtool.8.in
index a3c7fbb..e701919 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -312,7 +312,7 @@ ethtool \- query or control network driver and hardware settings
 .BN other
 .BN combined
 .HP
-.B ethtool \-m|\-\-dump\-module\-eeprom
+.B ethtool \-m|\-\-dump\-module\-eeprom|\-\-module\-info
 .I devname
 .B2 raw on off
 .B2 hex on off
@@ -815,8 +815,10 @@ Changes the number of channels used only for other purposes e.g. link interrupts
 .BI combined \ N
 Changes the number of multi-purpose channels.
 .TP
-.B \-m \-\-dump\-module\-eeprom
-Retrieves and if possible decodes the EEPROM from plugin modules, e.g SFP+, QSFP
+.B \-m \-\-dump\-module\-eeprom \-\-module\-info
+Retrieves and if possible decodes the EEPROM from plugin modules, e.g SFP+, QSFP.
+If the driver and module support it, the optical diagnostic information is also
+read and decoded.
 .TP
 .B \-\-show\-priv\-flags
 Queries the specified network device for its private flags.  The
diff --git a/ethtool.c b/ethtool.c
index 3db7fec..345c21c 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3604,6 +3604,16 @@ static int do_getmodule(struct cmd_context *ctx)
 		return 1;
 	}
 
+	/*
+	 * SFF-8079 EEPROM layout contains the memory available at A0 address on
+	 * the PHY EEPROM.
+	 * SFF-8472 defines a virtual extension of the EEPROM, where the
+	 * microcontroller on the SFP/SFP+ generates a page at the A2 address,
+	 * which contains data relative to optical diagnostics.
+	 * The current kernel implementation returns a blob, which contains:
+	 *  - ETH_MODULE_SFF_8079 => The A0 page only.
+	 *  - ETH_MODULE_SFF_8472 => The A0 and A2 page concatenated.
+	 */
 	if (geeprom_dump_raw) {
 		fwrite(eeprom->data, 1, eeprom->len, stdout);
 	} else {
@@ -3613,8 +3623,11 @@ static int do_getmodule(struct cmd_context *ctx)
 		} else if (!geeprom_dump_hex) {
 			switch (modinfo.type) {
 			case ETH_MODULE_SFF_8079:
+				sff8079_show_all(eeprom->data);
+				break;
 			case ETH_MODULE_SFF_8472:
 				sff8079_show_all(eeprom->data);
+				sff8472_show_all(eeprom->data);
 				break;
 			default:
 				geeprom_dump_hex = 1;
@@ -3831,8 +3844,8 @@ static const struct option {
 	{ "--show-priv-flags" , 1, do_gprivflags, "Query private flags" },
 	{ "--set-priv-flags", 1, do_sprivflags, "Set private flags",
 	  "		FLAG on|off ...\n" },
-	{ "-m|--dump-module-eeprom", 1, do_getmodule,
-	  "Qeuery/Decode Module EEPROM information",
+	{ "-m|--dump-module-eeprom|--module-info", 1, do_getmodule,
+	  "Query/Decode Module EEPROM information and optical diagnostics if available",
 	  "		[ raw on|off ]\n"
 	  "		[ hex on|off ]\n"
 	  "		[ offset N ]\n"
diff --git a/internal.h b/internal.h
index 4f96fd5..e977a81 100644
--- a/internal.h
+++ b/internal.h
@@ -253,4 +253,7 @@ int rxclass_rule_del(struct cmd_context *ctx, __u32 loc);
 /* Module EEPROM parsing code */
 void sff8079_show_all(const __u8 *id);
 
+/* Optics diagnostics */
+void sff8472_show_all(const __u8 *id);
+
 #endif /* ETHTOOL_INTERNAL_H__ */
diff --git a/sfpdiag.c b/sfpdiag.c
new file mode 100644
index 0000000..f67e491
--- /dev/null
+++ b/sfpdiag.c
@@ -0,0 +1,362 @@
+/*
+ * sfpdiag.c: Implements SFF-8472 optics diagnostics.
+ *
+ * Aurelien Guillaume <aurelien@iwi.me> (C) 2012
+ *   This implementation is loosely based on DOM patches
+ *   from Robert Olsson <robert@herjulf.se> (C) 2009
+ *   and SFF-8472 specs (ftp://ftp.seagate.com/pub/sff/SFF-8472.PDF)
+ *   by SFF Committee.
+ */
+
+#include <stdio.h>
+#include <math.h>
+#include <arpa/inet.h>
+#include "internal.h"
+
+/* Offsets in decimal, for direct comparison with the SFF specs */
+
+/* A0-based EEPROM offsets for DOM support checks */
+#define SFF_A0_DOM                        92
+#define SFF_A0_OPTIONS                    93
+#define SFF_A0_COMP                       94
+
+/* EEPROM bit values for various registers */
+#define SFF_A0_DOM_EXTCAL                 (1 << 4)
+#define SFF_A0_DOM_INTCAL                 (1 << 5)
+#define SFF_A0_DOM_IMPL                   (1 << 6)
+#define SFF_A0_DOM_PWRT                   (1 << 3)
+
+#define SFF_A0_OPTIONS_AW                 (1 << 7)
+
+/*
+ * See ethtool.c comments about SFF-8472, this is the offset
+ * at which the A2 page is in the EEPROM blob returned by the
+ * kernel.
+ */
+#define SFF_A2_BASE                       0x100
+
+/* A2-based offsets for DOM */
+#define SFF_A2_TEMP                       96
+#define SFF_A2_TEMP_HALRM                 0
+#define SFF_A2_TEMP_LALRM                 2
+#define SFF_A2_TEMP_HWARN                 4
+#define SFF_A2_TEMP_LWARN                 6
+
+#define SFF_A2_VCC                        98
+#define SFF_A2_VCC_HALRM                  8
+#define SFF_A2_VCC_LALRM                  10
+#define SFF_A2_VCC_HWARN                  12
+#define SFF_A2_VCC_LWARN                  14
+
+#define SFF_A2_BIAS                       96
+#define SFF_A2_BIAS_HALRM                 16
+#define SFF_A2_BIAS_LALRM                 18
+#define SFF_A2_BIAS_HWARN                 20
+#define SFF_A2_BIAS_LWARN                 22
+
+#define SFF_A2_TX_PWR                     102
+#define SFF_A2_TX_PWR_HALRM               24
+#define SFF_A2_TX_PWR_LALRM               26
+#define SFF_A2_TX_PWR_HWARN               28
+#define SFF_A2_TX_PWR_LWARN               30
+
+#define SFF_A2_RX_PWR                     104
+#define SFF_A2_RX_PWR_HALRM               32
+#define SFF_A2_RX_PWR_LALRM               34
+#define SFF_A2_RX_PWR_HWARN               36
+#define SFF_A2_RX_PWR_LWARN               38
+
+#define SFF_A2_ALRM_FLG                   112
+#define SFF_A2_WARN_FLG                   116
+
+/* 32-bit little-endian calibration constants */
+#define SFF_A2_CAL_RXPWR4                 56
+#define SFF_A2_CAL_RXPWR3                 60
+#define SFF_A2_CAL_RXPWR2                 64
+#define SFF_A2_CAL_RXPWR1                 68
+#define SFF_A2_CAL_RXPWR0                 72
+
+/* 16-bit little endian calibration constants */
+#define SFF_A2_CAL_TXI_SLP                76
+#define SFF_A2_CAL_TXI_OFF                78
+#define SFF_A2_CAL_TXPWR_SLP              80
+#define SFF_A2_CAL_TXPWR_OFF              82
+#define SFF_A2_CAL_T_SLP                  84
+#define SFF_A2_CAL_T_OFF                  86
+#define SFF_A2_CAL_V_SLP                  88
+#define SFF_A2_CAL_V_OFF                  90
+
+
+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 */
+	__u8  rx_power_type;    /* 0 = OMA, 1 = Average power */
+	__s16 sfp_temp[5];      /* SFP Temp in 16-bit signed 1/256 Celcius */
+	__u16 sfp_voltage[5];   /* SFP voltage in 0.1mV units */
+
+};
+
+static struct sff8472_aw_flags {
+	const char *str;        /* Human-readable string, null at the end */
+	int offset;             /* A2-relative adress offset */
+	__u8 value;             /* 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 },
+};
+
+static double convert_mw_to_dbm(double mw)
+{
+	return (10. * log10(mw / 1000.)) + 30.;
+}
+
+
+/* Most common case: 16-bit unsigned integer in a certain unit */
+#define A2_OFFSET_TO_U16(offset) \
+	(id[SFF_A2_BASE + (offset)] << 8 | id[SFF_A2_BASE + (offset) + 1])
+
+/* Calibration slope is a number between 0.0 included and 256.0 excluded. */
+#define A2_OFFSET_TO_SLP(offset) \
+	(id[SFF_A2_BASE + (offset)] + id[SFF_A2_BASE + (offset) + 1] / 256.)
+
+/* Calibration offset is an integer from -32768 to 32767 */
+#define A2_OFFSET_TO_OFF(offset) \
+	((__s16)A2_OFFSET_TO_U16(offset))
+
+/* RXPWR(x) are IEEE-754 floating point numbers in big-endian format */
+#define A2_OFFSET_TO_RXPWRx(offset) \
+	(befloattoh((__u32 *)(id + SFF_A2_BASE + (offset))))
+
+/*
+ * 2-byte internal temperature conversions:
+ * First byte is a signed 8-bit integer, which is the temp decimal part
+ * Second byte are 1/256th of degree, which are added to the dec part.
+ */
+#define A2_OFFSET_TO_TEMP(offset) ((__s16)A2_OFFSET_TO_U16(offset))
+
+
+static void sff8472_dom_parse(const __u8 *id, struct sff8472_diags *sd)
+{
+
+	sd->bias_cur[MCURR] = A2_OFFSET_TO_U16(SFF_A2_BIAS);
+	sd->bias_cur[HALRM] = A2_OFFSET_TO_U16(SFF_A2_BIAS_HALRM);
+	sd->bias_cur[LALRM] = A2_OFFSET_TO_U16(SFF_A2_BIAS_LALRM);
+	sd->bias_cur[HWARN] = A2_OFFSET_TO_U16(SFF_A2_BIAS_HWARN);
+	sd->bias_cur[LWARN] = A2_OFFSET_TO_U16(SFF_A2_BIAS_LWARN);
+
+	sd->sfp_voltage[MCURR] = A2_OFFSET_TO_U16(SFF_A2_VCC);
+	sd->sfp_voltage[HALRM] = A2_OFFSET_TO_U16(SFF_A2_VCC_HALRM);
+	sd->sfp_voltage[LALRM] = A2_OFFSET_TO_U16(SFF_A2_VCC_LALRM);
+	sd->sfp_voltage[HWARN] = A2_OFFSET_TO_U16(SFF_A2_VCC_HWARN);
+	sd->sfp_voltage[LWARN] = A2_OFFSET_TO_U16(SFF_A2_VCC_LWARN);
+
+	sd->tx_power[MCURR] = A2_OFFSET_TO_U16(SFF_A2_TX_PWR);
+	sd->tx_power[HALRM] = A2_OFFSET_TO_U16(SFF_A2_TX_PWR_HALRM);
+	sd->tx_power[LALRM] = A2_OFFSET_TO_U16(SFF_A2_TX_PWR_LALRM);
+	sd->tx_power[HWARN] = A2_OFFSET_TO_U16(SFF_A2_TX_PWR_HWARN);
+	sd->tx_power[LWARN] = A2_OFFSET_TO_U16(SFF_A2_TX_PWR_LWARN);
+
+	sd->rx_power[MCURR] = A2_OFFSET_TO_U16(SFF_A2_RX_PWR);
+	sd->rx_power[HALRM] = A2_OFFSET_TO_U16(SFF_A2_RX_PWR_HALRM);
+	sd->rx_power[LALRM] = A2_OFFSET_TO_U16(SFF_A2_RX_PWR_LALRM);
+	sd->rx_power[HWARN] = A2_OFFSET_TO_U16(SFF_A2_RX_PWR_HWARN);
+	sd->rx_power[LWARN] = A2_OFFSET_TO_U16(SFF_A2_RX_PWR_LWARN);
+
+	sd->sfp_temp[MCURR] = A2_OFFSET_TO_TEMP(SFF_A2_TEMP);
+	sd->sfp_temp[HALRM] = A2_OFFSET_TO_TEMP(SFF_A2_TEMP_HALRM);
+	sd->sfp_temp[LALRM] = A2_OFFSET_TO_TEMP(SFF_A2_TEMP_LALRM);
+	sd->sfp_temp[HWARN] = A2_OFFSET_TO_TEMP(SFF_A2_TEMP_HWARN);
+	sd->sfp_temp[LWARN] = A2_OFFSET_TO_TEMP(SFF_A2_TEMP_LWARN);
+
+}
+
+/* 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 = ntohl(*source);
+	return converter.dst;
+}
+
+static void sff8472_calibration(const __u8 *id, struct sff8472_diags *sd)
+{
+	int i;
+	__u16 rx_reading;
+
+	/* Calibration should occur for all values (threshold and current) */
+	for (i = 0; i < sizeof(sd->bias_cur); ++i) {
+		/*
+		 * Apply calibration formula 1 (Temp., Voltage, Bias, Tx Power)
+		 */
+		sd->bias_cur[i]    *= A2_OFFSET_TO_SLP(SFF_A2_CAL_TXI_SLP);
+		sd->tx_power[i]    *= A2_OFFSET_TO_SLP(SFF_A2_CAL_TXPWR_SLP);
+		sd->sfp_voltage[i] *= A2_OFFSET_TO_SLP(SFF_A2_CAL_V_SLP);
+		sd->sfp_temp[i]    *= A2_OFFSET_TO_SLP(SFF_A2_CAL_T_SLP);
+
+		sd->bias_cur[i]    += A2_OFFSET_TO_OFF(SFF_A2_CAL_TXI_OFF);
+		sd->tx_power[i]    += A2_OFFSET_TO_OFF(SFF_A2_CAL_TXPWR_OFF);
+		sd->sfp_voltage[i] += A2_OFFSET_TO_OFF(SFF_A2_CAL_V_OFF);
+		sd->sfp_temp[i]    += A2_OFFSET_TO_OFF(SFF_A2_CAL_T_OFF);
+
+		/*
+		 * Apply calibration formula 2 (Rx Power only)
+		 */
+		rx_reading = sd->rx_power[i];
+		sd->rx_power[i]    = A2_OFFSET_TO_RXPWRx(SFF_A2_CAL_RXPWR0);
+		sd->rx_power[i]    += rx_reading *
+			A2_OFFSET_TO_RXPWRx(SFF_A2_CAL_RXPWR1);
+		sd->rx_power[i]    += rx_reading *
+			A2_OFFSET_TO_RXPWRx(SFF_A2_CAL_RXPWR2);
+		sd->rx_power[i]    += rx_reading *
+			A2_OFFSET_TO_RXPWRx(SFF_A2_CAL_RXPWR3);
+	}
+}
+
+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);
+}
+
+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.))
+
+	PRINT_BIAS("Laser bias current", MCURR);
+	PRINT_xX_PWR("Laser output power", sd.tx_power, MCURR);
+
+	if (!sd.rx_power_type)
+		rx_power_string = "Receiver signal OMA";
+	else
+		rx_power_string = "Receiver signal average optical power";
+
+	PRINT_xX_PWR(rx_power_string, sd.rx_power, MCURR);
+
+	PRINT_TEMP("Module temperature", MCURR);
+	PRINT_VCC("Module voltage", MCURR);
+
+	printf("\t%-41s : %s\n", "Alarm/warning flags implemented",
+	       (sd.supports_alarms ? "Yes" : "No"));
+	if (sd.supports_alarms) {
+
+		for (i = 0; sff8472_aw_flags[i].str; ++i) {
+			printf("\t%-41s : %s\n", sff8472_aw_flags[i].str,
+			       id[SFF_A2_BASE + sff8472_aw_flags[i].offset]
+			       & sff8472_aw_flags[i].value ? "On" : "Off");
+		}
+
+		PRINT_BIAS("Laser bias current high alarm threshold",   HALRM);
+		PRINT_BIAS("Laser bias current low alarm threshold",    LALRM);
+		PRINT_BIAS("Laser bias current high warning threshold", HWARN);
+		PRINT_BIAS("Laser bias current low warning threshold",  LWARN);
+
+		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);
+
+		PRINT_TEMP("Module temperature high alarm threshold",   HALRM);
+		PRINT_TEMP("Module temperature low alarm threshold",    LALRM);
+		PRINT_TEMP("Module temperature high warning threshold", HWARN);
+		PRINT_TEMP("Module temperature low warning threshold",  LWARN);
+
+		PRINT_VCC("Module voltage high alarm threshold",   HALRM);
+		PRINT_VCC("Module voltage low alarm threshold",    LALRM);
+		PRINT_VCC("Module voltage high warning threshold", HWARN);
+		PRINT_VCC("Module voltage low warning threshold",  LWARN);
+
+		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);
+	}
+
+}
+
diff --git a/sfpid.c b/sfpid.c
index a4a671d..4f88aa2 100644
--- a/sfpid.c
+++ b/sfpid.c
@@ -12,7 +12,7 @@
 
 static void sff8079_show_identifier(const __u8 *id)
 {
-	printf("\tIdentifier          : 0x%02x", id[0]);
+	printf("\t%-41s : 0x%02x", "Identifier", id[0]);
 	switch (id[0]) {
 	case 0x00:
 		printf(" (no module present, unknown, or unspecified)\n");
@@ -34,7 +34,7 @@ static void sff8079_show_identifier(const __u8 *id)
 
 static void sff8079_show_ext_identifier(const __u8 *id)
 {
-	printf("\tExtended identifier : 0x%02x", id[1]);
+	printf("\t%-41s : 0x%02x", "Extended identifier", id[1]);
 	if (id[1] == 0x00)
 		printf(" (GBIC not specified / not MOD_DEF compliant)\n");
 	else if (id[1] == 0x04)
@@ -47,7 +47,7 @@ static void sff8079_show_ext_identifier(const __u8 *id)
 
 static void sff8079_show_connector(const __u8 *id)
 {
-	printf("\tConnector           : 0x%02x", id[2]);
+	printf("\t%-41s : 0x%02x", "Connector", id[2]);
 	switch (id[2]) {
 	case 0x00:
 		printf(" (unknown or unspecified)\n");
@@ -105,10 +105,12 @@ static void sff8079_show_connector(const __u8 *id)
 
 static void sff8079_show_transceiver(const __u8 *id)
 {
-	static const char *pfx = "\t                    :  =>";
+	static const char *pfx =
+		"\tTransceiver type                          :";
 
-	printf("\tTransceiver codes   : 0x%02x 0x%02x 0x%02x" \
+	printf("\t%-41s : 0x%02x 0x%02x 0x%02x " \
 	       "0x%02x 0x%02x 0x%02x 0x%02x 0x%02x\n",
+		   "Transceiver codes",
 	       id[3], id[4], id[5], id[6],
 	       id[7], id[8], id[9], id[10]);
 	/* 10G Ethernet Compliance Codes */
@@ -239,7 +241,7 @@ static void sff8079_show_transceiver(const __u8 *id)
 
 static void sff8079_show_encoding(const __u8 *id)
 {
-	printf("\tEncoding            : 0x%02x", id[11]);
+	printf("\t%-41s : 0x%02x", "Encoding", id[11]);
 	switch (id[11]) {
 	case 0x00:
 		printf(" (unspecified)\n");
@@ -270,7 +272,7 @@ static void sff8079_show_encoding(const __u8 *id)
 
 static void sff8079_show_rate_identifier(const __u8 *id)
 {
-	printf("\tRate identifier     : 0x%02x", id[13]);
+	printf("\t%-41s : 0x%02x", "Rate identifier", id[13]);
 	switch (id[13]) {
 	case 0x00:
 		printf(" (unspecified)\n");
@@ -295,14 +297,14 @@ static void sff8079_show_rate_identifier(const __u8 *id)
 
 static void sff8079_show_oui(const __u8 *id)
 {
-	printf("\tVendor OUI          : %02x:%02x:%02x\n",
+	printf("\t%-41s : %02x:%02x:%02x\n", "Vendor OUI",
 	       id[37], id[38], id[39]);
 }
 
 static void sff8079_show_wavelength_or_copper_compliance(const __u8 *id)
 {
 	if (id[8] & (1 << 2)) {
-		printf("\tPassive Cu cmplnce. : 0x%02x", id[60]);
+		printf("\t%-41s : 0x%02x", "Passive Cu cmplnce.", id[60]);
 		switch (id[60]) {
 		case 0x00:
 			printf(" (unspecified)");
@@ -316,7 +318,7 @@ static void sff8079_show_wavelength_or_copper_compliance(const __u8 *id)
 		}
 		printf(" [SFF-8472 rev10.4 only]\n");
 	} else if (id[8] & (1 << 3)) {
-		printf("\tActive Cu cmplnce.  : 0x%02x", id[60]);
+		printf("\t%-41s : 0x%02x", "Active Cu cmplnce.", id[60]);
 		switch (id[60]) {
 		case 0x00:
 			printf(" (unspecified)");
@@ -333,7 +335,7 @@ static void sff8079_show_wavelength_or_copper_compliance(const __u8 *id)
 		}
 		printf(" [SFF-8472 rev10.4 only]\n");
 	} else {
-		printf("\tLaser wavelength    : %unm\n",
+		printf("\t%-41s : %unm\n", "Laser wavelength",
 		       (id[60] << 8) | id[61]);
 	}
 }
@@ -344,7 +346,7 @@ static void sff8079_show_value_with_unit(const __u8 *id, unsigned int reg,
 {
 	unsigned int val = id[reg];
 
-	printf("\t%-20s: %u%s\n", name, val * mult, unit);
+	printf("\t%-41s : %u%s\n", name, val * mult, unit);
 }
 
 static void sff8079_show_ascii(const __u8 *id, unsigned int first_reg,
@@ -352,7 +354,7 @@ static void sff8079_show_ascii(const __u8 *id, unsigned int first_reg,
 {
 	unsigned int reg, val;
 
-	printf("\t%-20s: ", name);
+	printf("\t%-41s : ", name);
 	for (reg = first_reg; reg <= last_reg; reg++) {
 		val = id[reg];
 		putchar(((val >= 32) && (val <= 126)) ? val : '_');
diff --git a/test-cmdline.c b/test-cmdline.c
index 85b4ce0..f1d4555 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -213,6 +213,8 @@ static struct test_case {
 	{ 0, "-m devname" },
 	{ 1, "--dump-module-eeprom" },
 	{ 0, "--dump-module-eeprom devname" },
+	{ 1, "--module-info" },
+	{ 0, "--module-info devname" },
 	{ 0, "-m devname raw on" },
 	{ 0, "-m devname raw off" },
 	{ 0, "-m devname hex on" },
-- 
1.7.0.4