[PATCH net-next RFC 00/12] Change DSA's FDB API and perform switchdev cleanup

[PATCH net-next RFC 00/12] Change DSA's FDB API and perform switchdev cleanup

[Arkadi Sharshevsky]

The patchset moves the DSA driver into learning static FDB entries via
the switchdev notification chain rather then by using bridge bypass SELF
flag. 

The DSA drivers cannot sync the software bridge with hardware learned
entries and use the switchdev's implementation of bypass FDB dumping.
Because they are the only ones using this functionality, the fdb_dump
implementation is moved from switchdev code into DSA.

Finally after this changes a major cleanup in switchdev can be done.

Arkadi Sharshevsky (12):
  net: dsa: Change DSA slave FDB API to be switchdev independent
  net: dsa: Remove prepare phase for FDB
  net: dsa: Remove switchdev dependency from DSA switch notifier chain
  net: dsa: Add ordered workqueue
  net: dsa: Add support for learning FDB through notification
  net: dsa: Remove support for FDB add/del via SELF
  net: dsa: Add support for querying supported bridge flags
  net: dsa: Remove support for bypass bridge port attributes/vlan set
  net: dsa: Remove redundant MDB dump support
  net: dsa: Move FDB dump implementation inside DSA
  net: bridge: Remove FDB deletion through switchdev object
  net: switchdev: Remove bridge bypass support from switchdev

 drivers/net/dsa/b53/b53_common.c       |  86 +-----
 drivers/net/dsa/b53/b53_priv.h         |  16 +-
 drivers/net/dsa/bcm_sf2.c              |   1 -
 drivers/net/dsa/dsa_loop.c             |  38 ---
 drivers/net/dsa/microchip/ksz_common.c | 118 ++------
 drivers/net/dsa/mt7530.c               |  46 +--
 drivers/net/dsa/mv88e6xxx/chip.c       | 149 ++--------
 drivers/net/dsa/qca8k.c                |  41 +--
 include/net/dsa.h                      |  34 +--
 include/net/switchdev.h                |  87 ------
 net/bridge/br_fdb.c                    |  18 --
 net/dsa/dsa.c                          |  13 +
 net/dsa/dsa_priv.h                     |  21 +-
 net/dsa/port.c                         |  51 +---
 net/dsa/slave.c                        | 237 ++++++++++++---
 net/dsa/switch.c                       |  21 +-
 net/switchdev/switchdev.c              | 519 ---------------------------------
 17 files changed, 342 insertions(+), 1154 deletions(-)

-- 
2.4.11

[PATCH net-next RFC 01/12] net: dsa: Change DSA slave FDB API to be switchdev independent

[Arkadi Sharshevsky]

In order to support FDB add/del to be on a notifier chain the slave
API need to be changed to be switchdev independent.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/b53/b53_common.c       | 12 +++++-------
 drivers/net/dsa/b53/b53_priv.h         |  8 +++-----
 drivers/net/dsa/microchip/ksz_common.c | 34 ++++++++++++++++------------------
 drivers/net/dsa/mt7530.c               | 14 ++++++--------
 drivers/net/dsa/mv88e6xxx/chip.c       | 12 +++++-------
 drivers/net/dsa/qca8k.c                | 15 ++++++---------
 include/net/dsa.h                      |  8 +++-----
 net/dsa/switch.c                       |  8 +++++---
 8 files changed, 49 insertions(+), 62 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index e68d368..d0156dc 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1214,8 +1214,7 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 }
 
 int b53_fdb_prepare(struct dsa_switch *ds, int port,
-		    const struct switchdev_obj_port_fdb *fdb,
-		    struct switchdev_trans *trans)
+		    const unsigned char *addr, u16 vid)
 {
 	struct b53_device *priv = ds->priv;
 
@@ -1230,22 +1229,21 @@ int b53_fdb_prepare(struct dsa_switch *ds, int port,
 EXPORT_SYMBOL(b53_fdb_prepare);
 
 void b53_fdb_add(struct dsa_switch *ds, int port,
-		 const struct switchdev_obj_port_fdb *fdb,
-		 struct switchdev_trans *trans)
+		 const unsigned char *addr, u16 vid)
 {
 	struct b53_device *priv = ds->priv;
 
-	if (b53_arl_op(priv, 0, port, fdb->addr, fdb->vid, true))
+	if (b53_arl_op(priv, 0, port, addr, vid, true))
 		pr_err("%s: failed to add MAC address\n", __func__);
 }
 EXPORT_SYMBOL(b53_fdb_add);
 
 int b53_fdb_del(struct dsa_switch *ds, int port,
-		const struct switchdev_obj_port_fdb *fdb)
+		const unsigned char *addr, u16 vid)
 {
 	struct b53_device *priv = ds->priv;
 
-	return b53_arl_op(priv, 0, port, fdb->addr, fdb->vid, false);
+	return b53_arl_op(priv, 0, port, addr, vid, false);
 }
 EXPORT_SYMBOL(b53_fdb_del);
 
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 155a9c4..d417bca 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -397,13 +397,11 @@ int b53_vlan_dump(struct dsa_switch *ds, int port,
 		  struct switchdev_obj_port_vlan *vlan,
 		  switchdev_obj_dump_cb_t *cb);
 int b53_fdb_prepare(struct dsa_switch *ds, int port,
-		    const struct switchdev_obj_port_fdb *fdb,
-		    struct switchdev_trans *trans);
+		    const unsigned char *addr, u16 vid);
 void b53_fdb_add(struct dsa_switch *ds, int port,
-		 const struct switchdev_obj_port_fdb *fdb,
-		 struct switchdev_trans *trans);
+		 const unsigned char *addr, u16 vid);
 int b53_fdb_del(struct dsa_switch *ds, int port,
-		const struct switchdev_obj_port_fdb *fdb);
+		const unsigned char *addr, u16 vid);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
 		 struct switchdev_obj_port_fdb *fdb,
 		 switchdev_obj_dump_cb_t *cb);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b313ecd..db82808 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -679,8 +679,7 @@ static int ksz_port_vlan_dump(struct dsa_switch *ds, int port,
 }
 
 static int ksz_port_fdb_prepare(struct dsa_switch *ds, int port,
-				const struct switchdev_obj_port_fdb *fdb,
-				struct switchdev_trans *trans)
+				const unsigned char *addr, u16 vid)
 {
 	/* nothing needed */
 
@@ -707,8 +706,7 @@ struct alu_struct {
 };
 
 static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
-			     const struct switchdev_obj_port_fdb *fdb,
-			     struct switchdev_trans *trans)
+			     const unsigned char *addr, u16 vid)
 {
 	struct ksz_device *dev = ds->priv;
 	u32 alu_table[4];
@@ -717,12 +715,12 @@ static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
 	mutex_lock(&dev->alu_mutex);
 
 	/* find any entry with mac & vid */
-	data = fdb->vid << ALU_FID_INDEX_S;
-	data |= ((fdb->addr[0] << 8) | fdb->addr[1]);
+	data = vid << ALU_FID_INDEX_S;
+	data |= ((addr[0] << 8) | addr[1]);
 	ksz_write32(dev, REG_SW_ALU_INDEX_0, data);
 
-	data = ((fdb->addr[2] << 24) | (fdb->addr[3] << 16));
-	data |= ((fdb->addr[4] << 8) | fdb->addr[5]);
+	data = ((addr[2] << 24) | (addr[3] << 16));
+	data |= ((addr[4] << 8) | addr[5]);
 	ksz_write32(dev, REG_SW_ALU_INDEX_1, data);
 
 	/* start read operation */
@@ -740,12 +738,12 @@ static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
 	/* update ALU entry */
 	alu_table[0] = ALU_V_STATIC_VALID;
 	alu_table[1] |= BIT(port);
-	if (fdb->vid)
+	if (vid)
 		alu_table[1] |= ALU_V_USE_FID;
-	alu_table[2] = (fdb->vid << ALU_V_FID_S);
-	alu_table[2] |= ((fdb->addr[0] << 8) | fdb->addr[1]);
-	alu_table[3] = ((fdb->addr[2] << 24) | (fdb->addr[3] << 16));
-	alu_table[3] |= ((fdb->addr[4] << 8) | fdb->addr[5]);
+	alu_table[2] = (vid << ALU_V_FID_S);
+	alu_table[2] |= ((addr[0] << 8) | addr[1]);
+	alu_table[3] = ((addr[2] << 24) | (addr[3] << 16));
+	alu_table[3] |= ((addr[4] << 8) | addr[5]);
 
 	write_table(ds, alu_table);
 
@@ -760,7 +758,7 @@ static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
 }
 
 static int ksz_port_fdb_del(struct dsa_switch *ds, int port,
-			    const struct switchdev_obj_port_fdb *fdb)
+			    const unsigned char *addr, u16 vid)
 {
 	struct ksz_device *dev = ds->priv;
 	u32 alu_table[4];
@@ -770,12 +768,12 @@ static int ksz_port_fdb_del(struct dsa_switch *ds, int port,
 	mutex_lock(&dev->alu_mutex);
 
 	/* read any entry with mac & vid */
-	data = fdb->vid << ALU_FID_INDEX_S;
-	data |= ((fdb->addr[0] << 8) | fdb->addr[1]);
+	data = vid << ALU_FID_INDEX_S;
+	data |= ((addr[0] << 8) | addr[1]);
 	ksz_write32(dev, REG_SW_ALU_INDEX_0, data);
 
-	data = ((fdb->addr[2] << 24) | (fdb->addr[3] << 16));
-	data |= ((fdb->addr[4] << 8) | fdb->addr[5]);
+	data = ((addr[2] << 24) | (addr[3] << 16));
+	data |= ((addr[4] << 8) | addr[5]);
 	ksz_write32(dev, REG_SW_ALU_INDEX_1, data);
 
 	/* start read operation */
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1e46418..430e3ab 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -802,8 +802,7 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 
 static int
 mt7530_port_fdb_prepare(struct dsa_switch *ds, int port,
-			const struct switchdev_obj_port_fdb *fdb,
-			struct switchdev_trans *trans)
+			const unsigned char *addr, u16 vid)
 {
 	struct mt7530_priv *priv = ds->priv;
 	int ret;
@@ -813,7 +812,7 @@ mt7530_port_fdb_prepare(struct dsa_switch *ds, int port,
 	 * is called while the entry is still available.
 	 */
 	mutex_lock(&priv->reg_mutex);
-	mt7530_fdb_write(priv, fdb->vid, 0, fdb->addr, -1, STATIC_ENT);
+	mt7530_fdb_write(priv, vid, 0, addr, -1, STATIC_ENT);
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
 	mutex_unlock(&priv->reg_mutex);
 
@@ -822,28 +821,27 @@ mt7530_port_fdb_prepare(struct dsa_switch *ds, int port,
 
 static void
 mt7530_port_fdb_add(struct dsa_switch *ds, int port,
-		    const struct switchdev_obj_port_fdb *fdb,
-		    struct switchdev_trans *trans)
+		    const unsigned char *addr, u16 vid)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u8 port_mask = BIT(port);
 
 	mutex_lock(&priv->reg_mutex);
-	mt7530_fdb_write(priv, fdb->vid, port_mask, fdb->addr, -1, STATIC_ENT);
+	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
 	mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
 	mutex_unlock(&priv->reg_mutex);
 }
 
 static int
 mt7530_port_fdb_del(struct dsa_switch *ds, int port,
-		    const struct switchdev_obj_port_fdb *fdb)
+		    const unsigned char *addr, u16 vid)
 {
 	struct mt7530_priv *priv = ds->priv;
 	int ret;
 	u8 port_mask = BIT(port);
 
 	mutex_lock(&priv->reg_mutex);
-	mt7530_fdb_write(priv, fdb->vid, port_mask, fdb->addr, -1, STATIC_EMP);
+	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_EMP);
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
 	mutex_unlock(&priv->reg_mutex);
 
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 53b0881..4d4358d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1436,8 +1436,7 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 }
 
 static int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
-				      const struct switchdev_obj_port_fdb *fdb,
-				      struct switchdev_trans *trans)
+				      const unsigned char *addr, u16 vid)
 {
 	/* We don't need any dynamic resource from the kernel (yet),
 	 * so skip the prepare phase.
@@ -1446,13 +1445,12 @@ static int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
 }
 
 static void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
-				   const struct switchdev_obj_port_fdb *fdb,
-				   struct switchdev_trans *trans)
+				   const unsigned char *addr, u16 vid)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
 	mutex_lock(&chip->reg_lock);
-	if (mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+	if (mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					 MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC))
 		dev_err(ds->dev, "p%d: failed to load unicast MAC address\n",
 			port);
@@ -1460,13 +1458,13 @@ static void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 }
 
 static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
-				  const struct switchdev_obj_port_fdb *fdb)
+				  const unsigned char *addr, u16 vid)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
 	mutex_lock(&chip->reg_lock);
-	err = mv88e6xxx_port_db_load_purge(chip, port, fdb->addr, fdb->vid,
+	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
 					   MV88E6XXX_G1_ATU_DATA_STATE_UNUSED);
 	mutex_unlock(&chip->reg_lock);
 
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b3bee7e..913d76a 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -830,8 +830,7 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 
 static int
 qca8k_port_fdb_prepare(struct dsa_switch *ds, int port,
-		       const struct switchdev_obj_port_fdb *fdb,
-		       struct switchdev_trans *trans)
+		       const unsigned char *addr, u16 vid)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 
@@ -840,33 +839,31 @@ qca8k_port_fdb_prepare(struct dsa_switch *ds, int port,
 	 * when port_fdb_add is called an entry is still available. Otherwise
 	 * the last free entry might have been used up by auto learning
 	 */
-	return qca8k_port_fdb_insert(priv, fdb->addr, 0, fdb->vid);
+	return qca8k_port_fdb_insert(priv, addr, 0, vid);
 }
 
 static void
 qca8k_port_fdb_add(struct dsa_switch *ds, int port,
-		   const struct switchdev_obj_port_fdb *fdb,
-		   struct switchdev_trans *trans)
+		   const unsigned char *addr, u16 vid)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
 
 	/* Update the FDB entry adding the port_mask */
-	qca8k_port_fdb_insert(priv, fdb->addr, port_mask, fdb->vid);
+	qca8k_port_fdb_insert(priv, addr, port_mask, vid);
 }
 
 static int
 qca8k_port_fdb_del(struct dsa_switch *ds, int port,
-		   const struct switchdev_obj_port_fdb *fdb)
+		   const unsigned char *addr, u16 vid)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
-	u16 vid = fdb->vid;
 
 	if (!vid)
 		vid = 1;
 
-	return qca8k_fdb_del(priv, fdb->addr, port_mask, vid);
+	return qca8k_fdb_del(priv, addr, port_mask, vid);
 }
 
 static int
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 58969b9..54fb8f80 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -392,13 +392,11 @@ struct dsa_switch_ops {
 	 * Forwarding database
 	 */
 	int	(*port_fdb_prepare)(struct dsa_switch *ds, int port,
-				    const struct switchdev_obj_port_fdb *fdb,
-				    struct switchdev_trans *trans);
+				    const unsigned char *addr, u16 vid);
 	void	(*port_fdb_add)(struct dsa_switch *ds, int port,
-				const struct switchdev_obj_port_fdb *fdb,
-				struct switchdev_trans *trans);
+				const unsigned char *addr, u16 vid);
 	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
-				const struct switchdev_obj_port_fdb *fdb);
+				const unsigned char *addr, u16 vid);
 	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
 				 struct switchdev_obj_port_fdb *fdb,
 				  switchdev_obj_dump_cb_t *cb);
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 97e2e9c..a9edfba 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -94,10 +94,11 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
 		if (!ds->ops->port_fdb_prepare || !ds->ops->port_fdb_add)
 			return -EOPNOTSUPP;
 
-		return ds->ops->port_fdb_prepare(ds, info->port, fdb, trans);
+		return ds->ops->port_fdb_prepare(ds, info->port, fdb->addr,
+						 fdb->vid);
 	}
 
-	ds->ops->port_fdb_add(ds, info->port, fdb, trans);
+	ds->ops->port_fdb_add(ds, info->port, fdb->addr, fdb->vid);
 
 	return 0;
 }
@@ -114,7 +115,8 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_del)
 		return -EOPNOTSUPP;
 
-	return ds->ops->port_fdb_del(ds, info->port, fdb);
+	return ds->ops->port_fdb_del(ds, info->port, fdb->addr,
+				     fdb->vid);
 }
 
 static int dsa_switch_mdb_add(struct dsa_switch *ds,
-- 
2.4.11

Re: [PATCH net-next RFC 01/12] net: dsa: Change DSA slave FDB API to be switchdev independent

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> In order to support FDB add/del to be on a notifier chain the slave
> API need to be changed to be switchdev independent.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
> Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next RFC 02/12] net: dsa: Remove prepare phase for FDB

[Arkadi Sharshevsky]

The prepare phase for FDB add is unneeded because most of DSA devices
can have failures during bus transactions (SPI, I2C, etc.), thus, the
prepare phase cannot guarantee success of the commit stage.

The support for learning FDB through notification chain, which will be
introduced in the following patches, will provide the ability to notify
back the bridge about successful offload.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 drivers/net/dsa/b53/b53_common.c       | 17 +++--------------
 drivers/net/dsa/b53/b53_priv.h         |  6 ++----
 drivers/net/dsa/microchip/ksz_common.c | 24 ++++++++++--------------
 drivers/net/dsa/mt7530.c               | 25 ++++---------------------
 drivers/net/dsa/mv88e6xxx/chip.c       | 23 +++++++----------------
 drivers/net/dsa/qca8k.c                | 18 +-----------------
 include/net/dsa.h                      |  4 +---
 net/dsa/dsa_priv.h                     |  4 +---
 net/dsa/port.c                         |  4 +---
 net/dsa/slave.c                        |  4 +++-
 net/dsa/switch.c                       | 14 +++-----------
 11 files changed, 36 insertions(+), 107 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index d0156dc..c414b43 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1213,8 +1213,8 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
 	return b53_arl_rw_op(dev, 0);
 }
 
