[PATCH net-next 0/6] net: dsa: simplify switchdev prepare phase

[PATCH net-next 0/6] net: dsa: simplify switchdev prepare phase

[Vivien Didelot]

This patch series brings no functional changes.

It removes the unused switchdev_trans arguments from the dsa_switch_ops
for both MDB and VLAN operations, and provides function to prepare and
add these objects for a given bitmap of ports.

Vivien Didelot (6):
  net: dsa: remove trans argument from mdb ops
  net: dsa: return after mdb prepare phase
  net: dsa: add switch mdb bitmap functions
  net: dsa: remove trans argument from vlan ops
  net: dsa: return after vlan prepare phase
  net: dsa: add switch vlan bitmap functions

 drivers/net/dsa/b53/b53_common.c       |  6 +--
 drivers/net/dsa/b53/b53_priv.h         |  6 +--
 drivers/net/dsa/dsa_loop.c             |  9 ++--
 drivers/net/dsa/lan9303-core.c         |  6 +--
 drivers/net/dsa/microchip/ksz_common.c | 12 ++---
 drivers/net/dsa/mv88e6xxx/chip.c       | 12 ++---
 include/net/dsa.h                      | 20 +++-----
 net/dsa/switch.c                       | 93 ++++++++++++++++++++++++----------
 8 files changed, 93 insertions(+), 71 deletions(-)

-- 
2.15.0

[PATCH net-next 1/6] net: dsa: remove trans argument from mdb ops

[Vivien Didelot]

The DSA switch MDB ops pass the switchdev_trans structure down to the
drivers, but no one is using them and they aren't supposed to anyway.

