Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1352160 > unrolled thread

[PATCH net-next] net: dsa: mv88e6xxx: rework port state setter

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next] net: dsa: mv88e6xxx: rework port state setter Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2016-03-08 00:30 +0100
    Re: [PATCH net-next] net: dsa: mv88e6xxx: rework port state setter Andrew Lunn <andrew@lunn.ch> - 2016-03-08 15:40 +0100
    Re: [PATCH net-next] net: dsa: mv88e6xxx: rework port state setter David Miller <davem@davemloft.net> - 2016-03-10 22:20 +0100

#1352160 — [PATCH net-next] net: dsa: mv88e6xxx: rework port state setter

Apply a few non-functional changes on the port state setter:

  * add a dynamic debug message with state names to track changes
  * explicit states checking instead of assuming their numeric values
  * lock mutex only once when changing several port states
  * use bitmap macros to declare and access port_state_update_mask

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 54 +++++++++++++++++++++++++++------------------
 drivers/net/dsa/mv88e6xxx.h |  2 +-
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index d11c9d5..3a58a8a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1051,39 +1051,49 @@ static int _mv88e6xxx_atu_remove(struct dsa_switch *ds, u16 fid, int port,
 	return _mv88e6xxx_atu_move(ds, fid, port, 0x0f, static_too);
 }
 
-static int mv88e6xxx_set_port_state(struct dsa_switch *ds, int port, u8 state)
+static const char * const mv88e6xxx_port_state_names[] = {
+	[PORT_CONTROL_STATE_DISABLED] = "Disabled",
+	[PORT_CONTROL_STATE_BLOCKING] = "Blocking/Listening",
+	[PORT_CONTROL_STATE_LEARNING] = "Learning",
+	[PORT_CONTROL_STATE_FORWARDING] = "Forwarding",
+};
+
+static int _mv88e6xxx_port_state(struct dsa_switch *ds, int port, u8 state)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int reg, ret = 0;
 	u8 oldstate;
 
-	mutex_lock(&ps->smi_mutex);
-
 	reg = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_CONTROL);
-	if (reg < 0) {
-		ret = reg;
-		goto abort;
-	}
+	if (reg < 0)
+		return reg;
 
 	oldstate = reg & PORT_CONTROL_STATE_MASK;
+
 	if (oldstate != state) {
 		/* Flush forwarding database if we're moving a port
 		 * from Learning or Forwarding state to Disabled or
 		 * Blocking or Listening state.
 		 */
-		if (oldstate >= PORT_CONTROL_STATE_LEARNING &&
-		    state <= PORT_CONTROL_STATE_BLOCKING) {
+		if ((oldstate == PORT_CONTROL_STATE_LEARNING ||
+		     oldstate == PORT_CONTROL_STATE_FORWARDING)
+		    && (state == PORT_CONTROL_STATE_DISABLED ||
+			state == PORT_CONTROL_STATE_BLOCKING)) {
 			ret = _mv88e6xxx_atu_remove(ds, 0, port, false);
 			if (ret)
-				goto abort;
+				return ret;
 		}
+
 		reg = (reg & ~PORT_CONTROL_STATE_MASK) | state;
 		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_CONTROL,
 					   reg);
+		if (ret)
+			return ret;
+
+		netdev_dbg(ds->ports[port], "PortState %s (was %s)\n",
+			   mv88e6xxx_port_state_names[state],
+			   mv88e6xxx_port_state_names[oldstate]);
 	}
 
-abort:
-	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
 
@@ -1146,13 +1156,11 @@ int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
 		break;
 	}
 
-	netdev_dbg(ds->ports[port], "port state %d [%d]\n", state, stp_state);
-
 	/* mv88e6xxx_port_stp_update may be called with softirqs disabled,
 	 * so we can not update the port state directly but need to schedule it.
 	 */
 	ps->ports[port].state = stp_state;
-	set_bit(port, &ps->port_state_update_mask);
+	set_bit(port, ps->port_state_update_mask);
 	schedule_work(&ps->bridge_work);
 
 	return 0;
@@ -2228,11 +2236,15 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	ps = container_of(work, struct mv88e6xxx_priv_state, bridge_work);
 	ds = ((struct dsa_switch *)ps) - 1;
 
-	while (ps->port_state_update_mask) {
-		port = __ffs(ps->port_state_update_mask);
-		clear_bit(port, &ps->port_state_update_mask);
-		mv88e6xxx_set_port_state(ds, port, ps->ports[port].state);
-	}
+	mutex_lock(&ps->smi_mutex);
+
+	for (port = 0; port < ps->num_ports; ++port)
+		if (test_and_clear_bit(port, ps->port_state_update_mask) &&
+		    _mv88e6xxx_port_state(ds, port, ps->ports[port].state))
+			netdev_warn(ds->ports[port], "failed to update state to %s\n",
+				    mv88e6xxx_port_state_names[ps->ports[port].state]);
+
+	mutex_unlock(&ps->smi_mutex);
 }
 
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index d7b088d..3425616 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -426,7 +426,7 @@ struct mv88e6xxx_priv_state {
 
 	struct mv88e6xxx_priv_port	ports[DSA_MAX_PORTS];
 
-	unsigned long port_state_update_mask;
+	DECLARE_BITMAP(port_state_update_mask, DSA_MAX_PORTS);
 
 	struct work_struct bridge_work;
 };
-- 
2.7.2

[toc] | [next] | [standalone]

#1353105

On Mon, Mar 07, 2016 at 06:24:17PM -0500, Vivien Didelot wrote:
> Apply a few non-functional changes on the port state setter:
> 
>   * add a dynamic debug message with state names to track changes
>   * explicit states checking instead of assuming their numeric values
>   * lock mutex only once when changing several port states
>   * use bitmap macros to declare and access port_state_update_mask
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Tested-by: Andrew Lunn <andrew@lunn.ch>

