[PATCH net-next 0/3] net: dsa: add MDB support

[PATCH net-next 0/3] net: dsa: add MDB support

[Vivien Didelot]

This patchset adds the switchdev MDB object support to the DSA layer.

The MDB support for the mv88e6xxx driver is very similar to the FDB
support. The FDB operations care about unicast addresses while the MDB
operations care about multicast addresses.

Both operation set load/purge/dump the Address Translation Table (ATU),
thus common code is used.

Vivien Didelot (3):
  net: dsa: add MDB support
  net: dsa: mv88e6xxx: make switchdev DB ops generic
  net: dsa: mv88e6xxx: add MDB support

 Documentation/networking/dsa/dsa.txt |  23 +++++
 drivers/net/dsa/mv88e6xxx/chip.c     | 161 ++++++++++++++++++++++++++---------
 include/net/dsa.h                    |  16 ++++
 net/dsa/slave.c                      |  55 ++++++++++++
 4 files changed, 213 insertions(+), 42 deletions(-)

-- 
2.9.3

[PATCH net-next 1/3] net: dsa: add MDB support

[Vivien Didelot]

Add SWITCHDEV_OBJ_ID_PORT_MDB support to the DSA layer.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/networking/dsa/dsa.txt | 23 +++++++++++++++
 include/net/dsa.h                    | 16 +++++++++++
 net/dsa/slave.c                      | 55 ++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)

diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
index 44ed453..6db7bc8 100644
--- a/Documentation/networking/dsa/dsa.txt
+++ b/Documentation/networking/dsa/dsa.txt
@@ -584,6 +584,29 @@ of DSA, would be the its port-based VLAN, used by the associated bridge device.
   function that the driver has to call for each MAC address known to be behind
   the given port. A switchdev object is used to carry the VID and FDB info.
 
+- port_mdb_prepare: bridge layer function invoked when the bridge prepares the
+  installation of a multicast group database entry. If the operation is not
+  supported, this function should return -EOPNOTSUPP to inform the bridge code
+  to fallback to a software implementation. No hardware setup must be done in
+  this function. See port_fdb_add for this and details.
+
+- port_mdb_add: bridge layer function invoked when the bridge wants to install 
+  a multicast group database entry, the switch hardware should be programmed 
+  with the specified address in the specified VLAN ID in the forwarding database 
+  associated with this VLAN ID.
+
+Note: VLAN ID 0 corresponds to the port private database, which, in the context
+of DSA, would be the its port-based VLAN, used by the associated bridge device.
+
+- port_mdb_del: bridge layer function invoked when the bridge wants to remove a
+  multicast group database entry, the switch hardware should be programmed to 
+  delete the specified MAC address from the specified VLAN ID if it was mapped 
+  into this port forwarding database.
+
+- port_mdb_dump: bridge layer function invoked with a switchdev callback
+  function that the driver has to call for each MAC address known to be behind
+  the given port. A switchdev object is used to carry the VID and MDB info.
+
 TODO
 ====
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 2ebeba4..39f90c4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -234,6 +234,7 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 struct switchdev_trans;
 struct switchdev_obj;
 struct switchdev_obj_port_fdb;
