Re: [patch net-next v2 06/13] rocker: introduce worlds infrastructure

[Rustad, Mark D]

Jiri Pirko  wrote:

>>> +   int (*port_init)(struct rocker_port *rocker_port, void *priv,
>>> +void *port_priv);
>> 
>> Yuck, void *.  Can we do better?
> 
> I see nothing wrong with this priv usage. It's done like this on many
> places. I think it is completely legit, since the call points are well
> defined and wrapped.

This particular call is perhaps the most troubling. In general, if there is one 
void parameter you may well get a compile error on a non-void parameter if you 
get them switched around. With two void parameters that is no longer the case, 
making it even more error-prone than the other uses of void *.

--
Mark Rustad, Networking Division, Intel Corporation


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [patch net-next v2 06/13] rocker: introduce worlds infrastructure

[Jiri Pirko]

Tue, Oct 06, 2015 at 07:16:29AM CEST, sfel...@gmail.com wrote:
>On Mon, Oct 5, 2015 at 10:43 AM, Jiri Pirko  wrote:
>> From: Jiri Pirko 
>>
>> This is another step on the way to per-world clean cut. Introduce world
>> ops hooks which each world can implement in world-specific way.
>> Also introduce world infrastructure including function for port worlds
>> change.
>>
>> Signed-off-by: Jiri Pirko 
>> ---
>> v1->v2:
>>  - removed functions to change worlds based on name, as rtnl mode
>>set patch is removed from patchset.
>> ---
>>  drivers/net/ethernet/rocker/rocker.h  |  56 
>>  drivers/net/ethernet/rocker/rocker_main.c | 464 
>> +-
>>  2 files changed, 519 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/rocker/rocker.h 
>> b/drivers/net/ethernet/rocker/rocker.h
>> index 650caa0..d49bc5d 100644
>> --- a/drivers/net/ethernet/rocker/rocker.h
>> +++ b/drivers/net/ethernet/rocker/rocker.h
>> @@ -12,7 +12,11 @@
>>  #ifndef _ROCKER_H
>>  #define _ROCKER_H
>>
>> +#include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>>
>>  #include "rocker_hw.h"
>>
>> @@ -24,4 +28,56 @@ struct rocker_desc_info {
>> dma_addr_t mapaddr;
>>  };
>>
>> +struct rocker;
>> +struct rocker_port;
>> +
>> +struct rocker_world_ops {
>> +   const char *kind;
>> +   size_t priv_size;
>> +   size_t port_priv_size;
>> +   u8 mode;
>> +   int (*init)(struct rocker *rocker, void *priv);
>> +   void (*fini)(void *priv);
>> +   int (*port_init)(struct rocker_port *rocker_port, void *priv,
>> +void *port_priv);
>> +   void (*port_fini)(void *port_priv);
>> +   int (*port_open)(void *port_priv);
>> +   void (*port_stop)(void *port_priv);
>> +   int (*port_attr_stp_state_set)(void *port_priv, u8 state,
>> +  struct switchdev_trans *trans);
>> +   int (*port_attr_bridge_flags_set)(void *port_priv,
>> + unsigned long brport_flags,
>> + struct switchdev_trans *trans);
>> +   int (*port_attr_bridge_flags_get)(void *port_priv,
>> + unsigned long *p_brport_flags);
>> +   int (*port_obj_vlan_add)(void *port_priv,
>> +const struct switchdev_obj_port_vlan *vlan,
>> +struct switchdev_trans *trans);
>> +   int (*port_obj_vlan_del)(void *port_priv,
>> +const struct switchdev_obj_port_vlan *vlan);
>> +   int (*port_obj_vlan_dump)(void *port_priv,
>> + struct switchdev_obj_port_vlan *vlan,
>> + switchdev_obj_dump_cb_t *cb);
>> +   int (*port_obj_fib4_add)(void *port_priv,
>> +const struct switchdev_obj_ipv4_fib *fib4,
>> +struct switchdev_trans *trans);
>> +   int (*port_obj_fib4_del)(void *port_priv,
>> +const struct switchdev_obj_ipv4_fib *fib4);
>> +   int (*port_obj_fdb_add)(void *port_priv,
>> +   const struct switchdev_obj_port_fdb *fdb,
>> +   struct switchdev_trans *trans);
>> +   int (*port_obj_fdb_del)(void *port_priv,
>> +   const struct switchdev_obj_port_fdb *fdb);
>> +   int (*port_obj_fdb_dump)(void *port_priv,
>> +struct switchdev_obj_port_fdb *fdb,
>> +switchdev_obj_dump_cb_t *cb);
>> +   int (*port_master_linked)(void *port_priv, struct net_device 
>> *master);
>> +   int (*port_master_unlinked)(void *port_priv, struct net_device 
>> *master);
>> +   int (*port_neigh_update)(void *port_priv, struct neighbour *n);
>> +   int (*port_neigh_destroy)(void *port_priv, struct neighbour *n);
>> +   int (*port_ev_mac_vlan_seen)(void *port_priv,
>> +const unsigned char *addr,
>> +__be16 vlan_id);
>> +};
>
>Yuck, void *.  Can we do better?

