Re: [RFC v2 1/2] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy

[Joe Perches <joe@perches.com>]

On Wed, 2012-07-04 at 00:12 +0000, Arvid Brodin wrote:
> The kernel patch.

[]

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c

> @@ -0,0 +1,531 @@

> +static int is_admin_up(struct net_device *dev)
> +{
> +	return (dev->flags & IFF_UP);
> +}
> +
> +static int is_operstate_up(struct net_device *dev)
> +{
> +	return (dev->operstate == IF_OPER_UP);
> +}

bool?

> +static void __hsr_set_operstate(struct net_device *dev, int transition)
> +{
> +	if (dev->operstate != transition) {
> +/*
> +		switch (transition) {
> +		case IF_OPER_UP:
> +			printk(KERN_INFO "%s: new operstate is IF_OPER_UP\n", dev->name);

	netdev_info(dev, "new operstate is IF_OPER_UP\n");

> +			break;
> +		default:
> +			printk(KERN_INFO "%s: new operstate is !IF_OPER_UP (%d)\n", dev->name, transition);

etc.

> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
> +						struct net_device *slave2)
> +{
> +	if (!is_admin_up(hsr_dev)) {
> +		__hsr_set_operstate(hsr_dev, IF_OPER_DOWN);
> +		return;
> +	}
> +/*
> +	printk(KERN_INFO "Slave1/2 operstate: %d/%d\n",
> +					slave1->operstate, slave2->operstate);
> +*/

Please remove commented out code.

> +static void restore_slaves(struct net_device *hsr_dev)
> +{
> +	struct hsr_priv *hsr_priv;
> +	struct net_device *slave[2];
> +	int i;
> +	int res;
> +
> +	hsr_priv = netdev_priv(hsr_dev);
> +	for (i = 0; i < 2; i++)
> +		slave[i] = hsr_priv->slave_data[i].dev;
> +
> +	rtnl_lock();
> +
> +	/* Restore promiscuity */
> +	for (i = 0; i < 2; i++) {
> +		if (!hsr_priv->slave_data[i].promisc)
> +			continue;
> +		res = dev_set_promiscuity(slave[i],
> +					-hsr_priv->slave_data[i].promisc);
> +		if (res)
> +			pr_info("HSR: Cannot restore promiscuity (%s, %d)\n",
> +							slave[i]->name,
> +							res);

shouldn't this just be:

			netdev_info(slave[i], "cannot restore promiscuity: %d\n",
				    res);

If you must use pr_<level> please add
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any include and let the printk subsystem
add MODNAME as a prefix.


> +static int check_slave_ok(struct net_device *dev)
> +{
> +	/* Don't allow HSR on non-ethernet like devices */
> +	if ((dev->flags & IFF_LOOPBACK) || (dev->type != ARPHRD_ETHER) ||
> +						(dev->addr_len != ETH_ALEN)) {
> +		pr_info("%s: Cannot enslave loopback or non-ethernet device\n",
> +								dev->name);

		netdev_info(dev, "Cannot enslave...");

> +		return -EINVAL;
> +	}
> +
> +	/* Don't allow enslaving hsr devices */
> +	if (is_hsr_master(dev)) {
> +		pr_info("%s: Don't try to create trees of hsr devices!\n",
> +								dev->name);


		netdev_err(dev, "Cannot create trees of hsr devices\n");


> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2])
> +{

> +	/* Set hsr_dev's MAC address to that of mac_slave1 */
> +	memcpy(hsr_dev->dev_addr, hsr_priv->slave_data[0].dev->dev_addr,
> +							hsr_dev->addr_len);

ETH_ALEN?

> diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
[]
> @@ -0,0 +1,27 @@

> +void hsr_dev_setup(struct net_device *dev);
> +int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2]);
> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
> +						struct net_device *slave2);

please align arguments immediately after the open parenthesis.

> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
[]
> +static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
> +						unsigned char addr[ETH_ALEN])

static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
					     unsigned char addr[ETH_ALEN])
[]
> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
> +						unsigned char addr[ETH_ALEN])

Alignment...

> +int framereg_merge_node(struct hsr_priv *hsr_priv, enum hsr_dev_idx dev_idx,
> +							struct sk_buff *skb)
> +{
[]
> +	node = find_node_by_AddrA(&hsr_priv->node_db, hsr_stag->MacAddressA);
> +	if (!node) {
> +		rcu_read_unlock();
> +		found = 0;
> +		node = kmalloc(sizeof(*node), GFP_ATOMIC);

why GFP_ATOMIC?

> +		if (!node)
> +			return -ENOMEM;
> +
> +		memcpy(node->MacAddressA, hsr_stag->MacAddressA, ETH_ALEN);
> +		memcpy(node->MacAddressB, ethhdr->h_source, ETH_ALEN);
> +
> +		for (i = 0; i < HSR_MAX_SLAVE; i++)
> +			node->time_in[i] = 0;
> +		for (i = 0; i < HSR_MAX_DEV; i++)
> +			node->seq_out[i] = 0;
> +/*
> +		printk(KERN_INFO "HSR: Added node %pM / %pM\n",
> +							node->MacAddressA,
> +							node->MacAddressB);
> +*/

Please remove commented out code here and everywhere else...

[too long, stopped reading]