[RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Jarod Wilson]

There are some netdev features that make little sense to toggle on and
off in a stacked device setup on only one device in the stack. The prime
example is a bonded connection, where it really doesn't make sense to
disable LRO on the master, but not on any of the slaves, nor does it
really make sense to be able to shut LRO off on a slave when its still
enabled on the master.

The strategy here is to add a section near the end of
netdev_fix_features() that looks for upper and lower netdevs, then make
sure certain feature flags match both up and down the stack. At present,
only the LRO flag is included.

This has been successfully tested with bnx2x, qlcnic and netxen network
cards as slaves in a bond interface. Turning LRO on or off on the master
also turns it on or off on each of the slaves, new slaves are added with
LRO in the same state as the master, and LRO can't be toggled on the
slaves.

Also, this should largely remove the need for dev_disable_lro(), and most,
if not all, of its call sites can be replaced by simply making sure
NETIF_F_LRO isn't included in the relevant device's feature flags.

Note that this patch is driven by bug reports from users saying it was
confusing that bonds and slaves had different settings for the same
features, and while it won't be 100% in sync if a lower device doesn't
support a feature like LRO, I think this is a good step in the right
direction.

CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1225b4b..26f4e2d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6261,9 +6261,57 @@ static void rollback_registered(struct net_device *dev)
 	list_del(&single);
 }
 
+static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
+	struct net_device *upper, netdev_features_t features)
+{
+	netdev_features_t want = upper->wanted_features & lower->hw_features;
+
+	if (!(upper->wanted_features & NETIF_F_LRO)
+	    && (features & NETIF_F_LRO)) {
+		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
+			   upper->name);
+		features &= ~NETIF_F_LRO;
+	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
+		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
+			   upper->name);
+		features |= NETIF_F_LRO;
+	}
+
+	return features;
+}
+
+static void netdev_sync_lower_features(struct net_device *upper,
+	struct net_device *lower, netdev_features_t features)
+{
+	netdev_features_t want = features & lower->hw_features;
+
+	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
+		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
+			   lower->name);
+		upper->wanted_features &= ~NETIF_F_LRO;
+		lower->wanted_features &= ~NETIF_F_LRO;
+		netdev_update_features(lower);
+		if (unlikely(lower->features & NETIF_F_LRO))
+			netdev_WARN(upper, "failed to disable LRO on %s!\n",
+				    lower->name);
+	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
+		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
+			   lower->name);
+		upper->wanted_features |= NETIF_F_LRO;
+		lower->wanted_features |= NETIF_F_LRO;
+		netdev_update_features(lower);
+		if (unlikely(!(lower->features & NETIF_F_LRO)))
+			netdev_WARN(upper, "failed to enable LRO on %s!\n",
+				    lower->name);
+	}
+}
+
 static netdev_features_t netdev_fix_features(struct net_device *dev,
 	netdev_features_t features)
 {
+	struct net_device *upper, *lower;
+	struct list_head *iter;
+
 	/* Fix illegal checksum combinations */
 	if ((features & NETIF_F_HW_CSUM) &&
 	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		}
 	}
 
+	/* some features should be kept in sync with upper devices */
+	upper = netdev_master_upper_dev_get(dev);
+	if (upper)
+		features = netdev_sync_upper_features(dev, upper, features);
+
+	/* lower devices need some features altered to match upper devices */
+	netdev_for_each_lower_dev(dev, lower, iter)
+		netdev_sync_lower_features(dev, lower, features);
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	if (dev->netdev_ops->ndo_busy_poll)
 		features |= NETIF_F_BUSY_POLL;
-- 
1.8.3.1

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Tom Herbert]

On Fri, Oct 23, 2015 at 11:40 PM, Jarod Wilson <jarod@redhat.com> wrote:
> There are some netdev features that make little sense to toggle on and
> off in a stacked device setup on only one device in the stack. The prime
> example is a bonded connection, where it really doesn't make sense to
> disable LRO on the master, but not on any of the slaves, nor does it
> really make sense to be able to shut LRO off on a slave when its still
> enabled on the master.
>
> The strategy here is to add a section near the end of
> netdev_fix_features() that looks for upper and lower netdevs, then make
> sure certain feature flags match both up and down the stack. At present,
> only the LRO flag is included.
>
> This has been successfully tested with bnx2x, qlcnic and netxen network
> cards as slaves in a bond interface. Turning LRO on or off on the master
> also turns it on or off on each of the slaves, new slaves are added with
> LRO in the same state as the master, and LRO can't be toggled on the
> slaves.
>
> Also, this should largely remove the need for dev_disable_lro(), and most,
> if not all, of its call sites can be replaced by simply making sure
> NETIF_F_LRO isn't included in the relevant device's feature flags.
>
> Note that this patch is driven by bug reports from users saying it was
> confusing that bonds and slaves had different settings for the same
> features, and while it won't be 100% in sync if a lower device doesn't
> support a feature like LRO, I think this is a good step in the right
> direction.
>
I don't see what real problem this is solving. LRO is purely a feature
of physical devices and should be irrelevant to be configured on any
type of virtual device. I think the same thing will be true of RX csum
and other device RX functions (but this is not true for transmit
features). Seems like a better fix might be to disallow setting these
features on the bonding device in the first place, then we don't need
to worry about syncing them amongst slaves-- if a user needs that it's
a simple script.

Tom

> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1225b4b..26f4e2d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6261,9 +6261,57 @@ static void rollback_registered(struct net_device *dev)
>         list_del(&single);
>  }
>
> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> +       struct net_device *upper, netdev_features_t features)
> +{
> +       netdev_features_t want = upper->wanted_features & lower->hw_features;
> +
> +       if (!(upper->wanted_features & NETIF_F_LRO)
> +           && (features & NETIF_F_LRO)) {
> +               netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> +                          upper->name);
> +               features &= ~NETIF_F_LRO;
> +       } else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> +               netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> +                          upper->name);
> +               features |= NETIF_F_LRO;
> +       }
> +
> +       return features;
> +}
> +
> +static void netdev_sync_lower_features(struct net_device *upper,
> +       struct net_device *lower, netdev_features_t features)
> +{
> +       netdev_features_t want = features & lower->hw_features;
> +
> +       if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> +               netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> +                          lower->name);
> +               upper->wanted_features &= ~NETIF_F_LRO;
> +               lower->wanted_features &= ~NETIF_F_LRO;
> +               netdev_update_features(lower);
> +               if (unlikely(lower->features & NETIF_F_LRO))
> +                       netdev_WARN(upper, "failed to disable LRO on %s!\n",
> +                                   lower->name);
> +       } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> +               netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> +                          lower->name);
> +               upper->wanted_features |= NETIF_F_LRO;
> +               lower->wanted_features |= NETIF_F_LRO;
> +               netdev_update_features(lower);
> +               if (unlikely(!(lower->features & NETIF_F_LRO)))
> +                       netdev_WARN(upper, "failed to enable LRO on %s!\n",
> +                                   lower->name);
> +       }
> +}
> +
>  static netdev_features_t netdev_fix_features(struct net_device *dev,
>         netdev_features_t features)
>  {
> +       struct net_device *upper, *lower;
> +       struct list_head *iter;
> +
>         /* Fix illegal checksum combinations */
>         if ((features & NETIF_F_HW_CSUM) &&
>             (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> @@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>                 }
>         }
>
> +       /* some features should be kept in sync with upper devices */
> +       upper = netdev_master_upper_dev_get(dev);
> +       if (upper)
> +               features = netdev_sync_upper_features(dev, upper, features);
> +
> +       /* lower devices need some features altered to match upper devices */
> +       netdev_for_each_lower_dev(dev, lower, iter)
> +               netdev_sync_lower_features(dev, lower, features);
> +
>  #ifdef CONFIG_NET_RX_BUSY_POLL
>         if (dev->netdev_ops->ndo_busy_poll)
>                 features |= NETIF_F_BUSY_POLL;
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Alexander Duyck]

On 10/23/2015 08:40 PM, Jarod Wilson wrote:
> There are some netdev features that make little sense to toggle on and
> off in a stacked device setup on only one device in the stack. The prime
> example is a bonded connection, where it really doesn't make sense to
> disable LRO on the master, but not on any of the slaves, nor does it
> really make sense to be able to shut LRO off on a slave when its still
> enabled on the master.
>
> The strategy here is to add a section near the end of
> netdev_fix_features() that looks for upper and lower netdevs, then make
> sure certain feature flags match both up and down the stack. At present,
> only the LRO flag is included.
>
> This has been successfully tested with bnx2x, qlcnic and netxen network
> cards as slaves in a bond interface. Turning LRO on or off on the master
> also turns it on or off on each of the slaves, new slaves are added with
> LRO in the same state as the master, and LRO can't be toggled on the
> slaves.
>
> Also, this should largely remove the need for dev_disable_lro(), and most,
> if not all, of its call sites can be replaced by simply making sure
> NETIF_F_LRO isn't included in the relevant device's feature flags.
>
> Note that this patch is driven by bug reports from users saying it was
> confusing that bonds and slaves had different settings for the same
> features, and while it won't be 100% in sync if a lower device doesn't
> support a feature like LRO, I think this is a good step in the right
> direction.
>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>   net/core/dev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1225b4b..26f4e2d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6261,9 +6261,57 @@ static void rollback_registered(struct net_device *dev)
>   	list_del(&single);
>   }
>
> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> +	struct net_device *upper, netdev_features_t features)
> +{
> +	netdev_features_t want = upper->wanted_features & lower->hw_features;
> +
> +	if (!(upper->wanted_features & NETIF_F_LRO)
> +	    && (features & NETIF_F_LRO)) {
> +		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> +			   upper->name);
> +		features &= ~NETIF_F_LRO;
> +	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> +		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> +			   upper->name);
> +		features |= NETIF_F_LRO;
> +	}
> +
> +	return features;
> +}
> +

I'd say to drop the second half of this statement.  LRO is a feature 
that should be enabled explicitly per interface.  If someone enables LRO 
on the master they may only want it on one interface.  The fact is there 
are some implementations of LRO that work better than others so you want 
to give the end user the option to mix and match.

> +static void netdev_sync_lower_features(struct net_device *upper,
> +	struct net_device *lower, netdev_features_t features)
> +{
> +	netdev_features_t want = features & lower->hw_features;
> +
> +	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> +		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> +			   lower->name);
> +		upper->wanted_features &= ~NETIF_F_LRO;
> +		lower->wanted_features &= ~NETIF_F_LRO;
> +		netdev_update_features(lower);
> +		if (unlikely(lower->features & NETIF_F_LRO))
> +			netdev_WARN(upper, "failed to disable LRO on %s!\n",
> +				    lower->name);
> +	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> +		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> +			   lower->name);
> +		upper->wanted_features |= NETIF_F_LRO;
> +		lower->wanted_features |= NETIF_F_LRO;
> +		netdev_update_features(lower);
> +		if (unlikely(!(lower->features & NETIF_F_LRO)))
> +			netdev_WARN(upper, "failed to enable LRO on %s!\n",
> +				    lower->name);
> +	}
> +}
> +

Same thing here.  If a lower dev has it disabled then leave it disabled. 
  I believe your goal is to make it so that dev_disable_lro() can shut 
down LRO when it is making packets in the data-path unusable.  There is 
no need to make this an all or nothing scenario.  We can let the stack 
slam things down with dev_disable_lro() and then if a user so desires 
they can come back through and enable LRO more selectively if they for 
instance have an interface that can do a smarter job of putting together 
frames that could be routed.

You could probably look at doing something like this for RXCSUM as well. 
  The general idea is that if an upper device has it off then the value 
has to be off.  For example if RXCSUM is off in a upper device and LRO 
is enabled on the lower device there is a good chance that the upper 
device will report checksum errors since most LRO implementations don't 
recalculate the checksum.  If RXCSUM is forced down to the lower device 
hopefully its fix_features will know this and disable LRO on that device 
when the RXCSUM is disabled on it.

>   static netdev_features_t netdev_fix_features(struct net_device *dev,
>   	netdev_features_t features)
>   {
> +	struct net_device *upper, *lower;
> +	struct list_head *iter;
> +
>   	/* Fix illegal checksum combinations */
>   	if ((features & NETIF_F_HW_CSUM) &&
>   	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> @@ -6318,6 +6366,15 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>   		}
>   	}
>
> +	/* some features should be kept in sync with upper devices */
> +	upper = netdev_master_upper_dev_get(dev);
> +	if (upper)
> +		features = netdev_sync_upper_features(dev, upper, features);
> +
> +	/* lower devices need some features altered to match upper devices */
> +	netdev_for_each_lower_dev(dev, lower, iter)
> +		netdev_sync_lower_features(dev, lower, features);
> +
>   #ifdef CONFIG_NET_RX_BUSY_POLL
>   	if (dev->netdev_ops->ndo_busy_poll)
>   		features |= NETIF_F_BUSY_POLL;
>

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Michal Kubecek]