-int b53_fdb_prepare(struct dsa_switch *ds, int port,
-		    const unsigned char *addr, u16 vid)
+int b53_fdb_add(struct dsa_switch *ds, int port,
+		const unsigned char *addr, u16 vid)
 {
 	struct b53_device *priv = ds->priv;
 
@@ -1224,17 +1224,7 @@ int b53_fdb_prepare(struct dsa_switch *ds, int port,
 	if (is5325(priv) || is5365(priv))
 		return -EOPNOTSUPP;
 
-	return 0;
-}
-EXPORT_SYMBOL(b53_fdb_prepare);
-
-void b53_fdb_add(struct dsa_switch *ds, int port,
-		 const unsigned char *addr, u16 vid)
-{
-	struct b53_device *priv = ds->priv;
-
-	if (b53_arl_op(priv, 0, port, addr, vid, true))
-		pr_err("%s: failed to add MAC address\n", __func__);
+	return b53_arl_op(priv, 0, port, addr, vid, true);
 }
 EXPORT_SYMBOL(b53_fdb_add);
 
@@ -1563,7 +1553,6 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.port_vlan_add		= b53_vlan_add,
 	.port_vlan_del		= b53_vlan_del,
 	.port_vlan_dump		= b53_vlan_dump,
-	.port_fdb_prepare	= b53_fdb_prepare,
 	.port_fdb_dump		= b53_fdb_dump,
 	.port_fdb_add		= b53_fdb_add,
 	.port_fdb_del		= b53_fdb_del,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index d417bca..f29c892 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -396,10 +396,8 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 int b53_vlan_dump(struct dsa_switch *ds, int port,
 		  struct switchdev_obj_port_vlan *vlan,
 		  switchdev_obj_dump_cb_t *cb);
-int b53_fdb_prepare(struct dsa_switch *ds, int port,
-		    const unsigned char *addr, u16 vid);
-void b53_fdb_add(struct dsa_switch *ds, int port,
-		 const unsigned char *addr, u16 vid);
+int b53_fdb_add(struct dsa_switch *ds, int port,
+		const unsigned char *addr, u16 vid);
 int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index db82808..b55f364 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -678,14 +678,6 @@ static int ksz_port_vlan_dump(struct dsa_switch *ds, int port,
 	return err;
 }
 
-static int ksz_port_fdb_prepare(struct dsa_switch *ds, int port,
-				const unsigned char *addr, u16 vid)
-{
-	/* nothing needed */
-
-	return 0;
-}
-
 struct alu_struct {
 	/* entry 1 */
 	u8	is_static:1;
@@ -705,12 +697,13 @@ struct alu_struct {
 	u8	mac[ETH_ALEN];
 };
 
-static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
-			     const unsigned char *addr, u16 vid)
+static int ksz_port_fdb_add(struct dsa_switch *ds, int port,
+			    const unsigned char *addr, u16 vid)
 {
 	struct ksz_device *dev = ds->priv;
 	u32 alu_table[4];
 	u32 data;
+	int ret = 0;
 
 	mutex_lock(&dev->alu_mutex);
 
@@ -727,7 +720,8 @@ static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_READ | ALU_START);
 
 	/* wait to be finished */
-	if (wait_alu_ready(dev, ALU_START, 1000) < 0) {
+	ret = wait_alu_ready(dev, ALU_START, 1000);
+	if (ret < 0) {
 		dev_dbg(dev->dev, "Failed to read ALU\n");
 		goto exit;
 	}
@@ -750,11 +744,14 @@ static void ksz_port_fdb_add(struct dsa_switch *ds, int port,
 	ksz_write32(dev, REG_SW_ALU_CTRL__4, ALU_WRITE | ALU_START);
 
 	/* wait to be finished */
-	if (wait_alu_ready(dev, ALU_START, 1000) < 0)
-		dev_dbg(dev->dev, "Failed to read ALU\n");
+	ret = wait_alu_ready(dev, ALU_START, 1000);
+	if (ret < 0)
+		dev_dbg(dev->dev, "Failed to write ALU\n");
 
 exit:
 	mutex_unlock(&dev->alu_mutex);
+
+	return ret;
 }
 
 static int ksz_port_fdb_del(struct dsa_switch *ds, int port,
@@ -1128,7 +1125,6 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_vlan_add		= ksz_port_vlan_add,
 	.port_vlan_del		= ksz_port_vlan_del,
 	.port_vlan_dump		= ksz_port_vlan_dump,
-	.port_fdb_prepare	= ksz_port_fdb_prepare,
 	.port_fdb_dump		= ksz_port_fdb_dump,
 	.port_fdb_add		= ksz_port_fdb_add,
 	.port_fdb_del		= ksz_port_fdb_del,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 430e3ab..f92aae8 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -801,35 +801,19 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 }
 
 static int
-mt7530_port_fdb_prepare(struct dsa_switch *ds, int port,
-			const unsigned char *addr, u16 vid)
-{
-	struct mt7530_priv *priv = ds->priv;
-	int ret;
-
-	/* Because auto-learned entrie shares the same FDB table.
-	 * an entry is reserved with no port_mask to make sure fdb_add
-	 * is called while the entry is still available.
-	 */
-	mutex_lock(&priv->reg_mutex);
-	mt7530_fdb_write(priv, vid, 0, addr, -1, STATIC_ENT);
-	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
-	mutex_unlock(&priv->reg_mutex);
-
-	return ret;
-}
-
-static void
 mt7530_port_fdb_add(struct dsa_switch *ds, int port,
 		    const unsigned char *addr, u16 vid)
 {
 	struct mt7530_priv *priv = ds->priv;
+	int ret;
 	u8 port_mask = BIT(port);
 
 	mutex_lock(&priv->reg_mutex);
 	mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
-	mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
+	ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, 0);
 	mutex_unlock(&priv->reg_mutex);
+
+	return ret;
 }
 
 static int
@@ -1013,7 +997,6 @@ static struct dsa_switch_ops mt7530_switch_ops = {
 	.port_stp_state_set	= mt7530_stp_state_set,
 	.port_bridge_join	= mt7530_port_bridge_join,
 	.port_bridge_leave	= mt7530_port_bridge_leave,
-	.port_fdb_prepare	= mt7530_port_fdb_prepare,
 	.port_fdb_add		= mt7530_port_fdb_add,
 	.port_fdb_del		= mt7530_port_fdb_del,
 	.port_fdb_dump		= mt7530_port_fdb_dump,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4d4358d..fba68c8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1435,26 +1435,18 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_g1_atu_loadpurge(chip, vlan.fid, &entry);
 }
 
-static int mv88e6xxx_port_fdb_prepare(struct dsa_switch *ds, int port,
-				      const unsigned char *addr, u16 vid)
-{
-	/* We don't need any dynamic resource from the kernel (yet),
-	 * so skip the prepare phase.
-	 */
-	return 0;
-}
-
-static void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
-				   const unsigned char *addr, u16 vid)
+static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
+				  const unsigned char *addr, u16 vid)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	int err;
 
 	mutex_lock(&chip->reg_lock);
-	if (mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
-					 MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC))
-		dev_err(ds->dev, "p%d: failed to load unicast MAC address\n",
-			port);
+	err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
+					   MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
 	mutex_unlock(&chip->reg_lock);
+
+	return err;
 }
 
 static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
@@ -3867,7 +3859,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_vlan_add		= mv88e6xxx_port_vlan_add,
 	.port_vlan_del		= mv88e6xxx_port_vlan_del,
 	.port_vlan_dump		= mv88e6xxx_port_vlan_dump,
-	.port_fdb_prepare       = mv88e6xxx_port_fdb_prepare,
 	.port_fdb_add           = mv88e6xxx_port_fdb_add,
 	.port_fdb_del           = mv88e6xxx_port_fdb_del,
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 913d76a..9c3de3d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -829,28 +829,13 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
 }
 
 static int
-qca8k_port_fdb_prepare(struct dsa_switch *ds, int port,
-		       const unsigned char *addr, u16 vid)
-{
-	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
-
-	/* The FDB table for static and auto learned entries is the same. We
-	 * need to reserve an entry with no port_mask set to make sure that
-	 * when port_fdb_add is called an entry is still available. Otherwise
-	 * the last free entry might have been used up by auto learning
-	 */
-	return qca8k_port_fdb_insert(priv, addr, 0, vid);
-}
-
-static void
 qca8k_port_fdb_add(struct dsa_switch *ds, int port,
 		   const unsigned char *addr, u16 vid)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	u16 port_mask = BIT(port);
 
-	/* Update the FDB entry adding the port_mask */
-	qca8k_port_fdb_insert(priv, addr, port_mask, vid);
+	return qca8k_port_fdb_insert(priv, addr, port_mask, vid);
 }
 
 static int
@@ -918,7 +903,6 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_stp_state_set	= qca8k_port_stp_state_set,
 	.port_bridge_join	= qca8k_port_bridge_join,
 	.port_bridge_leave	= qca8k_port_bridge_leave,
-	.port_fdb_prepare	= qca8k_port_fdb_prepare,
 	.port_fdb_add		= qca8k_port_fdb_add,
 	.port_fdb_del		= qca8k_port_fdb_del,
 	.port_fdb_dump		= qca8k_port_fdb_dump,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 54fb8f80..f054d41 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -391,9 +391,7 @@ struct dsa_switch_ops {
 	/*
 	 * Forwarding database
 	 */
-	int	(*port_fdb_prepare)(struct dsa_switch *ds, int port,
-				    const unsigned char *addr, u16 vid);
-	void	(*port_fdb_add)(struct dsa_switch *ds, int port,
+	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,
 				const unsigned char *addr, u16 vid);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 55982cc..428402f 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -44,7 +44,6 @@ struct dsa_notifier_bridge_info {
 /* DSA_NOTIFIER_FDB_* */
 struct dsa_notifier_fdb_info {
 	const struct switchdev_obj_port_fdb *fdb;
-	struct switchdev_trans *trans;
 	int sw_index;
 	int port;
 };
@@ -121,8 +120,7 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 			 struct switchdev_trans *trans);
 int dsa_port_fdb_add(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb,
-		     struct switchdev_trans *trans);
+		     const struct switchdev_obj_port_fdb *fdb);
 int dsa_port_fdb_del(struct dsa_port *dp,
 		     const struct switchdev_obj_port_fdb *fdb);
 int dsa_port_fdb_dump(struct dsa_port *dp, struct switchdev_obj_port_fdb *fdb,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index efc3bce..bd271b9 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -147,13 +147,11 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 }
 
 int dsa_port_fdb_add(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb,
-		     struct switchdev_trans *trans)
+		     const struct switchdev_obj_port_fdb *fdb)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
-		.trans = trans,
 		.fdb = fdb,
 	};
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9507bd3..b4e68b2 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -251,7 +251,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		err = dsa_port_fdb_add(dp, SWITCHDEV_OBJ_PORT_FDB(obj), trans);
+		if (switchdev_trans_ph_prepare(trans))
+			return 0;
+		err = dsa_port_fdb_add(dp, SWITCHDEV_OBJ_PORT_FDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index a9edfba..eb20e0f 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -84,23 +84,15 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
 			      struct dsa_notifier_fdb_info *info)
 {
 	const struct switchdev_obj_port_fdb *fdb = info->fdb;
-	struct switchdev_trans *trans = info->trans;
 
 	/* Do not care yet about other switch chips of the fabric */
 	if (ds->index != info->sw_index)
 		return 0;
 
-	if (switchdev_trans_ph_prepare(trans)) {
-		if (!ds->ops->port_fdb_prepare || !ds->ops->port_fdb_add)
-			return -EOPNOTSUPP;
-
-		return ds->ops->port_fdb_prepare(ds, info->port, fdb->addr,
-						 fdb->vid);
-	}
-
-	ds->ops->port_fdb_add(ds, info->port, fdb->addr, fdb->vid);
+	if (!ds->ops->port_fdb_add)
+		return -EOPNOTSUPP;
 
-	return 0;
+	return ds->ops->port_fdb_add(ds, info->port, fdb->addr, fdb->vid);
 }
 
 static int dsa_switch_fdb_del(struct dsa_switch *ds,
-- 
2.4.11

Re: [PATCH net-next RFC 02/12] net: dsa: Remove prepare phase for FDB

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> The prepare phase for FDB add is unneeded because most of DSA devices
> can have failures during bus transactions (SPI, I2C, etc.), thus, the
> prepare phase cannot guarantee success of the commit stage.
> 
> The support for learning FDB through notification chain, which will be
> introduced in the following patches, will provide the ability to notify
> back the bridge about successful offload.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next RFC 02/12] net: dsa: Remove prepare phase for FDB

[Vivien Didelot]

Arkadi Sharshevsky <arkadis@mellanox.com> writes:

> The prepare phase for FDB add is unneeded because most of DSA devices
> can have failures during bus transactions (SPI, I2C, etc.), thus, the
> prepare phase cannot guarantee success of the commit stage.
>
> The support for learning FDB through notification chain, which will be
> introduced in the following patches, will provide the ability to notify
> back the bridge about successful offload.
>
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

[PATCH net-next RFC 03/12] net: dsa: Remove switchdev dependency from DSA switch notifier chain

[Arkadi Sharshevsky]

Currently, the switchdev objects are embedded inside the DSA notifier
info. This patch removes this dependency. This is done as a preparation
stage before adding support for learning FDB through the switchdev
notification chain.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 net/dsa/dsa_priv.h | 11 ++++++-----
 net/dsa/port.c     | 15 +++++++++------
 net/dsa/slave.c    |  6 ++++--
 net/dsa/switch.c   | 11 ++++-------
 4 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 428402f..2b2f124 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -43,9 +43,10 @@ struct dsa_notifier_bridge_info {
 
 /* DSA_NOTIFIER_FDB_* */
 struct dsa_notifier_fdb_info {
-	const struct switchdev_obj_port_fdb *fdb;
 	int sw_index;
 	int port;
+	const unsigned char *addr;
+	u16 vid;
 };
 
 /* DSA_NOTIFIER_MDB_* */
@@ -119,10 +120,10 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering,
 			    struct switchdev_trans *trans);
 int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 			 struct switchdev_trans *trans);
-int dsa_port_fdb_add(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb);
-int dsa_port_fdb_del(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb);
+int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
+		     u16 vid);
+int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+		     u16 vid);
 int dsa_port_fdb_dump(struct dsa_port *dp, struct switchdev_obj_port_fdb *fdb,
 		      switchdev_obj_dump_cb_t *cb);
 int dsa_port_mdb_add(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index bd271b9..86e0585 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -146,25 +146,28 @@ int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock,
 	return dsa_port_notify(dp, DSA_NOTIFIER_AGEING_TIME, &info);
 }
 
-int dsa_port_fdb_add(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb)
+int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
+		     u16 vid)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
-		.fdb = fdb,
+		.addr = addr,
+		.vid = vid,
 	};
 
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_ADD, &info);
 }
 
-int dsa_port_fdb_del(struct dsa_port *dp,
-		     const struct switchdev_obj_port_fdb *fdb)
+int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
+		     u16 vid)
 {
 	struct dsa_notifier_fdb_info info = {
 		.sw_index = dp->ds->index,
 		.port = dp->index,
-		.fdb = fdb,
+		.addr = addr,
+		.vid = vid,
+
 	};
 
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_DEL, &info);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b4e68b2..19395cc 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -253,7 +253,8 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_FDB:
 		if (switchdev_trans_ph_prepare(trans))
 			return 0;
-		err = dsa_port_fdb_add(dp, SWITCHDEV_OBJ_PORT_FDB(obj));
+		err = dsa_port_fdb_add(dp, SWITCHDEV_OBJ_PORT_FDB(obj)->addr,
+				       SWITCHDEV_OBJ_PORT_FDB(obj)->vid);
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
@@ -279,7 +280,8 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		err = dsa_port_fdb_del(dp, SWITCHDEV_OBJ_PORT_FDB(obj));
+		err = dsa_port_fdb_del(dp, SWITCHDEV_OBJ_PORT_FDB(obj)->addr,
+				       SWITCHDEV_OBJ_PORT_FDB(obj)->vid);
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index eb20e0f..e6c06aa 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -83,8 +83,6 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,
 static int dsa_switch_fdb_add(struct dsa_switch *ds,
 			      struct dsa_notifier_fdb_info *info)
 {
-	const struct switchdev_obj_port_fdb *fdb = info->fdb;
-
 	/* Do not care yet about other switch chips of the fabric */
 	if (ds->index != info->sw_index)
 		return 0;
@@ -92,14 +90,13 @@ static int dsa_switch_fdb_add(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_add)
 		return -EOPNOTSUPP;
 
-	return ds->ops->port_fdb_add(ds, info->port, fdb->addr, fdb->vid);
+	return ds->ops->port_fdb_add(ds, info->port, info->addr,
+				     info->vid);
 }
 
 static int dsa_switch_fdb_del(struct dsa_switch *ds,
 			      struct dsa_notifier_fdb_info *info)
 {
-	const struct switchdev_obj_port_fdb *fdb = info->fdb;
-
 	/* Do not care yet about other switch chips of the fabric */
 	if (ds->index != info->sw_index)
 		return 0;
@@ -107,8 +104,8 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	if (!ds->ops->port_fdb_del)
 		return -EOPNOTSUPP;
 
-	return ds->ops->port_fdb_del(ds, info->port, fdb->addr,
-				     fdb->vid);
+	return ds->ops->port_fdb_del(ds, info->port, info->addr,
+				     info->vid);
 }
 
 static int dsa_switch_mdb_add(struct dsa_switch *ds,
-- 
2.4.11

Re: [PATCH net-next RFC 03/12] net: dsa: Remove switchdev dependency from DSA switch notifier chain

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> Currently, the switchdev objects are embedded inside the DSA notifier
> info. This patch removes this dependency. This is done as a preparation
> stage before adding support for learning FDB through the switchdev
> notification chain.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next RFC 03/12] net: dsa: Remove switchdev dependency from DSA switch notifier chain

[Vivien Didelot]

Arkadi Sharshevsky <arkadis@mellanox.com> writes:

> Currently, the switchdev objects are embedded inside the DSA notifier
> info. This patch removes this dependency. This is done as a preparation
> stage before adding support for learning FDB through the switchdev
> notification chain.
>
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

[PATCH net-next RFC 04/12] net: dsa: Add ordered workqueue

[Arkadi Sharshevsky]

This workqueue will be used for FDB add/del processing. It should
be destroyed after all devices unregistered successfully.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 include/net/dsa.h |  1 +
 net/dsa/dsa.c     | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f054d41..4835b0e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -451,6 +451,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
 struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
 
 struct net_device *dsa_dev_to_net_device(struct device *dev);
+bool dsa_schedule_work(struct work_struct *work);
 
 /* Keep inline for faster access in hot path */
 static inline bool netdev_uses_dsa(struct net_device *dev)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 416ac4e..9abe6dc 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -271,10 +271,22 @@ static struct packet_type dsa_pack_type __read_mostly = {
 	.func	= dsa_switch_rcv,
 };
 
+static struct workqueue_struct *dsa_owq;
+
+bool dsa_schedule_work(struct work_struct *work)
+{
+	return queue_work(dsa_owq, work);
+}
+
 static int __init dsa_init_module(void)
 {
 	int rc;
 
+	dsa_owq = alloc_ordered_workqueue("dsa_ordered",
+					  WQ_MEM_RECLAIM);
+	if (!dsa_owq)
+		return -ENOMEM;
+
 	rc = dsa_slave_register_notifier();
 	if (rc)
 		return rc;
@@ -294,6 +306,7 @@ static void __exit dsa_cleanup_module(void)
 	dsa_slave_unregister_notifier();
 	dev_remove_pack(&dsa_pack_type);
 	dsa_legacy_unregister();
+	destroy_workqueue(dsa_owq);
 }
 module_exit(dsa_cleanup_module);
 
-- 
2.4.11

Re: [PATCH net-next RFC 04/12] net: dsa: Add ordered workqueue

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> This workqueue will be used for FDB add/del processing. It should
> be destroyed after all devices unregistered successfully.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
> ---
>  include/net/dsa.h |  1 +
>  net/dsa/dsa.c     | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index f054d41..4835b0e 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -451,6 +451,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
>  
>  struct net_device *dsa_dev_to_net_device(struct device *dev);
> +bool dsa_schedule_work(struct work_struct *work);

I'd move this to dsa_priv.h in net/dsa/ because it is not supposed to be
used by DSA drivers.

You may also consider squashing this into the next patch since in itself
it's not used just yet.

>  
>  /* Keep inline for faster access in hot path */
>  static inline bool netdev_uses_dsa(struct net_device *dev)
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 416ac4e..9abe6dc 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -271,10 +271,22 @@ static struct packet_type dsa_pack_type __read_mostly = {
>  	.func	= dsa_switch_rcv,
>  };
>  
> +static struct workqueue_struct *dsa_owq;
> +
> +bool dsa_schedule_work(struct work_struct *work)
> +{
> +	return queue_work(dsa_owq, work);
> +}
> +
>  static int __init dsa_init_module(void)
>  {
>  	int rc;
>  
> +	dsa_owq = alloc_ordered_workqueue("dsa_ordered",
> +					  WQ_MEM_RECLAIM);
> +	if (!dsa_owq)
> +		return -ENOMEM;
> +
>  	rc = dsa_slave_register_notifier();
>  	if (rc)
>  		return rc;
> @@ -294,6 +306,7 @@ static void __exit dsa_cleanup_module(void)
>  	dsa_slave_unregister_notifier();
>  	dev_remove_pack(&dsa_pack_type);
>  	dsa_legacy_unregister();
> +	destroy_workqueue(dsa_owq);
>  }
>  module_exit(dsa_cleanup_module);
>  
> 


-- 
Florian

Re: [PATCH net-next RFC 04/12] net: dsa: Add ordered workqueue

[Vivien Didelot]

Hi Arkadi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>> This workqueue will be used for FDB add/del processing. It should
>> be destroyed after all devices unregistered successfully.
>> 
>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>> ---
>>  include/net/dsa.h |  1 +
>>  net/dsa/dsa.c     | 13 +++++++++++++
>>  2 files changed, 14 insertions(+)
>> 
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index f054d41..4835b0e 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -451,6 +451,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
>>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
>>  
>>  struct net_device *dsa_dev_to_net_device(struct device *dev);
>> +bool dsa_schedule_work(struct work_struct *work);
>
> I'd move this to dsa_priv.h in net/dsa/ because it is not supposed to be
> used by DSA drivers.
>
> You may also consider squashing this into the next patch since in itself
> it's not used just yet.

I agree with Florian on this. Even though it'll make the next patch a
bit bigger, it'll make it easier to understand the whole workqueue
processing logic.

Thanks,

        Vivien

Re: [PATCH net-next RFC 04/12] net: dsa: Add ordered workqueue

[Arkadi Sharshevsky]

On 07/06/2017 09:45 PM, Vivien Didelot wrote:
> Hi Arkadi,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>>> This workqueue will be used for FDB add/del processing. It should
>>> be destroyed after all devices unregistered successfully.
>>>
>>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>>> ---
>>>  include/net/dsa.h |  1 +
>>>  net/dsa/dsa.c     | 13 +++++++++++++
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>>> index f054d41..4835b0e 100644
>>> --- a/include/net/dsa.h
>>> +++ b/include/net/dsa.h
>>> @@ -451,6 +451,7 @@ void unregister_switch_driver(struct dsa_switch_driver *type);
>>>  struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
>>>  
>>>  struct net_device *dsa_dev_to_net_device(struct device *dev);
>>> +bool dsa_schedule_work(struct work_struct *work);
>>
>> I'd move this to dsa_priv.h in net/dsa/ because it is not supposed to be
>> used by DSA drivers.
>>
>> You may also consider squashing this into the next patch since in itself
>> it's not used just yet.
> 
> I agree with Florian on this. Even though it'll make the next patch a
> bit bigger, it'll make it easier to understand the whole workqueue
> processing logic.
> 
> Thanks,
> 
>         Vivien
> 

Ok no problem, will squash.
Thanks

[PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Arkadi Sharshevsky]

Add support for learning FDB through notification. The driver defers
the hardware update via ordered work queue. In case of a successful
FDB add a notification is sent back to bridge.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 2 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 19395cc..d0592f2 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+struct dsa_switchdev_event_work {
+	struct work_struct work;
+	struct switchdev_notifier_fdb_info fdb_info;
+	struct net_device *dev;
+	unsigned long event;
+};
+
+static void dsa_switchdev_event_work(struct work_struct *work)
+{
+	struct dsa_switchdev_event_work *switchdev_work =
+		container_of(work, struct dsa_switchdev_event_work, work);
+	struct net_device *dev = switchdev_work->dev;
+	struct switchdev_notifier_fdb_info *fdb_info;
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	int err;
+
+	rtnl_lock();
+	switch (switchdev_work->event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE:
+		fdb_info = &switchdev_work->fdb_info;
+		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
+		if (err) {
+			netdev_dbg(dev, "fdb add failed err=%d\n", err);
+			break;
+		}
+		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
+					 &fdb_info->info);
+		break;
+
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		fdb_info = &switchdev_work->fdb_info;
+		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
+		if (err)
+			netdev_dbg(dev, "fdb del failed err=%d\n", err);
+		break;
+	}
+	rtnl_unlock();
+
+	kfree(switchdev_work->fdb_info.addr);
+	kfree(switchdev_work);
+	dev_put(dev);
+}
+
+static int
+dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
+				  switchdev_work,
+				  const struct switchdev_notifier_fdb_info *
+				  fdb_info)
+{
+	memcpy(&switchdev_work->fdb_info, fdb_info,
+	       sizeof(switchdev_work->fdb_info));
+	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
+	if (!switchdev_work->fdb_info.addr)
+		return -ENOMEM;
+	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
+			fdb_info->addr);
+	return 0;
+}
+
+/* Called under rcu_read_lock() */
+static int dsa_slave_switchdev_event(struct notifier_block *unused,
+				     unsigned long event, void *ptr)
+{
+	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+	struct dsa_switchdev_event_work *switchdev_work;
+
+	if (!dsa_slave_dev_check(dev))
+		return NOTIFY_DONE;
+
+	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
+	if (!switchdev_work)
+		return NOTIFY_BAD;
+
+	INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work);
+	switchdev_work->dev = dev;
+	switchdev_work->event = event;
+
+	switch (event) {
+	case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */
+	case SWITCHDEV_FDB_DEL_TO_DEVICE:
+		if (dsa_slave_switchdev_fdb_work_init(switchdev_work,
+						      ptr))
+			goto err_fdb_work_init;
+		dev_hold(dev);
+		break;
+	default:
+		kfree(switchdev_work);
+		return NOTIFY_DONE;
+	}
+
+	dsa_schedule_work(&switchdev_work->work);
+	return NOTIFY_OK;
+
+err_fdb_work_init:
+	kfree(switchdev_work);
+	return NOTIFY_BAD;
+}
+
 static struct notifier_block dsa_slave_nb __read_mostly = {
-	.notifier_call	= dsa_slave_netdevice_event,
+	.notifier_call  = dsa_slave_netdevice_event,
+};
+
+static struct notifier_block dsa_slave_switchdev_notifier = {
+	.notifier_call = dsa_slave_switchdev_event,
 };
 
 int dsa_slave_register_notifier(void)
 {
-	return register_netdevice_notifier(&dsa_slave_nb);
+	int err;
+
+	err = register_netdevice_notifier(&dsa_slave_nb);
+	if (err)
+		return err;
+
+	err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
+	if (err)
+		goto err_register_switchdev;
+
+	return 0;
+
+err_register_switchdev:
+	unregister_netdevice_notifier(&dsa_slave_nb);
+	return err;
 }
 
 void dsa_slave_unregister_notifier(void)
 {
 	int err;
 
+	err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier);
+	if (err)
+		pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err);
+
 	err = unregister_netdevice_notifier(&dsa_slave_nb);
 	if (err)
 		pr_err("DSA: failed to unregister slave notifier (%d)\n", err);
-- 
2.4.11

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> Add support for learning FDB through notification. The driver defers
> the hardware update via ordered work queue. In case of a successful
> FDB add a notification is sent back to bridge.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
> ---
>  net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 2 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 19395cc..d0592f2 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +struct dsa_switchdev_event_work {
> +	struct work_struct work;
> +	struct switchdev_notifier_fdb_info fdb_info;
> +	struct net_device *dev;
> +	unsigned long event;
> +};
> +
> +static void dsa_switchdev_event_work(struct work_struct *work)
> +{
> +	struct dsa_switchdev_event_work *switchdev_work =
> +		container_of(work, struct dsa_switchdev_event_work, work);
> +	struct net_device *dev = switchdev_work->dev;
> +	struct switchdev_notifier_fdb_info *fdb_info;
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	int err;
> +
> +	rtnl_lock();
> +	switch (switchdev_work->event) {
> +	case SWITCHDEV_FDB_ADD_TO_DEVICE:
> +		fdb_info = &switchdev_work->fdb_info;

You could probably move this assignment outside of the switch()/case
statement since it is repeated

> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
> +		if (err) {
> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
> +			break;
> +		}
> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
> +					 &fdb_info->info);
> +		break;
> +
> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> +		fdb_info = &switchdev_work->fdb_info;
> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
> +		if (err)
> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);

OK I must have missed from the off-list discussion why we are not
calling the switchdev notifier here?

> +		break;
> +	}
> +	rtnl_unlock();
> +
> +	kfree(switchdev_work->fdb_info.addr);
> +	kfree(switchdev_work);
> +	dev_put(dev);
> +}
> +
> +static int
> +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
> +				  switchdev_work,
> +				  const struct switchdev_notifier_fdb_info *
> +				  fdb_info)
> +{
> +	memcpy(&switchdev_work->fdb_info, fdb_info,
> +	       sizeof(switchdev_work->fdb_info));
> +	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
> +	if (!switchdev_work->fdb_info.addr)
> +		return -ENOMEM;

Can't we have the fdb_info structure contain pre-allocated 6 bytes
storage for address, it's really silly to do this here and every time we
need to offload an FDB entry.

> +	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
> +			fdb_info->addr);
> +	return 0;
> +}
> +
> +/* Called under rcu_read_lock() */
> +static int dsa_slave_switchdev_event(struct notifier_block *unused,
> +				     unsigned long event, void *ptr)
> +{
> +	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
> +	struct dsa_switchdev_event_work *switchdev_work;
> +
> +	if (!dsa_slave_dev_check(dev))
> +		return NOTIFY_DONE;
> +
> +	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
> +	if (!switchdev_work)
> +		return NOTIFY_BAD;

Same here, can we try to re-use a heap allocated workqueue item and we
just re-initialize it when we have to?

> +
> +	INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work);
> +	switchdev_work->dev = dev;
> +	switchdev_work->event = event;
> +
> +	switch (event) {
> +	case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */
> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
> +		if (dsa_slave_switchdev_fdb_work_init(switchdev_work,
> +						      ptr))
> +			goto err_fdb_work_init;
> +		dev_hold(dev);
> +		break;
> +	default:
> +		kfree(switchdev_work);
> +		return NOTIFY_DONE;
> +	}
> +
> +	dsa_schedule_work(&switchdev_work->work);
> +	return NOTIFY_OK;
> +
> +err_fdb_work_init:
> +	kfree(switchdev_work);
> +	return NOTIFY_BAD;
> +}
> +
>  static struct notifier_block dsa_slave_nb __read_mostly = {
> -	.notifier_call	= dsa_slave_netdevice_event,
> +	.notifier_call  = dsa_slave_netdevice_event,
> +};
> +
> +static struct notifier_block dsa_slave_switchdev_notifier = {
> +	.notifier_call = dsa_slave_switchdev_event,
>  };
>  
>  int dsa_slave_register_notifier(void)
>  {
> -	return register_netdevice_notifier(&dsa_slave_nb);
> +	int err;
> +
> +	err = register_netdevice_notifier(&dsa_slave_nb);
> +	if (err)
> +		return err;
> +
> +	err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
> +	if (err)
> +		goto err_register_switchdev;

this label is a bit misnamed since it really deals with the slave dsa
notifier, how about no label or just renaming it "error_slave_nb"?

> +
> +	return 0;
> +
> +err_register_switchdev:
> +	unregister_netdevice_notifier(&dsa_slave_nb);
> +	return err;
>  }
>  
>  void dsa_slave_unregister_notifier(void)
>  {
>  	int err;
>  
> +	err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier);
> +	if (err)
> +		pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err);
> +
>  	err = unregister_netdevice_notifier(&dsa_slave_nb);
>  	if (err)
>  		pr_err("DSA: failed to unregister slave notifier (%d)\n", err);
> 


-- 
Florian

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Arkadi Sharshevsky]

On 07/05/2017 10:35 PM, Florian Fainelli wrote:
> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>> Add support for learning FDB through notification. The driver defers
>> the hardware update via ordered work queue. In case of a successful
>> FDB add a notification is sent back to bridge.
>>
>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>> ---
>>  net/dsa/slave.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 122 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index 19395cc..d0592f2 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -1263,19 +1263,139 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>  
>> +struct dsa_switchdev_event_work {
>> +	struct work_struct work;
>> +	struct switchdev_notifier_fdb_info fdb_info;
>> +	struct net_device *dev;
>> +	unsigned long event;
>> +};
>> +
>> +static void dsa_switchdev_event_work(struct work_struct *work)
>> +{
>> +	struct dsa_switchdev_event_work *switchdev_work =
>> +		container_of(work, struct dsa_switchdev_event_work, work);
>> +	struct net_device *dev = switchdev_work->dev;
>> +	struct switchdev_notifier_fdb_info *fdb_info;
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	int err;
>> +
>> +	rtnl_lock();
>> +	switch (switchdev_work->event) {
>> +	case SWITCHDEV_FDB_ADD_TO_DEVICE:
>> +		fdb_info = &switchdev_work->fdb_info;
> 
> You could probably move this assignment outside of the switch()/case
> statement since it is repeated
> 

Yeah, but this cast is only correct for only this two cases. If in the
future it will be extended it will be weird getting it back inside the
switch.

Thanks for the review!

>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>> +		if (err) {
>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>> +			break;
>> +		}
>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>> +					 &fdb_info->info);
>> +		break;
>> +
>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>> +		fdb_info = &switchdev_work->fdb_info;
>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>> +		if (err)
>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
> 
> OK I must have missed from the off-list discussion why we are not
> calling the switchdev notifier here?
> 

We do not agree on it actually, that is why it was moved to the list.
I think that delete should succeed, you should retry until succession.

The deletion is done under spinlock in the bridge so you cannot block,
thus delete cannot fail due to hardware failure. Calling it here doesn't
make sense because the bridge probably already deleted this FDB.

>> +		break;
>> +	}
>> +	rtnl_unlock();
>> +
>> +	kfree(switchdev_work->fdb_info.addr);
>> +	kfree(switchdev_work);
>> +	dev_put(dev);
>> +}
>> +
>> +static int
>> +dsa_slave_switchdev_fdb_work_init(struct dsa_switchdev_event_work *
>> +				  switchdev_work,
>> +				  const struct switchdev_notifier_fdb_info *
>> +				  fdb_info)
>> +{
>> +	memcpy(&switchdev_work->fdb_info, fdb_info,
>> +	       sizeof(switchdev_work->fdb_info));
>> +	switchdev_work->fdb_info.addr = kzalloc(ETH_ALEN, GFP_ATOMIC);
>> +	if (!switchdev_work->fdb_info.addr)
>> +		return -ENOMEM;
> 
> Can't we have the fdb_info structure contain pre-allocated 6 bytes
> storage for address, it's really silly to do this here and every time we
> need to offload an FDB entry.
>

This fdb_info contains pointer to MAC which resides in the bridge's
internal fdb struct (struct net_bridge_fdb_entry). It has to be copied
at least once due to the async processing in the workqeue. Note that
no reference is taken on this struct, and while you process (async) the
add this struct can be already deleted followed by a del notification.

In case the fdb_info contains this 6 bytes, 2 copies would be performed:
1. Inside the bridge: from the net_bridge_fdb_entry to the fdb_info
   (which is allocated on the stack and used only during the
    notification call)
2. Inside the driver: duplication of the fdb_info to the workqeue item

The current way only does one copy, so I think its better. We can do a
little optimization here by having the dsa_switchdev_event_work contain
the address itself instead of ptr to fdb_info. That way only one
allocation of the dsa_switchdev_event_work is done, but I think its not
that crucial.

>> +	ether_addr_copy((u8 *)switchdev_work->fdb_info.addr,
>> +			fdb_info->addr);
>> +	return 0;
>> +}
>> +
>> +/* Called under rcu_read_lock() */
>> +static int dsa_slave_switchdev_event(struct notifier_block *unused,
>> +				     unsigned long event, void *ptr)
>> +{
>> +	struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
>> +	struct dsa_switchdev_event_work *switchdev_work;
>> +
>> +	if (!dsa_slave_dev_check(dev))
>> +		return NOTIFY_DONE;
>> +
>> +	switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
>> +	if (!switchdev_work)
>> +		return NOTIFY_BAD;
> 
> Same here, can we try to re-use a heap allocated workqueue item and we
> just re-initialize it when we have to?
> 

It cannot be the same one. Because if user added multiple FDBs and the
kernel thread which does the work was preempted this work should
be accumulated in the queue.

Maybe I misunderstood the comment..

>> +
>> +	INIT_WORK(&switchdev_work->work, dsa_switchdev_event_work);
>> +	switchdev_work->dev = dev;
>> +	switchdev_work->event = event;
>> +
>> +	switch (event) {
>> +	case SWITCHDEV_FDB_ADD_TO_DEVICE: /* fall through */
>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>> +		if (dsa_slave_switchdev_fdb_work_init(switchdev_work,
>> +						      ptr))
>> +			goto err_fdb_work_init;
>> +		dev_hold(dev);
>> +		break;
>> +	default:
>> +		kfree(switchdev_work);
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	dsa_schedule_work(&switchdev_work->work);
>> +	return NOTIFY_OK;
>> +
>> +err_fdb_work_init:
>> +	kfree(switchdev_work);
>> +	return NOTIFY_BAD;
>> +}
>> +
>>  static struct notifier_block dsa_slave_nb __read_mostly = {
>> -	.notifier_call	= dsa_slave_netdevice_event,
>> +	.notifier_call  = dsa_slave_netdevice_event,
>> +};
>> +
>> +static struct notifier_block dsa_slave_switchdev_notifier = {
>> +	.notifier_call = dsa_slave_switchdev_event,
>>  };
>>  
>>  int dsa_slave_register_notifier(void)
>>  {
>> -	return register_netdevice_notifier(&dsa_slave_nb);
>> +	int err;
>> +
>> +	err = register_netdevice_notifier(&dsa_slave_nb);
>> +	if (err)
>> +		return err;
>> +
>> +	err = register_switchdev_notifier(&dsa_slave_switchdev_notifier);
>> +	if (err)
>> +		goto err_register_switchdev;
> 
> this label is a bit misnamed since it really deals with the slave dsa
> notifier, how about no label or just renaming it "error_slave_nb"?
> 

You have two nb here so its misnamed too.. Maybe err_switchdev_nb?

>> +
>> +	return 0;
>> +
>> +err_register_switchdev:
>> +	unregister_netdevice_notifier(&dsa_slave_nb);
>> +	return err;
>>  }
>>  
>>  void dsa_slave_unregister_notifier(void)
>>  {
>>  	int err;
>>  
>> +	err = unregister_netdevice_notifier(&dsa_slave_switchdev_notifier);
>> +	if (err)
>> +		pr_err("DSA: failed to unregister switchdev notifier (%d)\n", err);
>> +
>>  	err = unregister_netdevice_notifier(&dsa_slave_nb);
>>  	if (err)
>>  		pr_err("DSA: failed to unregister slave notifier (%d)\n", err);
>>
> 
> 

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Vivien Didelot]

Hi Arkadi,

Arkadi Sharshevsky <arkadis@mellanox.com> writes:

>>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>>> +		if (err) {
>>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>>> +			break;
>>> +		}
>>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>>> +					 &fdb_info->info);
>>> +		break;
>>> +
>>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>>> +		fdb_info = &switchdev_work->fdb_info;
>>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>>> +		if (err)
>>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
>> 
>> OK I must have missed from the off-list discussion why we are not
>> calling the switchdev notifier here?
>
> We do not agree on it actually, that is why it was moved to the list.
> I think that delete should succeed, you should retry until succession.
>
> The deletion is done under spinlock in the bridge so you cannot block,
> thus delete cannot fail due to hardware failure. Calling it here doesn't
> make sense because the bridge probably already deleted this FDB.

