Re: [PATCH] ethtool: Add QSFP-DD support

[Michal Kubecek <mkubecek@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 3596 bytes --]

On Fri, Jul 31, 2020 at 11:47:25AM +0300, 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   |
>     +----------+----------+----------+----------+----------+----------
> 
> Several functions from qsfp.c could be reused, so an additional parameter
> was added to each and the functions were moved to sff-common.c.
> 
> Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
> Tested-by: Ido Schimmel <idosch@mellanox.com>

AFAICS the kernel counterpart is going to reach mainline in 5.9-rc1
merge window. Please base your patch on "next" branch or wait until next
is merged into master after 5.8 release (which should be later today or
tomorrow).

> ---
>  Makefile.am  |   7 +-
>  qsfp-dd.c    | 561 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qsfp-dd.h    | 236 ++++++++++++++++++++++
>  qsfp.c       |  60 ++----
>  qsfp.h       |   8 -
>  sff-common.c |  52 +++++
>  sff-common.h |  26 ++-
>  7 files changed, 894 insertions(+), 56 deletions(-)
>  create mode 100644 qsfp-dd.c
>  create mode 100644 qsfp-dd.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 2abb274..9fd8024 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -17,7 +17,8 @@ ethtool_SOURCES += \
>  		  smsc911x.c at76c50x-usb.c sfc.c stmmac.c	\
>  		  sff-common.c sff-common.h sfpid.c sfpdiag.c	\
>  		  ixgbevf.c tse.c vmxnet3.c qsfp.c qsfp.h fjes.c lan78xx.c \
> -		  igc.c
> +		  igc.c \
> +		  qsfp-dd.c qsfp-dd.h

There is no need to start a new line.

>  endif
>  
>  if ENABLE_BASH_COMPLETION
> @@ -47,12 +48,12 @@ endif
>  
>  TESTS = test-cmdline
>  check_PROGRAMS = test-cmdline
> -test_cmdline_SOURCES = test-cmdline.c test-common.c $(ethtool_SOURCES) 
> +test_cmdline_SOURCES = test-cmdline.c test-common.c $(ethtool_SOURCES)
>  test_cmdline_CFLAGS = -DTEST_ETHTOOL
>  if !ETHTOOL_ENABLE_NETLINK
>  TESTS += test-features
>  check_PROGRAMS += test-features
> -test_features_SOURCES = test-features.c test-common.c $(ethtool_SOURCES) 
> +test_features_SOURCES = test-features.c test-common.c $(ethtool_SOURCES)
>  test_features_CFLAGS = -DTEST_ETHTOOL
>  endif
>  

Do not mix unrelated changes like whitespace cleanups with your
functional changes, it makes it harder to see what the patch is doing.
Please split the cleanups (there are some more in qsfp.c) into
a separate patch.

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]