Re: [RFC v4 1/1] net/hsr: Support for the HSR protocol (IEC:2010 / HSR v0)

[Joe Perches <joe@perches.com>]

On Fri, 2012-10-12 at 17:11 +0000, Arvid Brodin wrote:
> This would add support for the High-availability Seamless Redundancy network
> protocol, version 0 (2010).
> 
> This RFC is NOT meant for mainline inclusion at the moment since we're trying
> to figure out how to handle a probable incompatibility with the newer
> HSR v1 (2012) standard.

just trivial comments, ignore as you choose.

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
[]
> +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;
> +	}
> +
> +	if (is_operstate_up(slave1) || is_operstate_up(slave2))
> +		__hsr_set_operstate(hsr_dev, IF_OPER_UP);
> +	else
> +		__hsr_set_operstate(hsr_dev, IF_OPER_LOWERLAYERDOWN);
> +}

sometimes it's better to have a single call like
{
	int type;

	if (!is_admin_up(hsr_dev))
		type = IF_OPER_DOWN;
	else if (is_operstate_up(slave1) || is_operstate_up(slave2))
		type = IF_OPER_UP;
	else
		type = IF_OPER_LOWERLAYERDOWN;

	__hsr_set_operstate(hsr_dev, type);
}

[]

> +static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct hsr_priv *hsr_priv;
> +	struct hsr_ethhdr *hsr_ethhdr;
> +	struct sk_buff *skb2;
> +	int res1, res2;
> +
> +	hsr_priv = netdev_priv(dev);
> +	hsr_ethhdr = (struct hsr_ethhdr *) skb->data;
> +
> +	if ((ntohs(skb->protocol) != ETH_P_HSR) ||
> +			(ntohs(hsr_ethhdr->ethhdr.h_proto) != ETH_P_HSR)) {

These ntohs calls are done at runtime.
The conversion could be done at compile time instead using

	if (skb->protocol != ntohs(ETH_P_HSR) ||
	    hsr_ethhdr->ethhdr.h_proto != ntohs(ETH_P_HSR)) {

[]

> +static void restore_slaves(struct net_device *hsr_dev)
> +{
[]
> +			netdev_info(hsr_dev, "HSR: Cannot restore slave "
> +						"promiscuity (%s, %d)\n",
> +						hsr_priv->slave[i]->name, res);

It's nicer to coalesce formats for easier grep

			netdev_info(hsr_dev, "HSR: Cannot restore slave promiscuity (%s, %d)\n",
				    hsr_priv->slave[i]->name, res);

or maybe use special wrappers for something like:
int hsr_info(hsr, slave, fmt, ...)
{
}

[]

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

> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
> +					     unsigned char addr[ETH_ALEN])
> +{
> +	struct node_entry *node;
> +
> +	list_for_each_entry_rcu(node, node_db, mac_list)
> +		if (!compare_ether_addr(node->MacAddressB, addr))
> +			return node;

please use braces when the list/for has an if