So as we discussed, the problem here is that if dsa_port_fdb_del fails
for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux
bridge will delete the entry in software, dumping bridge fdb will show
nothing, but the entry would still be programmed in hardware and the
network can thus be inconsistent, unsupposedly switching frames.

IMHO the correct way for bridge to use the notification chain is to make
SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE:
if an entry has been marked as offloaded, bridge must mark the entry as
to-be-deleted and do not delete the software entry until the driver
notifies back the successful deletion.

If that is hardly feasible due to some bridge limitations, we must
explain this in a comment and use something more explosive than a simple
netdev_dbg to warn the user about the broken network setup...


Thanks,

        Vivien

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Arkadi Sharshevsky]

On 07/10/2017 11:59 PM, Vivien Didelot wrote:
> Hi Arkadi,
> 
> Arkadi Sharshevsky <arkadis@mellanox.com> writes:
> 
>>>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>>>> +		if (err) {
>>>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>>>> +			break;
>>>> +		}
>>>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>>>> +					 &fdb_info->info);
>>>> +		break;
>>>> +
>>>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>>>> +		fdb_info = &switchdev_work->fdb_info;
>>>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>>>> +		if (err)
>>>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
>>>
>>> OK I must have missed from the off-list discussion why we are not
>>> calling the switchdev notifier here?
>>
>> We do not agree on it actually, that is why it was moved to the list.
>> I think that delete should succeed, you should retry until succession.
>>
>> The deletion is done under spinlock in the bridge so you cannot block,
>> thus delete cannot fail due to hardware failure. Calling it here doesn't
>> make sense because the bridge probably already deleted this FDB.
> 
> So as we discussed, the problem here is that if dsa_port_fdb_del fails
> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux
> bridge will delete the entry in software, dumping bridge fdb will show
> nothing, but the entry would still be programmed in hardware and the
> network can thus be inconsistent, unsupposedly switching frames.
> 
> IMHO the correct way for bridge to use the notification chain is to make
> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE:
> if an entry has been marked as offloaded, bridge must mark the entry as
> to-be-deleted and do not delete the software entry until the driver
> notifies back the successful deletion.
> 
> If that is hardly feasible due to some bridge limitations, we must
> explain this in a comment and use something more explosive than a simple
> netdev_dbg to warn the user about the broken network setup...
> 
> 
> Thanks,
> 
>         Vivien
> 

Hi Nikolay,

Vivien raised inconsistency issue with the current switchdev
notification chain in case of FDB del. In case of static FDB delete,
notification will be sent to the driver, followed by deletion of the
software entry without waiting for the hardware delete. In case of
hardware deletion failure the consecutive FDB dump will not show the
deleted entry, yet, the entry will stay in hardware.

The deletion is done under lock thus the hardware deletion is deferred,
and cannot fail due to hardware removal failure. Thus the above proposed
solution by Vivien can lead to confusing situation:

1. User deletes the entry
2. Deletion succeed
3. User dumps FDB and still sees this entry due to hardware failure,
   what should he do? retry to delete until the FDB dump will not show
   the entry?

Would like to hear you opinion about this solution.

IMHO in this case the driver should retry to delete, in case of
several retries the driver should maybe:
1. Trap the traffic to CPU (dint think it possible in case of DSA).
2. Disable the port (its more explosive then netdev_dbg).

Thanks,
Arkadi

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Nikolay Aleksandrov]

On 11/07/17 13:26, Arkadi Sharshevsky wrote:
> 
> 
> On 07/10/2017 11:59 PM, Vivien Didelot wrote:
>> Hi Arkadi,
>>
>> Arkadi Sharshevsky <arkadis@mellanox.com> writes:
>>
>>>>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>>>>> +		if (err) {
>>>>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>>>>> +			break;
>>>>> +		}
>>>>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>>>>> +					 &fdb_info->info);
>>>>> +		break;
>>>>> +
>>>>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>>>>> +		fdb_info = &switchdev_work->fdb_info;
>>>>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>>>>> +		if (err)
>>>>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
>>>>
>>>> OK I must have missed from the off-list discussion why we are not
>>>> calling the switchdev notifier here?
>>>
>>> We do not agree on it actually, that is why it was moved to the list.
>>> I think that delete should succeed, you should retry until succession.
>>>
>>> The deletion is done under spinlock in the bridge so you cannot block,
>>> thus delete cannot fail due to hardware failure. Calling it here doesn't
>>> make sense because the bridge probably already deleted this FDB.
>>
>> So as we discussed, the problem here is that if dsa_port_fdb_del fails
>> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux
>> bridge will delete the entry in software, dumping bridge fdb will show
>> nothing, but the entry would still be programmed in hardware and the
>> network can thus be inconsistent, unsupposedly switching frames.
>>
>> IMHO the correct way for bridge to use the notification chain is to make
>> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE:
>> if an entry has been marked as offloaded, bridge must mark the entry as
>> to-be-deleted and do not delete the software entry until the driver
>> notifies back the successful deletion.
>>
>> If that is hardly feasible due to some bridge limitations, we must
>> explain this in a comment and use something more explosive than a simple
>> netdev_dbg to warn the user about the broken network setup...
>>
>>
>> Thanks,
>>
>>         Vivien
>>
> 
> Hi Nikolay,
> 
> Vivien raised inconsistency issue with the current switchdev
> notification chain in case of FDB del. In case of static FDB delete,
> notification will be sent to the driver, followed by deletion of the
> software entry without waiting for the hardware delete. In case of
> hardware deletion failure the consecutive FDB dump will not show the
> deleted entry, yet, the entry will stay in hardware.
> 
> The deletion is done under lock thus the hardware deletion is deferred,
> and cannot fail due to hardware removal failure. Thus the above proposed
> solution by Vivien can lead to confusing situation:
> 
> 1. User deletes the entry
> 2. Deletion succeed
> 3. User dumps FDB and still sees this entry due to hardware failure,
>    what should he do? retry to delete until the FDB dump will not show
>    the entry?
> 
> Would like to hear you opinion about this solution.
> 
> IMHO in this case the driver should retry to delete, in case of
> several retries the driver should maybe:
> 1. Trap the traffic to CPU (dint think it possible in case of DSA).
> 2. Disable the port (its more explosive then netdev_dbg).
> 
> Thanks,
> Arkadi
> 
> 

Hi,
Looking at the code - it would seem that retrying is the only current option
with the way these switchdev notifications are handled. They cannot fail, meaning
from the bridge POV these ops must always succeed and errors are ignored, so the
driver should do everything possible to stay in sync, and in case all fails
then disabling the port seems like the best option to me, to show that something is
clearly wrong and avoid further issues, but DSA maintainers can comment more
on how to handle failure.

That being said:
This sounds a lot like the switchdev notifications vs callbacks discussions that we've
had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock
is the main problem let's work towards improving that and making the fdb code handle
switchdev similar to the vlan code.

Cheers,
 Nik

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Arkadi Sharshevsky]

On 07/11/2017 06:05 PM, Nikolay Aleksandrov wrote:
> On 11/07/17 13:26, Arkadi Sharshevsky wrote:
>>
>>
>> On 07/10/2017 11:59 PM, Vivien Didelot wrote:
>>> Hi Arkadi,
>>>
>>> Arkadi Sharshevsky <arkadis@mellanox.com> writes:
>>>
>>>>>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>>>>>> +		if (err) {
>>>>>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>>>>>> +			break;
>>>>>> +		}
>>>>>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>>>>>> +					 &fdb_info->info);
>>>>>> +		break;
>>>>>> +
>>>>>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>>>>>> +		fdb_info = &switchdev_work->fdb_info;
>>>>>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>>>>>> +		if (err)
>>>>>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
>>>>>
>>>>> OK I must have missed from the off-list discussion why we are not
>>>>> calling the switchdev notifier here?
>>>>
>>>> We do not agree on it actually, that is why it was moved to the list.
>>>> I think that delete should succeed, you should retry until succession.
>>>>
>>>> The deletion is done under spinlock in the bridge so you cannot block,
>>>> thus delete cannot fail due to hardware failure. Calling it here doesn't
>>>> make sense because the bridge probably already deleted this FDB.
>>>
>>> So as we discussed, the problem here is that if dsa_port_fdb_del fails
>>> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux
>>> bridge will delete the entry in software, dumping bridge fdb will show
>>> nothing, but the entry would still be programmed in hardware and the
>>> network can thus be inconsistent, unsupposedly switching frames.
>>>
>>> IMHO the correct way for bridge to use the notification chain is to make
>>> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE:
>>> if an entry has been marked as offloaded, bridge must mark the entry as
>>> to-be-deleted and do not delete the software entry until the driver
>>> notifies back the successful deletion.
>>>
>>> If that is hardly feasible due to some bridge limitations, we must
>>> explain this in a comment and use something more explosive than a simple
>>> netdev_dbg to warn the user about the broken network setup...
>>>
>>>
>>> Thanks,
>>>
>>>         Vivien
>>>
>>
>> Hi Nikolay,
>>
>> Vivien raised inconsistency issue with the current switchdev
>> notification chain in case of FDB del. In case of static FDB delete,
>> notification will be sent to the driver, followed by deletion of the
>> software entry without waiting for the hardware delete. In case of
>> hardware deletion failure the consecutive FDB dump will not show the
>> deleted entry, yet, the entry will stay in hardware.
>>
>> The deletion is done under lock thus the hardware deletion is deferred,
>> and cannot fail due to hardware removal failure. Thus the above proposed
>> solution by Vivien can lead to confusing situation:
>>
>> 1. User deletes the entry
>> 2. Deletion succeed
>> 3. User dumps FDB and still sees this entry due to hardware failure,
>>    what should he do? retry to delete until the FDB dump will not show
>>    the entry?
>>
>> Would like to hear you opinion about this solution.
>>
>> IMHO in this case the driver should retry to delete, in case of
>> several retries the driver should maybe:
>> 1. Trap the traffic to CPU (dint think it possible in case of DSA).
>> 2. Disable the port (its more explosive then netdev_dbg).
>>
>> Thanks,
>> Arkadi
>>
>>
> 
> Hi,
> Looking at the code - it would seem that retrying is the only current option
> with the way these switchdev notifications are handled. They cannot fail, meaning
> from the bridge POV these ops must always succeed and errors are ignored, so the
> driver should do everything possible to stay in sync, and in case all fails
> then disabling the port seems like the best option to me, to show that something is
> clearly wrong and avoid further issues, but DSA maintainers can comment more
> on how to handle failure.
> 
> That being said:
> This sounds a lot like the switchdev notifications vs callbacks discussions that we've
> had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock
> is the main problem let's work towards improving that and making the fdb code handle
> switchdev similar to the vlan code.
> 
> Cheers,
>  Nik
> 

Vlans can be only added by user under rtnl so its possible to sleep. On
the other hand FDBs can be learned in atomic context, thus the
notification chain is atomic. One Possible way I thought about doing it
is to maintain bridge internal ordered workqueue for the FDBs learned in
atomic context, furthermore the FDB table will be protected by mutex.
The STATIC entries are added in user context so the user will add
directly to this hash table and could sleep.

Its not very good, but I don't think about another solution here.

Regarding notification or not, drivers should know about FDBs that are
not directly related to them like in case of vxlan (vxlan device is not
upper device of some siwthcdev port) so notification is better approach
in my opinion.

Regarding prepare/commit, it is not used in mlxsw. And we would like
to remove it from DSA (in case of FDBs). The prepare phase cannot
assure success because errors could happen unrelated to reserved space
in the switch in the commit stage..

Thanks,
Arkadi
> 
> 
> 
> 
> 
> 
> 
> 

Re: [PATCH net-next RFC 05/12] net: dsa: Add support for learning FDB through notification

[Nikolay Aleksandrov]

On 12/07/17 14:23, Arkadi Sharshevsky wrote:
> 
> 
> On 07/11/2017 06:05 PM, Nikolay Aleksandrov wrote:
>> On 11/07/17 13:26, Arkadi Sharshevsky wrote:
>>>
>>>
>>> On 07/10/2017 11:59 PM, Vivien Didelot wrote:
>>>> Hi Arkadi,
>>>>
>>>> Arkadi Sharshevsky <arkadis@mellanox.com> writes:
>>>>
>>>>>>> +		err = dsa_port_fdb_add(p->dp, fdb_info->addr, fdb_info->vid);
>>>>>>> +		if (err) {
>>>>>>> +			netdev_dbg(dev, "fdb add failed err=%d\n", err);
>>>>>>> +			break;
>>>>>>> +		}
>>>>>>> +		call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED, dev,
>>>>>>> +					 &fdb_info->info);
>>>>>>> +		break;
>>>>>>> +
>>>>>>> +	case SWITCHDEV_FDB_DEL_TO_DEVICE:
>>>>>>> +		fdb_info = &switchdev_work->fdb_info;
>>>>>>> +		err = dsa_port_fdb_del(p->dp, fdb_info->addr, fdb_info->vid);
>>>>>>> +		if (err)
>>>>>>> +			netdev_dbg(dev, "fdb del failed err=%d\n", err);
>>>>>>
>>>>>> OK I must have missed from the off-list discussion why we are not
>>>>>> calling the switchdev notifier here?
>>>>>
>>>>> We do not agree on it actually, that is why it was moved to the list.
>>>>> I think that delete should succeed, you should retry until succession.
>>>>>
>>>>> The deletion is done under spinlock in the bridge so you cannot block,
>>>>> thus delete cannot fail due to hardware failure. Calling it here doesn't
>>>>> make sense because the bridge probably already deleted this FDB.
>>>>
>>>> So as we discussed, the problem here is that if dsa_port_fdb_del fails
>>>> for some probable reasons (MDIO timeout, weak GPIO lines, etc.), Linux
>>>> bridge will delete the entry in software, dumping bridge fdb will show
>>>> nothing, but the entry would still be programmed in hardware and the
>>>> network can thus be inconsistent, unsupposedly switching frames.
>>>>
>>>> IMHO the correct way for bridge to use the notification chain is to make
>>>> SWITCHDEV_FDB_DEL_TO_DEVICE symmetrical to SWITCHDEV_FDB_ADD_TO_DEVICE:
>>>> if an entry has been marked as offloaded, bridge must mark the entry as
>>>> to-be-deleted and do not delete the software entry until the driver
>>>> notifies back the successful deletion.
>>>>
>>>> If that is hardly feasible due to some bridge limitations, we must
>>>> explain this in a comment and use something more explosive than a simple
>>>> netdev_dbg to warn the user about the broken network setup...
>>>>
>>>>
>>>> Thanks,
>>>>
>>>>         Vivien
>>>>
>>>
>>> Hi Nikolay,
>>>
>>> Vivien raised inconsistency issue with the current switchdev
>>> notification chain in case of FDB del. In case of static FDB delete,
>>> notification will be sent to the driver, followed by deletion of the
>>> software entry without waiting for the hardware delete. In case of
>>> hardware deletion failure the consecutive FDB dump will not show the
>>> deleted entry, yet, the entry will stay in hardware.
>>>
>>> The deletion is done under lock thus the hardware deletion is deferred,
>>> and cannot fail due to hardware removal failure. Thus the above proposed
>>> solution by Vivien can lead to confusing situation:
>>>
>>> 1. User deletes the entry
>>> 2. Deletion succeed
>>> 3. User dumps FDB and still sees this entry due to hardware failure,
>>>    what should he do? retry to delete until the FDB dump will not show
>>>    the entry?
>>>
>>> Would like to hear you opinion about this solution.
>>>
>>> IMHO in this case the driver should retry to delete, in case of
>>> several retries the driver should maybe:
>>> 1. Trap the traffic to CPU (dint think it possible in case of DSA).
>>> 2. Disable the port (its more explosive then netdev_dbg).
>>>
>>> Thanks,
>>> Arkadi
>>>
>>>
>>
>> Hi,
>> Looking at the code - it would seem that retrying is the only current option
>> with the way these switchdev notifications are handled. They cannot fail, meaning
>> from the bridge POV these ops must always succeed and errors are ignored, so the
>> driver should do everything possible to stay in sync, and in case all fails
>> then disabling the port seems like the best option to me, to show that something is
>> clearly wrong and avoid further issues, but DSA maintainers can comment more
>> on how to handle failure.
>>
>> That being said:
>> This sounds a lot like the switchdev notifications vs callbacks discussions that we've
>> had in the past. Also what happened with the prepare+commit and all that ? If the hash_lock
>> is the main problem let's work towards improving that and making the fdb code handle
>> switchdev similar to the vlan code.
>>
>> Cheers,
>>  Nik
>>
> 
> Vlans can be only added by user under rtnl so its possible to sleep. On
> the other hand FDBs can be learned in atomic context, thus the
> notification chain is atomic. One Possible way I thought about doing it

Right, that's why I mentioned the hash_lock. :-)

> is to maintain bridge internal ordered workqueue for the FDBs learned in
> atomic context, furthermore the FDB table will be protected by mutex.
> The STATIC entries are added in user context so the user will add
> directly to this hash table and could sleep.

I was thinking in the same direction but instead just keep per-cpu
delayed learning lists that get emptied (i.e. learned) from a workqueue
that gets scheduled from atomic context, but it seems to me the
complexity is unjustified if the drivers can handle the errors, they're
the only reason for doing this right now.

So options so far look like:
1) return success & driver retry until op is successful or deal with
fallout (e.g. disable hw fwd, disable port etc)
2) move to mutex so we can fail the user requested op on hw failure
3) return success but don't actually delete fdb until hw succeeds

In the current situation 1) seems like a good option as we discussed, I
don't like 3). 2) and similar alternatives can be discussed further, I
have plans to migrate the fdb code to a rhashtable and can adjust the
change for it.