Remove the trans argument from MDB prepare and add operations.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/lan9303-core.c         |  6 ++----
 drivers/net/dsa/microchip/ksz_common.c |  6 ++----
 drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++----
 include/net/dsa.h                      | 10 ++++------
 net/dsa/switch.c                       |  4 ++--
 5 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 320651a57c6f..26dcc8f82c2e 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1190,8 +1190,7 @@ static int lan9303_port_fdb_dump(struct dsa_switch *ds, int port,
 }
 
 static int lan9303_port_mdb_prepare(struct dsa_switch *ds, int port,
-				    const struct switchdev_obj_port_mdb *mdb,
-				    struct switchdev_trans *trans)
+				    const struct switchdev_obj_port_mdb *mdb)
 {
 	struct lan9303 *chip = ds->priv;
 
@@ -1208,8 +1207,7 @@ static int lan9303_port_mdb_prepare(struct dsa_switch *ds, int port,
 }
 
 static void lan9303_port_mdb_add(struct dsa_switch *ds, int port,
-				 const struct switchdev_obj_port_mdb *mdb,
-				 struct switchdev_trans *trans)
+				 const struct switchdev_obj_port_mdb *mdb)
 {
 	struct lan9303 *chip = ds->priv;
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 56cd6d365352..062a32f9ed06 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -857,16 +857,14 @@ static int ksz_port_fdb_dump(struct dsa_switch *ds, int port,
 }
 
 static int ksz_port_mdb_prepare(struct dsa_switch *ds, int port,
-				const struct switchdev_obj_port_mdb *mdb,
-				struct switchdev_trans *trans)
+				const struct switchdev_obj_port_mdb *mdb)
 {
 	/* nothing to do */
 	return 0;
 }
 
 static void ksz_port_mdb_add(struct dsa_switch *ds, int port,
-			     const struct switchdev_obj_port_mdb *mdb,
-			     struct switchdev_trans *trans)
+			     const struct switchdev_obj_port_mdb *mdb)
 {
 	struct ksz_device *dev = ds->priv;
 	u32 static_table[4];
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 09a66d4d9492..55026cc7e43c 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3748,8 +3748,7 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 }
 
 static int mv88e6xxx_port_mdb_prepare(struct dsa_switch *ds, int port,
-				      const struct switchdev_obj_port_mdb *mdb,
-				      struct switchdev_trans *trans)
+				      const struct switchdev_obj_port_mdb *mdb)
 {
 	/* We don't need any dynamic resource from the kernel (yet),
 	 * so skip the prepare phase.
@@ -3759,8 +3758,7 @@ static int mv88e6xxx_port_mdb_prepare(struct dsa_switch *ds, int port,
 }
 
 static void mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port,
-				   const struct switchdev_obj_port_mdb *mdb,
-				   struct switchdev_trans *trans)
+				   const struct switchdev_obj_port_mdb *mdb)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index e54332968417..d48ced87a44d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -431,12 +431,10 @@ struct dsa_switch_ops {
 	/*
 	 * Multicast database
 	 */
-	int	(*port_mdb_prepare)(struct dsa_switch *ds, int port,
-				    const struct switchdev_obj_port_mdb *mdb,
-				    struct switchdev_trans *trans);
-	void	(*port_mdb_add)(struct dsa_switch *ds, int port,
-				const struct switchdev_obj_port_mdb *mdb,
-				struct switchdev_trans *trans);
+	int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_mdb *mdb);
+	void (*port_mdb_add)(struct dsa_switch *ds, int port,
+			     const struct switchdev_obj_port_mdb *mdb);
 	int	(*port_mdb_del)(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_mdb *mdb);
 	/*
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index e6c06aa349a6..04e8ad2c3d5d 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -129,14 +129,14 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 			return -EOPNOTSUPP;
 
 		for_each_set_bit(port, group, ds->num_ports) {
-			err = ds->ops->port_mdb_prepare(ds, port, mdb, trans);
+			err = ds->ops->port_mdb_prepare(ds, port, mdb);
 			if (err)
 				return err;
 		}
 	}
 
 	for_each_set_bit(port, group, ds->num_ports)
-		ds->ops->port_mdb_add(ds, port, mdb, trans);
+		ds->ops->port_mdb_add(ds, port, mdb);
 
 	return 0;
 }
-- 
2.15.0

Re: [PATCH net-next 1/6] net: dsa: remove trans argument from mdb ops

[Egil Hjelmeland]

On 08. nov. 2017 18:19, Vivien Didelot wrote:
> The DSA switch MDB ops pass the switchdev_trans structure down to the
> drivers, but no one is using them and they aren't supposed to anyway.
> 
> Remove the trans argument from MDB prepare and add operations.
> 
> -	int	(*port_mdb_prepare)(struct dsa_switch *ds, int port,
> -				    const struct switchdev_obj_port_mdb *mdb,
> -				    struct switchdev_trans *trans);
> -	void	(*port_mdb_add)(struct dsa_switch *ds, int port,
> -				const struct switchdev_obj_port_mdb *mdb,
> -				struct switchdev_trans *trans);
> +	int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_mdb *mdb);
> +	void (*port_mdb_add)(struct dsa_switch *ds, int port,
> +			     const struct switchdev_obj_port_mdb *mdb);
>   	int	(*port_mdb_del)(struct dsa_switch *ds, int port,
>   				const struct switchdev_obj_port_mdb *mdb);

Hi Vivien
Nice to get rid of "trans". I recall I was confused by this parameter. 
"Am I supposed to do something with this parameter?".

But when at it. What about getting rid of switchdev_obj_port_mdb, making 
similar signatures as the new .port_fdb_xxx functions? Would that make 
sense?

Egil

Re: [PATCH net-next 1/6] net: dsa: remove trans argument from mdb ops

[Vivien Didelot]

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> On 08. nov. 2017 18:19, Vivien Didelot wrote:
>> The DSA switch MDB ops pass the switchdev_trans structure down to the
>> drivers, but no one is using them and they aren't supposed to anyway.
>> 
>> Remove the trans argument from MDB prepare and add operations.
>> 
>> -	int	(*port_mdb_prepare)(struct dsa_switch *ds, int port,
>> -				    const struct switchdev_obj_port_mdb *mdb,
>> -				    struct switchdev_trans *trans);
>> -	void	(*port_mdb_add)(struct dsa_switch *ds, int port,
>> -				const struct switchdev_obj_port_mdb *mdb,
>> -				struct switchdev_trans *trans);
>> +	int (*port_mdb_prepare)(struct dsa_switch *ds, int port,
>> +				const struct switchdev_obj_port_mdb *mdb);
>> +	void (*port_mdb_add)(struct dsa_switch *ds, int port,
>> +			     const struct switchdev_obj_port_mdb *mdb);
>>   	int	(*port_mdb_del)(struct dsa_switch *ds, int port,
>>   				const struct switchdev_obj_port_mdb *mdb);
>
> Hi Vivien
> Nice to get rid of "trans". I recall I was confused by this parameter. 
> "Am I supposed to do something with this parameter?".
>
> But when at it. What about getting rid of switchdev_obj_port_mdb, making 
> similar signatures as the new .port_fdb_xxx functions? Would that make 
> sense?

There is definitely something to do to factorize FDB and MDB operations,
since an Ethernet switch driver will only manipulate an address database
in the end. I have something in mind but this is out-of-scope ATM.

Thanks,

        Vivien

[PATCH net-next 2/6] net: dsa: return after mdb prepare phase

[Vivien Didelot]

The current code does not return after successfully preparing the MDB
addition on every ports member of a multicast group. Fix this.

Fixes: a1a6b7ea7f2d ("net: dsa: add cross-chip multicast support")
Reported-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/switch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 04e8ad2c3d5d..0041aba5c339 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -133,6 +133,8 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 			if (err)
 				return err;
 		}
+
+		return 0;
 	}
 
 	for_each_set_bit(port, group, ds->num_ports)
-- 
2.15.0

Re: [PATCH net-next 2/6] net: dsa: return after mdb prepare phase

[Egil Hjelmeland]

On 08. nov. 2017 18:19, Vivien Didelot wrote:
> The current code does not return after successfully preparing the MDB
> addition on every ports member of a multicast group. Fix this.
> 
> Fixes: a1a6b7ea7f2d ("net: dsa: add cross-chip multicast support")
> Reported-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   net/dsa/switch.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 04e8ad2c3d5d..0041aba5c339 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -133,6 +133,8 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
>   			if (err)
>   				return err;
>   		}
> +
> +		return 0;
>   	}
>   
>   	for_each_set_bit(port, group, ds->num_ports)
> 

Hi Vivien!

Will this cause a merge-conflict with the "net" patch you sent, when 
"net" is merged to "net-next"? Perhaps more polite to hold on to this 
(and following patches) until "net" patch has trickled through the system?

Regards
Egil

[PATCH net-next 3/6] net: dsa: add switch mdb bitmap functions

[Vivien Didelot]

This patch brings no functional changes.
It moves out the MDB code iterating on a multicast group into new
dsa_switch_mdb_{prepare,add}_bitmap() functions.

This gives us a better isolation of the two switchdev phases.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/switch.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 0041aba5c339..8bfbbe408e82 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -108,13 +108,42 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 				     info->vid);
 }
 
+static int
+dsa_switch_mdb_prepare_bitmap(struct dsa_switch *ds,
+			      const struct switchdev_obj_port_mdb *mdb,
+			      const unsigned long *bitmap)
+{
+	int port, err;
+
+	if (!ds->ops->port_mdb_prepare || !ds->ops->port_mdb_add)
+		return -EOPNOTSUPP;
+
+	for_each_set_bit(port, bitmap, ds->num_ports) {
+		err = ds->ops->port_mdb_prepare(ds, port, mdb);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static void dsa_switch_mdb_add_bitmap(struct dsa_switch *ds,
+				      const struct switchdev_obj_port_mdb *mdb,
+				      const unsigned long *bitmap)
+{
+	int port;
+
+	for_each_set_bit(port, bitmap, ds->num_ports)
+		ds->ops->port_mdb_add(ds, port, mdb);
+}
+
 static int dsa_switch_mdb_add(struct dsa_switch *ds,
 			      struct dsa_notifier_mdb_info *info)
 {
 	const struct switchdev_obj_port_mdb *mdb = info->mdb;
 	struct switchdev_trans *trans = info->trans;
 	DECLARE_BITMAP(group, ds->num_ports);
-	int port, err;
+	int port;
 
 	/* Build a mask of Multicast group members */
 	bitmap_zero(group, ds->num_ports);
