Date:   Thu, 4 Mar 2021 16:50:04 -0800
From:   "Don Bollinger" <don@...bollingers.org>
To:     "'Moshe Shemesh'" <moshe@...dia.com>,
        "'David S. Miller'" <davem@...emloft.net>,
        "'Jakub Kicinski'" <kuba@...nel.org>,
        "'Andrew Lunn'" <andrew@...n.ch>,
        "'Adrian Pop'" <pop.adrian61@...il.com>,
        "'Michal Kubecek'" <mkubecek@...e.cz>, <netdev@...r.kernel.org>
Cc:     "'Vladyslav Tarasiuk'" <vladyslavt@...dia.com>,
        <don@...bollingers.org>
Subject: RE: [RFC PATCH V2 net-next 0/5] ethtool: Extend module EEPROM dump API

On Thu, Mar 04, 2021 at 10:57AM-0800, Moshe Shemesh wrote:
> Ethtool supports module EEPROM dumps via the `ethtool -m <dev>`
> command.
> But in current state its functionality is limited - offset and length
parameters,
> which are used to specify a linear desired region of EEPROM data to dump,
is
> not enough, considering emergence of complex module EEPROM layouts
> such as CMIS 4.0.
> Moreover, CMIS 4.0 extends the amount of pages that may be accessible by
> introducing another parameter for page addressing - banks.

This is nice work, addressing the banks problem (though there are no devices
with bank switching yet?)

I suggest this change increase the maximum size of EEPROM to the maximum
the architecture allows.  That's 256 pages (128 bytes) plus the lower page
for
a total of 257*256 bytes.  SFP devices can access another 256 bytes since
they
use two i2c addresses but only one of them is paged.  The size increase is
necessary for bank support since banked pages are all above the current
640 byte limit.  Note that the SFF-* specs do not specify what is in pages
above page 3 (except CMIS), but they DO specify that those pages are
available for proprietary uses by module vendors.  I will call out these
changes
in the following patches.

Ethtool also supports module 'change-eeprom', a write function mirroring the
dump function.  That path needs to be implemented too.  There are some 
very interesting proprietary tricks that some modules can do by writing
the right magic to the right registers, some of which are on pages in the
0xF0 range.

> 
> Besides, currently module EEPROM is represented as a chunk of
> concatenated pages, where lower 128 bytes of all pages, except page 00h,
> are omitted. Offset and length are used to address parts of this fake
linear
> memory. But in practice drivers, which implement
> get_module_info() and get_module_eeprom() ethtool ops still calculate
> page number and set I2C address on their own.
> 
> This series tackles these issues by adding ethtool op, which allows to
pass
> page number, bank number and I2C address in addition to offset and length
> parameters to the driver, adds corresponding netlink infrastructure and
> implements the new interface in mlx5 driver.
> 
> This allows to extend userspace 'ethtool -m' CLI by adding new parameters
-
> page, bank and i2c. New command line format:
>  ethtool -m <dev> [hex on|off] [raw on|off] [offset N] [length N] [page N]
> [bank N] [i2c N]
> 
> The consequence of this series is a possibility to dump arbitrary EEPROM
> page at a time, in contrast to dumps of concatenated pages. Therefore,
> offset and length change their semantics and may be used only to specify a
> part of data within a page, which size is currently limited to 256 bytes.

Just to be clear, if you define a page to be 256 bytes, and only specify
offset
within a page, then offset 0-127 is the same for every page on the device,
and useful offsets for each page start at 128.  This can be confusing, but I
think it is the right approach.

> 
> As for backwards compatibility with get_module_info() and
> get_module_eeprom() pair, the series addresses it as well by implementing
> a fallback mechanism. As mentioned earlier, drivers derive a page number
> from 'global' offset, so this can be done vice versa without their
involvement
> thanks to standardization. If kernel netlink handler of 'ethtool -m'
command
> detects that new ethtool op is not supported by the driver, it calculates
> offset from given page number and page offset and calls old ndos, if they
are
> available.
> 
> Change log:
> v1 -> v2:
> - Limited i2c_address values by 127
> - Added page bound check for offset and length
> - Added defines for these two points
> - Added extack to ndo parameters
> - Moved ethnl_ops_begin(dev) and set error path accordingly
> 
> 
> 
> Vladyslav Tarasiuk (5):
>   ethtool: Allow network drivers to dump arbitrary EEPROM data
>   net/mlx5: Refactor module EEPROM query
>   net/mlx5: Implement get_module_eeprom_data_by_page()
>   net/mlx5: Add support for DSFP module EEPROM dumps
>   ethtool: Add fallback to get_module_eeprom from netlink command
> 
>  .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  42 +++
>  .../net/ethernet/mellanox/mlx5/core/port.c    | 101 ++++++--
>  include/linux/ethtool.h                       |   7 +-
>  include/linux/mlx5/port.h                     |  12 +
>  include/uapi/linux/ethtool.h                  |  26 ++
>  include/uapi/linux/ethtool_netlink.h          |  19 ++
>  net/ethtool/Makefile                          |   2 +-
>  net/ethtool/eeprom.c                          | 239 ++++++++++++++++++
>  net/ethtool/netlink.c                         |  10 +
>  net/ethtool/netlink.h                         |   2 +
>  10 files changed, 430 insertions(+), 30 deletions(-)  create mode 100644
> net/ethtool/eeprom.c
> 
> --
> 2.18.2

Don Bollinger

Powered by blists - more mailing lists