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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 4 Aug 2020 12:14:56 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Adrian Pop <popadrian1996@...il.com>
Cc:     netdev@...r.kernel.org, linville@...driver.com,
        davem@...emloft.net, kuba@...nel.org, jiri@...lanox.com,
        vadimp@...lanox.com, mlxsw@...lanox.com, idosch@...lanox.com,
        andrew@...n.ch
Subject: Re: [PATCH] ethtool: Add QSFP-DD support

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@...il.com>
> Tested-by: Ido Schimmel <idosch@...lanox.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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists