[PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Vivien Didelot]

This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
objects.

The first two patches add minor but necessary changes to switchdev, the third
one implements the switchdev glue in DSA for FDB routines, and the remaining
ones refactor the FDB access functions in the mv88e6xxx code.

Below is an usage example (ports 0-2 belongs to br0, ports 3-4 belongs to br1):

    # bridge fdb add 3c:97:0e:11:30:6e dev swp2
    # bridge fdb add 3c:97:0e:11:40:78 dev swp3
    # bridge fdb add 3c:97:0e:11:50:86 dev swp4
    # bridge fdb del 3c:97:0e:11:40:78 dev swp3
    # bridge fdb
    01:00:5e:00:00:01 dev eth0 self permanent
    01:00:5e:00:00:01 dev eth1 self permanent
    00:50:d2:10:78:15 dev swp0 master br0 permanent
    3c:97:0e:11:30:6e dev swp2 self static
    00:50:d2:10:78:15 dev swp3 master br1 permanent
    3c:97:0e:11:50:86 dev swp4 self static
    # cat /sys/kernel/debug/dsa0/atu
    # DB   T/P  Vec State Addr
    # 001  Port 004   e   3c:97:0e:11:30:6e
    # 004  Port 010   e   3c:97:0e:11:50:86

For the 88E6xxx switches, FIDs 1 to num_ports will be reserved for non-bridged
ports and bridge groups, and the remaining will be later used by VLANs.

This change is necessary to welcome the support for hardware VLANs (which will
follow soon).

Changes in v2:

 - remove ndo_bridge_{get,set,del}link from switchdev/DSA glue code

 - use ether_addr_copy instead of memcpy for MAC addresses

 - constify MAC address in port_fdb_{add,del}

 - split the mv88e6xxx code refactoring into several patches

Vivien Didelot (7):
  net: switchdev: change fdb addr for a byte array
  net: switchdev: support static FDB addresses
  net: dsa: add support for switchdev FDB objects
  net: dsa: mv88e6xxx: extend fid mask
  net: dsa: mv88e6xxx: rename ATU MAC accessors
  net: dsa: mv88e6xxx: rework FDB getnext operation
  net: dsa: mv88e6xxx: rework FDB add/del operations

 drivers/net/dsa/mv88e6171.c          |   6 +-
 drivers/net/dsa/mv88e6352.c          |   6 +-
 drivers/net/dsa/mv88e6xxx.c          | 223 ++++++++++++++++++++++++-----------
 drivers/net/dsa/mv88e6xxx.h          |  31 +++--
 drivers/net/ethernet/rocker/rocker.c |   2 +-
 include/net/dsa.h                    |  16 ++-
 include/net/switchdev.h              |   3 +-
 net/bridge/br_fdb.c                  |   2 +-
 net/dsa/slave.c                      | 218 ++++++++++++++++++----------------
 net/switchdev/switchdev.c            |   7 +-
 10 files changed, 317 insertions(+), 197 deletions(-)

-- 
2.4.6

[PATCH net-next v2 1/7] net: switchdev: change fdb addr for a byte array

[Vivien Didelot]

The address in the switchdev_obj_fdb structure is currently represented
as a pointer. Replacing it for a 6-byte array allows switchdev to carry
addresses directly read from hardware registers, not stored by the
switch chip driver (as in Rocker).

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/ethernet/rocker/rocker.c | 2 +-
 include/net/switchdev.h              | 2 +-
 net/bridge/br_fdb.c                  | 2 +-
 net/switchdev/switchdev.c            | 5 +++--
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 4cd5a71..a5bf809 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4543,7 +4543,7 @@ static int rocker_port_fdb_dump(const struct rocker_port *rocker_port,
 	hash_for_each_safe(rocker->fdb_tbl, bkt, tmp, found, entry) {
 		if (found->key.pport != rocker_port->pport)
 			continue;
-		fdb->addr = found->key.addr;
+		ether_addr_copy(fdb->addr, found->key.addr);
 		fdb->vid = rocker_port_vlan_to_vid(rocker_port,
 						   found->key.vlan_id);
 		err = obj->cb(rocker_port->dev, obj);
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 89da893..e90e1a0 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -70,7 +70,7 @@ struct switchdev_obj {
 			u32 tb_id;
 		} ipv4_fib;
 		struct switchdev_obj_fdb {		/* PORT_FDB */
-			const unsigned char *addr;
+			u8 addr[ETH_ALEN];
 			u16 vid;
 		} fdb;
 	} u;
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9e9875d..5656b44 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -136,11 +136,11 @@ static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
 	struct switchdev_obj obj = {
 		.id = SWITCHDEV_OBJ_PORT_FDB,
 		.u.fdb = {
-			.addr = f->addr.addr,
 			.vid = f->vlan_id,
 		},
 	};
 
+	ether_addr_copy(obj.u.fdb.addr, f->addr.addr);
 	switchdev_port_obj_del(f->dst->dev, &obj);
 }
 
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 33bafa2..9db87a3 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -15,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/netdevice.h>
+#include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <net/ip_fib.h>
 #include <net/switchdev.h>
@@ -742,11 +743,11 @@ int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	struct switchdev_obj obj = {
 		.id = SWITCHDEV_OBJ_PORT_FDB,
 		.u.fdb = {
-			.addr = addr,
 			.vid = vid,
 		},
 	};
 
+	ether_addr_copy(obj.u.fdb.addr, addr);
 	return switchdev_port_obj_add(dev, &obj);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fdb_add);
@@ -769,11 +770,11 @@ int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 	struct switchdev_obj obj = {
 		.id = SWITCHDEV_OBJ_PORT_FDB,
 		.u.fdb = {
-			.addr = addr,
 			.vid = vid,
 		},
 	};
 
+	ether_addr_copy(obj.u.fdb.addr, addr);
 	return switchdev_port_obj_del(dev, &obj);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fdb_del);
-- 
2.4.6

[PATCH net-next v2 2/7] net: switchdev: support static FDB addresses

[Vivien Didelot]

This patch adds a is_static boolean to the switchdev_obj_fdb structure,
in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/switchdev.h   | 1 +
 net/switchdev/switchdev.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index e90e1a0..0e296b8 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -72,6 +72,7 @@ struct switchdev_obj {
 		struct switchdev_obj_fdb {		/* PORT_FDB */
 			u8 addr[ETH_ALEN];
 			u16 vid;
+			bool is_static;
 		} fdb;
 	} u;
 };
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 9db87a3..e9d1cac 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,
 	ndm->ndm_flags   = NTF_SELF;
 	ndm->ndm_type    = 0;
 	ndm->ndm_ifindex = dev->ifindex;