@@ -124,21 +153,10 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,
 		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
 			set_bit(port, group);
 
-	if (switchdev_trans_ph_prepare(trans)) {
-		if (!ds->ops->port_mdb_prepare || !ds->ops->port_mdb_add)
-			return -EOPNOTSUPP;
+	if (switchdev_trans_ph_prepare(trans))
+		return dsa_switch_mdb_prepare_bitmap(ds, mdb, group);
 
-		for_each_set_bit(port, group, ds->num_ports) {
-			err = ds->ops->port_mdb_prepare(ds, port, mdb);
-			if (err)
-				return err;
-		}
-
-		return 0;
-	}
-
-	for_each_set_bit(port, group, ds->num_ports)
-		ds->ops->port_mdb_add(ds, port, mdb);
+	dsa_switch_mdb_add_bitmap(ds, mdb, group);
 
 	return 0;
 }
-- 
2.15.0

[PATCH net-next 4/6] net: dsa: remove trans argument from vlan ops

[Vivien Didelot]

The DSA switch VLAN ops pass the switchdev_trans structure down to the
drivers, but no one is using them and they aren't supposed to anyway.

Remove the trans argument from VLAN prepare and add operations.

At the same time, fix the following checkpatch warning:

    WARNING: line over 80 characters
    #74: FILE: drivers/net/dsa/dsa_loop.c:177:
    +				      const struct switchdev_obj_port_vlan *vlan)

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/b53/b53_common.c       |  6 ++----
 drivers/net/dsa/b53/b53_priv.h         |  6 ++----
 drivers/net/dsa/dsa_loop.c             |  9 ++++-----
 drivers/net/dsa/microchip/ksz_common.c |  6 ++----
 drivers/net/dsa/mv88e6xxx/chip.c       |  6 ++----
 include/net/dsa.h                      | 10 ++++------
 net/dsa/switch.c                       |  4 ++--
 7 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a7ca62ba27b7..96f8eaa8d05b 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1030,8 +1030,7 @@ int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering)
 EXPORT_SYMBOL(b53_vlan_filtering);
 
 int b53_vlan_prepare(struct dsa_switch *ds, int port,
-		     const struct switchdev_obj_port_vlan *vlan,
-		     struct switchdev_trans *trans)
+		     const struct switchdev_obj_port_vlan *vlan)
 {
 	struct b53_device *dev = ds->priv;
 
@@ -1048,8 +1047,7 @@ int b53_vlan_prepare(struct dsa_switch *ds, int port,
 EXPORT_SYMBOL(b53_vlan_prepare);
 
 void b53_vlan_add(struct dsa_switch *ds, int port,
-		  const struct switchdev_obj_port_vlan *vlan,
-		  struct switchdev_trans *trans)
+		  const struct switchdev_obj_port_vlan *vlan)
 {
 	struct b53_device *dev = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index daaaa1ecb996..88382c5dfee4 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -295,11 +295,9 @@ void b53_br_set_stp_state(struct dsa_switch *ds, int port, u8 state);
 void b53_br_fast_age(struct dsa_switch *ds, int port);
 int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering);
 int b53_vlan_prepare(struct dsa_switch *ds, int port,
-		     const struct switchdev_obj_port_vlan *vlan,
-		     struct switchdev_trans *trans);
+		     const struct switchdev_obj_port_vlan *vlan);
 void b53_vlan_add(struct dsa_switch *ds, int port,
-		  const struct switchdev_obj_port_vlan *vlan,
-		  struct switchdev_trans *trans);
+		  const struct switchdev_obj_port_vlan *vlan);
 int b53_vlan_del(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan);
 int b53_fdb_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 3a3f4f7ba364..dcb53367f433 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -173,9 +173,9 @@ static int dsa_loop_port_vlan_filtering(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-static int dsa_loop_port_vlan_prepare(struct dsa_switch *ds, int port,
-				      const struct switchdev_obj_port_vlan *vlan,
-				      struct switchdev_trans *trans)
+static int
+dsa_loop_port_vlan_prepare(struct dsa_switch *ds, int port,
+			   const struct switchdev_obj_port_vlan *vlan)
 {
 	struct dsa_loop_priv *ps = ds->priv;
 	struct mii_bus *bus = ps->bus;
@@ -192,8 +192,7 @@ static int dsa_loop_port_vlan_prepare(struct dsa_switch *ds, int port,
 }
 
 static void dsa_loop_port_vlan_add(struct dsa_switch *ds, int port,
-				   const struct switchdev_obj_port_vlan *vlan,
-				   struct switchdev_trans *trans)
+				   const struct switchdev_obj_port_vlan *vlan)
 {
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 062a32f9ed06..53f2f17611e8 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -558,8 +558,7 @@ static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port, bool flag)
 }
 
 static int ksz_port_vlan_prepare(struct dsa_switch *ds, int port,
-				 const struct switchdev_obj_port_vlan *vlan,
-				 struct switchdev_trans *trans)
+				 const struct switchdev_obj_port_vlan *vlan)
 {
 	/* nothing needed */
 
@@ -567,8 +566,7 @@ static int ksz_port_vlan_prepare(struct dsa_switch *ds, int port,
 }
 
 static void ksz_port_vlan_add(struct dsa_switch *ds, int port,
-			      const struct switchdev_obj_port_vlan *vlan,
-			      struct switchdev_trans *trans)
+			      const struct switchdev_obj_port_vlan *vlan)
 {
 	struct ksz_device *dev = ds->priv;
 	u32 vlan_table[3];
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 55026cc7e43c..b6f82d7f1841 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1185,8 +1185,7 @@ static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port,
 
 static int
 mv88e6xxx_port_vlan_prepare(struct dsa_switch *ds, int port,
-			    const struct switchdev_obj_port_vlan *vlan,
-			    struct switchdev_trans *trans)
+			    const struct switchdev_obj_port_vlan *vlan)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int err;
@@ -1224,8 +1223,7 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,
 }
 
 static void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
