[RFC PATCH net-next 0/3] net: switchdev: extract specific object structures

[RFC PATCH net-next 0/3] net: switchdev: extract specific object structures

[Vivien Didelot]

Hi!

Current implementations of .switchdev_port_obj_add and .switchdev_port_obj_dump
must pass the generic switchdev_obj structure instead of a specific one (e.g.
switchdev_obj_fdb) to the related driver accessors, because it contains the
object transaction and callback. This is not very convenient for drivers.

Instead of having all specific structures included into a switchdev_obj union,
it would be simpler to embed a switchdev_obj structure as the first member of
the more specific ones. That way, a driver can cast the switchdev_obj pointer
to the specific structure and have access to all the information from it.

As an example, this allows Rocker to change its two FDB accessors:

    rocker_port_fdb_add(rocker_port, obj->trans, &obj->u.fdb);
    rocker_port_fdb_dump(rocker_port, obj);

for these most consistent ones:

    fdb = (struct switchdev_obj_fdb *) obj;
    rocker_port_fdb_add(rocker_port, fdb);
    rocker_port_fdb_dump(rocker_port, fdb);

This is what struct netdev_notifier_info and its specific supersets (e.g.
struct netdev_notifier_changeupper_info) do in include/linux/netdevice.h.

This patchset does that and updates bridge, Rocker and DSA accordingly.

Note that this patchset was sent as an RFC not to bother David with new
net-next stuffs, but if the changes look good, it is ready to merge.

Also, please take note that the change sits on top of:
http://patchwork.ozlabs.org/patch/514894/

Vivien Didelot (3):
  net: switchdev: extract switchdev_obj_vlan
  net: switchdev: extract switchdev_obj_ipv4_fib
  net: switchdev: extract switchdev_obj_fdb

 drivers/net/ethernet/rocker/rocker.c |  50 ++++++++------
 include/net/switchdev.h              |  49 ++++++++------
 net/bridge/br_fdb.c                  |  12 ++--
 net/bridge/br_vlan.c                 |  26 +++----
 net/dsa/slave.c                      |  59 +++++++++-------
 net/switchdev/switchdev.c            | 127 ++++++++++++++++-------------------
 6 files changed, 165 insertions(+), 158 deletions(-)

-- 
2.5.1

[RFC PATCH net-next 1/3] net: switchdev: extract switchdev_obj_vlan

[Vivien Didelot]

Move the switchdev_obj_vlan structure out of the switchdev_obj union.

This lightens the switchdev_obj structure and allows drivers to access
the object transaction and callback directly from a switchdev_obj_vlan.
This is more consistent and convenient for add and dump operations.

