[Intel-wired-lan] [PATCH net-next 15/19] iecm: implement ethtool callbacks

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Alan Brady <alan.brady@intel.com>
Date: Thu, 3 Feb 2022 03:13:41 +0100

> > -----Original Message-----
> > From: Lobakin, Alexandr <alexandr.lobakin@intel.com>
> > Sent: Friday, January 28, 2022 10:14 AM
> > To: Brady, Alan <alan.brady@intel.com>
> > Cc: Lobakin, Alexandr <alexandr.lobakin@intel.com>; intel-wired-
> > lan at lists.osuosl.org; Burra, Phani R <phani.r.burra@intel.com>; Chittim,
> > Madhu <madhu.chittim@intel.com>; Linga, Pavan Kumar
> > <pavan.kumar.linga@intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH net-next 15/19] iecm: implement
> > ethtool callbacks
> > 
> > From: Alan Brady <alan.brady@intel.com>
> > Date: Thu, 27 Jan 2022 16:10:05 -0800
> > 
> > > This does everything needed to handle ethtool ops minus a few stubs
> > > for cloud filters and other advanced features which will be added in
> > > later in this series.
> > >
> > > Signed-off-by: Phani Burra <phani.r.burra@intel.com>
> > > Signed-off-by: Joshua Hay <joshua.a.hay@intel.com>
> > > Signed-off-by: Madhu Chittim <madhu.chittim@intel.com>
> > > Signed-off-by: Pavan Kumar Linga <pavan.kumar.linga@intel.com>
> > > Signed-off-by: Alan Brady <alan.brady@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/iecm/Makefile      |    1 +
> > >  .../net/ethernet/intel/iecm/iecm_ethtool.c    | 1325
> > +++++++++++++++++
> > >  drivers/net/ethernet/intel/iecm/iecm_lib.c    |   11 +-
> > >  drivers/net/ethernet/intel/include/iecm.h     |    1 +
> > >  4 files changed, 1337 insertions(+), 1 deletion(-)  create mode
> > > 100644 drivers/net/ethernet/intel/iecm/iecm_ethtool.c
> > >
> > > diff --git a/drivers/net/ethernet/intel/iecm/Makefile
> > > b/drivers/net/ethernet/intel/iecm/Makefile
> > > index 205d6f2b436a..fe2ed403d35c 100644
> > > --- a/drivers/net/ethernet/intel/iecm/Makefile
> > > +++ b/drivers/net/ethernet/intel/iecm/Makefile
> > > @@ -15,6 +15,7 @@ iecm-y := \
> > >  	iecm_virtchnl.o \
> > >  	iecm_txrx.o \
> > >  	iecm_singleq_txrx.o \
> > > +	iecm_ethtool.o \
> > >  	iecm_controlq.o \
> > >  	iecm_controlq_setup.o \
> > >  	iecm_main.o
> > > diff --git a/drivers/net/ethernet/intel/iecm/iecm_ethtool.c
> > > b/drivers/net/ethernet/intel/iecm/iecm_ethtool.c
> > > new file mode 100644
> > > index 000000000000..32d905fb1bb6
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/intel/iecm/iecm_ethtool.c
> > > @@ -0,0 +1,1325 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Copyright (C) 2019 Intel Corporation */
> > > +
> > > +#include "iecm.h"
> > > +
> > > +/**
> > > + * iecm_get_rxnfc - command to get RX flow classification rules
> > > + * @netdev: network interface device structure
> > > + * @cmd: ethtool rxnfc command
> > > + * @rule_locs: pointer to store rule locations
> > > + *
> > > + * Returns Success if the command is supported.
> > > + */
> > > +static int iecm_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc
> > *cmd,
> > > +			  u32 __always_unused *rule_locs)
> > 
> > Kernel Kbuild system tell compilers to not complain on unused function
> > arguments.
> > It's pointless to add __always_unused here.
> > 
> 
> Sparse does complain about it (e.g. make ... C=2) and it's present in other (non-Intel) network drivers. I think I need a better argument to remove it.

No, it doesn't.
No, it's not.

Mellanox and ChelsIO almost never mark unused arguments, see e.g.
to [0] and [1] for reference.
And their drivers build cleanly with the latest sparse 0.6.4.

I've been using it for more than two years already and can't recall
a single episode when it would've complained about unused arguments.
If you talk about OOT, please clearly state this, to not confuse me
and other reviewers.

Marking unused arguments as __always_unused is at least a
deprecation and is not recommended for any new code. Kbuild (kernel
build system) explicitly sets -Wno-unused-parameter ([2]) to disable
this warning when building with -Wextra (W=1+).
This is due to that almost any driver has at least one (usually much
more) callback, and if we would've marked every unused argument with
this attribute, there would've been 1.5 times more lines of code.

Is this enough?

> 
> > > +{
> > > +	struct iecm_vport *vport = iecm_netdev_to_vport(netdev);

--- 8< ---

> > > +	if (adapter->virt_ver_maj < VIRTCHNL_VERSION_MAJOR_2) {
> > > +		err = adapter->dev_ops.vc_ops.add_queues(vport,
> > > +							 num_req_tx_q, 0,
> > > +							 num_req_rx_q, 0);
> > > +	} else {
> > > +		err = iecm_initiate_soft_reset(vport,
> > __IECM_SR_Q_CHANGE);
> > > +	}
> > 
> > One-liners, no need for braces.
> > 
> 
> The `if` exists across multiple lines so we would prefer to keep braces. As such the 'else' also gets them.

Please refer to my previous messages. There shouldn't be any braces,
unless they're required for code to work properly (except for
symmetrical if-else).

