From: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Sent: Monday, 27 January 2025 21:44
To: Danielle Ratson <redacted>; netdev@vger.kernel.org
Cc: mkubecek@suse.cz; matt@traverse.com.au; daniel.zahka@gmail.com;
Amit Cohen [off-list ref]; NBU-mlxsw <nbu-
mlxsw@exchange.nvidia.com>
Subject: Re: [PATCH ethtool-next 05/14] qsfp: Refactor sff8636_show_dom()
by moving code into separate functions

On 26/01/2025 03:56, Danielle Ratson wrote:

quoted

The sff8636_show_dom() function is quite lengthy, and with the planned
addition of JSON support, it will become even longer and more complex.

To improve readability and maintainability, refactor the function by
moving portions of the code into separate functions, following the
approach used in the cmis.c module.

Signed-off-by: Danielle Ratson <redacted>
---
  qsfp.c | 144 +++++++++++++++++++++++++++++++++++-------------------

---

quoted

  1 file changed, 89 insertions(+), 55 deletions(-)

diff --git a/qsfp.c b/qsfp.c
index 674242c..13d8fb7 100644
--- a/qsfp.c
+++ b/qsfp.c

@@ -649,13 +649,94 @@ out:
  	}
  }

-static void sff8636_show_dom(const struct sff8636_memory_map *map)
+static void sff8636_show_dom_chan_lvl_tx_bias(const struct sff_diags
+*sd) {
+	char power_string[MAX_DESC_SIZE];
+	int i;
+
+	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
+		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
+			 "Laser tx bias current", i+1);
+		PRINT_BIAS(power_string, sd->scd[i].bias_cur);
+	}
+}
+
+static void sff8636_show_dom_chan_lvl_tx_power(const struct sff_diags
+*sd) {
+	char power_string[MAX_DESC_SIZE];
+	int i;
+
+	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
+		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
+			 "Transmit avg optical power", i+1);
+		PRINT_xX_PWR(power_string, sd->scd[i].tx_power);
+	}
+}
+
+static void sff8636_show_dom_chan_lvl_rx_power(const struct sff_diags
+*sd)
  {
-	struct sff_diags sd = {0};
-	char *rx_power_string = NULL;
  	char power_string[MAX_DESC_SIZE];
+	char *rx_power_string = NULL;
+	int i;
+
+	if (!sd->rx_power_type)
+		rx_power_string = "Receiver signal OMA";
+	else
+		rx_power_string = "Rcvr signal avg optical power";
+
+	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
+		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
+			 rx_power_string, i+1);
+		PRINT_xX_PWR(power_string, sd->scd[i].rx_power);
+	}
+}
+
+static void
+sff8636_show_dom_chan_lvl_flags(const struct sff8636_memory_map

*map)

quoted

+{
+	bool value;
  	int i;

+	for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) {
+		int j = 1;
+
+		if (module_aw_chan_flags[i].type !=

MODULE_TYPE_SFF8636)

quoted

+			continue;
+
+		do {
+			value = map-
lower_memory[module_aw_chan_flags[i].offset] &
+				module_aw_chan_flags[i].adver_value;
+			printf("\t%-41s (Chan %d) : %s\n",
+			       module_aw_chan_flags[i].fmt_str, j,
+			       ONOFF(value));
+			j++;
+			i++;
+		}
+		while (module_aw_chan_flags[i].fmt_str &&
+		       strcmp(module_aw_chan_flags[i].fmt_str,
+			      module_aw_chan_flags[i-1].fmt_str) == 0);

Why do we need this complex logic of comparing strings and moving iterators
forth and back? We do have SFF8636_MAX_CHANNEL_NUM in other
functions, and we know that module_aw_chan_flags has items for all
channels, we can simplify the loop here by providing channel number as ((i %
SFF8636_MAX_CHANNEL_NUM) + 1). That will make the code much easier to
read.

This comment actually applies to the previous patch, as current one simply
moves it to a dedicated function. But I think patch 4 and patch 5 can be
merged in one.

Of course you are right ill change that, thanks!
Regarding the patches merge, I think it should be separated since this code is not easy to follow anyway so I think it is better to split as much as possible.

quoted

+		i--;
+	}
+}
+
+static void
+sff8636_show_dom_mod_lvl_flags(const struct sff8636_memory_map

*map)