-	ndm->ndm_state   = NUD_REACHABLE;
+	ndm->ndm_state   = obj->u.fdb.is_static ? NUD_NOARP : NUD_REACHABLE;
 
 	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, obj->u.fdb.addr))
 		goto nla_put_failure;
-- 
2.4.6

Re: [PATCH net-next v2 2/7] net: switchdev: support static FDB addresses

[Scott Feldman]

On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> This patch adds a is_static boolean to the switchdev_obj_fdb structure,
> in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  include/net/switchdev.h   | 1 +
>  net/switchdev/switchdev.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index e90e1a0..0e296b8 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -72,6 +72,7 @@ struct switchdev_obj {
>                 struct switchdev_obj_fdb {              /* PORT_FDB */
>                         u8 addr[ETH_ALEN];
>                         u16 vid;
> +                       bool is_static;

What do you think about changing this to u16 ndm_state?  That way, it
can be used on input (fdb add) and output (fdb dump), and the driver
can privately track the state, kind of like how the bridge keeps
is_static, is_local, etc.

>                 } fdb;
>         } u;
>  };
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 9db87a3..e9d1cac 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,
>         ndm->ndm_flags   = NTF_SELF;
>         ndm->ndm_type    = 0;
>         ndm->ndm_ifindex = dev->ifindex;
> -       ndm->ndm_state   = NUD_REACHABLE;
> +       ndm->ndm_state   = obj->u.fdb.is_static ? NUD_NOARP : NUD_REACHABLE;
>
>         if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, obj->u.fdb.addr))
>                 goto nla_put_failure;
> --
> 2.4.6
>

Re: [PATCH net-next v2 2/7] net: switchdev: support static FDB addresses

[Vivien Didelot]

On 15-08-05 23:28:15, Scott Feldman wrote:
> On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
> > This patch adds a is_static boolean to the switchdev_obj_fdb structure,
> > in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> > ---
> >  include/net/switchdev.h   | 1 +
> >  net/switchdev/switchdev.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> > index e90e1a0..0e296b8 100644
> > --- a/include/net/switchdev.h
> > +++ b/include/net/switchdev.h
> > @@ -72,6 +72,7 @@ struct switchdev_obj {
> >                 struct switchdev_obj_fdb {              /* PORT_FDB */
> >                         u8 addr[ETH_ALEN];
> >                         u16 vid;
> > +                       bool is_static;
> 
> What do you think about changing this to u16 ndm_state?  That way, it
> can be used on input (fdb add) and output (fdb dump), and the driver
> can privately track the state, kind of like how the bridge keeps
> is_static, is_local, etc.

I'm OK with the change. Should we consider NUD_NONE (0) a valid value?

> >                 } fdb;
> >         } u;
> >  };
> > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> > index 9db87a3..e9d1cac 100644
> > --- a/net/switchdev/switchdev.c
> > +++ b/net/switchdev/switchdev.c
> > @@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,
> >         ndm->ndm_flags   = NTF_SELF;
> >         ndm->ndm_type    = 0;
> >         ndm->ndm_ifindex = dev->ifindex;
> > -       ndm->ndm_state   = NUD_REACHABLE;
> > +       ndm->ndm_state   = obj->u.fdb.is_static ? NUD_NOARP : NUD_REACHABLE;

In other word, do we prefer this:

    ndm->ndm_state = obj->u.fdb.ndm_state == NUD_NONE ?
                     NUD_REACHABLE : obj->u.fdb.ndm_state;

Or this (meaning switchdev users cannot leave it blank and must at least
set NUD_REACHABLE themselves):

    ndm->ndm_state = obj->u.fdb.ndm_state;

Thanks,
-v

Re: [PATCH net-next v2 2/7] net: switchdev: support static FDB addresses

[Scott Feldman]

On Thu, Aug 6, 2015 at 7:19 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> On 15-08-05 23:28:15, Scott Feldman wrote:
>> On Wed, Aug 5, 2015 at 10:44 PM, Vivien Didelot
>> <vivien.didelot@savoirfairelinux.com> wrote:
>> > This patch adds a is_static boolean to the switchdev_obj_fdb structure,
>> > in order to set the ndm_state to either NUD_NOARP or NUD_REACHABLE.
>> >
>> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> > ---
>> >  include/net/switchdev.h   | 1 +
>> >  net/switchdev/switchdev.c | 2 +-
>> >  2 files changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> > index e90e1a0..0e296b8 100644
>> > --- a/include/net/switchdev.h
>> > +++ b/include/net/switchdev.h
>> > @@ -72,6 +72,7 @@ struct switchdev_obj {
>> >                 struct switchdev_obj_fdb {              /* PORT_FDB */
>> >                         u8 addr[ETH_ALEN];
>> >                         u16 vid;
>> > +                       bool is_static;
>>
>> What do you think about changing this to u16 ndm_state?  That way, it
>> can be used on input (fdb add) and output (fdb dump), and the driver
>> can privately track the state, kind of like how the bridge keeps
>> is_static, is_local, etc.
>
> I'm OK with the change. Should we consider NUD_NONE (0) a valid value?
>
>> >                 } fdb;
>> >         } u;
>> >  };
>> > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> > index 9db87a3..e9d1cac 100644
>> > --- a/net/switchdev/switchdev.c
>> > +++ b/net/switchdev/switchdev.c
>> > @@ -811,7 +811,7 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,
>> >         ndm->ndm_flags   = NTF_SELF;
>> >         ndm->ndm_type    = 0;
>> >         ndm->ndm_ifindex = dev->ifindex;
>> > -       ndm->ndm_state   = NUD_REACHABLE;
>> > +       ndm->ndm_state   = obj->u.fdb.is_static ? NUD_NOARP : NUD_REACHABLE;
>
> In other word, do we prefer this:
>
>     ndm->ndm_state = obj->u.fdb.ndm_state == NUD_NONE ?
>                      NUD_REACHABLE : obj->u.fdb.ndm_state;
>
> Or this (meaning switchdev users cannot leave it blank and must at least
> set NUD_REACHABLE themselves):
>
>     ndm->ndm_state = obj->u.fdb.ndm_state;

My preference would be this option.

[PATCH net-next v2 3/7] net: dsa: add support for switchdev FDB objects

[Vivien Didelot]

Remove the fdb_{add,del,getnext} function pointer in favor of new
port_fdb_{add,del,getnext}.

Implement the switchdev_port_obj_{add,del,dump} functions in DSA to
support the SWITCHDEV_OBJ_PORT_FDB objects.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6171.c |   3 -
 drivers/net/dsa/mv88e6352.c |   3 -
 include/net/dsa.h           |  16 ++--
 net/dsa/slave.c             | 218 +++++++++++++++++++++++---------------------
 4 files changed, 126 insertions(+), 114 deletions(-)

diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index 1c78084..cfa21ed 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -116,9 +116,6 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.port_join_bridge       = mv88e6xxx_join_bridge,
 	.port_leave_bridge      = mv88e6xxx_leave_bridge,
 	.port_stp_update        = mv88e6xxx_port_stp_update,
-	.fdb_add		= mv88e6xxx_port_fdb_add,
-	.fdb_del		= mv88e6xxx_port_fdb_del,
-	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
 };
 
 MODULE_ALIAS("platform:mv88e6171");
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index af210ef..eb4630f 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -341,9 +341,6 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.port_join_bridge	= mv88e6xxx_join_bridge,
 	.port_leave_bridge	= mv88e6xxx_leave_bridge,
 	.port_stp_update	= mv88e6xxx_port_stp_update,
-	.fdb_add		= mv88e6xxx_port_fdb_add,
-	.fdb_del		= mv88e6xxx_port_fdb_del,
-	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
 };
 
 MODULE_ALIAS("platform:mv88e6172");
diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63b..091d35f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -296,12 +296,16 @@ struct dsa_switch_driver {
 				     u32 br_port_mask);
 	int	(*port_stp_update)(struct dsa_switch *ds, int port,
 				   u8 state);
-	int	(*fdb_add)(struct dsa_switch *ds, int port,
-			   const unsigned char *addr, u16 vid);
-	int	(*fdb_del)(struct dsa_switch *ds, int port,
-			   const unsigned char *addr, u16 vid);
-	int	(*fdb_getnext)(struct dsa_switch *ds, int port,
-			       unsigned char *addr, bool *is_static);
+
+	/*
+	 * Forwarding database
+	 */
+	int	(*port_fdb_add)(struct dsa_switch *ds, int port, u16 vid,
+				const u8 addr[ETH_ALEN]);
+	int	(*port_fdb_del)(struct dsa_switch *ds, int port, u16 vid,
+				const u8 addr[ETH_ALEN]);
+	int	(*port_fdb_getnext)(struct dsa_switch *ds, int port, u16 *vid,
+				    u8 addr[ETH_ALEN], bool *is_static);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 0010c69..1dbdeaa 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -19,6 +19,7 @@
 #include <net/switchdev.h>
 #include <linux/if_bridge.h>
 #include <linux/netpoll.h>
+#include <linux/if_vlan.h>
 #include "dsa_priv.h"
 
 /* slave mii_bus handling ***************************************************/
@@ -200,105 +201,6 @@ out:
 	return 0;
 }
 
