Thanks for review Thomas.

Thu, Mar 20, 2014 at 02:59:01PM CET, tgraf@suug.ch wrote:

On 03/19/14 at 04:33pm, Jiri Pirko wrote:

quoted

+struct swdev_linked_ops {
+};

I've been trying to think of better names for this to make
it absolutely clear which is which (linked ops vs. ops).

What do you think about the following?

   sw_api       -> sw_api_ops / sw_api_port_ops 
      | 
  sw_device
      |
  sw_driver     -> sw_driver_ops / sw_driver_port_ops

Sure. Makes sense. Will change that.

quoted

+bool swdev_dev_check(const struct net_device *dev);
+void swdev_link(struct net_device *dev,
+		const struct swdev_linked_ops *linked_ops,
+		void *linked_priv);
+void swdev_unlink(struct net_device *dev);
+void *swdev_linked_priv(const struct net_device *dev);
+bool swdev_is_linked(const struct net_device *dev);
+int swdev_flow_insert(struct net_device *dev, struct sw_flow *flow);
+int swdev_flow_remove(struct net_device *dev, struct sw_flow *flow);
+int swdev_packet_upcall(struct net_device *dev, struct sk_buff *skb);
+
+struct swdev_ops {
+	const char *kind;
+	int (*flow_insert)(struct net_device *dev, struct sw_flow *flow);
+	int (*flow_remove)(struct net_device *dev, struct sw_flow *flow);

I think this API should be made more extendable. Flags might be
needed at some point or even switch specific configuration
blobs. How about adding a struct sw_flow_opts early on to avoid
cluttering the function parameter list later on?

Hmm. I'm not in favor to add thing for the reason "they might be needed".
I think it would be better to wait for the need and change the API after
that. No need to do it now IMO.

quoted

+int swdev_flow_insert(struct net_device *dev, struct sw_flow *flow)
+{
+	struct swdev *sw = netdev_priv(dev);
+
+	BUG_ON(!swdev_dev_check(dev));

How about taking the swdev struct instead to make it clear that
all these swdev_ functions are only supposed to be used with
swdev instances? We can translate the swdev pointer to a net_device.

Valid point. Will look into this.

quoted

+bool swportdev_dev_check(const struct net_device *port_dev)
+{
+	return port_dev->netdev_ops == &swportdev_netdev_ops;
+}
+EXPORT_SYMBOL(swportdev_dev_check);

Same as above

quoted

+struct net_device *swportdev_create(struct net_device *dev,
+				    const struct swportdev_ops *ops)
+{
+	struct net_device *port_dev;
+	char name[IFNAMSIZ];
+	struct swportdev *swp;
+	int err;

Needs a check that dev is of the same family as the provided
port ops.

Noted, will fix that.

quoted

+	err = snprintf(name, IFNAMSIZ, "%sp%%d", dev->name);
+	if (err >= IFNAMSIZ)
+		return ERR_PTR(-EINVAL);
+
+	port_dev = alloc_netdev(sizeof(struct swportdev), name, swportdev_setup);
+	if (!port_dev)
+		return ERR_PTR(-ENOMEM);
+
+	err = register_netdevice(port_dev);
+	if (err)
+		goto err_register_netdevice;
+
+	err = netdev_master_upper_dev_link(port_dev, dev);
+	if (err) {
+		netdev_err(dev, "Device %s failed to set upper link\n",
+			   port_dev->name);
+		goto err_set_upper_link;
+	}
+	swp = netdev_priv(port_dev);
+	err = netdev_rx_handler_register(port_dev, swportdev_handle_frame, swp);
+	if (err) {
+		netdev_err(dev, "Device %s failed to register rx_handler\n",
+			   port_dev->name);
+		goto err_handler_register;
+	}
+
+	swp = netdev_priv(port_dev);
+	swp->ops = ops;
+	netif_carrier_off(port_dev);
+	netdev_info(port_dev, "Switch port device created (%s)\n", swp->ops->kind);
+	return port_dev;
+
+err_handler_register:
+	netdev_upper_dev_unlink(port_dev, dev);
+err_set_upper_link:
+	unregister_netdevice(port_dev);
+err_register_netdevice:
+	free_netdev(port_dev);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL(swportdev_create);