[PATCH ethtool-next 0/7] ethtool: Small EEPROM changes

[PATCH ethtool-next 0/7] ethtool: Small EEPROM changes

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patchset contains small patches for various issues I noticed while
working on the module EEPROM parsing code.

Ido Schimmel (7):
  cmis: Fix CLEI code parsing
  cmis: Fix wrong define name
  cmis: Correct comment
  sff-8636: Remove incorrect comment
  sff-8636: Fix incorrect function name
  sff-8636: Convert if statement to switch-case
  sff-8636: Remove extra blank lines

 cmis.c | 12 +++++++-----
 cmis.h |  5 ++---
 qsfp.c | 18 +++++++++---------
 3 files changed, 18 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH ethtool-next 1/7] cmis: Fix CLEI code parsing

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
code (Common Language Equipment Identification) field. The field is
always present, but might not be supported. In which case, "a value of
all ASCII 20h (spaces) shall be entered".

Therefore, remove the erroneous check which seems to be influenced from
SFF-8636 and only print the string if it is supported and has a non-zero
length.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 cmis.c | 8 +++++---
 cmis.h | 3 +--
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/cmis.c b/cmis.c
index 1a91e798e4b8..2a48c1a1d56a 100644
--- a/cmis.c
+++ b/cmis.c
@@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
  */
 static void cmis_show_vendor_info(const __u8 *id)
 {
+	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
+
 	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
 		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
 	cmis_show_oui(id);
@@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
 	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
 		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
 
-	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
-		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
-			       CMIS_CLEI_END_OFFSET, "CLEI code");
+	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
+		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
+			       "CLEI code");
 }
 
 void qsfp_dd_show_all(const __u8 *id)
diff --git a/cmis.h b/cmis.h
index 78ee1495bc33..d365252baa48 100644
--- a/cmis.h
+++ b/cmis.h
@@ -34,10 +34,9 @@
 #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
 
 /* CLEI Code (Page 0) */
-#define CMIS_CLEI_PRESENT_BYTE			0x02
-#define CMIS_CLEI_PRESENT_MASK			0x20
 #define CMIS_CLEI_START_OFFSET			0xBE
 #define CMIS_CLEI_END_OFFSET			0xC7
+#define CMIS_CLEI_BLANK				"          "
 
 /* Cable assembly length */
 #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
-- 
2.31.1

[PATCH ethtool-next 2/7] cmis: Fix wrong define name

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Offset 0x10 in the Lower Memory stores the "VccMonVoltage".

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 cmis.c | 2 +-
 cmis.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmis.c b/cmis.c
index 2a48c1a1d56a..7fa7da87a92a 100644
--- a/cmis.c
+++ b/cmis.c
@@ -271,7 +271,7 @@ static void cmis_show_mod_lvl_monitors(const __u8 *id)
 	PRINT_TEMP("Module temperature",
 		   OFFSET_TO_TEMP(CMIS_CURR_TEMP_OFFSET));
 	PRINT_VCC("Module voltage",
-		  OFFSET_TO_U16(CMIS_CURR_CURR_OFFSET));
+		  OFFSET_TO_U16(CMIS_CURR_VCC_OFFSET));
 }
 
 static void cmis_show_link_len_from_page(const __u8 *page_one_data)
diff --git a/cmis.h b/cmis.h
index d365252baa48..5fb2efe08265 100644
--- a/cmis.h
+++ b/cmis.h
@@ -11,7 +11,7 @@
 
 /* Module-Level Monitors (Page 0) */
 #define CMIS_CURR_TEMP_OFFSET			0x0E
-#define CMIS_CURR_CURR_OFFSET			0x10
+#define CMIS_CURR_VCC_OFFSET			0x10
 
 #define CMIS_CTOR_OFFSET			0xCB
 
-- 
2.31.1