-				    const struct switchdev_obj_port_vlan *vlan,
-				    struct switchdev_trans *trans)
+				    const struct switchdev_obj_port_vlan *vlan)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index d48ced87a44d..62ba1edda5f6 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -410,12 +410,10 @@ struct dsa_switch_ops {
 	 */
 	int	(*port_vlan_filtering)(struct dsa_switch *ds, int port,
 				       bool vlan_filtering);
-	int	(*port_vlan_prepare)(struct dsa_switch *ds, int port,
-				     const struct switchdev_obj_port_vlan *vlan,
-				     struct switchdev_trans *trans);
-	void	(*port_vlan_add)(struct dsa_switch *ds, int port,
-				 const struct switchdev_obj_port_vlan *vlan,
-				 struct switchdev_trans *trans);
+	int (*port_vlan_prepare)(struct dsa_switch *ds, int port,
+				 const struct switchdev_obj_port_vlan *vlan);
+	void (*port_vlan_add)(struct dsa_switch *ds, int port,
+			      const struct switchdev_obj_port_vlan *vlan);
 	int	(*port_vlan_del)(struct dsa_switch *ds, int port,
 				 const struct switchdev_obj_port_vlan *vlan);
 	/*
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 8bfbbe408e82..233ce23b6abb 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -196,14 +196,14 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 			return -EOPNOTSUPP;
 
 		for_each_set_bit(port, members, ds->num_ports) {
-			err = ds->ops->port_vlan_prepare(ds, port, vlan, trans);
+			err = ds->ops->port_vlan_prepare(ds, port, vlan);
 			if (err)
 				return err;
 		}
 	}
 
 	for_each_set_bit(port, members, ds->num_ports)
-		ds->ops->port_vlan_add(ds, port, vlan, trans);
+		ds->ops->port_vlan_add(ds, port, vlan);
 
 	return 0;
 }
-- 
2.15.0

Re: [PATCH net-next 4/6] net: dsa: remove trans argument from vlan ops

[Joe Perches]

On Wed, 2017-11-08 at 12:19 -0500, Vivien Didelot wrote:
> The DSA switch VLAN ops pass the switchdev_trans structure down to the
> drivers, but no one is using them and they aren't supposed to anyway.
[]
> diff --git a/include/net/dsa.h b/include/net/dsa.h
[]
> @@ -410,12 +410,10 @@ struct dsa_switch_ops {
>  	 */
>  	int	(*port_vlan_filtering)(struct dsa_switch *ds, int port,
>  				       bool vlan_filtering);
> -	int	(*port_vlan_prepare)(struct dsa_switch *ds, int port,
> -				     const struct switchdev_obj_port_vlan *vlan,
> -				     struct switchdev_trans *trans);
> -	void	(*port_vlan_add)(struct dsa_switch *ds, int port,
> -				 const struct switchdev_obj_port_vlan *vlan,
> -				 struct switchdev_trans *trans);
> +	int (*port_vlan_prepare)(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan);
> +	void (*port_vlan_add)(struct dsa_switch *ds, int port,
> +			      const struct switchdev_obj_port_vlan *vlan);
>  	int	(*port_vlan_del)(struct dsa_switch *ds, int port,
>  				 const struct switchdev_obj_port_vlan *vlan);

I think this bit is slightly worse.
Mixing alignment styles seems odd.

I think it's better to either align all the (*func) uses
on a tabstop or
none of them.

Re: [PATCH net-next 4/6] net: dsa: remove trans argument from vlan ops

[Vivien Didelot]

Hi Joe,

Joe Perches <joe@perches.com> writes:

> On Wed, 2017-11-08 at 12:19 -0500, Vivien Didelot wrote:
>> The DSA switch VLAN ops pass the switchdev_trans structure down to the
>> drivers, but no one is using them and they aren't supposed to anyway.
> []
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
> []
>> @@ -410,12 +410,10 @@ struct dsa_switch_ops {
>>  	 */
>>  	int	(*port_vlan_filtering)(struct dsa_switch *ds, int port,
>>  				       bool vlan_filtering);
>> -	int	(*port_vlan_prepare)(struct dsa_switch *ds, int port,
>> -				     const struct switchdev_obj_port_vlan *vlan,
>> -				     struct switchdev_trans *trans);
>> -	void	(*port_vlan_add)(struct dsa_switch *ds, int port,
>> -				 const struct switchdev_obj_port_vlan *vlan,
>> -				 struct switchdev_trans *trans);
>> +	int (*port_vlan_prepare)(struct dsa_switch *ds, int port,
>> +				 const struct switchdev_obj_port_vlan *vlan);
>> +	void (*port_vlan_add)(struct dsa_switch *ds, int port,
>> +			      const struct switchdev_obj_port_vlan *vlan);
>>  	int	(*port_vlan_del)(struct dsa_switch *ds, int port,
>>  				 const struct switchdev_obj_port_vlan *vlan);
>
> I think this bit is slightly worse.
> Mixing alignment styles seems odd.
>
> I think it's better to either align all the (*func) uses
> on a tabstop or
> none of them.

I couldn't use a tab here as it is done in the other functions, because
of the 80-char limit. I will send a patch soon to remove all of this
alignment style in this header because it doesn't bring any value.


Thanks,

        Vivien

[PATCH net-next 5/6] net: dsa: return after vlan prepare phase

[Vivien Didelot]

The current code does not return after successfully preparing the VLAN
addition on every ports member of a it. Fix this.

Fixes: 1ca4aa9cd4cc ("net: dsa: check VLAN capability of every switch")
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/switch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 233ce23b6abb..0ed60456b669 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -200,6 +200,8 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 			if (err)
 				return err;
 		}
+
+		return 0;
 	}
 
 	for_each_set_bit(port, members, ds->num_ports)