-static int dsa_slave_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
-			     struct net_device *dev,
-			     const unsigned char *addr, u16 vid, u16 nlm_flags)
-{
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
-	int ret = -EOPNOTSUPP;
-
-	if (ds->drv->fdb_add)
-		ret = ds->drv->fdb_add(ds, p->port, addr, vid);
-
-	return ret;
-}
-
-static int dsa_slave_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
-			     struct net_device *dev,
-			     const unsigned char *addr, u16 vid)
-{
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
-	int ret = -EOPNOTSUPP;
-
-	if (ds->drv->fdb_del)
-		ret = ds->drv->fdb_del(ds, p->port, addr, vid);
-
-	return ret;
-}
-
-static int dsa_slave_fill_info(struct net_device *dev, struct sk_buff *skb,
-			       const unsigned char *addr, u16 vid,
-			       bool is_static,
-			       u32 portid, u32 seq, int type,
-			       unsigned int flags)
-{
-	struct nlmsghdr *nlh;
-	struct ndmsg *ndm;
-
-	nlh = nlmsg_put(skb, portid, seq, type, sizeof(*ndm), flags);
-	if (!nlh)
-		return -EMSGSIZE;
-
-	ndm = nlmsg_data(nlh);
-	ndm->ndm_family	 = AF_BRIDGE;
-	ndm->ndm_pad1    = 0;
-	ndm->ndm_pad2    = 0;
-	ndm->ndm_flags	 = NTF_EXT_LEARNED;
-	ndm->ndm_type	 = 0;
-	ndm->ndm_ifindex = dev->ifindex;
-	ndm->ndm_state   = is_static ? NUD_NOARP : NUD_REACHABLE;
-
-	if (nla_put(skb, NDA_LLADDR, ETH_ALEN, addr))
-		goto nla_put_failure;
-
-	if (vid && nla_put_u16(skb, NDA_VLAN, vid))
-		goto nla_put_failure;
-
-	nlmsg_end(skb, nlh);
-	return 0;
-
-nla_put_failure:
-	nlmsg_cancel(skb, nlh);
-	return -EMSGSIZE;
-}
-
-/* Dump information about entries, in response to GETNEIGH */
-static int dsa_slave_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
-			      struct net_device *dev,
-			      struct net_device *filter_dev, int idx)
-{
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
-	unsigned char addr[ETH_ALEN] = { 0 };
-	int ret;
-
-	if (!ds->drv->fdb_getnext)
-		return -EOPNOTSUPP;
-
-	for (; ; idx++) {
-		bool is_static;
-
-		ret = ds->drv->fdb_getnext(ds, p->port, addr, &is_static);
-		if (ret < 0)
-			break;
-
-		if (idx < cb->args[0])
-			continue;
-
-		ret = dsa_slave_fill_info(dev, skb, addr, 0,
-					  is_static,
-					  NETLINK_CB(cb->skb).portid,
-					  cb->nlh->nlmsg_seq,
-					  RTM_NEWNEIGH, NLM_F_MULTI);
-		if (ret < 0)
-			break;
-	}
-
-	return idx;
-}
-
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -364,6 +266,115 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
+static int dsa_slave_port_fdb_add(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int err;
+
+	if (obj->trans == SWITCHDEV_TRANS_PREPARE)
+		err = ds->drv->port_fdb_add ? 0 : -EOPNOTSUPP;
+	else if (obj->trans == SWITCHDEV_TRANS_COMMIT)
+		err = ds->drv->port_fdb_add(ds, p->port, fdb->vid, fdb->addr);
+	else
+		err = -EOPNOTSUPP;
+
+	return err;
+}
+
+static int dsa_slave_port_fdb_del(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (!ds->drv->port_fdb_del)
+		return -EOPNOTSUPP;
+
+	return ds->drv->port_fdb_del(ds, p->port, fdb->vid, fdb->addr);
+}
+
+static int dsa_slave_port_fdb_dump(struct net_device *dev,
+				   struct switchdev_obj *obj)
+{
+	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int err;
+
+	if (!ds->drv->port_fdb_getnext)
+		return -EOPNOTSUPP;
+
+	memset(fdb, 0, sizeof(*fdb));
+
+	for (;;) {
+		err = ds->drv->port_fdb_getnext(ds, p->port, &fdb->vid,
+						fdb->addr, &fdb->is_static);
+		if (err)
+			break;
+
+		err = obj->cb(dev, obj);
+		if (err)
+			break;
+	}
+
+	return err == -ENOENT ? 0 : err;
+}
+
+static int dsa_slave_port_obj_add(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	int err;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = dsa_slave_port_fdb_add(dev, obj);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int dsa_slave_port_obj_del(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	int err;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = dsa_slave_port_fdb_del(dev, obj);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int dsa_slave_port_obj_dump(struct net_device *dev,
+				   struct switchdev_obj *obj)
+{
+	int err;
+
+	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_FDB:
+		err = dsa_slave_port_fdb_dump(dev, obj);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
 static int dsa_slave_bridge_port_join(struct net_device *dev,
 				      struct net_device *br)
 {
@@ -765,9 +776,9 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_change_rx_flags	= dsa_slave_change_rx_flags,
 	.ndo_set_rx_mode	= dsa_slave_set_rx_mode,
 	.ndo_set_mac_address	= dsa_slave_set_mac_address,
-	.ndo_fdb_add		= dsa_slave_fdb_add,
-	.ndo_fdb_del		= dsa_slave_fdb_del,
-	.ndo_fdb_dump		= dsa_slave_fdb_dump,
+	.ndo_fdb_add		= switchdev_port_fdb_add,
+	.ndo_fdb_del		= switchdev_port_fdb_del,
+	.ndo_fdb_dump		= switchdev_port_fdb_dump,
 	.ndo_do_ioctl		= dsa_slave_ioctl,
 	.ndo_get_iflink		= dsa_slave_get_iflink,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -780,6 +791,9 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 static const struct switchdev_ops dsa_slave_switchdev_ops = {
 	.switchdev_port_attr_get	= dsa_slave_port_attr_get,
 	.switchdev_port_attr_set	= dsa_slave_port_attr_set,
+	.switchdev_port_obj_add		= dsa_slave_port_obj_add,
+	.switchdev_port_obj_del		= dsa_slave_port_obj_del,
+	.switchdev_port_obj_dump	= dsa_slave_port_obj_dump,
 };
 
 static void dsa_slave_adjust_link(struct net_device *dev)
-- 
2.4.6

Re: [PATCH net-next v2 3/7] net: dsa: add support for switchdev FDB objects

[Andrew Lunn]

Hi Vivien

Thanks for splitting up the big patch. This it is much easier to
review now.

Is this patch git bisectable?

Clearly after this patch, but before all the other patches are in, we
will not be programming the hardware. The call into the driver is
removed here, but the replacement is added later. But is the EOPNOTSUP
enough that the system keeps working, by falling back to software?

The two driver APIs are very similar, the main difference being the
MAC address. Can you do the refactoring first, and then make the API
change. That means we can test each patch individually, and have
proper git bisectability.

Thanks
       Andrew

Re: [PATCH net-next v2 3/7] net: dsa: add support for switchdev FDB objects

[Vivien Didelot]

On 15-08-06 16:04:32, Andrew Lunn wrote:
> Hi Vivien
> 
> Thanks for splitting up the big patch. This it is much easier to
> review now.
> 
> Is this patch git bisectable?

In terms of compilation, yes.

> Clearly after this patch, but before all the other patches are in, we
> will not be programming the hardware. The call into the driver is
> removed here, but the replacement is added later. But is the EOPNOTSUP
> enough that the system keeps working, by falling back to software?

You're right, it isn't. At this exact patch, issuing the example:

    bridge fdb add 3c:97:0e:11:30:6e dev swp2

returns "RTNETLINK answers: Operation not supported".

> The two driver APIs are very similar, the main difference being the
> MAC address. Can you do the refactoring first, and then make the API
> change. That means we can test each patch individually, and have
> proper git bisectability.

That'd be better indeed. I'll integrate the migration from
.fdb_{add,del,getnext} to .port_fdb_{add,del,getnext} in this patch,
then add the next patches improving FID and ATU management on top of it.

Thanks,
-v

[PATCH net-next v2 4/7] net: dsa: mv88e6xxx: extend fid mask

[Vivien Didelot]

The driver currently manages one FID per port (or bridge group), with a
mask of DSA_MAX_PORTS bits, where 0 means that the FID is in use.

The Marvell 88E6xxx switches support up to 4094 FIDs (from 1 to 0xfff;
FID 0 means that multiple address databases are not being used).

This patch changes the fid_mask for an fid_bitmap of 4096 bits.

>From now on, FIDs 1 to num_ports are reserved for non-bridged ports and
bridge groups (a bridge group gets the FID of its first member). The
remaining bits will be reserved for VLAN entries.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 20 +++++++++++++-------
 drivers/net/dsa/mv88e6xxx.h |  8 +++++---
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 438c73e..b051576 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1091,7 +1091,7 @@ int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
 	ps->bridge_mask[fid] = br_port_mask;
 
 	if (fid != ps->fid[port]) {
-		ps->fid_mask |= 1 << ps->fid[port];
+		clear_bit(ps->fid[port], ps->fid_bitmap);
 		ps->fid[port] = fid;
 		ret = _mv88e6xxx_update_bridge_config(ds, fid);
 	}
@@ -1125,9 +1125,16 @@ int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
 
 	mutex_lock(&ps->smi_mutex);
 
-	newfid = __ffs(ps->fid_mask);
+	newfid = find_next_zero_bit(ps->fid_bitmap, VLAN_N_VID, 1);
+	if (unlikely(newfid > ps->num_ports)) {
+		netdev_err(ds->ports[port], "all first %d FIDs are used\n",
+			   ps->num_ports);
+		ret = -ENOSPC;
+		goto unlock;
+	}
+
 	ps->fid[port] = newfid;
-	ps->fid_mask &= ~(1 << newfid);
+	set_bit(newfid, ps->fid_bitmap);
 	ps->bridge_mask[fid] &= ~(1 << port);
 	ps->bridge_mask[newfid] = 1 << port;
 
@@ -1135,6 +1142,7 @@ int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask)
 	if (!ret)
 		ret = _mv88e6xxx_update_bridge_config(ds, newfid);
 
+unlock:
 	mutex_unlock(&ps->smi_mutex);
 
 	return ret;
@@ -1554,9 +1562,9 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	 * ports, and allow each of the 'real' ports to only talk to
 	 * the upstream port.
 	 */
-	fid = __ffs(ps->fid_mask);
+	fid = port + 1;
 	ps->fid[port] = fid;
-	ps->fid_mask &= ~(1 << fid);
+	set_bit(fid, ps->fid_bitmap);
 
 	if (!dsa_is_cpu_port(ds, port))
 		ps->bridge_mask[fid] = 1 << port;
@@ -1855,8 +1863,6 @@ int mv88e6xxx_setup_common(struct dsa_switch *ds)
 
 	ps->id = REG_READ(REG_PORT(0), PORT_SWITCH_ID) & 0xfff0;
 
-	ps->fid_mask = (1 << DSA_MAX_PORTS) - 1;
-
 	INIT_WORK(&ps->bridge_work, mv88e6xxx_bridge_work);
 
 	name = kasprintf(GFP_KERNEL, "dsa%d", ds->index);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 6a66b4b..6d65b99 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -11,6 +11,8 @@
 #ifndef __MV88E6XXX_H
 #define __MV88E6XXX_H
 
+#include <linux/if_vlan.h>
+
 #ifndef UINT64_MAX
 #define UINT64_MAX		(u64)(~((u64)0))
 #endif
@@ -348,9 +350,9 @@ struct mv88e6xxx_priv_state {
 
 	/* hw bridging */
 
-	u32 fid_mask;
-	u8 fid[DSA_MAX_PORTS];
-	u16 bridge_mask[DSA_MAX_PORTS];
+	DECLARE_BITMAP(fid_bitmap, VLAN_N_VID);	/* FIDs 1 to 4095 available */
+	u16 fid[DSA_MAX_PORTS];			/* per (non-bridged) port FID */
+	u16 bridge_mask[DSA_MAX_PORTS];		/* br groups (indexed by FID) */
 
 	unsigned long port_state_update_mask;
 	u8 port_state[DSA_MAX_PORTS];
-- 
2.4.6

[PATCH net-next v2 5/7] net: dsa: mv88e6xxx: rename ATU MAC accessors

[Vivien Didelot]

Rename the __mv88e6xxx_{read,write}_addr functions to more explicit
_mv88e6xxx_atu_mac_{read,write} functions, which also respect the single
underscore convention used in the file (meaning SMI lock must be held).

In the meantime, define their MAC address parameters as an array of
ETH_ALEN bytes instead of a char pointer.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index b051576..9dad0a7 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1182,8 +1182,8 @@ int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state)
 	return 0;
 }
 
-static int __mv88e6xxx_write_addr(struct dsa_switch *ds,
-				  const unsigned char *addr)
+static int _mv88e6xxx_atu_mac_write(struct dsa_switch *ds,
+				    const u8 addr[ETH_ALEN])
 {
 	int i, ret;
 
@@ -1198,7 +1198,7 @@ static int __mv88e6xxx_write_addr(struct dsa_switch *ds,
 	return 0;
 }
 
-static int __mv88e6xxx_read_addr(struct dsa_switch *ds, unsigned char *addr)
+static int _mv88e6xxx_atu_mac_read(struct dsa_switch *ds, u8 addr[ETH_ALEN])
 {
 	int i, ret;
 
@@ -1225,7 +1225,7 @@ static int __mv88e6xxx_port_fdb_cmd(struct dsa_switch *ds, int port,
 	if (ret < 0)
 		return ret;
 
-	ret = __mv88e6xxx_write_addr(ds, addr);
+	ret = _mv88e6xxx_atu_mac_write(ds, addr);
 	if (ret < 0)
 		return ret;
 
@@ -1280,7 +1280,7 @@ static int __mv88e6xxx_port_getnext(struct dsa_switch *ds, int port,
 	if (ret < 0)
 		return ret;
 
-	ret = __mv88e6xxx_write_addr(ds, addr);
+	ret = _mv88e6xxx_atu_mac_write(ds, addr);
 	if (ret < 0)
 		return ret;
 
@@ -1297,7 +1297,7 @@ static int __mv88e6xxx_port_getnext(struct dsa_switch *ds, int port,
 			return -ENOENT;
 	} while (!(((ret >> 4) & 0xff) & (1 << port)));
 
-	ret = __mv88e6xxx_read_addr(ds, addr);
+	ret = _mv88e6xxx_atu_mac_read(ds, addr);
 	if (ret < 0)
 		return ret;
 
@@ -1661,7 +1661,7 @@ static int mv88e6xxx_atu_show_db(struct seq_file *s, struct dsa_switch *ds,
 	unsigned char addr[6];
 	int ret, data, state;
 
-	ret = __mv88e6xxx_write_addr(ds, bcast);
+	ret = _mv88e6xxx_atu_mac_write(ds, bcast);
 	if (ret < 0)
 		return ret;
 
@@ -1676,7 +1676,7 @@ static int mv88e6xxx_atu_show_db(struct seq_file *s, struct dsa_switch *ds,
 		state = data & GLOBAL_ATU_DATA_STATE_MASK;
 		if (state == GLOBAL_ATU_DATA_STATE_UNUSED)
 			break;
-		ret = __mv88e6xxx_read_addr(ds, addr);
+		ret = _mv88e6xxx_atu_mac_read(ds, addr);
 		if (ret < 0)
 			return ret;
 		mv88e6xxx_atu_show_entry(s, dbnum, addr, data);
-- 
2.4.6

[PATCH net-next v2 6/7] net: dsa: mv88e6xxx: rework FDB getnext operation

[Vivien Didelot]

This commit adds a low level _mv88e6xxx_atu_getnext function and helpers
to rewrite the mv88e6xxx_port_fdb_getnext operation.

A mv88e6xxx_atu_entry structure is added for convenient access to the
hardware, and GLOBAL_ATU_FID is defined instead of the raw 0x01 value.

The previous implementation did not handle the eventual trunk mapping.
If the related bit is set, then the ATU data register would contain the
trunk ID, and not the port vector.

Check this in the FDB getnext operation and do not handle it (yet).

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 | 97 +++++++++++++++++++++++++++++++++------------
 drivers/net/dsa/mv88e6xxx.h | 15 ++++++-
 4 files changed, 87 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index cfa21ed..b99fa50 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -116,6 +116,7 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.port_join_bridge       = mv88e6xxx_join_bridge,
 	.port_leave_bridge      = mv88e6xxx_leave_bridge,
 	.port_stp_update        = mv88e6xxx_port_stp_update,
+	.port_fdb_getnext	= mv88e6xxx_port_fdb_getnext,
 };
 
 MODULE_ALIAS("platform:mv88e6171");
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index eb4630f..0a77135 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -341,6 +341,7 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.port_join_bridge	= mv88e6xxx_join_bridge,
 	.port_leave_bridge	= mv88e6xxx_leave_bridge,
 	.port_stp_update	= mv88e6xxx_port_stp_update,
+	.port_fdb_getnext	= mv88e6xxx_port_fdb_getnext,
 };
 
 MODULE_ALIAS("platform:mv88e6172");
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 9dad0a7..6cad168 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -964,7 +964,7 @@ static int _mv88e6xxx_atu_cmd(struct dsa_switch *ds, int fid, u16 cmd)
 {
 	int ret;
 
-	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, 0x01, fid);
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_ATU_FID, fid);
 	if (ret < 0)
 		return ret;
 
@@ -1269,12 +1269,14 @@ int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
-static int __mv88e6xxx_port_getnext(struct dsa_switch *ds, int port,
-				    unsigned char *addr, bool *is_static)
+static int _mv88e6xxx_atu_getnext(struct dsa_switch *ds, u16 fid,
+				  const u8 addr[ETH_ALEN],
+				  struct mv88e6xxx_atu_entry *entry)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	u8 fid = ps->fid[port];
-	int ret, state;
+	struct mv88e6xxx_atu_entry next = { 0 };
+	int ret;
+
+	next.fid = fid;
 
 	ret = _mv88e6xxx_atu_wait(ds);
 	if (ret < 0)
@@ -1284,39 +1286,84 @@ static int __mv88e6xxx_port_getnext(struct dsa_switch *ds, int port,
 	if (ret < 0)
 		return ret;
 
-	do {
-		ret = _mv88e6xxx_atu_cmd(ds, fid,  GLOBAL_ATU_OP_GET_NEXT_DB);
-		if (ret < 0)
-			return ret;
+	ret = _mv88e6xxx_atu_cmd(ds, fid, GLOBAL_ATU_OP_GET_NEXT_DB);
+	if (ret < 0)
+		return ret;
 
-		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_ATU_DATA);
-		if (ret < 0)
-			return ret;
-		state = ret & GLOBAL_ATU_DATA_STATE_MASK;
-		if (state == GLOBAL_ATU_DATA_STATE_UNUSED)
-			return -ENOENT;
-	} while (!(((ret >> 4) & 0xff) & (1 << port)));
+	ret = _mv88e6xxx_atu_mac_read(ds, next.mac);
+	if (ret < 0)
+		return ret;
 
-	ret = _mv88e6xxx_atu_mac_read(ds, addr);
+	ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_ATU_DATA);
 	if (ret < 0)
 		return ret;
 
-	*is_static = state == (is_multicast_ether_addr(addr) ?
-			       GLOBAL_ATU_DATA_STATE_MC_STATIC :
-			       GLOBAL_ATU_DATA_STATE_UC_STATIC);
+	next.state = ret & GLOBAL_ATU_DATA_STATE_MASK;
+	if (next.state != GLOBAL_ATU_DATA_STATE_UNUSED) {
+		unsigned int mask, shift;
+
+		if (ret & GLOBAL_ATU_DATA_TRUNK) {
+			next.trunk = true;
+			mask = GLOBAL_ATU_DATA_TRUNK_ID_MASK;
+			shift = GLOBAL_ATU_DATA_TRUNK_ID_SHIFT;
+		} else {
+			next.trunk = false;
+			mask = GLOBAL_ATU_DATA_PORT_VECTOR_MASK;
+			shift = GLOBAL_ATU_DATA_PORT_VECTOR_SHIFT;
+		}
+
+		next.portv_trunkid = (ret & mask) >> shift;
+	}
 
+	*entry = next;
 	return 0;
 }
 
-/* get next entry for port */
-int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
-			       unsigned char *addr, bool *is_static)
+static int _mv88e6xxx_port_vid_to_fid(struct dsa_switch *ds, int port, u16 vid)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+
+	if (vid == 0)
+		return ps->fid[port];
+
+	return -ENOENT;
+}
+
+int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port, u16 *vid,
+			       u8 addr[ETH_ALEN], bool *is_static)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_atu_entry next;
+	u16 fid;
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = __mv88e6xxx_port_getnext(ds, port, addr, is_static);
+
+	ret = _mv88e6xxx_port_vid_to_fid(ds, port, *vid);
+	if (ret < 0)
+		goto unlock;
+	fid = ret;
+
+	do {
+		if (is_broadcast_ether_addr(addr)) {
+			ret = -ENOENT;
+			goto unlock;
+		}
+
+		ret = _mv88e6xxx_atu_getnext(ds, fid, addr, &next);
+		if (ret < 0)
+			goto unlock;
+
+		ether_addr_copy(addr, next.mac);
+
+		if (next.state == GLOBAL_ATU_DATA_STATE_UNUSED)
+			continue;
+	} while (next.trunk || (next.portv_trunkid & BIT(port)) == 0);
+
+	*is_static = next.state == (is_multicast_ether_addr(addr) ?
+				    GLOBAL_ATU_DATA_STATE_MC_STATIC :
+				    GLOBAL_ATU_DATA_STATE_UC_STATIC);
+unlock:
 	mutex_unlock(&ps->smi_mutex);
 
 	return ret;
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 6d65b99..d9bd6a0 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -172,6 +172,7 @@
 #define GLOBAL_MAC_01		0x01
 #define GLOBAL_MAC_23		0x02
 #define GLOBAL_MAC_45		0x03