+struct switchdev_obj_port_mdb;
 struct switchdev_obj_port_vlan;
 
 struct dsa_switch_ops {
@@ -369,6 +370,21 @@ struct dsa_switch_ops {
 	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
 				 struct switchdev_obj_port_fdb *fdb,
 				 int (*cb)(struct switchdev_obj *obj));
+
+	/*
+	 * Multicast group database
+	 */
+	int	(*port_mdb_prepare)(struct dsa_switch *ds, int port,
+				    const struct switchdev_obj_port_mdb *mdb,
+				    struct switchdev_trans *trans);
+	void	(*port_mdb_add)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_mdb *mdb,
+				struct switchdev_trans *trans);
+	int	(*port_mdb_del)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_mdb *mdb);
+	int	(*port_mdb_dump)(struct dsa_switch *ds, int port,
+				 struct switchdev_obj_port_mdb *mdb,
+				 int (*cb)(struct switchdev_obj *obj));
 };
 
 void register_switch_driver(struct dsa_switch_ops *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9f6c2a2..9ecbe78 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -290,6 +290,50 @@ static int dsa_slave_port_fdb_dump(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
+static int dsa_slave_port_mdb_add(struct net_device *dev,
+				  const struct switchdev_obj_port_mdb *mdb,
+				  struct switchdev_trans *trans)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (switchdev_trans_ph_prepare(trans)) {
+		if (!ds->ops->port_mdb_prepare || !ds->ops->port_mdb_add)
+			return -EOPNOTSUPP;
+
+		return ds->ops->port_mdb_prepare(ds, p->port, mdb, trans);
+	}
+
+	ds->ops->port_mdb_add(ds, p->port, mdb, trans);
+
+	return 0;
+}
+
+static int dsa_slave_port_mdb_del(struct net_device *dev,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->ops->port_mdb_del)
+		return ds->ops->port_mdb_del(ds, p->port, mdb);
+
+	return -EOPNOTSUPP;
+}
+
+static int dsa_slave_port_mdb_dump(struct net_device *dev,
+				   struct switchdev_obj_port_mdb *mdb,
+				   switchdev_obj_dump_cb_t *cb)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->ops->port_mdb_dump)
+		return ds->ops->port_mdb_dump(ds, p->port, mdb, cb);
+
+	return -EOPNOTSUPP;
+}
+
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -412,6 +456,10 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 					     SWITCHDEV_OBJ_PORT_FDB(obj),
 					     trans);
 		break;
+	case SWITCHDEV_OBJ_ID_PORT_MDB:
+		err = dsa_slave_port_mdb_add(dev, SWITCHDEV_OBJ_PORT_MDB(obj),
+					     trans);
+		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_slave_port_vlan_add(dev,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj),
@@ -435,6 +483,9 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 		err = dsa_slave_port_fdb_del(dev,
 					     SWITCHDEV_OBJ_PORT_FDB(obj));
 		break;
+	case SWITCHDEV_OBJ_ID_PORT_MDB:
+		err = dsa_slave_port_mdb_del(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
+		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_slave_port_vlan_del(dev,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj));
@@ -459,6 +510,10 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
 					      SWITCHDEV_OBJ_PORT_FDB(obj),
 					      cb);
 		break;
+	case SWITCHDEV_OBJ_ID_PORT_MDB:
+		err = dsa_slave_port_mdb_dump(dev, SWITCHDEV_OBJ_PORT_MDB(obj),
+					      cb);
+		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		err = dsa_slave_port_vlan_dump(dev,
 					       SWITCHDEV_OBJ_PORT_VLAN(obj),
-- 
2.9.3

Re: [PATCH net-next 1/3] net: dsa: add MDB support

[Andrew Lunn]

On Mon, Aug 29, 2016 at 04:32:44PM -0400, Vivien Didelot wrote:
> Add SWITCHDEV_OBJ_ID_PORT_MDB support to the DSA layer.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  Documentation/networking/dsa/dsa.txt | 23 +++++++++++++++
>  include/net/dsa.h                    | 16 +++++++++++
>  net/dsa/slave.c                      | 55 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)
> 
> diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
> index 44ed453..6db7bc8 100644
> --- a/Documentation/networking/dsa/dsa.txt
> +++ b/Documentation/networking/dsa/dsa.txt
> @@ -584,6 +584,29 @@ of DSA, would be the its port-based VLAN, used by the associated bridge device.
>    function that the driver has to call for each MAC address known to be behind
>    the given port. A switchdev object is used to carry the VID and FDB info.
>  
> +- port_mdb_prepare: bridge layer function invoked when the bridge prepares the
> +  installation of a multicast group database entry.

Hi Vivien

Terminology question. This function is used to add a multicast MAC
address to the switch tables. I've always considered a multicast group
as an IP layer thing. But this documentation is also considering a
multicast group to be a layer 2 thing.

Does the bridge code use multicast group when referring to L2?

     Thanks
	Andrew

Re: [PATCH net-next 1/3] net: dsa: add MDB support

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Mon, Aug 29, 2016 at 04:32:44PM -0400, Vivien Didelot wrote:
>> Add SWITCHDEV_OBJ_ID_PORT_MDB support to the DSA layer.
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  Documentation/networking/dsa/dsa.txt | 23 +++++++++++++++
>>  include/net/dsa.h                    | 16 +++++++++++
>>  net/dsa/slave.c                      | 55 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 94 insertions(+)
>> 
>> diff --git a/Documentation/networking/dsa/dsa.txt b/Documentation/networking/dsa/dsa.txt
>> index 44ed453..6db7bc8 100644
>> --- a/Documentation/networking/dsa/dsa.txt
>> +++ b/Documentation/networking/dsa/dsa.txt
>> @@ -584,6 +584,29 @@ of DSA, would be the its port-based VLAN, used by the associated bridge device.
>>    function that the driver has to call for each MAC address known to be behind
>>    the given port. A switchdev object is used to carry the VID and FDB info.
>>  
>> +- port_mdb_prepare: bridge layer function invoked when the bridge prepares the
>> +  installation of a multicast group database entry.
>
> Terminology question. This function is used to add a multicast MAC
> address to the switch tables. I've always considered a multicast group
> as an IP layer thing. But this documentation is also considering a
> multicast group to be a layer 2 thing.
>
> Does the bridge code use multicast group when referring to L2?

The switchdev MDB attribute is very similary to the FDB attribute. It
contains the VLAN ID and the Ethernet MAC address of the group.

Thanks,

        Vivien

Re: [PATCH net-next 1/3] net: dsa: add MDB support

[Andrew Lunn]

> > Does the bridge code use multicast group when referring to L2?
> 
> The switchdev MDB attribute is very similary to the FDB attribute. It
> contains the VLAN ID and the Ethernet MAC address of the group.

Hi Vivien

I'm just trying to avoid the use of 'multicast group' for a L2 entity,
unless it is already widespread used so in the bridge code. If the
bridge code does consider L2 a 'multciast group', fine, lets document
it so. But if the bridge code only uses 'multciast group' to mean L3,
we should not use it here for L2.

       Andrew

Re: [PATCH net-next 1/3] net: dsa: add MDB support

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> > Does the bridge code use multicast group when referring to L2?
>> 
>> The switchdev MDB attribute is very similary to the FDB attribute. It
>> contains the VLAN ID and the Ethernet MAC address of the group.
>
> I'm just trying to avoid the use of 'multicast group' for a L2 entity,
> unless it is already widespread used so in the bridge code. If the
> bridge code does consider L2 a 'multciast group', fine, lets document
> it so. But if the bridge code only uses 'multciast group' to mean L3,
> we should not use it here for L2.

I am not quite sure about that. I can see that SWITCHDEV_OBJ_ID_PORT_MDB
is only crafted in net/bridge/br_mdb.c:__br_mdb_notify().

What should we do if the bridge code consider a multicast group as L3?

Thanks,

        Vivien

Re: [PATCH net-next 1/3] net: dsa: add MDB support

[Andrew Lunn]

> What should we do if the bridge code consider a multicast group as L3?

Hi Vivien

Just drop the word 'group' from the Documentation. And there appears
to be one comment in the code which should be changed.

   Andrew

[PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

The MDB support for the mv88e6xxx driver will be very similar to the FDB
support, since it consists of loading/purging/dumping address to/from
the Address Translation Unit (ATU).

Prepare the support for MDB by making the FDB code accessing the ATU
generic. The FDB operations now provide access to the unicast addresses
while the MDB operations will provide access to the multicast addresses.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 750d01d..93abfff 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2093,9 +2093,9 @@ static int _mv88e6xxx_atu_load(struct mv88e6xxx_chip *chip,
 	return _mv88e6xxx_atu_cmd(chip, entry->fid, GLOBAL_ATU_OP_LOAD_DB);
 }
 
-static int _mv88e6xxx_port_fdb_load(struct mv88e6xxx_chip *chip, int port,
-				    const unsigned char *addr, u16 vid,
-				    u8 state)
+static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
+					const unsigned char *addr, u16 vid,
+					u8 state)
 {
 	struct mv88e6xxx_atu_entry entry = { 0 };
 	struct mv88e6xxx_vtu_stu_entry vlan;
@@ -2134,15 +2134,12 @@ static void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 				   const struct switchdev_obj_port_fdb *fdb,
 				   struct switchdev_trans *trans)
 {
-	int state = is_multicast_ether_addr(fdb->addr) ?
-		GLOBAL_ATU_DATA_STATE_MC_STATIC :
-		GLOBAL_ATU_DATA_STATE_UC_STATIC;
 	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
 
 	mutex_lock(&chip->reg_lock);
-	if (_mv88e6xxx_port_fdb_load(chip, port, fdb->addr, fdb->vid, state))
-		netdev_err(ds->ports[port].netdev,
-			   "failed to load MAC address\n");
+	if (mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+					 GLOBAL_ATU_DATA_STATE_UC_STATIC))
+		netdev_err(ds->ports[port].netdev, "failed to load unicast MAC address\n");
 	mutex_unlock(&chip->reg_lock);
 }
 
@@ -2150,14 +2147,14 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 				  const struct switchdev_obj_port_fdb *fdb)
 {
 	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
-	int ret;
+	int err;
 
 	mutex_lock(&chip->reg_lock);
-	ret = _mv88e6xxx_port_fdb_load(chip, port, fdb->addr, fdb->vid,
-				       GLOBAL_ATU_DATA_STATE_UNUSED);
+	err = mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+					   GLOBAL_ATU_DATA_STATE_UNUSED);
 	mutex_unlock(&chip->reg_lock);
 
-	return ret;
+	return err;
 }
 
 static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip *chip, u16 fid,
@@ -2205,10 +2202,10 @@ static int _mv88e6xxx_atu_getnext(struct mv88e6xxx_chip *chip, u16 fid,
 	return 0;
 }
 
-static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
-					u16 fid, u16 vid, int port,
-					struct switchdev_obj_port_fdb *fdb,
-					int (*cb)(struct switchdev_obj *obj))
+static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
+				      u16 fid, u16 vid, int port,
+				      struct switchdev_obj *obj,
+				      int (*cb)(struct switchdev_obj *obj))
 {
 	struct mv88e6xxx_atu_entry addr = {
 		.mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
@@ -2222,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
 	do {
 		err = _mv88e6xxx_atu_getnext(chip, fid, &addr);
 		if (err)
-			break;
+			return err;
 
 		if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
 			break;
 
-		if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
-			bool is_static = addr.state ==
-				(is_multicast_ether_addr(addr.mac) ?
-				 GLOBAL_ATU_DATA_STATE_MC_STATIC :
-				 GLOBAL_ATU_DATA_STATE_UC_STATIC);
+		if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
+			continue;
+
+		if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
+			struct switchdev_obj_port_fdb *fdb;
 
+			if (!is_unicast_ether_addr(addr.mac))
+				continue;
+
+			fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
 			fdb->vid = vid;
 			ether_addr_copy(fdb->addr, addr.mac);
-			fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
-
-			err = cb(&fdb->obj);
-			if (err)
-				break;
+			if (addr.state == GLOBAL_ATU_DATA_STATE_UC_STATIC)
+				fdb->ndm_state = NUD_NOARP;
+			else
+				fdb->ndm_state = NUD_REACHABLE;
 		}
+
+		err = cb(obj);
+		if (err)
+			return err;
 	} while (!is_broadcast_ether_addr(addr.mac));
 
 	return err;
 }
 
-static int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,
-				   struct switchdev_obj_port_fdb *fdb,
-				   int (*cb)(struct switchdev_obj *obj))
+static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
+				  struct switchdev_obj *obj,
+				  int (*cb)(struct switchdev_obj *obj))
 {
-	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
 	struct mv88e6xxx_vtu_stu_entry vlan = {
 		.vid = GLOBAL_VTU_VID_MASK, /* all ones */
 	};
 	u16 fid;
 	int err;
 
-	mutex_lock(&chip->reg_lock);
-
 	/* Dump port's default Filtering Information Database (VLAN ID 0) */
 	err = _mv88e6xxx_port_fid_get(chip, port, &fid);
 	if (err)
-		goto unlock;
+		return err;
 
-	err = _mv88e6xxx_port_fdb_dump_one(chip, fid, 0, port, fdb, cb);
+	err = mv88e6xxx_port_db_dump_one(chip, fid, 0, port, obj, cb);
 	if (err)
-		goto unlock;
+		return err;
 
 	/* Dump VLANs' Filtering Information Databases */
 	err = _mv88e6xxx_vtu_vid_write(chip, vlan.vid);
 	if (err)
-		goto unlock;
+		return err;
 
 	do {
 		err = _mv88e6xxx_vtu_getnext(chip, &vlan);
 		if (err)
-			break;
+			return err;
 
 		if (!vlan.valid)
 			break;
 
-		err = _mv88e6xxx_port_fdb_dump_one(chip, vlan.fid, vlan.vid,
-						   port, fdb, cb);
+		err = mv88e6xxx_port_db_dump_one(chip, vlan.fid, vlan.vid, port,
+						 obj, cb);
 		if (err)
-			break;
+			return err;
 	} while (vlan.vid < GLOBAL_VTU_VID_MASK);
 
-unlock:
+	return err;
+}
+
+static int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,
+				   struct switchdev_obj_port_fdb *fdb,
+				   int (*cb)(struct switchdev_obj *obj))
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_db_dump(chip, port, &fdb->obj, cb);
 	mutex_unlock(&chip->reg_lock);
 
 	return err;
-- 
2.9.3

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Andrew Lunn]

Hi Vivien