-- 
2.15.0

[PATCH net-next 6/6] net: dsa: add switch vlan bitmap functions

[Vivien Didelot]

This patch brings no functional changes.
It moves out the VLAN code iterating on a list of VLAN members into new
dsa_switch_vlan_{prepare,add}_bitmap() functions.

This gives us a better isolation of the two switchdev phases.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/switch.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 0ed60456b669..fa99624f76ab 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -175,13 +175,43 @@ static int dsa_switch_mdb_del(struct dsa_switch *ds,
 	return 0;
 }
 
+static int
+dsa_switch_vlan_prepare_bitmap(struct dsa_switch *ds,
+			       const struct switchdev_obj_port_vlan *vlan,
+			       const unsigned long *bitmap)
+{
+	int port, err;
+
+	if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
+		return -EOPNOTSUPP;
+
+	for_each_set_bit(port, bitmap, ds->num_ports) {
+		err = ds->ops->port_vlan_prepare(ds, port, vlan);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static void
+dsa_switch_vlan_add_bitmap(struct dsa_switch *ds,
+			   const struct switchdev_obj_port_vlan *vlan,
+			   const unsigned long *bitmap)
+{
+	int port;
+
+	for_each_set_bit(port, bitmap, ds->num_ports)
+		ds->ops->port_vlan_add(ds, port, vlan);
+}
+
 static int dsa_switch_vlan_add(struct dsa_switch *ds,
 			       struct dsa_notifier_vlan_info *info)
 {
 	const struct switchdev_obj_port_vlan *vlan = info->vlan;
 	struct switchdev_trans *trans = info->trans;
 	DECLARE_BITMAP(members, ds->num_ports);
-	int port, err;
+	int port;
 
 	/* Build a mask of VLAN members */
 	bitmap_zero(members, ds->num_ports);
@@ -191,21 +221,10 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds,
 		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
 			set_bit(port, members);
 
-	if (switchdev_trans_ph_prepare(trans)) {
-		if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
-			return -EOPNOTSUPP;
+	if (switchdev_trans_ph_prepare(trans))
+		return dsa_switch_vlan_prepare_bitmap(ds, vlan, members);
 
-		for_each_set_bit(port, members, ds->num_ports) {
-			err = ds->ops->port_vlan_prepare(ds, port, vlan);
-			if (err)
-				return err;
-		}
-
-		return 0;
-	}
-
-	for_each_set_bit(port, members, ds->num_ports)
-		ds->ops->port_vlan_add(ds, port, vlan);
+	dsa_switch_vlan_add_bitmap(ds, vlan, members);
 
 	return 0;
 }
-- 
2.15.0

Re: [PATCH net-next 0/6] net: dsa: simplify switchdev prepare phase

[David Miller]

From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Wed,  8 Nov 2017 12:19:11 -0500

> This patch series brings no functional changes.
> 
> It removes the unused switchdev_trans arguments from the dsa_switch_ops
> for both MDB and VLAN operations, and provides function to prepare and
> add these objects for a given bitmap of ports.

This series doesn't apply to net-next and it also seems to duplicate
some fixes that went into 'net'.

Please don't do things this way.

When a fix has to go to 'net' fix, _wait_ until it propagates to
'net-next' if you must use it as a dependency.

Thank you.