> 
> > > +
> > > +	if (err) {

--- 8< ---

> > > +
> > > +	is_reset_needed =
> > > +		!!(test_bit(__IECM_PRIV_FLAGS_HDR_SPLIT,
> > change_flags));
> > 
> > Shorter name would allow avoiding line break.
> > 
> 
> For IECM_PRIV_FLAGS_HDR_SPLIT? Sure we're open to suggestion if you have a name that communicates the same level of information effectively.

For `is_reset_needed`. HDR_SPLIT is perfect to me.

> 
> > > +
> > > +	/* Issue reset to cause things to take effect, as additional bits

--- 8< ---

> > > +	case ETH_SS_PRIV_FLAGS:
> > > +		iecm_get_priv_flag_strings(netdev, data);
> > > +		break;
> > > +	default:
> > > +		break;
> > 
> > Equivalent to not having a 'default' case.
> > 
> 
> Yes static tools will complain about uncaught switch without it.

Such as? Can I get some more details please?

Is this again for OOT or for the upstream kernel as well? I think I
can recall some initiatives in the past to enable -Wswitch-default,
but all of them were cancelled due to that it's pointless to have
`default` just to satisfy some checkers and, unlike e.g.
`fallthrough`, this has a very little benefit.
Now you can get it only on W=3 level which nobody takes seriosly.

> 
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * iecm_get_sset_count - Get length of string set

--- 8< ---

> > > +
> > > +		return IECM_PORT_STATS_LEN +
> > > +		       (IECM_TX_QUEUE_STATS_LEN * max_q) +
> > > +			(IECM_RX_QUEUE_STATS_LEN * max_q);
> > > +	} else if (sset == ETH_SS_PRIV_FLAGS) {
> > > +		return IECM_PRIV_FLAGS_STR_LEN;
> > > +	} else {
> > > +		return -EINVAL;
> > 
> > Check for this at first and save 1 level of indentation for ETH_SS_STATS.
> > 
> 
> So you would have it something like:
> 
> if (sset != ETH_SS_STATS && sset != ETH_SS_PRIV_FLAGS)
>    return -EINVAL
> 
> if (sset == ETH_SS_PRIV_FLAGS)
>    return IECM_PRIV_FLAGS_STR_LEN
> 
> /* if sset code */
> 
> This to me seems less readable because now the reader has to identify the bottom half of the function is actually for the SSET case whereas the way it is written currently, it's very explicit about what we're doing for what. I commend the intent to shave off indents where possible but many of these indent suggestions are teetering on excessive and sacrificing readability to save a tab.

You guessed a correct comment block here, if you feel that some code
is not super-intuitive, you can always add a short comment to give
a hint.

What more important for readability is to keep the code as flat as
possible, otherwise the main eye focus should be moved to the right,
plus 79-col limits starts wrapping more and more lines which doesn't
improve readability either.
It means that the longest code blocks should usually have a single
Tab indent.

I'm not speaking for this particular function right now -- I'm fine
with having all of it at 2 Tabs, it's just a big comment block which
makes it look like a big chunk of code. But at least make it a
switch-case then I guess?

> 
> > > +	}
> > > +}
> > > +
> > > +/**
> > > + * iecm_add_one_ethtool_stat - copy the stat into the supplied buffer

--- 8< ---

> > > +	do {
> > > +		start = u64_stats_fetch_begin_irq(&vport-
> > >port_stats.stats_sync);
> > > +		for (i = 0; i < size; i++) {
> > > +			iecm_add_one_ethtool_stat(&(*data)[i], vport,
> > > +
> > &iecm_gstrings_port_stats[i]);
> > > +		}
> > 
> > Redundant braces.
> > 
> 
> I believe this is multi line with wrapping.  Will not fix.

Explained previously.

> 
> > > +	} while (u64_stats_fetch_retry_irq(&vport->port_stats.stats_sync,
> > > +start));
> > > +
> > > +	*data += size;
> > > +}
> > > +
> > > +/**
> > > + * iecm_get_ethtool_stats - report device statistics

--- 8< ---

> > > +void iecm_set_ethtool_ops(struct net_device *netdev) {
> > > +	netdev->ethtool_ops = &iecm_ethtool_ops; }
> > 
> > Declaring @iecm_ethtool_ops as external and directly assigning it in
> > iecm_cfg_netdev() would result in smaller code size than this.
> > 
> 
> It seems trivial either way, but I'm having a hard time justifying this function, so will fix.

Some people believe ops structs should be not only const, but also
static. I don't really get why, but it's somewhat common across
different subsystems.

There's nothing bad in having ethtool_ops global, at the same time
global functions really occupy much more space than global
variables, so just a suggestion here to avoid that.

> 
> > > diff --git a/drivers/net/ethernet/intel/iecm/iecm_lib.c

--- 8< ---

> > > --
> > > 2.33.0

I'm sorry, I'll get back to your replies on patch #16 and further in about
14 hours.

> > 
> > Thanks,
> > Al

[0] https://elixir.bootlin.com/linux/v5.17-rc2/source/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c#L1736
[1] https://elixir.bootlin.com/linux/v5.17-rc2/source/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c#L10
[2] https://elixir.bootlin.com/linux/v5.17-rc2/source/scripts/Makefile.extrawarn#L25

Thanks,
Al