[PATCH ethtool-next 3/7] cmis: Correct comment

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The file is concerned with CMIS support, not QSFP-DD which is the
physical form factor.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 cmis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmis.c b/cmis.c
index 7fa7da87a92a..b85250a96a95 100644
--- a/cmis.c
+++ b/cmis.c
@@ -1,7 +1,7 @@
 /**
  * Description:
  *
- * This module adds QSFP-DD support to ethtool. The changes are similar to
+ * This module adds CMIS 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 specification's document.
  *
-- 
2.31.1

[PATCH ethtool-next 4/7] sff-8636: Remove incorrect comment

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The comment was copied from SFF-8472 (i.e., sfpdiag.c) where the
diagnostic page is at I2C address 0x51. SFF-8636 only uses I2C address
0x50.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 qsfp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qsfp.c b/qsfp.c
index e84226bc1554..263cf188377d 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -64,7 +64,7 @@
 
 static struct sff8636_aw_flags {
 	const char *str;        /* Human-readable string, null at the end */
-	int offset;             /* A2-relative address offset */
+	int offset;
 	__u8 value;             /* Alarm is on if (offset & value) != 0. */
 } sff8636_aw_flags[] = {
 	{ "Laser bias current high alarm   (Chan 1)",
-- 
2.31.1

[PATCH ethtool-next 5/7] sff-8636: Fix incorrect function name

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The specification is called SFF-8636, not SFF-6836.

Rename the function accordingly.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 qsfp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qsfp.c b/qsfp.c
index 263cf188377d..3401db84352d 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -820,7 +820,7 @@ static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eepro
 }
 
 
-static void sff6836_show_page_zero(const __u8 *id)
+static void sff8636_show_page_zero(const __u8 *id)
 {
 	sff8636_show_ext_identifier(id);
 	sff8636_show_connector(id);
@@ -866,7 +866,7 @@ void sff8636_show_all(const __u8 *id, __u32 eeprom_len)
 	if ((id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP) ||
 		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_PLUS) ||
 		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP28)) {
-		sff6836_show_page_zero(id);
+		sff8636_show_page_zero(id);
 		sff8636_show_dom(id, id + 3 * 0x80, eeprom_len);
 	}
 }
@@ -875,7 +875,7 @@ void sff8636_show_all_paged(const struct ethtool_module_eeprom *page_zero,
 			    const struct ethtool_module_eeprom *page_three)
 {
 	sff8636_show_identifier(page_zero->data);
-	sff6836_show_page_zero(page_zero->data);
+	sff8636_show_page_zero(page_zero->data);
 	if (page_three)
 		sff8636_show_dom(page_zero->data, page_three->data - 0x80,
 				 ETH_MODULE_SFF_8636_MAX_LEN);
-- 
2.31.1

[PATCH ethtool-next 6/7] sff-8636: Convert if statement to switch-case

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The indentation is wrong and the statement can be more clearly
represented using a switch-case statement. Convert it.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 qsfp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qsfp.c b/qsfp.c
index 3401db84352d..d1464cb50fdc 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -863,11 +863,13 @@ void sff8636_show_all(const __u8 *id, __u32 eeprom_len)
 	}
 
 	sff8636_show_identifier(id);
-	if ((id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP) ||
-		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_PLUS) ||
-		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP28)) {
+	switch (id[SFF8636_ID_OFFSET]) {
+	case SFF8024_ID_QSFP:
+	case SFF8024_ID_QSFP_PLUS:
+	case SFF8024_ID_QSFP28:
 		sff8636_show_page_zero(id);
 		sff8636_show_dom(id, id + 3 * 0x80, eeprom_len);
+		break;
 	}
 }
 
-- 
2.31.1

[PATCH ethtool-next 7/7] sff-8636: Remove extra blank lines

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Not needed, so remove them.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 qsfp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qsfp.c b/qsfp.c
index d1464cb50fdc..3f37f1036e96 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -738,7 +738,6 @@ static void sff8636_dom_parse(const __u8 *id, const __u8 *page_three, struct sff
 		sd->scd[i].rx_power = OFFSET_TO_U16(rx_power_offset);
 		sd->scd[i].tx_power = OFFSET_TO_U16(tx_power_offset);
 	}
-
 }
 
 static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eeprom_len)
@@ -819,7 +818,6 @@ static void sff8636_show_dom(const __u8 *id, const __u8 *page_three, __u32 eepro
 	}
 }
 
-
 static void sff8636_show_page_zero(const __u8 *id)
 {
 	sff8636_show_ext_identifier(id);
-- 
2.31.1

Re: [PATCH ethtool-next 1/7] cmis: Fix CLEI code parsing

[Michal Kubecek]

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

On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> code (Common Language Equipment Identification) field. The field is
> always present, but might not be supported. In which case, "a value of
> all ASCII 20h (spaces) shall be entered".
> 
> Therefore, remove the erroneous check which seems to be influenced from
> SFF-8636 and only print the string if it is supported and has a non-zero
> length.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
>  cmis.c | 8 +++++---
>  cmis.h | 3 +--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/cmis.c b/cmis.c
> index 1a91e798e4b8..2a48c1a1d56a 100644
> --- a/cmis.c
> +++ b/cmis.c
> @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
>   */
>  static void cmis_show_vendor_info(const __u8 *id)
>  {
> +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> +
>  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
>  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
>  	cmis_show_oui(id);
> @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
>  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
>  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
>  
> -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> +			       "CLEI code");
>  }
>  
>  void qsfp_dd_show_all(const __u8 *id)

Is it safe to assume that the string will be always null terminated?
Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
would fill the whole block at offsets 0xBE through 0xC7 with spaces and
offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
doesn't seem to expect a null terminated string, rather a space padded
one.

Michal


> diff --git a/cmis.h b/cmis.h
> index 78ee1495bc33..d365252baa48 100644
> --- a/cmis.h
> +++ b/cmis.h
> @@ -34,10 +34,9 @@
>  #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
>  
>  /* CLEI Code (Page 0) */
> -#define CMIS_CLEI_PRESENT_BYTE			0x02
> -#define CMIS_CLEI_PRESENT_MASK			0x20
>  #define CMIS_CLEI_START_OFFSET			0xBE
>  #define CMIS_CLEI_END_OFFSET			0xC7
> +#define CMIS_CLEI_BLANK				"          "
>  
>  /* Cable assembly length */
>  #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
> -- 
> 2.31.1
> 

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

Re: [PATCH ethtool-next 1/7] cmis: Fix CLEI code parsing

[Ido Schimmel]

On Thu, Sep 30, 2021 at 10:21:33PM +0200, Michal Kubecek wrote:
> On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> > code (Common Language Equipment Identification) field. The field is
> > always present, but might not be supported. In which case, "a value of
> > all ASCII 20h (spaces) shall be entered".
> > 
> > Therefore, remove the erroneous check which seems to be influenced from
> > SFF-8636 and only print the string if it is supported and has a non-zero
> > length.
> > 
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> >  cmis.c | 8 +++++---
> >  cmis.h | 3 +--
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cmis.c b/cmis.c
> > index 1a91e798e4b8..2a48c1a1d56a 100644
> > --- a/cmis.c
> > +++ b/cmis.c
> > @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
> >   */
> >  static void cmis_show_vendor_info(const __u8 *id)
> >  {
> > +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> > +
> >  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
> >  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
> >  	cmis_show_oui(id);
> > @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
> >  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
> >  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
> >  
> > -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> > -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> > -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> > +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> > +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> > +			       "CLEI code");
> >  }
> >  
> >  void qsfp_dd_show_all(const __u8 *id)
> 
> Is it safe to assume that the string will be always null terminated?

No. You want to see strnlen() and strncmp() instead?

> Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
> would fill the whole block at offsets 0xBE through 0xC7 with spaces and
> offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
> doesn't seem to expect a null terminated string, rather a space padded
> one.
> 
> Michal
> 
> 
> > diff --git a/cmis.h b/cmis.h
> > index 78ee1495bc33..d365252baa48 100644
> > --- a/cmis.h
> > +++ b/cmis.h
> > @@ -34,10 +34,9 @@
> >  #define CMIS_DATE_VENDOR_LOT_OFFSET		0xBC
> >  
> >  /* CLEI Code (Page 0) */
> > -#define CMIS_CLEI_PRESENT_BYTE			0x02
> > -#define CMIS_CLEI_PRESENT_MASK			0x20
> >  #define CMIS_CLEI_START_OFFSET			0xBE
> >  #define CMIS_CLEI_END_OFFSET			0xC7
> > +#define CMIS_CLEI_BLANK				"          "
> >  
> >  /* Cable assembly length */
> >  #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
> > -- 
> > 2.31.1
> > 

Re: [PATCH ethtool-next 1/7] cmis: Fix CLEI code parsing

[Michal Kubecek]

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

On Thu, Sep 30, 2021 at 11:41:02PM +0300, Ido Schimmel wrote:
> On Thu, Sep 30, 2021 at 10:21:33PM +0200, Michal Kubecek wrote:
> > On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:
> > > From: Ido Schimmel <idosch@nvidia.com>
> > > 
> > > In CMIS, unlike SFF-8636, there is no presence indication for the CLEI
> > > code (Common Language Equipment Identification) field. The field is
> > > always present, but might not be supported. In which case, "a value of
> > > all ASCII 20h (spaces) shall be entered".
> > > 
> > > Therefore, remove the erroneous check which seems to be influenced from
> > > SFF-8636 and only print the string if it is supported and has a non-zero
> > > length.
> > > 
> > > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > > ---
> > >  cmis.c | 8 +++++---
> > >  cmis.h | 3 +--
> > >  2 files changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/cmis.c b/cmis.c
> > > index 1a91e798e4b8..2a48c1a1d56a 100644
> > > --- a/cmis.c
> > > +++ b/cmis.c
> > > @@ -307,6 +307,8 @@ static void cmis_show_link_len(const __u8 *id)
> > >   */
> > >  static void cmis_show_vendor_info(const __u8 *id)
> > >  {
> > > +	const char *clei = (const char *)(id + CMIS_CLEI_START_OFFSET);
> > > +
> > >  	sff_show_ascii(id, CMIS_VENDOR_NAME_START_OFFSET,
> > >  		       CMIS_VENDOR_NAME_END_OFFSET, "Vendor name");
> > >  	cmis_show_oui(id);
> > > @@ -319,9 +321,9 @@ static void cmis_show_vendor_info(const __u8 *id)
> > >  	sff_show_ascii(id, CMIS_DATE_YEAR_OFFSET,
> > >  		       CMIS_DATE_VENDOR_LOT_OFFSET + 1, "Date code");
> > >  
> > > -	if (id[CMIS_CLEI_PRESENT_BYTE] & CMIS_CLEI_PRESENT_MASK)
> > > -		sff_show_ascii(id, CMIS_CLEI_START_OFFSET,
> > > -			       CMIS_CLEI_END_OFFSET, "CLEI code");
> > > +	if (strlen(clei) && strcmp(clei, CMIS_CLEI_BLANK))
> > > +		sff_show_ascii(id, CMIS_CLEI_START_OFFSET, CMIS_CLEI_END_OFFSET,
> > > +			       "CLEI code");
> > >  }
> > >  
> > >  void qsfp_dd_show_all(const __u8 *id)
> > 
> > Is it safe to assume that the string will be always null terminated?
> 
> No. You want to see strnlen() and strncmp() instead?

Yes, that would solve the problem. Actually, "strlen(clei)" could be
also replaced with "*clei" or "clei[0]" as a string is empty if and only
if its first byte is null.

Michal

> > Looking at the code below, CMIS_CLEI_BLANK consists of 10 spaces which
> > would fill the whole block at offsets 0xBE through 0xC7 with spaces and
> > offset 0xC8 is used as CMIS_PWR_CLASS_OFFSET. Also, sff_show_ascii()
> > doesn't seem to expect a null terminated string, rather a space padded
> > one.

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