From b25c2e7d3c44aaadee55d70f70c31cbc9014c713 Mon Sep 17 00:00:00 2001 From: Mahesh Bandewar Date: Sat, 31 Oct 2015 12:45:00 -0700 Subject: [PATCH 1/3] bonding: Simplify __get_duplex function. Eliminate 'else' clause by simply initializing variable Signed-off-by: Mahesh Bandewar Signed-off-by: David S. Miller --- drivers/net/bonding/bond_3ad.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 3c45358844eb..3a17fd207ec6 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -327,14 +327,12 @@ static u16 __get_link_speed(struct port *port) static u8 __get_duplex(struct port *port) { struct slave *slave = port->slave; - u8 retval; + u8 retval = 0x0; /* handling a special case: when the configuration starts with * link down, it sets the duplex to 0. */ - if (slave->link != BOND_LINK_UP) { - retval = 0x0; - } else { + if (slave->link == BOND_LINK_UP) { switch (slave->duplex) { case DUPLEX_FULL: retval = 0x1; From 7bb11dc9f59ddcb33ee317da77b235235aaa582a Mon Sep 17 00:00:00 2001 From: Mahesh Bandewar Date: Sat, 31 Oct 2015 12:45:06 -0700 Subject: [PATCH 2/3] bonding: unify all places where actor-oper key needs to be updated. actor_admin, and actor_oper key is changed at multiple locations in the code. This patch brings all those updates into one location in an attempt to avoid possible inconsistent updates causing LACP state machine to go in weird state. The unified place is ad_update_actor_key() with simple state-machine logic - (a) If port is "duplex" then only it can participate in LACP (b) Speed change reinitializes the LACP state-machine. Signed-off-by: Mahesh Bandewar Signed-off-by: David S. Miller --- drivers/net/bonding/bond_3ad.c | 87 ++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index 3a17fd207ec6..b9816b7f319f 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -127,6 +127,7 @@ static void ad_marker_info_received(struct bond_marker *marker_info, struct port *port); static void ad_marker_response_received(struct bond_marker *marker, struct port *port); +static void ad_update_actor_keys(struct port *port, bool reset); /* ================= api to bonding and kernel code ================== */ @@ -1951,14 +1952,7 @@ void bond_3ad_bind_slave(struct slave *slave) * user key */ port->actor_admin_port_key = bond->params.ad_user_port_key << 6; - port->actor_admin_port_key |= __get_duplex(port); - port->actor_admin_port_key |= (__get_link_speed(port) << 1); - port->actor_oper_port_key = port->actor_admin_port_key; - /* if the port is not full duplex, then the port should be not - * lacp Enabled - */ - if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)) - port->sm_vars &= ~AD_PORT_LACP_ENABLED; + ad_update_actor_keys(port, false); /* actor system is the bond's system */ port->actor_system = BOND_AD_INFO(bond).system.sys_mac_addr; port->actor_system_priority = @@ -2307,6 +2301,52 @@ static int bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, return ret; } +/** + * ad_update_actor_keys - Update the oper / admin keys for a port based on + * its current speed and duplex settings. + * + * @port: the port we'are looking at + * @reset: Boolean to just reset the speed and the duplex part of the key + * + * The logic to change the oper / admin keys is: + * (a) A full duplex port can participate in LACP with partner. + * (b) When the speed is changed, LACP need to be reinitiated. + */ +static void ad_update_actor_keys(struct port *port, bool reset) +{ + u8 duplex = 0; + u16 ospeed = 0, speed = 0; + u16 old_oper_key = port->actor_oper_port_key; + + port->actor_admin_port_key &= ~(AD_SPEED_KEY_MASKS|AD_DUPLEX_KEY_MASKS); + if (!reset) { + speed = __get_link_speed(port); + ospeed = (old_oper_key & AD_SPEED_KEY_MASKS) >> 1; + duplex = __get_duplex(port); + port->actor_admin_port_key |= (speed << 1) | duplex; + } + port->actor_oper_port_key = port->actor_admin_port_key; + + if (old_oper_key != port->actor_oper_port_key) { + /* Only 'duplex' port participates in LACP */ + if (duplex) + port->sm_vars |= AD_PORT_LACP_ENABLED; + else + port->sm_vars &= ~AD_PORT_LACP_ENABLED; + + if (!reset) { + if (!speed) { + netdev_err(port->slave->dev, + "speed changed to 0 for port %s", + port->slave->dev->name); + } else if (duplex && ospeed != speed) { + /* Speed change restarts LACP state-machine */ + port->sm_vars |= AD_PORT_BEGIN; + } + } + } +} + /** * bond_3ad_adapter_speed_changed - handle a slave's speed change indication * @slave: slave struct to work on @@ -2328,14 +2368,8 @@ void bond_3ad_adapter_speed_changed(struct slave *slave) spin_lock_bh(&slave->bond->mode_lock); - port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS; - port->actor_admin_port_key |= __get_link_speed(port) << 1; - port->actor_oper_port_key = port->actor_admin_port_key; + ad_update_actor_keys(port, false); netdev_dbg(slave->bond->dev, "Port %d changed speed\n", port->actor_port_number); - /* there is no need to reselect a new aggregator, just signal the - * state machines to reinitialize - */ - port->sm_vars |= AD_PORT_BEGIN; spin_unlock_bh(&slave->bond->mode_lock); } @@ -2361,17 +2395,9 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave) spin_lock_bh(&slave->bond->mode_lock); - port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS; - port->actor_admin_port_key |= __get_duplex(port); - port->actor_oper_port_key = port->actor_admin_port_key; + ad_update_actor_keys(port, false); netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n", port->actor_port_number, slave->dev->name); - if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS) - port->sm_vars |= AD_PORT_LACP_ENABLED; - /* there is no need to reselect a new aggregator, just signal the - * state machines to reinitialize - */ - port->sm_vars |= AD_PORT_BEGIN; spin_unlock_bh(&slave->bond->mode_lock); } @@ -2404,26 +2430,17 @@ void bond_3ad_handle_link_change(struct slave *slave, char link) * on link up we are forcing recheck on the duplex and speed since * some of he adaptors(ce1000.lan) report. */ - port->actor_admin_port_key &= ~(AD_DUPLEX_KEY_MASKS|AD_SPEED_KEY_MASKS); if (link == BOND_LINK_UP) { port->is_enabled = true; - port->actor_admin_port_key |= - (__get_link_speed(port) << 1) | __get_duplex(port); - if (port->actor_admin_port_key & AD_DUPLEX_KEY_MASKS) - port->sm_vars |= AD_PORT_LACP_ENABLED; + ad_update_actor_keys(port, false); } else { /* link has failed */ port->is_enabled = false; - port->sm_vars &= ~AD_PORT_LACP_ENABLED; + ad_update_actor_keys(port, true); } - port->actor_oper_port_key = port->actor_admin_port_key; netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n", port->actor_port_number, link == BOND_LINK_UP ? "UP" : "DOWN"); - /* there is no need to reselect a new aggregator, just signal the - * state machines to reinitialize - */ - port->sm_vars |= AD_PORT_BEGIN; spin_unlock_bh(&slave->bond->mode_lock); From 52bc67168109ade61014a36feedf09f4bc53d8f1 Mon Sep 17 00:00:00 2001 From: Mahesh Bandewar Date: Sat, 31 Oct 2015 12:45:11 -0700 Subject: [PATCH 3/3] bonding: simplify / unify event handling code for 3ad mode. Old logic of updating state-machine is not required since ad_update_actor_keys() does it implicitly. The only loss is the notification differentiation between speed vs. duplex change. Now only one unified notification is printed. Signed-off-by: Mahesh Bandewar Signed-off-by: David S. Miller --- drivers/net/bonding/bond_3ad.c | 40 ++++++--------------------------- drivers/net/bonding/bond_main.c | 14 ++---------- include/net/bond_3ad.h | 3 +-- 3 files changed, 10 insertions(+), 47 deletions(-) diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index b9816b7f319f..940e2ebbdea8 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -2348,12 +2348,14 @@ static void ad_update_actor_keys(struct port *port, bool reset) } /** - * bond_3ad_adapter_speed_changed - handle a slave's speed change indication + * bond_3ad_adapter_speed_duplex_changed - handle a slave's speed / duplex + * change indication + * * @slave: slave struct to work on * * Handle reselection of aggregator (if needed) for this port. */ -void bond_3ad_adapter_speed_changed(struct slave *slave) +void bond_3ad_adapter_speed_duplex_changed(struct slave *slave) { struct port *port; @@ -2361,44 +2363,16 @@ void bond_3ad_adapter_speed_changed(struct slave *slave) /* if slave is null, the whole port is not initialized */ if (!port->slave) { - netdev_warn(slave->bond->dev, "speed changed for uninitialized port on %s\n", + netdev_warn(slave->bond->dev, + "speed/duplex changed for uninitialized port %s\n", slave->dev->name); return; } spin_lock_bh(&slave->bond->mode_lock); - ad_update_actor_keys(port, false); - netdev_dbg(slave->bond->dev, "Port %d changed speed\n", port->actor_port_number); - - spin_unlock_bh(&slave->bond->mode_lock); -} - -/** - * bond_3ad_adapter_duplex_changed - handle a slave's duplex change indication - * @slave: slave struct to work on - * - * Handle reselection of aggregator (if needed) for this port. - */ -void bond_3ad_adapter_duplex_changed(struct slave *slave) -{ - struct port *port; - - port = &(SLAVE_AD_INFO(slave)->port); - - /* if slave is null, the whole port is not initialized */ - if (!port->slave) { - netdev_warn(slave->bond->dev, "duplex changed for uninitialized port on %s\n", - slave->dev->name); - return; - } - - spin_lock_bh(&slave->bond->mode_lock); - - ad_update_actor_keys(port, false); - netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n", + netdev_dbg(slave->bond->dev, "Port %d slave %s changed speed/duplex\n", port->actor_port_number, slave->dev->name); - spin_unlock_bh(&slave->bond->mode_lock); } diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index d0f23cd6e236..b4351caf8e01 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2943,8 +2943,6 @@ static int bond_slave_netdev_event(unsigned long event, struct slave *slave = bond_slave_get_rtnl(slave_dev), *primary; struct bonding *bond; struct net_device *bond_dev; - u32 old_speed; - u8 old_duplex; /* A netdev event can be generated while enslaving a device * before netdev_rx_handler_register is called in which case @@ -2965,17 +2963,9 @@ static int bond_slave_netdev_event(unsigned long event, break; case NETDEV_UP: case NETDEV_CHANGE: - old_speed = slave->speed; - old_duplex = slave->duplex; - bond_update_speed_duplex(slave); - - if (BOND_MODE(bond) == BOND_MODE_8023AD) { - if (old_speed != slave->speed) - bond_3ad_adapter_speed_changed(slave); - if (old_duplex != slave->duplex) - bond_3ad_adapter_duplex_changed(slave); - } + if (BOND_MODE(bond) == BOND_MODE_8023AD) + bond_3ad_adapter_speed_duplex_changed(slave); /* Fallthrough */ case NETDEV_DOWN: /* Refresh slave-array if applicable! diff --git a/include/net/bond_3ad.h b/include/net/bond_3ad.h index c2a40a172fcd..f1fbc3b11962 100644 --- a/include/net/bond_3ad.h +++ b/include/net/bond_3ad.h @@ -297,8 +297,7 @@ void bond_3ad_bind_slave(struct slave *slave); void bond_3ad_unbind_slave(struct slave *slave); void bond_3ad_state_machine_handler(struct work_struct *); void bond_3ad_initiate_agg_selection(struct bonding *bond, int timeout); -void bond_3ad_adapter_speed_changed(struct slave *slave); -void bond_3ad_adapter_duplex_changed(struct slave *slave); +void bond_3ad_adapter_speed_duplex_changed(struct slave *slave); void bond_3ad_handle_link_change(struct slave *slave, char link); int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info); int __bond_3ad_get_active_agg_info(struct bonding *bond,