| 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: <1354335498.2640.23.camel@bwh-desktop.uk.solarflarecom.com>
Date: Sat, 1 Dec 2012 04:18:18 +0000
From: Ben Hutchings <bhutchings@...arflare.com>
To: <footplus@...il.com>
CC: <netdev@...r.kernel.org>
Subject: Re: Optics (SFP) monitoring on ixgbe and igbe
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
> <bhutchings@...arflare.com> 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.
This version drops the -lm completely, so it doesn't link. Maybe you
edited the generated Makefile or Makefile.in?
> > 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.
[...]
> > 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:
[...]
> --- /dev/null
> +++ b/sfpdiag.c
[...]
> +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.
> +}
> +
> diff --git a/sfpid.c b/sfpid.c
> index a4a671d..2982d0d 100644
> --- a/sfpid.c
> +++ b/sfpid.c
[...]
> 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 copper compliance", 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 copper compliance", id[60]);
> switch (id[60]) {
> case 0x00:
> printf(" (unspecified)");
If you want to change these labels, do that in a separate patch.
[...]
> @@ -368,14 +370,15 @@ void sff8079_show_all(const __u8 *id)
> sff8079_show_connector(id);
> sff8079_show_transceiver(id);
> sff8079_show_encoding(id);
> - sff8079_show_value_with_unit(id, 12, "BR, Nominal", 100, "MBd");
> + sff8079_show_value_with_unit(id, 12,
> + "Nominal signalling rate", 100, "MBd");
> sff8079_show_rate_identifier(id);
> sff8079_show_value_with_unit(id, 14,
> - "Length (SMF,km)", 1, "km");
> + "Length (SMF,km)", 1, "km");
> sff8079_show_value_with_unit(id, 15, "Length (SMF)", 100, "m");
> sff8079_show_value_with_unit(id, 16, "Length (50um)", 10, "m");
> sff8079_show_value_with_unit(id, 17,
> - "Length (62.5um)", 10, "m");
> + "Length (62.5um)", 10, "m");
> sff8079_show_value_with_unit(id, 18, "Length (Copper)", 1, "m");
> sff8079_show_value_with_unit(id, 19, "Length (OM3)", 10, "m");
> sff8079_show_wavelength_or_copper_compliance(id);
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.
--
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