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

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

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

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

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

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

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

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

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

On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:

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

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:

quoted

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

quoted

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

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:

quoted

On Fri, Sep 17, 2021 at 05:40:37PM +0300, Ido Schimmel wrote:

quoted

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

quoted

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.