netdev - Re: Optics (SFP) monitoring on ixgbe and igbe

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPN4dA9f3y1mDPubqd9s+v5supj3hNvZaWym0_y3EMZd7L6MyQ@mail.gmail.com>
Date:	Sun, 18 Nov 2012 22:35:59 +0100
From:	Aurélien <footplus@...il.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: Optics (SFP) monitoring on ixgbe and igbe

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.

>
> 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

Download attachment "0001-Implemented-basic-optics-diagnostics-for-SFF-8472-co.patch" of type "application/octet-stream" (21479 bytes)