On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
> >
> >+static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> >+	struct net_device *upper, netdev_features_t features)
> >+{
> >+	netdev_features_t want = upper->wanted_features & lower->hw_features;
> >+
> >+	if (!(upper->wanted_features & NETIF_F_LRO)
> >+	    && (features & NETIF_F_LRO)) {
> >+		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
> >+			   upper->name);
> >+		features &= ~NETIF_F_LRO;
> >+	} else if ((want & NETIF_F_LRO) && !(features & NETIF_F_LRO)) {
> >+		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
> >+			   upper->name);
> >+		features |= NETIF_F_LRO;
> >+	}
> >+
> >+	return features;
> >+}
> >+
> 
> I'd say to drop the second half of this statement.  LRO is a feature
> that should be enabled explicitly per interface.  If someone enables
> LRO on the master they may only want it on one interface.  The fact
> is there are some implementations of LRO that work better than
> others so you want to give the end user the option to mix and match.

Agreed. IMHO it makes sense to allow setups with LRO disabled on some
slaves and enabled on other.

Also, the logic seems to only consider the 1 upper : N lower scheme
(bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
For these, there is no way to propagate both 0 and 1 down as this would
result in a conflict.

> >+static void netdev_sync_lower_features(struct net_device *upper,
> >+	struct net_device *lower, netdev_features_t features)
> >+{
> >+	netdev_features_t want = features & lower->hw_features;
> >+
> >+	if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
> >+		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
> >+			   lower->name);
> >+		upper->wanted_features &= ~NETIF_F_LRO;
> >+		lower->wanted_features &= ~NETIF_F_LRO;
> >+		netdev_update_features(lower);
> >+		if (unlikely(lower->features & NETIF_F_LRO))
> >+			netdev_WARN(upper, "failed to disable LRO on %s!\n",
> >+				    lower->name);
> >+	} else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
> >+		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
> >+			   lower->name);
> >+		upper->wanted_features |= NETIF_F_LRO;
> >+		lower->wanted_features |= NETIF_F_LRO;
> >+		netdev_update_features(lower);
> >+		if (unlikely(!(lower->features & NETIF_F_LRO)))
> >+			netdev_WARN(upper, "failed to enable LRO on %s!\n",
> >+				    lower->name);
> >+	}
> >+}
> >+
> 
> Same thing here.  If a lower dev has it disabled then leave it
> disabled.  I believe your goal is to make it so that
> dev_disable_lro() can shut down LRO when it is making packets in the
> data-path unusable.

This is already the case since commit fbe168ba91f7 ("net: generic
dev_disable_lro() stacked device handling"). That commit makes sure
dev_disable_lro() is propagated down the stack and also makes sure new
slaves added to a bond/team with LRO disabled have it disabled too.

What it does not do is propagating LRO disabling down if it is disabled
in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
sure if this should be done or not, both options have their pros and
cons. However, I believe enabling LRO shouldn't be propagated down.

                                                         Michal Kubecek

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Jarod Wilson]

Michal Kubecek wrote:
> On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
>>> +	struct net_device *upper, netdev_features_t features)
>>> +{
>>> +	netdev_features_t want = upper->wanted_features&  lower->hw_features;
>>> +
>>> +	if (!(upper->wanted_features&  NETIF_F_LRO)
>>> +	&&  (features&  NETIF_F_LRO)) {
>>> +		netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
>>> +			   upper->name);
>>> +		features&= ~NETIF_F_LRO;
>>> +	} else if ((want&  NETIF_F_LRO)&&  !(features&  NETIF_F_LRO)) {
>>> +		netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
>>> +			   upper->name);
>>> +		features |= NETIF_F_LRO;
>>> +	}
>>> +
>>> +	return features;
>>> +}
>>> +
>> I'd say to drop the second half of this statement.  LRO is a feature
>> that should be enabled explicitly per interface.  If someone enables
>> LRO on the master they may only want it on one interface.  The fact
>> is there are some implementations of LRO that work better than
>> others so you want to give the end user the option to mix and match.
>
> Agreed. IMHO it makes sense to allow setups with LRO disabled on some
> slaves and enabled on other.
>
> Also, the logic seems to only consider the 1 upper : N lower scheme
> (bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
> For these, there is no way to propagate both 0 and 1 down as this would
> result in a conflict.

Okay, so we're thinking do prevent lower devices turning LRO on if the 
upper device has it off. Or rather, if *an* upper device has it off. 
Probably need to rework the bit that calls this function to use 
netdev_for_each_upper_dev{_rcu}() to walk all of adj_list.upper here.

Rather than outright dropping the second bit though, I was thinking 
maybe just drop a note in dmesg along the lines of "hey, you shut off 
LRO, it is still enabled on upper dev foo", to placate end-users.


>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>> +	struct net_device *lower, netdev_features_t features)
>>> +{
>>> +	netdev_features_t want = features&  lower->hw_features;
>>> +
>>> +	if (!(features&  NETIF_F_LRO)&&  (lower->features&  NETIF_F_LRO)) {
>>> +		netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>> +			   lower->name);
>>> +		upper->wanted_features&= ~NETIF_F_LRO;
>>> +		lower->wanted_features&= ~NETIF_F_LRO;
>>> +		netdev_update_features(lower);
>>> +		if (unlikely(lower->features&  NETIF_F_LRO))
>>> +			netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>> +				    lower->name);
>>> +	} else if ((want&  NETIF_F_LRO)&&  !(lower->features&  NETIF_F_LRO)) {
>>> +		netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>> +			   lower->name);
>>> +		upper->wanted_features |= NETIF_F_LRO;
>>> +		lower->wanted_features |= NETIF_F_LRO;
>>> +		netdev_update_features(lower);
>>> +		if (unlikely(!(lower->features&  NETIF_F_LRO)))
>>> +			netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>> +				    lower->name);
>>> +	}
>>> +}
>>> +
>> Same thing here.  If a lower dev has it disabled then leave it
>> disabled.  I believe your goal is to make it so that
>> dev_disable_lro() can shut down LRO when it is making packets in the
>> data-path unusable.
>
> This is already the case since commit fbe168ba91f7 ("net: generic
> dev_disable_lro() stacked device handling"). That commit makes sure
> dev_disable_lro() is propagated down the stack and also makes sure new
> slaves added to a bond/team with LRO disabled have it disabled too.
>
> What it does not do is propagating LRO disabling down if it is disabled
> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
> sure if this should be done or not, both options have their pros and
> cons.

Making it work with ethtool was one of my primary goals with this 
change, as it was users prodding things with ethtool that prompted the 
"hey, this doesn't make sense" bug reports.

> However, I believe enabling LRO shouldn't be propagated down.

Hm. Devices that should never have LRO enabled still won't get it 
enabled, so I'm not clear what harm it would cause. I tend to think you 
do want this sync'ing down the stack if set on an upper dev (i.e., 
ethtool -K bond0 lro on), for consistency's sake. You can always come 
back through afterwards and disable things on lower devs individually if 
they're really not wanted, since we're in agreement that we shouldn't 
prevent disabling features on lower devices.

-- 
Jarod Wilson
jarod@redhat.com

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Alexander Duyck]

On 10/30/2015 09:25 AM, Jarod Wilson wrote:
> Michal Kubecek wrote:
>> On Fri, Oct 23, 2015 at 10:51:09PM -0700, Alexander Duyck wrote:
>>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>>> +static netdev_features_t netdev_sync_upper_features(struct
>>>> net_device *lower,
>>>> +    struct net_device *upper, netdev_features_t features)
>>>> +{
>>>> +    netdev_features_t want = upper->wanted_features&
>>>> lower->hw_features;
>>>> +
>>>> +    if (!(upper->wanted_features&  NETIF_F_LRO)
>>>> +    &&  (features&  NETIF_F_LRO)) {
>>>> +        netdev_info(lower, "Dropping LRO, upper dev %s has it off.\n",
>>>> +               upper->name);
>>>> +        features&= ~NETIF_F_LRO;
>>>> +    } else if ((want&  NETIF_F_LRO)&&  !(features&  NETIF_F_LRO)) {
>>>> +        netdev_info(lower, "Keeping LRO, upper dev %s has it on.\n",
>>>> +               upper->name);
>>>> +        features |= NETIF_F_LRO;
>>>> +    }
>>>> +
>>>> +    return features;
>>>> +}
>>>> +
>>> I'd say to drop the second half of this statement.  LRO is a feature
>>> that should be enabled explicitly per interface.  If someone enables
>>> LRO on the master they may only want it on one interface.  The fact
>>> is there are some implementations of LRO that work better than
>>> others so you want to give the end user the option to mix and match.
>>
>> Agreed. IMHO it makes sense to allow setups with LRO disabled on some
>> slaves and enabled on other.
>>
>> Also, the logic seems to only consider the 1 upper : N lower scheme
>> (bond, team) but we also have N upper : 1 lower setups (vlan, macvlan).
>> For these, there is no way to propagate both 0 and 1 down as this would
>> result in a conflict.
>
> Okay, so we're thinking do prevent lower devices turning LRO on if the
> upper device has it off. Or rather, if *an* upper device has it off.
> Probably need to rework the bit that calls this function to use
> netdev_for_each_upper_dev{_rcu}() to walk all of adj_list.upper here.

Right.  This part sounds fine.

> Rather than outright dropping the second bit though, I was thinking
> maybe just drop a note in dmesg along the lines of "hey, you shut off
> LRO, it is still enabled on upper dev foo", to placate end-users.

I would rather not see it.  It would be mostly noise.  It is perfectly 
valid to have LRO advertised on an upper device, but not supported on a 
lower one.  It basically just means that the path will allow LRO frames 
through, it doesn't guarantee that we are going to provide them.

>>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>>> +    struct net_device *lower, netdev_features_t features)
>>>> +{
>>>> +    netdev_features_t want = features&  lower->hw_features;
>>>> +
>>>> +    if (!(features&  NETIF_F_LRO)&&  (lower->features&
>>>> NETIF_F_LRO)) {
>>>> +        netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>>> +               lower->name);
>>>> +        upper->wanted_features&= ~NETIF_F_LRO;
>>>> +        lower->wanted_features&= ~NETIF_F_LRO;
>>>> +        netdev_update_features(lower);
>>>> +        if (unlikely(lower->features&  NETIF_F_LRO))
>>>> +            netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>>> +                    lower->name);
>>>> +    } else if ((want&  NETIF_F_LRO)&&  !(lower->features&
>>>> NETIF_F_LRO)) {
>>>> +        netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>>> +               lower->name);
>>>> +        upper->wanted_features |= NETIF_F_LRO;
>>>> +        lower->wanted_features |= NETIF_F_LRO;
>>>> +        netdev_update_features(lower);
>>>> +        if (unlikely(!(lower->features&  NETIF_F_LRO)))
>>>> +            netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>>> +                    lower->name);
>>>> +    }
>>>> +}
>>>> +
>>> Same thing here.  If a lower dev has it disabled then leave it
>>> disabled.  I believe your goal is to make it so that
>>> dev_disable_lro() can shut down LRO when it is making packets in the
>>> data-path unusable.
>>
>> This is already the case since commit fbe168ba91f7 ("net: generic
>> dev_disable_lro() stacked device handling"). That commit makes sure
>> dev_disable_lro() is propagated down the stack and also makes sure new
>> slaves added to a bond/team with LRO disabled have it disabled too.
>>
>> What it does not do is propagating LRO disabling down if it is disabled
>> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
>> sure if this should be done or not, both options have their pros and
>> cons.
>
> Making it work with ethtool was one of my primary goals with this
> change, as it was users prodding things with ethtool that prompted the
> "hey, this doesn't make sense" bug reports.

I'd say make it work like dev_disable_lro already does.  Disabling LRO 
propagates down, enabling LRO only enables it on the specific device.

The way to think of it is as a warning flag.  With LRO enabled this 
device may report frames larger than MTU to the stack and will mangle 
checksums.  Without LRO all of the frames received should be restricted 
to MTU.  That is why you have to force the disabling down to all lower 
devices, and why you cannot enable it if an upper device has it disabled.

>> However, I believe enabling LRO shouldn't be propagated down.
>
> Hm. Devices that should never have LRO enabled still won't get it
> enabled, so I'm not clear what harm it would cause.I tend to think you

How do you define "devices that should never have LRO enabled"?  The 
fact is LRO is very messy in terms of the way it functions.  Different 
drivers handle it different ways.  Usually it results in the Rx checksum 
being mangled, it provides frames larger than MTU, and uses fraglist 
instead of frags on some drivers.

> do want this sync'ing down the stack if set on an upper dev (i.e.,
> ethtool -K bond0 lro on), for consistency's sake. You can always come
> back through afterwards and disable things on lower devs individually if
> they're really not wanted, since we're in agreement that we shouldn't
> prevent disabling features on lower devices.

