netdev - Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics Information Support

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]

Date:	Sun, 26 Jun 2016 23:41:16 +0300
From:	Rami Rosen <roszenrami@...il.com>
To:	Vidya Sagar Ravipati <vidya@...ulusnetworks.com>
Cc:	ben@...adent.org.uk, Netdev <netdev@...r.kernel.org>,
	bwh@...nel.org, David Miller <davem@...emloft.net>,
	bkenward@...arflare.com, daniel@...earbox.net, galp@...lanox.com,
	Roopa Prabhu <roopa@...ulusnetworks.com>,
	gospo@...ulusnetworks.com, dustin@...ulusnetworks.com
Subject: Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics
 Information Support

Hi Vidya,
Thanks a lot for your your work!

Here are my 2 cents:

> +       /* SFP voltage in 0.1mV units */
> +       __u16 sfp_voltage;
> +       /* SFP Temp in 16-bit signed 1/256 Celcius */
> +       __s16 sfp_temp;
> +       /* [4] tables are low/high warn, low/high alarm */

You already had just 5 lines earlier: /* SFP voltage in 0.1mV units */
Shouldn't it be: /* SFP threshold voltage in 0.1mV units */ ?

> +       /* SFP voltage in 0.1mV units */
> +       __u16 thresh_sfp_voltage[4];


pagging should be: paging,
support_alarms should be:supports_alarms
> +        * If pagging support exists, then support_alarms is marked as 1
> +        */
> +
+       if (eeprom_len == ETH_MODULE_SFF_8636_MAX_LEN) {
+               if (!(id[SFF8636_STATUS_2_OFFSET] &
+                                       SFF8636_STATUS_PAGE_3_PRESENT)) {
+                       sd.supports_alarms = 1;
+               }
+       }


Shouldn't it be power/ TX bias current fields?  ("current" only once)
> +       /*
> +        * SFF-8636/8436 spec is not clear whether RX power/ TX bias current
> +        * current fields are supported or not. A valid temperature reading
> +        * is used as existence for TX/RX power.

Should it be: SFF-8472/8079   (without "$")?
> + *
> + * Common utilities across SFF-8436/8636 and SFF-8472/8079$
> + * are defined in this file
> + *


Regards,
Rami Rosen