> -static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
> -					u16 fid, u16 vid, int port,
> -					struct switchdev_obj_port_fdb *fdb,
> -					int (*cb)(struct switchdev_obj *obj))
> +static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
> +				      u16 fid, u16 vid, int port,
> +				      struct switchdev_obj *obj,
> +				      int (*cb)(struct switchdev_obj *obj))
>  {
>  	struct mv88e6xxx_atu_entry addr = {
>  		.mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
> @@ -2222,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
>  	do {
>  		err = _mv88e6xxx_atu_getnext(chip, fid, &addr);
>  		if (err)
> -			break;
> +			return err;
>  
>  		if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
>  			break;
>  
> -		if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
> -			bool is_static = addr.state ==
> -				(is_multicast_ether_addr(addr.mac) ?
> -				 GLOBAL_ATU_DATA_STATE_MC_STATIC :
> -				 GLOBAL_ATU_DATA_STATE_UC_STATIC);
> +		if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
> +			continue;
> +
> +		if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
> +			struct switchdev_obj_port_fdb *fdb;
>  
> +			if (!is_unicast_ether_addr(addr.mac))
> +				continue;
> +
> +			fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
>  			fdb->vid = vid;
>  			ether_addr_copy(fdb->addr, addr.mac);
> -			fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
> -
> -			err = cb(&fdb->obj);
> -			if (err)
> -				break;
> +			if (addr.state == GLOBAL_ATU_DATA_STATE_UC_STATIC)
> +				fdb->ndm_state = NUD_NOARP;
> +			else
> +				fdb->ndm_state = NUD_REACHABLE;
>  		}
> +
> +		err = cb(obj);
> +		if (err)
> +			return err;
>  	} while (!is_broadcast_ether_addr(addr.mac));

Humm, maybe i'm reading this patch wrong....

This function is called mv88e6xxx_port_db_dump_one(). But i don't see
a way out of the while loop, after dumping one. It seems to dump the
whole table until it reaches the end marker, which is the MAC
broadcast address.

Should we rename this function, drop the _one?

       Andrew

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Hi Vivien
>
>> -static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
>> -					u16 fid, u16 vid, int port,
>> -					struct switchdev_obj_port_fdb *fdb,
>> -					int (*cb)(struct switchdev_obj *obj))
>> +static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>> +				      u16 fid, u16 vid, int port,
>> +				      struct switchdev_obj *obj,
>> +				      int (*cb)(struct switchdev_obj *obj))
>>  {
>>  	struct mv88e6xxx_atu_entry addr = {
>>  		.mac = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
>> @@ -2222,72 +2219,87 @@ static int _mv88e6xxx_port_fdb_dump_one(struct mv88e6xxx_chip *chip,
>>  	do {
>>  		err = _mv88e6xxx_atu_getnext(chip, fid, &addr);
>>  		if (err)
>> -			break;
>> +			return err;
>>  
>>  		if (addr.state == GLOBAL_ATU_DATA_STATE_UNUSED)
>>  			break;
>>  
>> -		if (!addr.trunk && addr.portv_trunkid & BIT(port)) {
>> -			bool is_static = addr.state ==
>> -				(is_multicast_ether_addr(addr.mac) ?
>> -				 GLOBAL_ATU_DATA_STATE_MC_STATIC :
>> -				 GLOBAL_ATU_DATA_STATE_UC_STATIC);
>> +		if (addr.trunk || (addr.portv_trunkid & BIT(port)) == 0)
>> +			continue;
>> +
>> +		if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>> +			struct switchdev_obj_port_fdb *fdb;
>>  
>> +			if (!is_unicast_ether_addr(addr.mac))
>> +				continue;
>> +
>> +			fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
>>  			fdb->vid = vid;
>>  			ether_addr_copy(fdb->addr, addr.mac);
>> -			fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
>> -
>> -			err = cb(&fdb->obj);
>> -			if (err)
>> -				break;
>> +			if (addr.state == GLOBAL_ATU_DATA_STATE_UC_STATIC)
>> +				fdb->ndm_state = NUD_NOARP;
>> +			else
>> +				fdb->ndm_state = NUD_REACHABLE;
>>  		}
>> +
>> +		err = cb(obj);
>> +		if (err)
>> +			return err;
>>  	} while (!is_broadcast_ether_addr(addr.mac));
>
> Humm, maybe i'm reading this patch wrong....
>
> This function is called mv88e6xxx_port_db_dump_one(). But i don't see
> a way out of the while loop, after dumping one. It seems to dump the
> whole table until it reaches the end marker, which is the MAC
> broadcast address.
>
> Should we rename this function, drop the _one?

No. mv88e6xxx_port_db_dump already exists, this is the function called
mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
(forwarding database). 88E6352 have 4096 FIDs.

The way to dump a single FID is to run the ATU GetNext operation
starting with ff:ff:ff:ff:ff:ff, until that same address is reached
again. This is what mv88e6xxx_port_db_dump_one does.

mv88e6xxx_port_db_dump first dump the port's assigned FID (i.e. VID 0),
then iterate on active VLANs to get and dump their FIDs.

Thanks,

        Vivien

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Andrew Lunn]

> No. mv88e6xxx_port_db_dump already exists, this is the function called
> mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
> (forwarding database). 88E6352 have 4096 FIDs.

So maybe s/_one/_fid/ ?

   Andrew

Re: [PATCH net-next 2/3] net: dsa: mv88e6xxx: make switchdev DB ops generic

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

>> No. mv88e6xxx_port_db_dump already exists, this is the function called
>> mv88e6xxx_port_db_dump_one multiple time. Here, _one refers to an FID
>> (forwarding database). 88E6352 have 4096 FIDs.
>
> So maybe s/_one/_fid/ ?

If I have to respin this series, sure.

Thanks Andrew,

       Vivien

[PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Vivien Didelot]

Add support for the MDB operations. This consists of
loading/purging/dumping multicast addresses for a given port in the ATU.

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

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 93abfff..812cb47 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
 				fdb->ndm_state = NUD_NOARP;
 			else
 				fdb->ndm_state = NUD_REACHABLE;
+		} else {
+			struct switchdev_obj_port_mdb *mdb;
+
+			if (!is_multicast_ether_addr(addr.mac))
+				continue;
+
+			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
+			mdb->vid = vid;
+			ether_addr_copy(mdb->addr, addr.mac);
 		}
 
 		err = cb(obj);
@@ -3988,6 +3997,58 @@ free:
 	return NULL;
 }
 