+#define GLOBAL_ATU_FID		0x01	/* 6097 6165 6351 6352 */
 #define GLOBAL_CONTROL		0x04
 #define GLOBAL_CONTROL_SW_RESET		BIT(15)
 #define GLOBAL_CONTROL_PPU_ENABLE	BIT(14)
@@ -206,6 +207,8 @@
 #define GLOBAL_ATU_OP_GET_CLR_VIOLATION	  ((7 << 12) | GLOBAL_ATU_OP_BUSY)
 #define GLOBAL_ATU_DATA		0x0c
 #define GLOBAL_ATU_DATA_TRUNK			BIT(15)
+#define GLOBAL_ATU_DATA_TRUNK_ID_MASK		0x00f0
+#define GLOBAL_ATU_DATA_TRUNK_ID_SHIFT		4
 #define GLOBAL_ATU_DATA_PORT_VECTOR_MASK	0x3ff0
 #define GLOBAL_ATU_DATA_PORT_VECTOR_SHIFT	4
 #define GLOBAL_ATU_DATA_STATE_MASK		0x0f
@@ -312,6 +315,14 @@
 #define GLOBAL2_QOS_WEIGHT	0x1c
 #define GLOBAL2_MISC		0x1d
 
+struct mv88e6xxx_atu_entry {
+	u16	fid;
+	u8	state;
+	bool	trunk;
+	u16	portv_trunkid;
+	u8	mac[ETH_ALEN];
+};
+
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
@@ -416,11 +427,11 @@ 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,
 			   const unsigned char *addr, u16 vid);
