Re: [PATCH v5] net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

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

On 2013-10-23 18:52, Joe Perches wrote:
> On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:
>> High-availability Seamless Redundancy ("HSR") provides instant failover
>> redundancy for Ethernet networks. It requires a special network topology where
>> all nodes are connected in a ring (each node having two physical network
>> interfaces). It is suited for applications that demand high availability and
>> very short reaction time.
> 
> trivia: (can be ignored/fixed later)
> 
>> +static void restore_slaves(struct net_device *hsr_dev)
>> +{
>> +	struct hsr_priv *hsr_priv;
>> +	int i;
>> +	int res;
>> +
>> +	hsr_priv = netdev_priv(hsr_dev);
>> +
>> +	rtnl_lock();
>> +
>> +	/* Restore promiscuity */
>> +	for (i = 0; i < 2; i++) {
> 
> I presume all of these for slave loops that use
> for (i = 0; i < 2; i++) should be i < HSR_MAX_SLAVE

Yes, that's not nice at all. Will fix.
 
> Maybe it'd be useful to add a foreach_slave() helper.

I experimented a bit with this, but since the slaves are held in an array and
can be NULL, and since we aren't allowed to do for loop initial declarations,
it became a bit cumbersome. So I'll stick with the "manual" for loops.

 
>> +static struct node_entry *find_node_by_AddrA(struct list_head *node_db,
>> +					     const unsigned char addr[ETH_ALEN])
>> +{
>> +	struct node_entry *node;
>> +
>> +	list_for_each_entry_rcu(node, node_db, mac_list) {
>> +		if (!compare_ether_addr(node->MacAddressA, addr))
> 
> Please use ether_addr_equal instead for all these uses.
> compare_ether_addr should be removed one day.

Ok! Much nicer with ether_addr_equal().


>> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
>> +					     const unsigned char addr[ETH_ALEN])
> []
>> +		if (!compare_ether_addr(node->MacAddressB, addr))
> []
>> +struct node_entry *hsr_find_node(struct list_head *node_db, struct sk_buff *skb)
> []
>> +		if (!compare_ether_addr(node->MacAddressA, ethhdr->h_source))
>> +			return node;
>> +		if (!compare_ether_addr(node->MacAddressB, ethhdr->h_source))
>> +			return node;
> 
>> +struct node_entry *hsr_merge_node(struct hsr_priv *hsr_priv,
>> +				  struct node_entry *node,
>> +				  struct sk_buff *skb,
>> +				  enum hsr_dev_idx dev_idx)
> []
>> +	if (node && compare_ether_addr(node->MacAddressA, hsr_sp->MacAddressA)) {
> []
>> +	if (node && (dev_idx == node->AddrB_if) &&
>> +	    compare_ether_addr(node->MacAddressB, hsr_ethsup->ethhdr.h_source)) {
> []
>> +	if (node && (dev_idx != node->AddrB_if) &&
>> +	    (node->AddrB_if != HSR_DEV_NONE) &&
>> +	    compare_ether_addr(node->MacAddressA, hsr_ethsup->ethhdr.h_source)) {
> []
>> +	if (compare_ether_addr(hsr_sp->MacAddressA, hsr_ethsup->ethhdr.h_source))
> 
> []
> 
>> +/* above(a, b) - return 1 if a > b, 0 otherwise.
>> + */
>> +static bool above(u16 a, u16 b)
>> +{
>> +	/* Remove inconsistency where above(a, b) == below(a, b) */
>> +	if ((int) b - a == 32768)
>> +		return 0;
>> +
>> +	return (((s16) (b - a)) < 0);
>> +}
>> +#define below(a, b)		above((b), (a))
>> +#define above_or_eq(a, b)	(!below((a), (b)))
>> +#define below_or_eq(a, b)	(!above((a), (b)))
> 
> This looks odd. 

It relies on unsigned arithmetic to compare two values that may wrap. I.e.,
it doesn't care about the absolute sizes, but only about the distance 
between the numbers. 

It is inspired in part by the code in jiffies.h, but adapted to 16-bit
types. The code you suggested (below) will not work in this case.

See e.g. http://en.wikipedia.org/wiki/Serial_number_arithmetic


> 
> static bool above(u16 a, u16 b)
> {
> 	return a > b;
> }
> #define below(a, b) above(b, a)
> 
> static bool above_or_eq(u16 a, u16 b)
> {
> 	return a >= b;
> }
> #define below_or_eq(a, b) above_or_eq(b, a)
> 
> 
> 


-- 
Arvid Brodin | Consultant (Linux)
XDIN AB | Knarrarnäsgatan 7 | SE-164 40 Kista | Sweden | xdin.com