Think of it this way.  Lets say I have a NIC that I know is problematic 
when LRO is enabled, it might cause a kernel panic due to an skb 
overrun.  So I have a bond with it and some other NIC which can run with 
LRO enabled without issues.  How do I enable LRO on the other device 
without causing a kernel panic, and without tearing apart the existing 
bond?  With the approach you have described I can't because I have to 
enable it at the bond and doing so will enable it on the NIC with the 
faulty implementation.

This is why we cannot enable LRO unless all upper devices support it, 
and why we should propagate disabling LRO down to all lower devices. 
Trying to force it on for a lower device just because the upper device 
supports it is a bad idea because there are multiple LRO implementations 
and they all behave very differently.

- Alex

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Jarod Wilson]

Alexander Duyck wrote:
> On 10/30/2015 09:25 AM, Jarod Wilson wrote:
...
>> Rather than outright dropping the second bit though, I was thinking
>> maybe just drop a note in dmesg along the lines of "hey, you shut off
>> LRO, it is still enabled on upper dev foo", to placate end-users.
>
> I would rather not see it. It would be mostly noise. It is perfectly
> valid to have LRO advertised on an upper device, but not supported on a
> lower one. It basically just means that the path will allow LRO frames
> through, it doesn't guarantee that we are going to provide them.

Okay, dropping this.

...
>>>> Same thing here. If a lower dev has it disabled then leave it
>>>> disabled. I believe your goal is to make it so that
>>>> dev_disable_lro() can shut down LRO when it is making packets in the
>>>> data-path unusable.
>>>
>>> This is already the case since commit fbe168ba91f7 ("net: generic
>>> dev_disable_lro() stacked device handling"). That commit makes sure
>>> dev_disable_lro() is propagated down the stack and also makes sure new
>>> slaves added to a bond/team with LRO disabled have it disabled too.
>>>
>>> What it does not do is propagating LRO disabling down if it is disabled
>>> in ways that do not call dev_disable_lro() (e.g. via ethtool). I'm not
>>> sure if this should be done or not, both options have their pros and
>>> cons.
>>
>> Making it work with ethtool was one of my primary goals with this
>> change, as it was users prodding things with ethtool that prompted the
>> "hey, this doesn't make sense" bug reports.
>
> I'd say make it work like dev_disable_lro already does. Disabling LRO
> propagates down, enabling LRO only enables it on the specific device.
>
> The way to think of it is as a warning flag. With LRO enabled this
> device may report frames larger than MTU to the stack and will mangle
> checksums. Without LRO all of the frames received should be restricted
> to MTU. That is why you have to force the disabling down to all lower
> devices, and why you cannot enable it if an upper device has it disabled.
>
>>> However, I believe enabling LRO shouldn't be propagated down.
>>
>> Hm. Devices that should never have LRO enabled still won't get it
>> enabled, so I'm not clear what harm it would cause.I tend to think you
>
> How do you define "devices that should never have LRO enabled"?

No NETIF_F_LRO flag set in hw_features is what I was thinking.

> The fact
> is LRO is very messy in terms of the way it functions. Different drivers
> handle it different ways. Usually it results in the Rx checksum being
> mangled, it provides frames larger than MTU, and uses fraglist instead
> of frags on some drivers.
>
>> do want this sync'ing down the stack if set on an upper dev (i.e.,
>> ethtool -K bond0 lro on), for consistency's sake. You can always come
>> back through afterwards and disable things on lower devs individually if
>> they're really not wanted, since we're in agreement that we shouldn't
>> prevent disabling features on lower devices.
>
> Think of it this way. Lets say I have a NIC that I know is problematic
> when LRO is enabled, it might cause a kernel panic due to an skb
> overrun. So I have a bond with it and some other NIC which can run with
> LRO enabled without issues. How do I enable LRO on the other device
> without causing a kernel panic, and without tearing apart the existing
> bond? With the approach you have described I can't because I have to
> enable it at the bond and doing so will enable it on the NIC with the
> faulty implementation.

I'd argue that if enabling LRO on a device causes a panic, that device 
probably shouldn't be advertising LRO support, and the driver ought to 
be fixed, but that's somewhat tangential. I'm already sold on only 
disabling down the stack.

> This is why we cannot enable LRO unless all upper devices support it,
> and why we should propagate disabling LRO down to all lower devices.
> Trying to force it on for a lower device just because the upper device
> supports it is a bad idea because there are multiple LRO implementations
> and they all behave very differently.

That's a bit concerning, given that we default to LRO on in a bond, as 
should all the slaves, regardless of which LRO implementation the device 
has (so long as the driver claims to support LRO, anyway).

But again, that's probably a separate issue, I've got a forthcoming 
patch that I'm still beating around and touching up, but I think looks 
sane and lines up with what you've suggested.

> If nothing else you might start looking at working with a mask of
> bits that function like this.  You could probably start with GRO,
> LRO, and RXCSUM and work your way up from there.  If they aren't set
> on the upper devices you cannot enable them, and if they are cleared
> then they must be cleared on all lower devices.

For step one, I've added a feature mask and a new helper that iterates 
over it looking for set feature flags. In the case of the bnx2x equipped 
host I'm currently testing on, adding RXCSUM had an interesting and as 
yet unexplained side-effect of preventing LRO from being enabled on the 
bnx2x cards -- ethtool showed "off [requested on]".

-- 
Jarod Wilson
jarod@redhat.com

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Jarod Wilson]

Alexander Duyck wrote:
> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>> There are some netdev features that make little sense to toggle on and
>> off in a stacked device setup on only one device in the stack. The prime
>> example is a bonded connection, where it really doesn't make sense to
>> disable LRO on the master, but not on any of the slaves, nor does it
>> really make sense to be able to shut LRO off on a slave when its still
>> enabled on the master.
>>
>> The strategy here is to add a section near the end of
>> netdev_fix_features() that looks for upper and lower netdevs, then make
>> sure certain feature flags match both up and down the stack. At present,
>> only the LRO flag is included.
...
>> +static void netdev_sync_lower_features(struct net_device *upper,
>> + struct net_device *lower, netdev_features_t features)
>> +{
>> + netdev_features_t want = features & lower->hw_features;
>> +
>> + if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
>> + netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>> + lower->name);
>> + upper->wanted_features &= ~NETIF_F_LRO;
>> + lower->wanted_features &= ~NETIF_F_LRO;
>> + netdev_update_features(lower);
>> + if (unlikely(lower->features & NETIF_F_LRO))
>> + netdev_WARN(upper, "failed to disable LRO on %s!\n",
>> + lower->name);
>> + } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
>> + netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>> + lower->name);
>> + upper->wanted_features |= NETIF_F_LRO;
>> + lower->wanted_features |= NETIF_F_LRO;
>> + netdev_update_features(lower);
>> + if (unlikely(!(lower->features & NETIF_F_LRO)))
>> + netdev_WARN(upper, "failed to enable LRO on %s!\n",
>> + lower->name);
>> + }
>> +}
>> +
>
> Same thing here. If a lower dev has it disabled then leave it disabled.
> I believe your goal is to make it so that dev_disable_lro() can shut
> down LRO when it is making packets in the data-path unusable. There is
> no need to make this an all or nothing scenario. We can let the stack
> slam things down with dev_disable_lro() and then if a user so desires
> they can come back through and enable LRO more selectively if they for
> instance have an interface that can do a smarter job of putting together
> frames that could be routed.
>
> You could probably look at doing something like this for RXCSUM as well.
> The general idea is that if an upper device has it off then the value
> has to be off. For example if RXCSUM is off in a upper device and LRO is
> enabled on the lower device there is a good chance that the upper device
> will report checksum errors since most LRO implementations don't
> recalculate the checksum. If RXCSUM is forced down to the lower device
> hopefully its fix_features will know this and disable LRO on that device
> when the RXCSUM is disabled on it.

Yeah, I was thinking there might be more flags to treat the same way, 
just wanted to hammer out the plausibility of doing it at all first. I 
can add RXCSUM to v2, or just wait until there's something that people 
might consider merge-worthy before worrying about additional flags. From 
what I've seen, most device's fix_features are reasonably intelligent 
about allowing/disallowing certain flag combos, so this does look pretty 
safe at a glance, and if a specific device tips over, it probably needs 
to be fixed in the device's driver anyway.

-- 
Jarod Wilson
jarod@redhat.com

Re: [RFC PATCH net-next] net/core: initial support for stacked dev feature toggles

[Alexander Duyck]

On 10/30/2015 09:35 AM, Jarod Wilson wrote:
> Alexander Duyck wrote:
>> On 10/23/2015 08:40 PM, Jarod Wilson wrote:
>>> There are some netdev features that make little sense to toggle on and
>>> off in a stacked device setup on only one device in the stack. The prime
>>> example is a bonded connection, where it really doesn't make sense to
>>> disable LRO on the master, but not on any of the slaves, nor does it
>>> really make sense to be able to shut LRO off on a slave when its still
>>> enabled on the master.
>>>
>>> The strategy here is to add a section near the end of
>>> netdev_fix_features() that looks for upper and lower netdevs, then make
>>> sure certain feature flags match both up and down the stack. At present,
>>> only the LRO flag is included.
> ...
>>> +static void netdev_sync_lower_features(struct net_device *upper,
>>> + struct net_device *lower, netdev_features_t features)
>>> +{
>>> + netdev_features_t want = features & lower->hw_features;
>>> +
>>> + if (!(features & NETIF_F_LRO) && (lower->features & NETIF_F_LRO)) {
>>> + netdev_info(upper, "Disabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features &= ~NETIF_F_LRO;
>>> + lower->wanted_features &= ~NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(lower->features & NETIF_F_LRO))
>>> + netdev_WARN(upper, "failed to disable LRO on %s!\n",
>>> + lower->name);
>>> + } else if ((want & NETIF_F_LRO) && !(lower->features & NETIF_F_LRO)) {
>>> + netdev_info(upper, "Enabling LRO on lower dev %s.\n",
>>> + lower->name);
>>> + upper->wanted_features |= NETIF_F_LRO;
>>> + lower->wanted_features |= NETIF_F_LRO;
>>> + netdev_update_features(lower);
>>> + if (unlikely(!(lower->features & NETIF_F_LRO)))
>>> + netdev_WARN(upper, "failed to enable LRO on %s!\n",
>>> + lower->name);
>>> + }
>>> +}
>>> +
>>
>> Same thing here. If a lower dev has it disabled then leave it disabled.
>> I believe your goal is to make it so that dev_disable_lro() can shut
>> down LRO when it is making packets in the data-path unusable. There is
>> no need to make this an all or nothing scenario. We can let the stack
>> slam things down with dev_disable_lro() and then if a user so desires
>> they can come back through and enable LRO more selectively if they for
>> instance have an interface that can do a smarter job of putting together
>> frames that could be routed.
>>
>> You could probably look at doing something like this for RXCSUM as well.
>> The general idea is that if an upper device has it off then the value
>> has to be off. For example if RXCSUM is off in a upper device and LRO is
>> enabled on the lower device there is a good chance that the upper device
>> will report checksum errors since most LRO implementations don't
>> recalculate the checksum. If RXCSUM is forced down to the lower device
>> hopefully its fix_features will know this and disable LRO on that device
>> when the RXCSUM is disabled on it.
>
> Yeah, I was thinking there might be more flags to treat the same way,
> just wanted to hammer out the plausibility of doing it at all first. I
> can add RXCSUM to v2, or just wait until there's something that people
> might consider merge-worthy before worrying about additional flags. From
> what I've seen, most device's fix_features are reasonably intelligent
> about allowing/disallowing certain flag combos, so this does look pretty
> safe at a glance, and if a specific device tips over, it probably needs
> to be fixed in the device's driver anyway.

If nothing else you might start looking at working with a mask of bits 
that function like this.  You could probably start with GRO, LRO, and 
RXCSUM and work your way up from there.  If they aren't set on the upper 
devices you cannot enable them, and if they are cleared then they must 
be cleared on all lower devices.

- Alex

[PATCH net-next] net/core: generic support for disabling netdev features down stack

[Jarod Wilson]

There are some netdev features, which when disabled on an upper device,
such as a bonding master or a bridge, must be disabled and cannot be
re-enabled on underlying devices.

This is a rework of an earlier more heavy-handed appraoch, which simply
disables and prevents re-enabling of netdev features listed in a new
define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper
device that disables a flag in that feature mask, the disabling will
propagate down the stack, and any lower device that has any upper device
with one of those flags disabled should not be able to enable said flag.

Initially, only LRO is included for proof of concept, and because this
code effectively does the same thing as dev_disable_lro(), though it will
also activate from the ethtool path, which was one of the goals here.

