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" <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Jay Vosburgh <[email protected]>
CC: Veaceslav Falico <[email protected]>
CC: Andy Gospodarek <[email protected]>
CC: Jiri Pirko <[email protected]>
CC: Nikolay Aleksandrov <[email protected]>
CC: Michal Kubecek <[email protected]>
CC: Alexander Duyck <[email protected]>
CC: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
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
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" <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: Jay Vosburgh <[email protected]>
> CC: Veaceslav Falico <[email protected]>
> CC: Andy Gospodarek <[email protected]>
> CC: Jiri Pirko <[email protected]>
> CC: Nikolay Aleksandrov <[email protected]>
> CC: Michal Kubecek <[email protected]>
> CC: Alexander Duyck <[email protected]>
> CC: [email protected]
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> 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;
>
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" <[email protected]>
>> CC: Eric Dumazet <[email protected]>
>> CC: Jay Vosburgh <[email protected]>
>> CC: Veaceslav Falico <[email protected]>
>> CC: Andy Gospodarek <[email protected]>
>> CC: Jiri Pirko <[email protected]>
>> CC: Nikolay Aleksandrov <[email protected]>
>> CC: Michal Kubecek <[email protected]>
>> CC: Alexander Duyck <[email protected]>
>> CC: [email protected]
>> Signed-off-by: Jarod Wilson <[email protected]>
>> ---
>> 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
[email protected]
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" <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Jay Vosburgh <[email protected]>
CC: Veaceslav Falico <[email protected]>
CC: Andy Gospodarek <[email protected]>
CC: Jiri Pirko <[email protected]>
CC: Nikolay Aleksandrov <[email protected]>
CC: Michal Kubecek <[email protected]>
CC: Alexander Duyck <[email protected]>
CC: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
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
From: Jarod Wilson <[email protected]>
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 <[email protected]>
Looks good to me, applied, thanks Jarod.
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]
On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
<[email protected]> 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 -- [email protected]
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
Geert Uytterhoeven wrote:
> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
> <[email protected]> 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
[email protected]
On 11/03/2015 02:57 PM, Jarod Wilson wrote:
> Geert Uytterhoeven wrote:
>> On Tue, Nov 3, 2015 at 11:03 AM, Nikolay Aleksandrov
>> <[email protected]> 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
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" <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Jay Vosburgh <[email protected]>
CC: Veaceslav Falico <[email protected]>
CC: Andy Gospodarek <[email protected]>
CC: Jiri Pirko <[email protected]>
CC: Nikolay Aleksandrov <[email protected]>
CC: Michal Kubecek <[email protected]>
CC: Alexander Duyck <[email protected]>
CC: Geert Uytterhoeven <[email protected]>
CC: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
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
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
>>> <[email protected]> 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
[email protected]
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" <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: Jay Vosburgh <[email protected]>
> CC: Veaceslav Falico <[email protected]>
> CC: Andy Gospodarek <[email protected]>
> CC: Jiri Pirko <[email protected]>
> CC: Nikolay Aleksandrov <[email protected]>
> CC: Michal Kubecek <[email protected]>
> CC: Alexander Duyck <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: [email protected]
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
Nik
From: Jarod Wilson <[email protected]>
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 <[email protected]>
Applied.
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" <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Jay Vosburgh <[email protected]>
CC: Veaceslav Falico <[email protected]>
CC: Andy Gospodarek <[email protected]>
CC: Jiri Pirko <[email protected]>
CC: Nikolay Aleksandrov <[email protected]>
CC: Michal Kubecek <[email protected]>
CC: Alexander Duyck <[email protected]>
CC: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
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
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" <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: Jay Vosburgh <[email protected]>
> CC: Veaceslav Falico <[email protected]>
> CC: Andy Gospodarek <[email protected]>
> CC: Jiri Pirko <[email protected]>
> CC: Nikolay Aleksandrov <[email protected]>
> CC: Michal Kubecek <[email protected]>
> CC: Alexander Duyck <[email protected]>
> CC: [email protected]
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> 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;
> }
>
> /**
>
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" <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: Jay Vosburgh <[email protected]>
> CC: Veaceslav Falico <[email protected]>
> CC: Andy Gospodarek <[email protected]>
> CC: Jiri Pirko <[email protected]>
> CC: Nikolay Aleksandrov <[email protected]>
> CC: Michal Kubecek <[email protected]>
> CC: Alexander Duyck <[email protected]>
> CC: [email protected]
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> net/core/dev.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
Thanks for the quick response,
Reported-by: Nikolay Aleksandrov <[email protected]>
Tested-by: Nikolay Aleksandrov <[email protected]>
Acked-by: Nikolay Aleksandrov <[email protected]>
Fixes: fd867d51f889 ("net/core: generic support for disabling netdev features down stack")
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
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
[email protected]
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
[email protected]
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?
>
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" <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: Jay Vosburgh <[email protected]>
CC: Veaceslav Falico <[email protected]>
CC: Andy Gospodarek <[email protected]>
CC: Jiri Pirko <[email protected]>
CC: Nikolay Aleksandrov <[email protected]>
CC: Michal Kubecek <[email protected]>
CC: Alexander Duyck <[email protected]>
CC: [email protected]
Reported-by: Nikolay Aleksandrov <[email protected]>
Signed-off-by: Jarod Wilson <[email protected]>
---
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
From: Jarod Wilson <[email protected]>
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 <[email protected]>
> Signed-off-by: Jarod Wilson <[email protected]>
> ---
> 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.
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 <[email protected]>
>> Signed-off-by: Jarod Wilson <[email protected]>
>> ---
>> 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
Fri, Nov 13, 2015 at 01:26:18AM CET, [email protected] 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 <[email protected]>
>>> Signed-off-by: Jarod Wilson <[email protected]>
>>> ---
>>> 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 <[email protected]>
[ 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>
On 11/13/2015 11:29 AM, Jiri Pirko wrote:
> Fri, Nov 13, 2015 at 01:26:18AM CET, [email protected] 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 <[email protected]>
>>>> Signed-off-by: Jarod Wilson <[email protected]>
>>>> ---
>>>> 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,
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, [email protected] 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 <[email protected]>
>>>>> Signed-off-by: Jarod Wilson <[email protected]>
>>>>> ---
>>>>> 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
On Fri, Nov 13, 2015 at 1:26 AM, Florian Fainelli <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Jarod Wilson <[email protected]>
>>> ---
>>> 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 -- [email protected]
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
On Tue, Nov 17, 2015 at 10:02 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Fri, Nov 13, 2015 at 1:26 AM, Florian Fainelli <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Jarod Wilson <[email protected]>
>>>> ---
>>>> 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 -- [email protected]
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
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 <[email protected]>:
> 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