Message-ID: <CAE4R7bCdAQxLo3tKbXAqyGa_F3PhUTUkqrM5jTkYqcJJbCDmog@mail.gmail.com>
Date:	Tue, 6 Oct 2015 09:06:45 -0700
From:	Scott Feldman <sfeldma@...il.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	Netdev <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ido Schimmel <idosch@...lanox.com>, eladr@...lanox.com,
	Thomas Graf <tgraf@...g.ch>,
	Alexei Starovoitov <ast@...mgrid.com>,
	David Laight <David.Laight@...lab.com>,
	john fastabend <john.fastabend@...il.com>
Subject: Re: [patch net-next v3 06/14] rocker: introduce worlds infrastructure

On Tue, Oct 6, 2015 at 12:51 AM, Jiri Pirko <jiri@...nulli.us> wrote:
> From: Jiri Pirko <jiri@...lanox.com>
>
> 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 <jiri@...lanox.com>
> ---
> v1->v2:
>  - removed functions to change worlds based on name, as rtnl mode
>    set patch is removed from patchset.
> v2->v3:
>  - fix checks in rocker_world_port_open and rocker_world_port_stop
> ---
>  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 <linux/kernel.h>
>  #include <linux/types.h>
> +#include <linux/netdevice.h>
> +#include <net/neighbour.h>
> +#include <net/switchdev.h>
>
>  #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);
> +};

Using void * in these ops is unacceptable, I can't agree to this patch.

There is a much cleaner way to architect this.  If you look at the ops
defined, they're mostly duplicates of the already defined
switchdev_ops.  It would be much cleaner to:

0) set port mode on qemu/rocker (the device)
1) get the port mode on port probe
2) based on port mode, set the switchdev_ops to point to the port mode
world switchdev_ops
3) sub-class rocker_port, like I mentioned in before, to store
world-specific stuff in rocker_port

I don't buy the argument that we need to change port mode dynamically
from the driver.  Set it at the device and be done.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists