All of lore.kernel.org
 help / color / mirror / Atom feed
From: Adrian Pop <popadrian1996@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org, linville@tuxdriver.com
Subject: Re: [PATCH] ethtool: Add QSFP-DD support
Date: Fri, 15 Nov 2019 21:34:21 +0000	[thread overview]
Message-ID: <CAL_jBfTX7jnbQ7Ydxt=Q1avH+H8j+TD_gWY0ioJrexQ=_nZhgg@mail.gmail.com> (raw)
In-Reply-To: <20191115201508.GF24205@lunn.ch>

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@lunn.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

  parent reply	other threads:[~2019-11-15 21:35 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-09 12:42 [PATCH] ethtool: Add QSFP-DD support Adrian Pop
2019-11-09 15:33 ` Andrew Lunn
     [not found]   ` <CAL_jBfQhVAy24xbz_VbpPM0QtRu-Uzawhyn=AY0b41B9=v3Ytg@mail.gmail.com>
2019-11-13 13:59     ` Andrew Lunn
2019-11-15 19:00       ` Adrian Pop
     [not found] ` <20191115201508.GF24205@lunn.ch>
2019-11-15 21:34   ` Adrian Pop [this message]
2020-07-31  8:47 Adrian Pop
2020-08-02 16:55 ` Andrew Lunn
2020-08-04  7:44 ` Ido Schimmel
2020-08-04  8:23   ` Adrian Pop
2020-08-04 10:14 ` Michal Kubecek
2020-08-04 10:25   ` Adrian Pop
2020-08-04 17:18     ` Michal Kubecek
2020-08-06 15:13       ` Adrian Pop
2020-08-04 10:30 ` Michal Kubecek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL_jBfTX7jnbQ7Ydxt=Q1avH+H8j+TD_gWY0ioJrexQ=_nZhgg@mail.gmail.com' \
    --to=popadrian1996@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=linville@tuxdriver.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.