netdev - Re: [net-next 10/10] ixgbe: convert to ndo_fix

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: <20110722080255.GA10125@rere.qmqm.pl>
Date:	Fri, 22 Jul 2011 10:02:55 +0200
From:	Michal Miroslaw <mirq-linux@...e.qmqm.pl>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	davem@...emloft.net, Don Skidmore <donald.c.skidmore@...el.com>,
	netdev@...r.kernel.org, gospo@...hat.com, sassmann@...hat.com
Subject: Re: [net-next 10/10] ixgbe: convert to ndo_fix_features

On Thu, Jul 21, 2011 at 11:09:11PM -0700, Jeff Kirsher wrote:
> From: Don Skidmore <donald.c.skidmore@...el.com>
> 
> Private rx_csum flags are now duplicate of netdev->features &
> NETIF_F_RXCSUM.  We remove those duplicates and now use the net_device_ops
> ndo_set_features.  This was based on the original patch submitted by
> Michal Miroslaw <mirq-linux@...e.qmqm.pl>.  I also removed the special
> case not requiring a reset for X540 hardware.  It is needed just as it is
> in 82599 hardware.

Looks mostly good now. Minor hints below.

[...]
> +static u32 ixgbe_fix_features(struct net_device *netdev, u32 data)
> +{
[...]
> +	/* Turn off LRO if not RSC capable or invalid ITR settings */
> +	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)) {
> +		data &= ~NETIF_F_LRO;
> +	} else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> +		   (adapter->rx_itr_setting != 1 &&
> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)) {
> +		data &= ~NETIF_F_LRO;
> +		e_info(probe, "rx-usecs set too low, not enabling RSC\n");
> +	}

Better:

... else if (data & NETIF_F_LRO && adapter->rx_itr_setting != 1 && adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
	e_info(...)
	data &= ~NETIF_F_LRO;
}

> +
> +	return data;
> +}
> +
> +static int ixgbe_set_features(struct net_device *netdev, u32 data)
> +{
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +	bool need_reset = false;
> +
> +	/* If Rx checksum is disabled, then RSC/LRO should also be disabled */
> +	if (!(data & NETIF_F_RXCSUM))
> +		adapter->flags &= ~IXGBE_FLAG_RX_CSUM_ENABLED;
> +	else
> +		adapter->flags |= IXGBE_FLAG_RX_CSUM_ENABLED;

This exactly mirrors NETIF_F_RXCSUM. Waiting for later cleanup?

[...]
> +	/*
> +	 * Check if Flow Director n-tuple support was enabled or disabled.  If
> +	 * the state changed, we need to reset.
> +	 */
> +	if (!(adapter->flags & IXGBE_FLAG_FDIR_PERFECT_CAPABLE)) {
> +		/* turn off ATR, enable perfect filters and reset */
> +		if (data & NETIF_F_NTUPLE) {
> +			adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
> +			adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> +			need_reset = true;
> +		}
> +	} else if (!(data & NETIF_F_NTUPLE)) {
> +		/* turn off Flow Director, set ATR and reset */
> +		adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
> +		if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
> +		    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
> +			adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
> +		need_reset = true;
> +	}

You could make this more readable:

old = adapter->flags;
if (data & NETIF_F_NTUPLE) {
	adapter->flags |= IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
	adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
} else {
	adapter->flags &= ~IXGBE_FLAG_FDIR_PERFECT_CAPABLE;
	if ((adapter->flags &  IXGBE_FLAG_RSS_ENABLED) &&
	    !(adapter->flags &  IXGBE_FLAG_DCB_ENABLED))
		adapter->flags |= IXGBE_FLAG_FDIR_HASH_CAPABLE;
}
if (old != adapter->flags)
	need_reset = true;

> +
> +	if (need_reset)
> +		ixgbe_do_reset(netdev);
> +
> +	return 0;
> +
> +}
> +
>  static const struct net_device_ops ixgbe_netdev_ops = {
>  	.ndo_open		= ixgbe_open,
>  	.ndo_stop		= ixgbe_close,
> @@ -7153,6 +7228,8 @@ static const struct net_device_ops ixgbe_netdev_ops = {
>  	.ndo_fcoe_disable = ixgbe_fcoe_disable,
>  	.ndo_fcoe_get_wwn = ixgbe_fcoe_get_wwn,
>  #endif /* IXGBE_FCOE */
> +	.ndo_set_features = ixgbe_set_features,
> +	.ndo_fix_features = ixgbe_fix_features,
>  };
>  
>  static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
> @@ -7420,20 +7497,24 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  	netdev->features = NETIF_F_SG |
>  			   NETIF_F_IP_CSUM |
> +			   NETIF_F_IPV6_CSUM |
>  			   NETIF_F_HW_VLAN_TX |
>  			   NETIF_F_HW_VLAN_RX |
> -			   NETIF_F_HW_VLAN_FILTER;
> +			   NETIF_F_HW_VLAN_FILTER |
> +			   NETIF_F_TSO |
> +			   NETIF_F_TSO6 |
> +			   NETIF_F_GRO |
> +			   NETIF_F_RXHASH |
> +			   NETIF_F_RXCSUM;

Drop NETIF_F_GRO here, as its always set by network core now.

> -	netdev->features |= NETIF_F_IPV6_CSUM;
> -	netdev->features |= NETIF_F_TSO;
> -	netdev->features |= NETIF_F_TSO6;
> -	netdev->features |= NETIF_F_GRO;
> -	netdev->features |= NETIF_F_RXHASH;
> +	netdev->hw_features = netdev->features;
>  
>  	switch (adapter->hw.mac.type) {
>  	case ixgbe_mac_82599EB:
>  	case ixgbe_mac_X540:
>  		netdev->features |= NETIF_F_SCTP_CSUM;
> +		netdev->hw_features |= NETIF_F_SCTP_CSUM |
> +				       NETIF_F_NTUPLE;

NTUPLE disabled by default. That's the idea?

Best Regards,
Michał Mirosław
--
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