netdev - Re: [patch net-next RFC 2/4] net: introduce switchdev API

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 20 Mar 2014 13:59:01 +0000
From:	Thomas Graf <tgraf@...g.ch>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, nhorman@...driver.com,
	andy@...yhouse.net, dborkman@...hat.com, ogerlitz@...lanox.com,
	jesse@...ira.com, pshelar@...ira.com, azhou@...ira.com,
	ben@...adent.org.uk, stephen@...workplumber.org,
	jeffrey.t.kirsher@...el.com, vyasevic@...hat.com,
	xiyou.wangcong@...il.com, john.r.fastabend@...el.com,
	edumazet@...gle.com
Subject: Re: [patch net-next RFC 2/4] net: introduce switchdev API

On 03/19/14 at 04:33pm, Jiri Pirko wrote:
> +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

> +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?

> +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.

> +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

> +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.

> +	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);
--
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