Re: [PATCH net-next v3 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Vivien Didelot]

Hi Petr,

Petr Machata  writes:

> A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
> initializing a struct switchdev_obj_port_vlan, a piece of code that
> repeats on each call site almost verbatim. While in the current codebase
> there is just one duplicated add call, the follow-up patches add more of
> both add and del calls.
>
> Thus to remove the duplication, extract the repetition into named
> functions and reuse.
>
> Signed-off-by: Petr Machata 

Considering Dan's comment as well:

Reviewed-by: Vivien Didelot 

Thanks,

Vivien
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH net-next v3 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Dan Carpenter]

On Mon, May 28, 2018 at 05:10:28PM +0200, Petr Machata wrote:
>   /* Try switchdev op first. In case it is not supported, fallback to
>* 8021q add.
>*/
> - err = switchdev_port_obj_add(dev, &v.obj);
> + int err = br_switchdev_port_vlan_add(dev, vid, flags);
>   if (err == -EOPNOTSUPP)

This will introduce a checkpatch warning if you re-run with the --file
option.  (I haven't tested).  It's sort of ugly to put function logic in
the declaration block.  That's really just for initializing variables
like "int start = 0; struct foo *p = parent_of_bar();"  Do it like this
instead:

int err;

err = br_switchdev_port_vlan_add(dev, vid, flags);
if (err == -EOPNOTSUPP)

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH net-next v3 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Petr Machata]

A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
initializing a struct switchdev_obj_port_vlan, a piece of code that
repeats on each call site almost verbatim. While in the current codebase
there is just one duplicated add call, the follow-up patches add more of
both add and del calls.

Thus to remove the duplication, extract the repetition into named
functions and reuse.

Signed-off-by: Petr Machata 
---
 net/bridge/br_private.h   | 13 +
 net/bridge/br_switchdev.c | 25 +
 net/bridge/br_vlan.c  | 31 ---
 3 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 11520ed..5216a52 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1139,6 +1139,8 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p,
   unsigned long mask);
 void br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb,
 int type);
+int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags);
+int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid);
 
 static inline void br_switchdev_frame_unmark(struct sk_buff *skb)
 {
@@ -1168,6 +1170,17 @@ static inline int br_switchdev_set_port_flag(struct 
net_bridge_port *p,
return 0;
 }
 
+static inline int br_switchdev_port_vlan_add(struct net_device *dev,
+u16 vid, u16 flags)
+{
+   return -EOPNOTSUPP;
+}
+
+static inline int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid)
+{
+   return -EOPNOTSUPP;
+}
+
 static inline void
 br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
 {
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 35474d4..d77f807 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -136,3 +136,28 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry 
*fdb, int type)
break;
}
 }
+
+int br_switchdev_port_vlan_add(struct net_device *dev, u16 vid, u16 flags)
+{
+   struct switchdev_obj_port_vlan v = {
+   .obj.orig_dev = dev,
+   .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+   .flags = flags,
+   .vid_begin = vid,
+   .vid_end = vid,
+   };
+
+   return switchdev_port_obj_add(dev, &v.obj);
+}
+
+int br_switchdev_port_vlan_del(struct net_device *dev, u16 vid)
+{
+   struct switchdev_obj_port_vlan v = {
+   .obj.orig_dev = dev,
+   .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+   .vid_begin = vid,
+   .vid_end = vid,
+   };
+
+   return switchdev_port_obj_del(dev, &v.obj);
+}
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index dc832c09..8ad5756 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -82,19 +82,10 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 
flags)
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
  u16 vid, u16 flags)
 {
-   struct switchdev_obj_port_vlan v = {
-   .obj.orig_dev = dev,
-   .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-   .flags = flags,
-   .vid_begin = vid,
-   .vid_end = vid,
-   };
-   int err;
-
/* Try switchdev op first. In case it is not supported, fallback to
 * 8021q add.
 */
-   err = switchdev_port_obj_add(dev, &v.obj);
+   int err = br_switchdev_port_vlan_add(dev, vid, flags);
if (err == -EOPNOTSUPP)
return vlan_vid_add(dev, br->vlan_proto, vid);
return err;
@@ -130,18 +121,11 @@ static void __vlan_del_list(struct net_bridge_vlan *v)
 static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
  u16 vid)
 {
-   struct switchdev_obj_port_vlan v = {
-   .obj.orig_dev = dev,
-   .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-   .vid_begin = vid,
-   .vid_end = vid,
-   };
-   int err;
-
/* Try switchdev op first. In case it is not supported, fallback to
 * 8021q del.
 */
-   err = switchdev_port_obj_del(dev, &v.obj);
+   int err = br_switchdev_port_vlan_del(dev, vid);
+
if (err == -EOPNOTSUPP) {
vlan_vid_del(dev, br->vlan_proto, vid);
return 0;
@@ -1053,13 +1037,6 @@ int nbp_vlan_init(struct net_bridge_port *p)
 int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 bool *changed)
 {
-   struct switchdev_obj_port_vlan v = {
-   .obj.orig_dev = port->dev,
-   .obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-   .flags = flags,
-   .vid_begin = vid,
-   .vid_end = vid,
-   };
struct net_bridge_vlan *vlan;
int ret;
 
@@ -1069,7 +1046,7 @@ int