[root@dell-per730-01 ~]# ethtool -k bond0 |grep large
large-receive-offload: on
[root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
large-receive-offload: on
[root@dell-per730-01 ~]# ethtool -K bond0 lro off
[root@dell-per730-01 ~]# ethtool -k bond0 |grep large
large-receive-offload: off
[root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
large-receive-offload: off

dmesg dump:

[ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
[ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
[ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

This has been successfully tested with bnx2x, qlcnic and netxen network
cards as slaves in a bond interface. Turning LRO on or off on the master
also turns it on or off on each of the slaves, new slaves are added with
LRO in the same state as the master, and LRO can't be toggled on the
slaves.

Also, this should largely remove the need for dev_disable_lro(), and most,
if not all, of its call sites can be replaced by simply making sure
NETIF_F_LRO isn't included in the relevant device's feature flags.

Note that this patch is driven by bug reports from users saying it was
confusing that bonds and slaves had different settings for the same
features, and while it won't be 100% in sync if a lower device doesn't
support a feature like LRO, I think this is a good step in the right
direction.

CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
Note: this replaces "[RFC PATCH net-next] net/core: initial support for
stacked dev feature toggles" for consideration.

 include/linux/netdev_features.h | 11 +++++++++
 net/core/dev.c                  | 52 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781..0f5837a 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,6 +125,11 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
+#define for_each_netdev_feature(mask_addr, feature)				\
+	int bit;								\
+	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)	\
+		feature = __NETIF_F_BIT(bit);
+
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
 #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
@@ -167,6 +172,12 @@ enum {
  */
 #define NETIF_F_ALL_FOR_ALL	(NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
 
+/*
+ * If upper/master device has these features disabled, they must be disabled
+ * on all lower/slave devices as well.
+ */
+#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
+
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 13f49f8..3a8dbbc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6288,9 +6288,51 @@ static void rollback_registered(struct net_device *dev)
 	list_del(&single);
 }
 
+static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
+	struct net_device *upper, netdev_features_t features)
+{
+	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
+	netdev_features_t feature;
+
+	for_each_netdev_feature(&upper_disables, feature) {
+		if (!(upper->wanted_features & feature)
+		    && (features & feature)) {
+			netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
+				   &feature, upper->name);
+			features &= ~feature;
+		}
+	}
+
+	return features;
+}
+
+static void netdev_sync_lower_features(struct net_device *upper,
+	struct net_device *lower, netdev_features_t features)
+{
+	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
+	netdev_features_t feature;
+
+	for_each_netdev_feature(&upper_disables, feature) {
+		if (!(features & feature) && (lower->features & feature)) {
+			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
+				   &feature, lower->name);
+			upper->wanted_features &= ~feature;
+			lower->wanted_features &= ~feature;
+			netdev_update_features(lower);
+
+			if (unlikely(lower->features & feature))
+				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
+					    &feature, lower->name);
+		}
+	}
+}
+
 static netdev_features_t netdev_fix_features(struct net_device *dev,
 	netdev_features_t features)
 {
+	struct net_device *upper, *lower;
+	struct list_head *iter;
+
 	/* Fix illegal checksum combinations */
 	if ((features & NETIF_F_HW_CSUM) &&
 	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
@@ -6345,6 +6387,16 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		}
 	}
 
+	/* some features can't be enabled if they're off an an upper device */
+	netdev_for_each_upper_dev_rcu(dev, upper, iter)
+		features = netdev_sync_upper_features(dev, upper, features);
+
+	/* some features must be disabled on lower devices when disabled
+	 * on an upper device (think: bonding master or bridge)
+	 */
+	netdev_for_each_lower_dev(dev, lower, iter)
+		netdev_sync_lower_features(dev, lower, features);
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	if (dev->netdev_ops->ndo_busy_poll)
 		features |= NETIF_F_BUSY_POLL;
-- 
1.8.3.1

Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack

[Alexander Duyck]

On 11/02/2015 09:53 AM, Jarod Wilson wrote:
> There are some netdev features, which when disabled on an upper device,
> such as a bonding master or a bridge, must be disabled and cannot be
> re-enabled on underlying devices.
>
> This is a rework of an earlier more heavy-handed appraoch, which simply
> disables and prevents re-enabling of netdev features listed in a new
> define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper
> device that disables a flag in that feature mask, the disabling will
> propagate down the stack, and any lower device that has any upper device
> with one of those flags disabled should not be able to enable said flag.
>
> Initially, only LRO is included for proof of concept, and because this
> code effectively does the same thing as dev_disable_lro(), though it will
> also activate from the ethtool path, which was one of the goals here.
>
> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large
> large-receive-offload: on
> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
> large-receive-offload: on
> [root@dell-per730-01 ~]# ethtool -K bond0 lro off
> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large
> large-receive-offload: off
> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
> large-receive-offload: off
>
> dmesg dump:
>
> [ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
> [ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
> [ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
> [ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71
>
> This has been successfully tested with bnx2x, qlcnic and netxen network
> cards as slaves in a bond interface. Turning LRO on or off on the master
> also turns it on or off on each of the slaves, new slaves are added with
> LRO in the same state as the master, and LRO can't be toggled on the
> slaves.
>
> Also, this should largely remove the need for dev_disable_lro(), and most,
> if not all, of its call sites can be replaced by simply making sure
> NETIF_F_LRO isn't included in the relevant device's feature flags.
>
> Note that this patch is driven by bug reports from users saying it was
> confusing that bonds and slaves had different settings for the same
> features, and while it won't be 100% in sync if a lower device doesn't
> support a feature like LRO, I think this is a good step in the right
> direction.
>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
> Note: this replaces "[RFC PATCH net-next] net/core: initial support for
> stacked dev feature toggles" for consideration.
>
>   include/linux/netdev_features.h | 11 +++++++++
>   net/core/dev.c                  | 52 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 63 insertions(+)
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 9672781..0f5837a 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -125,6 +125,11 @@ enum {
>   #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
>   #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
>
> +#define for_each_netdev_feature(mask_addr, feature)				\
> +	int bit;								\
> +	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)	\
> +		feature = __NETIF_F_BIT(bit);
> +
>   /* Features valid for ethtool to change */
>   /* = all defined minus driver/device-class-related */
>   #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
> @@ -167,6 +172,12 @@ enum {
>    */
>   #define NETIF_F_ALL_FOR_ALL	(NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
>
> +/*
> + * If upper/master device has these features disabled, they must be disabled
> + * on all lower/slave devices as well.
> + */
> +#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
> +
>   /* changeable features with no special hardware requirements */
>   #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 13f49f8..3a8dbbc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6288,9 +6288,51 @@ static void rollback_registered(struct net_device *dev)
>   	list_del(&single);
>   }
>
> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> +	struct net_device *upper, netdev_features_t features)
> +{
> +	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
> +	netdev_features_t feature;
> +
> +	for_each_netdev_feature(&upper_disables, feature) {
> +		if (!(upper->wanted_features & feature)
> +		    && (features & feature)) {
> +			netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
> +				   &feature, upper->name);
> +			features &= ~feature;
> +		}
> +	}
> +
> +	return features;
> +}
> +
> +static void netdev_sync_lower_features(struct net_device *upper,
> +	struct net_device *lower, netdev_features_t features)
> +{
> +	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
> +	netdev_features_t feature;
> +
> +	for_each_netdev_feature(&upper_disables, feature) {
> +		if (!(features & feature) && (lower->features & feature)) {
> +			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
> +				   &feature, lower->name);
> +			upper->wanted_features &= ~feature;

Isn't this line redundant? The upper device should have already cleared 
the bit from the wanted_features?  That is unless the ndo_fix_features 
call modified it in which case we shouldn't be modifying it ourselves.

> +			lower->wanted_features &= ~feature;
> +			netdev_update_features(lower);
> +
> +			if (unlikely(lower->features & feature))
> +				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
> +					    &feature, lower->name);
> +		}
> +	}
> +}
> +
>   static netdev_features_t netdev_fix_features(struct net_device *dev,
>   	netdev_features_t features)
>   {
> +	struct net_device *upper, *lower;
> +	struct list_head *iter;
> +
>   	/* Fix illegal checksum combinations */
>   	if ((features & NETIF_F_HW_CSUM) &&
>   	    (features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))) {
> @@ -6345,6 +6387,16 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>   		}
>   	}
>
> +	/* some features can't be enabled if they're off an an upper device */
> +	netdev_for_each_upper_dev_rcu(dev, upper, iter)
> +		features = netdev_sync_upper_features(dev, upper, features);
> +
> +	/* some features must be disabled on lower devices when disabled
> +	 * on an upper device (think: bonding master or bridge)
> +	 */
> +	netdev_for_each_lower_dev(dev, lower, iter)
> +		netdev_sync_lower_features(dev, lower, features);
> +

I don't know if this is the right spot for this.  You might want to look 
at placing this after the ndo_set_features call to handle things if 
there wasn't an error.  That way if a lower device for some reason has 
an issue with one of the other settings being changed you don't end up 
in a state where all the lower devices have the feature stripped while 
the upper device still reports it as being enabled.

>   #ifdef CONFIG_NET_RX_BUSY_POLL
>   	if (dev->netdev_ops->ndo_busy_poll)
>   		features |= NETIF_F_BUSY_POLL;
>

Re: [PATCH net-next] net/core: generic support for disabling netdev features down stack

[Jarod Wilson]

