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 15:18:15 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Thomas Graf <tgraf@...g.ch>
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

Thanks for review Thomas.

Thu, Mar 20, 2014 at 02:59:01PM CET, tgraf@...g.ch wrote:
>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

Sure. Makes sense. Will change that.


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


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


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

Noted, will fix that.


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