netdev - Re: [PATCH net-next 1/2] mlxsw: core: Add ethtool support for QSFP-DD transceivers

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: <CAL_jBfQMQbMAFeHji2_Y_Y_gC20S_0QL33wjPgPBaKeVRLg1SQ@mail.gmail.com>
Date:   Fri, 26 Jun 2020 23:13:42 +0100
From:   Adrian Pop <popadrian1996@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Ido Schimmel <idosch@...sch.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org, jiri@...lanox.com,
        vadimp@...lanox.com, mlxsw@...lanox.com,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next 1/2] mlxsw: core: Add ethtool support for QSFP-DD transceivers

> You are saying pages 00h, 01h and 02h are mandatory for QSPF-DD.  Page
> 03h is optional, but when present, it seems to contain what is page
> 02h above. Since the QSPF KAPI has it, QSPF-DD KAPI should also have
> it. So i would suggest that pages 10h and 11h come after that.
>
> If a driver wants to pass a subset, it can, but it must always trim
> from the right, it cannot remove pages from the middle.
>
>      Andrew

I agree with this. Basically there are two big cases:
- passive copper transceivers with flat memory => just 00h will be
present (both lower and higher => 256 bytes)
- optical transceivers with paged memory => 00h, 01h, 02h, 10h, 11h => 768 bytes
Having page 03h after 02h (so 896 bytes in total) seems like a good
idea and the updates I'll have to do to my old patch are minor
(updating the offset value of page 10h and 11h). When I tested my
patch, I did it with both passive copper transceivers and optical
transceivers and there weren't any issues.

In this patch, Ido added a comment in the code stating "Upper pages
10h and 11h are currently not supported by the driver.". This won't
actually be a problem! In CMIS Rev. 4, Table 8-12 Byte 85 (55h), we
learn that if the value of that byte is 01h or 02h, we have a SMF or
MMF interface (both optical). In the qsfp_dd_show_sig_optical_pwr
function (in my patch) there is this bit:

+ __u8 module_type = id[QSFP_DD_MODULE_TYPE_OFFSET];
[...]
+ /**
+ * The thresholds and the high/low alarms/warnings are available
+ * only if an optical interface (MMF/SMF) is present (if this is
+ * the case, it means that 5 pages are available).
+ */
+ if (module_type != QSFP_DD_MT_MMF &&
+    module_type != QSFP_DD_MT_SMF &&
+    eeprom_len != QSFP_DD_EEPROM_5PAG)
+ return;

But Ido sets the eeprom_len to be ETH_MODULE_SFF_8472_LEN which is
512, while QSFP_DD_EEPROM_5PAG is defined as 80h * 6 = 768. So there
won't be any issues of accessing non-existent values, since I return
from the function that deals with the pages 10h and 11h early. When
the driver will support them too everything will just work so your
idea of a driver being able to pass only a subset of pages (being
allowed to trim only from the right) holds.

Adrian