linux-kernel - Re: [PATCH v2 net-next 6/6] net: switchdev: extract struct switchdev_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20150930150031.GG2098@nanopsycho.orion>
Date:	Wed, 30 Sep 2015 17:00:31 +0200
From:	Jiri Pirko <jiri@...nulli.us>
To:	Scott Feldman <sfeldma@...il.com>
Cc:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
	Netdev <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	kernel <kernel@...oirfairelinux.com>,
	"David S. Miller" <davem@...emloft.net>,
	Florian Fainelli <f.fainelli@...il.com>,
	Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v2 net-next 6/6] net: switchdev: extract struct
 switchdev_obj_*

Wed, Sep 30, 2015 at 04:58:38PM CEST, sfeldma@...il.com wrote:
>On Wed, Sep 30, 2015 at 3:36 AM, Jiri Pirko <jiri@...nulli.us> wrote:
>> Tue, Sep 29, 2015 at 06:07:18PM CEST, vivien.didelot@...oirfairelinux.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@...oirfairelinux.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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/