| 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
| ||
|
Message-ID: <1353094719.2743.21.camel@bwh-desktop.uk.solarflarecom.com>
Date: Fri, 16 Nov 2012 19:38:39 +0000
From: Ben Hutchings <bhutchings@...arflare.com>
To: <footplus@...il.com>
CC: <netdev@...r.kernel.org>, <jeffrey.t.kirsher@...el.com>
Subject: Re: Optics (SFP) monitoring on ixgbe and igbe
On Fri, 2012-11-16 at 03:23 +0100, Aurélien wrote:
> On Fri, Nov 16, 2012 at 12:30 AM, Ben Hutchings
> <bhutchings@...arflare.com> 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 ?
[...]
> --- a/configure.ac
> +++ b/configure.ac
> @@ -13,9 +13,11 @@ AC_PROG_GCC_TRADITIONAL
> AM_PROG_CC_C_O
>
> dnl Checks for libraries.
> +AC_CHECK_LIB([m], [log10])
>
> dnl Checks for header files.
> AC_CHECK_HEADERS(sys/ioctl.h)
> +AC_CHECK_HEADERS(math.h)
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.
> dnl Checks for typedefs, structures, and compiler characteristics.
> AC_MSG_CHECKING([whether <linux/types.h> defines big-endian types])
> diff --git a/ethtool.c b/ethtool.c
> index 3db7fec..e18fc85 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3549,6 +3549,47 @@ static int do_tsinfo(struct cmd_context *ctx)
> return 0;
> }
>
> +static int do_getmoduleoptics(struct cmd_context *ctx)
> +{
> + struct ethtool_modinfo modinfo;
> + struct ethtool_eeprom *eeprom;
> + int err;
> +
> + modinfo.cmd = ETHTOOL_GMODULEINFO;
> + err = send_ioctl(ctx, &modinfo);
> + if (err < 0) {
> + perror("Cannot get module information");
> + return 1;
> + }
> +
> + if (modinfo.type != ETH_MODULE_SFF_8472)
> + {
> + perror("Module is not SFF-8472 (DOM) compliant");
> + return 1;
> + }
> +
> + eeprom = calloc(1, sizeof(*eeprom) + modinfo.eeprom_len);
> + if (!eeprom) {
> + perror("Cannot allocate memory for module EEPROM data");
> + return 1;
> + }
> +
> + eeprom->cmd = ETHTOOL_GMODULEEEPROM;
> + eeprom->len = modinfo.eeprom_len;
> + eeprom->offset = 0;
> + err = send_ioctl(ctx, eeprom);
> + if (err < 0) {
> + perror("Cannot access module EEPROM");
> + free(eeprom);
> + return 1;
> + }
> +
> + printf("Physical interface: %s\n", ctx->devname);
> + sff8472_show_all(eeprom->data);
> + free(eeprom);
> + return 0;
> +}
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.
> static int do_getmodule(struct cmd_context *ctx)
> {
> struct ethtool_modinfo modinfo;
> @@ -3832,11 +3873,13 @@ static const struct option {
> { "--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",
> + "Query/Decode Module EEPROM information",
> " [ raw on|off ]\n"
> " [ hex on|off ]\n"
> " [ offset N ]\n"
> " [ length N ]\n" },
> + { "-O|--module-optics", 1, do_getmoduleoptics,
> + "Show module optical diagnostics" },
> { "--show-eee", 1, do_geee, "Show EEE settings"},
> { "--set-eee", 1, do_seee, "Set EEE settings",
> " [ eee on|off ]\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..aa7c14c
> --- /dev/null
> +++ b/sfpdiag.c
[...]
> +#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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists