[PATCH iwl-net v1 0/2] Fix for bond 802.3ad mode with VFs

[PATCH iwl-net v1 0/2] Fix for bond 802.3ad mode with VFs

[Mateusz Palczewski]

Bond 802.3ad mode with PFs works fine.

Problem appears when one or both of the nodes contains VFs.
Let's assume there is setup with 2 hosts, on both there are 2 PFs with each one with 1 VFs.
Bond(802.3ad)--|--VFs(two VFs)--Switch(with lacp enabled)--VFs-Bond(802.3ad)
In this particular scenario, there are 2 problems:

1. VFs needs some more time than PFs to set the link up, bonding drivers checks link only once, without any delay.
This caused issues with setting up bonding link. Fixed by adding small delay loop which checks link state.
2. Only master link works properly. Slave link cannot negotiate connection. This happens because permanent hw address
is used for creating lacpdu packets, not current address. Fixed by using current hw address to create lacpdu packet.

Sebastian Basierski (2):
  drivers/net/bonding/bond_3ad: Use updated MAC address for lacpdu
    packets
  drivers/net/bonding: Added some delay while checking for VFs link

 drivers/net/bonding/bond_3ad.c  |  4 ++--
 drivers/net/bonding/bond_main.c | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH iwl-net v1 1/2] drivers/net/bonding/bond_3ad: Use updated MAC address for lacpdu packets

[Mateusz Palczewski]

From: Sebastian Basierski <sebastianx.basierski@intel.com>

After changing VFs MAC address, bonding driver shouldn't use
the old address. Otherwise lapcdu packets will have set wrong
source MAC address.

Fixes: ada0f8633c5b ("bonding: Convert memcpy(foo, bar, ETH_ALEN) to ether_addr_copy(foo, bar)")
Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
 drivers/net/bonding/bond_3ad.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c99ffe6c683a..b5202af79f20 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -869,10 +869,10 @@ static int ad_lacpdu_send(struct port *port)
 	lacpdu_header = skb_put(skb, length);
 
 	ether_addr_copy(lacpdu_header->hdr.h_dest, lacpdu_mcast_addr);
-	/* Note: source address is set to be the member's PERMANENT address,
+	/* Note: source address is set to be the member's CURRENT address,
 	 * because we use it to identify loopback lacpdus in receive.
 	 */
-	ether_addr_copy(lacpdu_header->hdr.h_source, slave->perm_hwaddr);
+	ether_addr_copy(lacpdu_header->hdr.h_source, slave->dev->dev_addr);
 	lacpdu_header->hdr.h_proto = PKT_TYPE_LACPDU;
 
 	lacpdu_header->lacpdu = port->lacpdu;
-- 
2.31.1

Re: [PATCH iwl-net v1 1/2] drivers/net/bonding/bond_3ad: Use updated MAC address for lacpdu packets

[Jay Vosburgh]

Mateusz Palczewski <mateusz.palczewski@intel.com> wrote:

>From: Sebastian Basierski <sebastianx.basierski@intel.com>
>
>After changing VFs MAC address, bonding driver shouldn't use
>the old address. Otherwise lapcdu packets will have set wrong
>source MAC address.

	This patch is incorrect, the existing code is behaving
correctly.

	Bonding uses the original device MAC address deliberately, as
IEEE 802.1AX-2014 6.2.1.i requires that each port utilize a MAC address
that is "unique over the LAG" as the source address for LACPDUs.

	As bonding sets all ports of the bond to the same MAC (so that
non-control traffic uses the same source MAC per 802.1AX 6.2.1.j), this
change would cause every port of the bond to use a single MAC address
for the LACPDU source address, thus violating 802.1AX.

	-J

>Fixes: ada0f8633c5b ("bonding: Convert memcpy(foo, bar, ETH_ALEN) to ether_addr_copy(foo, bar)")
>Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
>Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
>---
> drivers/net/bonding/bond_3ad.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index c99ffe6c683a..b5202af79f20 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -869,10 +869,10 @@ static int ad_lacpdu_send(struct port *port)
> 	lacpdu_header = skb_put(skb, length);
> 
> 	ether_addr_copy(lacpdu_header->hdr.h_dest, lacpdu_mcast_addr);
>-	/* Note: source address is set to be the member's PERMANENT address,
>+	/* Note: source address is set to be the member's CURRENT address,
> 	 * because we use it to identify loopback lacpdus in receive.
> 	 */
>-	ether_addr_copy(lacpdu_header->hdr.h_source, slave->perm_hwaddr);
>+	ether_addr_copy(lacpdu_header->hdr.h_source, slave->dev->dev_addr);
> 	lacpdu_header->hdr.h_proto = PKT_TYPE_LACPDU;
> 
> 	lacpdu_header->lacpdu = port->lacpdu;
>-- 
>2.31.1

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

[PATCH iwl-net v1 2/2] drivers/net/bonding: Added some delay while checking for VFs link

[Mateusz Palczewski]

From: Sebastian Basierski <sebastianx.basierski@intel.com>

Right now bonding driver checks if link is ready once.
VF interface takes a little more time to get ready than PF,
so driver needs to wait for it to be ready.
1000ms delay was set, if VF link will not be set within given amount
of time, for sure problems should be investigated elsewhere.

Fixes: b3c898e20b18 ("Revert "bonding: allow carrier and link status to determine link state"")
Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
 drivers/net/bonding/bond_main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 710548dbd0c1..6d49fb25969e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -736,6 +736,8 @@ const char *bond_slave_link_status(s8 link)
  * It'd be nice if there was a good way to tell if a driver supports
  * netif_carrier, but there really isn't.
  */
+#define BOND_CARRIER_CHECK_TIMEOUT 1000
+
 static int bond_check_dev_link(struct bonding *bond,
 			       struct net_device *slave_dev, int reporting)
 {
@@ -743,12 +745,22 @@ static int bond_check_dev_link(struct bonding *bond,
 	int (*ioctl)(struct net_device *, struct ifreq *, int);
 	struct ifreq ifr;
 	struct mii_ioctl_data *mii;
+	int delay;
 
 	if (!reporting && !netif_running(slave_dev))
 		return 0;
 
+	for (delay = 0; delay < BOND_CARRIER_CHECK_TIMEOUT; delay++) {
+		mdelay(1);
+
+		if (bond->params.use_carrier &&
+		    netif_carrier_ok(slave_dev)) {
+			return BMSR_LSTATUS;
+		}
+	}
+
 	if (bond->params.use_carrier)
-		return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
+		return 0;
 
 	/* Try to get link status using Ethtool first. */
 	if (slave_dev->ethtool_ops->get_link)
-- 
2.31.1

Re: [PATCH iwl-net v1 2/2] drivers/net/bonding: Added some delay while checking for VFs link

[Jay Vosburgh]

Mateusz Palczewski <mateusz.palczewski@intel.com> wrote:

>From: Sebastian Basierski <sebastianx.basierski@intel.com>
>
>Right now bonding driver checks if link is ready once.
>VF interface takes a little more time to get ready than PF,
>so driver needs to wait for it to be ready.
>1000ms delay was set, if VF link will not be set within given amount
>of time, for sure problems should be investigated elsewhere.

	Why is the "updelay" mechanism that's already available
insufficient for this purpose?

	Even without updelay, I'd expect the behavior to simply be that
the carrier state flaps once or twice (because the VF is delayed in
asserting carrier up).  This is reflecting reality; I'm unsure why we
would want to hack in an extra delay to cover that up.

	Regardless of whether updelay handles this case or not, adding a
1 second busy wait loop as this patch does is not a reasonable
implementation.  This would cause a 1 second stall in the link state
check for every bond interface that is carrier down.

	-J

>Fixes: b3c898e20b18 ("Revert "bonding: allow carrier and link status to determine link state"")
>Signed-off-by: Sebastian Basierski <sebastianx.basierski@intel.com>
>Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
>---
> drivers/net/bonding/bond_main.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 710548dbd0c1..6d49fb25969e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -736,6 +736,8 @@ const char *bond_slave_link_status(s8 link)
>  * It'd be nice if there was a good way to tell if a driver supports
>  * netif_carrier, but there really isn't.
>  */
>+#define BOND_CARRIER_CHECK_TIMEOUT 1000
>+
> static int bond_check_dev_link(struct bonding *bond,
> 			       struct net_device *slave_dev, int reporting)
> {
>@@ -743,12 +745,22 @@ static int bond_check_dev_link(struct bonding *bond,
> 	int (*ioctl)(struct net_device *, struct ifreq *, int);
> 	struct ifreq ifr;
> 	struct mii_ioctl_data *mii;
>+	int delay;
> 
> 	if (!reporting && !netif_running(slave_dev))
> 		return 0;
> 
>+	for (delay = 0; delay < BOND_CARRIER_CHECK_TIMEOUT; delay++) {
>+		mdelay(1);
>+
>+		if (bond->params.use_carrier &&
>+		    netif_carrier_ok(slave_dev)) {
>+			return BMSR_LSTATUS;
>+		}
>+	}
>+
> 	if (bond->params.use_carrier)
>-		return netif_carrier_ok(slave_dev) ? BMSR_LSTATUS : 0;
>+		return 0;
> 
> 	/* Try to get link status using Ethtool first. */
> 	if (slave_dev->ethtool_ops->get_link)
>-- 
>2.31.1
>
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com