-int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
-			       unsigned char *addr, bool *is_static);
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg);
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
+int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port, u16 *vid,
+			       u8 addr[ETH_ALEN], bool *is_static);
 
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
-- 
2.4.6

[PATCH net-next v2 7/7] net: dsa: mv88e6xxx: rework FDB add/del operations

[Vivien Didelot]

Add a low level function for the ATU Load operation, and provide FDB add
and delete wrappers functions.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6171.c |   2 +
 drivers/net/dsa/mv88e6352.c |   2 +
 drivers/net/dsa/mv88e6xxx.c | 110 +++++++++++++++++++++++++++++---------------
 drivers/net/dsa/mv88e6xxx.h |   8 ++--
 4 files changed, 80 insertions(+), 42 deletions(-)

diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index b99fa50..735f04c 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -116,6 +116,8 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.port_join_bridge       = mv88e6xxx_join_bridge,
 	.port_leave_bridge      = mv88e6xxx_leave_bridge,
 	.port_stp_update        = mv88e6xxx_port_stp_update,
+	.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 0a77135..191fb25 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -341,6 +341,8 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.port_join_bridge	= mv88e6xxx_join_bridge,
 	.port_leave_bridge	= mv88e6xxx_leave_bridge,
 	.port_stp_update	= mv88e6xxx_port_stp_update,
