| Message ID | 1444261711-1829-5-git-send-email-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Oct 07, 2015 at 07:48:29PM -0400, Vivien Didelot wrote: > Push the prepare phase for FDB operations down to the DSA drivers, with > a new port_fdb_prepare function. Currently only mv88e6xxx is affected. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > --- > drivers/net/dsa/mv88e6171.c | 1 + > drivers/net/dsa/mv88e6352.c | 1 + > drivers/net/dsa/mv88e6xxx.c | 10 ++++++++++ > drivers/net/dsa/mv88e6xxx.h | 3 +++ > include/net/dsa.h | 4 ++++ > net/dsa/slave.c | 7 +++++-- > 6 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c > index c95cfab..ca3330a 100644 > --- a/drivers/net/dsa/mv88e6171.c > +++ b/drivers/net/dsa/mv88e6171.c > @@ -121,6 +121,7 @@ struct dsa_switch_driver mv88e6171_switch_driver = { > .port_vlan_add = mv88e6xxx_port_vlan_add, > .port_vlan_del = mv88e6xxx_port_vlan_del, > .vlan_getnext = mv88e6xxx_vlan_getnext, > + .port_fdb_prepare = mv88e6xxx_port_fdb_prepare, Hi Vivien Bike shedding a bit, but i would call this mv88e6xxx_port_fdb_prepare_add. > .port_fdb_add = mv88e6xxx_port_fdb_add, > .port_fdb_del = mv88e6xxx_port_fdb_del, > .port_fdb_getnext = mv88e6xxx_port_fdb_getnext, Taking a theoretical example, say mv88e6xxx_port_fdb_getnext needed a prepare call to allocate memory to put the returned ATU into. What would you call that? mv88e6xxx_port_fdb_prepare_add and mv88e6xxx_port_fdb_prepare_getnext just seems unambiguous and future proof. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 7, 2015 at 4:48 PM, Vivien Didelot <vivien.didelot@savoirfairelinux.com> wrote: > Push the prepare phase for FDB operations down to the DSA drivers, with > a new port_fdb_prepare function. Currently only mv88e6xxx is affected. > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Reviewed-by: Scott Feldman <sfeldma@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, On Oct. Thursday 08 (41) 02:25 AM, Andrew Lunn wrote: > On Wed, Oct 07, 2015 at 07:48:29PM -0400, Vivien Didelot wrote: > > Push the prepare phase for FDB operations down to the DSA drivers, with > > a new port_fdb_prepare function. Currently only mv88e6xxx is affected. > > > > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > > --- > > drivers/net/dsa/mv88e6171.c | 1 + > > drivers/net/dsa/mv88e6352.c | 1 + > > drivers/net/dsa/mv88e6xxx.c | 10 ++++++++++ > > drivers/net/dsa/mv88e6xxx.h | 3 +++ > > include/net/dsa.h | 4 ++++ > > net/dsa/slave.c | 7 +++++-- > > 6 files changed, 24 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c > > index c95cfab..ca3330a 100644 > > --- a/drivers/net/dsa/mv88e6171.c > > +++ b/drivers/net/dsa/mv88e6171.c > > @@ -121,6 +121,7 @@ struct dsa_switch_driver mv88e6171_switch_driver = { > > .port_vlan_add = mv88e6xxx_port_vlan_add, > > .port_vlan_del = mv88e6xxx_port_vlan_del, > > .vlan_getnext = mv88e6xxx_vlan_getnext, > > + .port_fdb_prepare = mv88e6xxx_port_fdb_prepare, > > Hi Vivien > > Bike shedding a bit, but i would call this > mv88e6xxx_port_fdb_prepare_add. I think port_fdb_prepare is fine because it is the only step that actually needs the 2-phase model. del and dump are safe and don't need pre-check. > > .port_fdb_add = mv88e6xxx_port_fdb_add, > > .port_fdb_del = mv88e6xxx_port_fdb_del, > > .port_fdb_getnext = mv88e6xxx_port_fdb_getnext, > > Taking a theoretical example, say mv88e6xxx_port_fdb_getnext needed a > prepare call to allocate memory to put the returned ATU into. What > would you call that? > > mv88e6xxx_port_fdb_prepare_add and mv88e6xxx_port_fdb_prepare_getnext > just seems unambiguous and future proof. the switchdev dump operation is called just once, so no preparation is implied (from the switchdev point of view). It is the responsability of the driver to call the switchdev dump callback itself. Thanks, -v -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Hi Vivien > > > > Bike shedding a bit, but i would call this > > mv88e6xxx_port_fdb_prepare_add. > > I think port_fdb_prepare is fine because it is the only step that > actually needs the 2-phase model. del and dump are safe and don't need > pre-check. O.K. I don't have a strong opinion, i just think sometime later we might run into a naming consistency issue. If this does happen, we can fix it then. Andrew -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c index c95cfab..ca3330a 100644 --- a/drivers/net/dsa/mv88e6171.c +++ b/drivers/net/dsa/mv88e6171.c @@ -121,6 +121,7 @@ struct dsa_switch_driver mv88e6171_switch_driver = { .port_vlan_add = mv88e6xxx_port_vlan_add, .port_vlan_del = mv88e6xxx_port_vlan_del, .vlan_getnext = mv88e6xxx_vlan_getnext, + .port_fdb_prepare = mv88e6xxx_port_fdb_prepare, .port_fdb_add = mv88e6xxx_port_fdb_add, .port_fdb_del = mv88e6xxx_port_fdb_del, .port_fdb_getnext = mv88e6xxx_port_fdb_getnext, diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c index 3736706..078a358 100644 --- a/drivers/net/dsa/mv88e6352.c +++ b/drivers/net/dsa/mv88e6352.c @@ -348,6 +348,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = { .port_vlan_add = mv88e6xxx_port_vlan_add, .port_vlan_del = mv88e6xxx_port_vlan_del, .vlan_getnext = mv88e6xxx_vlan_getnext, + .port_fdb_prepare = mv88e6xxx_port_fdb_prepare, .port_fdb_add = mv88e6xxx_port_fdb_add, .port_fdb_del = mv88e6xxx_port_fdb_del, .port_fdb_getnext = mv88e6xxx_port_fdb_getnext, diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c index 6053d11..9fbb727 100644 --- a/drivers/net/dsa/mv88e6xxx.c +++ b/drivers/net/dsa/mv88e6xxx.c @@ -1841,6 +1841,16 @@ static int _mv88e6xxx_port_fdb_load(struct dsa_switch *ds, int port, return _mv88e6xxx_atu_load(ds, &entry); } +int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_fdb *fdb, + struct switchdev_trans *trans) +{ + /* We don't need any dynamic resource from the kernel (yet), + * so skip the prepare phase. + */ + return 0; +} + int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid) { diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h index 39b261f..4475640 100644 --- a/drivers/net/dsa/mv88e6xxx.h +++ b/drivers/net/dsa/mv88e6xxx.h @@ -479,6 +479,9 @@ int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid, int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, u16 vid); int mv88e6xxx_vlan_getnext(struct dsa_switch *ds, u16 *vid, unsigned long *ports, unsigned long *untagged); +int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_fdb *fdb, + struct switchdev_trans *trans); int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid); int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port, diff --git a/include/net/dsa.h b/include/net/dsa.h index 3e9eb6c..3aee8a5 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -19,6 +19,7 @@ #include <linux/phy.h> #include <linux/phy_fixed.h> #include <linux/ethtool.h> +#include <net/switchdev.h> enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = 0, @@ -316,6 +317,9 @@ struct dsa_switch_driver { /* * Forwarding database */ + int (*port_fdb_prepare)(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_fdb *fdb, + struct switchdev_trans *trans); int (*port_fdb_add)(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid); int (*port_fdb_del)(struct dsa_switch *ds, int port, diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 4f607bc..48e8c15 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -346,10 +346,13 @@ static int dsa_slave_port_fdb_add(struct net_device *dev, { struct dsa_slave_priv *p = netdev_priv(dev); struct dsa_switch *ds = p->parent; - int ret = -EOPNOTSUPP; + int ret; + + if (!ds->drv->port_fdb_prepare || !ds->drv->port_fdb_add) + return -EOPNOTSUPP; if (switchdev_trans_ph_prepare(trans)) - ret = ds->drv->port_fdb_add ? 0 : -EOPNOTSUPP; + ret = ds->drv->port_fdb_prepare(ds, p->port, fdb, trans); else ret = ds->drv->port_fdb_add(ds, p->port, fdb->addr, fdb->vid);
Push the prepare phase for FDB operations down to the DSA drivers, with a new port_fdb_prepare function. Currently only mv88e6xxx is affected. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- drivers/net/dsa/mv88e6171.c | 1 + drivers/net/dsa/mv88e6352.c | 1 + drivers/net/dsa/mv88e6xxx.c | 10 ++++++++++ drivers/net/dsa/mv88e6xxx.h | 3 +++ include/net/dsa.h | 4 ++++ net/dsa/slave.c | 7 +++++-- 6 files changed, 24 insertions(+), 2 deletions(-)