Re: [PATCH] net/bonding: bonding: Adjust coding style for bond_3ad files.

[Joe Perches <joe@perches.com>]

On Wed, 2011-05-04 at 19:18 -0300, Rafael Azenha Aquini wrote:
> I did some mods there, in an attempt to make that code stick as
> closely as possible with the Kernel coding style.
> Signed-off-by: Rafael Aquini <aquini@linux.com>
>  drivers/net/bonding/bond_3ad.c |  918 +++++++++++++++++++++++-----------------
>  drivers/net/bonding/bond_3ad.h |  189 +++++----
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 31912f1..97e7528 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
[]
> @@ -59,7 +59,10 @@
>  #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)
> + */

If you're going to reflow comments, please try to keep
operational phrases on the same line.  Something like

/*
 * Port Variables definitions used by the State Machines
 * (43.4.7 in the 802.3ad standard)
 */

btw: David Miller, the drivers/net maintainer prefers
comments to start on the same opening line as the comment
initiator like this:

/* Port Variables definitions used by the State Machines
 * (43.4.7 in the 802.3ad standard)
 */

[]

> -// compare MAC addresses
>  #define MAC_ADDRESS_COMPARE(A, B) memcmp(A, B, ETH_ALEN)

You could convert all uses of this macro to compare_ether_addr
and delete this macro.

>  static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };

const

> @@ -113,14 +115,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);

Better to use either of these forms.

Args aligned to open parenthesis:

static void ad_marker_info_received(struct bond_marker *marker_info,
				    struct port *port);

Return type on separate line:

static void
ad_marker_info_received(struct bond_marker *marker_info, struct port *port);

In any case, please align all additional args to the open parenthesis.

> @@ -161,7 +161,6 @@ 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

I think these comments are non-obvious enough to leave.

> @@ -365,22 +365,26 @@ static u8 __get_duplex(struct port *port)
>  
>  	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.
> +	 */

	/* 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 {
>  		switch (slave->duplex) {
>  		case DUPLEX_FULL:
>  			retval = 0x1;
> -			pr_debug("Port %d Received status full duplex update from adapter\n",
> -				 port->actor_port_number);
> +			pr_debug("Port %d Received status "
> +					"full duplex update from adapter\n",
> +					port->actor_port_number);

Don't reflow format strings.
These aren't errors and are preferred for easier grep.

> @@ -394,12 +398,9 @@ static u8 __get_duplex(struct port *port)
>   */
>  static inline void __initialize_port_locks(struct port *port)
>  {
> -	// make sure it isn't called twice
>  	spin_lock_init(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
>  }

Lost comment?

[]

> @@ -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
>  	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)) &&
> -	     (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)
> -	    ((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)
> -		) {
> -		// update the state machine Matched variable
> +	     (ntohs(lacpdu->partner_port_priority) == port->actor_port_priority)
> +	    && !MAC_ADDRESS_COMPARE(&(lacpdu->partner_system),
> +							&(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)))
> +	    || ((lacpdu->actor_state & AD_STATE_AGGREGATION) == 0)) {
> +		/* update the state machine Selected variable */

Not an improvement.

Logical continuations are put at the EOL.
If these are longer than 80 char, I'd ignore them.

> -		// set the partner sync. to on if the partner is sync. and the port is matched
> +		/* switch on partner sync. if partner is synchronized,
> +		 * and the port is matched
> +		 */

I think the original text better.

> @@ -675,7 +673,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 verfy
> +		 * if they are all ready
> +		 */

verify.

For the rest, too long, didn't read...