netdev - Re: [PATCH] ethtool: Add QSFP-DD 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>] [day] [month] [year] [list]
Date:   Fri, 15 Nov 2019 21:34:21 +0000
From:   Adrian Pop <popadrian1996@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, linville@...driver.com
Subject: Re: [PATCH] ethtool: Add QSFP-DD support

Hi Andrew!

The QSFP-DD support I added doesn't replace and/or overrides the
current QSFP support and memory mapping. When I started developing
this, I tried to reuse as much as the old code as possible, but most
of the properties where placed at totally different offsets.

In qsfp.c, I added this:

void sff8636_show_all(const __u8 *id, __u32 eeprom_len)
{
    if (id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_DD) {
        qsfp_dd_show_all(id, eeprom_len);
    return;
    }
    [...]

This was needed because although modinfo.type (see ethtool.c,
do_getmodule) is still ETH_MODULE_SFF_8636, the byte in the eeprom
containing the ID was different (0x18 for QSFP-DD). Since QSFP and
QSFP-DD were not fully backwards compatible, I decided to use the ID
to differentiate between them (this is done in the same way for QSFP+
for example).

If I run an older version of ethtool on an interface from a network
card having a QSFP-DD transceiver plugged in, the tool prints 0x18 and
exits. With my changes, if we have a QSFP-DD, the tool prints the
correct stats without altering the old flow for normal QSFPs.

Of course, as you stated in the previous messages, it still looks like
I'm trying to define an KAPI. If we still want to keep page 3 and just
append 0x10 and 0x11 after it, that can be easily done (although there
aren't any stats needed for QSFP-DD that are found in page 0x03).

Adrian

On Fri, 15 Nov 2019 at 20:15, Andrew Lunn <andrew@...n.ch> wrote:
>
> On Sat, Nov 09, 2019 at 02:42:05PM +0200, Adrian Pop wrote:
> > The Common Management Interface Specification (CMIS) for QSFP-DD shares
> > some similarities with other form factors such as QSFP or SFP, but due to
> > the fact that the module memory map is different, the current ethtool
> > version is not able to provide relevant information about an interface.
> >
> > This patch adds QSFP-DD support to ethtool. The changes are similar to
> > the ones already existing in qsfp.c, but customized to use the memory
> > addresses and logic as defined in the specifications document.
> >
> > Page 0x00 (lower and higher memory) are always implemented, so the ethtool
> > expects at least 256 bytes if the identifier matches the one for QSFP-DD.
> > For optical connected cables, additional pages are usually available (the
> > contain module defined  thresholds or lane diagnostic information). In
> > this case, ethtool expects to receive 768 bytes in the following format:
> >
> >     +----------+----------+----------+----------+----------+----------+
> >     |   Page   |   Page   |   Page   |   Page   |   Page   |   Page   |
> >     |   0x00   |   0x00   |   0x01   |   0x02   |   0x10   |   0x11   |
> >     |  (lower) | (higher) | (higher) | (higher) | (higher) | (higher) |
> >     |   128B   |   128B   |   128B   |   128B   |   128B   |   128B   |
> >     +----------+----------+----------+----------+----------+----------
>
> Hi Adrian
>
> From ethtool/qsfp.c
>
>  *      b) SFF 8636 based 640 bytes memory layout is presented for parser
>  *
>  *           SFF 8636 based QSFP Memory Map
>  *
>  *           2-Wire Serial Address: 1010000x
>  *
>  *           Lower Page 00h (128 bytes)
>  *           ======================
>  *           |                     |
>  *           |Page Select Byte(127)|
>  *           ======================
>  *                    |
>  *                    V
>  *           ----------------------------------------
>  *          |             |            |             |
>  *          V             V            V             V
>  *       ----------   ----------   ---------    ------------
>  *      | Upper    | | Upper    | | Upper    | | Upper      |
>  *      | Page 00h | | Page 01h | | Page 02h | | Page 03h   |
>  *      |          | |(Optional)| |(Optional)| | (Optional) |
>  *      |          | |          | |          | |            |
>  *      |          | |          | |          | |            |
>  *      |    ID    | |   AST    | |  User    | |  For       |
>  *      |  Fields  | |  Table   | | EEPROM   | |  Cable     |
>  *      |          | |          | | Data     | | Assemblies |
>  *      |          | |          | |          | |            |
>  *      |          | |          | |          | |            |
>  *      -----------  -----------   ----------  --------------
>
> I don't think dropping page 3 is good for backwards/forwards
> compatibility. An old ethtool is going to decode what it thinks is
> page 3 when in fact it is page 0x10. For backwards compatibility, you
> should not dropping page 3, but append pages 0x10 and 0x11 after page
> 3.
>
> Then there is the question, what do we do when page 4 is needed by
> some other module? Maybe the better solution is to wait until netlink
> ethtool is available. We can then label each page with an attribute
> indicate what page it actually is, and so allow sparse pages.
>
>          Andrew