+static int mv88e6xxx_port_mdb_prepare(struct dsa_switch *ds, int port,
+				      const struct switchdev_obj_port_mdb *mdb,
+				      struct switchdev_trans *trans)
+{
+	/* We don't need any dynamic resource from the kernel (yet),
+	 * so skip the prepare phase.
+	 */
+
+	return 0;
+}
+
+static void mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
+				   const struct switchdev_obj_port_mdb *mdb,
+				   struct switchdev_trans *trans)
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+
+	mutex_lock(&chip->reg_lock);
+	if (mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
+					 GLOBAL_ATU_DATA_STATE_MC_STATIC))
+		netdev_err(ds->ports[port].netdev, "failed to load multicast MAC address\n");
+	mutex_unlock(&chip->reg_lock);
+}
+
+static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
+				  const struct switchdev_obj_port_mdb *mdb)
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid,
+					   GLOBAL_ATU_DATA_STATE_UNUSED);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
+				   struct switchdev_obj_port_mdb *mdb,
+				   int (*cb)(struct switchdev_obj *obj))
+{
+	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
+	int err;
+
+	mutex_lock(&chip->reg_lock);
+	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
 static struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.probe			= mv88e6xxx_drv_probe,
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
@@ -4023,6 +4084,10 @@ static struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_fdb_add           = mv88e6xxx_port_fdb_add,
 	.port_fdb_del           = mv88e6xxx_port_fdb_del,
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
+	.port_mdb_prepare       = mv88e6xxx_port_mdb_prepare,
+	.port_mdb_add           = mv88e6xxx_port_mdb_add,
+	.port_mdb_del           = mv88e6xxx_port_mdb_del,
+	.port_mdb_dump          = mv88e6xxx_port_mdb_dump,
 };
 
 static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip,
-- 
2.9.3

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Andrew Lunn]

Hi Vivien

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 93abfff..812cb47 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>  				fdb->ndm_state = NUD_NOARP;
>  			else
>  				fdb->ndm_state = NUD_REACHABLE;
> +		} else {

Rather than else, i think it would be safer to do

		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
> +			struct switchdev_obj_port_mdb *mdb;
> +
> +			if (!is_multicast_ether_addr(addr.mac))
> +				continue;
> +
> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
> +			mdb->vid = vid;
> +			ether_addr_copy(mdb->addr, addr.mac);
>  		}

It should not happen, but the day it does, we get very confused...

> +static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
> +				   struct switchdev_obj_port_mdb *mdb,
> +				   int (*cb)(struct switchdev_obj *obj))
> +{
> +	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
> +	int err;
> +
> +	mutex_lock(&chip->reg_lock);
> +	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return err;
> +}