> 
> Its not very good, but I don't think about another solution here.
> 
> Regarding notification or not, drivers should know about FDBs that are
> not directly related to them like in case of vxlan (vxlan device is not
> upper device of some siwthcdev port) so notification is better approach
> in my opinion.
> 
> Regarding prepare/commit, it is not used in mlxsw. And we would like
> to remove it from DSA (in case of FDBs). The prepare phase cannot
> assure success because errors could happen unrelated to reserved space
> in the switch in the commit stage..
> 

Yeah, I figured as much.

> Thanks,
> Arkadi
>>

Cheers,
 Nik

>>
>>
>>
>>
>>
>>
>>

[PATCH net-next RFC 06/12] net: dsa: Remove support for FDB add/del via SELF

[Arkadi Sharshevsky]

FDB add/del can be added via switchdev notification chain. Thus the support
for configuration via switchdev objects can be removed.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 net/dsa/slave.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d0592f2..7307195 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -250,12 +250,6 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	 */
 
 	switch (obj->id) {
-	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		if (switchdev_trans_ph_prepare(trans))
-			return 0;
-		err = dsa_port_fdb_add(dp, SWITCHDEV_OBJ_PORT_FDB(obj)->addr,
-				       SWITCHDEV_OBJ_PORT_FDB(obj)->vid);
-		break;
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj), trans);
 		break;
@@ -279,10 +273,6 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	int err;
 
 	switch (obj->id) {
-	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		err = dsa_port_fdb_del(dp, SWITCHDEV_OBJ_PORT_FDB(obj)->addr,
-				       SWITCHDEV_OBJ_PORT_FDB(obj)->vid);
-		break;
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
@@ -925,8 +915,6 @@ 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		= 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,
-- 
2.4.11

[PATCH net-next RFC 07/12] net: dsa: Add support for querying supported bridge flags

[Arkadi Sharshevsky]

The DSA drivers do not support bridge flags offload. Yet, this attribute
should be added in order for the bridge to fail when one tries set a
flag on the port, as explained in commit dc0ecabd6231 ("net: switchdev:
Add support for querying supported bridge flags by hardware").

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 net/dsa/slave.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 7307195..53326ba 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -324,6 +324,9 @@ static int dsa_slave_port_attr_get(struct net_device *dev,
 		attr->u.ppid.id_len = sizeof(ds->index);
 		memcpy(&attr->u.ppid.id, &ds->index, attr->u.ppid.id_len);
 		break;
+	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT:
+		attr->u.brport_flags_support = 0;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.4.11

[PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Arkadi Sharshevsky]

The bridge port attributes/vlan for DSA devices should be set only
from bridge code. Furthermore, The vlans are synced totally with the
bridge so there is no need for special dump support.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 drivers/net/dsa/b53/b53_common.c       | 44 --------------------------
 drivers/net/dsa/b53/b53_priv.h         |  3 --
 drivers/net/dsa/bcm_sf2.c              |  1 -
 drivers/net/dsa/dsa_loop.c             | 38 -----------------------
 drivers/net/dsa/microchip/ksz_common.c | 41 -------------------------
 drivers/net/dsa/mv88e6xxx/chip.c       | 56 ----------------------------------
 include/net/dsa.h                      |  4 ---
 net/dsa/dsa_priv.h                     |  4 ---
 net/dsa/port.c                         | 12 --------
 net/dsa/slave.c                        |  6 ----
 10 files changed, 209 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index c414b43..6020e88 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1053,49 +1053,6 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL(b53_vlan_del);
 
-int b53_vlan_dump(struct dsa_switch *ds, int port,
-		  struct switchdev_obj_port_vlan *vlan,
-		  switchdev_obj_dump_cb_t *cb)
-{
-	struct b53_device *dev = ds->priv;
-	u16 vid, vid_start = 0, pvid;
-	struct b53_vlan *vl;
-	int err = 0;
-
-	if (is5325(dev) || is5365(dev))
-		vid_start = 1;
-
-	b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
-
-	/* Use our software cache for dumps, since we do not have any HW
-	 * operation returning only the used/valid VLANs
-	 */
-	for (vid = vid_start; vid < dev->num_vlans; vid++) {
-		vl = &dev->vlans[vid];
-
-		if (!vl->valid)
-			continue;
-
-		if (!(vl->members & BIT(port)))
-			continue;
-
-		vlan->vid_begin = vlan->vid_end = vid;
-		vlan->flags = 0;
-
-		if (vl->untag & BIT(port))
-			vlan->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
-		if (pvid == vid)
-			vlan->flags |= BRIDGE_VLAN_INFO_PVID;
-
-		err = cb(&vlan->obj);
-		if (err)
-			break;
-	}
-
-	return err;
-}
-EXPORT_SYMBOL(b53_vlan_dump);
-
 /* Address Resolution Logic routines */
 static int b53_arl_op_wait(struct b53_device *dev)
 {
@@ -1552,7 +1509,6 @@ static const struct dsa_switch_ops b53_switch_ops = {
 	.port_vlan_prepare	= b53_vlan_prepare,
 	.port_vlan_add		= b53_vlan_add,
 	.port_vlan_del		= b53_vlan_del,
-	.port_vlan_dump		= b53_vlan_dump,
 	.port_fdb_dump		= b53_fdb_dump,
 	.port_fdb_add		= b53_fdb_add,
 	.port_fdb_del		= b53_fdb_del,
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index f29c892..af5d6c1 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -393,9 +393,6 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
 		  struct switchdev_trans *trans);
 int b53_vlan_del(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan);
-int b53_vlan_dump(struct dsa_switch *ds, int port,
-		  struct switchdev_obj_port_vlan *vlan,
-		  switchdev_obj_dump_cb_t *cb);
 int b53_fdb_add(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid);
 int b53_fdb_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 648f91b..f0d2dc4 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -1033,7 +1033,6 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
 	.port_vlan_prepare	= b53_vlan_prepare,
 	.port_vlan_add		= b53_vlan_add,
 	.port_vlan_del		= b53_vlan_del,
-	.port_vlan_dump		= b53_vlan_dump,
 	.port_fdb_prepare	= b53_fdb_prepare,
 	.port_fdb_dump		= b53_fdb_dump,
 	.port_fdb_add		= b53_fdb_add,
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index fdd8f38..76d6660 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -257,43 +257,6 @@ static int dsa_loop_port_vlan_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int dsa_loop_port_vlan_dump(struct dsa_switch *ds, int port,
-				   struct switchdev_obj_port_vlan *vlan,
-				   switchdev_obj_dump_cb_t *cb)
-{
-	struct dsa_loop_priv *ps = ds->priv;
-	struct mii_bus *bus = ps->bus;
-	struct dsa_loop_vlan *vl;
-	u16 vid, vid_start = 0;
-	int err = 0;
-
-	dev_dbg(ds->dev, "%s\n", __func__);
-
-	/* Just do a sleeping operation to make lockdep checks effective */
-	mdiobus_read(bus, ps->port_base + port, MII_BMSR);
-
-	for (vid = vid_start; vid < DSA_LOOP_VLANS; vid++) {
-		vl = &ps->vlans[vid];
-
-		if (!(vl->members & BIT(port)))
-			continue;
-
-		vlan->vid_begin = vlan->vid_end = vid;
-		vlan->flags = 0;
-
-		if (vl->untagged & BIT(port))
-			vlan->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
-		if (ps->pvid == vid)
-			vlan->flags |= BRIDGE_VLAN_INFO_PVID;
-
-		err = cb(&vlan->obj);
-		if (err)
-			break;
-	}
-
-	return err;
-}
-
 static struct dsa_switch_ops dsa_loop_driver = {
 	.get_tag_protocol	= dsa_loop_get_protocol,
 	.setup			= dsa_loop_setup,
@@ -310,7 +273,6 @@ static struct dsa_switch_ops dsa_loop_driver = {
 	.port_vlan_prepare	= dsa_loop_port_vlan_prepare,
 	.port_vlan_add		= dsa_loop_port_vlan_add,
 	.port_vlan_del		= dsa_loop_port_vlan_del,
-	.port_vlan_dump		= dsa_loop_port_vlan_dump,
 };
 
 static int dsa_loop_drv_probe(struct mdio_device *mdiodev)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b55f364..a53ce59 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -638,46 +638,6 @@ static int ksz_port_vlan_del(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int ksz_port_vlan_dump(struct dsa_switch *ds, int port,
-			      struct switchdev_obj_port_vlan *vlan,
-			      switchdev_obj_dump_cb_t *cb)
-{
-	struct ksz_device *dev = ds->priv;
-	u16 vid;
-	u16 data;
-	struct vlan_table *vlan_cache;
-	int err = 0;
-
-	mutex_lock(&dev->vlan_mutex);
-
-	/* use dev->vlan_cache due to lack of searching valid vlan entry */
-	for (vid = vlan->vid_begin; vid < dev->num_vlans; vid++) {
-		vlan_cache = &dev->vlan_cache[vid];
-
-		if (!(vlan_cache->table[0] & VLAN_VALID))
-			continue;
-
-		vlan->vid_begin = vid;
-		vlan->vid_end = vid;
-		vlan->flags = 0;
-		if (vlan_cache->table[2] & BIT(port)) {
-			if (vlan_cache->table[1] & BIT(port))
-				vlan->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
-			ksz_pread16(dev, port, REG_PORT_DEFAULT_VID, &data);
-			if (vid == (data & 0xFFFFF))
-				vlan->flags |= BRIDGE_VLAN_INFO_PVID;
-
-			err = cb(&vlan->obj);
-			if (err)
-				break;
-		}
-	}
-
-	mutex_unlock(&dev->vlan_mutex);
-
-	return err;
-}
-
 struct alu_struct {
 	/* entry 1 */
 	u8	is_static:1;
@@ -1124,7 +1084,6 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_vlan_prepare	= ksz_port_vlan_prepare,
 	.port_vlan_add		= ksz_port_vlan_add,
 	.port_vlan_del		= ksz_port_vlan_del,
-	.port_vlan_dump		= ksz_port_vlan_dump,
 	.port_fdb_dump		= ksz_port_fdb_dump,
 	.port_fdb_add		= ksz_port_fdb_add,
 	.port_fdb_del		= ksz_port_fdb_del,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fba68c8..9cc6269 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1040,61 +1040,6 @@ static int mv88e6xxx_vtu_loadpurge(struct mv88e6xxx_chip *chip,
 	return chip->info->ops->vtu_loadpurge(chip, entry);
 }
 
-static int mv88e6xxx_port_vlan_dump(struct dsa_switch *ds, int port,
-				    struct switchdev_obj_port_vlan *vlan,
-				    switchdev_obj_dump_cb_t *cb)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	struct mv88e6xxx_vtu_entry next = {
-		.vid = chip->info->max_vid,
-	};
-	u16 pvid;
-	int err;
-
-	if (!chip->info->max_vid)
-		return -EOPNOTSUPP;
-
-	mutex_lock(&chip->reg_lock);
-
-	err = mv88e6xxx_port_get_pvid(chip, port, &pvid);
-	if (err)
-		goto unlock;
-
-	do {
-		err = mv88e6xxx_vtu_getnext(chip, &next);
-		if (err)
-			break;
-
-		if (!next.valid)
-			break;
-
-		if (next.member[port] ==
-		    MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_NON_MEMBER)
-			continue;
-
-		/* reinit and dump this VLAN obj */
-		vlan->vid_begin = next.vid;
-		vlan->vid_end = next.vid;
-		vlan->flags = 0;
-
-		if (next.member[port] ==
-		    MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED)
-			vlan->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
-
-		if (next.vid == pvid)
-			vlan->flags |= BRIDGE_VLAN_INFO_PVID;
-
-		err = cb(&vlan->obj);
-		if (err)
-			break;
-	} while (next.vid < chip->info->max_vid);
-
-unlock:
-	mutex_unlock(&chip->reg_lock);
-
-	return err;
-}
-
 static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid)
 {
 	DECLARE_BITMAP(fid_bitmap, MV88E6XXX_N_FID);
@@ -3858,7 +3803,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_vlan_prepare	= mv88e6xxx_port_vlan_prepare,
 	.port_vlan_add		= mv88e6xxx_port_vlan_add,
 	.port_vlan_del		= mv88e6xxx_port_vlan_del,
-	.port_vlan_dump		= mv88e6xxx_port_vlan_dump,
 	.port_fdb_add           = mv88e6xxx_port_fdb_add,
 	.port_fdb_del           = mv88e6xxx_port_fdb_del,
 	.port_fdb_dump          = mv88e6xxx_port_fdb_dump,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4835b0e..9171b11 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -384,10 +384,6 @@ struct dsa_switch_ops {
 				 struct switchdev_trans *trans);
 	int	(*port_vlan_del)(struct dsa_switch *ds, int port,
 				 const struct switchdev_obj_port_vlan *vlan);
-	int	(*port_vlan_dump)(struct dsa_switch *ds, int port,
-				  struct switchdev_obj_port_vlan *vlan,
-				  switchdev_obj_dump_cb_t *cb);
-
 	/*
 	 * Forwarding database
 	 */
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 2b2f124..421df4f 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -138,10 +138,6 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct switchdev_trans *trans);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
-int dsa_port_vlan_dump(struct dsa_port *dp,
-		       struct switchdev_obj_port_vlan *vlan,
-		       switchdev_obj_dump_cb_t *cb);
-
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 86e0585..ce19216 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -246,15 +246,3 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
-
-int dsa_port_vlan_dump(struct dsa_port *dp,
-		       struct switchdev_obj_port_vlan *vlan,
-		       switchdev_obj_dump_cb_t *cb)
-{
-	struct dsa_switch *ds = dp->ds;
-
-	if (ds->ops->port_vlan_dump)
-		return ds->ops->port_vlan_dump(ds, dp->index, vlan, cb);
-
-	return -EOPNOTSUPP;
-}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 53326ba..7393b36 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -302,9 +302,6 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		err = dsa_port_mdb_dump(dp, SWITCHDEV_OBJ_PORT_MDB(obj), cb);
 		break;
-	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		err = dsa_port_vlan_dump(dp, SWITCHDEV_OBJ_PORT_VLAN(obj), cb);
-		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -926,9 +923,6 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_netpoll_cleanup	= dsa_slave_netpoll_cleanup,
 	.ndo_poll_controller	= dsa_slave_poll_controller,
 #endif
-	.ndo_bridge_getlink	= switchdev_port_bridge_getlink,
-	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
-	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
 	.ndo_get_phys_port_name	= dsa_slave_get_phys_port_name,
 	.ndo_setup_tc		= dsa_slave_setup_tc,
 };
-- 
2.4.11

Re: [PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Florian Fainelli]

On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
> The bridge port attributes/vlan for DSA devices should be set only
> from bridge code. Furthermore, The vlans are synced totally with the
> bridge so there is no need for special dump support.
> 
> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>

I would be more comfortable if we still had a way to dump HW entries by
calling into drivers, because it's useful for debugging, and doing that
using standard tools plus an additional flag for instance:

bridge fdb show

would dump the SW-maintained VLANs, but:

bridge fdb show hw

would dump the HW-maintained VLANs, or any flag name really, but the
point is that we can keep using a standard tool to expose debugging
features. debugfs (or something else) could be used, but was rejected by
David.

Also, technically, it ought to be possible to configure a port's VLAN
membership (although using poor semantics) through e.g: vconfig add
sw0p0 1, which would call ndo_vlan_rx_add_vid() (DSA does not support it
but mlxsw does), and so being able to dump that configuration would be
somehow helpful.
-- 
Florian

Re: [PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Arkadi Sharshevsky]

On 07/05/2017 10:45 PM, Florian Fainelli wrote:
> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>> The bridge port attributes/vlan for DSA devices should be set only
>> from bridge code. Furthermore, The vlans are synced totally with the
>> bridge so there is no need for special dump support.
>>
>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
> 
> I would be more comfortable if we still had a way to dump HW entries by
> calling into drivers, because it's useful for debugging, and doing that
> using standard tools plus an additional flag for instance:
> 
> bridge fdb show
> 
> would dump the SW-maintained VLANs, but:
> 
> bridge fdb show hw
> 

I agree that in your case, the FDBs are exception because they cannot be
synced with the sw bridge due to lack of interrupt per learned entry,
but note that this ability of bypass dump should not be supported by
switchdev code because its contradicts the model. That is why I moved it
inside DSA in one of the following patches (another reason is that
nobody is using it beside you so there is no reason to leave it there).

This is especially true for vlans which are synced completely with the
software bridge.

Regarding debugging, if you want to see the hardware tables you can
easily use dpipe for that. For example you can add table which does
match on vlan and mac and set the output port, or some table that
matches port and incoming packets vlan into internal metadata (FID)
that can be used for lookup.

I just don't think it is good idea for mixing this tools together.

> would dump the HW-maintained VLANs, or any flag name really, but the
> point is that we can keep using a standard tool to expose debugging
> features. debugfs (or something else) could be used, but was rejected by
> David.
> 
> Also, technically, it ought to be possible to configure a port's VLAN
> membership (although using poor semantics) through e.g: vconfig add
> sw0p0 1, which would call ndo_vlan_rx_add_vid() (DSA does not support it
> but mlxsw does), and so being able to dump that configuration would be

Yeah, in mlxsw the vlan devices are used mainly for vlan unaware bridge,
so the vlans will not be visible by the bridge, but the correct API to
see this is via:

$ip -d link show sw1p7.8

> somehow helpful.
> 

Re: [PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Florian Fainelli]

On 07/06/2017 04:00 AM, Arkadi Sharshevsky wrote:
> 
> 
> On 07/05/2017 10:45 PM, Florian Fainelli wrote:
>> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>>> The bridge port attributes/vlan for DSA devices should be set only
>>> from bridge code. Furthermore, The vlans are synced totally with the
>>> bridge so there is no need for special dump support.
>>>
>>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>>
>> I would be more comfortable if we still had a way to dump HW entries by
>> calling into drivers, because it's useful for debugging, and doing that
>> using standard tools plus an additional flag for instance:
>>
>> bridge fdb show
>>
>> would dump the SW-maintained VLANs, but:
>>
>> bridge fdb show hw
>>
> 
> I agree that in your case, the FDBs are exception because they cannot be
> synced with the sw bridge due to lack of interrupt per learned entry,
> but note that this ability of bypass dump should not be supported by
> switchdev code because its contradicts the model. That is why I moved it
> inside DSA in one of the following patches (another reason is that
> nobody is using it beside you so there is no reason to leave it there).

Sure, I agree with that, no need to clutter switchdev and other
switchdev only drivers like mlxsw with FDB dumping code that they don't
need.

> 
> This is especially true for vlans which are synced completely with the
> software bridge.
> 
> Regarding debugging, if you want to see the hardware tables you can
> easily use dpipe for that. For example you can add table which does
> match on vlan and mac and set the output port, or some table that
> matches port and incoming packets vlan into internal metadata (FID)
> that can be used for lookup.
> 
> I just don't think it is good idea for mixing this tools together.

Well, that is once again debatable, ethtool for instance contains a
collection of debug and functional information about your hardware.
Having the ability to dump the VLAN tables using the standard bridge
vlan output has a lot of value in that you can easily correlated whether
the SW representation matches the HW representation (as fed by the
driver back into bridge's netlink messages).

> 
>> would dump the HW-maintained VLANs, or any flag name really, but the
>> point is that we can keep using a standard tool to expose debugging
>> features. debugfs (or something else) could be used, but was rejected by
>> David.
>>
>> Also, technically, it ought to be possible to configure a port's VLAN
>> membership (although using poor semantics) through e.g: vconfig add
>> sw0p0 1, which would call ndo_vlan_rx_add_vid() (DSA does not support it
>> but mlxsw does), and so being able to dump that configuration would be
> 
> Yeah, in mlxsw the vlan devices are used mainly for vlan unaware bridge,
> so the vlans will not be visible by the bridge, but the correct API to
> see this is via:
> 
> $ip -d link show sw1p7.8

Since the VLAN-aware device is visible to user-space, my comment does
not really make sense in that this very interface existence indicates
that the port has been properly set up with a VLAN tag
-- 
Florian

Re: [PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Arkadi Sharshevsky]

On 07/07/2017 12:36 AM, Florian Fainelli wrote:
> On 07/06/2017 04:00 AM, Arkadi Sharshevsky wrote:
>>
>>
>> On 07/05/2017 10:45 PM, Florian Fainelli wrote:
>>> On 07/05/2017 08:36 AM, Arkadi Sharshevsky wrote:
>>>> The bridge port attributes/vlan for DSA devices should be set only
>>>> from bridge code. Furthermore, The vlans are synced totally with the
>>>> bridge so there is no need for special dump support.
>>>>
>>>> Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
>>>
>>> I would be more comfortable if we still had a way to dump HW entries by
>>> calling into drivers, because it's useful for debugging, and doing that
>>> using standard tools plus an additional flag for instance:
>>>
>>> bridge fdb show
>>>
>>> would dump the SW-maintained VLANs, but:
>>>
>>> bridge fdb show hw
>>>
>>
>> I agree that in your case, the FDBs are exception because they cannot be
>> synced with the sw bridge due to lack of interrupt per learned entry,
>> but note that this ability of bypass dump should not be supported by
>> switchdev code because its contradicts the model. That is why I moved it
>> inside DSA in one of the following patches (another reason is that
>> nobody is using it beside you so there is no reason to leave it there).
> 
> Sure, I agree with that, no need to clutter switchdev and other
> switchdev only drivers like mlxsw with FDB dumping code that they don't
> need.
> 
>>
>> This is especially true for vlans which are synced completely with the
>> software bridge.
>>
>> Regarding debugging, if you want to see the hardware tables you can
>> easily use dpipe for that. For example you can add table which does
>> match on vlan and mac and set the output port, or some table that
>> matches port and incoming packets vlan into internal metadata (FID)
>> that can be used for lookup.
>>
>> I just don't think it is good idea for mixing this tools together.
> 
> Well, that is once again debatable, ethtool for instance contains a
> collection of debug and functional information about your hardware.
> Having the ability to dump the VLAN tables using the standard bridge
> vlan output has a lot of value in that you can easily correlated whether
> the SW representation matches the HW representation (as fed by the
> driver back into bridge's netlink messages).
>

In my opinion all of this is very confusing. When the user does:

$bridge vlan show dev sw1p5
port    vlan ids
sw1p5    1 PVID Egress Untagged

sw1p5    1 PVID Egress Untagged

and sees two set of identical vlans its just confusing, Furthermore
if the hw/sw are different it even more weird. For example in what
scenario would a user want to configure different PVID only for hw?

I think that for switchdev device, in case the bridge wants to add
a vlan and hardware fails, the operation should fail as well (done
under rtnl so it can sleep). By this the user gains:

1. No confusion due to mismatched behavior hw/sw. Vlans should be
   synced!.
2. Indication about successful offload. So user knows if its in
   hardware or not.

It seems that you already discussed it:
https://www.mail-archive.com/netdev@vger.kernel.org/msg147067.html

It even seems you have a bug here so deleting this bypass add/del
/dump makes the API more consistent and fixes a bug.

>>
>>> would dump the HW-maintained VLANs, or any flag name really, but the
>>> point is that we can keep using a standard tool to expose debugging
>>> features. debugfs (or something else) could be used, but was rejected by
>>> David.
>>>
>>> Also, technically, it ought to be possible to configure a port's VLAN
>>> membership (although using poor semantics) through e.g: vconfig add
>>> sw0p0 1, which would call ndo_vlan_rx_add_vid() (DSA does not support it
>>> but mlxsw does), and so being able to dump that configuration would be
>>
>> Yeah, in mlxsw the vlan devices are used mainly for vlan unaware bridge,
>> so the vlans will not be visible by the bridge, but the correct API to
>> see this is via:
>>
>> $ip -d link show sw1p7.8
> 
> Since the VLAN-aware device is visible to user-space, my comment does
> not really make sense in that this very interface existence indicates
> that the port has been properly set up with a VLAN tag
> 

Re: [PATCH net-next RFC 08/12] net: dsa: Remove support for bypass bridge port attributes/vlan set

[Vivien Didelot]

Hi Florian, Arkadi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> I would be more comfortable if we still had a way to dump HW entries by
> calling into drivers, because it's useful for debugging, and doing that
> using standard tools plus an additional flag for instance:
>
> bridge fdb show
>
> would dump the SW-maintained VLANs, but:
>
> bridge fdb show hw

I don't think this makes much sense because the network hardware
architecture must be abstracted to the user, who only deals with user
network interfaces. This is also only needed for development and debug.

> would dump the HW-maintained VLANs, or any flag name really, but the
> point is that we can keep using a standard tool to expose debugging
> features. debugfs (or something else) could be used, but was rejected by
> David.

IIRC, what David is opposed to is having a *driver specific* debugfs
interface, i.e. debugfs for only mv88e6xxx. But he was opened to having
a *generic* debugfs interface in net/dsa/ from which any DSA chip
(e.g. lan9303, b53, qca8k, etc.) would benefit.

That makes even more sense for CPU and DSA ports which are not exposed
to userspace. Andrew played with dpipe but that wasn't quite satisfying
if I'm not mistaken. What we need for development and debug is a compile
time enableable interface to simply call into dsa_switch_ops directly.

So far here's what I have: port's VLAN, FDB, MDB, regs, stats, and
switch fabric topology (symlinks between interfaces): http://ix.io/yq0

I can split and submit this as RFC and see if I get slapped.


Thanks,

        Vivien

[PATCH net-next RFC 09/12] net: dsa: Remove redundant MDB dump support

[Arkadi Sharshevsky]

Currently the MDB HW database is synced with the bridge's one, thus,
There is no need to support special dump functionality.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 drivers/net/dsa/microchip/ksz_common.c |  9 ---------
 drivers/net/dsa/mv88e6xxx/chip.c       | 24 ------------------------
 include/net/dsa.h                      |  4 ----
 net/dsa/dsa_priv.h                     |  2 --
 net/dsa/port.c                         | 11 -----------
 net/dsa/slave.c                        |  3 ---
 6 files changed, 53 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index a53ce59..4de9d90 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1020,14 +1020,6 @@ static int ksz_port_mdb_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
-static int ksz_port_mdb_dump(struct dsa_switch *ds, int port,
-			     struct switchdev_obj_port_mdb *mdb,
-			     switchdev_obj_dump_cb_t *cb)
-{
-	/* this is not called by switch layer */
-	return 0;
-}
-
 static int ksz_port_mirror_add(struct dsa_switch *ds, int port,
 			       struct dsa_mall_mirror_tc_entry *mirror,
 			       bool ingress)
@@ -1090,7 +1082,6 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_mdb_prepare       = ksz_port_mdb_prepare,
 	.port_mdb_add           = ksz_port_mdb_add,
 	.port_mdb_del           = ksz_port_mdb_del,
-	.port_mdb_dump          = ksz_port_mdb_dump,
 	.port_mirror_add	= ksz_port_mirror_add,
 	.port_mirror_del	= ksz_port_mirror_del,
 };
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 9cc6269..97b77b9 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1443,15 +1443,6 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
 				fdb->ndm_state = NUD_NOARP;
 			else
 				fdb->ndm_state = NUD_REACHABLE;
-		} else 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);
 		} else {
 			return -EOPNOTSUPP;
 		}
@@ -3762,20 +3753,6 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
-static int mv88e6xxx_port_mdb_dump(struct dsa_switch *ds, int port,
-				   struct switchdev_obj_port_mdb *mdb,
-				   switchdev_obj_dump_cb_t *cb)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	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 const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.probe			= mv88e6xxx_drv_probe,
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
@@ -3809,7 +3786,6 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.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,
 	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
 	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
 };
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 9171b11..2d85ad2 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -406,10 +406,6 @@ struct dsa_switch_ops {
 				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,
-				  switchdev_obj_dump_cb_t *cb);
-
 	/*
 	 * RXNFC
 	 */
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 421df4f..85f53a0 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -131,8 +131,6 @@ int dsa_port_mdb_add(struct dsa_port *dp,
 		     struct switchdev_trans *trans);
 int dsa_port_mdb_del(struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb);
-int dsa_port_mdb_dump(struct dsa_port *dp, struct switchdev_obj_port_mdb *mdb,
-		      switchdev_obj_dump_cb_t *cb);
 int dsa_port_vlan_add(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan,
 		      struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ce19216..7378782 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -210,17 +210,6 @@ int dsa_port_mdb_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_MDB_DEL, &info);
 }
 
