| 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:
<DM6PR12MB45162A643F9543C1BC2BB784D8EF2@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Tue, 28 Jan 2025 13:17:49 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>
CC: "mkubecek@...e.cz" <mkubecek@...e.cz>, "matt@...verse.com.au"
<matt@...verse.com.au>, "daniel.zahka@...il.com" <daniel.zahka@...il.com>,
Amit Cohen <amcohen@...dia.com>, NBU-mlxsw <NBU-mlxsw@...hange.nvidia.com>
Subject: RE: [PATCH ethtool-next 05/14] qsfp: Refactor sff8636_show_dom() by
moving code into separate functions
> From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
> Sent: Monday, 27 January 2025 21:44
> To: Danielle Ratson <danieller@...dia.com>; netdev@...r.kernel.org
> Cc: mkubecek@...e.cz; matt@...verse.com.au; daniel.zahka@...il.com;
> Amit Cohen <amcohen@...dia.com>; NBU-mlxsw <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.
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.
>
>
> > + 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