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

[Vivien Didelot]

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>
---
 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