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

On Mon, Jul 25, 2011 at 03:42:09PM -0700, Skidmore, Donald C wrote:

From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Michal Miroslaw

quoted

On Thu, Jul 21, 2011 at 11:09:11PM -0700, Jeff Kirsher wrote:

quoted

From: Don Skidmore <redacted>

Private rx_csum flags are now duplicate of netdev->features &
NETIF_F_RXCSUM.  We remove those duplicates and now use the

net_device_ops

quoted

ndo_set_features.  This was based on the original patch submitted by
Michal Miroslaw [off-list ref].  I also removed the special
case not requiring a reset for X540 hardware.  It is needed just as it

is

quoted

in 82599 hardware.

Looks mostly good now. Minor hints below.

[...]

quoted

+static u32 ixgbe_fix_features(struct net_device *netdev, u32 data)
+{

[...]

quoted

+	/* 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;
}

I see your point here, the added complexity (checking IXGBE_FLAG2_RSC_ENABLED) is there to cover cases that come up in our out of tree driver's kcompat code.  This is why we mirror these feature flags to begin with.  I would like to modify the driver to use only the feature flags but that will require some interesting kcompat code as well as touch large parts of the driver so I was thinking would probably be best as another patch.  

My concern in this conditional is when RSC is not just possible but actually turned on.  Currently the NETIF_F_LRO flag is enabled in probe if we are RSC capable.  But it is possible to be RSC capable but not have it enabled due to setting interrupts throttling rates too high.

You should base decisions in ndo_fix_features on what will be enabled in
following ndo_set_features call, not what is enabled at the time of
ndo_fix_features, unless the dependency is not controlled in ndo_set_features
(e.g. MTU).

Best Regards,
Michał Mirosław