I see nothing wrong with this priv usage. It's done like this on many
places. I think it is completely legit, since the call points are well
defined and wrapped.


>
>How about using base struct rocker_port and then sub-classing
>world-specific rocker ports.  And make rocker_world_ops pass struct
>rocker_port * (for the port-centric ops).
>
>struct rocker_port {
>// common stuff for all worlds
>};
>
>struct my_world_port {
>struct rocker_port rocker_port;
>// world-specific stuff for port
>};
>
>#define MY_WORLD_PORT(rocker_port) \
>container_of(rocker_port, struct my_world_port, rocker_port)
>
>Same for world-centric ops:
>
>struct rocker_world {
>   // common stuff for all worlds
>};
>
>struct my_world {
>struct 

[patch net-next v2 06/13] rocker: introduce worlds infrastructure

[Jiri Pirko]

From: Jiri Pirko 

This is another step on the way to per-world clean cut. Introduce world
ops hooks which each world can implement in world-specific way.
Also introduce world infrastructure including function for port worlds
change.

Signed-off-by: Jiri Pirko 
---
v1->v2:
 - removed functions to change worlds based on name, as rtnl mode
   set patch is removed from patchset.
---
 drivers/net/ethernet/rocker/rocker.h  |  56 
 drivers/net/ethernet/rocker/rocker_main.c | 464 +-
 2 files changed, 519 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/rocker/rocker.h 
b/drivers/net/ethernet/rocker/rocker.h
index 650caa0..d49bc5d 100644
--- a/drivers/net/ethernet/rocker/rocker.h
+++ b/drivers/net/ethernet/rocker/rocker.h
@@ -12,7 +12,11 @@
 #ifndef _ROCKER_H
 #define _ROCKER_H
 
+#include 
 #include 
+#include 
+#include 
+#include 
 
 #include "rocker_hw.h"
 
@@ -24,4 +28,56 @@ struct rocker_desc_info {
dma_addr_t mapaddr;
 };
 
+struct rocker;
+struct rocker_port;
+
+struct rocker_world_ops {
+   const char *kind;
+   size_t priv_size;
+   size_t port_priv_size;
+   u8 mode;
+   int (*init)(struct rocker *rocker, void *priv);
+   void (*fini)(void *priv);
+   int (*port_init)(struct rocker_port *rocker_port, void *priv,
+void *port_priv);
+   void (*port_fini)(void *port_priv);
+   int (*port_open)(void *port_priv);
+   void (*port_stop)(void *port_priv);
+   int (*port_attr_stp_state_set)(void *port_priv, u8 state,
+  struct switchdev_trans *trans);
+   int (*port_attr_bridge_flags_set)(void *port_priv,
+ unsigned long brport_flags,
+ struct switchdev_trans *trans);
+   int (*port_attr_bridge_flags_get)(void *port_priv,
+ unsigned long *p_brport_flags);
+   int (*port_obj_vlan_add)(void *port_priv,
+const struct switchdev_obj_port_vlan *vlan,
+struct switchdev_trans *trans);
+   int (*port_obj_vlan_del)(void *port_priv,
+const struct switchdev_obj_port_vlan *vlan);
+   int (*port_obj_vlan_dump)(void *port_priv,
+ struct switchdev_obj_port_vlan *vlan,
+ switchdev_obj_dump_cb_t *cb);
+   int (*port_obj_fib4_add)(void *port_priv,
+const struct switchdev_obj_ipv4_fib *fib4,
+struct switchdev_trans *trans);
+   int (*port_obj_fib4_del)(void *port_priv,
+const struct switchdev_obj_ipv4_fib *fib4);
+   int (*port_obj_fdb_add)(void *port_priv,
+   const struct switchdev_obj_port_fdb *fdb,
+   struct switchdev_trans *trans);
+   int (*port_obj_fdb_del)(void *port_priv,
+   const struct switchdev_obj_port_fdb *fdb);
+   int (*port_obj_fdb_dump)(void *port_priv,
+struct switchdev_obj_port_fdb *fdb,
+switchdev_obj_dump_cb_t *cb);
+   int (*port_master_linked)(void *port_priv, struct net_device *master);
+   int (*port_master_unlinked)(void *port_priv, struct net_device *master);
+   int (*port_neigh_update)(void *port_priv, struct neighbour *n);
+   int (*port_neigh_destroy)(void *port_priv, struct neighbour *n);
+   int (*port_ev_mac_vlan_seen)(void *port_priv,
+const unsigned char *addr,
+__be16 vlan_id);
+};
+
 #endif
diff --git a/drivers/net/ethernet/rocker/rocker_main.c 
b/drivers/net/ethernet/rocker/rocker_main.c
index 54d2f46..3bfb9e9 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -210,6 +210,8 @@ struct rocker_port {
struct net_device *dev;
struct net_device *bridge_dev;
struct rocker *rocker;
+   struct rocker_world *world;
+   void *world_port_priv;
unsigned int port_number;
u32 pport;
__be16 internal_vlan_id;
@@ -236,6 +238,7 @@ struct rocker {
spinlock_t cmd_ring_lock;   /* for cmd ring accesses */
struct rocker_dma_ring_info cmd_ring;
struct rocker_dma_ring_info event_ring;
+   struct list_head worlds;
DECLARE_HASHTABLE(flow_tbl, 16);
spinlock_t flow_tbl_lock;   /* for flow tbl accesses */
u64 flow_tbl_next_cookie;
@@ -1236,6 +1239,9 @@ static int rocker_port_fdb(struct rocker_port 
*rocker_port,
   struct switchdev_trans *trans,
   const unsigned char *addr,
   __be16 vlan_id, int flags);
+static int 

Re: [patch net-next v2 06/13] rocker: introduce worlds infrastructure

[Scott Feldman]

On Mon, Oct 5, 2015 at 10:43 AM, Jiri Pirko  wrote:
> From: Jiri Pirko 
>
> This is another step on the way to per-world clean cut. Introduce world
> ops hooks which each world can implement in world-specific way.
> Also introduce world infrastructure including function for port worlds
> change.
>
> Signed-off-by: Jiri Pirko 
> ---
> v1->v2:
>  - removed functions to change worlds based on name, as rtnl mode
>set patch is removed from patchset.
> ---
>  drivers/net/ethernet/rocker/rocker.h  |  56 
>  drivers/net/ethernet/rocker/rocker_main.c | 464 
> +-
>  2 files changed, 519 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/rocker/rocker.h 
> b/drivers/net/ethernet/rocker/rocker.h
> index 650caa0..d49bc5d 100644
> --- a/drivers/net/ethernet/rocker/rocker.h
> +++ b/drivers/net/ethernet/rocker/rocker.h
> @@ -12,7 +12,11 @@
>  #ifndef _ROCKER_H
>  #define _ROCKER_H
>
> +#include 
>  #include 
> +#include 
> +#include 
> +#include 
>
>  #include "rocker_hw.h"
>
> @@ -24,4 +28,56 @@ struct rocker_desc_info {
> dma_addr_t mapaddr;
>  };
>
> +struct rocker;
> +struct rocker_port;
> +
> +struct rocker_world_ops {
> +   const char *kind;
> +   size_t priv_size;
> +   size_t port_priv_size;
> +   u8 mode;
> +   int (*init)(struct rocker *rocker, void *priv);
> +   void (*fini)(void *priv);
> +   int (*port_init)(struct rocker_port *rocker_port, void *priv,
> +void *port_priv);
> +   void (*port_fini)(void *port_priv);
> +   int (*port_open)(void *port_priv);
> +   void (*port_stop)(void *port_priv);
> +   int (*port_attr_stp_state_set)(void *port_priv, u8 state,
> +  struct switchdev_trans *trans);
> +   int (*port_attr_bridge_flags_set)(void *port_priv,
> + unsigned long brport_flags,
> + struct switchdev_trans *trans);
> +   int (*port_attr_bridge_flags_get)(void *port_priv,
> + unsigned long *p_brport_flags);
> +   int (*port_obj_vlan_add)(void *port_priv,
> +const struct switchdev_obj_port_vlan *vlan,
> +struct switchdev_trans *trans);
> +   int (*port_obj_vlan_del)(void *port_priv,
> +const struct switchdev_obj_port_vlan *vlan);
> +   int (*port_obj_vlan_dump)(void *port_priv,
> + struct switchdev_obj_port_vlan *vlan,
> + switchdev_obj_dump_cb_t *cb);
> +   int (*port_obj_fib4_add)(void *port_priv,
> +const struct switchdev_obj_ipv4_fib *fib4,
> +struct switchdev_trans *trans);
> +   int (*port_obj_fib4_del)(void *port_priv,
> +const struct switchdev_obj_ipv4_fib *fib4);
> +   int (*port_obj_fdb_add)(void *port_priv,
> +   const struct switchdev_obj_port_fdb *fdb,
> +   struct switchdev_trans *trans);
> +   int (*port_obj_fdb_del)(void *port_priv,
> +   const struct switchdev_obj_port_fdb *fdb);
> +   int (*port_obj_fdb_dump)(void *port_priv,
> +struct switchdev_obj_port_fdb *fdb,
> +switchdev_obj_dump_cb_t *cb);
> +   int (*port_master_linked)(void *port_priv, struct net_device *master);
> +   int (*port_master_unlinked)(void *port_priv, struct net_device 
> *master);
> +   int (*port_neigh_update)(void *port_priv, struct neighbour *n);
> +   int (*port_neigh_destroy)(void *port_priv, struct neighbour *n);
> +   int (*port_ev_mac_vlan_seen)(void *port_priv,
> +const unsigned char *addr,
> +__be16 vlan_id);
> +};

Yuck, void *.  Can we do better?

How about using base struct rocker_port and then sub-classing
world-specific rocker ports.  And make rocker_world_ops pass struct
rocker_port * (for the port-centric ops).

struct rocker_port {
// common stuff for all worlds
};

struct my_world_port {
struct rocker_port rocker_port;
// world-specific stuff for port
};

#define MY_WORLD_PORT(rocker_port) \
container_of(rocker_port, struct my_world_port, rocker_port)

Same for world-centric ops:

struct rocker_world {
   // common stuff for all worlds
};

struct my_world {
struct rocker_world rocker_world;
// stuff for this world
};


> +static int rocker_world_port_ev_mac_vlan_seen(struct rocker_port 
> *rocker_port,
> + const unsigned char *addr,
> + __be16 vlan_id);
>
>  static int rocker_event_mac_vlan_seen(const struct rocker *rocker,