lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 09 May 2011 19:00:40 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	aquini@...ux.com
cc:	kernel-janitors@...r.kernel.org,
	David Miller <davem@...emloft.net>,
	Joe Perches <joe@...ches.com>,
	Andy Gospodarek <andy@...yhouse.net>, shemminger@...tta.com,
	netdev@...r.kernel.org, Nicolas Kaiser <nikai@...ai.net>
Subject: Re: [PATCH] net/bonding: adjust codingstyle for bond_3ad files

Rafael Aquini <aquini@...ux.com> wrote:

>Howdy,
>
>While I was studying what bond_3ad has under its hood, I realized its coding
>style did not follow all Documentation/CodingStyle recommendations. As a tiny
>collaboration I did some mods there, in an attempt to make that code stick as
>closely as possible with Kernel's coding style. Also, Nicolas Kaiser has kindly
>suggested some conditional simplifications integrated in this patch.
>Modifications:
>        * switched all comments from C99-style to C89-style;
>        * replaced MAC_ADDRESS_COMPARE macro for compare_ether_addr();
>	* use pr_debug to print info on unexpected status checkings;
>        * simplify conditionals:
>		(a || (!a && !b)) => (a || !b)
>		(!(!a && b)) => (a || !b)
>
>Signed-off-by: Rafael Aquini <aquini@...ux.com>
>Signed-off-by: Nicolas Kaiser <nikai@...ai.net>
>---
> drivers/net/bonding/bond_3ad.c |  886 +++++++++++++++++++++++-----------------
> drivers/net/bonding/bond_3ad.h |  195 +++++-----
> 2 files changed, 618 insertions(+), 463 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>index 31912f1..af3fc20 100644
>--- a/drivers/net/bonding/bond_3ad.c
>+++ b/drivers/net/bonding/bond_3ad.c
>@@ -34,14 +34,14 @@
> #include "bonding.h"
> #include "bond_3ad.h"
>
>-// General definitions
>+/* General definitions */
> #define AD_SHORT_TIMEOUT           1
> #define AD_LONG_TIMEOUT            0
> #define AD_STANDBY                 0x2
> #define AD_MAX_TX_IN_SECOND        3
> #define AD_COLLECTOR_MAX_DELAY     0
>
>-// Timer definitions(43.4.4 in the 802.3ad standard)
>+/* Timer definitions (43.4.4 in the 802.3ad standard) */
> #define AD_FAST_PERIODIC_TIME      1
> #define AD_SLOW_PERIODIC_TIME      30
> #define AD_SHORT_TIMEOUT_TIME      (3*AD_FAST_PERIODIC_TIME)
>@@ -49,7 +49,7 @@
> #define AD_CHURN_DETECTION_TIME    60
> #define AD_AGGREGATE_WAIT_TIME     2
>
>-// Port state definitions(43.4.2.2 in the 802.3ad standard)
>+/* Port state definitions (43.4.2.2 in the 802.3ad standard) */
> #define AD_STATE_LACP_ACTIVITY   0x1
> #define AD_STATE_LACP_TIMEOUT    0x2
> #define AD_STATE_AGGREGATION     0x4
>@@ -59,7 +59,9 @@
> #define AD_STATE_DEFAULTED       0x40
> #define AD_STATE_EXPIRED         0x80
>
>-// Port Variables definitions used by the State Machines(43.4.7 in the 802.3ad standard)
>+/* Port Variables definitions used by the State Machines
>+ * (43.4.7 in the 802.3ad standard)
>+ */
> #define AD_PORT_BEGIN           0x1
> #define AD_PORT_LACP_ENABLED    0x2
> #define AD_PORT_ACTOR_CHURN     0x4
>@@ -71,27 +73,23 @@
> #define AD_PORT_SELECTED        0x100
> #define AD_PORT_MOVED           0x200
>
>-// Port Key definitions
>-// key is determined according to the link speed, duplex and
>-// user key(which is yet not supported)
>-//              ------------------------------------------------------------
>-// Port key :   | User key                       |      Speed       |Duplex|
>-//              ------------------------------------------------------------
>-//              16                               6               1 0
>+/* Port Key definitions:
>+ *  key is determined according to the link speed, duplex and
>+ *  user key (which is yet not supported)
>+ *             ------------------------------------------------------------
>+ *  Port key:  | User key                       |      Speed       |Duplex|
>+ *             ------------------------------------------------------------
>+ *             16                               6                  1      0
>+ */
> #define  AD_DUPLEX_KEY_BITS    0x1
> #define  AD_SPEED_KEY_BITS     0x3E
> #define  AD_USER_KEY_BITS      0xFFC0
>
>-//dalloun
> #define     AD_LINK_SPEED_BITMASK_1MBPS       0x1
> #define     AD_LINK_SPEED_BITMASK_10MBPS      0x2
> #define     AD_LINK_SPEED_BITMASK_100MBPS     0x4
> #define     AD_LINK_SPEED_BITMASK_1000MBPS    0x8
> #define     AD_LINK_SPEED_BITMASK_10000MBPS   0x10
>-//endalloun
>-
>-// compare MAC addresses
>-#define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)
>
> static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };
> static u16 ad_ticks_per_sec;
>@@ -99,7 +97,7 @@ static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;
>
> static const u8 lacpdu_mcast_addr[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
>
>-// ================= main 802.3ad protocol functions ==================
>+/* ================= main 802.3ad protocol functions ================== */
> static int ad_lacpdu_send(struct port *port);
> static int ad_marker_send(struct port *port, struct bond_marker *marker);
> static void ad_mux_machine(struct port *port);
>@@ -113,14 +111,12 @@ static void ad_initialize_agg(struct aggregator *aggregator);
> static void ad_initialize_port(struct port *port, int lacp_fast);
> static void ad_enable_collecting_distributing(struct port *port);
> static void ad_disable_collecting_distributing(struct port *port);
>-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);
>-
>-
>-/////////////////////////////////////////////////////////////////////////////////
>-// ================= api to bonding and kernel code ==================
>-/////////////////////////////////////////////////////////////////////////////////
>+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);
>
>+/* ================= api to bonding and kernel code ================== */
> /**
>  * __get_bond_by_port - get the port's bonding struct
>  * @port: the port we're looking at
>@@ -161,7 +157,7 @@ static inline struct port *__get_next_port(struct port *port)
> 	struct bonding *bond = __get_bond_by_port(port);
> 	struct slave *slave = port->slave;
>
>-	// If there's no bond for this port, or this is the last slave
>+	/* If there's no bond for this port, or this is the last slave */
> 	if ((bond == NULL) || (slave->next == bond->first_slave))
> 		return NULL;
>
>@@ -179,7 +175,7 @@ static inline struct aggregator *__get_first_agg(struct port *port)
> {
> 	struct bonding *bond = __get_bond_by_port(port);
>
>-	// If there's no bond for this port, or bond has no slaves
>+	/* If there's no bond for this port, or bond has no slaves */
> 	if ((bond == NULL) || (bond->slave_cnt == 0))
> 		return NULL;
>
>@@ -198,7 +194,7 @@ static inline struct aggregator *__get_next_agg(struct aggregator *aggregator)
> 	struct slave *slave = aggregator->slave;
> 	struct bonding *bond = bond_get_bond_by_slave(slave);
>
>-	// If there's no bond for this aggregator, or this is the last slave
>+	/* If there's no bond for this aggregator, or this is the last slave */
> 	if ((bond == NULL) || (slave->next == bond->first_slave))
> 		return NULL;
>
>@@ -316,10 +312,9 @@ static u16 __get_link_speed(struct port *port)
> 	struct slave *slave = port->slave;
> 	u16 speed;
>
>-	/* this if covers only a special case: when the configuration starts with
>-	 * link down, it sets the speed to 0.
>-	 * This is done in spite of the fact that the e100 driver reports 0 to be
>-	 * compatible with MVT in the future.*/
>+	/* handling a special case:
>+	 * when the configuration starts with link down, it sets the speed to 0
>+	 */
> 	if (slave->link != BOND_LINK_UP)
> 		speed = 0;
> 	else {
>@@ -341,7 +336,9 @@ static u16 __get_link_speed(struct port *port)
> 			break;
>
> 		default:
>-			speed = 0; // unknown speed value from ethtool. shouldn't happen
>+			/* unknown speed value from ethtool. shouldn't happen */
>+			pr_debug("Unexpected slave->speed: %d\n", slave->speed);
>+			speed = 0;
> 			break;
> 		}
> 	}
>@@ -362,13 +359,13 @@ static u16 __get_link_speed(struct port *port)
> static u8 __get_duplex(struct port *port)
> {
> 	struct slave *slave = port->slave;
>+	u8 retval = 0x0;
>
>-	u8 retval;
>-
>-	//  handling a special case: when the configuration starts with
>-	// link down, it sets the duplex to 0.
>+	/* 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;
>+		goto out;
> 	else {
> 		switch (slave->duplex) {
> 		case DUPLEX_FULL:
>@@ -384,6 +381,7 @@ static u8 __get_duplex(struct port *port)
> 			break;
> 		}
> 	}
>+out:
> 	return retval;
> }
>
>@@ -394,12 +392,10 @@ static u8 __get_duplex(struct port *port)
>  */
> static inline void __initialize_port_locks(struct port *port)
> {
>-	// make sure it isn't called twice
>+	/* make sure it isn't called twice */
> 	spin_lock_init(&(SLAVE_AD_INFO(port->slave).state_machine_lock));

	This comment can just be removed; I think we know what locks are
for.

>
>-//conversions
>-
> /**
>  * __ad_timer_to_ticks - convert a given timer type to AD module ticks
>  * @timer_type:	which timer to operate
>@@ -411,36 +407,37 @@ static inline void __initialize_port_locks(struct port *port)
>  */
> static u16 __ad_timer_to_ticks(u16 timer_type, u16 par)
> {
>-	u16 retval = 0; /* to silence the compiler */
>+	u16 uninitialized_var(retval);
>
> 	switch (timer_type) {
>-	case AD_CURRENT_WHILE_TIMER:   // for rx machine usage
>+	case AD_CURRENT_WHILE_TIMER:	/* for rx machine usage */
> 		if (par)
>-			retval = (AD_SHORT_TIMEOUT_TIME*ad_ticks_per_sec); // short timeout
>+			retval = (AD_SHORT_TIMEOUT_TIME*ad_ticks_per_sec);
> 		else
>-			retval = (AD_LONG_TIMEOUT_TIME*ad_ticks_per_sec); // long timeout
>+			retval = (AD_LONG_TIMEOUT_TIME*ad_ticks_per_sec);
> 		break;
>-	case AD_ACTOR_CHURN_TIMER:	    // for local churn machine
>+	case AD_ACTOR_CHURN_TIMER:	/* for local churn machine */
> 		retval = (AD_CHURN_DETECTION_TIME*ad_ticks_per_sec);
> 		break;
>-	case AD_PERIODIC_TIMER:	    // for periodic machine
>-		retval = (par*ad_ticks_per_sec); // long timeout
>+	case AD_PERIODIC_TIMER:		/* for periodic machine */
>+		retval = (par*ad_ticks_per_sec);
> 		break;
>-	case AD_PARTNER_CHURN_TIMER:   // for remote churn machine
>+	case AD_PARTNER_CHURN_TIMER:	/* for remote churn machine */
> 		retval = (AD_CHURN_DETECTION_TIME*ad_ticks_per_sec);
> 		break;
>-	case AD_WAIT_WHILE_TIMER:	    // for selection machine
>+	case AD_WAIT_WHILE_TIMER:	/* for selection machine */
> 		retval = (AD_AGGREGATE_WAIT_TIME*ad_ticks_per_sec);
> 		break;
>+	default:
>+		/* Exception: unexpected timer_type received */
>+		pr_debug("Unexpected AD timer_type: %d\n", timer_type);
>+		retval = 0;
>+		break;

	I think this should be more verbose than a pr_debug, because
this is an impossible case that should never happen in actual practice,
and, if it does, may break things in odd ways.

> 	}
> 	return retval;
> }
>
>-
>-/////////////////////////////////////////////////////////////////////////////////
>-// ================= ad_rx_machine helper functions ==================
>-/////////////////////////////////////////////////////////////////////////////////
>-
>+/* ================= ad_rx_machine helper functions ================== */
> /**
>  * __choose_matched - update a port's matched variable from a received lacpdu
>  * @lacpdu: the lacpdu we've received
>@@ -466,17 +463,17 @@ static u16 __ad_timer_to_ticks(u16 timer_type, u16 par)
>  */
> static void __choose_matched(struct lacpdu *lacpdu, struct port *port)
> {
>-	// check if all parameters are alike
>+	/* check if all parameters are alike */
> 	if (((ntohs(lacpdu->partner_port) == port->actor_port_number) &&
> 	     (ntohs(lacpdu->partner_port_priority) == port->actor_port_priority) &&
>-	     !MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) &&
>+	     !compare_ether_addr((const u8 *)&(lacpdu->partner_system), (const u8 *)&(port->actor_system)) &&
> 	     (ntohs(lacpdu->partner_system_priority) == port->actor_system_priority) &&
> 	     (ntohs(lacpdu->partner_key) == port->actor_oper_port_key) &&
> 	     ((lacpdu->partner_state & AD_STATE_AGGREGATION) == (port->actor_oper_port_state & AD_STATE_AGGREGATION))) ||
>-	    // or this is individual link(aggregation == FALSE)
>+	    /* or this is individual link(aggregation == FALSE) */
> 	    ((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
> 		) {
>-		// update the state machine Matched variable
>+		/* update the state machine Matched variable */
> 		port->sm_vars |= AD_PORT_MATCHED;
> 	} else {
> 		port->sm_vars &= ~AD_PORT_MATCHED;
>@@ -498,7 +495,7 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
> 		struct port_params *partner = &port->partner_oper;
>
> 		__choose_matched(lacpdu, port);
>-		// record the new parameter values for the partner operational
>+		/* record the new values for the operational partner */
> 		partner->port_number = ntohs(lacpdu->actor_port);
> 		partner->port_priority = ntohs(lacpdu->actor_port_priority);
> 		partner->system = lacpdu->actor_system;
>@@ -506,12 +503,14 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
> 		partner->key = ntohs(lacpdu->actor_key);
> 		partner->port_state = lacpdu->actor_state;
>
>-		// set actor_oper_port_state.defaulted to FALSE
>+		/* set actor_oper_port_state.defaulted to FALSE */
> 		port->actor_oper_port_state &= ~AD_STATE_DEFAULTED;
>
>-		// set the partner sync. to on if the partner is sync. and the port is matched
>-		if ((port->sm_vars & AD_PORT_MATCHED)
>-		    && (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
>+		/* set the partner sync to on if the partner is sync
>+		 * and the port is matched.
>+		 */
>+		if ((port->sm_vars & AD_PORT_MATCHED) &&
>+		    (lacpdu->actor_state & AD_STATE_SYNCHRONIZATION))
> 			partner->port_state |= AD_STATE_SYNCHRONIZATION;
> 		else
> 			partner->port_state &= ~AD_STATE_SYNCHRONIZATION;
>@@ -529,11 +528,8 @@ static void __record_pdu(struct lacpdu *lacpdu, struct port *port)
> static void __record_default(struct port *port)
> {
> 	if (port) {
>-		// record the partner admin parameters
> 		memcpy(&port->partner_oper, &port->partner_admin,
> 		       sizeof(struct port_params));
>-
>-		// set actor_oper_port_state.defaulted to true
> 		port->actor_oper_port_state |= AD_STATE_DEFAULTED;
> 	}
> }
>@@ -556,14 +552,14 @@ static void __update_selected(struct lacpdu *lacpdu, struct port *port)
> 	if (lacpdu && port) {
> 		const struct port_params *partner = &port->partner_oper;
>
>-		// check if any parameter is different
>+		/* check if any parameter is different */

	I don't see that this comment adds anything; it can go away.

> 		if (ntohs(lacpdu->actor_port) != partner->port_number ||
> 		    ntohs(lacpdu->actor_port_priority) != partner->port_priority ||
>-		    MAC_ADDRESS_COMPARE(&lacpdu->actor_system, &partner->system) ||
>+		    compare_ether_addr((const u8 *)&lacpdu->actor_system, (const u8 *)&partner->system) ||
> 		    ntohs(lacpdu->actor_system_priority) != partner->system_priority ||
> 		    ntohs(lacpdu->actor_key) != partner->key ||
> 		    (lacpdu->actor_state & AD_STATE_AGGREGATION) != (partner->port_state & AD_STATE_AGGREGATION)) {
>-			// update the state machine Selected variable
>+			/* update the state machine Selected variable */
> 			port->sm_vars &= ~AD_PORT_SELECTED;
> 		}
> 	}
>@@ -587,15 +583,15 @@ static void __update_default_selected(struct port *port)
> 		const struct port_params *admin = &port->partner_admin;
> 		const struct port_params *oper = &port->partner_oper;
>
>-		// check if any parameter is different
> 		if (admin->port_number != oper->port_number ||
> 		    admin->port_priority != oper->port_priority ||
>-		    MAC_ADDRESS_COMPARE(&admin->system, &oper->system) ||
>+		    compare_ether_addr((const u8 *)&admin->system,
>+					 (const u8 *)&oper->system) ||
> 		    admin->system_priority != oper->system_priority ||
> 		    admin->key != oper->key ||
> 		    (admin->port_state & AD_STATE_AGGREGATION)
> 			!= (oper->port_state & AD_STATE_AGGREGATION)) {
>-			// update the state machine Selected variable
>+			/* update the state machine Selected variable */
> 			port->sm_vars &= ~AD_PORT_SELECTED;
> 		}
> 	}
>@@ -615,12 +611,12 @@ static void __update_default_selected(struct port *port)
>  */
> static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
> {
>-	// validate lacpdu and port
>+	/* validate lacpdu and port */
> 	if (lacpdu && port) {
>-		// check if any parameter is different
>+		/* check if any parameter is different */

	This one and the previous one ("validate") can both go away.

> 		if ((ntohs(lacpdu->partner_port) != port->actor_port_number) ||
> 		    (ntohs(lacpdu->partner_port_priority) != port->actor_port_priority) ||
>-		    MAC_ADDRESS_COMPARE(&(lacpdu->partner_system), &(port->actor_system)) ||
>+		    compare_ether_addr((const u8 *)&(lacpdu->partner_system), (const u8 *)&(port->actor_system)) ||
> 		    (ntohs(lacpdu->partner_system_priority) != port->actor_system_priority) ||
> 		    (ntohs(lacpdu->partner_key) != port->actor_oper_port_key) ||
> 		    ((lacpdu->partner_state & AD_STATE_LACP_ACTIVITY) != (port->actor_oper_port_state & AD_STATE_LACP_ACTIVITY)) ||
>@@ -628,7 +624,7 @@ static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
> 		    ((lacpdu->partner_state & AD_STATE_SYNCHRONIZATION) != (port->actor_oper_port_state & AD_STATE_SYNCHRONIZATION)) ||
> 		    ((lacpdu->partner_state & AD_STATE_AGGREGATION) != (port->actor_oper_port_state & AD_STATE_AGGREGATION))
> 		   ) {
>-
>+			/* set port ntt */

	Can go away.

> 			port->ntt = true;
> 		}
> 	}
>@@ -644,9 +640,7 @@ static void __update_ntt(struct lacpdu *lacpdu, struct port *port)
>  */
> static void __attach_bond_to_agg(struct port *port)
> {
>-	port = NULL; /* just to satisfy the compiler */
>-	// This function does nothing since the parser/multiplexer of the receive
>-	// and the parser/multiplexer of the aggregator are already combined
>+	port = NULL;
> }
>
> /**
>@@ -659,9 +653,7 @@ static void __attach_bond_to_agg(struct port *port)
>  */
> static void __detach_bond_from_agg(struct port *port)
> {
>-	port = NULL; /* just to satisfy the compiler */
>-	// This function does nothing sience the parser/multiplexer of the receive
>-	// and the parser/multiplexer of the aggregator are already combined
>+	port = NULL;

	These two do-nothing functions should either

	a) be removed entirely, and their call site replaced with a
comment explaining this variation on the standard (that we don't need
the "Attach_Mux_To_Aggregator" or "Detach_Mux_From_Aggregator" functions
specified in the standard for the "Mux machine state diagram"), or

	b) the comment needs to remain in some form to document this
variation on the standard.

> }
>
> /**
>@@ -675,7 +667,9 @@ static int __agg_ports_are_ready(struct aggregator *aggregator)
> 	int retval = 1;
>
> 	if (aggregator) {
>-		// scan all ports in this aggregator to verfy if they are all ready
>+		/* scan all ports in this aggregator
>+		 * to verify if they are all ready.
>+		 */
> 		for (port = aggregator->lag_ports;
> 		     port;
> 		     port = port->next_port_in_aggregator) {
>@@ -715,8 +709,8 @@ static void __set_agg_ports_ready(struct aggregator *aggregator, int val)
>  */
> static u32 __get_agg_bandwidth(struct aggregator *aggregator)
> {
>-	u32 bandwidth = 0;
>-	u32 basic_speed;
>+	u32 uninitialized_var(bandwidth);
>+	u32 uninitialized_var(basic_speed);
>
> 	if (aggregator->num_of_ports) {
> 		basic_speed = __get_link_speed(aggregator->lag_ports);
>@@ -737,7 +731,11 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
> 			bandwidth = aggregator->num_of_ports * 10000;
> 			break;
> 		default:
>-			bandwidth = 0; /*to silence the compiler ....*/
>+			/* Exception: unexpected basic_speed returned */
>+			pr_debug("Unexpected aggregator basic_speed: %d\n",
>+				 basic_speed);
>+			bandwidth = 0;
>+			break;

	This won't apply to current net-next-2.6; the "basic_speed"
variable was removed from this function in net-next-2.6 a few weeks ago.

	In any event, the "uninitialized_var" business shouldn't be
necessary if the function is arranged reasonably, e.g.,

{
	u32 bandwidth;

	if (aggregator->num_of_ports) {
		switch (...) {
		case AD_LINK_SPEED_WHATEVER:
			bandwidth = something;
			break;
		default:
			pr_warn or WARN(1, ...);
			bandwidth = 0;
		}
		return bandwidth;
	}
	return 0;
}


> 		}
> 	}
> 	return bandwidth;
>@@ -809,10 +807,7 @@ static inline void __update_lacpdu_from_port(struct port *port)
> 	 */
> }
>
>-//////////////////////////////////////////////////////////////////////////////////////
>-// ================= main 802.3ad protocol code ======================================
>-//////////////////////////////////////////////////////////////////////////////////////
>-
>+/* ================= main 802.3ad protocol code ================= */
> /**
>  * ad_lacpdu_send - send out a lacpdu packet on a given port
>  * @port: the port we're looking at
>@@ -841,11 +836,12 @@ static int ad_lacpdu_send(struct port *port)
>
> 	memcpy(lacpdu_header->hdr.h_dest, lacpdu_mcast_addr, ETH_ALEN);
> 	/* Note: source address is set to be the member's PERMANENT address,
>-	   because we use it to identify loopback lacpdus in receive. */
>+	 * because we use it to identify loopback lacpdus in receive.
>+	 */
> 	memcpy(lacpdu_header->hdr.h_source, slave->perm_hwaddr, ETH_ALEN);
> 	lacpdu_header->hdr.h_proto = PKT_TYPE_LACPDU;
>
>-	lacpdu_header->lacpdu = port->lacpdu; // struct copy
>+	lacpdu_header->lacpdu = port->lacpdu;

	I personally think "struct copy" comments are useful.

>
> 	dev_queue_xmit(skb);
>
>@@ -886,7 +882,7 @@ static int ad_marker_send(struct port *port, struct bond_marker *marker)
> 	memcpy(marker_header->hdr.h_source, slave->perm_hwaddr, ETH_ALEN);
> 	marker_header->hdr.h_proto = PKT_TYPE_LACPDU;
>
>-	marker_header->marker = *marker; // struct copy
>+	marker_header->marker = *marker;

	Same comment.

> 	dev_queue_xmit(skb);
>
>@@ -902,80 +898,104 @@ static void ad_mux_machine(struct port *port)
> {
> 	mux_states_t last_state;
>
>-	// keep current State Machine state to compare later if it was changed
> 	last_state = port->sm_mux_state;
>
> 	if (port->sm_vars & AD_PORT_BEGIN) {
>-		port->sm_mux_state = AD_MUX_DETACHED;		 // next state
>+		port->sm_mux_state = AD_MUX_DETACHED;
> 	} else {
> 		switch (port->sm_mux_state) {
> 		case AD_MUX_DETACHED:
>-			if ((port->sm_vars & AD_PORT_SELECTED)
>-			    || (port->sm_vars & AD_PORT_STANDBY))
>+			if ((port->sm_vars & AD_PORT_SELECTED) ||
>+			    (port->sm_vars & AD_PORT_STANDBY))
> 				/* if SELECTED or STANDBY */
>-				port->sm_mux_state = AD_MUX_WAITING; // next state
>+				port->sm_mux_state = AD_MUX_WAITING;
> 			break;
> 		case AD_MUX_WAITING:
>-			// if SELECTED == FALSE return to DETACH state
>-			if (!(port->sm_vars & AD_PORT_SELECTED)) { // if UNSELECTED
>+			/* if SELECTED == FALSE return to DETACH state */
>+			if (!(port->sm_vars & AD_PORT_SELECTED)) {
> 				port->sm_vars &= ~AD_PORT_READY_N;
>-				// in order to withhold the Selection Logic to check all ports READY_N value
>-				// every callback cycle to update ready variable, we check READY_N and update READY here
>-				__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
>-				port->sm_mux_state = AD_MUX_DETACHED;	 // next state
>+				/* in order to withhold the Selection Logic
>+				 * to check all ports READY_N value at every
>+				 * callback cycle to update ready variable,
>+				 * we check READY_N and update READY here
>+				 */
>+				__set_agg_ports_ready(port->aggregator,
>+				       __agg_ports_are_ready(port->aggregator));
>+				port->sm_mux_state = AD_MUX_DETACHED;
> 				break;
> 			}
>
>-			// check if the wait_while_timer expired
>-			if (port->sm_mux_timer_counter
>-			    && !(--port->sm_mux_timer_counter))
>+			/* check if the wait_while_timer expired */
>+			if (port->sm_mux_timer_counter &&
>+			    !(--port->sm_mux_timer_counter))
> 				port->sm_vars |= AD_PORT_READY_N;
>
>-			// in order to withhold the selection logic to check all ports READY_N value
>-			// every callback cycle to update ready variable, we check READY_N and update READY here
>-			__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
>-
>-			// if the wait_while_timer expired, and the port is in READY state, move to ATTACHED state
>-			if ((port->sm_vars & AD_PORT_READY)
>-			    && !port->sm_mux_timer_counter)
>-				port->sm_mux_state = AD_MUX_ATTACHED;	 // next state
>+			/* in order to withhold the selection logic
>+			 * to check all ports READY_N value at every
>+			 * callback cycle to update ready variable,
>+			 * we check READY_N and update READY here
>+			 */
>+			__set_agg_ports_ready(port->aggregator,
>+				       __agg_ports_are_ready(port->aggregator));
>+
>+			/* if the wait_while_timer expired,  and the port
>+			 * is in READY state, move to ATTACHED state
>+			 */
>+			if ((port->sm_vars & AD_PORT_READY) &&
>+			    !port->sm_mux_timer_counter)
>+				port->sm_mux_state = AD_MUX_ATTACHED;
> 			break;
> 		case AD_MUX_ATTACHED:
>-			// check also if agg_select_timer expired(so the edable port will take place only after this timer)
>-			if ((port->sm_vars & AD_PORT_SELECTED) && (port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION) && !__check_agg_selection_timer(port)) {
>-				port->sm_mux_state = AD_MUX_COLLECTING_DISTRIBUTING;// next state
>-			} else if (!(port->sm_vars & AD_PORT_SELECTED) || (port->sm_vars & AD_PORT_STANDBY)) {	  // if UNSELECTED or STANDBY
>+			/* check also if agg_select_timer expired
>+			 * so the enable port will take place
>+			 * only after this timer
>+			 */
>+			if ((port->sm_vars & AD_PORT_SELECTED) &&
>+			    (port->partner_oper.port_state &
>+			     AD_STATE_SYNCHRONIZATION) &&
>+			    !__check_agg_selection_timer(port)) {
>+				port->sm_mux_state =
>+					AD_MUX_COLLECTING_DISTRIBUTING;
>+			} else if (!(port->sm_vars & AD_PORT_SELECTED) ||
>+				   (port->sm_vars & AD_PORT_STANDBY)) {
>+				/* if UNSELECTED or STANDBY */
> 				port->sm_vars &= ~AD_PORT_READY_N;
>-				// in order to withhold the selection logic to check all ports READY_N value
>-				// every callback cycle to update ready variable, we check READY_N and update READY here
>-				__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
>-				port->sm_mux_state = AD_MUX_DETACHED;// next state
>+				/* in order to withhold the Selection Logic
>+				 * to check all ports READY_N value at every
>+				 * callback cycle to update ready variable,
>+				 * we check READY_N and update READY here
>+				 */
>+				__set_agg_ports_ready(port->aggregator,
>+				       __agg_ports_are_ready(port->aggregator));
>+				port->sm_mux_state = AD_MUX_DETACHED;
> 			}
> 			break;
> 		case AD_MUX_COLLECTING_DISTRIBUTING:
>-			if (!(port->sm_vars & AD_PORT_SELECTED) || (port->sm_vars & AD_PORT_STANDBY) ||
>-			    !(port->partner_oper.port_state & AD_STATE_SYNCHRONIZATION)
>-			   ) {
>-				port->sm_mux_state = AD_MUX_ATTACHED;// next state
>-
>+			if (!(port->sm_vars & AD_PORT_SELECTED) ||
>+			    (port->sm_vars & AD_PORT_STANDBY) ||
>+			    !(port->partner_oper.port_state &
>+			     AD_STATE_SYNCHRONIZATION)) {
>+				port->sm_mux_state = AD_MUX_ATTACHED;
> 			} else {
>-				// if port state hasn't changed make
>-				// sure that a collecting distributing
>-				// port in an active aggregator is enabled
>+				/* if port state hasn't changed make
>+				 * sure that a collecting distributing
>+				 * port in an active aggregator is enabled
>+				 */
> 				if (port->aggregator &&
> 				    port->aggregator->is_active &&
>-				    !__port_is_enabled(port)) {
>-
>+				    !__port_is_enabled(port))
> 					__enable_port(port);
>-				}
> 			}
> 			break;
>-		default:    //to silence the compiler
>+		default:
>+			/* Exception: unexpected port->sm_mux_state */
>+			pr_debug("Unexpected port->sm_mux_state: %d\n",
>+				 port->sm_mux_state);

	Should be something more potent than pr_debug, like WARN_ONCE.


> 			break;
> 		}
> 	}
>
>-	// check if the state machine was changed
>+	/* check if the state machine was changed */
> 	if (port->sm_mux_state != last_state) {
> 		pr_debug("Mux Machine: Port=%d, Last State=%d, Curr State=%d\n",
> 			 port->actor_port_number, last_state,
>@@ -983,14 +1003,16 @@ static void ad_mux_machine(struct port *port)
> 		switch (port->sm_mux_state) {
> 		case AD_MUX_DETACHED:
> 			__detach_bond_from_agg(port);
>-			port->actor_oper_port_state &= ~AD_STATE_SYNCHRONIZATION;
>+			port->actor_oper_port_state &=
>+					~AD_STATE_SYNCHRONIZATION;
> 			ad_disable_collecting_distributing(port);
> 			port->actor_oper_port_state &= ~AD_STATE_COLLECTING;
> 			port->actor_oper_port_state &= ~AD_STATE_DISTRIBUTING;
> 			port->ntt = true;
> 			break;
> 		case AD_MUX_WAITING:
>-			port->sm_mux_timer_counter = __ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
>+			port->sm_mux_timer_counter =
>+				__ad_timer_to_ticks(AD_WAIT_WHILE_TIMER, 0);
> 			break;
> 		case AD_MUX_ATTACHED:
> 			__attach_bond_to_agg(port);
>@@ -1006,7 +1028,10 @@ static void ad_mux_machine(struct port *port)
> 			ad_enable_collecting_distributing(port);
> 			port->ntt = true;
> 			break;
>-		default:    //to silence the compiler
>+		default:
>+			/* Exception: unexpected port->sm_mux_state */
>+			pr_debug("Unexpected port->sm_mux_state: %d\n",
>+				 port->sm_mux_state);

	As above, WARN_ONCE or the like.

> 			break;
> 		}
> 	}
>@@ -1023,61 +1048,67 @@ static void ad_mux_machine(struct port *port)
>  */
> static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> {
>-	rx_states_t last_state;
>-
>-	// keep current State Machine state to compare later if it was changed
>-	last_state = port->sm_rx_state;
>+	rx_states_t last_state = port->sm_rx_state;
>
>-	// check if state machine should change state
>-	// first, check if port was reinitialized
>+	/* check if state machine should change state
>+	 * first, check if port was reinitialized
>+	 */
> 	if (port->sm_vars & AD_PORT_BEGIN)
> 		/* next state */
> 		port->sm_rx_state = AD_RX_INITIALIZE;
>-	// check if port is not enabled
>-	else if (!(port->sm_vars & AD_PORT_BEGIN)
>-		 && !port->is_enabled && !(port->sm_vars & AD_PORT_MOVED))
>+	/* check if port is not enabled */
>+	else if (!(port->sm_vars & AD_PORT_BEGIN) &&
>+		 !port->is_enabled && !(port->sm_vars & AD_PORT_MOVED))
> 		/* next state */
> 		port->sm_rx_state = AD_RX_PORT_DISABLED;
>-	// check if new lacpdu arrived
>-	else if (lacpdu && ((port->sm_rx_state == AD_RX_EXPIRED) || (port->sm_rx_state == AD_RX_DEFAULTED) || (port->sm_rx_state == AD_RX_CURRENT))) {
>-		port->sm_rx_timer_counter = 0; // zero timer
>+	/* check if new lacpdu arrived */
>+	else if (lacpdu && ((port->sm_rx_state == AD_RX_EXPIRED) ||
>+		 (port->sm_rx_state == AD_RX_DEFAULTED) ||
>+		 (port->sm_rx_state == AD_RX_CURRENT))) {
>+		port->sm_rx_timer_counter = 0; /* zero timer */
> 		port->sm_rx_state = AD_RX_CURRENT;
> 	} else {
>-		// if timer is on, and if it is expired
>-		if (port->sm_rx_timer_counter && !(--port->sm_rx_timer_counter)) {
>+		/* if timer is on, and if it is expired */
>+		if (port->sm_rx_timer_counter &&
>+		    !(--port->sm_rx_timer_counter)) {
> 			switch (port->sm_rx_state) {
> 			case AD_RX_EXPIRED:
>-				port->sm_rx_state = AD_RX_DEFAULTED;		// next state
>+				port->sm_rx_state = AD_RX_DEFAULTED;
> 				break;
> 			case AD_RX_CURRENT:
>-				port->sm_rx_state = AD_RX_EXPIRED;	    // next state
>+				port->sm_rx_state = AD_RX_EXPIRED;
> 				break;
>-			default:    //to silence the compiler
>+			default:
>+				/* Exception: unexpected port->sm_rx_state */
>+				pr_debug("Unexpected port->sm_rx_state: %d\n",
>+					 port->sm_rx_state);

	As above.  I don't think the comment is needed.

> 				break;
> 			}
> 		} else {
>-			// if no lacpdu arrived and no timer is on
>+			/* if no lacpdu arrived and no timer is on */
> 			switch (port->sm_rx_state) {
> 			case AD_RX_PORT_DISABLED:
> 				if (port->sm_vars & AD_PORT_MOVED)
>-					port->sm_rx_state = AD_RX_INITIALIZE;	    // next state
>-				else if (port->is_enabled
>-					 && (port->sm_vars
>-					     & AD_PORT_LACP_ENABLED))
>-					port->sm_rx_state = AD_RX_EXPIRED;	// next state
>-				else if (port->is_enabled
>-					 && ((port->sm_vars
>-					      & AD_PORT_LACP_ENABLED) == 0))
>-					port->sm_rx_state = AD_RX_LACP_DISABLED;    // next state
>+					port->sm_rx_state = AD_RX_INITIALIZE;
>+				else if (port->is_enabled &&
>+					 (port->sm_vars &
>+					     AD_PORT_LACP_ENABLED))
>+					port->sm_rx_state = AD_RX_EXPIRED;
>+				else if (port->is_enabled &&
>+					 ((port->sm_vars &
>+					      AD_PORT_LACP_ENABLED) == 0))
>+					port->sm_rx_state = AD_RX_LACP_DISABLED;
> 				break;
>-			default:    //to silence the compiler
>+			default:
>+				/* Exception: unexpected port->sm_rx_state */
>+				pr_debug("Unexpected port->sm_rx_state: %d\n",
>+					 port->sm_rx_state);

	As above.

> 				break;
>-
> 			}
> 		}
> 	}
>
>-	// check if the State machine was changed or new lacpdu arrived
>+	/* check if the State machine was changed or new lacpdu arrived */
> 	if ((port->sm_rx_state != last_state) || (lacpdu)) {
> 		pr_debug("Rx Machine: Port=%d, Last State=%d, Curr State=%d\n",
> 			 port->actor_port_number, last_state,
>@@ -1092,7 +1123,7 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 			__record_default(port);
> 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
> 			port->sm_vars &= ~AD_PORT_MOVED;
>-			port->sm_rx_state = AD_RX_PORT_DISABLED;	// next state
>+			port->sm_rx_state = AD_RX_PORT_DISABLED;
>
> 			/*- Fall Through -*/
>
>@@ -1107,14 +1138,20 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
> 			break;
> 		case AD_RX_EXPIRED:
>-			//Reset of the Synchronization flag. (Standard 43.4.12)
>-			//This reset cause to disable this port in the COLLECTING_DISTRIBUTING state of the
>-			//mux machine in case of EXPIRED even if LINK_DOWN didn't arrive for the port.
>-			port->partner_oper.port_state &= ~AD_STATE_SYNCHRONIZATION;
>+			/* Reset of the Synchronization flag. (Standard 43.4.12)
>+			 * This reset cause to disable this port in the
>+			 * COLLECTING_DISTRIBUTING state of the mux machine
>+			 * in case of EXPIRED even if LINK_DOWN didn't arrive
>+			 * for the port.
>+			 */
>+			port->partner_oper.port_state &=
>+						~AD_STATE_SYNCHRONIZATION;
> 			port->sm_vars &= ~AD_PORT_MATCHED;
> 			port->partner_oper.port_state |=
> 				AD_STATE_LACP_ACTIVITY;
>-			port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(AD_SHORT_TIMEOUT));
>+			port->sm_rx_timer_counter =
>+				__ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER,
>+						(u16)(AD_SHORT_TIMEOUT));
> 			port->actor_oper_port_state |= AD_STATE_EXPIRED;
> 			break;
> 		case AD_RX_DEFAULTED:
>@@ -1124,12 +1161,13 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
> 			break;
> 		case AD_RX_CURRENT:
>-			// detect loopback situation
>-			if (!MAC_ADDRESS_COMPARE(&(lacpdu->actor_system), &(port->actor_system))) {
>-				// INFO_RECEIVED_LOOPBACK_FRAMES
>+			/* detect loopback situation */
>+			if (!compare_ether_addr((const u8 *)&(lacpdu->actor_system), (const u8 *)&(port->actor_system))) {
>+				/* INFO_RECEIVED_LOOPBACK_FRAMES */
> 				pr_err("%s: An illegal loopback occurred on adapter (%s).\n"
> 				       "Check the configuration to verify that all adapters are connected to 802.3ad compliant switch ports\n",
>-				       port->slave->dev->master->name, port->slave->dev->name);
>+				       port->slave->dev->master->name,
>+				       port->slave->dev->name);
> 				return;
> 			}
> 			__update_selected(lacpdu, port);
>@@ -1137,15 +1175,22 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
> 			__record_pdu(lacpdu, port);
> 			port->sm_rx_timer_counter = __ad_timer_to_ticks(AD_CURRENT_WHILE_TIMER, (u16)(port->actor_oper_port_state & AD_STATE_LACP_TIMEOUT));
> 			port->actor_oper_port_state &= ~AD_STATE_EXPIRED;
>-			// verify that if the aggregator is enabled, the port is enabled too.
>-			//(because if the link goes down for a short time, the 802.3ad will not
>-			// catch it, and the port will continue to be disabled)
>+
>+			/* verify that if the aggregator is enabled,
>+			 * so the port is enabled too.
>+			 * because if the link goes down for a short time,
>+			 * the 802.3ad will not catch it,
>+			 * and the port will continue to be disabled
>+			 */
> 			if (port->aggregator
> 			    && port->aggregator->is_active
> 			    && !__port_is_enabled(port))
> 				__enable_port(port);
> 			break;
>-		default:    //to silence the compiler
>+		default:
>+			/* Exception: unexpected port->sm_rx_state */
>+			pr_debug("Unexpected port->sm_rx_state: %d\n",
>+				 port->sm_rx_state);

	Again, stronger warning, don't need the comment.  There are more
of these; assume I said this about each of them.

> 			break;
> 		}
> 	}
>@@ -1158,9 +1203,11 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
>  */
> static void ad_tx_machine(struct port *port)
> {
>-	// check if tx timer expired, to verify that we do not send more than 3 packets per second
>+	/* check if tx timer has expired,
>+	 * to verify that we do not send more than 3 packets per second
>+	 */
> 	if (port->sm_tx_timer_counter && !(--port->sm_tx_timer_counter)) {
>-		// check if there is something to send
>+		/* check if there is something to send */
> 		if (port->ntt && (port->sm_vars & AD_PORT_LACP_ENABLED)) {
> 			__update_lacpdu_from_port(port);
>
>@@ -1168,14 +1215,17 @@ static void ad_tx_machine(struct port *port)
> 				pr_debug("Sent LACPDU on port %d\n",
> 					 port->actor_port_number);
>
>-				/* mark ntt as false, so it will not be sent again until
>-				   demanded */
>+				/* mark ntt as false,
>+				 * so it won't be sent again until demanded
>+				 */
> 				port->ntt = false;
> 			}
> 		}
>-		// restart tx timer(to verify that we will not exceed AD_MAX_TX_IN_SECOND
>+		/* restart tx timer
>+		 * to verify that we won't exceed AD_MAX_TX_IN_SECOND
>+		 */
> 		port->sm_tx_timer_counter =
>-			ad_ticks_per_sec/AD_MAX_TX_IN_SECOND;
>+				ad_ticks_per_sec/AD_MAX_TX_IN_SECOND;
> 	}
> }
>
>@@ -1189,76 +1239,96 @@ static void ad_periodic_machine(struct port *port)
> {
> 	periodic_states_t last_state;
>
>-	// keep current state machine state to compare later if it was changed
>+	/* keep current state machine state to compare later if it was changed*/

	Could just remove this comment.

	I'm out of time right now to look at this; I'll go through the
rest of it later, or if you respin against current net-next-2.6.

	-J

> 	last_state = port->sm_periodic_state;
>
>-	// check if port was reinitialized
>-	if (((port->sm_vars & AD_PORT_BEGIN) || !(port->sm_vars & AD_PORT_LACP_ENABLED) || !port->is_enabled) ||
>-	    (!(port->actor_oper_port_state & AD_STATE_LACP_ACTIVITY) && !(port->partner_oper.port_state & AD_STATE_LACP_ACTIVITY))
>-	   ) {
>-		port->sm_periodic_state = AD_NO_PERIODIC;	     // next state
>+	/* check if port was reinitialized */
>+	if (((port->sm_vars & AD_PORT_BEGIN) ||
>+	    !(port->sm_vars & AD_PORT_LACP_ENABLED) ||
>+	    !port->is_enabled) ||
>+	    (!(port->actor_oper_port_state & AD_STATE_LACP_ACTIVITY) &&
>+	     !(port->partner_oper.port_state & AD_STATE_LACP_ACTIVITY))) {
>+		port->sm_periodic_state = AD_NO_PERIODIC; /* next state */
> 	}
>-	// check if state machine should change state
>+	/* check if state machine should change state */
> 	else if (port->sm_periodic_timer_counter) {
>-		// check if periodic state machine expired
>+		/* check if periodic state machine expired */
> 		if (!(--port->sm_periodic_timer_counter)) {
>-			// if expired then do tx
>-			port->sm_periodic_state = AD_PERIODIC_TX;    // next state
>+			/* if expired then do tx, next state */
>+			port->sm_periodic_state = AD_PERIODIC_TX;
> 		} else {
>-			// If not expired, check if there is some new timeout parameter from the partner state
>+			/* If not expired, check if there is some
>+			 * new timeout parameter from the partner state
>+			 */
> 			switch (port->sm_periodic_state) {
> 			case AD_FAST_PERIODIC:
> 				if (!(port->partner_oper.port_state
> 				      & AD_STATE_LACP_TIMEOUT))
>-					port->sm_periodic_state = AD_SLOW_PERIODIC;  // next state
>+					port->sm_periodic_state = AD_SLOW_PERIODIC;
> 				break;
> 			case AD_SLOW_PERIODIC:
>-				if ((port->partner_oper.port_state & AD_STATE_LACP_TIMEOUT)) {
>-					// stop current timer
>+				if ((port->partner_oper.port_state &
>+				     AD_STATE_LACP_TIMEOUT)) {
>+					/* stop current timer */
> 					port->sm_periodic_timer_counter = 0;
>-					port->sm_periodic_state = AD_PERIODIC_TX;	 // next state
>+					port->sm_periodic_state = AD_PERIODIC_TX;
> 				}
> 				break;
>-			default:    //to silence the compiler
>+			default:
>+				/* unexpected port->sm_sm_periodic_state */
>+				pr_debug("Unexpected port->sm_periodic_state: %d\n",
>+					 port->sm_periodic_state);
> 				break;
> 			}
> 		}
> 	} else {
> 		switch (port->sm_periodic_state) {
> 		case AD_NO_PERIODIC:
>-			port->sm_periodic_state = AD_FAST_PERIODIC;	 // next state
>+			port->sm_periodic_state = AD_FAST_PERIODIC;
> 			break;
> 		case AD_PERIODIC_TX:
>-			if (!(port->partner_oper.port_state
>-			      & AD_STATE_LACP_TIMEOUT))
>-				port->sm_periodic_state = AD_SLOW_PERIODIC;  // next state
>+			if (!(port->partner_oper.port_state &
>+			      AD_STATE_LACP_TIMEOUT))
>+				port->sm_periodic_state = AD_SLOW_PERIODIC;
> 			else
>-				port->sm_periodic_state = AD_FAST_PERIODIC;  // next state
>+				port->sm_periodic_state = AD_FAST_PERIODIC;
> 			break;
>-		default:    //to silence the compiler
>+		default:
>+			/* unexpected port->sm_sm_periodic_state */
>+			pr_debug("Unexpected port->sm_periodic_state: %d\n",
>+				 port->sm_periodic_state);
> 			break;
> 		}
> 	}
>
>-	// check if the state machine was changed
>+	/* check if the state machine was changed */
> 	if (port->sm_periodic_state != last_state) {
> 		pr_debug("Periodic Machine: Port=%d, Last State=%d, Curr State=%d\n",
> 			 port->actor_port_number, last_state,
> 			 port->sm_periodic_state);
> 		switch (port->sm_periodic_state) {
> 		case AD_NO_PERIODIC:
>-			port->sm_periodic_timer_counter = 0;	   // zero timer
>+			port->sm_periodic_timer_counter = 0;
> 			break;
> 		case AD_FAST_PERIODIC:
>-			port->sm_periodic_timer_counter = __ad_timer_to_ticks(AD_PERIODIC_TIMER, (u16)(AD_FAST_PERIODIC_TIME))-1; // decrement 1 tick we lost in the PERIODIC_TX cycle
>+			/* decrement 1 tick we lost in PERIODIC_TX cycle */
>+			port->sm_periodic_timer_counter =
>+					__ad_timer_to_ticks(AD_PERIODIC_TIMER,
>+						(u16)(AD_FAST_PERIODIC_TIME))-1;
> 			break;
> 		case AD_SLOW_PERIODIC:
>-			port->sm_periodic_timer_counter = __ad_timer_to_ticks(AD_PERIODIC_TIMER, (u16)(AD_SLOW_PERIODIC_TIME))-1; // decrement 1 tick we lost in the PERIODIC_TX cycle
>+			/* decrement 1 tick we lost in PERIODIC_TX cycle */
>+			port->sm_periodic_timer_counter =
>+					__ad_timer_to_ticks(AD_PERIODIC_TIMER,
>+						(u16)(AD_SLOW_PERIODIC_TIME))-1;
> 			break;
> 		case AD_PERIODIC_TX:
> 			port->ntt = true;
> 			break;
>-		default:    //to silence the compiler
>+		default:
>+			/* unexpected port->sm_sm_periodic_state */
>+			pr_debug("Unexpected port->sm_periodic_state: %d\n",
>+				 port->sm_periodic_state);
> 			break;
> 		}
> 	}
>@@ -1274,46 +1344,58 @@ static void ad_periodic_machine(struct port *port)
>  */
> static void ad_port_selection_logic(struct port *port)
> {
>-	struct aggregator *aggregator, *free_aggregator = NULL, *temp_aggregator;
>+	struct aggregator *aggregator, *temp_aggregator;
>+	struct aggregator *free_aggregator = NULL;
> 	struct port *last_port = NULL, *curr_port;
> 	int found = 0;
>
>-	// if the port is already Selected, do nothing
>+	/* if the port is already Selected, do nothing */
> 	if (port->sm_vars & AD_PORT_SELECTED)
> 		return;
>
>-	// if the port is connected to other aggregator, detach it
>+	/* if the port is connected to other aggregator, detach it */
> 	if (port->aggregator) {
>-		// detach the port from its former aggregator
>+		/* detach the port from its former aggregator */
> 		temp_aggregator = port->aggregator;
> 		for (curr_port = temp_aggregator->lag_ports; curr_port;
> 		     last_port = curr_port,
> 			     curr_port = curr_port->next_port_in_aggregator) {
> 			if (curr_port == port) {
> 				temp_aggregator->num_of_ports--;
>-				if (!last_port) {// if it is the first port attached to the aggregator
>+				if (!last_port) {
>+					/* if it is the first port attached
>+					   to the aggregator */
> 					temp_aggregator->lag_ports =
> 						port->next_port_in_aggregator;
>-				} else {// not the first port attached to the aggregator
>+				} else {
>+					/* not the first port attached
>+					   to the aggregator */
> 					last_port->next_port_in_aggregator =
> 						port->next_port_in_aggregator;
> 				}
>
>-				// clear the port's relations to this aggregator
>+				/* clear the port's relations
>+				   to this aggregator */
> 				port->aggregator = NULL;
> 				port->next_port_in_aggregator = NULL;
> 				port->actor_port_aggregator_identifier = 0;
>
> 				pr_debug("Port %d left LAG %d\n",
>-					 port->actor_port_number,
>-					 temp_aggregator->aggregator_identifier);
>-				// if the aggregator is empty, clear its parameters, and set it ready to be attached
>+					port->actor_port_number,
>+					temp_aggregator->aggregator_identifier);
>+				/* if the aggregator is empty,
>+				 * clear its parameters, and set it ready
>+				 * to be attached
>+				 */
> 				if (!temp_aggregator->lag_ports)
> 					ad_clear_agg(temp_aggregator);
> 				break;
> 			}
> 		}
>-		if (!curr_port) { // meaning: the port was related to an aggregator but was not on the aggregator port list
>+		if (!curr_port) {
>+			/* meaning: the port was related to an aggregator
>+			 * but was not on the aggregator port list.
>+			 */
> 			pr_warning("%s: Warning: Port %d (on %s) was related to aggregator %d but was not on its port list\n",
> 				   port->slave->dev->master->name,
> 				   port->actor_port_number,
>@@ -1321,27 +1403,34 @@ static void ad_port_selection_logic(struct port *port)
> 				   port->aggregator->aggregator_identifier);
> 		}
> 	}
>-	// search on all aggregators for a suitable aggregator for this port
>+	/* search on all aggregators for a suitable aggregator for this port */
> 	for (aggregator = __get_first_agg(port); aggregator;
> 	     aggregator = __get_next_agg(aggregator)) {
>
>-		// keep a free aggregator for later use(if needed)
>+		/* keep a free aggregator for later use (if needed) */
> 		if (!aggregator->lag_ports) {
> 			if (!free_aggregator)
> 				free_aggregator = aggregator;
> 			continue;
> 		}
>-		// check if current aggregator suits us
>-		if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) && // if all parameters match AND
>-		     !MAC_ADDRESS_COMPARE(&(aggregator->partner_system), &(port->partner_oper.system)) &&
>+
>+		/* check if current aggregator suits us
>+		 * a suitable aggregator must fit the following requirements,
>+		 * tested here:
>+		 *   i. match all parameters;
>+		 *  ii. has partner answers;
>+		 * iii. it is not individual
>+		 */
>+		if (((aggregator->actor_oper_aggregator_key == port->actor_oper_port_key) &&
>+		     !compare_ether_addr((const u8 *)&(aggregator->partner_system), (const u8 *)&(port->partner_oper.system)) &&
> 		     (aggregator->partner_system_priority == port->partner_oper.system_priority) &&
> 		     (aggregator->partner_oper_aggregator_key == port->partner_oper.key)
> 		    ) &&
>-		    ((MAC_ADDRESS_COMPARE(&(port->partner_oper.system), &(null_mac_addr)) && // partner answers
>-		      !aggregator->is_individual)  // but is not individual OR
>+		    ((compare_ether_addr((const u8 *)&(port->partner_oper.system), (const u8 *)&(null_mac_addr)) &&
>+		      !aggregator->is_individual)
> 		    )
> 		   ) {
>-			// attach to the founded aggregator
>+			/* attach to the founded aggregator */
> 			port->aggregator = aggregator;
> 			port->actor_port_aggregator_identifier =
> 				port->aggregator->aggregator_identifier;
>@@ -1352,42 +1441,45 @@ static void ad_port_selection_logic(struct port *port)
> 				 port->actor_port_number,
> 				 port->aggregator->aggregator_identifier);
>
>-			// mark this port as selected
>+			/* mark this port as selected */
> 			port->sm_vars |= AD_PORT_SELECTED;
> 			found = 1;
> 			break;
> 		}
> 	}
>
>-	// the port couldn't find an aggregator - attach it to a new aggregator
>+	/* the port couldn't find an aggregator, attach it to a new aggregator*/
> 	if (!found) {
> 		if (free_aggregator) {
>-			// assign port a new aggregator
>+			/* assign port a new aggregator */
> 			port->aggregator = free_aggregator;
> 			port->actor_port_aggregator_identifier =
> 				port->aggregator->aggregator_identifier;
>
>-			// update the new aggregator's parameters
>-			// if port was responsed from the end-user
>+			/* update the new aggregator's parameters
>+			   if port was replied from the end-user */
> 			if (port->actor_oper_port_key & AD_DUPLEX_KEY_BITS)
> 				/* if port is full duplex */
> 				port->aggregator->is_individual = false;
> 			else
> 				port->aggregator->is_individual = true;
>
>-			port->aggregator->actor_admin_aggregator_key = port->actor_admin_port_key;
>-			port->aggregator->actor_oper_aggregator_key = port->actor_oper_port_key;
>+			port->aggregator->actor_admin_aggregator_key =
>+						port->actor_admin_port_key;
>+			port->aggregator->actor_oper_aggregator_key =
>+						port->actor_oper_port_key;
> 			port->aggregator->partner_system =
>-				port->partner_oper.system;
>+						port->partner_oper.system;
> 			port->aggregator->partner_system_priority =
>-				port->partner_oper.system_priority;
>-			port->aggregator->partner_oper_aggregator_key = port->partner_oper.key;
>+					port->partner_oper.system_priority;
>+			port->aggregator->partner_oper_aggregator_key =
>+						port->partner_oper.key;
> 			port->aggregator->receive_state = 1;
> 			port->aggregator->transmit_state = 1;
> 			port->aggregator->lag_ports = port;
> 			port->aggregator->num_of_ports++;
>
>-			// mark this port as selected
>+			/* mark this port as selected */
> 			port->sm_vars |= AD_PORT_SELECTED;
>
> 			pr_debug("Port %d joined LAG %d(new LAG)\n",
>@@ -1399,16 +1491,18 @@ static void ad_port_selection_logic(struct port *port)
> 			       port->actor_port_number, port->slave->dev->name);
> 		}
> 	}
>-	// if all aggregator's ports are READY_N == TRUE, set ready=TRUE in all aggregator's ports
>-	// else set ready=FALSE in all aggregator's ports
>-	__set_agg_ports_ready(port->aggregator, __agg_ports_are_ready(port->aggregator));
>+	/* if all aggregator's ports are READY_N == TRUE,
>+	 * set ready=TRUE in all aggregator's ports
>+	 * else set ready=FALSE in all aggregator's ports
>+	 */
>+	__set_agg_ports_ready(port->aggregator,
>+				__agg_ports_are_ready(port->aggregator));
>
> 	aggregator = __get_first_agg(port);
> 	ad_agg_selection_logic(aggregator);
> }
>
>-/*
>- * Decide if "agg" is a better choice for the new active aggregator that
>+/* Decide if "agg" is a better choice for the new active aggregator that
>  * the current best, according to the ad_select policy.
>  */
> static struct aggregator *ad_agg_selection_test(struct aggregator *best,
>@@ -1533,18 +1627,16 @@ static void ad_agg_selection_logic(struct aggregator *agg)
>
> 	if (best &&
> 	    __get_agg_selection_mode(best->lag_ports) == BOND_AD_STABLE) {
>-		/*
>-		 * For the STABLE policy, don't replace the old active
>-		 * aggregator if it's still active (it has an answering
>-		 * partner) or if both the best and active don't have an
>-		 * answering partner.
>+
>+		/* For the STABLE policy, don't replace the old active
>+		 * aggregator if it's still active (it has an answering partner)
>+		 * or if both the best and active don't have answering partners
> 		 */
> 		if (active && active->lag_ports &&
> 		    active->lag_ports->is_enabled &&
>-		    (__agg_has_partner(active) ||
>-		     (!__agg_has_partner(active) && !__agg_has_partner(best)))) {
>-			if (!(!active->actor_oper_aggregator_key &&
>-			      best->actor_oper_aggregator_key)) {
>+		    (__agg_has_partner(active) || !__agg_has_partner(best))) {
>+			if (active->actor_oper_aggregator_key ||
>+			    !best->actor_oper_aggregator_key) {
> 				best = NULL;
> 				active->is_active = 1;
> 			}
>@@ -1556,7 +1648,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> 		active->is_active = 1;
> 	}
>
>-	// if there is new best aggregator, activate it
>+	/* if there is new best aggregator, activate it */
> 	if (best) {
> 		pr_debug("best Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
> 			 best->aggregator_identifier, best->num_of_ports,
>@@ -1577,7 +1669,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> 				 agg->is_individual, agg->is_active);
> 		}
>
>-		// check if any partner replys
>+		/* check if any partner replies */
> 		if (best->is_individual) {
> 			pr_warning("%s: Warning: No 802.3ad response from the link partner for any adapters in the bond\n",
> 				   best->slave ? best->slave->dev->master->name : "NULL");
>@@ -1592,7 +1684,9 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> 			 best->partner_oper_aggregator_key,
> 			 best->is_individual, best->is_active);
>
>-		// disable the ports that were related to the former active_aggregator
>+		/* disable the ports that were related to
>+		 * the former active_aggregator
>+		 */
> 		if (active) {
> 			for (port = active->lag_ports; port;
> 			     port = port->next_port_in_aggregator) {
>@@ -1601,8 +1695,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> 		}
> 	}
>
>-	/*
>-	 * if the selected aggregator is of join individuals
>+	/* if the selected aggregator is of join individuals
> 	 * (partner_system is NULL), enable their ports
> 	 */
> 	active = __get_active_agg(origin);
>@@ -1701,8 +1794,10 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
> 		port->ntt = false;
> 		port->actor_admin_port_key = 1;
> 		port->actor_oper_port_key  = 1;
>-		port->actor_admin_port_state = AD_STATE_AGGREGATION | AD_STATE_LACP_ACTIVITY;
>-		port->actor_oper_port_state  = AD_STATE_AGGREGATION | AD_STATE_LACP_ACTIVITY;
>+		port->actor_admin_port_state =
>+				AD_STATE_AGGREGATION | AD_STATE_LACP_ACTIVITY;
>+		port->actor_oper_port_state  =
>+				AD_STATE_AGGREGATION | AD_STATE_LACP_ACTIVITY;
>
> 		if (lacp_fast)
> 			port->actor_oper_port_state |= AD_STATE_LACP_TIMEOUT;
>@@ -1711,7 +1806,7 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
> 		memcpy(&port->partner_oper, &tmpl, sizeof(tmpl));
>
> 		port->is_enabled = true;
>-		// ****** private parameters ******
>+		/* ****** private parameters ****** */
> 		port->sm_vars = 0x3;
> 		port->sm_rx_state = 0;
> 		port->sm_rx_timer_counter = 0;
>@@ -1753,7 +1848,9 @@ static void ad_enable_collecting_distributing(struct port *port)
>  */
> static void ad_disable_collecting_distributing(struct port *port)
> {
>-	if (port->aggregator && MAC_ADDRESS_COMPARE(&(port->aggregator->partner_system), &(null_mac_addr))) {
>+	if (port->aggregator &&
>+	    compare_ether_addr((const u8 *)&(port->aggregator->partner_system),
>+	     (const u8 *)&(null_mac_addr))) {
> 		pr_debug("Disabling port %d(LAG %d)\n",
> 			 port->actor_port_number,
> 			 port->aggregator->aggregator_identifier);
>@@ -1775,27 +1872,28 @@ static void ad_marker_info_send(struct port *port)
> 	struct bond_marker marker;
> 	u16 index;
>
>-	// fill the marker PDU with the appropriate values
>+	/* fill the marker PDU with the appropriate values */
> 	marker.subtype = 0x02;
> 	marker.version_number = 0x01;
> 	marker.tlv_type = AD_MARKER_INFORMATION_SUBTYPE;
> 	marker.marker_length = 0x16;
>-	// convert requester_port to Big Endian
>-	marker.requester_port = (((port->actor_port_number & 0xFF) << 8) |((u16)(port->actor_port_number & 0xFF00) >> 8));
>+	/* convert requester_port to Big Endian */
>+	marker.requester_port = (((port->actor_port_number & 0xFF) << 8) |
>+				((u16)(port->actor_port_number & 0xFF00) >> 8));
> 	marker.requester_system = port->actor_system;
>-	// convert requester_port(u32) to Big Endian
>+	/* convert requester_port(u32) to Big Endian */
> 	marker.requester_transaction_id =
>-		(((++port->transaction_id & 0xFF) << 24)
>-		 | ((port->transaction_id & 0xFF00) << 8)
>-		 | ((port->transaction_id & 0xFF0000) >> 8)
>-		 | ((port->transaction_id & 0xFF000000) >> 24));
>+		(((++port->transaction_id & 0xFF) << 24) |
>+		  ((port->transaction_id & 0xFF00) << 8) |
>+		  ((port->transaction_id & 0xFF0000) >> 8) |
>+		  ((port->transaction_id & 0xFF000000) >> 24));
> 	marker.pad = 0;
> 	marker.tlv_type_terminator = 0x00;
> 	marker.terminator_length = 0x00;
> 	for (index = 0; index < 90; index++)
> 		marker.reserved_90[index] = 0;
>
>-	// send the marker information
>+	/* send the marker information */
> 	if (ad_marker_send(port, &marker) >= 0) {
> 		pr_debug("Sent Marker Information on port %d\n",
> 			 port->actor_port_number);
>@@ -1814,12 +1912,13 @@ static void ad_marker_info_received(struct bond_marker *marker_info,
> {
> 	struct bond_marker marker;
>
>-	// copy the received marker data to the response marker
>-	//marker = *marker_info;
>+	/* copy the received marker data to the response marker
>+	 * marker = *marker_info;
>+	 */
> 	memcpy(&marker, marker_info, sizeof(struct bond_marker));
>-	// change the marker subtype to marker response
>+	/* change the marker subtype to marker response */
> 	marker.tlv_type = AD_MARKER_RESPONSE_SUBTYPE;
>-	// send the marker response
>+	/* send the marker response */
>
> 	if (ad_marker_send(port, &marker) >= 0) {
> 		pr_debug("Sent Marker Response on port %d\n",
>@@ -1839,16 +1938,13 @@ static void ad_marker_info_received(struct bond_marker *marker_info,
> static void ad_marker_response_received(struct bond_marker *marker,
> 	struct port *port)
> {
>-	marker = NULL; /* just to satisfy the compiler */
>-	port = NULL;  /* just to satisfy the compiler */
>-	// DO NOTHING, SINCE WE DECIDED NOT TO IMPLEMENT THIS FEATURE FOR NOW
>+	marker = NULL;
>+	port = NULL;
> }
>
>-//////////////////////////////////////////////////////////////////////////////////////
>-// ================= AD exported functions to the main bonding code ==================
>-//////////////////////////////////////////////////////////////////////////////////////
>+/* ============= AD exported functions to the main bonding code ============ */
>
>-// Check aggregators status in team every T seconds
>+/* Check aggregators status in team every T seconds */
> #define AD_AGGREGATOR_SELECTION_TIMER  8
>
> /*
>@@ -1876,17 +1972,20 @@ static u16 aggregator_identifier;
>  */
> void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast)
> {
>-	// check that the bond is not initialized yet
>-	if (MAC_ADDRESS_COMPARE(&(BOND_AD_INFO(bond).system.sys_mac_addr),
>+	/* check that the bond is not initialized yet */
>+	if (compare_ether_addr((const u8 *)&(BOND_AD_INFO(bond).system.sys_mac_addr),
> 				bond->dev->dev_addr)) {
>
> 		aggregator_identifier = 0;
>
> 		BOND_AD_INFO(bond).lacp_fast = lacp_fast;
> 		BOND_AD_INFO(bond).system.sys_priority = 0xFFFF;
>-		BOND_AD_INFO(bond).system.sys_mac_addr = *((struct mac_addr *)bond->dev->dev_addr);
>+		BOND_AD_INFO(bond).system.sys_mac_addr =
>+				      *((struct mac_addr *)bond->dev->dev_addr);
>
>-		// initialize how many times this module is called in one second(should be about every 100ms)
>+		/* initialize how many times this module is
>+		 * called in one second(should be about every 100ms)
>+		 */
> 		ad_ticks_per_sec = tick_resolution;
>
> 		bond_3ad_initiate_agg_selection(bond,
>@@ -1914,31 +2013,37 @@ int bond_3ad_bind_slave(struct slave *slave)
> 		return -1;
> 	}
>
>-	//check that the slave has not been initialized yet.
>+	/* check that the slave has not been initialized yet. */
> 	if (SLAVE_AD_INFO(slave).port.slave != slave) {
>
>-		// port initialization
>+		/* port initialization */
> 		port = &(SLAVE_AD_INFO(slave).port);
>
> 		ad_initialize_port(port, BOND_AD_INFO(bond).lacp_fast);
>
> 		port->slave = slave;
> 		port->actor_port_number = SLAVE_AD_INFO(slave).id;
>-		// key is determined according to the link speed, duplex and user key(which is yet not supported)
>-		//              ------------------------------------------------------------
>-		// Port key :   | User key                       |      Speed       |Duplex|
>-		//              ------------------------------------------------------------
>-		//              16                               6               1 0
>-		port->actor_admin_port_key = 0;	// initialize this parameter
>+		/* key is determined according to the link speed,
>+		 * duplex and user key(which is yet not supported)
>+		 * Port key:
>+		 * ------------------------------------------------------------
>+		 * | User key                       |      Speed       |Duplex|
>+		 * ------------------------------------------------------------
>+		 * 16                               6                  1      0
>+		 */
>+		port->actor_admin_port_key = 0;	/* initialize this parameter */
> 		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 the port is not full duplex,
>+		 * then the port should be not lacp Enabled
>+		 */
> 		if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_BITS))
> 			port->sm_vars &= ~AD_PORT_LACP_ENABLED;
>-		// actor system is the bond's system
>+		/* actor system is the bond's system */
> 		port->actor_system = BOND_AD_INFO(bond).system.sys_mac_addr;
>-		// tx timer(to verify that no more than MAX_TX_IN_SECOND lacpdu's are sent in one second)
>+
>+		/* certify that no more than MAX_TX_IN_SECOND lacpdu sent/sec */
> 		port->sm_tx_timer_counter = ad_ticks_per_sec/AD_MAX_TX_IN_SECOND;
> 		port->aggregator = NULL;
> 		port->next_port_in_aggregator = NULL;
>@@ -1947,12 +2052,13 @@ int bond_3ad_bind_slave(struct slave *slave)
> 		__initialize_port_locks(port);
>
>
>-		// aggregator initialization
>+		/* aggregator initialization */
> 		aggregator = &(SLAVE_AD_INFO(slave).aggregator);
>
> 		ad_initialize_agg(aggregator);
>
>-		aggregator->aggregator_mac_address = *((struct mac_addr *)bond->dev->dev_addr);
>+		aggregator->aggregator_mac_address =
>+				      *((struct mac_addr *)bond->dev->dev_addr);
> 		aggregator->aggregator_identifier = (++aggregator_identifier);
> 		aggregator->slave = slave;
> 		aggregator->is_active = 0;
>@@ -1976,13 +2082,13 @@ void bond_3ad_unbind_slave(struct slave *slave)
> 	struct aggregator *aggregator, *new_aggregator, *temp_aggregator;
> 	int select_new_active_agg = 0;
>
>-	// find the aggregator related to this slave
>+	/* find the aggregator related to this slave */
> 	aggregator = &(SLAVE_AD_INFO(slave).aggregator);
>
>-	// find the port related to this slave
>+	/* find the port related to this slave */
> 	port = &(SLAVE_AD_INFO(slave).port);
>
>-	// if slave is null, the whole port is not initialized
>+	/* if slave is null, the whole port is not initialized */
> 	if (!port->slave) {
> 		pr_warning("Warning: %s: Trying to unbind an uninitialized port on %s\n",
> 			   slave->dev->master->name, slave->dev->name);
>@@ -1997,32 +2103,43 @@ void bond_3ad_unbind_slave(struct slave *slave)
> 	__update_lacpdu_from_port(port);
> 	ad_lacpdu_send(port);
>
>-	// check if this aggregator is occupied
>+	/* check if this aggregator is occupied */
> 	if (aggregator->lag_ports) {
>-		// check if there are other ports related to this aggregator except
>-		// the port related to this slave(thats ensure us that there is a
>-		// reason to search for new aggregator, and that we will find one
>-		if ((aggregator->lag_ports != port) || (aggregator->lag_ports->next_port_in_aggregator)) {
>-			// find new aggregator for the related port(s)
>+		/* check if there are other ports related to this aggregator
>+		 * except the port related to this slave
>+		 * (it ensures us that there is a reason to search for
>+		 *  new aggregator, and that we will find one)
>+		 */
>+		if ((aggregator->lag_ports != port) ||
>+		    (aggregator->lag_ports->next_port_in_aggregator)) {
>+			/* find new aggregator for the related port(s) */
> 			new_aggregator = __get_first_agg(port);
>-			for (; new_aggregator; new_aggregator = __get_next_agg(new_aggregator)) {
>-				// if the new aggregator is empty, or it is connected to our port only
>-				if (!new_aggregator->lag_ports
>-				    || ((new_aggregator->lag_ports == port)
>-					&& !new_aggregator->lag_ports->next_port_in_aggregator))
>+			for (; new_aggregator;
>+			     new_aggregator = __get_next_agg(new_aggregator)) {
>+				/* if the new aggregator is empty,
>+				   or it is connected to our port only */
>+				if (!new_aggregator->lag_ports ||
>+				    ((new_aggregator->lag_ports == port) &&
>+				     !new_aggregator->lag_ports->next_port_in_aggregator))
> 					break;
> 			}
>-			// if new aggregator found, copy the aggregator's parameters
>-			// and connect the related lag_ports to the new aggregator
>-			if ((new_aggregator) && ((!new_aggregator->lag_ports) || ((new_aggregator->lag_ports == port) && !new_aggregator->lag_ports->next_port_in_aggregator))) {
>-				pr_debug("Some port(s) related to LAG %d - replaceing with LAG %d\n",
>+			/* if new aggregator found, copy the aggregator's
>+			 * parameters and connect the related lag_ports to the
>+			 * new aggregator
>+			 */
>+			if ((new_aggregator) &&
>+			    ((!new_aggregator->lag_ports) ||
>+			     ((new_aggregator->lag_ports == port) &&
>+			      !new_aggregator->lag_ports->next_port_in_aggregator))) {
>+				pr_debug("Some port(s) related to LAG %d - replacing with LAG %d\n",
> 					 aggregator->aggregator_identifier,
> 					 new_aggregator->aggregator_identifier);
>
>-				if ((new_aggregator->lag_ports == port) && new_aggregator->is_active) {
>+				if ((new_aggregator->lag_ports == port) &&
>+				     new_aggregator->is_active) {
> 					pr_info("%s: Removing an active aggregator\n",
> 						aggregator->slave->dev->master->name);
>-					// select new active aggregator
>+					/* select new active aggregator */
> 					 select_new_active_agg = 1;
> 				}
>
>@@ -2038,14 +2155,15 @@ void bond_3ad_unbind_slave(struct slave *slave)
> 				new_aggregator->is_active = aggregator->is_active;
> 				new_aggregator->num_of_ports = aggregator->num_of_ports;
>
>-				// update the information that is written on the ports about the aggregator
>-				for (temp_port = aggregator->lag_ports; temp_port;
>-				     temp_port = temp_port->next_port_in_aggregator) {
>+				/* update the information that is written on
>+				 * the ports about the aggregator
>+				 */
>+				for (temp_port = aggregator->lag_ports; temp_port; temp_port = temp_port->next_port_in_aggregator) {
> 					temp_port->aggregator = new_aggregator;
> 					temp_port->actor_port_aggregator_identifier = new_aggregator->aggregator_identifier;
> 				}
>
>-				// clear the aggregator
>+				/* clear the aggregator */
> 				ad_clear_agg(aggregator);
>
> 				if (select_new_active_agg)
>@@ -2054,42 +2172,50 @@ void bond_3ad_unbind_slave(struct slave *slave)
> 				pr_warning("%s: Warning: unbinding aggregator, and could not find a new aggregator for its ports\n",
> 					   slave->dev->master->name);
> 			}
>-		} else { // in case that the only port related to this aggregator is the one we want to remove
>+		} else {
>+			 /* in case that the only port related to this
>+			  * aggregator is the one we want to remove
>+			  */
> 			select_new_active_agg = aggregator->is_active;
>-			// clear the aggregator
>+			/* clear the aggregator */
> 			ad_clear_agg(aggregator);
> 			if (select_new_active_agg) {
> 				pr_info("%s: Removing an active aggregator\n",
> 					slave->dev->master->name);
>-				// select new active aggregator
>+				/* select new active aggregator */
> 				ad_agg_selection_logic(__get_first_agg(port));
> 			}
> 		}
> 	}
>
> 	pr_debug("Unbinding port %d\n", port->actor_port_number);
>-	// find the aggregator that this port is connected to
>+	/* find the aggregator that this port is connected to */
> 	temp_aggregator = __get_first_agg(port);
>-	for (; temp_aggregator; temp_aggregator = __get_next_agg(temp_aggregator)) {
>+	for (; temp_aggregator;
>+	     temp_aggregator = __get_next_agg(temp_aggregator)) {
> 		prev_port = NULL;
>-		// search the port in the aggregator's related ports
>+		/* search the port in the aggregator's related ports */
> 		for (temp_port = temp_aggregator->lag_ports; temp_port;
> 		     prev_port = temp_port,
> 			     temp_port = temp_port->next_port_in_aggregator) {
>-			if (temp_port == port) { // the aggregator found - detach the port from this aggregator
>+			if (temp_port == port) {
>+				/* the aggregator found
>+				   detach the port from this aggregator */
> 				if (prev_port)
>-					prev_port->next_port_in_aggregator = temp_port->next_port_in_aggregator;
>+					prev_port->next_port_in_aggregator =
>+					     temp_port->next_port_in_aggregator;
> 				else
>-					temp_aggregator->lag_ports = temp_port->next_port_in_aggregator;
>+					temp_aggregator->lag_ports =
>+					     temp_port->next_port_in_aggregator;
> 				temp_aggregator->num_of_ports--;
> 				if (temp_aggregator->num_of_ports == 0) {
> 					select_new_active_agg = temp_aggregator->is_active;
>-					// clear the aggregator
>+					/* clear the aggregator */
> 					ad_clear_agg(temp_aggregator);
> 					if (select_new_active_agg) {
> 						pr_info("%s: Removing an active aggregator\n",
> 							slave->dev->master->name);
>-						// select new active aggregator
>+						/* select new active aggreg */
> 						ad_agg_selection_logic(__get_first_agg(port));
> 					}
> 				}
>@@ -2125,13 +2251,14 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> 	if (bond->kill_timers)
> 		goto out;
>
>-	//check if there are any slaves
>+	/* check if there are any slaves */
> 	if (bond->slave_cnt == 0)
> 		goto re_arm;
>
>-	// check if agg_select_timer timer after initialize is timed out
>-	if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>-		// select the active aggregator for the bond
>+	/* check if agg_select_timer timer after initialize is timed out */
>+	if (BOND_AD_INFO(bond).agg_select_timer &&
>+	    !(--BOND_AD_INFO(bond).agg_select_timer)) {
>+		/* select the active aggregator for the bond */
> 		if ((port = __get_first_port(bond))) {
> 			if (!port->slave) {
> 				pr_warning("%s: Warning: bond's first port is uninitialized\n",
>@@ -2145,17 +2272,18 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> 		bond_3ad_set_carrier(bond);
> 	}
>
>-	// for each port run the state machines
>-	for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
>+	/* for each port run the state machines */
>+	for (port = __get_first_port(bond); port;
>+	     port = __get_next_port(port)) {
> 		if (!port->slave) {
> 			pr_warning("%s: Warning: Found an uninitialized port\n",
> 				   bond->dev->name);
> 			goto re_arm;
> 		}
>
>-		/* Lock around state machines to protect data accessed
>-		 * by all (e.g., port->sm_vars).  ad_rx_machine may run
>-		 * concurrently due to incoming LACPDU.
>+		/* Lock around state machines to protect data accessed by all
>+		 * (e.g., port->sm_vars).
>+		 * ad_rx_machine may run concurrently due to incoming LACPDU.
> 		 */
> 		__get_state_machine_lock(port);
>
>@@ -2165,7 +2293,7 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
> 		ad_mux_machine(port);
> 		ad_tx_machine(port);
>
>-		// turn off the BEGIN bit, since we already handled it
>+		/* turn off the BEGIN bit, since we already handled it */
> 		if (port->sm_vars & AD_PORT_BEGIN)
> 			port->sm_vars &= ~AD_PORT_BEGIN;
>
>@@ -2198,7 +2326,8 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
>
> 		if (!port->slave) {
> 			pr_warning("%s: Warning: port of slave %s is uninitialized\n",
>-				   slave->dev->name, slave->dev->master->name);
>+				   slave->dev->name,
>+				   slave->dev->master->name);
> 			return;
> 		}
>
>@@ -2213,7 +2342,9 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
> 			break;
>
> 		case AD_TYPE_MARKER:
>-			// No need to convert fields to Little Endian since we don't use the marker's fields.
>+			/* No need to convert fields to Little Endian
>+			 *  since we don't use the marker's fields.
>+			 */
>
> 			switch (((struct bond_marker *)lacpdu)->tlv_type) {
> 			case AD_MARKER_INFORMATION_SUBTYPE:
>@@ -2248,19 +2379,22 @@ void bond_3ad_adapter_speed_changed(struct slave *slave)
>
> 	port = &(SLAVE_AD_INFO(slave).port);
>
>-	// if slave is null, the whole port is not initialized
>+	/* if slave is null, the whole port is not initialized */
> 	if (!port->slave) {
> 		pr_warning("Warning: %s: speed changed for uninitialized port on %s\n",
>-			   slave->dev->master->name, slave->dev->name);
>+			   slave->dev->master->name,
>+			   slave->dev->name);
> 		return;
> 	}
>
> 	port->actor_admin_port_key &= ~AD_SPEED_KEY_BITS;
> 	port->actor_oper_port_key = port->actor_admin_port_key |=
> 		(__get_link_speed(port) << 1);
>+
> 	pr_debug("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
>+	/* there is no need to reselect a new aggregator,
>+	 * just signal the state machines to reinitialize
>+	 */
> 	port->sm_vars |= AD_PORT_BEGIN;
> }
>
>@@ -2276,7 +2410,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
>
> 	port = &(SLAVE_AD_INFO(slave).port);
>
>-	// if slave is null, the whole port is not initialized
>+	/* if slave is null, the whole port is not initialized */
> 	if (!port->slave) {
> 		pr_warning("%s: Warning: duplex changed for uninitialized port on %s\n",
> 			   slave->dev->master->name, slave->dev->name);
>@@ -2286,9 +2420,11 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
> 	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
> 	port->actor_oper_port_key = port->actor_admin_port_key |=
> 		__get_duplex(port);
>+
> 	pr_debug("Port %d changed duplex\n", port->actor_port_number);
>-	// there is no need to reselect a new aggregator, just signal the
>-	// state machines to reinitialize
>+	/* there is no need to reselect a new aggregator,
>+	 * just signal the state machines to reinitialize
>+	 */
> 	port->sm_vars |= AD_PORT_BEGIN;
> }
>
>@@ -2305,15 +2441,18 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
>
> 	port = &(SLAVE_AD_INFO(slave).port);
>
>-	// if slave is null, the whole port is not initialized
>+	/* if slave is null, the whole port is not initialized */
> 	if (!port->slave) {
> 		pr_warning("Warning: %s: link status changed for uninitialized port on %s\n",
> 			   slave->dev->master->name, slave->dev->name);
> 		return;
> 	}
>
>-	// on link down we are zeroing duplex and speed since some of the adaptors(ce1000.lan) report full duplex/speed instead of N/A(duplex) / 0(speed)
>-	// on link up we are forcing recheck on the duplex and speed since some of he adaptors(ce1000.lan) report
>+	/* on link down we are zeroing duplex and speed
>+	 * since some of the adapters (ce1000.lan) report full duplex/speed
>+	 * instead of N/A (duplex) / 0(speed)
>+	 * on link up we are forcing recheck on the duplex and speed
>+	 */
> 	if (link == BOND_LINK_UP) {
> 		port->is_enabled = true;
> 		port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
>@@ -2329,9 +2468,15 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
> 		port->actor_oper_port_key = (port->actor_admin_port_key &=
> 					     ~AD_SPEED_KEY_BITS);
> 	}
>-	//BOND_PRINT_DBG(("Port %d changed link status to %s", 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
>+
>+	/* BOND_PRINT_DBG(("Port %d changed link status to %s",
>+	 *		  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;
> }
>
>@@ -2387,7 +2532,8 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
> 		ad_info->ports = aggregator->num_of_ports;
> 		ad_info->actor_key = aggregator->actor_oper_aggregator_key;
> 		ad_info->partner_key = aggregator->partner_oper_aggregator_key;
>-		memcpy(ad_info->partner_system, aggregator->partner_system.mac_addr_value, ETH_ALEN);
>+		memcpy(ad_info->partner_system,
>+			 aggregator->partner_system.mac_addr_value, ETH_ALEN);
> 		return 0;
> 	}
>
>@@ -2405,9 +2551,7 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> 	struct ad_info ad_info;
> 	int res = 1;
>
>-	/* make sure that the slaves list will
>-	 * not change during tx
>-	 */
>+	/* make sure that the slaves list will not change during tx */
> 	read_lock(&bond->lock);
>
> 	if (!BOND_IS_OK(bond))
>@@ -2442,7 +2586,7 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>
> 	if (slave_agg_no >= 0) {
> 		pr_err("%s: Error: Couldn't find a slave to tx on for aggregator ID %d\n",
>-		       dev->name, agg_id);
>+			dev->name, agg_id);
> 		goto out;
> 	}
>
>diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
>index b28baff..f12e054 100644
>--- a/drivers/net/bonding/bond_3ad.h
>+++ b/drivers/net/bonding/bond_3ad.h
>@@ -28,7 +28,7 @@
> #include <linux/netdevice.h>
> #include <linux/if_ether.h>
>
>-// General definitions
>+/* General definitions */
> #define PKT_TYPE_LACPDU         cpu_to_be16(ETH_P_SLOW)
> #define AD_TIMER_INTERVAL       100 /*msec*/
>
>@@ -47,54 +47,54 @@ enum {
> 	BOND_AD_COUNT = 2,
> };
>
>-// rx machine states(43.4.11 in the 802.3ad standard)
>+/* rx machine states (43.4.11 in the 802.3ad standard) */
> typedef enum {
> 	AD_RX_DUMMY,
>-	AD_RX_INITIALIZE,     // rx Machine
>-	AD_RX_PORT_DISABLED,  // rx Machine
>-	AD_RX_LACP_DISABLED,  // rx Machine
>-	AD_RX_EXPIRED,	      // rx Machine
>-	AD_RX_DEFAULTED,      // rx Machine
>-	AD_RX_CURRENT	      // rx Machine
>+	AD_RX_INITIALIZE,
>+	AD_RX_PORT_DISABLED,
>+	AD_RX_LACP_DISABLED,
>+	AD_RX_EXPIRED,
>+	AD_RX_DEFAULTED,
>+	AD_RX_CURRENT
> } rx_states_t;
>
>-// periodic machine states(43.4.12 in the 802.3ad standard)
>+/* periodic machine states (43.4.12 in the 802.3ad standard) */
> typedef enum {
> 	AD_PERIODIC_DUMMY,
>-	AD_NO_PERIODIC,	       // periodic machine
>-	AD_FAST_PERIODIC,      // periodic machine
>-	AD_SLOW_PERIODIC,      // periodic machine
>-	AD_PERIODIC_TX	   // periodic machine
>+	AD_NO_PERIODIC,
>+	AD_FAST_PERIODIC,
>+	AD_SLOW_PERIODIC,
>+	AD_PERIODIC_TX
> } periodic_states_t;
>
>-// mux machine states(43.4.13 in the 802.3ad standard)
>+/* mux machine states (43.4.13 in the 802.3ad standard) */
> typedef enum {
> 	AD_MUX_DUMMY,
>-	AD_MUX_DETACHED,       // mux machine
>-	AD_MUX_WAITING,	       // mux machine
>-	AD_MUX_ATTACHED,       // mux machine
>-	AD_MUX_COLLECTING_DISTRIBUTING // mux machine
>+	AD_MUX_DETACHED,
>+	AD_MUX_WAITING,
>+	AD_MUX_ATTACHED,
>+	AD_MUX_COLLECTING_DISTRIBUTING
> } mux_states_t;
>
>-// tx machine states(43.4.15 in the 802.3ad standard)
>+/* tx machine states (43.4.15 in the 802.3ad standard) */
> typedef enum {
> 	AD_TX_DUMMY,
>-	AD_TRANSMIT	   // tx Machine
>+	AD_TRANSMIT
> } tx_states_t;
>
>-// rx indication types
>+/* rx indication types */
> typedef enum {
>-	AD_TYPE_LACPDU = 1,    // type lacpdu
>-	AD_TYPE_MARKER	   // type marker
>+	AD_TYPE_LACPDU = 1,
>+	AD_TYPE_MARKER
> } pdu_type_t;
>
>-// rx marker indication types
>+/* rx marker indication types */
> typedef enum {
>-	AD_MARKER_INFORMATION_SUBTYPE = 1, // marker imformation subtype
>-	AD_MARKER_RESPONSE_SUBTYPE     // marker response subtype
>+	AD_MARKER_INFORMATION_SUBTYPE = 1,
>+	AD_MARKER_RESPONSE_SUBTYPE
> } bond_marker_subtype_t;
>
>-// timers types(43.4.9 in the 802.3ad standard)
>+/* timers types (43.4.9 in the 802.3ad standard) */
> typedef enum {
> 	AD_CURRENT_WHILE_TIMER,
> 	AD_ACTOR_CHURN_TIMER,
>@@ -105,35 +105,37 @@ typedef enum {
>
> #pragma pack(1)
>
>-// Link Aggregation Control Protocol(LACP) data unit structure(43.4.2.2 in the 802.3ad standard)
>+/* Link Aggregation Control Protocol (LACP) data unit structure
>+ * (43.4.2.2 in the 802.3ad standard)
>+ */
> typedef struct lacpdu {
>-	u8 subtype;		     // = LACP(= 0x01)
>+	u8 subtype;		          /* = LACP(= 0x01) */
> 	u8 version_number;
>-	u8 tlv_type_actor_info;	      // = actor information(type/length/value)
>-	u8 actor_information_length; // = 20
>+	u8 tlv_type_actor_info;	          /* = actor info(type/length/value)*/
>+	u8 actor_information_length;      /* = 20 */
> 	__be16 actor_system_priority;
> 	struct mac_addr actor_system;
> 	__be16 actor_key;
> 	__be16 actor_port_priority;
> 	__be16 actor_port;
> 	u8 actor_state;
>-	u8 reserved_3_1[3];	     // = 0
>-	u8 tlv_type_partner_info;     // = partner information
>-	u8 partner_information_length;	 // = 20
>+	u8 reserved_3_1[3];	          /* = 0 */
>+	u8 tlv_type_partner_info;         /* = partner information */
>+	u8 partner_information_length;	  /* = 20 */
> 	__be16 partner_system_priority;
> 	struct mac_addr partner_system;
> 	__be16 partner_key;
> 	__be16 partner_port_priority;
> 	__be16 partner_port;
> 	u8 partner_state;
>-	u8 reserved_3_2[3];	     // = 0
>-	u8 tlv_type_collector_info;	  // = collector information
>-	u8 collector_information_length; // = 16
>+	u8 reserved_3_2[3];	          /* = 0 */
>+	u8 tlv_type_collector_info;	  /* = collector information */
>+	u8 collector_information_length;  /* = 16 */
> 	__be16 collector_max_delay;
> 	u8 reserved_12[12];
>-	u8 tlv_type_terminator;	     // = terminator
>-	u8 terminator_length;	     // = 0
>-	u8 reserved_50[50];	     // = 0
>+	u8 tlv_type_terminator;	          /* = terminator */
>+	u8 terminator_length;	          /* = 0 */
>+	u8 reserved_50[50];	          /* = 0 */
> } lacpdu_t;
>
> typedef struct lacpdu_header {
>@@ -141,20 +143,22 @@ typedef struct lacpdu_header {
> 	struct lacpdu lacpdu;
> } lacpdu_header_t;
>
>-// Marker Protocol Data Unit(PDU) structure(43.5.3.2 in the 802.3ad standard)
>+/* Marker Protocol Data Unit(PDU) structure
>+ * (43.5.3.2 in the 802.3ad standard)
>+ */
> typedef struct bond_marker {
>-	u8 subtype;		 //  = 0x02  (marker PDU)
>-	u8 version_number;	 //  = 0x01
>-	u8 tlv_type;		 //  = 0x01  (marker information)
>-	//  = 0x02  (marker response information)
>-	u8 marker_length;	 //  = 0x16
>-	u16 requester_port;	 //   The number assigned to the port by the requester
>-	struct mac_addr requester_system;      //   The requester's system id
>-	u32 requester_transaction_id;	//   The transaction id allocated by the requester,
>-	u16 pad;		 //  = 0
>-	u8 tlv_type_terminator;	     //  = 0x00
>-	u8 terminator_length;	     //  = 0x00
>-	u8 reserved_90[90];	     //  = 0
>+	u8 subtype;			  /* = 0x02 (marker PDU) */
>+	u8 version_number;		  /* = 0x01 */
>+	u8 tlv_type;			  /* = 0x01 (marker information)
>+					   * = 0x02  (marker response info */
>+	u8 marker_length;		  /* = 0x16 */
>+	u16 requester_port;
>+	struct mac_addr requester_system; /* The requester's system id */
>+	u32 requester_transaction_id;
>+	u16 pad;			  /* = 0 */
>+	u8 tlv_type_terminator;		  /* = 0x00 */
>+	u8 terminator_length;		  /* = 0x00 */
>+	u8 reserved_90[90];		  /* = 0 */
> } bond_marker_t;
>
> typedef struct bond_marker_header {
>@@ -173,7 +177,7 @@ struct port;
> #pragma pack(8)
> #endif
>
>-// aggregator structure(43.4.5 in the 802.3ad standard)
>+/* aggregator structure (43.4.5 in the 802.3ad standard) */
> typedef struct aggregator {
> 	struct mac_addr aggregator_mac_address;
> 	u16 aggregator_identifier;
>@@ -183,12 +187,13 @@ typedef struct aggregator {
> 	struct mac_addr partner_system;
> 	u16 partner_system_priority;
> 	u16 partner_oper_aggregator_key;
>-	u16 receive_state;		// BOOLEAN
>-	u16 transmit_state;		// BOOLEAN
>+	u16 receive_state;		/* BOOLEAN */
>+	u16 transmit_state;		/* BOOLEAN */
> 	struct port *lag_ports;
>-	// ****** PRIVATE PARAMETERS ******
>-	struct slave *slave;	    // pointer to the bond slave that this aggregator belongs to
>-	u16 is_active;	    // BOOLEAN. Indicates if this aggregator is active
>+	/* ****** PRIVATE PARAMETERS ****** */
>+	struct slave *slave; /* pointer to the bond slave
>+				that this aggregator belongs to */
>+	u16 is_active;	     /* BOOLEAN. Indicates if the aggregator is active*/
> 	u16 num_of_ports;
> } aggregator_t;
>
>@@ -201,12 +206,18 @@ struct port_params {
> 	u16 port_state;
> };
>
>-// port structure(43.4.6 in the 802.3ad standard)
>+/* port structure (43.4.6 in the 802.3ad standard) */
> typedef struct port {
> 	u16 actor_port_number;
> 	u16 actor_port_priority;
>-	struct mac_addr actor_system;	       // This parameter is added here although it is not specified in the standard, just for simplification
>-	u16 actor_system_priority;	 // This parameter is added here although it is not specified in the standard, just for simplification
>+
>+	/* in a attempt to simplify operations the
>+	 * following two elements were included here
>+	 * despite they are not specified in the standard
>+	 */
>+	struct mac_addr actor_system;
>+	u16 actor_system_priority;
>+
> 	u16 actor_port_aggregator_identifier;
> 	bool ntt;
> 	u16 actor_admin_port_key;
>@@ -219,21 +230,21 @@ typedef struct port {
>
> 	bool is_enabled;
>
>-	// ****** PRIVATE PARAMETERS ******
>-	u16 sm_vars;	      // all state machines variables for this port
>-	rx_states_t sm_rx_state;	// state machine rx state
>-	u16 sm_rx_timer_counter;    // state machine rx timer counter
>-	periodic_states_t sm_periodic_state;// state machine periodic state
>-	u16 sm_periodic_timer_counter;	// state machine periodic timer counter
>-	mux_states_t sm_mux_state;	// state machine mux state
>-	u16 sm_mux_timer_counter;   // state machine mux timer counter
>-	tx_states_t sm_tx_state;	// state machine tx state
>-	u16 sm_tx_timer_counter;    // state machine tx timer counter(allways on - enter to transmit state 3 time per second)
>-	struct slave *slave;	    // pointer to the bond slave that this port belongs to
>-	struct aggregator *aggregator;	   // pointer to an aggregator that this port related to
>-	struct port *next_port_in_aggregator; // Next port on the linked list of the parent aggregator
>-	u32 transaction_id;	    // continuous number for identification of Marker PDU's;
>-	struct lacpdu lacpdu;	       // the lacpdu that will be sent for this port
>+	/* ****** PRIVATE PARAMETERS ****** */
>+	u16 sm_vars;
>+	rx_states_t sm_rx_state;
>+	u16 sm_rx_timer_counter;
>+	periodic_states_t sm_periodic_state;
>+	u16 sm_periodic_timer_counter;
>+	mux_states_t sm_mux_state;
>+	u16 sm_mux_timer_counter;
>+	tx_states_t sm_tx_state;
>+	u16 sm_tx_timer_counter;
>+	struct slave *slave;
>+	struct aggregator *aggregator;
>+	struct port *next_port_in_aggregator;
>+	u32 transaction_id;
>+	struct lacpdu lacpdu;
> } port_t;
>
> // system structure
>@@ -246,41 +257,41 @@ struct ad_system {
> #pragma pack()
> #endif
>
>-// ================= AD Exported structures to the main bonding code ==================
>+/* =========== AD Exported structures to the main bonding code ============ */
> #define BOND_AD_INFO(bond)   ((bond)->ad_info)
> #define SLAVE_AD_INFO(slave) ((slave)->ad_info)
>
> struct ad_bond_info {
> 	struct ad_system system;	    /* 802.3ad system structure */
>-	u32 agg_select_timer;	    // Timer to select aggregator after all adapter's hand shakes
>-	u32 agg_select_mode;	    // Mode of selection of active aggregator(bandwidth/count)
>-	int lacp_fast;		/* whether fast periodic tx should be
>-				 * requested
>-				 */
>+	u32 agg_select_timer;	            /* aggregator's selected timer */
>+	u32 agg_select_mode;	            /* aggregator's selected mode */
>+	int lacp_fast;
> 	struct timer_list ad_timer;
> 	struct packet_type ad_pkt_type;
> };
>
> struct ad_slave_info {
>-	struct aggregator aggregator;	    // 802.3ad aggregator structure
>-	struct port port;		    // 802.3ad port structure
>-	spinlock_t state_machine_lock; /* mutex state machines vs.
>-					  incoming LACPDU */
>+	struct aggregator aggregator;	    /* 802.3ad aggregator structure */
>+	struct port port;		    /* 802.3ad port structure */
>+	spinlock_t state_machine_lock;	    /* mutex state machines vs.
>+					     * incoming LACPDU */
> 	u16 id;
> };
>
>-// ================= AD Exported functions to the main bonding code ==================
>-void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution, int lacp_fast);
>-int  bond_3ad_bind_slave(struct slave *slave);
>+/* ========= AD Exported functions to the main bonding code ========== */
>+void bond_3ad_initialize(struct bonding *bond, u16 tick_resolution,
>+			 int lacp_fast);
>+int 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_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, struct ad_info *ad_info);
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev);
>-int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype, struct net_device *orig_dev);
>+int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev,
>+			struct packet_type* ptype, struct net_device *orig_dev);
> int bond_3ad_set_carrier(struct bonding *bond);
> #endif //__BOND_3AD_H__
>
>-- 
>1.7.4.4
>
>
>-- 
>Rafael Aquini <aquini@...ux.com>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists