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] [day] [month] [year] [list]

Message-ID: <AAEA33E297BCAC4B9BB20A7C2DF0AB8D654ECD76@FMSMSX113.amr.corp.intel.com>
Date:	Wed, 8 Jan 2014 16:49:53 +0000
From:	"Williams, Mitch A" <mitch.a.williams@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"Rose, Gregory V" <gregory.v.rose@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 3/8] i40evf: core ethtool functionality



> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Wednesday, January 08, 2014 6:14 AM
> To: Kirsher, Jeffrey T
> Cc: davem@...emloft.net; Rose, Gregory V; netdev@...r.kernel.org;
> gospo@...hat.com; sassmann@...hat.com; Williams, Mitch A
> 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.

Thanks for the review, Ben.  I'll get working on all of the issues you brought up and we'll get some patches out soon.

-Mitch