quoted

+{
+	int i;
+
+	for (i = 0; module_aw_mod_flags[i].str; ++i) {
+		if (module_aw_mod_flags[i].type ==

MODULE_TYPE_SFF8636)

quoted

+			printf("\t%-41s : %s\n",
+			       module_aw_mod_flags[i].str,
+			       ONOFF(map-
lower_memory[module_aw_mod_flags[i].offset]
+				     & module_aw_mod_flags[i].value));
+	}
+}
+
+static void sff8636_show_dom(const struct sff8636_memory_map *map)

{

quoted

+	struct sff_diags sd = {0};
+
  	/*
  	 * There is no clear identifier to signify the existence of
  	 * optical diagnostics similar to SFF-8472. So checking existence

@@ -687,60 +768,13 @@ static void sff8636_show_dom(const struct

sff8636_memory_map *map)

quoted

  	printf("\t%-41s : %s\n", "Alarm/warning flags implemented",
  		(sd.supports_alarms ? "Yes" : "No"));

-	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
-		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
-					"Laser tx bias current", i+1);
-		PRINT_BIAS(power_string, sd.scd[i].bias_cur);
-	}
-
-	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
-		snprintf(power_string, MAX_DESC_SIZE, "%s (Channel %d)",
-					"Transmit avg optical power", i+1);
-		PRINT_xX_PWR(power_string, sd.scd[i].tx_power);
-	}
-
-	if (!sd.rx_power_type)
-		rx_power_string = "Receiver signal OMA";
-	else
-		rx_power_string = "Rcvr signal avg optical power";
-
-	for (i = 0; i < SFF8636_MAX_CHANNEL_NUM; i++) {
-		snprintf(power_string, MAX_DESC_SIZE, "%s(Channel %d)",
-					rx_power_string, i+1);
-		PRINT_xX_PWR(power_string, sd.scd[i].rx_power);
-	}
+	sff8636_show_dom_chan_lvl_tx_bias(&sd);
+	sff8636_show_dom_chan_lvl_tx_power(&sd);
+	sff8636_show_dom_chan_lvl_rx_power(&sd);

  	if (sd.supports_alarms) {
-		bool value;
-
-		for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) {
-			int j = 1;
-
-			if (module_aw_chan_flags[i].type !=

MODULE_TYPE_SFF8636)

quoted

-				continue;
-
-			do {
-				value = map-
lower_memory[module_aw_chan_flags[i].offset] &
-

	module_aw_chan_flags[i].adver_value;

quoted

-				printf("\t%-41s (Chan %d) : %s\n",
-				       module_aw_chan_flags[i].fmt_str, j,
-				       value ? "On" : "Off");
-				j++;
-				i++;
-			}
-			while (module_aw_chan_flags[i].fmt_str &&
-			       strcmp(module_aw_chan_flags[i].fmt_str,
-				      module_aw_chan_flags[i-1].fmt_str) == 0);
-			i--;
-		}
-		for (i = 0; module_aw_mod_flags[i].str; ++i) {
-			if (module_aw_mod_flags[i].type ==

MODULE_TYPE_SFF8636)

quoted

-				printf("\t%-41s : %s\n",
-				       module_aw_mod_flags[i].str,
-				       (map-
lower_memory[module_aw_mod_flags[i].offset]
-				       & module_aw_mod_flags[i].value) ?
-				       "On" : "Off");
-		}
+		sff8636_show_dom_chan_lvl_flags(map);
+		sff8636_show_dom_mod_lvl_flags(map);

  		sff_show_thresholds(sd);
  	}