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 <[email protected]>
---
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
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 <[email protected]>
Tested-by: Andrew Lunn <[email protected]>
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
>
From: Vivien Didelot <[email protected]>
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 <[email protected]>
Applied.