Alexander Duyck wrote:
> On 11/02/2015 09:53 AM, Jarod Wilson wrote:
>> There are some netdev features, which when disabled on an upper device,
>> such as a bonding master or a bridge, must be disabled and cannot be
>> re-enabled on underlying devices.
>>
>> This is a rework of an earlier more heavy-handed appraoch, which simply
>> disables and prevents re-enabling of netdev features listed in a new
>> define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any
>> upper
>> device that disables a flag in that feature mask, the disabling will
>> propagate down the stack, and any lower device that has any upper device
>> with one of those flags disabled should not be able to enable said flag.
>>
>> Initially, only LRO is included for proof of concept, and because this
>> code effectively does the same thing as dev_disable_lro(), though it will
>> also activate from the ethtool path, which was one of the goals here.
>>
>> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large
>> large-receive-offload: on
>> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
>> large-receive-offload: on
>> [root@dell-per730-01 ~]# ethtool -K bond0 lro off
>> [root@dell-per730-01 ~]# ethtool -k bond0 |grep large
>> large-receive-offload: off
>> [root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
>> large-receive-offload: off
>>
>> dmesg dump:
>>
>> [ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower
>> dev p5p2.
>> [ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X IRQs: sp 74 fp[0]
>> 76 ... fp[7] 83
>> [ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower
>> dev p5p1.
>> [ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X IRQs: sp 62 fp[0]
>> 64 ... fp[7] 71
>>
>> This has been successfully tested with bnx2x, qlcnic and netxen network
>> cards as slaves in a bond interface. Turning LRO on or off on the master
>> also turns it on or off on each of the slaves, new slaves are added with
>> LRO in the same state as the master, and LRO can't be toggled on the
>> slaves.
>>
>> Also, this should largely remove the need for dev_disable_lro(), and
>> most,
>> if not all, of its call sites can be replaced by simply making sure
>> NETIF_F_LRO isn't included in the relevant device's feature flags.
>>
>> Note that this patch is driven by bug reports from users saying it was
>> confusing that bonds and slaves had different settings for the same
>> features, and while it won't be 100% in sync if a lower device doesn't
>> support a feature like LRO, I think this is a good step in the right
>> direction.
>>
>> CC: "David S. Miller" <davem@davemloft.net>
>> CC: Eric Dumazet <edumazet@google.com>
>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>> CC: Veaceslav Falico <vfalico@gmail.com>
>> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Nikolay Aleksandrov <razor@blackwall.org>
>> CC: Michal Kubecek <mkubecek@suse.cz>
>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>> CC: netdev@vger.kernel.org
>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> ---
>> Note: this replaces "[RFC PATCH net-next] net/core: initial support for
>> stacked dev feature toggles" for consideration.
>>
>> include/linux/netdev_features.h | 11 +++++++++
>> net/core/dev.c | 52 +++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/include/linux/netdev_features.h
>> b/include/linux/netdev_features.h
>> index 9672781..0f5837a 100644
>> --- a/include/linux/netdev_features.h
>> +++ b/include/linux/netdev_features.h
>> @@ -125,6 +125,11 @@ enum {
>> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD)
>> #define NETIF_F_BUSY_POLL __NETIF_F(BUSY_POLL)
>>
>> +#define for_each_netdev_feature(mask_addr, feature) \
>> + int bit; \
>> + for_each_set_bit(bit, (unsigned long *)mask_addr,
>> NETDEV_FEATURE_COUNT) \
>> + feature = __NETIF_F_BIT(bit);
>> +
>> /* Features valid for ethtool to change */
>> /* = all defined minus driver/device-class-related */
>> #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \
>> @@ -167,6 +172,12 @@ enum {
>> */
>> #define NETIF_F_ALL_FOR_ALL (NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
>>
>> +/*
>> + * If upper/master device has these features disabled, they must be
>> disabled
>> + * on all lower/slave devices as well.
>> + */
>> +#define NETIF_F_UPPER_DISABLES NETIF_F_LRO
>> +
>> /* changeable features with no special hardware requirements */
>> #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 13f49f8..3a8dbbc 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -6288,9 +6288,51 @@ static void rollback_registered(struct
>> net_device *dev)
>> list_del(&single);
>> }
>>
>> +static netdev_features_t netdev_sync_upper_features(struct net_device
>> *lower,
>> + struct net_device *upper, netdev_features_t features)
>> +{
>> + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
>> + netdev_features_t feature;
>> +
>> + for_each_netdev_feature(&upper_disables, feature) {
>> + if (!(upper->wanted_features & feature)
>> + && (features & feature)) {
>> + netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
>> + &feature, upper->name);
>> + features &= ~feature;
>> + }
>> + }
>> +
>> + return features;
>> +}
>> +
>> +static void netdev_sync_lower_features(struct net_device *upper,
>> + struct net_device *lower, netdev_features_t features)
>> +{
>> + netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
>> + netdev_features_t feature;
>> +
>> + for_each_netdev_feature(&upper_disables, feature) {
>> + if (!(features & feature) && (lower->features & feature)) {
>> + netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
>> + &feature, lower->name);
>> + upper->wanted_features &= ~feature;
>
> Isn't this line redundant? The upper device should have already cleared
> the bit from the wanted_features? That is unless the ndo_fix_features
> call modified it in which case we shouldn't be modifying it ourselves.

With the upper/lower feature sync calls where they currently are, no, 
its actually necessary to prevent flip-flopping flag values.

...
>> @@ -6345,6 +6387,16 @@ static netdev_features_t
>> netdev_fix_features(struct net_device *dev,
>> }
>> }
>>
>> + /* some features can't be enabled if they're off an an upper device */
>> + netdev_for_each_upper_dev_rcu(dev, upper, iter)
>> + features = netdev_sync_upper_features(dev, upper, features);
>> +
>> + /* some features must be disabled on lower devices when disabled
>> + * on an upper device (think: bonding master or bridge)
>> + */
>> + netdev_for_each_lower_dev(dev, lower, iter)
>> + netdev_sync_lower_features(dev, lower, features);
>> +
>
> I don't know if this is the right spot for this. You might want to look
> at placing this after the ndo_set_features call to handle things if
> there wasn't an error. That way if a lower device for some reason has an
> issue with one of the other settings being changed you don't end up in a
> state where all the lower devices have the feature stripped while the
> upper device still reports it as being enabled.

I'll give it a go after the .ndo_set_features calls in 
__netdev_update_features(), see what shakes loose. Might even not need 
the extra upper->wanted_features twiddling with it here.


-- 
Jarod Wilson
jarod@redhat.com

[PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Jarod Wilson]

There are some netdev features, which when disabled on an upper device,
such as a bonding master or a bridge, must be disabled and cannot be
re-enabled on underlying devices.

This is a rework of an earlier more heavy-handed appraoch, which simply
disables and prevents re-enabling of netdev features listed in a new
define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper
device that disables a flag in that feature mask, the disabling will
propagate down the stack, and any lower device that has any upper device
with one of those flags disabled should not be able to enable said flag.

Initially, only LRO is included for proof of concept, and because this
code effectively does the same thing as dev_disable_lro(), though it will
also activate from the ethtool path, which was one of the goals here.

[root@dell-per730-01 ~]# ethtool -k bond0 |grep large
large-receive-offload: on
[root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
large-receive-offload: on
[root@dell-per730-01 ~]# ethtool -K bond0 lro off
[root@dell-per730-01 ~]# ethtool -k bond0 |grep large
large-receive-offload: off
[root@dell-per730-01 ~]# ethtool -k p5p1 |grep large
large-receive-offload: off

dmesg dump:

[ 1033.277986] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
[ 1034.067949] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[ 1034.753612] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
[ 1035.591019] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

This has been successfully tested with bnx2x, qlcnic and netxen network
cards as slaves in a bond interface. Turning LRO on or off on the master
also turns it on or off on each of the slaves, new slaves are added with
LRO in the same state as the master, and LRO can't be toggled on the
slaves.

Also, this should largely remove the need for dev_disable_lro(), and most,
if not all, of its call sites can be replaced by simply making sure
NETIF_F_LRO isn't included in the relevant device's feature flags.

Note that this patch is driven by bug reports from users saying it was
confusing that bonds and slaves had different settings for the same
features, and while it won't be 100% in sync if a lower device doesn't
support a feature like LRO, I think this is a good step in the right
direction.

CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
v2: Move manipulation of lower devices after ndo_set_features so the upper
device gets fully set up first and remove now redundant flag clearing, as
suggested by Alex Duyck. Filtering features on a current lower device
needs to be done prior to ndo_set_features, but both are now in
__netdev_update_features().

 include/linux/netdev_features.h | 11 +++++++++
 net/core/dev.c                  | 50 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 9672781..0f5837a 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,6 +125,11 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
+#define for_each_netdev_feature(mask_addr, feature)				\
+	int bit;								\
+	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)	\
+		feature = __NETIF_F_BIT(bit);
+
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
 #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
@@ -167,6 +172,12 @@ enum {
  */
 #define NETIF_F_ALL_FOR_ALL	(NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
 
+/*
+ * If upper/master device has these features disabled, they must be disabled
+ * on all lower/slave devices as well.
+ */
+#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
+
 /* changeable features with no special hardware requirements */
 #define NETIF_F_SOFT_FEATURES	(NETIF_F_GSO | NETIF_F_GRO)
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 13f49f8..c4d2b43 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6288,6 +6288,44 @@ static void rollback_registered(struct net_device *dev)
 	list_del(&single);
 }
 
+static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
+	struct net_device *upper, netdev_features_t features)
+{
+	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
+	netdev_features_t feature;
+
+	for_each_netdev_feature(&upper_disables, feature) {
+		if (!(upper->wanted_features & feature)
+		    && (features & feature)) {
+			netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
+				   &feature, upper->name);
+			features &= ~feature;
+		}
+	}
+
+	return features;
+}
+
+static void netdev_sync_lower_features(struct net_device *upper,
+	struct net_device *lower, netdev_features_t features)
+{
+	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
+	netdev_features_t feature;
+
+	for_each_netdev_feature(&upper_disables, feature) {
+		if (!(features & feature) && (lower->features & feature)) {
+			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
+				   &feature, lower->name);
+			lower->wanted_features &= ~feature;
+			netdev_update_features(lower);
+
+			if (unlikely(lower->features & feature))
+				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
+					    &feature, lower->name);
+		}
+	}
+}
+
 static netdev_features_t netdev_fix_features(struct net_device *dev,
 	netdev_features_t features)
 {
@@ -6357,7 +6395,9 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 
 int __netdev_update_features(struct net_device *dev)
 {
+	struct net_device *upper, *lower;
 	netdev_features_t features;
+	struct list_head *iter;
 	int err = 0;
 
 	ASSERT_RTNL();
@@ -6370,6 +6410,10 @@ int __netdev_update_features(struct net_device *dev)
 	/* driver might be less strict about feature dependencies */
 	features = netdev_fix_features(dev, features);
 
+	/* some features can't be enabled if they're off an an upper device */
+	netdev_for_each_upper_dev_rcu(dev, upper, iter)
+		features = netdev_sync_upper_features(dev, upper, features);
+
 	if (dev->features == features)
 		return 0;
 
@@ -6386,6 +6430,12 @@ int __netdev_update_features(struct net_device *dev)
 		return -1;
 	}
 
+	/* some features must be disabled on lower devices when disabled
+	 * on an upper device (think: bonding master or bridge)
+	 */
+	netdev_for_each_lower_dev(dev, lower, iter)
+		netdev_sync_lower_features(dev, lower, features);
+
 	if (!err)
 		dev->features = features;
 
-- 
1.8.3.1

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Mon,  2 Nov 2015 21:55:59 -0500

> There are some netdev features, which when disabled on an upper device,
> such as a bonding master or a bridge, must be disabled and cannot be
> re-enabled on underlying devices.
> 
> This is a rework of an earlier more heavy-handed appraoch, which simply
> disables and prevents re-enabling of netdev features listed in a new
> define in include/net/netdev_features.h, NETIF_F_UPPER_DISABLES. Any upper
> device that disables a flag in that feature mask, the disabling will
> propagate down the stack, and any lower device that has any upper device
> with one of those flags disabled should not be able to enable said flag.
> 
> Initially, only LRO is included for proof of concept, and because this
> code effectively does the same thing as dev_disable_lro(), though it will
> also activate from the ethtool path, which was one of the goals here.
 ...
> This has been successfully tested with bnx2x, qlcnic and netxen network
> cards as slaves in a bond interface. Turning LRO on or off on the master
> also turns it on or off on each of the slaves, new slaves are added with
> LRO in the same state as the master, and LRO can't be toggled on the
> slaves.
> 
> Also, this should largely remove the need for dev_disable_lro(), and most,
> if not all, of its call sites can be replaced by simply making sure
> NETIF_F_LRO isn't included in the relevant device's feature flags.
> 
> Note that this patch is driven by bug reports from users saying it was
> confusing that bonds and slaves had different settings for the same
> features, and while it won't be 100% in sync if a lower device doesn't
> support a feature like LRO, I think this is a good step in the right
> direction.
 ...
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Looks good to me, applied, thanks Jarod.

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Nikolay Aleksandrov]

On 11/03/2015 03:55 AM, Jarod Wilson wrote:
[snip]
>  
> +#define for_each_netdev_feature(mask_addr, feature)				\
> +	int bit;								\
> +	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)	\
> +		feature = __NETIF_F_BIT(bit);
> +
^
This is broken, it will not work for more than a single feature.

>  /* Features valid for ethtool to change */
>  /* = all defined minus driver/device-class-related */
>  #define NETIF_F_NEVER_CHANGE	(NETIF_F_VLAN_CHALLENGED | \
> @@ -167,6 +172,12 @@ enum {
>   */
>  #define NETIF_F_ALL_FOR_ALL	(NETIF_F_NOCACHE_COPY | NETIF_F_FSO)
>  
> +/*
> + * If upper/master device has these features disabled, they must be disabled
> + * on all lower/slave devices as well.
> + */
> +#define NETIF_F_UPPER_DISABLES	NETIF_F_LRO
> +
[snip]

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Geert Uytterhoeven]

On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
> [snip]
>>
>> +#define for_each_netdev_feature(mask_addr, feature)                          \
>> +     int bit;                                                                \
>> +     for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \
>> +             feature = __NETIF_F_BIT(bit);
>> +
> ^
> This is broken, it will not work for more than a single feature.

Indeed it is.

This is used as:

        for_each_netdev_feature(&upper_disables, feature) {
        ...
        }

which expands to:

        int bit;
        for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
                feature = __NETIF_F_BIT(bit);
        {
                ...
        }

Note the assignment to "feature" happens outside the {}-delimited block.
And the block is always executed once.

Interestingly, arm-unknown-linux-gnueabi-gcc 4.9.0 warns about this in
some of my configs, but not in all of them:

net/core/dev.c: In function '__netdev_update_features':
net/core/dev.c:6302:16: warning: 'feature' may be used uninitialized
in this function [-Wmaybe-uninitialized]
    features &= ~feature;
                ^
net/core/dev.c:6295:20: note: 'feature' was declared here
  netdev_features_t feature;
                    ^
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Jarod Wilson]

Geert Uytterhoeven wrote:
> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com>  wrote:
>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>> [snip]
>>> +#define for_each_netdev_feature(mask_addr, feature)                          \
>>> +     int bit;                                                                \
>>> +     for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \
>>> +             feature = __NETIF_F_BIT(bit);
>>> +
>> ^
>> This is broken, it will not work for more than a single feature.
>
> Indeed it is.
>
> This is used as:
>
>          for_each_netdev_feature(&upper_disables, feature) {
>          ...
>          }
>
> which expands to:
>
>          int bit;
>          for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>                  feature = __NETIF_F_BIT(bit);
>          {
>                  ...
>          }
>
> Note the assignment to "feature" happens outside the {}-delimited block.
> And the block is always executed once.

Bah, crap, I was still staring at the code not seeing it, thank you for 
the detailed cluebat. I'll fix that up right now.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Nikolay Aleksandrov]

On 11/03/2015 02:57 PM, Jarod Wilson wrote:
> Geert Uytterhoeven wrote:
>> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
>> <nikolay@cumulusnetworks.com>  wrote:
>>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>>> [snip]
>>>> +#define for_each_netdev_feature(mask_addr, feature)                          \
>>>> +     int bit;                                                                \
>>>> +     for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \
>>>> +             feature = __NETIF_F_BIT(bit);
>>>> +
>>> ^
>>> This is broken, it will not work for more than a single feature.
>>
>> Indeed it is.
>>
>> This is used as:
>>
>>          for_each_netdev_feature(&upper_disables, feature) {
>>          ...
>>          }
>>
>> which expands to:
>>
>>          int bit;
>>          for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>>                  feature = __NETIF_F_BIT(bit);
>>          {
>>                  ...
>>          }
>>
>> Note the assignment to "feature" happens outside the {}-delimited block.
>> And the block is always executed once.
> 
> Bah, crap, I was still staring at the code not seeing it, thank you for the detailed cluebat. I'll fix that up right now.
> 

