Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754215AbcCHOhE (ORCPT ); Tue, 8 Mar 2016 09:37:04 -0500 Received: from vps0.lunn.ch ([178.209.37.122]:52707 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752039AbcCHOgz (ORCPT ); Tue, 8 Mar 2016 09:36:55 -0500 Date: Tue, 8 Mar 2016 15:36:51 +0100 From: Andrew Lunn To: Vivien Didelot Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com, "David S. Miller" , Kevin Smith Subject: Re: [PATCH net-next] net: dsa: mv88e6xxx: rework port state setter Message-ID: <20160308143651.GE351@lunn.ch> References: <1457393057-22618-1-git-send-email-vivien.didelot@savoirfairelinux.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1457393057-22618-1-git-send-email-vivien.didelot@savoirfairelinux.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4744 Lines: 139 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 Tested-by: Andrew Lunn 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 >