[PATCH net-next v1] bonding: cosmetic/readability changes for admin, oper-key ops

[PATCH net-next v1] bonding: cosmetic/readability changes for admin, oper-key ops

[Mahesh Bandewar]

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/bonding/bond_3ad.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b31673a476d9..4eeb02210fed 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1423,8 +1423,10 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
 			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->aggregator->partner_system_priority =
@@ -1756,8 +1758,6 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
 		port->actor_system_priority = 0xffff;
 		port->actor_port_aggregator_identifier = 0;
 		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 |
@@ -2348,8 +2348,8 @@ void bond_3ad_adapter_speed_changed(struct slave *slave)
 	spin_lock_bh(&slave->bond->mode_lock);
 
 	port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
-	port->actor_oper_port_key = port->actor_admin_port_key |=
-		(__get_link_speed(port) << 1);
+	port->actor_admin_port_key |= __get_link_speed(port) << 1;
+	port->actor_oper_port_key = port->actor_admin_port_key;
 	netdev_dbg(slave->bond->dev, "Port %d changed speed\n", port->actor_port_number);
 	/* there is no need to reselect a new aggregator, just signal the
 	 * state machines to reinitialize
@@ -2381,8 +2381,8 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
 	spin_lock_bh(&slave->bond->mode_lock);
 
 	port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
-	port->actor_oper_port_key = port->actor_admin_port_key |=
-		__get_duplex(port);
+	port->actor_admin_port_key |= __get_duplex(port);
+	port->actor_oper_port_key = port->actor_admin_port_key;
 	netdev_dbg(slave->bond->dev, "Port %d slave %s changed duplex\n",
 		   port->actor_port_number, slave->dev->name);
 	if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
@@ -2423,24 +2423,19 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
 	 * on link up we are forcing recheck on the duplex and speed since
 	 * some of he adaptors(ce1000.lan) report.
 	 */
+	port->actor_admin_port_key &= ~(AD_DUPLEX_KEY_MASKS|AD_SPEED_KEY_MASKS);
 	if (link == BOND_LINK_UP) {
 		port->is_enabled = true;
-		port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
-		port->actor_oper_port_key = port->actor_admin_port_key |=
-			__get_duplex(port);
-		port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
-		port->actor_oper_port_key = port->actor_admin_port_key |=
-			(__get_link_speed(port) << 1);
-		if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
+		port->actor_admin_port_key |=
+			(__get_link_speed(port) << 1) | __get_duplex(port);
+		if (port->actor_admin_port_key & AD_DUPLEX_KEY_MASKS)
 			port->sm_vars |= AD_PORT_LACP_ENABLED;
 	} else {
 		/* link has failed */
 		port->is_enabled = false;
-		port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
-		port->actor_oper_port_key = (port->actor_admin_port_key &=
-					     ~AD_SPEED_KEY_MASKS);
 		port->sm_vars &= ~AD_PORT_LACP_ENABLED;
 	}
+	port->actor_oper_port_key = port->actor_admin_port_key;
 	netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n",
 		   port->actor_port_number,
 		   link == BOND_LINK_UP ? "UP" : "DOWN");
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net-next v1] bonding: cosmetic/readability changes for admin, oper-key ops

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Fri,  3 Apr 2015 13:54:00 -0700

> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

This is a very low quality patch submission.

You aren't explaining what you are really doing, or why.

Furthermore:

> @@ -1756,8 +1758,6 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
>  		port->actor_system_priority = 0xffff;
>  		port->actor_port_aggregator_identifier = 0;
>  		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 |

This is not just a readability or cosmetic change, it is changing behavior.

You need to describe why removing these initializers is valid, and also
this change is outside of the scope of a cosmetic/readability change so
would need to be split into another patch.

It is almost never correct to have an empty commit log message, espcially
when real changes are being made to the code as is happening here in this
code block.

Re: [PATCH net-next v1] bonding: cosmetic/readability changes for admin, oper-key ops

[Mahesh Bandewar]

On Mon, Apr 6, 2015 at 2:01 PM, David Miller <davem@davemloft.net> wrote:
> From: Mahesh Bandewar <maheshb@google.com>
> Date: Fri,  3 Apr 2015 13:54:00 -0700
>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>
> This is a very low quality patch submission.
>
> You aren't explaining what you are really doing, or why.
>
> Furthermore:
>
>> @@ -1756,8 +1758,6 @@ static void ad_initialize_port(struct port *port, int lacp_fast)
>>               port->actor_system_priority = 0xffff;
>>               port->actor_port_aggregator_identifier = 0;
>>               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 |
>
> This is not just a readability or cosmetic change, it is changing behavior.
>
> You need to describe why removing these initializers is valid, and also
> this change is outside of the scope of a cosmetic/readability change so
> would need to be split into another patch.
>
> It is almost never correct to have an empty commit log message, espcially
> when real changes are being made to the code as is happening here in this
> code block.

These are bogus initializers but I agree that this piece deserves a
separate patch with an explanation. Rest of the patch is nothing but
code-re-factoring (breaking rich assignment to simpler, multiple
assignments, removing duplicate code etc.) to make it readable. I
though it wont be necessary to add that into the commit log since it's
very obvious hence I kept it empty.  Will break it into two patches
and send it again.

Thanks,
--mahesh..