-int dsa_port_mdb_dump(struct dsa_port *dp, struct switchdev_obj_port_mdb *mdb,
-		      switchdev_obj_dump_cb_t *cb)
-{
-	struct dsa_switch *ds = dp->ds;
-
-	if (ds->ops->port_mdb_dump)
-		return ds->ops->port_mdb_dump(ds, dp->index, mdb, cb);
-
-	return -EOPNOTSUPP;
-}
-
 int dsa_port_vlan_add(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan,
 		      struct switchdev_trans *trans)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 7393b36..acbc0f2 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -299,9 +299,6 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
 	case SWITCHDEV_OBJ_ID_PORT_FDB:
 		err = dsa_port_fdb_dump(dp, SWITCHDEV_OBJ_PORT_FDB(obj), cb);
 		break;
-	case SWITCHDEV_OBJ_ID_PORT_MDB:
-		err = dsa_port_mdb_dump(dp, SWITCHDEV_OBJ_PORT_MDB(obj), cb);
-		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.4.11

[PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

[Arkadi Sharshevsky]

>From all switchdev devices only DSA requires special FDB dump. This is due
to lack of ability for syncing the hardware learned FDBs with the bridge.
Due to this it is removed from switchdev and moved inside DSA.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 drivers/net/dsa/b53/b53_common.c       | 19 ++++---
 drivers/net/dsa/b53/b53_priv.h         |  3 +-
 drivers/net/dsa/microchip/ksz_common.c | 14 +++--
 drivers/net/dsa/mt7530.c               | 15 +++---
 drivers/net/dsa/mv88e6xxx/chip.c       | 40 ++++++---------
 drivers/net/dsa/qca8k.c                | 14 +++--
 include/net/dsa.h                      | 15 ++++--
 include/net/switchdev.h                | 12 -----
 net/dsa/dsa_priv.h                     |  2 -
 net/dsa/port.c                         | 11 ----
 net/dsa/slave.c                        | 93 ++++++++++++++++++++++++++--------
 net/switchdev/switchdev.c              | 84 ------------------------------
 12 files changed, 128 insertions(+), 194 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 6020e88..9d41dc6 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1227,25 +1227,24 @@ static void b53_arl_search_rd(struct b53_device *dev, u8 idx,
 }
 
 static int b53_fdb_copy(int port, const struct b53_arl_entry *ent,
-			struct switchdev_obj_port_fdb *fdb,
-			switchdev_obj_dump_cb_t *cb)
+			struct dsa_slave_dump_ctx *dump)
 {
+	u16 ndm_state;
+
 	if (!ent->is_valid)
 		return 0;
 
 	if (port != ent->port)
 		return 0;
 
-	ether_addr_copy(fdb->addr, ent->mac);
-	fdb->vid = ent->vid;
-	fdb->ndm_state = ent->is_static ? NUD_NOARP : NUD_REACHABLE;
+	ndm_state = ent->is_static ? NUD_NOARP : NUD_REACHABLE;
 
-	return cb(&fdb->obj);
+	return dsa_slave_port_fdb_do_dump(dump, ent->mac, ent->vid,
+					  ndm_state);
 }
 
 int b53_fdb_dump(struct dsa_switch *ds, int port,
-		 struct switchdev_obj_port_fdb *fdb,
-		 switchdev_obj_dump_cb_t *cb)
+		 struct dsa_slave_dump_ctx *dump)
 {
 	struct b53_device *priv = ds->priv;
 	struct b53_arl_entry results[2];
@@ -1263,13 +1262,13 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,
 			return ret;
 
 		b53_arl_search_rd(priv, 0, &results[0]);
-		ret = b53_fdb_copy(port, &results[0], fdb, cb);
+		ret = b53_fdb_copy(port, &results[0], dump);
 		if (ret)
 			return ret;
 
 		if (priv->num_arl_entries > 2) {
 			b53_arl_search_rd(priv, 1, &results[1]);
-			ret = b53_fdb_copy(port, &results[1], fdb, cb);
+			ret = b53_fdb_copy(port, &results[1], dump);
 			if (ret)
 				return ret;
 
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index af5d6c1..5cc94d9 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -398,8 +398,7 @@ int b53_fdb_add(struct dsa_switch *ds, int port,
 int b53_fdb_del(struct dsa_switch *ds, int port,
 		const unsigned char *addr, u16 vid);
 int b53_fdb_dump(struct dsa_switch *ds, int port,
-		 struct switchdev_obj_port_fdb *fdb,
-		 switchdev_obj_dump_cb_t *cb);
+		 struct dsa_slave_dump_ctx *dump);
 int b53_mirror_add(struct dsa_switch *ds, int port,
 		   struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
 void b53_mirror_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 4de9d90..94be26e 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -805,8 +805,7 @@ static void convert_alu(struct alu_struct *alu, u32 *alu_table)
 }
 
 static int ksz_port_fdb_dump(struct dsa_switch *ds, int port,
-			     struct switchdev_obj_port_fdb *fdb,
-			     switchdev_obj_dump_cb_t *cb)
+			     struct dsa_slave_dump_ctx *dump)
 {
 	struct ksz_device *dev = ds->priv;
 	int ret = 0;
@@ -814,6 +813,7 @@ static int ksz_port_fdb_dump(struct dsa_switch *ds, int port,
 	u32 alu_table[4];
 	struct alu_struct alu;
 	int timeout;
+	u16 ndm_state;
 
 	mutex_lock(&dev->alu_mutex);
 
@@ -841,14 +841,12 @@ static int ksz_port_fdb_dump(struct dsa_switch *ds, int port,
 		convert_alu(&alu, alu_table);
 
 		if (alu.port_forward & BIT(port)) {
-			fdb->vid = alu.fid;
 			if (alu.is_static)
-				fdb->ndm_state = NUD_NOARP;
+				ndm_state = NUD_NOARP;
 			else
-				fdb->ndm_state = NUD_REACHABLE;
-			ether_addr_copy(fdb->addr, alu.mac);
-
-			ret = cb(&fdb->obj);
+				ndm_state = NUD_REACHABLE;
+			ret = dsa_slave_port_fdb_do_dump(dump, alu.mac,
+							 alu.fid, ndm_state);
 			if (ret)
 				goto exit;
 		}
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index f92aae8..41afa4c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -834,12 +834,12 @@ mt7530_port_fdb_del(struct dsa_switch *ds, int port,
 
 static int
 mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
-		     struct switchdev_obj_port_fdb *fdb,
-		     switchdev_obj_dump_cb_t *cb)
+		     struct dsa_slave_dump_ctx *dump)
 {
 	struct mt7530_priv *priv = ds->priv;
 	struct mt7530_fdb _fdb = { 0 };
 	int cnt = MT7530_NUM_FDB_RECORDS;
+	u16 ndm_state;
 	int ret = 0;
 	u32 rsp = 0;
 
@@ -853,11 +853,12 @@ mt7530_port_fdb_dump(struct dsa_switch *ds, int port,
 		if (rsp & ATC_SRCH_HIT) {
 			mt7530_fdb_read(priv, &_fdb);
 			if (_fdb.port_mask & BIT(port)) {
-				ether_addr_copy(fdb->addr, _fdb.mac);
-				fdb->vid = _fdb.vid;
-				fdb->ndm_state = _fdb.noarp ?
-						NUD_NOARP : NUD_REACHABLE;
-				ret = cb(&fdb->obj);
+				ndm_state = _fdb.noarp ?
+					    NUD_NOARP : NUD_REACHABLE;
+				ret = dsa_slave_port_fdb_do_dump(dump,
+								 _fdb.mac,
+								 _fdb.vid,
+								 ndm_state);
 				if (ret < 0)
 					break;
 			}
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 97b77b9..6dccecd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1410,10 +1410,10 @@ static int mv88e6xxx_port_fdb_del(struct dsa_switch *ds, int port,
 
 static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
 				      u16 fid, u16 vid, int port,
-				      struct switchdev_obj *obj,
-				      switchdev_obj_dump_cb_t *cb)
+				      struct dsa_slave_dump_ctx *dump)
 {
 	struct mv88e6xxx_atu_entry addr;
+	u16 ndm_state;
 	int err;
 
 	addr.state = MV88E6XXX_G1_ATU_DATA_STATE_UNUSED;
@@ -1430,24 +1430,16 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
 		if (addr.trunk || (addr.portvec & 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;
+		if (!is_unicast_ether_addr(addr.mac))
+			continue;
 
-			fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
-			fdb->vid = vid;
-			ether_addr_copy(fdb->addr, addr.mac);
-			if (addr.state == MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC)
-				fdb->ndm_state = NUD_NOARP;
-			else
-				fdb->ndm_state = NUD_REACHABLE;
-		} else {
-			return -EOPNOTSUPP;
-		}
+		if (addr.state == MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC)
+			ndm_state = NUD_NOARP;
+		else
+			ndm_state = NUD_REACHABLE;
 
-		err = cb(obj);
+		err = dsa_slave_port_fdb_do_dump(dump, addr.mac, vid,
+						 ndm_state);
 		if (err)
 			return err;
 	} while (!is_broadcast_ether_addr(addr.mac));
@@ -1456,8 +1448,7 @@ static int mv88e6xxx_port_db_dump_fid(struct mv88e6xxx_chip *chip,
 }
 
 static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
-				  struct switchdev_obj *obj,
-				  switchdev_obj_dump_cb_t *cb)
+				  struct dsa_slave_dump_ctx *dump)
 {
 	struct mv88e6xxx_vtu_entry vlan = {
 		.vid = chip->info->max_vid,
@@ -1470,7 +1461,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 	if (err)
 		return err;
 
-	err = mv88e6xxx_port_db_dump_fid(chip, fid, 0, port, obj, cb);
+	err = mv88e6xxx_port_db_dump_fid(chip, fid, 0, port, dump);
 	if (err)
 		return err;
 
@@ -1484,7 +1475,7 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 			break;
 
 		err = mv88e6xxx_port_db_dump_fid(chip, vlan.fid, vlan.vid, port,
-						 obj, cb);
+						 dump);
 		if (err)
 			return err;
 	} while (vlan.vid < chip->info->max_vid);
@@ -1493,14 +1484,13 @@ static int mv88e6xxx_port_db_dump(struct mv88e6xxx_chip *chip, int port,
 }
 
 static int mv88e6xxx_port_fdb_dump(struct dsa_switch *ds, int port,
-				   struct switchdev_obj_port_fdb *fdb,
-				   switchdev_obj_dump_cb_t *cb)
+				   struct dsa_slave_dump_ctx *dump)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
 
 	mutex_lock(&chip->reg_lock);
-	err = mv88e6xxx_port_db_dump(chip, port, &fdb->obj, cb);
+	err = mv88e6xxx_port_db_dump(chip, port, dump);
 	mutex_unlock(&chip->reg_lock);
 
 	return err;
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9c3de3d..c6f7dfb 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -853,27 +853,25 @@ qca8k_port_fdb_del(struct dsa_switch *ds, int port,
 
 static int
 qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
-		    struct switchdev_obj_port_fdb *fdb,
-		    switchdev_obj_dump_cb_t *cb)
+		    struct dsa_slave_dump_ctx *dump)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
 	struct qca8k_fdb _fdb = { 0 };
 	int cnt = QCA8K_NUM_FDB_RECORDS;
+	u16 ndm_state;
 	int ret = 0;
 
 	mutex_lock(&priv->reg_mutex);
 	while (cnt-- && !qca8k_fdb_next(priv, &_fdb, port)) {
 		if (!_fdb.aging)
 			break;
-
-		ether_addr_copy(fdb->addr, _fdb.mac);
-		fdb->vid = _fdb.vid;
 		if (_fdb.aging == QCA8K_ATU_STATUS_STATIC)
-			fdb->ndm_state = NUD_NOARP;
+			ndm_state = NUD_NOARP;
 		else
-			fdb->ndm_state = NUD_REACHABLE;
+			ndm_state = NUD_REACHABLE;
 
-		ret = cb(&fdb->obj);
+		ret = dsa_slave_port_fdb_do_dump(dump, _fdb.mac,
+						 _fdb.vid, ndm_state);
 		if (ret)
 			break;
 	}
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 2d85ad2..6a99869 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -277,6 +277,13 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 		return ds->rtable[dst->cpu_dp->ds->index];
 }
 
+struct dsa_slave_dump_ctx {
+	struct net_device *dev;
+	struct sk_buff *skb;
+	struct netlink_callback *cb;
+	int idx;
+};
+
 struct dsa_switch_ops {
 	/*
 	 * Legacy probing.
@@ -392,9 +399,7 @@ struct dsa_switch_ops {
 	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
 				const unsigned char *addr, u16 vid);
 	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
-				 struct switchdev_obj_port_fdb *fdb,
-				  switchdev_obj_dump_cb_t *cb);
-
+				 struct dsa_slave_dump_ctx *dump);
 	/*
 	 * Multicast database
 	 */
@@ -457,6 +462,10 @@ static inline bool netdev_uses_dsa(struct net_device *dev)
 struct dsa_switch *dsa_switch_alloc(struct device *dev, size_t n);
 void dsa_unregister_switch(struct dsa_switch *ds);
 int dsa_register_switch(struct dsa_switch *ds);
+int dsa_slave_port_fdb_do_dump(struct dsa_slave_dump_ctx *dump,
+			       const unsigned char *addr, u16 vid,
+			       u16 ndm_state);
+
 #ifdef CONFIG_PM_SLEEP
 int dsa_switch_suspend(struct dsa_switch *ds);
 int dsa_switch_resume(struct dsa_switch *ds);
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 8ae9e3b..d2637a6 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -208,9 +208,6 @@ int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 			   struct net_device *dev, const unsigned char *addr,
 			   u16 vid);
-int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
-			    struct net_device *dev,
-			    struct net_device *filter_dev, int *idx);
 void switchdev_port_fwd_mark_set(struct net_device *dev,
 				 struct net_device *group_dev,
 				 bool joining);
