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

[PATCH ethtool-next v2 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.

v2:
* Patch #1: Do not assume the CLEI code is NULL terminated

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 |  6 +++---
 qsfp.c | 18 +++++++++---------
 3 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH ethtool-next v2 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 | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/cmis.c b/cmis.c
index 1a91e798e4b8..499355d0e024 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 (*clei && strncmp(clei, CMIS_CLEI_BLANK, CMIS_CLEI_LEN))
+		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..cfac08f42904 100644
--- a/cmis.h
+++ b/cmis.h
@@ -34,10 +34,10 @@
 #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				"          "
+#define CMIS_CLEI_LEN				0x0A
 
 /* Cable assembly length */
 #define CMIS_CBL_ASM_LEN_OFFSET			0xCA
-- 
2.31.1

[PATCH ethtool-next v2 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 499355d0e024..408db6f26c3b 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 cfac08f42904..e3012ccfdd79 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 v2 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 408db6f26c3b..591cc72953b7 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 v2 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 v2 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 v2 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 v2 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 v2 0/7] ethtool: Small EEPROM changes

[Ido Schimmel]

On Fri, Oct 01, 2021 at 06:06:20PM +0300, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset contains small patches for various issues I noticed while
> working on the module EEPROM parsing code.
> 
> v2:
> * Patch #1: Do not assume the CLEI code is NULL terminated

Michal, any comments?

Thanks

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to ethtool/ethtool.git (master)
by Michal Kubecek <mkubecek@suse.cz>:

On Fri,  1 Oct 2021 18:06:20 +0300 you wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset contains small patches for various issues I noticed while
> working on the module EEPROM parsing code.
> 
> v2:
> * Patch #1: Do not assume the CLEI code is NULL terminated
> 
> [...]

Here is the summary with links:
  - [ethtool-next,v2,1/7] cmis: Fix CLEI code parsing
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=2c2fa88b0578
  - [ethtool-next,v2,2/7] cmis: Fix wrong define name
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=1bad83c00b39
  - [ethtool-next,v2,3/7] cmis: Correct comment
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=001aecdb5119
  - [ethtool-next,v2,4/7] sff-8636: Remove incorrect comment
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=86e97841aca4
  - [ethtool-next,v2,5/7] sff-8636: Fix incorrect function name
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=7ff603badbbb
  - [ethtool-next,v2,6/7] sff-8636: Convert if statement to switch-case
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=128e97c5f6e5
  - [ethtool-next,v2,7/7] sff-8636: Remove extra blank lines
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=79cb4ab4787b

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html