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.dide...@savoirfairelinux.com>
Tested-by: Andrew Lunn <and...@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
>