@@ -309,15 +306,6 @@ static inline int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 	return -EOPNOTSUPP;
 }
 
-static inline int switchdev_port_fdb_dump(struct sk_buff *skb,
-					  struct netlink_callback *cb,
-					  struct net_device *dev,
-					  struct net_device *filter_dev,
-					  int *idx)
-{
-       return *idx;
-}
-
 static inline bool switchdev_port_same_parent_id(struct net_device *a,
 						 struct net_device *b)
 {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 85f53a0..ab4a6a9 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -124,8 +124,6 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
 int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 		     u16 vid);
-int dsa_port_fdb_dump(struct dsa_port *dp, struct switchdev_obj_port_fdb *fdb,
-		      switchdev_obj_dump_cb_t *cb);
 int dsa_port_mdb_add(struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb,
 		     struct switchdev_trans *trans);
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 7378782..659676b 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -173,17 +173,6 @@ int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr,
 	return dsa_port_notify(dp, DSA_NOTIFIER_FDB_DEL, &info);
 }
 
-int dsa_port_fdb_dump(struct dsa_port *dp, struct switchdev_obj_port_fdb *fdb,
-		      switchdev_obj_dump_cb_t *cb)
-{
-	struct dsa_switch *ds = dp->ds;
-
-	if (ds->ops->port_fdb_dump)
-		return ds->ops->port_fdb_dump(ds, dp->index, fdb, cb);
-
-	return -EOPNOTSUPP;
-}
-
 int dsa_port_mdb_add(struct dsa_port *dp,
 		     const struct switchdev_obj_port_mdb *mdb,
 		     struct switchdev_trans *trans)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index acbc0f2..515369f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -199,6 +199,76 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 	return 0;
 }
 
+int dsa_slave_port_fdb_do_dump(struct dsa_slave_dump_ctx *dump,
+			       const unsigned char *addr, u16 vid,
+			       u16 ndm_state)
+{
+	u32 portid = NETLINK_CB(dump->cb->skb).portid;
+	u32 seq = dump->cb->nlh->nlmsg_seq;
+	struct nlmsghdr *nlh;
+	struct ndmsg *ndm;
+
+	if (dump->idx < dump->cb->args[2])
+		goto skip;
+
+	nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH,
+			sizeof(*ndm), NLM_F_MULTI);
+	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_SELF;
+	ndm->ndm_type    = 0;
+	ndm->ndm_ifindex = dump->dev->ifindex;
+	ndm->ndm_state   = ndm_state;
+
+	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, addr))
+		goto nla_put_failure;
+
+	if (vid && nla_put_u16(dump->skb, NDA_VLAN, vid))
+		goto nla_put_failure;
+
+	nlmsg_end(dump->skb, nlh);
+
+skip:
+	dump->idx++;
+	return 0;
+
+nla_put_failure:
+	nlmsg_cancel(dump->skb, nlh);
+	return -EMSGSIZE;
+}
+EXPORT_SYMBOL_GPL(dsa_slave_port_fdb_do_dump);
+
+int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
+			    struct net_device *dev,
+			    struct net_device *filter_dev, int *idx)
+{
+	struct dsa_slave_dump_ctx dump = {
+		.dev = dev,
+		.skb = skb,
+		.cb = cb,
+		.idx = *idx,
+	};
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_port *dp = p->dp;
+	struct dsa_switch *ds = dp->ds;
+	int err;
+
+	if (!ds->ops->port_fdb_dump) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
+out:
+	*idx = dump.idx;
+	return err;
+}
+
 static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
@@ -287,26 +357,6 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 	return err;
 }
 
-static int dsa_slave_port_obj_dump(struct net_device *dev,
-				   struct switchdev_obj *obj,
-				   switchdev_obj_dump_cb_t *cb)
-{
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_port *dp = p->dp;
-	int err;
-
-	switch (obj->id) {
-	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		err = dsa_port_fdb_dump(dp, SWITCHDEV_OBJ_PORT_FDB(obj), cb);
-		break;
-	default:
-		err = -EOPNOTSUPP;
-		break;
-	}
-
-	return err;
-}
-
 static int dsa_slave_port_attr_get(struct net_device *dev,
 				   struct switchdev_attr *attr)
 {
@@ -912,7 +962,7 @@ 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_dump		= switchdev_port_fdb_dump,
+	.ndo_fdb_dump		= dsa_slave_port_fdb_dump,
 	.ndo_do_ioctl		= dsa_slave_ioctl,
 	.ndo_get_iflink		= dsa_slave_get_iflink,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -929,7 +979,6 @@ static const struct switchdev_ops dsa_slave_switchdev_ops = {
 	.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 struct device_type dsa_type = {
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 25dc67e..3d32981 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -1009,90 +1009,6 @@ int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fdb_del);
 
-struct switchdev_fdb_dump {
-	struct switchdev_obj_port_fdb fdb;
-	struct net_device *dev;
-	struct sk_buff *skb;
-	struct netlink_callback *cb;
-	int idx;
-};
-
-static int switchdev_port_fdb_dump_cb(struct switchdev_obj *obj)
-{
-	struct switchdev_obj_port_fdb *fdb = SWITCHDEV_OBJ_PORT_FDB(obj);
-	struct switchdev_fdb_dump *dump =
-		container_of(fdb, struct switchdev_fdb_dump, fdb);
-	u32 portid = NETLINK_CB(dump->cb->skb).portid;
-	u32 seq = dump->cb->nlh->nlmsg_seq;
-	struct nlmsghdr *nlh;
-	struct ndmsg *ndm;
-
-	if (dump->idx < dump->cb->args[2])
-		goto skip;
-
-	nlh = nlmsg_put(dump->skb, portid, seq, RTM_NEWNEIGH,
-			sizeof(*ndm), NLM_F_MULTI);
-	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_SELF;
-	ndm->ndm_type    = 0;
-	ndm->ndm_ifindex = dump->dev->ifindex;
-	ndm->ndm_state   = fdb->ndm_state;
-
-	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, fdb->addr))
-		goto nla_put_failure;
-
-	if (fdb->vid && nla_put_u16(dump->skb, NDA_VLAN, fdb->vid))
-		goto nla_put_failure;
-
-	nlmsg_end(dump->skb, nlh);
-
-skip:
-	dump->idx++;
-	return 0;
-
-nla_put_failure:
-	nlmsg_cancel(dump->skb, nlh);
-	return -EMSGSIZE;
-}
-
-/**
- *	switchdev_port_fdb_dump - Dump port FDB (MAC/VLAN) entries
- *
- *	@skb: netlink skb
- *	@cb: netlink callback
- *	@dev: port device
- *	@filter_dev: filter device
- *	@idx:
- *
- *	Dump FDB entries from switch device.
- */
-int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
-			    struct net_device *dev,
-			    struct net_device *filter_dev, int *idx)
-{
-	struct switchdev_fdb_dump dump = {
-		.fdb.obj.orig_dev = dev,
-		.fdb.obj.id = SWITCHDEV_OBJ_ID_PORT_FDB,
-		.dev = dev,
-		.skb = skb,
-		.cb = cb,
-		.idx = *idx,
-	};
-	int err;
-
-	err = switchdev_port_obj_dump(dev, &dump.fdb.obj,
-				      switchdev_port_fdb_dump_cb);
-	*idx = dump.idx;
-	return err;
-}
-EXPORT_SYMBOL_GPL(switchdev_port_fdb_dump);
-
 bool switchdev_port_same_parent_id(struct net_device *a,
 				   struct net_device *b)
 {
-- 
2.4.11

Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

[Vivien Didelot]

Hi Arkadi,

Arkadi Sharshevsky <arkadis@mellanox.com> writes:

> +struct dsa_slave_dump_ctx {
> +	struct net_device *dev;
> +	struct sk_buff *skb;
> +	struct netlink_callback *cb;
> +	int idx;
> +};
> +
>  struct dsa_switch_ops {
>  	/*
>  	 * Legacy probing.
> @@ -392,9 +399,7 @@ struct dsa_switch_ops {
>  	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
>  				const unsigned char *addr, u16 vid);
>  	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
> -				 struct switchdev_obj_port_fdb *fdb,
> -				  switchdev_obj_dump_cb_t *cb);
> -
> +				 struct dsa_slave_dump_ctx *dump);

I would prefer to pass a callback to the driver, so that we can call
port_fdb_dump for other interfaces like debugfs, and for non exposed
switch ports (CPU and DSA links) as well. Something like:

    typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
                                  bool is_static, void *data);

    int (*port_fdb_dump)(struct dsa_switch *ds, int port,
                         dsa_fdb_dump_cb_t *cb, void *data);

Then dsa_slave_dump_ctx and dsa_slave_port_fdb_do_dump can be static to
net/dsa/slave.c.

> +int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
> +			    struct net_device *dev,
> +			    struct net_device *filter_dev, int *idx)
> +{
> +	struct dsa_slave_dump_ctx dump = {
> +		.dev = dev,
> +		.skb = skb,
> +		.cb = cb,
> +		.idx = *idx,
> +	};
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_port *dp = p->dp;
> +	struct dsa_switch *ds = dp->ds;
> +	int err;
> +
> +	if (!ds->ops->port_fdb_dump) {
> +		err = -EOPNOTSUPP;
> +		goto out;
> +	}
> +
> +	err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
> +out:
> +	*idx = dump.idx;
> +	return err;

There is no need to reassign *idx in case of error:

    if (!ds->ops->port_fdb_dump)
        return -EOPNOTSUPP;

    err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
    *idx = dump.idx;

    return err;

> +	.ndo_fdb_dump		= dsa_slave_port_fdb_dump,

And s/dsa_slave_port_fdb_dump/dsa_slave_fdb_dump/ here will be even
better ;-)


Thanks,

        Vivien

Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

[Arkadi Sharshevsky]

On 07/10/2017 10:36 PM, Vivien Didelot wrote:
> Hi Arkadi,
> 
> Arkadi Sharshevsky <arkadis@mellanox.com> writes:
> 
>> +struct dsa_slave_dump_ctx {
>> +	struct net_device *dev;
>> +	struct sk_buff *skb;
>> +	struct netlink_callback *cb;
>> +	int idx;
>> +};
>> +
>>  struct dsa_switch_ops {
>>  	/*
>>  	 * Legacy probing.
>> @@ -392,9 +399,7 @@ struct dsa_switch_ops {
>>  	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
>>  				const unsigned char *addr, u16 vid);
>>  	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
>> -				 struct switchdev_obj_port_fdb *fdb,
>> -				  switchdev_obj_dump_cb_t *cb);
>> -
>> +				 struct dsa_slave_dump_ctx *dump);
> 
> I would prefer to pass a callback to the driver, so that we can call
> port_fdb_dump for other interfaces like debugfs, and for non exposed
> switch ports (CPU and DSA links) as well. Something like:
> 
>     typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
>                                   bool is_static, void *data);
> 
>     int (*port_fdb_dump)(struct dsa_switch *ds, int port,
>                          dsa_fdb_dump_cb_t *cb, void *data);
> 
> Then dsa_slave_dump_ctx and dsa_slave_port_fdb_do_dump can be static to
> net/dsa/slave.c.
> 

Originally thought about it. But it makes sense to pass a callback
when different functions can be supplied.

Here is only one option which is the fdb_do_dump(). So I thought
exporting it makes more sense..

>> +int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>> +			    struct net_device *dev,
>> +			    struct net_device *filter_dev, int *idx)
>> +{
>> +	struct dsa_slave_dump_ctx dump = {
>> +		.dev = dev,
>> +		.skb = skb,
>> +		.cb = cb,
>> +		.idx = *idx,
>> +	};
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_port *dp = p->dp;
>> +	struct dsa_switch *ds = dp->ds;
>> +	int err;
>> +
>> +	if (!ds->ops->port_fdb_dump) {
>> +		err = -EOPNOTSUPP;
>> +		goto out;
>> +	}
>> +
>> +	err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
>> +out:
>> +	*idx = dump.idx;
>> +	return err;
> 
> There is no need to reassign *idx in case of error:
> 
>     if (!ds->ops->port_fdb_dump)
>         return -EOPNOTSUPP;
> 
>     err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
>     *idx = dump.idx;
> 
>     return err;
>
Will fix, thanks.

>> +	.ndo_fdb_dump		= dsa_slave_port_fdb_dump,
> 
> And s/dsa_slave_port_fdb_dump/dsa_slave_fdb_dump/ here will be even
> better ;-)
> 
> 
> Thanks,
> 
>         Vivien
> 
Will fix, thanks.

[PATCH net-next RFC 11/12] net: bridge: Remove FDB deletion through switchdev object

[Arkadi Sharshevsky]

At this point no driver supports FDB add/del through switchdev object
but rather via notification chain, thus, it is removed.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 net/bridge/br_fdb.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index a5e4a73..a79b648 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -169,29 +169,11 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr)
 	}
 }
 
-static void fdb_del_external_learn(struct net_bridge_fdb_entry *f)
-{
-	struct switchdev_obj_port_fdb fdb = {
-		.obj = {
-			.orig_dev = f->dst->dev,
-			.id = SWITCHDEV_OBJ_ID_PORT_FDB,
-			.flags = SWITCHDEV_F_DEFER,
-		},
-		.vid = f->vlan_id,
-	};
-
-	ether_addr_copy(fdb.addr, f->addr.addr);
-	switchdev_port_obj_del(f->dst->dev, &fdb.obj);
-}
-
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
 {
 	if (f->is_static)
 		fdb_del_hw_addr(br, f->addr.addr);
 
-	if (f->added_by_external_learn)
-		fdb_del_external_learn(f);
-
 	hlist_del_init_rcu(&f->hlist);
 	fdb_notify(br, f, RTM_DELNEIGH);
 	call_rcu(&f->rcu, fdb_rcu_free);
-- 
2.4.11

[PATCH net-next RFC 12/12] net: switchdev: Remove bridge bypass support from switchdev

[Arkadi Sharshevsky]

Currently the bridge port flags, vlans, FDBs and MDBs can be offloaded
through the bridge code, making the switchdev's SELF bridge bypass
implementation to be redundant. This implies several changes:
- No need for dump infra in switchdev, DSA's special case is handled
  privately.
- Remove obj_dump from switchdev_ops.
- FDBs are removed from obj_add/del routines, due to the fact that they
  are offloaded through the bridge notifcation chain.
- The switchdev_port_bridge_xx() and switchdev_port_fdb_xx() functions
  can be removed.

Signed-off-by: Arkadi Sharshevsky <arkadis@mellanox.com>
---
 include/net/switchdev.h   |  75 --------
 net/switchdev/switchdev.c | 435 ----------------------------------------------
 2 files changed, 510 deletions(-)

diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index d2637a6..d767b79 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -74,7 +74,6 @@ struct switchdev_attr {
 enum switchdev_obj_id {
 	SWITCHDEV_OBJ_ID_UNDEFINED,
 	SWITCHDEV_OBJ_ID_PORT_VLAN,
-	SWITCHDEV_OBJ_ID_PORT_FDB,
 	SWITCHDEV_OBJ_ID_PORT_MDB,
 };
 
@@ -97,17 +96,6 @@ struct switchdev_obj_port_vlan {
 #define SWITCHDEV_OBJ_PORT_VLAN(obj) \
 	container_of(obj, struct switchdev_obj_port_vlan, obj)
 
-/* SWITCHDEV_OBJ_ID_PORT_FDB */
-struct switchdev_obj_port_fdb {
-	struct switchdev_obj obj;
-	unsigned char addr[ETH_ALEN];
-	u16 vid;
-	u16 ndm_state;
-};
-
-#define SWITCHDEV_OBJ_PORT_FDB(obj) \
-	container_of(obj, struct switchdev_obj_port_fdb, obj)
-
 /* SWITCHDEV_OBJ_ID_PORT_MDB */
 struct switchdev_obj_port_mdb {
 	struct switchdev_obj obj;
@@ -135,8 +123,6 @@ typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
  * @switchdev_port_obj_add: Add an object to port (see switchdev_obj_*).
  *
  * @switchdev_port_obj_del: Delete an object from port (see switchdev_obj_*).
- *
- * @switchdev_port_obj_dump: Dump port objects (see switchdev_obj_*).
  */
 struct switchdev_ops {
 	int	(*switchdev_port_attr_get)(struct net_device *dev,
@@ -149,9 +135,6 @@ struct switchdev_ops {
 					  struct switchdev_trans *trans);
 	int	(*switchdev_port_obj_del)(struct net_device *dev,
 					  const struct switchdev_obj *obj);
-	int	(*switchdev_port_obj_dump)(struct net_device *dev,
-					   struct switchdev_obj *obj,
-					   switchdev_obj_dump_cb_t *cb);
 };
 
 enum switchdev_notifier_type {
@@ -189,25 +172,10 @@ int switchdev_port_obj_add(struct net_device *dev,
 			   const struct switchdev_obj *obj);
 int switchdev_port_obj_del(struct net_device *dev,
 			   const struct switchdev_obj *obj);
-int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
-			    switchdev_obj_dump_cb_t *cb);
 int register_switchdev_notifier(struct notifier_block *nb);
 int unregister_switchdev_notifier(struct notifier_block *nb);
 int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
 			     struct switchdev_notifier_info *info);
-int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
-				  struct net_device *dev, u32 filter_mask,
-				  int nlflags);
-int switchdev_port_bridge_setlink(struct net_device *dev,
-				  struct nlmsghdr *nlh, u16 flags);
-int switchdev_port_bridge_dellink(struct net_device *dev,
-				  struct nlmsghdr *nlh, u16 flags);
-int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
-			   struct net_device *dev, const unsigned char *addr,
-			   u16 vid, u16 nlm_flags);
-int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
-			   struct net_device *dev, const unsigned char *addr,
-			   u16 vid);
 void switchdev_port_fwd_mark_set(struct net_device *dev,
 				 struct net_device *group_dev,
 				 bool joining);