+	.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 6cad168..39203bb 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1214,59 +1214,42 @@ static int _mv88e6xxx_atu_mac_read(struct dsa_switch *ds, u8 addr[ETH_ALEN])
 	return 0;
 }
 
-static int __mv88e6xxx_port_fdb_cmd(struct dsa_switch *ds, int port,
-				    const unsigned char *addr, int state)
+static int _mv88e6xxx_atu_load(struct dsa_switch *ds,
+			       struct mv88e6xxx_atu_entry *entry)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	u8 fid = ps->fid[port];
+	u16 reg = 0;
 	int ret;
 
 	ret = _mv88e6xxx_atu_wait(ds);
 	if (ret < 0)
 		return ret;
 
-	ret = _mv88e6xxx_atu_mac_write(ds, addr);
+	ret = _mv88e6xxx_atu_mac_write(ds, entry->mac);
 	if (ret < 0)
 		return ret;
 
-	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_ATU_DATA,
-				   (0x10 << port) | state);
-	if (ret)
-		return ret;
+	if (entry->state != GLOBAL_ATU_DATA_STATE_UNUSED) {
+		unsigned int mask, shift;
 
-	ret = _mv88e6xxx_atu_cmd(ds, fid, GLOBAL_ATU_OP_LOAD_DB);
+		if (entry->trunk) {
+			reg |= GLOBAL_ATU_DATA_TRUNK;
+			mask = GLOBAL_ATU_DATA_TRUNK_ID_MASK;
+			shift = GLOBAL_ATU_DATA_TRUNK_ID_SHIFT;
+		} else {
+			mask = GLOBAL_ATU_DATA_PORT_VECTOR_MASK;
+			shift = GLOBAL_ATU_DATA_PORT_VECTOR_SHIFT;
+		}
 
-	return ret;
-}
+		reg |= (entry->portv_trunkid << shift) & mask;
+	}
 
-int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
-			   const unsigned char *addr, u16 vid)
-{
-	int state = is_multicast_ether_addr(addr) ?
-		GLOBAL_ATU_DATA_STATE_MC_STATIC :
-		GLOBAL_ATU_DATA_STATE_UC_STATIC;
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret;
+	reg |= entry->state & GLOBAL_ATU_DATA_STATE_MASK;
 
-	mutex_lock(&ps->smi_mutex);
-	ret = __mv88e6xxx_port_fdb_cmd(ds, port, addr, state);
-	mutex_unlock(&ps->smi_mutex);
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_ATU_DATA, reg);
+	if (ret < 0)
+		return ret;
 
-	return ret;
-}
-
-int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
-			   const unsigned char *addr, u16 vid)
-{
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
-	int ret;
-
-	mutex_lock(&ps->smi_mutex);
-	ret = __mv88e6xxx_port_fdb_cmd(ds, port, addr,
-				       GLOBAL_ATU_DATA_STATE_UNUSED);
-	mutex_unlock(&ps->smi_mutex);
-
-	return ret;
+	return _mv88e6xxx_atu_cmd(ds, entry->fid, GLOBAL_ATU_OP_LOAD_DB);
 }
 
 static int _mv88e6xxx_atu_getnext(struct dsa_switch *ds, u16 fid,
@@ -1329,6 +1312,57 @@ static int _mv88e6xxx_port_vid_to_fid(struct dsa_switch *ds, int port, u16 vid)
 	return -ENOENT;
 }
 
+static int _mv88e6xxx_port_fdb_load(struct dsa_switch *ds, int port, u16 vid,
+				    const u8 addr[ETH_ALEN], u8 state)
+{
+	struct mv88e6xxx_atu_entry entry = { 0 };
+	int ret;
+
+	ret = _mv88e6xxx_port_vid_to_fid(ds, port, vid);
+	if (ret < 0)
+		return ret;
+
+	entry.fid = ret;
+	entry.state = state;
+	ether_addr_copy(entry.mac, addr);
+	if (state != GLOBAL_ATU_DATA_STATE_UNUSED) {
+		entry.trunk = false;
+		entry.portv_trunkid = BIT(port);
+	}
+
+	return _mv88e6xxx_atu_load(ds, &entry);
+}
+
+int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, u16 vid,
+			   const u8 addr[ETH_ALEN])
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	u8 state = is_multicast_ether_addr(addr) ?
+		GLOBAL_ATU_DATA_STATE_MC_STATIC :
+		GLOBAL_ATU_DATA_STATE_UC_STATIC;
+	int ret;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_port_fdb_load(ds, port, vid, addr, state);
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port, u16 vid,
+			   const u8 addr[ETH_ALEN])
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	u8 state = GLOBAL_ATU_DATA_STATE_UNUSED;
+	int ret;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_port_fdb_load(ds, port, vid, addr, state);
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
 int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port, u16 *vid,
 			       u8 addr[ETH_ALEN], bool *is_static)
 {
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index d9bd6a0..1821095 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -423,13 +423,13 @@ int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 int mv88e6xxx_join_bridge(struct dsa_switch *ds, int port, u32 br_port_mask);
 int mv88e6xxx_leave_bridge(struct dsa_switch *ds, int port, u32 br_port_mask);
 int mv88e6xxx_port_stp_update(struct dsa_switch *ds, int port, u8 state);
-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,
-			   const unsigned char *addr, u16 vid);
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg);
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
+int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, u16 vid,
+			   const u8 addr[ETH_ALEN]);
+int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port, u16 vid,
+			   const u8 addr[ETH_ALEN]);
 int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port, u16 *vid,
 			       u8 addr[ETH_ALEN], bool *is_static);
 
-- 
2.4.6

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu,  6 Aug 2015 01:44:01 -0400

> This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
> objects.

Series applied, thanks.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Vivien Didelot]

Hi David,

On 15-08-09 22:48:22, David Miller wrote:
> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Thu,  6 Aug 2015 01:44:01 -0400
> 
> > This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
> > objects.
> 
> Series applied, thanks.

