net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)

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.

+struct node_entry *hsr_find_node(struct list_head *node_db, struct sk_buff *skb)

Quoted 5 lines · Arvid Brodin

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)

net/hsr/hsr_device.c line 397

Quoted 12 lines

net/hsr/hsr_device.c new file +12 -0

+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

Maybe it'd be useful to add a foreach_slave() helper.

net/hsr/hsr_framereg.c line 51

Quoted 7 lines

net/hsr/hsr_framereg.c new file +7 -0

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

net/hsr/hsr_framereg.c line 62

Quoted 2 lines

net/hsr/hsr_framereg.c new file +2 -0

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

[]

net/hsr/hsr_framereg.c line 67

Quoted 1 line

net/hsr/hsr_framereg.c new file +1 -0

+		if (!compare_ether_addr(node->MacAddressB, addr))

[]

Quoted 1 line

+struct node_entry *hsr_find_node(struct list_head *node_db, struct sk_buff *skb)

[]

Review

Quoted 9 lines

(quoted diff) +8 -0

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

[]

net/hsr/hsr_framereg.c line 175

Quoted 1 line

net/hsr/hsr_framereg.c new file +1 -0

+	if (node && compare_ether_addr(node->MacAddressA, hsr_sp->MacAddressA)) {

[]

net/hsr/hsr_framereg.c line 183

Quoted 2 lines

net/hsr/hsr_framereg.c new file +2 -0

+	if (node && (dev_idx == node->AddrB_if) &&
+	    compare_ether_addr(node->MacAddressB, hsr_ethsup->ethhdr.h_source)) {

[]

net/hsr/hsr_framereg.c line 192

Quoted 3 lines

net/hsr/hsr_framereg.c new file +3 -0

+	if (node && (dev_idx != node->AddrB_if) &&
+	    (node->AddrB_if != HSR_DEV_NONE) &&
+	    compare_ether_addr(node->MacAddressA, hsr_ethsup->ethhdr.h_source)) {

[]

net/hsr/hsr_framereg.c line 218

Quoted 1 line

net/hsr/hsr_framereg.c new file +1 -0

+	if (compare_ether_addr(hsr_sp->MacAddressA, hsr_ethsup->ethhdr.h_source))

[]

net/hsr/hsr_framereg.c line 297

Quoted 13 lines

net/hsr/hsr_framereg.c new file +13 -0

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

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)

Quoted 1 line · Joe Perches

On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:

Quoted 7 lines

Quoted 7 lines

net/hsr/hsr_framereg.c new file +7 -0

+/* 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;

Quoted 8 lines

net/hsr/hsr_framereg.c line 297

Quoted 6 lines

net/hsr/hsr_framereg.c new file +6 -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

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

Quoted 2 lines · Arvid Brodin

Quoted 1 line · Joe Perches

On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:

[]

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

net/hsr/hsr_framereg.c new file +7 -0

+/* 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;

Quoted 8 lines

net/hsr/hsr_framereg.c line 297

Quoted 6 lines

net/hsr/hsr_framereg.c new file +6 -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.

Quoted 23 lines

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

Quoted 2 lines · Arvid Brodin

Quoted 1 line · Joe Perches

On Wed, 2013-10-23 at 18:09 +0200, Arvid Brodin wrote:

[]

Quoted 16 lines

Quoted 15 lines

net/hsr/hsr_framereg.c line 297

Quoted 13 lines

net/hsr/hsr_framereg.c new file +13 -0

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

Quoted 3 lines

Quoted 3 lines

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.

I'd have thought it wasn't really worth worrying about what happens when a ^ b == 0x8000. Anything using modulo 16 distances shouldn't really see such values.
Perhaps just document the effect.

Also, if these definitions are in a .h file they need better names

David