netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Ido Schimmel <idosch@idosch.org>
Cc: Adrian Pop <popadrian1996@gmail.com>,
	netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
	jiri@mellanox.com, vadimp@mellanox.com, mlxsw@mellanox.com,
	Ido Schimmel <idosch@mellanox.com>
Subject: Re: [PATCH net-next 1/2] mlxsw: core: Add ethtool support for QSFP-DD transceivers
Date: Tue, 30 Jun 2020 02:21:59 +0200	[thread overview]
Message-ID: <20200630002159.GA597495@lunn.ch> (raw)
In-Reply-To: <20200628115557.GA273881@shredder>

> Adrian,
> 
> Thanks for the detailed response. I don't think the kernel should pass
> fake pages only to make it easier for user space to parse the
> information. What you are describing is basic dissection and it's done
> all the time by wireshark / tcpdump.
> 
> Anyway, even we pass a fake page 03h, page 11h can still be at a
> variable offset. See table 8-28 [1], bits 1-0 at offset 142 in page 01h
> determine the size of pages 10h and 11h:
> 
> 0 - each page is 128 bytes in size
> 1 - each page is 256 bytes in size
> 2 - each page is 512 bytes in size
> 
> So a completely stable layout (unless I missed something) will entail
> the kernel sending 1664 bytes to user space each time. This looks
> unnecessarily rigid to me. The people who wrote the standard obviously
> took into account the fact that the page layout needs to be discoverable
> from the data and I think we should embrace it and only pass valid
> information to user space.

O.K. That makes things more complex. And i would say, Ido is correct,
we need to make use of the discoverable features.

I've no practice experience with modules other than plain old SFPs,
1G. And those have all sorts of errors, even basic things like the CRC
are systematically incorrect because they are not recalculated after
adding the serial number. We have had people trying to submit patches
to ethtool to make it ignore bits so that it dumps more information,
because the manufacturer failed to set the correct bits, etc.

Ido, Adrian, what is your experience with these QSFP-DD devices. Are
they generally of better quality, the EEPROM can be trusted? Is there
any form of compliance test.

If we go down the path of using the discovery information, it means we
have no way for user space to try to correct for when the information
is incorrect. It cannot request specific pages. So maybe we should
consider an alternative?

The netlink ethtool gives us more flexibility. How about we make a new
API where user space can request any pages it want, and specify the
size of the page. ethtool can start out by reading page 0. That should
allow it to identify the basic class of device. It can then request
additional pages as needed.

The nice thing about that is we don't need two parsers of the
discovery information, one in user and second in kernel space. We
don't need to guarantee these two parsers agree with each other, in
order to correctly decode what the kernel sent to user space. And user
space has the flexibility to work around known issues when
manufactures get their EEPROM wrong.

	     Andrew

  parent reply	other threads:[~2020-06-30  0:22 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-26 14:47 [PATCH net-next 0/2] mlxsw: Add support for QSFP-DD transceiver type Ido Schimmel
2020-06-26 14:47 ` [PATCH net-next 1/2] mlxsw: core: Add ethtool support for QSFP-DD transceivers Ido Schimmel
2020-06-26 15:19   ` Andrew Lunn
2020-06-26 17:33     ` Adrian Pop
2020-06-26 19:07       ` Andrew Lunn
2020-06-26 22:13         ` Adrian Pop
2020-06-27 19:16           ` Ido Schimmel
2020-06-27 20:42             ` Adrian Pop
2020-06-28 11:55               ` Ido Schimmel
2020-06-28 12:27                 ` Adrian Pop
2020-06-30  0:21                 ` Andrew Lunn [this message]
2020-06-30  5:59                   ` Ido Schimmel
2020-06-30  9:37                     ` Vadim Pasternak
2020-06-30 13:00                     ` Andrew Lunn
2020-06-26 14:47 ` [PATCH net-next 2/2] mlxsw: core: Add support for temperature thresholds reading " Ido Schimmel
2020-06-26 14:53 ` [PATCH net-next 0/2] mlxsw: Add support for QSFP-DD transceiver type Ido Schimmel
2020-06-26 15:06   ` Andrew Lunn
2020-06-26 16:45     ` Adrian Pop

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=20200630002159.GA597495@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=idosch@idosch.org \
    --cc=idosch@mellanox.com \
    --cc=jiri@mellanox.com \
    --cc=kuba@kernel.org \
    --cc=mlxsw@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=popadrian1996@gmail.com \
    --cc=vadimp@mellanox.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).