I noticed you didn't push the serie yet. I've just sent the v3 which
includes the switchdev change (ndm_state) mentioned by Scott and the
reordering of commits to improve bisectability, as suggested by Andrew.

Please consider the v3 "[PATCH net-next v3 0/8] net: dsa: mv88e6xxx:
support switchdev FDB objects" instead.

Thanks,
-v

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Vivien Didelot]

Hi David,

On Aug 10, 2015, at 9:39 AM, Vivien Didelot vivien.didelot@savoirfairelinux.com wrote:

> Hi David,
> 
> On 15-08-09 22:48:22, David Miller wrote:
>> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> Date: Thu,  6 Aug 2015 01:44:01 -0400
>> 
>> > This patchset refactors the DSA and mv88e6xxx code to use the switchdev FDB
>> > objects.
>> 
>> Series applied, thanks.
> 
> I noticed you didn't push the serie yet. I've just sent the v3 which
> includes the switchdev change (ndm_state) mentioned by Scott and the
> reordering of commits to improve bisectability, as suggested by Andrew.
> 
> Please consider the v3 "[PATCH net-next v3 0/8] net: dsa: mv88e6xxx:
> support switchdev FDB objects" instead.

Somehow this message was ignored or seen too late, and v2 got pushed in
the net-next tree.

v2 introduces an uneeded patch to convert switchdev fdb address (1/7);
an is_static member in switchdev that Scott didn't fully agree on (2/7);
and the calls into the driver are removed in 3/7 and added later in the
patchset, making it hard to bisect, as mentioned by Andrew.

v3 fixes all of that, Scott acked the switchdev change and Andrew
reviewed the whole patchset. It is indeed more readable and simpler:

v2: 10 files changed, 317 insertions(+), 197 deletions(-)
v3: 9 files changed, 260 insertions(+), 129 deletions(-)

I can work on fixup patches to restore v3 changes on top of v2, but this
won't fix the bisectability issue.

Instead of fixing individual portions, reverting the merge commit
f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
series at once, then v3 can be merged on top of it.

Can you consider this as an option?

Thanks,
-v

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 11 Aug 2015 12:25:06 -0400 (EDT)

> I can work on fixup patches to restore v3 changes on top of v2, but this
> won't fix the bisectability issue.
> 
> Instead of fixing individual portions, reverting the merge commit
> f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
> series at once, then v3 can be merged on top of it.
> 
> Can you consider this as an option?

Nothing will fix bisectability, so don't try.

Reverting an entire series when you have the fix available
already is excessive.

So as I have already asked you, send a relative fixup to clear
up this situation.

Thanks.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Florian Fainelli]

On 11/08/15 10:38, David Miller wrote:
> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Tue, 11 Aug 2015 12:25:06 -0400 (EDT)
> 
>> I can work on fixup patches to restore v3 changes on top of v2, but this
>> won't fix the bisectability issue.
>>
>> Instead of fixing individual portions, reverting the merge commit
>> f1d5ca4: "Merge branch 'mv88e6xxx-switchdev-fdb'" would undo all the v2
>> series at once, then v3 can be merged on top of it.
>>
>> Can you consider this as an option?
> 
> Nothing will fix bisectability, so don't try.
> 
> Reverting an entire series when you have the fix available
> already is excessive.
> 
> So as I have already asked you, send a relative fixup to clear
> up this situation.

What if the fix is to actually not break bisectability? Put differently,
my question is how do you value not rewriting history vs. breaking
bisectability (by accident of course)?
-- 
Florian

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 11 Aug 2015 11:03:35 -0700

> Put differently, my question is how do you value not rewriting
> history vs. breaking bisectability (by accident of course)?

I never will rewrite history, ever.

Too many people clone my tree and depend upon it.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Vivien Didelot]

Hi David,

On Aug 11, 2015, at 2:07 PM, David davem@davemloft.net wrote:

> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Tue, 11 Aug 2015 11:03:35 -0700
> 
>> Put differently, my question is how do you value not rewriting
>> history vs. breaking bisectability (by accident of course)?
> 
> I never will rewrite history, ever.
> 
> Too many people clone my tree and depend upon it.

Sorry, I still don't understand. What are the consequences of:

    git revert -m 1 f1d5ca4

Then applying v3?

You already did that in the past:
https://github.com/torvalds/linux/commit/1f2cd84

Thanks,
-v

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Tue, 11 Aug 2015 14:18:42 -0400 (EDT)

> On Aug 11, 2015, at 2:07 PM, David davem@davemloft.net wrote:
> 
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Date: Tue, 11 Aug 2015 11:03:35 -0700
>> 
>>> Put differently, my question is how do you value not rewriting
>>> history vs. breaking bisectability (by accident of course)?
>> 
>> I never will rewrite history, ever.
>> 
>> Too many people clone my tree and depend upon it.
> 
> Sorry, I still don't understand. What are the consequences of:
> 
>     git revert -m 1 f1d5ca4
> 
> Then applying v3?

In this scenerio I think a relative fixup works better.

> You already did that in the past:
> https://github.com/torvalds/linux/commit/1f2cd84

Each and every situation is evaluated by me on a case by case
basis.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 11 Aug 2015 11:52:49 -0700 (PDT)

> From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Date: Tue, 11 Aug 2015 14:18:42 -0400 (EDT)
> 
>> On Aug 11, 2015, at 2:07 PM, David davem@davemloft.net wrote:
>> 
>>> From: Florian Fainelli <f.fainelli@gmail.com>
>>> Date: Tue, 11 Aug 2015 11:03:35 -0700
>>> 
>>>> Put differently, my question is how do you value not rewriting
>>>> history vs. breaking bisectability (by accident of course)?
>>> 
>>> I never will rewrite history, ever.
>>> 
>>> Too many people clone my tree and depend upon it.
>> 
>> Sorry, I still don't understand. What are the consequences of:
>> 
>>     git revert -m 1 f1d5ca4
>> 
>> Then applying v3?
> 
> In this scenerio I think a relative fixup works better.
> 
>> You already did that in the past:
>> https://github.com/torvalds/linux/commit/1f2cd84
> 
> Each and every situation is evaluated by me on a case by case
> basis.

Ok, if you guys really want me to I'll do the revert-reapply thing.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 11 Aug 2015 12:00:27 -0700 (PDT)

> Ok, if you guys really want me to I'll do the revert-reapply thing.

Done.

Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: support switchdev FDB objects

[Vivien Didelot]

Hi David,

On 15-08-11 12:05:18, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 11 Aug 2015 12:00:27 -0700 (PDT)
> 
> > Ok, if you guys really want me to I'll do the revert-reapply thing.
> 
> Done.

Thank you, this is much appreciated.

-v