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

[Jay Vosburgh <fubar@us.ibm.com>]

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