| Message ID | 20250129131547.964711-6-danieller@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | Add JSON output to --module-info | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 29/01/2025 05:15, Danielle Ratson wrote: > 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 <danieller@nvidia.com> > --- > qsfp.c | 126 ++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 80 insertions(+), 46 deletions(-) > > diff --git a/qsfp.c b/qsfp.c > index d272dbf..994ad5f 100644 > --- a/qsfp.c > +++ b/qsfp.c > @@ -649,13 +649,85 @@ 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) > { > - struct sff_diags sd = {0}; > - char *rx_power_string = NULL; > 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) > +{ > + 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) > +{ > + bool value; > + int i; > + > + for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) { > + if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636) > + continue; > + > + 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, > + (i % SFF8636_MAX_CHANNEL_NUM) + 1, Looks like this way will only work when MODULE_TYPE_SFF8636 properties starts at (offset % SFF8636_MAX_CHANNEL_NUM) == 0. Maybe we have to save the offset of the first SFF8636 item in module_aw_chan_flags[] ? > + value ? "On" : "Off"); > + } > +} > + > +static void > +sff8636_show_dom_mod_lvl_flags(const struct sff8636_memory_map *map) > +{ > + int i; > + > + for (i = 0; module_aw_mod_flags[i].str; ++i) { > + if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636) > + 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) > +{ > + 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,51 +759,13 @@ static void sff8636_show_dom(const struct sff8636_memory_map *map) > 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) { > - if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636) > - continue; > - > - 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, > - (i % SFF8636_MAX_CHANNEL_NUM) + 1, > - value ? "On" : "Off"); > - } > - for (i = 0; module_aw_mod_flags[i].str; ++i) { > - if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636) > - 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); > }
> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Sent: Thursday, 30 January 2025 14:26 > To: Danielle Ratson <danieller@nvidia.com>; netdev@vger.kernel.org > Cc: mkubecek@suse.cz; matt@traverse.com.au; daniel.zahka@gmail.com; > Amit Cohen <amcohen@nvidia.com>; NBU-mlxsw <nbu- > mlxsw@exchange.nvidia.com> > Subject: Re: [PATCH ethtool-next v2 05/14] qsfp: Refactor > sff8636_show_dom() by moving code into separate functions > > On 29/01/2025 05:15, Danielle Ratson wrote: > > 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 <danieller@nvidia.com> > > --- > > qsfp.c | 126 ++++++++++++++++++++++++++++++++++++------------------ > --- > > 1 file changed, 80 insertions(+), 46 deletions(-) > > > > diff --git a/qsfp.c b/qsfp.c > > index d272dbf..994ad5f 100644 > > --- a/qsfp.c > > +++ b/qsfp.c > > @@ -649,13 +649,85 @@ 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) > > { > > - struct sff_diags sd = {0}; > > - char *rx_power_string = NULL; > > 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) { > > + 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) > > +{ > > + bool value; > > + int i; > > + > > + for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) { > > + if (module_aw_chan_flags[i].type != > MODULE_TYPE_SFF8636) > > + continue; > > + > > + 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, > > + (i % SFF8636_MAX_CHANNEL_NUM) + 1, > > Looks like this way will only work when MODULE_TYPE_SFF8636 properties > starts at (offset % SFF8636_MAX_CHANNEL_NUM) == 0. Maybe we have to > save the offset of the first SFF8636 item in module_aw_chan_flags[] ? You are basically right, but not sure if it is relevant in this case. Since if the number of channels changes, then all the channels field will be changed accordingly. So it should stay in multiples of SFF8636_MAX_CHANNEL_NUM. > > > + value ? "On" : "Off"); > > + } > > +} > > + > > +static void > > +sff8636_show_dom_mod_lvl_flags(const struct sff8636_memory_map > *map) > > +{ > > + int i; > > + > > + for (i = 0; module_aw_mod_flags[i].str; ++i) { > > + if (module_aw_mod_flags[i].type == > MODULE_TYPE_SFF8636) > > + 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) > { > > + 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,51 +759,13 @@ static void sff8636_show_dom(const struct > sff8636_memory_map *map) > > 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) { > > - if (module_aw_chan_flags[i].type != > MODULE_TYPE_SFF8636) > > - continue; > > - > > - 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, > > - (i % SFF8636_MAX_CHANNEL_NUM) + 1, > > - value ? "On" : "Off"); > > - } > > - for (i = 0; module_aw_mod_flags[i].str; ++i) { > > - if (module_aw_mod_flags[i].type == > MODULE_TYPE_SFF8636) > > - 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); > > }
diff --git a/qsfp.c b/qsfp.c index d272dbf..994ad5f 100644 --- a/qsfp.c +++ b/qsfp.c @@ -649,13 +649,85 @@ 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) { - struct sff_diags sd = {0}; - char *rx_power_string = NULL; 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) +{ + 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) +{ + bool value; + int i; + + for (i = 0; module_aw_chan_flags[i].fmt_str; ++i) { + if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636) + continue; + + 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, + (i % SFF8636_MAX_CHANNEL_NUM) + 1, + value ? "On" : "Off"); + } +} + +static void +sff8636_show_dom_mod_lvl_flags(const struct sff8636_memory_map *map) +{ + int i; + + for (i = 0; module_aw_mod_flags[i].str; ++i) { + if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636) + 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) +{ + 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,51 +759,13 @@ static void sff8636_show_dom(const struct sff8636_memory_map *map) 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) { - if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636) - continue; - - 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, - (i % SFF8636_MAX_CHANNEL_NUM) + 1, - value ? "On" : "Off"); - } - for (i = 0; module_aw_mod_flags[i].str; ++i) { - if (module_aw_mod_flags[i].type == MODULE_TYPE_SFF8636) - 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); }
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 <danieller@nvidia.com> --- qsfp.c | 126 ++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 80 insertions(+), 46 deletions(-)