Messages in this thread | | | Date | Fri, 19 Nov 2021 03:19:30 +0100 | From | Ansuel Smith <> | Subject | Re: [net-next PATCH 14/19] net: dsa: qca8k: add support for mdb_add/del |
| |
On Fri, Nov 19, 2021 at 04:06:57AM +0200, Vladimir Oltean wrote: > On Wed, Nov 17, 2021 at 10:04:46PM +0100, Ansuel Smith wrote: > > Add support for mdb add/del function. The ARL table is used to insert > > the rule. A new search function is introduced to search the rule and add > > additional port to it. If every port is removed from the rule, it's > > removed. It's set STATIC in the ARL table (aka it doesn't age) to not be > > flushed by fast age function. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > drivers/net/dsa/qca8k.c | 82 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 82 insertions(+) > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > index dda99263fe8c..a217c842ab45 100644 > > --- a/drivers/net/dsa/qca8k.c > > +++ b/drivers/net/dsa/qca8k.c > > @@ -417,6 +417,23 @@ qca8k_fdb_flush(struct qca8k_priv *priv) > > mutex_unlock(&priv->reg_mutex); > > } > > > > +static int > > +qca8k_fdb_search(struct qca8k_priv *priv, struct qca8k_fdb *fdb, const u8 *mac, u16 vid) > > +{ > > + int ret; > > + > > + mutex_lock(&priv->reg_mutex); > > If I were you, I'd create a locking scheme where the entire FDB entry is > updated under the same critical section. Right now you're relying on the > rtnl_mutex serializing calls to ->port_mdb_add()/->port_mdb_del(). But > that might change. Don't leave that task to someone that has non-expert > knowledge of the driver. >
Ok will change the logic and do the search and add/del operation in one lock.
> > + qca8k_fdb_write(priv, vid, 0, mac, 0); > > + ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1); > > + if (ret < 0) > > + goto exit; > > + > > + ret = qca8k_fdb_read(priv, fdb); > > +exit: > > + mutex_unlock(&priv->reg_mutex); > > + return ret; > > +} > > + > > static int > > qca8k_vlan_access(struct qca8k_priv *priv, enum qca8k_vlan_cmd cmd, u16 vid) > > { > > @@ -1959,6 +1976,69 @@ qca8k_port_fdb_dump(struct dsa_switch *ds, int port, > > return 0; > > } > > > > +static int > > +qca8k_port_mdb_add(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_mdb *mdb) > > +{ > > + struct qca8k_priv *priv = ds->priv; > > + struct qca8k_fdb fdb = { 0 }; > > + const u8 *addr = mdb->addr; > > + u8 port_mask = BIT(port); > > This doesn't really need to be kept in a temporary variable as it is > only used once. > > > + u16 vid = mdb->vid; > > + int ret; > > + > > + /* Check if entry already exist */ > > + ret = qca8k_fdb_search(priv, &fdb, addr, vid); > > + if (ret < 0) > > + return ret; > > + > > + /* Rule exist. Delete first */ > > + if (!fdb.aging) { > > + ret = qca8k_fdb_del(priv, addr, fdb.port_mask, vid); > > + if (ret) > > + return ret; > > + } > > + > > + /* Add port to fdb portmask */ > > + fdb.port_mask |= port_mask; > > + > > + return qca8k_port_fdb_insert(priv, addr, fdb.port_mask, vid); > > +} > > + > > +static int > > +qca8k_port_mdb_del(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_mdb *mdb) > > +{ > > + struct qca8k_priv *priv = ds->priv; > > + struct qca8k_fdb fdb = { 0 }; > > + const u8 *addr = mdb->addr; > > + u8 port_mask = BIT(port); > > + u16 vid = mdb->vid; > > + int ret; > > + > > + /* Check if entry already exist */ > > + ret = qca8k_fdb_search(priv, &fdb, addr, vid); > > + if (ret < 0) > > + return ret; > > + > > + /* Rule doesn't exist. Why delete? */ > > Because refcounting is hard. In fact with VLANs it is quite possible to > delete an absent entry. For MDBs and FDBs, the bridge should now error > out before it even reaches to you. >
So in this specific case I should simply return 0 to correctly decrement the ref, correct?
> > + if (!fdb.aging) > > + return -EINVAL; > > + > > + ret = qca8k_fdb_del(priv, addr, port_mask, vid); > > + if (ret) > > + return ret; > > + > > + /* Only port in the rule is this port. Don't re insert */ > > + if (fdb.port_mask == port_mask) > > + return 0; > > + > > + /* Remove port from port mask */ > > + fdb.port_mask &= ~port_mask; > > + > > + return qca8k_port_fdb_insert(priv, addr, fdb.port_mask, vid); > > +} > > + > > static int > > qca8k_port_mirror_add(struct dsa_switch *ds, int port, > > struct dsa_mall_mirror_tc_entry *mirror, > > @@ -2160,6 +2240,8 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > > .port_fdb_add = qca8k_port_fdb_add, > > .port_fdb_del = qca8k_port_fdb_del, > > .port_fdb_dump = qca8k_port_fdb_dump, > > + .port_mdb_add = qca8k_port_mdb_add, > > + .port_mdb_del = qca8k_port_mdb_del, > > .port_mirror_add = qca8k_port_mirror_add, > > .port_mirror_del = qca8k_port_mirror_del, > > .port_vlan_filtering = qca8k_port_vlan_filtering, > > -- > > 2.32.0 > > >
-- Ansuel
| |