Yeah, sorry for not elaborating, I wrote it in a hurry. :-)
Thanks Geert!

By the way since you'll be changing this code, I don't know if it's okay to
declare caller-visible hidden local variables in a macro like this, at the very
least please consider renaming it to something that's much less common, I can see
"bit" being used here and there. IMO either try to find a way to avoid it
altogether or add another argument to the macro so it's explicitly passed.

Cheers,
 Nik

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Jarod Wilson]

Nikolay Aleksandrov wrote:
> On 11/03/2015 02:57 PM, Jarod Wilson wrote:
>> Geert Uytterhoeven wrote:
>>> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
>>> <nikolay@cumulusnetworks.com>   wrote:
>>>> On 11/03/2015 03:55 AM, Jarod Wilson wrote:
>>>> [snip]
>>>>> +#define for_each_netdev_feature(mask_addr, feature)                          \
>>>>> +     int bit;                                                                \
>>>>> +     for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT) \
>>>>> +             feature = __NETIF_F_BIT(bit);
>>>>> +
>>>> ^
>>>> This is broken, it will not work for more than a single feature.
>>> Indeed it is.
>>>
>>> This is used as:
>>>
>>>           for_each_netdev_feature(&upper_disables, feature) {
>>>           ...
>>>           }
>>>
>>> which expands to:
>>>
>>>           int bit;
>>>           for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
>>>                   feature = __NETIF_F_BIT(bit);
>>>           {
>>>                   ...
>>>           }
>>>
>>> Note the assignment to "feature" happens outside the {}-delimited block.
>>> And the block is always executed once.
>> Bah, crap, I was still staring at the code not seeing it, thank you for the detailed cluebat. I'll fix that up right now.
>>
>
> Yeah, sorry for not elaborating, I wrote it in a hurry. :-)
> Thanks Geert!
>
> By the way since you'll be changing this code, I don't know if it's okay to
> declare caller-visible hidden local variables in a macro like this, at the very
> least please consider renaming it to something that's much less common, I can see
> "bit" being used here and there. IMO either try to find a way to avoid it
> altogether or add another argument to the macro so it's explicitly passed.

Just posted a follow-up that removes the macro-internal use of bit and 
doesn't botch up assigning feature. It's not as pretty, but it works 
correctly with multiple feature bits.

-- 
Jarod Wilson
jarod@redhat.com

[PATCH net-next] net/core: fix for_each_netdev_feature

[Jarod Wilson]

As pointed out by Nikolay and further explained by Geert, the initial
for_each_netdev_feature macro was broken, as feature would get set outside
of the block of code it was intended to run in, thus only ever working for
the first feature bit in the mask. While less pretty this way, this is
tested and confirmed functional with multiple feature bits set in
NETIF_F_UPPER_DISABLES.

[root@dell-per730-01 ~]# ethtool -K bond0 lro off
...
[  242.761394] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
[  243.552178] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[  244.353978] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
[  245.147420] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

[root@dell-per730-01 ~]# ethtool -K bond0 gro off
...
[  251.925645] bond0: Disabling feature 0x0000000000004000 on lower dev p5p2.
[  252.713693] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
[  253.499085] bond0: Disabling feature 0x0000000000004000 on lower dev p5p1.
[  254.290922] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71

Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 include/linux/netdev_features.h | 6 ++----
 net/core/dev.c                  | 8 ++++++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 0f5837a..f0d8734 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -125,10 +125,8 @@ enum {
 #define NETIF_F_HW_L2FW_DOFFLOAD	__NETIF_F(HW_L2FW_DOFFLOAD)
 #define NETIF_F_BUSY_POLL	__NETIF_F(BUSY_POLL)
 
-#define for_each_netdev_feature(mask_addr, feature)				\
-	int bit;								\
-	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)	\
-		feature = __NETIF_F_BIT(bit);
+#define for_each_netdev_feature(mask_addr, bit)	\
+	for_each_set_bit(bit, (unsigned long *)mask_addr, NETDEV_FEATURE_COUNT)
 
 /* Features valid for ethtool to change */
 /* = all defined minus driver/device-class-related */
diff --git a/net/core/dev.c b/net/core/dev.c
index c4d2b43..8ce3f74 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6293,8 +6293,10 @@ static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 {
 	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
 	netdev_features_t feature;
+	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature) {
+	for_each_netdev_feature(&upper_disables, feature_bit) {
+		feature = __NETIF_F_BIT(feature_bit);
 		if (!(upper->wanted_features & feature)
 		    && (features & feature)) {
 			netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
@@ -6311,8 +6313,10 @@ static void netdev_sync_lower_features(struct net_device *upper,
 {
 	netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
 	netdev_features_t feature;
+	int feature_bit;
 
-	for_each_netdev_feature(&upper_disables, feature) {
+	for_each_netdev_feature(&upper_disables, feature_bit) {
+		feature = __NETIF_F_BIT(feature_bit);
 		if (!(features & feature) && (lower->features & feature)) {
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
 				   &feature, lower->name);
-- 
1.8.3.1

Re: [PATCH net-next] net/core: fix for_each_netdev_feature

[Nikolay Aleksandrov]

On 11/03/2015 04:15 PM, Jarod Wilson wrote:
> As pointed out by Nikolay and further explained by Geert, the initial
> for_each_netdev_feature macro was broken, as feature would get set outside
> of the block of code it was intended to run in, thus only ever working for
> the first feature bit in the mask. While less pretty this way, this is
> tested and confirmed functional with multiple feature bits set in
> NETIF_F_UPPER_DISABLES.
> 
> [root@dell-per730-01 ~]# ethtool -K bond0 lro off
> ...
> [  242.761394] bond0: Disabling feature 0x0000000000008000 on lower dev p5p2.
> [  243.552178] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
> [  244.353978] bond0: Disabling feature 0x0000000000008000 on lower dev p5p1.
> [  245.147420] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71
> 
> [root@dell-per730-01 ~]# ethtool -K bond0 gro off
> ...
> [  251.925645] bond0: Disabling feature 0x0000000000004000 on lower dev p5p2.
> [  252.713693] bnx2x 0000:06:00.1 p5p2: using MSI-X  IRQs: sp 74  fp[0] 76 ... fp[7] 83
> [  253.499085] bond0: Disabling feature 0x0000000000004000 on lower dev p5p1.
> [  254.290922] bnx2x 0000:06:00.0 p5p1: using MSI-X  IRQs: sp 62  fp[0] 64 ... fp[7] 71
> 
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  include/linux/netdev_features.h | 6 ++----
>  net/core/dev.c                  | 8 ++++++--
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 

Looks good to me especially without the hidden side-effects,

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Thanks,
 Nik

Re: [PATCH net-next] net/core: fix for_each_netdev_feature

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Tue,  3 Nov 2015 10:15:59 -0500

> As pointed out by Nikolay and further explained by Geert, the initial
> for_each_netdev_feature macro was broken, as feature would get set outside
> of the block of code it was intended to run in, thus only ever working for
> the first feature bit in the mask. While less pretty this way, this is
> tested and confirmed functional with multiple feature bits set in
> NETIF_F_UPPER_DISABLES.
 ...
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
 ...
> Signed-off-by: Jarod Wilson <jarod@redhat.com>

Applied.

[PATCH net-next] net/core: ensure features get disabled on new lower devs

[Jarod Wilson]

With moving netdev_sync_lower_features() after the .ndo_set_features
calls, I neglected to verify that devices added *after* a flag had been
disabled on an upper device were properly added with that flag disabled as
well. This currently happens, because we exit __netdev_update_features()
when we see dev->features == features for the upper dev. We can retain the
optimization of leaving without calling .ndo_set_features with a bit of
tweaking and a goto here.

Changing err to ret was somewhat arbitrary and makes the patch look more
involved, but seems to better fit the altered use.

Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 net/core/dev.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8ce3f74..90e0a62 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6402,7 +6402,7 @@ int __netdev_update_features(struct net_device *dev)
 	struct net_device *upper, *lower;
 	netdev_features_t features;
 	struct list_head *iter;
-	int err = 0;
+	int ret = 0;
 
 	ASSERT_RTNL();
 
@@ -6419,31 +6419,34 @@ int __netdev_update_features(struct net_device *dev)
 		features = netdev_sync_upper_features(dev, upper, features);
 
 	if (dev->features == features)
-		return 0;
+		goto sync_lower;
 
 	netdev_dbg(dev, "Features changed: %pNF -> %pNF\n",
 		&dev->features, &features);
 
 	if (dev->netdev_ops->ndo_set_features)
-		err = dev->netdev_ops->ndo_set_features(dev, features);
+		ret = dev->netdev_ops->ndo_set_features(dev, features);
 
-	if (unlikely(err < 0)) {
+	if (unlikely(ret < 0)) {
 		netdev_err(dev,
 			"set_features() failed (%d); wanted %pNF, left %pNF\n",
-			err, &features, &dev->features);
+			ret, &features, &dev->features);
 		return -1;
 	}
 
+	if (!ret) {
+		dev->features = features;
+		ret = 1;
+	}
+
+sync_lower:
 	/* some features must be disabled on lower devices when disabled
 	 * on an upper device (think: bonding master or bridge)
 	 */
 	netdev_for_each_lower_dev(dev, lower, iter)
 		netdev_sync_lower_features(dev, lower, features);
 
-	if (!err)
-		dev->features = features;
-
-	return 1;
+	return ret;
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Alexander Duyck]

On 11/03/2015 12:36 PM, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.
>
> Changing err to ret was somewhat arbitrary and makes the patch look more
> involved, but seems to better fit the altered use.
>
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>   net/core/dev.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 8ce3f74..90e0a62 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6402,7 +6402,7 @@ int __netdev_update_features(struct net_device *dev)
>   	struct net_device *upper, *lower;
>   	netdev_features_t features;
>   	struct list_head *iter;
> -	int err = 0;
> +	int ret = 0;
>
>   	ASSERT_RTNL();
>
> @@ -6419,31 +6419,34 @@ int __netdev_update_features(struct net_device *dev)
>   		features = netdev_sync_upper_features(dev, upper, features);
>
>   	if (dev->features == features)
> -		return 0;
> +		goto sync_lower;
>
>   	netdev_dbg(dev, "Features changed: %pNF -> %pNF\n",
>   		&dev->features, &features);
>
>   	if (dev->netdev_ops->ndo_set_features)
> -		err = dev->netdev_ops->ndo_set_features(dev, features);
> +		ret = dev->netdev_ops->ndo_set_features(dev, features);
>
> -	if (unlikely(err < 0)) {
> +	if (unlikely(ret < 0)) {
>   		netdev_err(dev,
>   			"set_features() failed (%d); wanted %pNF, left %pNF\n",
> -			err, &features, &dev->features);
> +			ret, &features, &dev->features);
>   		return -1;
>   	}
>
> +	if (!ret) {
> +		dev->features = features;
> +		ret = 1;
> +	}
> +

I would take the "ret = 1;" out of the if statement and let it stay here 
by itself.  Technically anything that traversed this path was returning 
1 previously so we probably want to retain that behavior.

> +sync_lower:
>   	/* some features must be disabled on lower devices when disabled
>   	 * on an upper device (think: bonding master or bridge)
>   	 */
>   	netdev_for_each_lower_dev(dev, lower, iter)
>   		netdev_sync_lower_features(dev, lower, features);
>
> -	if (!err)
> -		dev->features = features;

You could just alter the if statement here to check for a non-zero ret 
value since you should have it as either 0 or 1.  It shouldn't have any 
other values.

That way you will have disabled the feature on the lower devices before 
advertising that it has been disabled on the upper device.

> -	return 1;
> +	return ret;
>   }
>
>   /**
>

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Jarod Wilson]

Alexander Duyck wrote:
> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>> With moving netdev_sync_lower_features() after the .ndo_set_features
>> calls, I neglected to verify that devices added *after* a flag had been
>> disabled on an upper device were properly added with that flag
>> disabled as
>> well. This currently happens, because we exit __netdev_update_features()
>> when we see dev->features == features for the upper dev. We can retain
>> the
>> optimization of leaving without calling .ndo_set_features with a bit of
>> tweaking and a goto here.
>>
>> Changing err to ret was somewhat arbitrary and makes the patch look more
>> involved, but seems to better fit the altered use.
...
>> + 	if (!ret) {
>> + 		dev->features = features;
>> + 		ret = 1;
>> + 	}
>> +
>
> I would take the "ret = 1;" out of the if statement and let it stay here
> by itself. Technically anything that traversed this path was returning 1
> previously so we probably want to retain that behavior.

Ah, that. I took a look at all the callers of __netdev_update_features, 
and most don't even check return value, the one that does 
(netdev_update_features) only cares if its zero or not zero, so I 
figured it didn't really matter here, but it would indeed return 2 now 
instead of 1, if it got that from ndo_set_features. For consistency's 
sake, I can respin and just always set ret = 1 though.

>> +sync_lower:
>> 	/* some features must be disabled on lower devices when disabled
>> 	 * on an upper device (think: bonding master or bridge)
>> 	 */
>> 	netdev_for_each_lower_dev(dev, lower, iter)
>> 		netdev_sync_lower_features(dev, lower, features);
>>
>> - 	if (!err)
>> - 		dev->features = features;
>
> You could just alter the if statement here to check for a non-zero ret
> value since you should have it as either 0 or 1. It shouldn't have any
> other values.
>
> That way you will have disabled the feature on the lower devices before
> advertising that it has been disabled on the upper device.

If this check is down here, the goto will trigger, setting dev->features 
= features, but then, we got there because dev->features == features 
already, so meh. But it would also NOT trigger in the case of 
ndo_set_features returning 0 anymore, because we set ret = 1. Or am I 
missing something or misunderstanding what you're suggesting here?

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Alexander Duyck]

On 11/03/2015 02:11 PM, Jarod Wilson wrote:
> Alexander Duyck wrote:
>> On 11/03/2015 12:36 PM, Jarod Wilson wrote:
>>> With moving netdev_sync_lower_features() after the .ndo_set_features
>>> calls, I neglected to verify that devices added *after* a flag had been
>>> disabled on an upper device were properly added with that flag
>>> disabled as
>>> well. This currently happens, because we exit 
>>> __netdev_update_features()
>>> when we see dev->features == features for the upper dev. We can retain
>>> the
>>> optimization of leaving without calling .ndo_set_features with a bit of
>>> tweaking and a goto here.
>>>
>>> Changing err to ret was somewhat arbitrary and makes the patch look 
>>> more
>>> involved, but seems to better fit the altered use.
> ...
>>> +     if (!ret) {
>>> +         dev->features = features;
>>> +         ret = 1;
>>> +     }
>>> +
>>
>> I would take the "ret = 1;" out of the if statement and let it stay here
>> by itself. Technically anything that traversed this path was returning 1
>> previously so we probably want to retain that behavior.
>
> Ah, that. I took a look at all the callers of 
> __netdev_update_features, and most don't even check return value, the 
> one that does (netdev_update_features) only cares if its zero or not 
> zero, so I figured it didn't really matter here, but it would indeed 
> return 2 now instead of 1, if it got that from ndo_set_features. For 
> consistency's sake, I can respin and just always set ret = 1 though.

One thought I just had would be to make it so that we assign -1 to ret 
and then jump inside the earlier features==features check rather than 
altering the ret value here.  Then we could just use a ternary value at 
the end and just do "return ret < 0 ? 0 : 1;".  That would take care of 
the return values and the features flag you called out below.

>>> +sync_lower:
>>>     /* some features must be disabled on lower devices when disabled
>>>      * on an upper device (think: bonding master or bridge)
>>>      */
>>>     netdev_for_each_lower_dev(dev, lower, iter)
>>>         netdev_sync_lower_features(dev, lower, features);
>>>
>>> -     if (!err)
>>> -         dev->features = features;
>>
>> You could just alter the if statement here to check for a non-zero ret
>> value since you should have it as either 0 or 1. It shouldn't have any
>> other values.
>>
>> That way you will have disabled the feature on the lower devices before
>> advertising that it has been disabled on the upper device.
>
> If this check is down here, the goto will trigger, setting 
> dev->features = features, but then, we got there because dev->features 
> == features already, so meh. But it would also NOT trigger in the case 
> of ndo_set_features returning 0 anymore, because we set ret = 1. Or am 
> I missing something or misunderstanding what you're suggesting here?
>

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Nikolay Aleksandrov]

On 11/03/2015 09:36 PM, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.
> 
> Changing err to ret was somewhat arbitrary and makes the patch look more
> involved, but seems to better fit the altered use.
> 
> Fixes: fd867d51f ("net/core: generic support for disabling netdev features down stack")
> CC: "David S. Miller" <davem@davemloft.net>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> CC: Andy Gospodarek <gospo@cumulusnetworks.com>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Nikolay Aleksandrov <razor@blackwall.org>
> CC: Michal Kubecek <mkubecek@suse.cz>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  net/core/dev.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 

Thanks for the quick response,

Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Tested-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Michal Kubecek]

On Tue, Nov 03, 2015 at 03:36:57PM -0500, Jarod Wilson wrote:
> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.

I haven't reviewed the patch yet (I'm going to take a look with fresher
mind in the morning) but if you are going to handle this in a generic
way, you may want to remove the LRO specific hacks added to
bond_enslave() and team_port_add() by commit fbe168ba91f7 ("net: generic
dev_disable_lro() stacked device handling").

                                                          Michal Kubecek

Re: [PATCH net-next] net/core: ensure features get disabled on new lower devs

[Jarod Wilson]

Michal Kubecek wrote:
> On Tue, Nov 03, 2015 at 03:36:57PM -0500, Jarod Wilson wrote:
>> With moving netdev_sync_lower_features() after the .ndo_set_features
>> calls, I neglected to verify that devices added *after* a flag had been
>> disabled on an upper device were properly added with that flag disabled as
>> well. This currently happens, because we exit __netdev_update_features()
>> when we see dev->features == features for the upper dev. We can retain the
>> optimization of leaving without calling .ndo_set_features with a bit of
>> tweaking and a goto here.
>
> I haven't reviewed the patch yet (I'm going to take a look with fresher
> mind in the morning) but if you are going to handle this in a generic
> way, you may want to remove the LRO specific hacks added to
> bond_enslave() and team_port_add() by commit fbe168ba91f7 ("net: generic
> dev_disable_lro() stacked device handling").

Yeah, that's on tap for a follow-up series. I wanted to get all this in 
first and soaked before pulling those bits out. My current 
haven't-yet-fully-investigated thinking is that dev_disable_lro() can go 
away entirely, not just the hacks added to bond and team.

-- 
Jarod Wilson
jarod@redhat.com

[PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Jarod Wilson]

With moving netdev_sync_lower_features() after the .ndo_set_features
calls, I neglected to verify that devices added *after* a flag had been
disabled on an upper device were properly added with that flag disabled as
well. This currently happens, because we exit __netdev_update_features()
when we see dev->features == features for the upper dev. We can retain the
optimization of leaving without calling .ndo_set_features with a bit of
tweaking and a goto here.

Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <gospo@cumulusnetworks.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Nikolay Aleksandrov <razor@blackwall.org>
CC: Michal Kubecek <mkubecek@suse.cz>
CC: Alexander Duyck <alexander.duyck@gmail.com>
CC: netdev@vger.kernel.org
Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
v2: Based on suggestions from Alex, and with not changing err to ret, this
patch actually becomes quite minimal and doesn't ugly up the code much.

 net/core/dev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8ce3f74..ab9b8d0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6402,7 +6402,7 @@ int __netdev_update_features(struct net_device *dev)
 	struct net_device *upper, *lower;
 	netdev_features_t features;
 	struct list_head *iter;
-	int err = 0;
+	int err = -1;
 
 	ASSERT_RTNL();
 
@@ -6419,7 +6419,7 @@ int __netdev_update_features(struct net_device *dev)
 		features = netdev_sync_upper_features(dev, upper, features);
 
 	if (dev->features == features)
-		return 0;
+		goto sync_lower;
 
 	netdev_dbg(dev, "Features changed: %pNF -> %pNF\n",
 		&dev->features, &features);
@@ -6434,6 +6434,7 @@ int __netdev_update_features(struct net_device *dev)
 		return -1;
 	}
 
+sync_lower:
 	/* some features must be disabled on lower devices when disabled
 	 * on an upper device (think: bonding master or bridge)
 	 */
@@ -6443,7 +6444,7 @@ int __netdev_update_features(struct net_device *dev)
 	if (!err)
 		dev->features = features;
 
-	return 1;
+	return err < 0 ? 0 : 1;
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[David Miller]

From: Jarod Wilson <jarod@redhat.com>
Date: Tue,  3 Nov 2015 23:09:32 -0500

> With moving netdev_sync_lower_features() after the .ndo_set_features
> calls, I neglected to verify that devices added *after* a flag had been
> disabled on an upper device were properly added with that flag disabled as
> well. This currently happens, because we exit __netdev_update_features()
> when we see dev->features == features for the upper dev. We can retain the
> optimization of leaving without calling .ndo_set_features with a bit of
> tweaking and a goto here.
> 
> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
 ...
> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
> v2: Based on suggestions from Alex, and with not changing err to ret, this
> patch actually becomes quite minimal and doesn't ugly up the code much.

Applied, thanks.

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Florian Fainelli]

