netdev - Re: [net-next 3/8] i40evf: core ethtool functionality

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1389190454.2728.56.camel@deadeye.wl.decadent.org.uk>
Date:	Wed, 8 Jan 2014 14:14:14 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
CC:	<davem@...emloft.net>, Greg Rose <gregory.v.rose@...el.com>,
	<netdev@...r.kernel.org>, <gospo@...hat.com>,
	<sassmann@...hat.com>,
	"Mitch Williams" <mitch.a.williams@...el.com>
Subject: Re: [net-next 3/8] i40evf: core ethtool functionality

On Tue, 2013-12-31 at 16:53 -0800, Jeff Kirsher wrote:
[...]
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/i40evf/i40evf_ethtool.c
[...]
> +#define I40EVF_QUEUE_STATS_LEN \
> +	(((struct i40evf_adapter *) \
> +		netdev_priv(netdev))->vsi_res->num_queue_pairs * 4)

netdev should be a macro parameter, not just assumed as a local
variable.

[...]
> +static int i40evf_get_settings(struct net_device *netdev,
> +			       struct ethtool_cmd *ecmd)
> +{
> +	/* In the future the VF will be able to query the PF for
> +	 * some information - for now use a dummy value
> +	 */
> +	ecmd->supported = SUPPORTED_10000baseT_Full;
> +	ecmd->autoneg = AUTONEG_DISABLE;
> +	ecmd->transceiver = XCVR_DUMMY1;
> +	ecmd->port = PORT_NONE;

This is not even consistent, as its claims that 1000BASE-T is supported
but speed/duplex are always set to 0.  Why not set supported = 0?

> +	return 0;
> +}
[...]
> +static int i40evf_get_sset_count(struct net_device *netdev, int sset)
> +{
> +	if (sset == ETH_SS_STATS)
> +		return I40EVF_STATS_LEN;
> +	else
> +		return -ENOTSUPP;

-EINVAL

> +}
[...]
> +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 || ec->rx_max_coalesced_frames)
> +		vsi->work_limit = ec->tx_max_coalesced_frames;

Why is the actual value of ec->rx_max_coalesced_frames ignored here?
Should this be min() or max() of the two fields?

> +	switch (ec->rx_coalesce_usecs) {
> +	case 0:
> +		vsi->rx_itr_setting = 0;
> +		break;
> +	case 1:
> +		vsi->rx_itr_setting = (I40E_ITR_DYNAMIC
> +				       | ITR_REG_TO_USEC(I40E_ITR_RX_DEF));
> +		break;

This looks a bit magic; why is 1 us interpreted as 'dynamic' (adaptive?)
when there is a separate flag for enabling adaptive moderation?

[...]
> +static struct ethtool_ops i40evf_ethtool_ops = {
[...]

Should be const.

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