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

[Arvid Brodin <Arvid.Brodin@xdin.com>]

On 2012-07-04 02:30, Joe Perches wrote:
> On Wed, 2012-07-04 at 00:12 +0000, Arvid Brodin wrote:
>> The kernel patch.
> 
> []

What does this mean (the "[]")?

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

Yep, didn't know the bool type existed.

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

I intended to do so when I send the patch of course. I didn't know it would be so frowned
upon in a RFC, I thought it would be enough to note the existence of the commented out
code under known problems, as I did in RFC part 0. Lesson learned!

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

This function is (indirectly) called by the receive callback for packet type ETH_P_HSR
(hsr_rcv() in hsr_main.c). If I recall correctly, I tried GFP_KERNEL first but the kernel
complained over sleeping in atomic context. I'll check it out again.


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

Thank you for your time. I will take care of these issues when I get back from my vacation. :)


-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com