On 04/11/15 18:56, David Miller wrote:
>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>  ...
>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>> ---
>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>> patch actually becomes quite minimal and doesn't ugly up the code much.
> 
> Applied, thanks.

This causes some warnings to be displayed for DSA stacked devices:

[    1.272297] brcm-sf2 f0b00000.ethernet_switch: Starfighter 2 top:
4.00, core: 2.00 base: 0xf0c80000, IRQs: 68, 69
[    1.283181] libphy: dsa slave smi: probed
[    1.344088] f0b403c0.mdio:05: Broadcom BCM7445 PHY revision: 0xd0,
patch: 3
[    1.658917] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
attached PHY at address 5 [Broadcom BCM7445]
[    1.669414] brcm-sf2 f0b00000.ethernet_switch gphy: set_features()
failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
[    1.734202] brcm-sf2 f0b00000.ethernet_switch rgmii_1
(uninitialized): attached PHY at address 0 [Generic PHY]
[    1.744486] brcm-sf2 f0b00000.ethernet_switch rgmii_1: set_features()
failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
[    1.809091] brcm-sf2 f0b00000.ethernet_switch rgmii_2
(uninitialized): attached PHY at address 1 [Generic PHY]
[    1.819364] brcm-sf2 f0b00000.ethernet_switch rgmii_2: set_features()
failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
[    1.884090] brcm-sf2 f0b00000.ethernet_switch moca (uninitialized):
attached PHY at address 2 [Generic PHY]
[    1.894109] brcm-sf2 f0b00000.ethernet_switch moca: set_features()
failed (-1); wanted 0x0000000000004020, left 0x0000000000004820

DSA slave network devices are not associated with their master network
device using the typical lower/upper netdev helpers.

I do not have a good fix to come up with yet, but if you see something
obvious with net/dsa/slave.c, feel free to send patches for testing, I
can boot net-next on this platform.
-- 
Florian

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Jiri Pirko]

Fri, Nov 13, 2015 at 01:26:18AM CET, f.fainelli@gmail.com wrote:
>On 04/11/15 18:56, David Miller wrote:
>>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>>  ...
>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>> ---
>>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>>> patch actually becomes quite minimal and doesn't ugly up the code much.
>> 
>> Applied, thanks.
>
>This causes some warnings to be displayed for DSA stacked devices:
>
>[    1.272297] brcm-sf2 f0b00000.ethernet_switch: Starfighter 2 top:
>4.00, core: 2.00 base: 0xf0c80000, IRQs: 68, 69
>[    1.283181] libphy: dsa slave smi: probed
>[    1.344088] f0b403c0.mdio:05: Broadcom BCM7445 PHY revision: 0xd0,
>patch: 3
>[    1.658917] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
>attached PHY at address 5 [Broadcom BCM7445]
>[    1.669414] brcm-sf2 f0b00000.ethernet_switch gphy: set_features()
>failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>[    1.734202] brcm-sf2 f0b00000.ethernet_switch rgmii_1
>(uninitialized): attached PHY at address 0 [Generic PHY]
>[    1.744486] brcm-sf2 f0b00000.ethernet_switch rgmii_1: set_features()
>failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>[    1.809091] brcm-sf2 f0b00000.ethernet_switch rgmii_2
>(uninitialized): attached PHY at address 1 [Generic PHY]
>[    1.819364] brcm-sf2 f0b00000.ethernet_switch rgmii_2: set_features()
>failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>[    1.884090] brcm-sf2 f0b00000.ethernet_switch moca (uninitialized):
>attached PHY at address 2 [Generic PHY]
>[    1.894109] brcm-sf2 f0b00000.ethernet_switch moca: set_features()
>failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>
>DSA slave network devices are not associated with their master network
>device using the typical lower/upper netdev helpers.
>
>I do not have a good fix to come up with yet, but if you see something
>obvious with net/dsa/slave.c, feel free to send patches for testing, I
>can boot net-next on this platform.

I'm having similar issues with bridge, with linus's git now:

