Patchwork [2/2] bonding: restore NETIF_F_VLAN_CHALLENGED properly in bond_del_vlan()

login
register
mail settings
Submitter Jiri Bohac
Date June 3, 2011, 8:14 p.m.
Message ID <20110603201424.GB24804@midget.suse.cz>
Download mbox | patch
Permalink /patch/98650/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jiri Bohac - June 3, 2011, 8:14 p.m.
Since commit ad1afb00, bond_del_vlan() never restores
NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
empty once the 8021q module is loaded, because the special VLAN 0
is always kept registered on the bond interface. Change the
condition to check if bond->vlan_list contains exactly one item
instead of checking for an empty list.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>
Jay Vosburgh - June 4, 2011, 12:26 a.m.
Jiri Bohac <jbohac@suse.cz> wrote:

>Since commit ad1afb00, bond_del_vlan() never restores
>NETIF_F_VLAN_CHALLENGED as intended.  bond->vlan_list is never
>empty once the 8021q module is loaded, because the special VLAN 0
>is always kept registered on the bond interface. Change the
>condition to check if bond->vlan_list contains exactly one item
>instead of checking for an empty list.
>
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 17b4dd9..4d317cd 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -329,9 +329,10 @@ static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
>
> 			kfree(vlan);
>
>-			if (list_empty(&bond->vlan_list) &&
>+			if (bond->vlan_list.next->next == &bond->vlan_list &&
> 			    (bond->slave_cnt == 0)) {
>-				/* Last VLAN removed and no slaves, so
>+				/* Last VLAN removed (the only member of vlan_list
>+				 * is the special vid == 0 vlan) and no slaves, so
> 				 * restore block on adding VLANs. This will
> 				 * be removed once new slaves that are not
> 				 * VLAN challenged will be added.

	Could we do this instead in bond_release, when the last slave is
removed?  The CHALLENGED flag just prevents adding new VLANs; existing
ones would persist until a new slave was added.

	Since CHALLENGED slaves are in the minority these days (looks
like just IPoIB, one wimax and one obscure ethernet chipset) we could
even invert the logic: only assert CHALLENGED for the master when such a
slave is added.  We'd have to issue a NETDEV_CHANGEADDR when the master
picks up or releases its MAC address so the VLANs would pick it up, but
that's not a really big deal, and we probably ought to do that anyway.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
--
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

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 17b4dd9..4d317cd 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -329,9 +329,10 @@  static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 
 			kfree(vlan);
 
-			if (list_empty(&bond->vlan_list) &&
+			if (bond->vlan_list.next->next == &bond->vlan_list &&
 			    (bond->slave_cnt == 0)) {
-				/* Last VLAN removed and no slaves, so
+				/* Last VLAN removed (the only member of vlan_list
+				 * is the special vid == 0 vlan) and no slaves, so
 				 * restore block on adding VLANs. This will
 				 * be removed once new slaves that are not
 				 * VLAN challenged will be added.