Isn't this identical to mv88e6xxx_port_fdb_dump()? Maybe we should
just have one function, and register it twice?

     Andrew

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> Hi Vivien
>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 93abfff..812cb47 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>>  				fdb->ndm_state = NUD_NOARP;
>>  			else
>>  				fdb->ndm_state = NUD_REACHABLE;
>> +		} else {
>
> Rather than else, i think it would be safer to do
>
> 		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>> +			struct switchdev_obj_port_mdb *mdb;
>> +
>> +			if (!is_multicast_ether_addr(addr.mac))
>> +				continue;
>> +
>> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
>> +			mdb->vid = vid;
>> +			ether_addr_copy(mdb->addr, addr.mac);
>>  		}
>
> It should not happen, but the day it does, we get very confused...

Do you mean the something like this?

    if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
        ...
    } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
        ...
    } else {
        return -EOPNOTSUPP;
    }

I'm OK with that if you think it is better.

>> +static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
>> +				   struct switchdev_obj_port_mdb *mdb,
>> +				   int (*cb)(struct switchdev_obj *obj))
>> +{
>> +	struct mv88e6xxx_chip *chip = ds_to_priv(ds);
>> +	int err;
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +	err = mv88e6xxx_port_db_dump(chip, port, &mdb->obj, cb);
>> +	mutex_unlock(&chip->reg_lock);
>> +
>> +	return err;
>> +}
>
> Isn't this identical to mv88e6xxx_port_fdb_dump()? Maybe we should
> just have one function, and register it twice?

No the signatures are differentes. See _fdb vs. _mdb. They are basically
the same thing (at least, from the switch driver's point of view).

We can abstract the FDB (unicast and multicast) related operations
directly in the DSA layer, but I think it is better to map directly the
switchdev obj operations for the moment...

What do you guys think?

Thanks,

        Vivien

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Andrew Lunn]

> Do you mean the something like this?
> 
>     if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>         ...
>     } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>         ...
>     } else {
>         return -EOPNOTSUPP;
>     }

Hi Vivien

The -ENONOTSUPP is even better. I was not going as far as that.
 
> No the signatures are differentes. See _fdb vs. _mdb.

Ah, missed that. Then two functions are O.K. I don't think we should
consolidate it in the DSA layer.

	    Andrew

	    

Re: [PATCH net-next 3/3] net: dsa: mv88e6xxx: add MDB support

[Sergei Shtylyov]

Hello.

On 08/31/2016 05:46 PM, Vivien Didelot wrote:

>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>>> index 93abfff..812cb47 100644
>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>>> @@ -2240,6 +2240,15 @@ static int mv88e6xxx_port_db_dump_one(struct mv88e6xxx_chip *chip,
>>>  				fdb->ndm_state = NUD_NOARP;
>>>  			else
>>>  				fdb->ndm_state = NUD_REACHABLE;
>>> +		} else {
>>
>> Rather than else, i think it would be safer to do
>>
>> 		if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>>> +			struct switchdev_obj_port_mdb *mdb;
>>> +
>>> +			if (!is_multicast_ether_addr(addr.mac))
>>> +				continue;
>>> +
>>> +			mdb = SWITCHDEV_OBJ_PORT_MDB(obj);
>>> +			mdb->vid = vid;
>>> +			ether_addr_copy(mdb->addr, addr.mac);
>>>  		}
>>
>> It should not happen, but the day it does, we get very confused...
>
> Do you mean the something like this?
>
>     if (obj->id == SWITCHDEV_OBJ_ID_PORT_FDB) {
>         ...
>     } else if (obj->id == SWITCHDEV_OBJ_ID_PORT_MDB) {
>         ...
>     } else {
>         return -EOPNOTSUPP;
>     }
>
> I'm OK with that if you think it is better.

    Just code it as a *switch*, please. :-)

[...]

MBR, Sergei