<dmesg>
...
[   14.354362] br0: set_features() failed (-1); wanted 0x000000801fd978a9, left 0x000000801fff78e9
[   14.430480] br0: set_features() failed (-1); wanted 0x000000801fd978a9, left 0x000000801fff78e9
[   14.430550] IPv6: ADDRCONF(NETDEV_UP): br0: link is not ready
[   17.938637] tg3 0000:01:00.0 eno1: Link is up at 1000 Mbps, full duplex
[   17.938647] tg3 0000:01:00.0 eno1: Flow control is off for TX and off for RX
[   17.938651] tg3 0000:01:00.0 eno1: EEE is disabled
[   17.938669] IPv6: ADDRCONF(NETDEV_CHANGE): eno1: link becomes ready
[   17.938753] br0: port 1(eno1) entered forwarding state
[   17.938762] br0: port 1(eno1) entered forwarding state
[   17.938834] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
[   29.763514] FS-Cache: Loaded
[   29.917680] FS-Cache: Netfs 'nfs' registered for caching
[   29.936739] Key type dns_resolver registered
[   30.637482] NFS: Registering the id_resolver key type
[   30.637502] Key type id_resolver registered
[   30.637504] Key type id_legacy registered
[   31.286444] ip6_tables: (C) 2000-2006 Netfilter Core Team
[   31.403005] Ebtables v2.0 registered
[   31.630354] tun: Universal TUN/TAP device driver, 1.6
[   31.630358] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
[   31.630824] virbr0-nic: set_features() failed (-1); wanted 0x00000080000048c1, left 0x00000080001b48c9
[   31.677764] virbr0-nic: set_features() failed (-1); wanted 0x00000080000048c1, left 0x00000080001b48c9
[   31.677855] device virbr0-nic entered promiscuous mode
[   31.677898] virbr0: set_features() failed (-1); wanted 0x000000801fdb78c9, left 0x000000801fff78e9
[   31.904892] nf_conntrack version 0.5.0 (65536 buckets, 262144 max)
[   32.087094] virbr0: set_features() failed (-1); wanted 0x000000801fdb78c9, left 0x000000801fff78e9
[   32.087196] virbr0: port 1(virbr0-nic) entered listening state
[   32.087205] virbr0: port 1(virbr0-nic) entered listening state
[   32.093676] br0: set_features() failed (-1); wanted 0x000000801fd978a9, left 0x000000801fff78e9
[   32.093786] virbr0: set_features() failed (-1); wanted 0x000000801fdb78c9, left 0x000000801fff78e9
[   32.093872] virbr0-nic: set_features() failed (-1); wanted 0x00000080000048c1, left 0x00000080001b48c9
[   32.093966] virbr0: set_features() failed (-1); wanted 0x000000801fdb78c9, left 0x000000801fff78e9
[   32.094051] virbr0-nic: set_features() failed (-1); wanted 0x00000080000048c1, left 0x00000080001b48c9
[   32.094132] virbr0: set_features() failed (-1); wanted 0x000000801fdb78c9, left 0x000000801fff78e9
[   32.124341] virbr0: port 1(virbr0-nic) entered disabled state
</dmesg>

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Nikolay Aleksandrov]

On 11/13/2015 11:29 AM, Jiri Pirko wrote:
> Fri, Nov 13, 2015 at 01:26:18AM CET, f.fainelli@gmail.com wrote:
>> On 04/11/15 18:56, David Miller wrote:
>>>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>>>  ...
>>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>>> ---
>>>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>>>> patch actually becomes quite minimal and doesn't ugly up the code much.
>>>
>>> Applied, thanks.
>>
>> This causes some warnings to be displayed for DSA stacked devices:
>>
>> [    1.272297] brcm-sf2 f0b00000.ethernet_switch: Starfighter 2 top:
>> 4.00, core: 2.00 base: 0xf0c80000, IRQs: 68, 69
>> [    1.283181] libphy: dsa slave smi: probed
>> [    1.344088] f0b403c0.mdio:05: Broadcom BCM7445 PHY revision: 0xd0,
>> patch: 3
>> [    1.658917] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
>> attached PHY at address 5 [Broadcom BCM7445]
>> [    1.669414] brcm-sf2 f0b00000.ethernet_switch gphy: set_features()
>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>> [    1.734202] brcm-sf2 f0b00000.ethernet_switch rgmii_1
>> (uninitialized): attached PHY at address 0 [Generic PHY]
>> [    1.744486] brcm-sf2 f0b00000.ethernet_switch rgmii_1: set_features()
>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>> [    1.809091] brcm-sf2 f0b00000.ethernet_switch rgmii_2
>> (uninitialized): attached PHY at address 1 [Generic PHY]
>> [    1.819364] brcm-sf2 f0b00000.ethernet_switch rgmii_2: set_features()
>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>> [    1.884090] brcm-sf2 f0b00000.ethernet_switch moca (uninitialized):
>> attached PHY at address 2 [Generic PHY]
>> [    1.894109] brcm-sf2 f0b00000.ethernet_switch moca: set_features()
>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>>
>> DSA slave network devices are not associated with their master network
>> device using the typical lower/upper netdev helpers.
>>
>> I do not have a good fix to come up with yet, but if you see something
>> obvious with net/dsa/slave.c, feel free to send patches for testing, I
>> can boot net-next on this platform.
> 
> I'm having similar issues with bridge, with linus's git now:
> 
[snip]

Hmm, I think it's because the bridge and dsa/slave don't have ndo_set_features()
so err is left as -1 and thus an error is reported which isn't actually true.
Before in this case the features would just get set, so could you please try
the following patch ?


diff --git a/net/core/dev.c b/net/core/dev.c
index ab9b8d0d115e..4a1d198dbbff 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6426,6 +6426,8 @@ int __netdev_update_features(struct net_device *dev)
 
 	if (dev->netdev_ops->ndo_set_features)
 		err = dev->netdev_ops->ndo_set_features(dev, features);
+	else
+		err = 0;
 
 	if (unlikely(err < 0)) {
 		netdev_err(dev,

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Laura Abbott]

On 11/13/2015 02:51 AM, Nikolay Aleksandrov wrote:
> On 11/13/2015 11:29 AM, Jiri Pirko wrote:
>> Fri, Nov 13, 2015 at 01:26:18AM CET, f.fainelli@gmail.com wrote:
>>> On 04/11/15 18:56, David Miller wrote:
>>>>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>>>>   ...
>>>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>>>> ---
>>>>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>>>>> patch actually becomes quite minimal and doesn't ugly up the code much.
>>>>
>>>> Applied, thanks.
>>>
>>> This causes some warnings to be displayed for DSA stacked devices:
>>>
>>> [    1.272297] brcm-sf2 f0b00000.ethernet_switch: Starfighter 2 top:
>>> 4.00, core: 2.00 base: 0xf0c80000, IRQs: 68, 69
>>> [    1.283181] libphy: dsa slave smi: probed
>>> [    1.344088] f0b403c0.mdio:05: Broadcom BCM7445 PHY revision: 0xd0,
>>> patch: 3
>>> [    1.658917] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
>>> attached PHY at address 5 [Broadcom BCM7445]
>>> [    1.669414] brcm-sf2 f0b00000.ethernet_switch gphy: set_features()
>>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>>> [    1.734202] brcm-sf2 f0b00000.ethernet_switch rgmii_1
>>> (uninitialized): attached PHY at address 0 [Generic PHY]
>>> [    1.744486] brcm-sf2 f0b00000.ethernet_switch rgmii_1: set_features()
>>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>>> [    1.809091] brcm-sf2 f0b00000.ethernet_switch rgmii_2
>>> (uninitialized): attached PHY at address 1 [Generic PHY]
>>> [    1.819364] brcm-sf2 f0b00000.ethernet_switch rgmii_2: set_features()
>>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>>> [    1.884090] brcm-sf2 f0b00000.ethernet_switch moca (uninitialized):
>>> attached PHY at address 2 [Generic PHY]
>>> [    1.894109] brcm-sf2 f0b00000.ethernet_switch moca: set_features()
>>> failed (-1); wanted 0x0000000000004020, left 0x0000000000004820
>>>
>>> DSA slave network devices are not associated with their master network
>>> device using the typical lower/upper netdev helpers.
>>>
>>> I do not have a good fix to come up with yet, but if you see something
>>> obvious with net/dsa/slave.c, feel free to send patches for testing, I
>>> can boot net-next on this platform.
>>
>> I'm having similar issues with bridge, with linus's git now:
>>
> [snip]
>
> Hmm, I think it's because the bridge and dsa/slave don't have ndo_set_features()
> so err is left as -1 and thus an error is reported which isn't actually true.
> Before in this case the features would just get set, so could you please try
> the following patch ?
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ab9b8d0d115e..4a1d198dbbff 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6426,6 +6426,8 @@ int __netdev_update_features(struct net_device *dev)
>
>   	if (dev->netdev_ops->ndo_set_features)
>   		err = dev->netdev_ops->ndo_set_features(dev, features);
> +	else
> +		err = 0;
>
>   	if (unlikely(err < 0)) {
>   		netdev_err(dev,


The patch seems to be working for at least one person who reported the
problem in Fedora rawhide https://bugzilla.redhat.com/show_bug.cgi?id=1281674

Thanks,
Laura

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Geert Uytterhoeven]

On Fri, Nov 13, 2015 at 1:26 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 04/11/15 18:56, David Miller wrote:
>>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>>  ...
>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>> ---
>>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>>> patch actually becomes quite minimal and doesn't ugly up the code much.
>>
>> Applied, thanks.
>
> This causes some warnings to be displayed for DSA stacked devices:

And a few more:

sh-eth ee700000.ethernet eth0: set_features() failed (-1); wanted
0x0000000000004000, left 0x0000000000004800
g_ether gadget usb0: set_features() failed (-1); wanted
0x0000000000004000, left 0x0000000000004800

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 net-next] net/core: ensure features get disabled on new lower devs

[Geert Uytterhoeven]

On Tue, Nov 17, 2015 at 10:02 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Nov 13, 2015 at 1:26 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 04/11/15 18:56, David Miller wrote:
>>>> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
>>>  ...
>>>> Reported-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>>> ---
>>>> v2: Based on suggestions from Alex, and with not changing err to ret, this
>>>> patch actually becomes quite minimal and doesn't ugly up the code much.
>>>
>>> Applied, thanks.
>>
>> This causes some warnings to be displayed for DSA stacked devices:
>
> And a few more:
>
> sh-eth ee700000.ethernet eth0: set_features() failed (-1); wanted
> 0x0000000000004000, left 0x0000000000004800
> g_ether gadget usb0: set_features() failed (-1); wanted
> 0x0000000000004000, left 0x0000000000004800

smsc911x 8000000.ethernet eth0: set_features() failed (-1); wanted
0x0000000000004000, left 0x0000000000004800

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 net-next] net/core: generic support for disabling netdev features down stack

[Michał Mirosław]

Hi,

Sorry for digging up an old patch, but... ;-)

dev_disable_lro() is a leftover from ancient times. If you read commit
27660515a,
there is a hint where it should go. Please, read on if you'd like to
fix this properly.

2015-11-03 3:55 GMT+01:00 Jarod Wilson <jarod@redhat.com>:
> There are some netdev features, which when disabled on an upper device,
> such as a bonding master or a bridge, must be disabled and cannot be
> re-enabled on underlying devices.
[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6288,6 +6288,44 @@ static void rollback_registered(struct net_device *dev)
>         list_del(&single);
>  }
>
> +static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
> +       struct net_device *upper, netdev_features_t features)
> +{
> +       netdev_features_t upper_disables = NETIF_F_UPPER_DISABLES;
> +       netdev_features_t feature;
> +
> +       for_each_netdev_feature(&upper_disables, feature) {
> +               if (!(upper->wanted_features & feature)
> +                   && (features & feature)) {
> +                       netdev_dbg(lower, "Dropping feature %pNF, upper dev %s has it off.\n",
> +                                  &feature, upper->name);
> +                       features &= ~feature;
> +               }
> +       }

You could do this once:

disable_features = ~upper->features & features & NETIF_F_UPPER_DISABLES;
if (features & disable_features)
  netdev_dbg(...)
features &= ~disable_features;

> +
> +       return features;
> +}
[...]
> @@ -6370,6 +6410,10 @@ int __netdev_update_features(struct net_device *dev)
>         /* driver might be less strict about feature dependencies */
>         features = netdev_fix_features(dev, features);
>
> +       /* some features can't be enabled if they're off an an upper device */
> +       netdev_for_each_upper_dev_rcu(dev, upper, iter)
> +               features = netdev_sync_upper_features(dev, upper, features);
> +
>         if (dev->features == features)
>                 return 0;
>

This should go into netdev_fix_features(), as it is a one single place,
where are feature dependencies are verified.

[...]
> @@ -6386,6 +6430,12 @@ int __netdev_update_features(struct net_device *dev)
>                 return -1;
>         }
>
> +       /* some features must be disabled on lower devices when disabled
> +        * on an upper device (think: bonding master or bridge)
> +        */
> +       netdev_for_each_lower_dev(dev, lower, iter)
> +               netdev_sync_lower_features(dev, lower, features);
> +
[...]

This should be equal in resulting flags to:

   netdev_for_each_lower_dev(dev, lower...)
     netdev_update_features(lower);

because netdev_fix_features(lower) will see already changed dev->features.

Best Regards,
Michał Mirosław