Thanks
	Andrew

> ---
>  drivers/net/dsa/mv88e6xxx.c | 54 +++++++++++++++++++++++++++------------------
>  drivers/net/dsa/mv88e6xxx.h |  2 +-
>  2 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index d11c9d5..3a58a8a 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1051,39 +1051,49 @@ static int _mv88e6xxx_atu_remove(struct dsa_switch *ds, u16 fid, int port,
>  	return _mv88e6xxx_atu_move(ds, fid, port, 0x0f, static_too);
>  }
>  
> -static int mv88e6xxx_set_port_state(struct dsa_switch *ds, int port, u8 state)
> +static const char * const mv88e6xxx_port_state_names[] = {
> +	[PORT_CONTROL_STATE_DISABLED] = "Disabled",
> +	[PORT_CONTROL_STATE_BLOCKING] = "Blocking/Listening",
> +	[PORT_CONTROL_STATE_LEARNING] = "Learning",
> +	[PORT_CONTROL_STATE_FORWARDING] = "Forwarding",
> +};
> +
> +static int _mv88e6xxx_port_state(struct dsa_switch *ds, int port, u8 state)
>  {
> -	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
>  	int reg, ret = 0;
>  	u8 oldstate;
>  
> -	mutex_lock(&ps->smi_mutex);
> -
>  	reg = _mv88e6xxx_reg_read(ds, REG_PORT(port), PORT_CONTROL);
> -	if (reg < 0) {
> -		ret = reg;
> -		goto abort;
> -	}
> +	if (reg < 0)
> +		return reg;
>  
>  	oldstate = reg & PORT_CONTROL_STATE_MASK;
> +
>  	if (oldstate != state) {
>  		/* Flush forwarding database if we're moving a port
>  		 * from Learning or Forwarding state to Disabled or
>  		 * Blocking or Listening state.
>  		 */
> -		if (oldstate >= PORT_CONTROL_STATE_LEARNING &&
> -		    state <= PORT_CONTROL_STATE_BLOCKING) {
> +		if ((oldstate == PORT_CONTROL_STATE_LEARNING ||
> +		     oldstate == PORT_CONTROL_STATE_FORWARDING)
> +		    && (state == PORT_CONTROL_STATE_DISABLED ||
> +			state == PORT_CONTROL_STATE_BLOCKING)) {
>  			ret = _mv88e6xxx_atu_remove(ds, 0, port, false);
>  			if (ret)
> -				goto abort;
> +				return ret;
>  		}
> +
>  		reg = (reg & ~PORT_CONTROL_STATE_MASK) | state;
>  		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_CONTROL,
>  					   reg);
> +		if (ret)
> +			return ret;
> +
> +		netdev_dbg(ds->ports[port], "PortState %s (was %s)\n",
> +			   mv88e6xxx_port_state_names[state],
> +			   mv88e6xxx_port_state_names[oldstate]);
>  	}
>  
> -abort:
> -	mutex_unlock(&ps->smi_mutex);
>  	return ret;
>  }
>  
> @@ -1146,13 +1156,11 @@ int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
>  		break;
>  	}
>  
> -	netdev_dbg(ds->ports[port], "port state %d [%d]\n", state, stp_state);
> -
>  	/* mv88e6xxx_port_stp_update may be called with softirqs disabled,
>  	 * so we can not update the port state directly but need to schedule it.
>  	 */
>  	ps->ports[port].state = stp_state;
> -	set_bit(port, &ps->port_state_update_mask);
> +	set_bit(port, ps->port_state_update_mask);
>  	schedule_work(&ps->bridge_work);
>  
>  	return 0;
> @@ -2228,11 +2236,15 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
>  	ps = container_of(work, struct mv88e6xxx_priv_state, bridge_work);
>  	ds = ((struct dsa_switch *)ps) - 1;
>  
> -	while (ps->port_state_update_mask) {
> -		port = __ffs(ps->port_state_update_mask);
> -		clear_bit(port, &ps->port_state_update_mask);
> -		mv88e6xxx_set_port_state(ds, port, ps->ports[port].state);
> -	}
> +	mutex_lock(&ps->smi_mutex);
> +
> +	for (port = 0; port < ps->num_ports; ++port)
> +		if (test_and_clear_bit(port, ps->port_state_update_mask) &&
> +		    _mv88e6xxx_port_state(ds, port, ps->ports[port].state))
> +			netdev_warn(ds->ports[port], "failed to update state to %s\n",
> +				    mv88e6xxx_port_state_names[ps->ports[port].state]);
> +
> +	mutex_unlock(&ps->smi_mutex);
>  }
>  
>  static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
> diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
> index d7b088d..3425616 100644
> --- a/drivers/net/dsa/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx.h
> @@ -426,7 +426,7 @@ struct mv88e6xxx_priv_state {
>  
>  	struct mv88e6xxx_priv_port	ports[DSA_MAX_PORTS];
>  
> -	unsigned long port_state_update_mask;
> +	DECLARE_BITMAP(port_state_update_mask, DSA_MAX_PORTS);
>  
>  	struct work_struct bridge_work;
>  };
> -- 
> 2.7.2
> 

[toc] | [prev] | [next] | [standalone]

#1355408

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Mon,  7 Mar 2016 18:24:17 -0500

> Apply a few non-functional changes on the port state setter:
> 
>   * add a dynamic debug message with state names to track changes
>   * explicit states checking instead of assuming their numeric values
>   * lock mutex only once when changing several port states
>   * use bitmap macros to declare and access port_state_update_mask
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Applied.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web