| Message ID | 1443542838-19234-7-git-send-email-vivien.didelot@savoirfairelinux.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Tue, Sep 29, 2015 at 06:07:18PM CEST, vivien.didelot@savoirfairelinux.com wrote: >Now that switchdev and its drivers directly use specific switchdev_obj_* >structures, move them out of the switchdev_obj union and get rif of this >outer structure. > >Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >--- > include/net/switchdev.h | 53 ++++++++++++++++++++++++------------------------- > 1 file changed, 26 insertions(+), 27 deletions(-) > >diff --git a/include/net/switchdev.h b/include/net/switchdev.h >index bcadac3..e11425e 100644 >--- a/include/net/switchdev.h >+++ b/include/net/switchdev.h >@@ -64,30 +64,29 @@ enum switchdev_obj_id { > SWITCHDEV_OBJ_PORT_FDB, > }; > >-struct switchdev_obj { >- enum switchdev_obj_id id; >- int (*cb)(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; >- 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; >- u16 ndm_state; >- } fdb; >- } u; >+/* SWITCHDEV_OBJ_PORT_VLAN */ >+struct switchdev_obj_vlan { >+ u16 flags; >+ u16 vid_begin; >+ u16 vid_end; >+}; >+ >+/* SWITCHDEV_OBJ_IPV4_FIB */ >+struct switchdev_obj_ipv4_fib { >+ u32 dst; >+ int dst_len; >+ struct fib_info *fi; >+ u8 tos; >+ u8 type; >+ u32 nlflags; >+ u32 tb_id; >+}; >+ >+/* SWITCHDEV_OBJ_PORT_FDB */ >+struct switchdev_obj_fdb { >+ const unsigned char *addr; >+ u16 vid; >+ u16 ndm_state; > }; I don't like these structs being passed down as a "void *". I think that we should have some "common" struct for these objects, event if it would be empty and pass it down. "void *" does not look good at all, does not tell the reader what that param is about. How about: struct switchdev_obj { }; struct switchdev_obj_vlan { struct switchdev_obj obj; u16 flags; u16 vid_begin; u16 vid_end; }; #define SWITCHDEV_OBJ_VLAN(obj) \ container_of(obj, struct switchdev_obj_vlan, obj) /* SWITCHDEV_OBJ_IPV4_FIB */ struct switchdev_obj_ipv4_fib { struct switchdev_obj obj; u32 dst; int dst_len; struct fib_info *fi; u8 tos; u8 type; u32 nlflags; u32 tb_id; }; #define SWITCHDEV_OBJ_IPV4_FIB(obj) \ container_of(obj, struct switchdev_obj_ipv4_fib, obj) /* SWITCHDEV_OBJ_PORT_FDB */ struct switchdev_obj_fdb { struct switchdev_obj obj; const unsigned char *addr; u16 vid; u16 ndm_state; }; #define SWITCHDEV_OBJ_FDB(obj) \ container_of(obj, struct switchdev_obj_fdb, obj) then pass struct switchdev_obj *obj down to drivers and in driver, get original object by SWITCHDEV_OBJ_* ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 30, 2015 at 3:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Tue, Sep 29, 2015 at 06:07:18PM CEST, vivien.didelot@savoirfairelinux.com wrote: >>Now that switchdev and its drivers directly use specific switchdev_obj_* >>structures, move them out of the switchdev_obj union and get rif of this >>outer structure. >> >>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >>--- >> include/net/switchdev.h | 53 ++++++++++++++++++++++++------------------------- >> 1 file changed, 26 insertions(+), 27 deletions(-) >> >>diff --git a/include/net/switchdev.h b/include/net/switchdev.h >>index bcadac3..e11425e 100644 >>--- a/include/net/switchdev.h >>+++ b/include/net/switchdev.h >>@@ -64,30 +64,29 @@ enum switchdev_obj_id { >> SWITCHDEV_OBJ_PORT_FDB, >> }; >> >>-struct switchdev_obj { >>- enum switchdev_obj_id id; >>- int (*cb)(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; >>- 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; >>- u16 ndm_state; >>- } fdb; >>- } u; >>+/* SWITCHDEV_OBJ_PORT_VLAN */ >>+struct switchdev_obj_vlan { >>+ u16 flags; >>+ u16 vid_begin; >>+ u16 vid_end; >>+}; >>+ >>+/* SWITCHDEV_OBJ_IPV4_FIB */ >>+struct switchdev_obj_ipv4_fib { >>+ u32 dst; >>+ int dst_len; >>+ struct fib_info *fi; >>+ u8 tos; >>+ u8 type; >>+ u32 nlflags; >>+ u32 tb_id; >>+}; >>+ >>+/* SWITCHDEV_OBJ_PORT_FDB */ >>+struct switchdev_obj_fdb { >>+ const unsigned char *addr; >>+ u16 vid; >>+ u16 ndm_state; >> }; > > > I don't like these structs being passed down as a "void *". I think that > we should have some "common" struct for these objects, event if it would > be empty and pass it down. "void *" does not look good at all, does not > tell the reader what that param is about. How about: > > struct switchdev_obj { > }; > > struct switchdev_obj_vlan { > struct switchdev_obj obj; > u16 flags; > u16 vid_begin; > u16 vid_end; > }; > #define SWITCHDEV_OBJ_VLAN(obj) \ > container_of(obj, struct switchdev_obj_vlan, obj) > > /* SWITCHDEV_OBJ_IPV4_FIB */ > struct switchdev_obj_ipv4_fib { > struct switchdev_obj obj; > u32 dst; > int dst_len; > struct fib_info *fi; > u8 tos; > u8 type; > u32 nlflags; > u32 tb_id; > }; > #define SWITCHDEV_OBJ_IPV4_FIB(obj) \ > container_of(obj, struct switchdev_obj_ipv4_fib, obj) > > /* SWITCHDEV_OBJ_PORT_FDB */ > struct switchdev_obj_fdb { > struct switchdev_obj obj; > const unsigned char *addr; > u16 vid; > u16 ndm_state; > }; > #define SWITCHDEV_OBJ_FDB(obj) \ > container_of(obj, struct switchdev_obj_fdb, obj) > > > then pass struct switchdev_obj *obj down to drivers and in driver, get > original object by SWITCHDEV_OBJ_* ? Yes, I agree with Jiri, keep the struct switchdev_obj, use it within each specific struct, and pass struct switchdev * in core funcs rather than void *. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wed, Sep 30, 2015 at 04:58:38PM CEST, sfeldma@gmail.com wrote: >On Wed, Sep 30, 2015 at 3:36 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Tue, Sep 29, 2015 at 06:07:18PM CEST, vivien.didelot@savoirfairelinux.com wrote: >>>Now that switchdev and its drivers directly use specific switchdev_obj_* >>>structures, move them out of the switchdev_obj union and get rif of this >>>outer structure. >>> >>>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> >>>--- >>> include/net/switchdev.h | 53 ++++++++++++++++++++++++------------------------- >>> 1 file changed, 26 insertions(+), 27 deletions(-) >>> >>>diff --git a/include/net/switchdev.h b/include/net/switchdev.h >>>index bcadac3..e11425e 100644 >>>--- a/include/net/switchdev.h >>>+++ b/include/net/switchdev.h >>>@@ -64,30 +64,29 @@ enum switchdev_obj_id { >>> SWITCHDEV_OBJ_PORT_FDB, >>> }; >>> >>>-struct switchdev_obj { >>>- enum switchdev_obj_id id; >>>- int (*cb)(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; >>>- 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; >>>- u16 ndm_state; >>>- } fdb; >>>- } u; >>>+/* SWITCHDEV_OBJ_PORT_VLAN */ >>>+struct switchdev_obj_vlan { >>>+ u16 flags; >>>+ u16 vid_begin; >>>+ u16 vid_end; >>>+}; >>>+ >>>+/* SWITCHDEV_OBJ_IPV4_FIB */ >>>+struct switchdev_obj_ipv4_fib { >>>+ u32 dst; >>>+ int dst_len; >>>+ struct fib_info *fi; >>>+ u8 tos; >>>+ u8 type; >>>+ u32 nlflags; >>>+ u32 tb_id; >>>+}; >>>+ >>>+/* SWITCHDEV_OBJ_PORT_FDB */ >>>+struct switchdev_obj_fdb { >>>+ const unsigned char *addr; >>>+ u16 vid; >>>+ u16 ndm_state; >>> }; >> >> >> I don't like these structs being passed down as a "void *". I think that >> we should have some "common" struct for these objects, event if it would >> be empty and pass it down. "void *" does not look good at all, does not >> tell the reader what that param is about. How about: >> >> struct switchdev_obj { >> }; >> >> struct switchdev_obj_vlan { >> struct switchdev_obj obj; >> u16 flags; >> u16 vid_begin; >> u16 vid_end; >> }; >> #define SWITCHDEV_OBJ_VLAN(obj) \ >> container_of(obj, struct switchdev_obj_vlan, obj) >> >> /* SWITCHDEV_OBJ_IPV4_FIB */ >> struct switchdev_obj_ipv4_fib { >> struct switchdev_obj obj; >> u32 dst; >> int dst_len; >> struct fib_info *fi; >> u8 tos; >> u8 type; >> u32 nlflags; >> u32 tb_id; >> }; >> #define SWITCHDEV_OBJ_IPV4_FIB(obj) \ >> container_of(obj, struct switchdev_obj_ipv4_fib, obj) >> >> /* SWITCHDEV_OBJ_PORT_FDB */ >> struct switchdev_obj_fdb { >> struct switchdev_obj obj; >> const unsigned char *addr; >> u16 vid; >> u16 ndm_state; >> }; >> #define SWITCHDEV_OBJ_FDB(obj) \ >> container_of(obj, struct switchdev_obj_fdb, obj) >> >> >> then pass struct switchdev_obj *obj down to drivers and in driver, get >> original object by SWITCHDEV_OBJ_* ? > > >Yes, I agree with Jiri, keep the struct switchdev_obj, use it within >each specific struct, and pass struct switchdev * in core funcs rather >than void *. I will cook-up a patch then. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/switchdev.h b/include/net/switchdev.h index bcadac3..e11425e 100644 --- a/include/net/switchdev.h +++ b/include/net/switchdev.h @@ -64,30 +64,29 @@ enum switchdev_obj_id { SWITCHDEV_OBJ_PORT_FDB, }; -struct switchdev_obj { - enum switchdev_obj_id id; - int (*cb)(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; - 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; - u16 ndm_state; - } fdb; - } u; +/* SWITCHDEV_OBJ_PORT_VLAN */ +struct switchdev_obj_vlan { + u16 flags; + u16 vid_begin; + u16 vid_end; +}; + +/* SWITCHDEV_OBJ_IPV4_FIB */ +struct switchdev_obj_ipv4_fib { + u32 dst; + int dst_len; + struct fib_info *fi; + u8 tos; + u8 type; + u32 nlflags; + u32 tb_id; +}; + +/* SWITCHDEV_OBJ_PORT_FDB */ +struct switchdev_obj_fdb { + const unsigned char *addr; + u16 vid; + u16 ndm_state; }; void switchdev_trans_item_enqueue(struct switchdev_trans *trans, @@ -102,11 +101,11 @@ void *switchdev_trans_item_dequeue(struct switchdev_trans *trans); * * @switchdev_port_attr_set: Set a port attribute (see switchdev_attr). * - * @switchdev_port_obj_add: Add an object to port (see switchdev_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_del: Delete an object from port (see switchdev_obj_*). * - * @switchdev_port_obj_dump: Dump port objects (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,
Now that switchdev and its drivers directly use specific switchdev_obj_* structures, move them out of the switchdev_obj union and get rif of this outer structure. Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com> --- include/net/switchdev.h | 53 ++++++++++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 27 deletions(-)