The patch updates bridge, the Rocker driver and DSA accordingly.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/ethernet/rocker/rocker.c | 21 +++++++++--------
 include/net/switchdev.h              | 13 +++++++----
 net/bridge/br_vlan.c                 | 26 +++++++++------------
 net/dsa/slave.c                      | 27 ++++++++++++----------
 net/switchdev/switchdev.c            | 44 ++++++++++++++++++------------------
 5 files changed, 68 insertions(+), 63 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 34ac41a..e72d49a 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4394,14 +4394,13 @@ static int rocker_port_vlan_add(struct rocker_port *rocker_port,
 }
 
 static int rocker_port_vlans_add(struct rocker_port *rocker_port,
-				 enum switchdev_trans trans,
 				 const struct switchdev_obj_vlan *vlan)
 {
 	u16 vid;
 	int err;
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		err = rocker_port_vlan_add(rocker_port, trans,
+		err = rocker_port_vlan_add(rocker_port, vlan->obj.trans,
 					   vid, vlan->flags);
 		if (err)
 			return err;
@@ -4427,6 +4426,7 @@ static int rocker_port_obj_add(struct net_device *dev,
 			       struct switchdev_obj *obj)
 {
 	struct rocker_port *rocker_port = netdev_priv(dev);
+	const struct switchdev_obj_vlan *vlan;
 	const struct switchdev_obj_ipv4_fib *fib4;
 	int err = 0;
 
@@ -4443,8 +4443,8 @@ static int rocker_port_obj_add(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = rocker_port_vlans_add(rocker_port, obj->trans,
-					    &obj->u.vlan);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = rocker_port_vlans_add(rocker_port, vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
 		fib4 = &obj->u.ipv4_fib;
@@ -4509,12 +4509,14 @@ static int rocker_port_obj_del(struct net_device *dev,
 			       struct switchdev_obj *obj)
 {
 	struct rocker_port *rocker_port = netdev_priv(dev);
+	const struct switchdev_obj_vlan *vlan;
 	const struct switchdev_obj_ipv4_fib *fib4;
 	int err = 0;
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = rocker_port_vlans_del(rocker_port, &obj->u.vlan);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = rocker_port_vlans_del(rocker_port, vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
 		fib4 = &obj->u.ipv4_fib;
@@ -4563,9 +4565,8 @@ static int rocker_port_fdb_dump(const struct rocker_port *rocker_port,
 }
 
 static int rocker_port_vlan_dump(const struct rocker_port *rocker_port,
-				 struct switchdev_obj *obj)
+				 struct switchdev_obj_vlan *vlan)
 {
-	struct switchdev_obj_vlan *vlan = &obj->u.vlan;
 	u16 vid;
 	int err = 0;
 
@@ -4576,7 +4577,7 @@ static int rocker_port_vlan_dump(const struct rocker_port *rocker_port,
 		if (rocker_vlan_id_is_internal(htons(vid)))
 			vlan->flags |= BRIDGE_VLAN_INFO_PVID;
 		vlan->vid_begin = vlan->vid_end = vid;
-		err = obj->cb(rocker_port->dev, obj);
+		err = vlan->obj.cb(rocker_port->dev, &vlan->obj);
 		if (err)
 			break;
 	}
@@ -4588,6 +4589,7 @@ static int rocker_port_obj_dump(struct net_device *dev,
 				struct switchdev_obj *obj)
 {
 	const struct rocker_port *rocker_port = netdev_priv(dev);
+	struct switchdev_obj_vlan *vlan;
 	int err = 0;
 
 	switch (obj->id) {
@@ -4595,7 +4597,8 @@ static int rocker_port_obj_dump(struct net_device *dev,
 		err = rocker_port_fdb_dump(rocker_port, obj);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = rocker_port_vlan_dump(rocker_port, obj);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = rocker_port_vlan_dump(rocker_port, vlan);
 		break;
 	default:
 		err = -EOPNOTSUPP;
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 319baab..55fa106 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -55,11 +55,6 @@ struct switchdev_obj {
 	enum switchdev_trans trans;
 	int (*cb)(struct net_device *dev, struct switchdev_obj *obj);
 	union {
-		struct switchdev_obj_vlan {		/* PORT_VLAN */
-			u16 flags;
-			u16 vid_begin;
-			u16 vid_end;
-		} vlan;
 		struct switchdev_obj_ipv4_fib {		/* IPV4_FIB */
 			u32 dst;
 			int dst_len;
@@ -77,6 +72,14 @@ struct switchdev_obj {
 	} u;
 };
 
+/* SWITCHDEV_OBJ_PORT_VLAN */
+struct switchdev_obj_vlan {
+	struct switchdev_obj obj;	/* must be first */
+	u16 flags;
+	u16 vid_begin;
+	u16 vid_end;
+};
+
 /**
  * struct switchdev_ops - switchdev operations
  *
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 5f5a02b..44e8bc1 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -50,16 +50,14 @@ static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 	if (ops->ndo_vlan_rx_add_vid) {
 		err = vlan_vid_add(dev, br->vlan_proto, vid);
 	} else {
-		struct switchdev_obj vlan_obj = {
-			.id = SWITCHDEV_OBJ_PORT_VLAN,
-			.u.vlan = {
-				.flags = flags,
-				.vid_begin = vid,
-				.vid_end = vid,
-			},
+		struct switchdev_obj_vlan vlan = {
+			.obj.id = SWITCHDEV_OBJ_PORT_VLAN,
+			.flags = flags,
+			.vid_begin = vid,
+			.vid_end = vid,
 		};
 
-		err = switchdev_port_obj_add(dev, &vlan_obj);
+		err = switchdev_port_obj_add(dev, &vlan.obj);
 		if (err == -EOPNOTSUPP)
 			err = 0;
 	}
@@ -130,15 +128,13 @@ static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
 	if (ops->ndo_vlan_rx_kill_vid) {
 		vlan_vid_del(dev, br->vlan_proto, vid);
 	} else {
-		struct switchdev_obj vlan_obj = {
-			.id = SWITCHDEV_OBJ_PORT_VLAN,
-			.u.vlan = {
-				.vid_begin = vid,
-				.vid_end = vid,
-			},
+		struct switchdev_obj_vlan vlan = {
+			.obj.id = SWITCHDEV_OBJ_PORT_VLAN,
+			.vid_begin = vid,
+			.vid_end = vid,
 		};
 
-		err = switchdev_port_obj_del(dev, &vlan_obj);
+		err = switchdev_port_obj_del(dev, &vlan.obj);
 		if (err == -EOPNOTSUPP)
 			err = 0;
 	}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index cce9738..654afc8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -242,15 +242,14 @@ static int dsa_bridge_check_vlan_range(struct dsa_switch *ds,
 }
 
 static int dsa_slave_port_vlan_add(struct net_device *dev,
-				   struct switchdev_obj *obj)
+				   struct switchdev_obj_vlan *vlan)
 {
-	struct switchdev_obj_vlan *vlan = &obj->u.vlan;
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 	u16 vid;
 	int err;
 
-	switch (obj->trans) {
+	switch (vlan->obj.trans) {
 	case SWITCHDEV_TRANS_PREPARE:
 		if (!ds->drv->port_vlan_add || !ds->drv->port_pvid_set)
 			return -EOPNOTSUPP;
@@ -283,9 +282,8 @@ static int dsa_slave_port_vlan_add(struct net_device *dev,
 }
 
 static int dsa_slave_port_vlan_del(struct net_device *dev,
-				   struct switchdev_obj *obj)
+				   struct switchdev_obj_vlan *vlan)
 {
-	struct switchdev_obj_vlan *vlan = &obj->u.vlan;
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 	u16 vid;
@@ -304,9 +302,8 @@ static int dsa_slave_port_vlan_del(struct net_device *dev,
 }
 
 static int dsa_slave_port_vlan_dump(struct net_device *dev,
-				    struct switchdev_obj *obj)
+				    struct switchdev_obj_vlan *vlan)
 {
-	struct switchdev_obj_vlan *vlan = &obj->u.vlan;
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 	DECLARE_BITMAP(members, DSA_MAX_PORTS);
@@ -329,7 +326,7 @@ static int dsa_slave_port_vlan_dump(struct net_device *dev,
 		if (!test_bit(p->port, members))
 			continue;
 
-		memset(vlan, 0, sizeof(*vlan));
+		vlan->flags = 0;
 		vlan->vid_begin = vlan->vid_end = vid;
 
 		if (vid == pvid)
@@ -338,7 +335,7 @@ static int dsa_slave_port_vlan_dump(struct net_device *dev,
 		if (test_bit(p->port, untagged))
 			vlan->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 
-		err = obj->cb(dev, obj);
+		err = vlan->obj.cb(dev, &vlan->obj);
 		if (err)
 			break;
 	}
@@ -476,6 +473,7 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 static int dsa_slave_port_obj_add(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
+	struct switchdev_obj_vlan *vlan;
 	int err;
 
 	/* For the prepare phase, ensure the full set of changes is feasable in
@@ -488,7 +486,8 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 		err = dsa_slave_port_fdb_add(dev, obj);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = dsa_slave_port_vlan_add(dev, obj);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = dsa_slave_port_vlan_add(dev, vlan);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -501,6 +500,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 static int dsa_slave_port_obj_del(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
+	struct switchdev_obj_vlan *vlan;
 	int err;
 
 	switch (obj->id) {
@@ -508,7 +508,8 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 		err = dsa_slave_port_fdb_del(dev, obj);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = dsa_slave_port_vlan_del(dev, obj);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = dsa_slave_port_vlan_del(dev, vlan);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -521,6 +522,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 static int dsa_slave_port_obj_dump(struct net_device *dev,
 				   struct switchdev_obj *obj)
 {
+	struct switchdev_obj_vlan *vlan;
 	int err;
 
 	switch (obj->id) {
@@ -528,7 +530,8 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
 		err = dsa_slave_port_fdb_dump(dev, obj);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
-		err = dsa_slave_port_vlan_dump(dev, obj);
+		vlan = (struct switchdev_obj_vlan *) obj;
+		err = dsa_slave_port_vlan_dump(dev, vlan);
 		break;
 	default:
 		err = -EOPNOTSUPP;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 16c1c43..9923a97 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -397,7 +397,7 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
 EXPORT_SYMBOL_GPL(call_switchdev_notifiers);
 
 struct switchdev_vlan_dump {
-	struct switchdev_obj obj;
+	struct switchdev_obj_vlan vlan;	/* must be first */
 	struct sk_buff *skb;
 	u32 filter_mask;
 	u16 flags;
@@ -439,9 +439,8 @@ static int switchdev_port_vlan_dump_put(struct net_device *dev,
 static int switchdev_port_vlan_dump_cb(struct net_device *dev,
 				       struct switchdev_obj *obj)
 {
-	struct switchdev_vlan_dump *dump =
-		container_of(obj, struct switchdev_vlan_dump, obj);
-	struct switchdev_obj_vlan *vlan = &dump->obj.u.vlan;
+	struct switchdev_vlan_dump *dump = (struct switchdev_vlan_dump *) obj;
+	struct switchdev_obj_vlan *vlan = (struct switchdev_obj_vlan *) obj;
 	int err = 0;
 
 	if (vlan->vid_begin > vlan->vid_end)
@@ -493,7 +492,7 @@ static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
 				    u32 filter_mask)
 {
 	struct switchdev_vlan_dump dump = {
-		.obj = {
+		.vlan.obj = {
 			.id = SWITCHDEV_OBJ_PORT_VLAN,
 			.cb = switchdev_port_vlan_dump_cb,
 		},
@@ -504,7 +503,7 @@ static int switchdev_port_vlan_fill(struct sk_buff *skb, struct net_device *dev,
 
 	if ((filter_mask & RTEXT_FILTER_BRVLAN) ||
 	    (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)) {
-		err = switchdev_port_obj_dump(dev, &dump.obj);
+		err = switchdev_port_obj_dump(dev, &dump.vlan.obj);
 		if (err)
 			goto err_out;
 		if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
@@ -621,10 +620,9 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 {
 	struct nlattr *attr;
 	struct bridge_vlan_info *vinfo;
-	struct switchdev_obj obj = {
-		.id = SWITCHDEV_OBJ_PORT_VLAN,
+	struct switchdev_obj_vlan vlan = {
+		.obj.id = SWITCHDEV_OBJ_PORT_VLAN,
 	};
-	struct switchdev_obj_vlan *vlan = &obj.u.vlan;
 	int rem;
 	int err;
 
@@ -634,30 +632,32 @@ static int switchdev_port_br_afspec(struct net_device *dev,
 		if (nla_len(attr) != sizeof(struct bridge_vlan_info))
 			return -EINVAL;
 		vinfo = nla_data(attr);
-		vlan->flags = vinfo->flags;
+		vlan.flags = vinfo->flags;
 		if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
-			if (vlan->vid_begin)
+			if (vlan.vid_begin)
 				return -EINVAL;
-			vlan->vid_begin = vinfo->vid;
+			vlan.vid_begin = vinfo->vid;
 		} else if (vinfo->flags & BRIDGE_VLAN_INFO_RANGE_END) {
-			if (!vlan->vid_begin)
+			if (!vlan.vid_begin)
 				return -EINVAL;
-			vlan->vid_end = vinfo->vid;
-			if (vlan->vid_end <= vlan->vid_begin)
+			vlan.vid_end = vinfo->vid;
+			if (vlan.vid_end <= vlan.vid_begin)
 				return -EINVAL;
-			err = f(dev, &obj);
+			err = f(dev, &vlan.obj);
 			if (err)
 				return err;
-			memset(vlan, 0, sizeof(*vlan));
+			vlan.flags = 0;
+			vlan.vid_begin = vlan.vid_end = 0;
 		} else {
-			if (vlan->vid_begin)
+			if (vlan.vid_begin)
 				return -EINVAL;
-			vlan->vid_begin = vinfo->vid;
-			vlan->vid_end = vinfo->vid;
-			err = f(dev, &obj);
+			vlan.vid_begin = vinfo->vid;
+			vlan.vid_end = vinfo->vid;
+			err = f(dev, &vlan.obj);
 			if (err)
 				return err;
-			memset(vlan, 0, sizeof(*vlan));
+			vlan.flags = 0;
+			vlan.vid_begin = vlan.vid_end = 0;
 		}
 	}
 
-- 
2.5.1

Re: [RFC PATCH net-next 1/3] net: switchdev: extract switchdev_obj_vlan

[Andrew Lunn]

> +/* SWITCHDEV_OBJ_PORT_VLAN */
> +struct switchdev_obj_vlan {
> +	struct switchdev_obj obj;	/* must be first */
> +	u16 flags;
> +	u16 vid_begin;
> +	u16 vid_end;
> +};

I know you give a few examples for where this is done in network code,
but i think container_of() is used much more. You can then place the
struct switchdev_obj anywhere in the structure, and it will work.

#define to_switchdev_obj_vlan(o) container_of(o, struct switchdev_obj_vlan, obj)

	Andrew

Re: [RFC PATCH net-next 1/3] net: switchdev: extract switchdev_obj_vlan

[Scott Feldman]

On Tue, Sep 8, 2015 at 1:47 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Move the switchdev_obj_vlan structure out of the switchdev_obj union.
>
> This lightens the switchdev_obj structure and allows drivers to access
> the object transaction and callback directly from a switchdev_obj_vlan.
> This is more consistent and convenient for add and dump operations.
>
> The patch updates bridge, the Rocker driver and DSA accordingly.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---

[cut]

> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 16c1c43..9923a97 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -397,7 +397,7 @@ int call_switchdev_notifiers(unsigned long val, struct net_device *dev,
>  EXPORT_SYMBOL_GPL(call_switchdev_notifiers);
>
>  struct switchdev_vlan_dump {
> -       struct switchdev_obj obj;
> +       struct switchdev_obj_vlan vlan; /* must be first */
>         struct sk_buff *skb;
>         u32 filter_mask;
>         u16 flags;
> @@ -439,9 +439,8 @@ static int switchdev_port_vlan_dump_put(struct net_device *dev,
>  static int switchdev_port_vlan_dump_cb(struct net_device *dev,
>                                        struct switchdev_obj *obj)
>  {
> -       struct switchdev_vlan_dump *dump =
> -               container_of(obj, struct switchdev_vlan_dump, obj);
> -       struct switchdev_obj_vlan *vlan = &dump->obj.u.vlan;
> +       struct switchdev_vlan_dump *dump = (struct switchdev_vlan_dump *) obj;
> +       struct switchdev_obj_vlan *vlan = (struct switchdev_obj_vlan *) obj;

Same comment as Andrew's: use container_of() rather than casting for
both of the use-cases above.  (And don't require inner-structure to be
fist in outer structure).

[RFC PATCH net-next 2/3] net: switchdev: extract switchdev_obj_ipv4_fib

[Vivien Didelot]

Move the switchdev_obj_ipv4_fib structure out of the switchdev_obj
union.

This lightens the switchdev_obj structure and allows drivers to access
the object transaction and callback directly from a
switchdev_obj_ipv4_fib. This is more consistent and convenient for add
and dump operations.

The patch updates the Rocker driver accordingly.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/ethernet/rocker/rocker.c |  4 ++--
 include/net/switchdev.h              | 21 +++++++++--------
 net/switchdev/switchdev.c            | 44 ++++++++++++++++--------------------
 3 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index e72d49a..41aabbc 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4447,7 +4447,7 @@ static int rocker_port_obj_add(struct net_device *dev,
 		err = rocker_port_vlans_add(rocker_port, vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
-		fib4 = &obj->u.ipv4_fib;
+		fib4 = (struct switchdev_obj_ipv4_fib *) obj;
 		err = rocker_port_fib_ipv4(rocker_port, obj->trans,
 					   htonl(fib4->dst), fib4->dst_len,
 					   fib4->fi, fib4->tb_id, 0);
@@ -4519,7 +4519,7 @@ static int rocker_port_obj_del(struct net_device *dev,
 		err = rocker_port_vlans_del(rocker_port, vlan);
 		break;
 	case SWITCHDEV_OBJ_IPV4_FIB:
-		fib4 = &obj->u.ipv4_fib;
+		fib4 = (struct switchdev_obj_ipv4_fib *) obj;
 		err = rocker_port_fib_ipv4(rocker_port, SWITCHDEV_TRANS_NONE,
 					   htonl(fib4->dst), fib4->dst_len,
 					   fib4->fi, fib4->tb_id,
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 55fa106..0b76aa8 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -55,15 +55,6 @@ struct switchdev_obj {
 	enum switchdev_trans trans;
 	int (*cb)(struct net_device *dev, struct switchdev_obj *obj);
 	union {
-		struct switchdev_obj_ipv4_fib {		/* IPV4_FIB */
-			u32 dst;
-			int dst_len;
-			struct fib_info *fi;
-			u8 tos;
-			u8 type;
-			u32 nlflags;
-			u32 tb_id;
-		} ipv4_fib;
 		struct switchdev_obj_fdb {		/* PORT_FDB */
 			const unsigned char *addr;
 			u16 vid;
@@ -80,6 +71,18 @@ struct switchdev_obj_vlan {
 	u16 vid_end;
 };
 
+/* SWITCHDEV_OBJ_IPV4_FIB */
+struct switchdev_obj_ipv4_fib {
+	struct switchdev_obj obj;	/* must be first */
+	u32 dst;
+	int dst_len;
+	struct fib_info *fi;
+	u8 tos;
+	u8 type;
+	u32 nlflags;
+	u32 tb_id;
+};
+
 /**
  * struct switchdev_ops - switchdev operations
  *
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 9923a97..10fde6f 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -936,17 +936,15 @@ static struct net_device *switchdev_get_dev_by_nhs(struct fib_info *fi)
 int switchdev_fib_ipv4_add(u32 dst, int dst_len, struct fib_info *fi,
 			   u8 tos, u8 type, u32 nlflags, u32 tb_id)
 {
-	struct switchdev_obj fib_obj = {
-		.id = SWITCHDEV_OBJ_IPV4_FIB,
-		.u.ipv4_fib = {
-			.dst = dst,
-			.dst_len = dst_len,
-			.fi = fi,
-			.tos = tos,
-			.type = type,
-			.nlflags = nlflags,
-			.tb_id = tb_id,
-		},
+	struct switchdev_obj_ipv4_fib fib_obj = {
+		.obj.id = SWITCHDEV_OBJ_IPV4_FIB,
+		.dst = dst,
+		.dst_len = dst_len,
+		.fi = fi,
+		.tos = tos,
+		.type = type,
+		.nlflags = nlflags,
+		.tb_id = tb_id,
 	};
 	struct net_device *dev;
 	int err = 0;
@@ -967,7 +965,7 @@ int switchdev_fib_ipv4_add(u32 dst, int dst_len, struct fib_info *fi,
 	if (!dev)
 		return 0;
 
-	err = switchdev_port_obj_add(dev, &fib_obj);
+	err = switchdev_port_obj_add(dev, &fib_obj.obj);
 	if (!err)
 		fi->fib_flags |= RTNH_F_OFFLOAD;
 
@@ -990,17 +988,15 @@ EXPORT_SYMBOL_GPL(switchdev_fib_ipv4_add);
 int switchdev_fib_ipv4_del(u32 dst, int dst_len, struct fib_info *fi,
 			   u8 tos, u8 type, u32 tb_id)
 {
-	struct switchdev_obj fib_obj = {
-		.id = SWITCHDEV_OBJ_IPV4_FIB,
-		.u.ipv4_fib = {
-			.dst = dst,
-			.dst_len = dst_len,
-			.fi = fi,
-			.tos = tos,
-			.type = type,
-			.nlflags = 0,
-			.tb_id = tb_id,
-		},
+	struct switchdev_obj_ipv4_fib fib_obj = {
+		.obj.id = SWITCHDEV_OBJ_IPV4_FIB,
+		.dst = dst,
+		.dst_len = dst_len,
+		.fi = fi,
+		.tos = tos,
+		.type = type,
+		.nlflags = 0,
+		.tb_id = tb_id,
 	};
 	struct net_device *dev;
 	int err = 0;
@@ -1012,7 +1008,7 @@ int switchdev_fib_ipv4_del(u32 dst, int dst_len, struct fib_info *fi,
 	if (!dev)
 		return 0;
 
-	err = switchdev_port_obj_del(dev, &fib_obj);
+	err = switchdev_port_obj_del(dev, &fib_obj.obj);
 	if (!err)
 		fi->fib_flags &= ~RTNH_F_OFFLOAD;
 
-- 
2.5.1

[RFC PATCH net-next 3/3] net: switchdev: extract switchdev_obj_fdb

[Vivien Didelot]

Move the switchdev_obj_fdb structure out of the switchdev_obj union.

This lightens the switchdev_obj structure and allows drivers to access
the object transaction and callback directly from a switchdev_obj_fdb.
This is more consistent and convenient for add and dump operations.

The patch updates bridge, the Rocker driver and DSA accordingly.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/ethernet/rocker/rocker.c | 25 ++++++++++++++---------
 include/net/switchdev.h              | 15 +++++++-------
 net/bridge/br_fdb.c                  | 12 +++++------
 net/dsa/slave.c                      | 32 ++++++++++++++++-------------
 net/switchdev/switchdev.c            | 39 ++++++++++++++++--------------------
 5 files changed, 63 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 41aabbc..c3f25a9 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4410,7 +4410,6 @@ static int rocker_port_vlans_add(struct rocker_port *rocker_port,
 }
 
 static int rocker_port_fdb_add(struct rocker_port *rocker_port,
-			       enum switchdev_trans trans,
 			       const struct switchdev_obj_fdb *fdb)
 {
 	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, fdb->vid, NULL);
@@ -4419,7 +4418,8 @@ static int rocker_port_fdb_add(struct rocker_port *rocker_port,
 	if (!rocker_port_is_bridged(rocker_port))
 		return -EINVAL;
 
-	return rocker_port_fdb(rocker_port, trans, fdb->addr, vlan_id, flags);
+	return rocker_port_fdb(rocker_port, fdb->obj.trans, fdb->addr, vlan_id,
+			       flags);
 }
 
 static int rocker_port_obj_add(struct net_device *dev,
@@ -4428,6 +4428,7 @@ static int rocker_port_obj_add(struct net_device *dev,
 	struct rocker_port *rocker_port = netdev_priv(dev);
 	const struct switchdev_obj_vlan *vlan;
 	const struct switchdev_obj_ipv4_fib *fib4;
+	const struct switchdev_obj_fdb *fdb;
 	int err = 0;
 
 	switch (obj->trans) {
@@ -4453,7 +4454,8 @@ static int rocker_port_obj_add(struct net_device *dev,
 					   fib4->fi, fib4->tb_id, 0);
 		break;
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = rocker_port_fdb_add(rocker_port, obj->trans, &obj->u.fdb);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = rocker_port_fdb_add(rocker_port, fdb);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -4493,7 +4495,6 @@ static int rocker_port_vlans_del(struct rocker_port *rocker_port,
 }
 
 static int rocker_port_fdb_del(struct rocker_port *rocker_port,
-			       enum switchdev_trans trans,
 			       const struct switchdev_obj_fdb *fdb)
 {
 	__be16 vlan_id = rocker_port_vid_to_vlan(rocker_port, fdb->vid, NULL);
@@ -4502,7 +4503,8 @@ static int rocker_port_fdb_del(struct rocker_port *rocker_port,
 	if (!rocker_port_is_bridged(rocker_port))
 		return -EINVAL;
 
-	return rocker_port_fdb(rocker_port, trans, fdb->addr, vlan_id, flags);
+	return rocker_port_fdb(rocker_port, fdb->obj.trans, fdb->addr, vlan_id,
+			       flags);
 }
 
 static int rocker_port_obj_del(struct net_device *dev,
@@ -4511,6 +4513,7 @@ static int rocker_port_obj_del(struct net_device *dev,
 	struct rocker_port *rocker_port = netdev_priv(dev);
 	const struct switchdev_obj_vlan *vlan;
 	const struct switchdev_obj_ipv4_fib *fib4;
+	const struct switchdev_obj_fdb *fdb;
 	int err = 0;
 
 	switch (obj->id) {
@@ -4526,7 +4529,8 @@ static int rocker_port_obj_del(struct net_device *dev,
 					   ROCKER_OP_FLAG_REMOVE);
 		break;
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = rocker_port_fdb_del(rocker_port, obj->trans, &obj->u.fdb);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = rocker_port_fdb_del(rocker_port, fdb);
 		break;
 	default:
 		err = -EOPNOTSUPP;
@@ -4537,10 +4541,9 @@ static int rocker_port_obj_del(struct net_device *dev,
 }
 
 static int rocker_port_fdb_dump(const struct rocker_port *rocker_port,
-				struct switchdev_obj *obj)
+				struct switchdev_obj_fdb *fdb)
 {
 	struct rocker *rocker = rocker_port->rocker;
-	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
 	struct rocker_fdb_tbl_entry *found;
 	struct hlist_node *tmp;
 	unsigned long lock_flags;
@@ -4555,7 +4558,7 @@ static int rocker_port_fdb_dump(const struct rocker_port *rocker_port,
 		fdb->ndm_state = NUD_REACHABLE;
 		fdb->vid = rocker_port_vlan_to_vid(rocker_port,
 						   found->key.vlan_id);
-		err = obj->cb(rocker_port->dev, obj);
+		err = fdb->obj.cb(rocker_port->dev, &fdb->obj);
 		if (err)
 			break;
 	}
@@ -4590,11 +4593,13 @@ static int rocker_port_obj_dump(struct net_device *dev,
 {
 	const struct rocker_port *rocker_port = netdev_priv(dev);
 	struct switchdev_obj_vlan *vlan;
+	struct switchdev_obj_fdb *fdb;
 	int err = 0;
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = rocker_port_fdb_dump(rocker_port, obj);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = rocker_port_fdb_dump(rocker_port, fdb);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
 		vlan = (struct switchdev_obj_vlan *) obj;
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 0b76aa8..302f027 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -54,13 +54,6 @@ struct switchdev_obj {
 	enum switchdev_obj_id id;
 	enum switchdev_trans trans;
 	int (*cb)(struct net_device *dev, struct switchdev_obj *obj);
-	union {
-		struct switchdev_obj_fdb {		/* PORT_FDB */
-			const unsigned char *addr;
-			u16 vid;
-			u16 ndm_state;
-		} fdb;
-	} u;
 };
 
 /* SWITCHDEV_OBJ_PORT_VLAN */
@@ -83,6 +76,14 @@ struct switchdev_obj_ipv4_fib {
 	u32 tb_id;
 };
 
+/* SWITCHDEV_OBJ_PORT_FDB */
+struct switchdev_obj_fdb {
+	struct switchdev_obj obj;	/* must be first */
+	const unsigned char *addr;
+	u16 vid;
+	u16 ndm_state;
+};
+
 /**
  * struct switchdev_ops - switchdev operations
  *
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 9e9875d..d20d9b9 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -133,15 +133,13 @@ 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 obj = {
-		.id = SWITCHDEV_OBJ_PORT_FDB,
-		.u.fdb = {
-			.addr = f->addr.addr,
-			.vid = f->vlan_id,
-		},
+	struct switchdev_obj_fdb fdb = {
+		.obj.id = SWITCHDEV_OBJ_PORT_FDB,
+		.addr = f->addr.addr,
+		.vid = f->vlan_id,
 	};
 
-	switchdev_port_obj_del(f->dst->dev, &obj);
+	switchdev_port_obj_del(f->dst->dev, &fdb.obj);
 }
 
 static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 654afc8..d1de9df 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -344,25 +344,23 @@ static int dsa_slave_port_vlan_dump(struct net_device *dev,
 }
 
 static int dsa_slave_port_fdb_add(struct net_device *dev,
-				  struct switchdev_obj *obj)
+				  struct switchdev_obj_fdb *fdb)
 {
-	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 	int ret = -EOPNOTSUPP;
 
-	if (obj->trans == SWITCHDEV_TRANS_PREPARE)
+	if (fdb->obj.trans == SWITCHDEV_TRANS_PREPARE)
 		ret = ds->drv->port_fdb_add ? 0 : -EOPNOTSUPP;
-	else if (obj->trans == SWITCHDEV_TRANS_COMMIT)
+	else if (fdb->obj.trans == SWITCHDEV_TRANS_COMMIT)
 		ret = ds->drv->port_fdb_add(ds, p->port, fdb->addr, fdb->vid);
 
 	return ret;
 }
 
 static int dsa_slave_port_fdb_del(struct net_device *dev,
-				  struct switchdev_obj *obj)
+				  struct switchdev_obj_fdb *fdb)
 {
-	struct switchdev_obj_fdb *fdb = &obj->u.fdb;
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 	int ret = -EOPNOTSUPP;
@@ -374,7 +372,7 @@ static int dsa_slave_port_fdb_del(struct net_device *dev,
 }
 
 static int dsa_slave_port_fdb_dump(struct net_device *dev,
-				   struct switchdev_obj *obj)
+				   struct switchdev_obj_fdb *fdb)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
@@ -393,11 +391,11 @@ static int dsa_slave_port_fdb_dump(struct net_device *dev,
 		if (ret < 0)
 			break;
 
-		obj->u.fdb.addr = addr;
-		obj->u.fdb.vid = vid;
-		obj->u.fdb.ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
+		fdb->addr = addr;
+		fdb->vid = vid;
+		fdb->ndm_state = is_static ? NUD_NOARP : NUD_REACHABLE;
 
-		ret = obj->cb(dev, obj);
+		ret = fdb->obj.cb(dev, &fdb->obj);
 		if (ret < 0)
 			break;
 	}
@@ -474,6 +472,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
 	struct switchdev_obj_vlan *vlan;
+	struct switchdev_obj_fdb *fdb;
 	int err;
 
 	/* For the prepare phase, ensure the full set of changes is feasable in
@@ -483,7 +482,8 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = dsa_slave_port_fdb_add(dev, obj);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = dsa_slave_port_fdb_add(dev, fdb);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
 		vlan = (struct switchdev_obj_vlan *) obj;
@@ -501,11 +501,13 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
 	struct switchdev_obj_vlan *vlan;
+	struct switchdev_obj_fdb *fdb;
 	int err;
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = dsa_slave_port_fdb_del(dev, obj);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = dsa_slave_port_fdb_del(dev, fdb);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
 		vlan = (struct switchdev_obj_vlan *) obj;
@@ -523,11 +525,13 @@ static int dsa_slave_port_obj_dump(struct net_device *dev,
 				   struct switchdev_obj *obj)
 {
 	struct switchdev_obj_vlan *vlan;
+	struct switchdev_obj_fdb *fdb;
 	int err;
 
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_PORT_FDB:
-		err = dsa_slave_port_fdb_dump(dev, obj);
+		fdb = (struct switchdev_obj_fdb *) obj;
+		err = dsa_slave_port_fdb_dump(dev, fdb);
 		break;
 	case SWITCHDEV_OBJ_PORT_VLAN:
 		vlan = (struct switchdev_obj_vlan *) obj;
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 10fde6f..5349752 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -739,15 +739,13 @@ 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 obj = {
-		.id = SWITCHDEV_OBJ_PORT_FDB,
-		.u.fdb = {
-			.addr = addr,
-			.vid = vid,
-		},
+	struct switchdev_obj_fdb fdb = {
+		.obj.id = SWITCHDEV_OBJ_PORT_FDB,
+		.addr = addr,
+		.vid = vid,
 	};
 
-	return switchdev_port_obj_add(dev, &obj);
+	return switchdev_port_obj_add(dev, &fdb.obj);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fdb_add);
 
@@ -766,20 +764,18 @@ int switchdev_port_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 			   struct net_device *dev, const unsigned char *addr,
 			   u16 vid)
 {
-	struct switchdev_obj obj = {
-		.id = SWITCHDEV_OBJ_PORT_FDB,
-		.u.fdb = {
-			.addr = addr,
-			.vid = vid,
-		},
+	struct switchdev_obj_fdb fdb = {
+		.obj.id = SWITCHDEV_OBJ_PORT_FDB,
+		.addr = addr,
+		.vid = vid,
 	};
 
-	return switchdev_port_obj_del(dev, &obj);
+	return switchdev_port_obj_del(dev, &fdb.obj);
 }
 EXPORT_SYMBOL_GPL(switchdev_port_fdb_del);
 
 struct switchdev_fdb_dump {
-	struct switchdev_obj obj;
+	struct switchdev_obj_fdb fdb;	/* must be first */
 	struct sk_buff *skb;
 	struct netlink_callback *cb;
 	int idx;
@@ -788,8 +784,7 @@ struct switchdev_fdb_dump {
 static int switchdev_port_fdb_dump_cb(struct net_device *dev,
 				      struct switchdev_obj *obj)
 {
-	struct switchdev_fdb_dump *dump =
-		container_of(obj, struct switchdev_fdb_dump, obj);
+	struct switchdev_fdb_dump *dump = (struct switchdev_fdb_dump *) obj;
 	u32 portid = NETLINK_CB(dump->cb->skb).portid;
 	u32 seq = dump->cb->nlh->nlmsg_seq;
 	struct nlmsghdr *nlh;
@@ -810,12 +805,12 @@ static int switchdev_port_fdb_dump_cb(struct net_device *dev,
 	ndm->ndm_flags   = NTF_SELF;
 	ndm->ndm_type    = 0;
 	ndm->ndm_ifindex = dev->ifindex;
-	ndm->ndm_state   = obj->u.fdb.ndm_state;
+	ndm->ndm_state   = dump->fdb.ndm_state;
 
-	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, obj->u.fdb.addr))
+	if (nla_put(dump->skb, NDA_LLADDR, ETH_ALEN, dump->fdb.addr))
 		goto nla_put_failure;
 
-	if (obj->u.fdb.vid && nla_put_u16(dump->skb, NDA_VLAN, obj->u.fdb.vid))
+	if (dump->fdb.vid && nla_put_u16(dump->skb, NDA_VLAN, dump->fdb.vid))
 		goto nla_put_failure;
 
 	nlmsg_end(dump->skb, nlh);
@@ -845,7 +840,7 @@ int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 			    struct net_device *filter_dev, int idx)
 {
 	struct switchdev_fdb_dump dump = {
-		.obj = {
+		.fdb.obj = {
 			.id = SWITCHDEV_OBJ_PORT_FDB,
 			.cb = switchdev_port_fdb_dump_cb,
 		},
@@ -855,7 +850,7 @@ int switchdev_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
 	};
 	int err;
 
-	err = switchdev_port_obj_dump(dev, &dump.obj);
+	err = switchdev_port_obj_dump(dev, &dump.fdb.obj);
 	if (err)
 		return err;
 
-- 
2.5.1

Re: [RFC PATCH net-next 0/3] net: switchdev: extract specific object structures

[Jiri Pirko]

Tue, Sep 08, 2015 at 10:47:52PM CEST, vivien.didelot@savoirfairelinux.com wrote:
>Hi!
>
>Current implementations of .switchdev_port_obj_add and .switchdev_port_obj_dump
>must pass the generic switchdev_obj structure instead of a specific one (e.g.
>switchdev_obj_fdb) to the related driver accessors, because it contains the
>object transaction and callback. This is not very convenient for drivers.
>
>Instead of having all specific structures included into a switchdev_obj union,
>it would be simpler to embed a switchdev_obj structure as the first member of
>the more specific ones. That way, a driver can cast the switchdev_obj pointer
>to the specific structure and have access to all the information from it.
>
>As an example, this allows Rocker to change its two FDB accessors:
>
>    rocker_port_fdb_add(rocker_port, obj->trans, &obj->u.fdb);
>    rocker_port_fdb_dump(rocker_port, obj);
>
>for these most consistent ones:
>
>    fdb = (struct switchdev_obj_fdb *) obj;
>    rocker_port_fdb_add(rocker_port, fdb);
>    rocker_port_fdb_dump(rocker_port, fdb);
>
>This is what struct netdev_notifier_info and its specific supersets (e.g.
>struct netdev_notifier_changeupper_info) do in include/linux/netdevice.h.
>
>This patchset does that and updates bridge, Rocker and DSA accordingly.
>
>Note that this patchset was sent as an RFC not to bother David with new
>net-next stuffs, but if the changes look good, it is ready to merge.
>
>Also, please take note that the change sits on top of:
>http://patchwork.ozlabs.org/patch/514894/
>
>Vivien Didelot (3):
>  net: switchdev: extract switchdev_obj_vlan
>  net: switchdev: extract switchdev_obj_ipv4_fib
>  net: switchdev: extract switchdev_obj_fdb

I like this patchset a lot! I think it is the right way. Thanks for doing this.

Re: [RFC PATCH net-next 0/3] net: switchdev: extract specific object structures

[Scott Feldman]

On Tue, Sep 8, 2015 at 1:47 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Hi!
>
> Current implementations of .switchdev_port_obj_add and .switchdev_port_obj_dump
> must pass the generic switchdev_obj structure instead of a specific one (e.g.
> switchdev_obj_fdb) to the related driver accessors, because it contains the
> object transaction and callback. This is not very convenient for drivers.
>
> Instead of having all specific structures included into a switchdev_obj union,
> it would be simpler to embed a switchdev_obj structure as the first member of
> the more specific ones. That way, a driver can cast the switchdev_obj pointer
> to the specific structure and have access to all the information from it.
>
> As an example, this allows Rocker to change its two FDB accessors:
>
>     rocker_port_fdb_add(rocker_port, obj->trans, &obj->u.fdb);
>     rocker_port_fdb_dump(rocker_port, obj);
>
> for these most consistent ones:
>
>     fdb = (struct switchdev_obj_fdb *) obj;
>     rocker_port_fdb_add(rocker_port, fdb);
>     rocker_port_fdb_dump(rocker_port, fdb);
>
> This is what struct netdev_notifier_info and its specific supersets (e.g.
> struct netdev_notifier_changeupper_info) do in include/linux/netdevice.h.
>
> This patchset does that and updates bridge, Rocker and DSA accordingly.
>
> Note that this patchset was sent as an RFC not to bother David with new
> net-next stuffs, but if the changes look good, it is ready to merge.
>
> Also, please take note that the change sits on top of:
> http://patchwork.ozlabs.org/patch/514894/
>

HI Vivien, looks good. Some requests for the next version:

1) Move the cb() member out of struct switchdev_obj and into the
containing struct switchdev_obj_xxx structure.  This way the second
arg to cb() can be struct switchdev_obj_xxx *.  (And, I have pending
patches where the first arg to cb() is different, so making the dump
cb() func specific to obj type will help).

2) As already mentioned, use container_of where possible to avoid
locking struct member at a position within struct.

Should the same treatment be done for switchdev_attr?  Maybe that's a
second patchset.

-scott