Date:	Tue, 27 Aug 2013 18:27:52 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
CC:	<davem@...emloft.net>,
	Mitch A Williams <mitch.a.williams@...el.com>,
	<netdev@...r.kernel.org>, <gospo@...hat.com>, <sassmann@...hat.com>
Subject: Re: [net-next RFC 3/7] i40evf: core ethtool functionality

On Thu, 2013-08-22 at 21:53 -0700, Jeff Kirsher wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
[...]
> +/**
> + * i40evf_get_settings - Get Link Speed and Duplex settings
> + * @netdev: network interface device structure
> + * @ecmd: ethtool command
> + *
> + * Reports speed/duplex settings. Because this is a VF, we don't know what
> + * kind of link we really have, so we fake it.
> + **/
> +static int i40evf_get_settings(struct net_device *netdev,
> +			       struct ethtool_cmd *ecmd)
> +{
> +	ecmd->supported = SUPPORTED_10000baseT_Full;

This is silly.

> +	ecmd->autoneg = AUTONEG_DISABLE;
> +	ecmd->transceiver = XCVR_DUMMY1;
> +	ecmd->port = -1;

PORT_NONE, I think.

> +	return 0;
> +}
> +
> +/**
> + * i40evf_get_sset_count - Get length of string set
> + * @netdev: network interface device structure
> + * @sset: id of string set
> + *
> + * Reports size (in bytes) of string set.

Not in bytes.

>  This driver only supports
> + * strings for statistics.
> + **/
[...]
> +/**
> + * i40evf_get_strings - Get string set
> + * @netdev: network interface device structure
> + * @sset: id of string set
> + * @data: buffer for string data
> + *
> + * Strings are concatenated into the data buffer, separated by nulls.

This looks like a description of a Windows-style double-null-terminated
string list, not an ethtool string set.

> + **/
> +static void i40evf_get_strings(struct net_device *netdev, u32 sset, u8 *data)
> +{
> +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> +	u8 *p = data;
> +	int i;
> +
> +	if (sset == ETH_SS_STATS) {
> +		for (i = 0; i < I40EVF_GLOBAL_STATS_LEN; i++) {
> +			memcpy(p, i40evf_gstrings_stats[i].stat_string,
> +			       ETH_GSTRING_LEN);
> +			p += ETH_GSTRING_LEN;
> +		}
> +		for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> +			snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.packets",
> +				 netdev->name, i);
> +			p += ETH_GSTRING_LEN;
> +			snprintf(p, ETH_GSTRING_LEN, "%s.tx_queue_%u.bytes",
> +				 netdev->name, i);
> +			p += ETH_GSTRING_LEN;
> +		}
> +		for (i = 0; i < adapter->vsi_res->num_queue_pairs; i++) {
> +			snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.packets",
> +				 netdev->name, i);
> +			p += ETH_GSTRING_LEN;
> +			snprintf(p, ETH_GSTRING_LEN, "%s.rx_queue_%u.bytes",
> +				 netdev->name, i);
> +			p += ETH_GSTRING_LEN;

It is completely redundant to put the device name into statistic names,
and is liable to cause the names to be truncated.  There was also a
discussion about how to name per-queue statistics a while back, which
seemed to end in agreement:

http://thread.gmane.org/gmane.linux.network/273014/focus=12205

So I think the name formats should be "tx-%u.packets" etc.

[...]
> +/**
> + * i40evf_get_drvinto - Get driver info
> + * @netdev: network interface device structure
> + * @drvinfo: ethool driver info structure
> + *
> + * Returns information about the driver and device for display to the user.
> + **/
> +static void i40evf_get_drvinfo(struct net_device *netdev,
> +			       struct ethtool_drvinfo *drvinfo)
> +{
> +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> +
> +	strncpy(drvinfo->driver, i40evf_driver_name, 32);
> +	strncpy(drvinfo->version, i40evf_driver_version, 32);
> +
> +	strncpy(drvinfo->fw_version, "N/A", 4);

If there is no firmware version then don't set fw_version at all.

> +	strncpy(drvinfo->bus_info, pci_name(adapter->pdev), 32);

Use strlcpy() not strncpy() for all of the other strings.

> +	drvinfo->n_stats = I40EVF_STATS_LEN;
> +	drvinfo->regdump_len = 0;

Don't set these at all, as they are initialised by the ethtool core
based on other driver operations.

[...]
> +static void i40evf_get_ringparam(struct net_device *netdev,
> +				  struct ethtool_ringparam *ring)
> +{
> +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> +	struct i40e_ring *tx_ring = &adapter->tx_rings[0];
> +	struct i40e_ring *rx_ring = &adapter->rx_rings[0];
> +
> +	ring->rx_max_pending = I40EVF_MAX_RXD;
> +	ring->tx_max_pending = I40EVF_MAX_TXD;
> +	ring->rx_mini_max_pending = 0;
> +	ring->rx_jumbo_max_pending = 0;
> +	ring->rx_pending = rx_ring->count;
> +	ring->tx_pending = tx_ring->count;
> +	ring->rx_mini_pending = 0;
> +	ring->rx_jumbo_pending = 0;
> +}

No need to set the unsupported fields to 0.

[...]
> +static int i40evf_get_coalesce(struct net_device *netdev,
> +			     struct ethtool_coalesce *ec)
> +{
> +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> +	struct i40e_vsi *vsi = &adapter->vsi;
> +
> +	ec->tx_max_coalesced_frames_irq = vsi->work_limit;
> +	ec->rx_max_coalesced_frames_irq = vsi->work_limit;

Use the fields without the '_irq' suffix.  The '_irq' suffixed fields
are there for some NICs that change coalescing behaviour depending on
whether the previous IRQ is still being handled.

[...]
> +static int i40evf_set_coalesce(struct net_device *netdev,
> +			     struct ethtool_coalesce *ec)
> +{
> +	struct i40evf_adapter *adapter = netdev_priv(netdev);
> +	struct i40e_hw *hw = &adapter->hw;
> +	struct i40e_vsi *vsi = &adapter->vsi;
> +	struct i40e_q_vector *q_vector;
> +	int i;
> +
> +	if (ec->tx_max_coalesced_frames_irq || ec->rx_max_coalesced_frames_irq)
> +		vsi->work_limit = ec->tx_max_coalesced_frames_irq;
[...]

Again, use the fields without the '_irq' suffix.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists