On Thu, May 12, 2011 at 05:29:07PM +0100, Ben Hutchings wrote:

On Thu, 2011-05-12 at 18:06 +0200, Michał Mirosław wrote:

quoted

This implements checks for forwarding mode in netdev_fix_features() using
dev->priv_flags bits. As a side effect, after device is no longer
forwarding it gets LRO back. This also means that user is not allowed to
enable LRO after device is put to forwarding mode.

This patch depends on removal of discrete offload setting ethtool ops.

This is nice, but:

[...]

quoted

--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c

@@ -158,6 +158,8 @@ static void del_nbp(struct net_bridge_port *p)
 	br_netpoll_disable(p);
 
 	call_rcu(&p->rcu, destroy_nbp_rcu);
+
+	netdev_update_features(dev);
 }
 
 /* called with RTNL */

@@ -368,11 +370,9 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 
 	dev->priv_flags |= IFF_BRIDGE_PORT;
 
-	dev_disable_lro(dev);
-
 	list_add_rcu(&p->list, &br->port_list);
 
-	netdev_update_features(br->dev);
+	netdev_change_features(dev);
 
 	spin_lock_bh(&br->lock);
 	changed_addr = br_stp_recalculate_bridge_id(br);

Why netdev_change_features() here?  I thought that was primarily for use
when vlan_features may have been changed.

Because this recalculates port's features and then cascades to bridge's
device. I could have written:

	netdev_update_features(dev);
	netdev_update_features(br->dev);

But that might cause redundant recalculations for br->dev (first through
notifier calls). I'll add a comment here.

[...]

quoted

--- a/net/core/dev.c
+++ b/net/core/dev.c

[...]

quoted

@@ -5241,6 +5221,11 @@ u32 netdev_fix_features(struct net_device *dev, u32 features)
 		}
 	}
 
+	if (features & NETIF_F_LRO && dev->priv_flags & IFF_LRO_FORBIDDEN) {
+		netdev_info(dev, "Disabling LRO for forwarding interface.\n");
+		features &= NETIF_F_LRO;

[...]

Mising '~'.

Fixed.

Best Regards,
Michał Mirosław