@@ -246,13 +214,6 @@ static inline int switchdev_port_obj_del(struct net_device *dev,
 	return -EOPNOTSUPP;
 }
 
-static inline int switchdev_port_obj_dump(struct net_device *dev,
-					  const struct switchdev_obj *obj,
-					  switchdev_obj_dump_cb_t *cb)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline int register_switchdev_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -270,42 +231,6 @@ static inline int call_switchdev_notifiers(unsigned long val,
 	return NOTIFY_DONE;
 }
 
-static inline int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid,
-					    u32 seq, struct net_device *dev,
-					    u32 filter_mask, int nlflags)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int switchdev_port_bridge_setlink(struct net_device *dev,
-						struct nlmsghdr *nlh,
-						u16 flags)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int switchdev_port_bridge_dellink(struct net_device *dev,
-						struct nlmsghdr *nlh,
-						u16 flags)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
-					 struct net_device *dev,
-					 const unsigned char *addr,
-					 u16 vid, u16 nlm_flags)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
-					 struct net_device *dev,
-					 const unsigned char *addr, u16 vid)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline bool switchdev_port_same_parent_id(struct net_device *a,
 						 struct net_device *b)
 {
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 3d32981..0531b41 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -343,8 +343,6 @@ static size_t switchdev_obj_size(const struct switchdev_obj *obj)
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
 		return sizeof(struct switchdev_obj_port_vlan);
-	case SWITCHDEV_OBJ_ID_PORT_FDB:
-		return sizeof(struct switchdev_obj_port_fdb);
 	case SWITCHDEV_OBJ_ID_PORT_MDB:
 		return sizeof(struct switchdev_obj_port_mdb);
 	default:
@@ -534,43 +532,6 @@ int switchdev_port_obj_del(struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
 
-/**
- *	switchdev_port_obj_dump - Dump port objects
- *
- *	@dev: port device
- *	@id: object ID
- *	@obj: object to dump
- *	@cb: function to call with a filled object
- *
- *	rtnl_lock must be held.
- */
-int switchdev_port_obj_dump(struct net_device *dev, struct switchdev_obj *obj,
-			    switchdev_obj_dump_cb_t *cb)
-{
-	const struct switchdev_ops *ops = dev->switchdev_ops;
-	struct net_device *lower_dev;
-	struct list_head *iter;
-	int err = -EOPNOTSUPP;
-
-	ASSERT_RTNL();
-
-	if (ops && ops->switchdev_port_obj_dump)
-		return ops->switchdev_port_obj_dump(dev, obj, cb);
-
-	/* Switch device port(s) may be stacked under
-	 * bond/team/vlan dev, so recurse down to dump objects on
-	 * first port at bottom of stack.
-	 */
-
-	netdev_for_each_lower_dev(dev, lower_dev, iter) {
-		err = switchdev_port_obj_dump(lower_dev, obj, cb);
-		break;
-	}
-
-	return err;
-}
-EXPORT_SYMBOL_GPL(switchdev_port_obj_dump);
-
 static ATOMIC_NOTIFIER_HEAD(switchdev_notif_chain);
 
 /**
@@ -613,402 +574,6 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
 }
 EXPORT_SYMBOL_GPL(call_switchdev_notifiers);
 
-struct switchdev_vlan_dump {
-	struct switchdev_obj_port_vlan vlan;
-	struct sk_buff *skb;
-	u32 filter_mask;
-	u16 flags;
-	u16 begin;
-	u16 end;
-};
-
-static int switchdev_port_vlan_dump_put(struct switchdev_vlan_dump *dump)
-{
-	struct bridge_vlan_info vinfo;
-
-	vinfo.flags = dump->flags;
-
-	if (dump->begin == 0 && dump->end == 0) {
-		return 0;
-	} else if (dump->begin == dump->end) {
-		vinfo.vid = dump->begin;
-		if (nla_put(dump->skb, IFLA_BRIDGE_VLAN_INFO,
-			    sizeof(vinfo), &vinfo))
-			return -EMSGSIZE;
-	} else {
-		vinfo.vid = dump->begin;
-		vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_BEGIN;
-		if (nla_put(dump->skb, IFLA_BRIDGE_VLAN_INFO,
-			    sizeof(vinfo), &vinfo))
-			return -EMSGSIZE;
-		vinfo.vid = dump->end;
-		vinfo.flags &= ~BRIDGE_VLAN_INFO_RANGE_BEGIN;
-		vinfo.flags |= BRIDGE_VLAN_INFO_RANGE_END;
-		if (nla_put(dump->skb, IFLA_BRIDGE_VLAN_INFO,
-			    sizeof(vinfo), &vinfo))
-			return -EMSGSIZE;
-	}
-
-	return 0;
-}
-
-static int switchdev_port_vlan_dump_cb(struct switchdev_obj *obj)
-{
-	struct switchdev_obj_port_vlan *vlan = SWITCHDEV_OBJ_PORT_VLAN(obj);
-	struct switchdev_vlan_dump *dump =
-		container_of(vlan, struct switchdev_vlan_dump, vlan);
-	int err = 0;
-
-	if (vlan->vid_begin > vlan->vid_end)
-		return -EINVAL;
-
-	if (dump->filter_mask & RTEXT_FILTER_BRVLAN) {
-		dump->flags = vlan->flags;
-		for (dump->begin = dump->end = vlan->vid_begin;
-		     dump->begin <= vlan->vid_end;
-		     dump->begin++, dump->end++) {
-			err = switchdev_port_vlan_dump_put(dump);
-			if (err)
-				return err;
-		}
-	} else if (dump->filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED) {
-		if (dump->begin > vlan->vid_begin &&
-		    dump->begin >= vlan->vid_end) {
-			if ((dump->begin - 1) == vlan->vid_end &&
-			    dump->flags == vlan->flags) {
-				/* prepend */
-				dump->begin = vlan->vid_begin;
-			} else {
-				err = switchdev_port_vlan_dump_put(dump);
-				dump->flags = vlan->flags;
-				dump->begin = vlan->vid_begin;
-				dump->end = vlan->vid_end;
-			}
-		} else if (dump->end <= vlan->vid_begin &&
-		           dump->end < vlan->vid_end) {
-			if ((dump->end  + 1) == vlan->vid_begin &&
-			    dump->flags == vlan->flags) {
-				/* append */
-				dump->end = vlan->vid_end;
-			} else {
-				err = switchdev_port_vlan_dump_put(dump);
-				dump->flags = vlan->flags;
-				dump->begin = vlan->vid_begin;
-				dump->end = vlan->vid_end;
-			}
-		} else {
-			err = -EINVAL;
-		}
-	}
-
-	return err;
-}
-
-static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
-				    u32 filter_mask)
-{
-	struct switchdev_vlan_dump dump = {
-		.vlan.obj.orig_dev = dev,
-		.vlan.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.skb = skb,
-		.filter_mask = filter_mask,
-	};
-	int err = 0;
-
-	if ((filter_mask & RTEXT_FILTER_BRVLAN) ||
-	    (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)) {
-		err = switchdev_port_obj_dump(dev, &dump.vlan.obj,
-					      switchdev_port_vlan_dump_cb);
-		if (err)
-			goto err_out;
-		if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
-			/* last one */
-			err = switchdev_port_vlan_dump_put(&dump);
-	}
-
-err_out:
-	return err == -EOPNOTSUPP ? 0 : err;
-}
-
-/**
- *	switchdev_port_bridge_getlink - Get bridge port attributes
- *
- *	@dev: port device
- *
- *	Called for SELF on rtnl_bridge_getlink to get bridge port
- *	attributes.
- */
-int switchdev_port_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
-				  struct net_device *dev, u32 filter_mask,
-				  int nlflags)
-{
-	struct switchdev_attr attr = {
-		.orig_dev = dev,
-		.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
-	};
-	u16 mode = BRIDGE_MODE_UNDEF;
-	u32 mask = BR_LEARNING | BR_LEARNING_SYNC | BR_FLOOD;
-	int err;
-
-	if (!netif_is_bridge_port(dev))
-		return -EOPNOTSUPP;
-
-	err = switchdev_port_attr_get(dev, &attr);
-	if (err && err != -EOPNOTSUPP)
-		return err;
-
-	return ndo_dflt_bridge_getlink(skb, pid, seq, dev, mode,
-				       attr.u.brport_flags, mask, nlflags,
-				       filter_mask, switchdev_port_vlan_fill);
-}
-EXPORT_SYMBOL_GPL(switchdev_port_bridge_getlink);
-
-static int switchdev_port_br_setflag(struct net_device *dev,
-				     struct nlattr *nlattr,
-				     unsigned long brport_flag)
-{
-	struct switchdev_attr attr = {
-		.orig_dev = dev,
-		.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS,
-	};
-	u8 flag = nla_get_u8(nlattr);
-	int err;
-
-	err = switchdev_port_attr_get(dev, &attr);
-	if (err)
-		return err;
-
-	if (flag)
-		attr.u.brport_flags |= brport_flag;
-	else
-		attr.u.brport_flags &= ~brport_flag;
-
-	return switchdev_port_attr_set(dev, &attr);
-}
-
-static const struct nla_policy
-switchdev_port_bridge_policy[IFLA_BRPORT_MAX + 1] = {
-	[IFLA_BRPORT_STATE]		= { .type = NLA_U8 },
-	[IFLA_BRPORT_COST]		= { .type = NLA_U32 },
-	[IFLA_BRPORT_PRIORITY]		= { .type = NLA_U16 },
-	[IFLA_BRPORT_MODE]		= { .type = NLA_U8 },
-	[IFLA_BRPORT_GUARD]		= { .type = NLA_U8 },
-	[IFLA_BRPORT_PROTECT]		= { .type = NLA_U8 },
-	[IFLA_BRPORT_FAST_LEAVE]	= { .type = NLA_U8 },
-	[IFLA_BRPORT_LEARNING]		= { .type = NLA_U8 },
-	[IFLA_BRPORT_LEARNING_SYNC]	= { .type = NLA_U8 },
-	[IFLA_BRPORT_UNICAST_FLOOD]	= { .type = NLA_U8 },
-};
-
-static int switchdev_port_br_setlink_protinfo(struct net_device *dev,
-					      struct nlattr *protinfo)
-{
-	struct nlattr *attr;
-	int rem;
-	int err;
-
-	err = nla_validate_nested(protinfo, IFLA_BRPORT_MAX,
-				  switchdev_port_bridge_policy, NULL);
-	if (err)
-		return err;
-
-	nla_for_each_nested(attr, protinfo, rem) {
-		switch (nla_type(attr)) {
-		case IFLA_BRPORT_LEARNING:
-			err = switchdev_port_br_setflag(dev, attr,
-							BR_LEARNING);
-			break;
-		case IFLA_BRPORT_LEARNING_SYNC:
-			err = switchdev_port_br_setflag(dev, attr,
-							BR_LEARNING_SYNC);
-			break;
-		case IFLA_BRPORT_UNICAST_FLOOD:
-			err = switchdev_port_br_setflag(dev, attr, BR_FLOOD);
-			break;
-		default:
-			err = -EOPNOTSUPP;
-			break;
-		}
-		if (err)
-			return err;
-	}
-
-	return 0;
-}
-
-static int switchdev_port_br_afspec(struct net_device *dev,
-				    struct nlattr *afspec,
-				    int (*f)(struct net_device *dev,
-					     const struct switchdev_obj *obj))
-{
-	struct nlattr *attr;
-	struct bridge_vlan_info *vinfo;
-	struct switchdev_obj_port_vlan vlan = {
-		.obj.orig_dev = dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-	};
-	int rem;
-	int err;
-
-	nla_for_each_nested(attr, afspec, rem) {
-		if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
-			continue;
-		if (nla_len(attr) != sizeof(struct bridge_vlan_info))
-			return -EINVAL;
-		vinfo = nla_data(attr);
-		if (!vinfo->vid || vinfo->vid >= VLAN_VID_MASK)
-			return -EINVAL;
-		vlan.flags = vinfo->flags;
-		if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
-			if (vlan.vid_begin)
-				return -EINVAL;
-			vlan.vid_begin = vinfo->vid;
-			/* don't allow range of pvids */
-			if (vlan.flags & BRIDGE_VLAN_INFO_PVID)
-				return -EINVAL;
-		} else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
-			if (!vlan.vid_begin)
-				return -EINVAL;
-			vlan.vid_end = vinfo->vid;
-			if (vlan.vid_end <= vlan.vid_begin)
-				return -EINVAL;
-			err = f(dev, &vlan.obj);
-			if (err)
-				return err;
-			vlan.vid_begin = 0;
-		} else {
-			if (vlan.vid_begin)
-				return -EINVAL;
-			vlan.vid_begin = vinfo->vid;
-			vlan.vid_end = vinfo->vid;
-			err = f(dev, &vlan.obj);
-			if (err)
-				return err;
-			vlan.vid_begin = 0;
-		}
-	}
-
-	return 0;
-}
-
-/**
- *	switchdev_port_bridge_setlink - Set bridge port attributes
- *
- *	@dev: port device
- *	@nlh: netlink header
- *	@flags: netlink flags
- *
- *	Called for SELF on rtnl_bridge_setlink to set bridge port
- *	attributes.
- */
-int switchdev_port_bridge_setlink(struct net_device *dev,
-				  struct nlmsghdr *nlh, u16 flags)
-{
-	struct nlattr *protinfo;
-	struct nlattr *afspec;
-	int err = 0;
-
-	if (!netif_is_bridge_port(dev))
-		return -EOPNOTSUPP;
-
-	protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
-				   IFLA_PROTINFO);
-	if (protinfo) {
-		err = switchdev_port_br_setlink_protinfo(dev, protinfo);
-		if (err)
-			return err;
-	}
-
-	afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
-				 IFLA_AF_SPEC);
-	if (afspec)
-		err = switchdev_port_br_afspec(dev, afspec,
-					       switchdev_port_obj_add);
-
-	return err;
-}
-EXPORT_SYMBOL_GPL(switchdev_port_bridge_setlink);
-
-/**
- *	switchdev_port_bridge_dellink - Set bridge port attributes
- *
- *	@dev: port device
- *	@nlh: netlink header
- *	@flags: netlink flags
- *
- *	Called for SELF on rtnl_bridge_dellink to set bridge port
- *	attributes.
- */
-int switchdev_port_bridge_dellink(struct net_device *dev,
-				  struct nlmsghdr *nlh, u16 flags)
-{
-	struct nlattr *afspec;
-
-	if (!netif_is_bridge_port(dev))
-		return -EOPNOTSUPP;
-
-	afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg),
-				 IFLA_AF_SPEC);
-	if (afspec)
-		return switchdev_port_br_afspec(dev, afspec,
-						switchdev_port_obj_del);
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(switchdev_port_bridge_dellink);
-
-/**
- *	switchdev_port_fdb_add - Add FDB (MAC/VLAN) entry to port
- *
- *	@ndmsg: netlink hdr
- *	@nlattr: netlink attributes
- *	@dev: port device
- *	@addr: MAC address to add
- *	@vid: VLAN to add
- *
- *	Add FDB entry to switch device.
- */
-int switchdev_port_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
-			   struct net_device *dev, const unsigned char *addr,
-			   u16 vid, u16 nlm_flags)
-{
-	struct switchdev_obj_port_fdb fdb = {
-		.obj.orig_dev = dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_FDB,
-		.vid = vid,
-	};
-
-	ether_addr_copy(fdb.addr, addr);
-	return switchdev_port_obj_add(dev, &fdb.obj);
-}
-EXPORT_SYMBOL_GPL(switchdev_port_fdb_add);
-
-/**
- *	switchdev_port_fdb_del - Delete FDB (MAC/VLAN) entry from port
- *
- *	@ndmsg: netlink hdr
- *	@nlattr: netlink attributes
- *	@dev: port device
- *	@addr: MAC address to delete
- *	@vid: VLAN to delete
- *
- *	Delete FDB entry from switch device.
- */
-int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
-			   struct net_device *dev, const unsigned char *addr,
-			   u16 vid)
-{
-	struct switchdev_obj_port_fdb fdb = {
-		.obj.orig_dev = dev,
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_FDB,
-		.vid = vid,
-	};
-
-	ether_addr_copy(fdb.addr, addr);
-	return switchdev_port_obj_del(dev, &fdb.obj);
-}
-EXPORT_SYMBOL_GPL(switchdev_port_fdb_del);
-
 bool switchdev_port_same_parent_id(struct net_device *a,
 				   struct net_device *b)
 {
-- 
2.4.11