| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dd574fdf-8e65-4c03-9dd7-105de4cece74@linux.dev>
Date: Mon, 27 Jan 2025 11:44:03 -0800
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Danielle Ratson <danieller@...dia.com>, netdev@...r.kernel.org
Cc: mkubecek@...e.cz, matt@...verse.com.au, daniel.zahka@...il.com,
amcohen@...dia.com, nbu-mlxsw@...hange.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:
> 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@...dia.com>
> ---
> qsfp.c | 144 +++++++++++++++++++++++++++++++++++----------------------
> 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)
> +{
> + 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)
> + 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.
> + i--;
> + }
> +}
> +
> +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,60 +768,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) {
> - int j = 1;
> -
> - if (module_aw_chan_flags[i].type != MODULE_TYPE_SFF8636)
> - 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,
> - 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)
> - 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);
> }
Powered by blists - more mailing lists