Re: [net-next 5/5] ixgbe: convert to ndo_fix_features

Good to see the idea converging. :) More hints below.

On Tue, Jul 12, 2011 at 02:28:15PM -0700, Skidmore, Donald C wrote:
[...]

The logic in this conditional is a little complex. I believe it would
end up looking something like the following:

in fix_features:

	/* Prevent change if not RSC capable or invalid ITR setting */
	if (data & NETIF_F_LRO)

That's not a fast path - you can drop (data & NETIF_F_LRO) check.

		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;
		}
	} 

Same for !(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED). After this change,
you might add a call to netdev_update_features() whenever the second part
of the condition is changed by user and you get correctness all the time
'for free'.

in set_feature:

	/* Make sure RSC matches LRO, reset if change */
	if (!!(data & NETIF_F_LRO) != 
	    !!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)) {
		adapter->flags2 ^= IXGBE_FLAG2_RSC_ENABLED;
		switch (adapter->hw.mac.type) {
		case ixgbe_mac_X540:
		case ixgbe_mac_82599EB:
			need_reset = true;
			break;
		default:
			break;	
		}
	}

Correct. Why would you need IXGBE_FLAG2_RSC_ENABLED now, when it just
mirrors NETIF_F_LRO?

[...]

Thanks again for the clarifications, the netdev-features.txt helped a lot too. 

Nice to hear (read) that!

Best Regards,
Michał Mirosław