[net-next PATCH v5 0/4] net: hsr: Add support for HSR-SAN (RedBOX)

[net-next PATCH v5 0/4] net: hsr: Add support for HSR-SAN (RedBOX)

[Lukasz Majewski]

This patch set provides v5 of HSR-SAN (RedBOX) as well as hsr_redbox.sh
test script.

Applied on top of:
Branch: net-next/main
SHA1: 50aee97d1511

Runs inside: Buildroot (2024.02.1+):
SHA1: b31443e09cb7bb67b97ae6fb7614fe3a22889d50

Lukasz Majewski (4):
  net: hsr: Provide RedBox support (HSR-SAN)
  test: hsr: Move common code to hsr_common.sh file
  test: hsr: Extract version agnostic information from ping command
    output
  test: hsr: Add test for HSR RedBOX (HSR-SAN) mode of operation

 include/uapi/linux/if_link.h                  |  1 +
 net/hsr/hsr_device.c                          | 36 ++++++-
 net/hsr/hsr_device.h                          |  4 +-
 net/hsr/hsr_forward.c                         | 85 ++++++++++++++--
 net/hsr/hsr_framereg.c                        | 52 ++++++++++
 net/hsr/hsr_framereg.h                        |  6 ++
 net/hsr/hsr_main.h                            |  7 ++
 net/hsr/hsr_netlink.c                         | 30 +++++-
 net/hsr/hsr_slave.c                           |  1 +
 tools/testing/selftests/net/hsr/hsr_common.sh | 97 +++++++++++++++++++
 tools/testing/selftests/net/hsr/hsr_ping.sh   | 93 +-----------------
 tools/testing/selftests/net/hsr/hsr_redbox.sh | 97 +++++++++++++++++++
 12 files changed, 403 insertions(+), 106 deletions(-)
 create mode 100644 tools/testing/selftests/net/hsr/hsr_common.sh
 create mode 100755 tools/testing/selftests/net/hsr/hsr_redbox.sh

-- 
2.20.1

[net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

Introduce RedBox support (HSR-SAN to be more precise) for HSR networks.
Following traffic reduction optimizations have been implemented:
- Do not send HSR supervisory frames to Port C (interlink)
- Do not forward to HSR ring frames addressed to Port C
- Do not forward to Port C frames from HSR ring
- Do not send duplicate HSR frame to HSR ring when destination is Port C

The corresponding patch to modify iptable2 sources has already been sent:
https://lore.kernel.org/netdev/20240308145729.490863-1-lukma@denx.de/T/

Testing procedure:
------------------
The EVB-KSZ9477 has been used for testing on net-next branch
(SHA1: 5fc68320c1fb3c7d456ddcae0b4757326a043e6f).

Ports 4/5 were used for SW managed HSR (hsr1) as first hsr0 for ports 1/2
(with HW offloading for ksz9477) was created. Port 3 has been used as
interlink port (single USB-ETH dongle).

Configuration - RedBox (EVB-KSZ9477):
if link set lan1 down;ip link set lan2 down
ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink lan3 supervision 45 version 1
ip link set lan4 up;ip link set lan5 up
ip link set lan3 up
ip addr add 192.168.0.11/24 dev hsr1
ip link set hsr1 up

Configuration - DAN-H (EVB-KSZ9477):

ip link set lan1 down;ip link set lan2 down
ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 supervision 45 version 1
ip link set lan4 up;ip link set lan5 up
ip addr add 192.168.0.12/24 dev hsr1
ip link set hsr1 up

This approach uses only SW based HSR devices (hsr1).

--------------          -----------------       ------------
DAN-H  Port5 | <------> | Port5         |       |
       Port4 | <------> | Port4   Port3 | <---> | PC
             |          | (RedBox)      |       | (USB-ETH)
EVB-KSZ9477  |          | EVB-KSZ9477   |       |
--------------          -----------------       ------------

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---
Changes for v2:

- Add deleting of HSR_PT_INTERLINK node to hsr_del_ports()
- Rewrite handle_std_frame() to awoid code duplication
- Fix reverse christmas tree in hsr_prune_proxy_nodes() as well as
  remove stale node indication as interlink doesn't need this detection
  (only check if node is inactive for more than HSR_PROXY_NODE_FORGET_TIME
  is required)
- Rewrite commit message

Changes for v3:

- Modify frame passed Port C (Interlink) to have RedBox's source address (SA)
  This fixes issue with connecting L2 switch to Interlink Port as switches
  drop frames with SA other than one registered in their (internal) routing
  tables.

- Do not forward to port C (Interlink) frames from nodes A and B if DA is
  in NodeTable.

- Fix problem with ProxyNodeTable being pollued by nodes already registered
  in HSR ring.

- Prevent from sending frames to HSR ring when destination addresses (DA)
  are in ProxyNodeTable

Changes for v4:

- Replace memcpy() with dedicated ether_addr_copy() function

- Update commit message description to replace ifconfig with ip command
  usage

Changes for v5:

- Delete INTERLINK port before the main HSR one is deleted

- Delete timer responsible for triggering function to prune proxy nodes
---
 include/uapi/linux/if_link.h |  1 +
 net/hsr/hsr_device.c         | 36 ++++++++++++++-
 net/hsr/hsr_device.h         |  4 +-
 net/hsr/hsr_forward.c        | 85 ++++++++++++++++++++++++++++++++----
 net/hsr/hsr_framereg.c       | 52 ++++++++++++++++++++++
 net/hsr/hsr_framereg.h       |  6 +++
 net/hsr/hsr_main.h           |  7 +++
 net/hsr/hsr_netlink.c        | 30 +++++++++++--
 net/hsr/hsr_slave.c          |  1 +
 9 files changed, 206 insertions(+), 16 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index ffa637b38c93..e9f10860ec8e 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1771,6 +1771,7 @@ enum {
 	IFLA_HSR_PROTOCOL,		/* Indicate different protocol than
 					 * HSR. For example PRP.
 					 */
+	IFLA_HSR_INTERLINK,		/* HSR interlink network device */
 	__IFLA_HSR_MAX,
 };
 
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index e9d45133d641..cd1e7c6d2fc0 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -146,6 +146,9 @@ static int hsr_dev_open(struct net_device *dev)
 		case HSR_PT_SLAVE_B:
 			designation = "Slave B";
 			break;
+		case HSR_PT_INTERLINK:
+			designation = "Interlink";
+			break;
 		default:
 			designation = "Unknown";
 		}
@@ -285,6 +288,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
 	struct hsr_priv *hsr = master->hsr;
 	__u8 type = HSR_TLV_LIFE_CHECK;
 	struct hsr_sup_payload *hsr_sp;
+	struct hsr_sup_tlv *hsr_stlv;
 	struct hsr_sup_tag *hsr_stag;
 	struct sk_buff *skb;
 
@@ -324,6 +328,16 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
 	hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
 	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
 
+	if (hsr->redbox) {
+		hsr_stlv = skb_put(skb, sizeof(struct hsr_sup_tlv));
+		hsr_stlv->HSR_TLV_type = PRP_TLV_REDBOX_MAC;
+		hsr_stlv->HSR_TLV_length = sizeof(struct hsr_sup_payload);
+
+		/* Payload: MacAddressRedBox */
+		hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
+		ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
+	}
+
 	if (skb_put_padto(skb, ETH_ZLEN)) {
 		spin_unlock_bh(&hsr->seqnr_lock);
 		return;
@@ -405,6 +419,10 @@ void hsr_del_ports(struct hsr_priv *hsr)
 	if (port)
 		hsr_del_port(port);
 
+	port = hsr_port_get_hsr(hsr, HSR_PT_INTERLINK);
+	if (port)
+		hsr_del_port(port);
+
 	port = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
 	if (port)
 		hsr_del_port(port);
@@ -534,8 +552,8 @@ static const unsigned char def_multicast_addr[ETH_ALEN] __aligned(2) = {
 };
 
 int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
-		     unsigned char multicast_spec, u8 protocol_version,
-		     struct netlink_ext_ack *extack)
+		     struct net_device *interlink, unsigned char multicast_spec,
+		     u8 protocol_version, struct netlink_ext_ack *extack)
 {
 	bool unregister = false;
 	struct hsr_priv *hsr;
@@ -544,6 +562,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	hsr = netdev_priv(hsr_dev);
 	INIT_LIST_HEAD(&hsr->ports);
 	INIT_LIST_HEAD(&hsr->node_db);
+	INIT_LIST_HEAD(&hsr->proxy_node_db);
 	spin_lock_init(&hsr->list_lock);
 
 	eth_hw_addr_set(hsr_dev, slave[0]->dev_addr);
@@ -569,6 +588,7 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	/* Overflow soon to find bugs easier: */
 	hsr->sequence_nr = HSR_SEQNR_START;
 	hsr->sup_sequence_nr = HSR_SUP_SEQNR_START;
+	hsr->interlink_sequence_nr = HSR_SEQNR_START;
 
 	timer_setup(&hsr->announce_timer, hsr_announce, 0);
 	timer_setup(&hsr->prune_timer, hsr_prune_nodes, 0);
@@ -604,6 +624,18 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	if (res)
 		goto err_unregister;
 
+	if (interlink) {
+		res = hsr_add_port(hsr, interlink, HSR_PT_INTERLINK, extack);
+		if (res)
+			goto err_unregister;
+
+		hsr->redbox = true;
+		ether_addr_copy(hsr->macaddress_redbox, interlink->dev_addr);
+		timer_setup(&hsr->prune_proxy_timer, hsr_prune_proxy_nodes, 0);
+		mod_timer(&hsr->prune_proxy_timer,
+			  jiffies + msecs_to_jiffies(PRUNE_PROXY_PERIOD));
+	}
+
 	hsr_debugfs_init(hsr, hsr_dev);
 	mod_timer(&hsr->prune_timer, jiffies + msecs_to_jiffies(PRUNE_PERIOD));
 
diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
index 9060c92168f9..655284095b78 100644
--- a/net/hsr/hsr_device.h
+++ b/net/hsr/hsr_device.h
@@ -16,8 +16,8 @@
 void hsr_del_ports(struct hsr_priv *hsr);
 void hsr_dev_setup(struct net_device *dev);
 int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
-		     unsigned char multicast_spec, u8 protocol_version,
-		     struct netlink_ext_ack *extack);
+		     struct net_device *interlink, unsigned char multicast_spec,
+		     u8 protocol_version, struct netlink_ext_ack *extack);
 void hsr_check_carrier_and_operstate(struct hsr_priv *hsr);
 int hsr_get_max_mtu(struct hsr_priv *hsr);
 #endif /* __HSR_DEVICE_H */
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index 5d68cb181695..05a61b8286ec 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -377,6 +377,15 @@ static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port,
 		 */
 		ether_addr_copy(eth_hdr(skb)->h_source, port->dev->dev_addr);
 	}
+
+	/* When HSR node is used as RedBox - the frame received from HSR ring
+	 * requires source MAC address (SA) replacement to one which can be
+	 * recognized by SAN devices (otherwise, frames are dropped by switch)
+	 */
+	if (port->type == HSR_PT_INTERLINK)
+		ether_addr_copy(eth_hdr(skb)->h_source,
+				port->hsr->macaddress_redbox);
+
 	return dev_queue_xmit(skb);
 }
 
@@ -390,9 +399,57 @@ bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
 
 bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
 {
+	struct sk_buff *skb;
+
 	if (port->dev->features & NETIF_F_HW_HSR_FWD)
 		return prp_drop_frame(frame, port);
 
+	/* RedBox specific frames dropping policies
+	 *
+	 * Do not send HSR supervisory frames to SAN devices
+	 */
+	if (frame->is_supervision && port->type == HSR_PT_INTERLINK)
+		return true;
+
+	/* Do not forward to other HSR port (A or B) unicast frames which
+	 * are addressed to interlink port (and are in the ProxyNodeTable).
+	 */
+	skb = frame->skb_hsr;
+	if (skb && prp_drop_frame(frame, port) &&
+	    is_unicast_ether_addr(eth_hdr(skb)->h_dest) &&
+	    hsr_is_node_in_db(&port->hsr->proxy_node_db,
+			      eth_hdr(skb)->h_dest)) {
+		return true;
+	}
+
+	/* Do not forward to port C (Interlink) frames from nodes A and B
+	 * if DA is in NodeTable.
+	 */
+	if ((frame->port_rcv->type == HSR_PT_SLAVE_A ||
+	     frame->port_rcv->type == HSR_PT_SLAVE_B) &&
+	    port->type == HSR_PT_INTERLINK) {
+		skb = frame->skb_hsr;
+		if (skb && is_unicast_ether_addr(eth_hdr(skb)->h_dest) &&
+		    hsr_is_node_in_db(&port->hsr->node_db,
+				      eth_hdr(skb)->h_dest)) {
+			return true;
+		}
+	}
+
+	/* Do not forward to port A and B unicast frames received on the
+	 * interlink port if it is addressed to one of nodes registered in
+	 * the ProxyNodeTable.
+	 */
+	if ((port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B) &&
+	    frame->port_rcv->type == HSR_PT_INTERLINK) {
+		skb = frame->skb_std;
+		if (skb && is_unicast_ether_addr(eth_hdr(skb)->h_dest) &&
+		    hsr_is_node_in_db(&port->hsr->proxy_node_db,
+				      eth_hdr(skb)->h_dest)) {
+			return true;
+		}
+	}
+
 	return false;
 }
 
@@ -448,13 +505,14 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		}
 
 		/* Check if frame is to be dropped. Eg. for PRP no forward
-		 * between ports.
+		 * between ports, or sending HSR supervision to RedBox.
 		 */
 		if (hsr->proto_ops->drop_frame &&
 		    hsr->proto_ops->drop_frame(frame, port))
 			continue;
 
-		if (port->type != HSR_PT_MASTER)
+		if (port->type == HSR_PT_SLAVE_A ||
+		    port->type == HSR_PT_SLAVE_B)
 			skb = hsr->proto_ops->create_tagged_frame(frame, port);
 		else
 			skb = hsr->proto_ops->get_untagged_frame(frame, port);
@@ -469,7 +527,9 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 			hsr_deliver_master(skb, port->dev, frame->node_src);
 		} else {
 			if (!hsr_xmit(skb, port, frame))
-				sent = true;
+				if (port->type == HSR_PT_SLAVE_A ||
+				    port->type == HSR_PT_SLAVE_B)
+					sent = true;
 		}
 	}
 }
@@ -503,10 +563,12 @@ static void handle_std_frame(struct sk_buff *skb,
 	frame->skb_prp = NULL;
 	frame->skb_std = skb;
 
-	if (port->type != HSR_PT_MASTER) {
+	if (port->type != HSR_PT_MASTER)
 		frame->is_from_san = true;
-	} else {
-		/* Sequence nr for the master node */
+
+	if (port->type == HSR_PT_MASTER ||
+	    port->type == HSR_PT_INTERLINK) {
+		/* Sequence nr for the master/interlink node */
 		lockdep_assert_held(&hsr->seqnr_lock);
 		frame->sequence_nr = hsr->sequence_nr;
 		hsr->sequence_nr++;
@@ -564,6 +626,7 @@ static int fill_frame_info(struct hsr_frame_info *frame,
 {
 	struct hsr_priv *hsr = port->hsr;
 	struct hsr_vlan_ethhdr *vlan_hdr;
+	struct list_head *n_db;
 	struct ethhdr *ethhdr;
 	__be16 proto;
 	int ret;
@@ -574,9 +637,13 @@ static int fill_frame_info(struct hsr_frame_info *frame,
 
 	memset(frame, 0, sizeof(*frame));
 	frame->is_supervision = is_supervision_frame(port->hsr, skb);
-	frame->node_src = hsr_get_node(port, &hsr->node_db, skb,
-				       frame->is_supervision,
-				       port->type);
+
+	n_db = &hsr->node_db;
+	if (port->type == HSR_PT_INTERLINK)
+		n_db = &hsr->proxy_node_db;
+
+	frame->node_src = hsr_get_node(port, n_db, skb,
+				       frame->is_supervision, port->type);
 	if (!frame->node_src)
 		return -1; /* Unknown node and !is_supervision, or no mem */
 
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 26329db09210..614df9649794 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -71,6 +71,14 @@ static struct hsr_node *find_node_by_addr_A(struct list_head *node_db,
 	return NULL;
 }
 
+/* Check if node for a given MAC address is already present in data base
+ */
+bool hsr_is_node_in_db(struct list_head *node_db,
+		       const unsigned char addr[ETH_ALEN])
+{
+	return !!find_node_by_addr_A(node_db, addr);
+}
+
 /* Helper for device init; the self_node is used in hsr_rcv() to recognize
  * frames from self that's been looped over the HSR ring.
  */
@@ -223,6 +231,15 @@ struct hsr_node *hsr_get_node(struct hsr_port *port, struct list_head *node_db,
 		}
 	}
 
+	/* Check if required node is not in proxy nodes table */
+	list_for_each_entry_rcu(node, &hsr->proxy_node_db, mac_list) {
+		if (ether_addr_equal(node->macaddress_A, ethhdr->h_source)) {
+			if (hsr->proto_ops->update_san_info)
+				hsr->proto_ops->update_san_info(node, is_sup);
+			return node;
+		}
+	}
+
 	/* Everyone may create a node entry, connected node to a HSR/PRP
 	 * device.
 	 */
@@ -418,6 +435,10 @@ void hsr_addr_subst_dest(struct hsr_node *node_src, struct sk_buff *skb,
 
 	node_dst = find_node_by_addr_A(&port->hsr->node_db,
 				       eth_hdr(skb)->h_dest);
+	if (!node_dst && port->hsr->redbox)
+		node_dst = find_node_by_addr_A(&port->hsr->proxy_node_db,
+					       eth_hdr(skb)->h_dest);
+
 	if (!node_dst) {
 		if (port->hsr->prot_version != PRP_V1 && net_ratelimit())
 			netdev_err(skb->dev, "%s: Unknown node\n", __func__);
@@ -561,6 +582,37 @@ void hsr_prune_nodes(struct timer_list *t)
 		  jiffies + msecs_to_jiffies(PRUNE_PERIOD));
 }
 
+void hsr_prune_proxy_nodes(struct timer_list *t)
+{
+	struct hsr_priv *hsr = from_timer(hsr, t, prune_proxy_timer);
+	unsigned long timestamp;
+	struct hsr_node *node;
+	struct hsr_node *tmp;
+
+	spin_lock_bh(&hsr->list_lock);
+	list_for_each_entry_safe(node, tmp, &hsr->proxy_node_db, mac_list) {
+		timestamp = node->time_in[HSR_PT_INTERLINK];
+
+		/* Prune old entries */
+		if (time_is_before_jiffies(timestamp +
+				msecs_to_jiffies(HSR_PROXY_NODE_FORGET_TIME))) {
+			hsr_nl_nodedown(hsr, node->macaddress_A);
+			if (!node->removed) {
+				list_del_rcu(&node->mac_list);
+				node->removed = true;
+				/* Note that we need to free this entry later: */
+				kfree_rcu(node, rcu_head);
+			}
+		}
+	}
+
+	spin_unlock_bh(&hsr->list_lock);
+
+	/* Restart timer */
+	mod_timer(&hsr->prune_proxy_timer,
+		  jiffies + msecs_to_jiffies(PRUNE_PROXY_PERIOD));
+}
+
 void *hsr_get_next_node(struct hsr_priv *hsr, void *_pos,
 			unsigned char addr[ETH_ALEN])
 {
diff --git a/net/hsr/hsr_framereg.h b/net/hsr/hsr_framereg.h
index b23556251d62..67456a75d8fe 100644
--- a/net/hsr/hsr_framereg.h
+++ b/net/hsr/hsr_framereg.h
@@ -46,6 +46,7 @@ int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
 			   u16 sequence_nr);
 
 void hsr_prune_nodes(struct timer_list *t);
+void hsr_prune_proxy_nodes(struct timer_list *t);
 
 int hsr_create_self_node(struct hsr_priv *hsr,
 			 const unsigned char addr_a[ETH_ALEN],
@@ -63,10 +64,15 @@ int hsr_get_node_data(struct hsr_priv *hsr,
 		      int *if2_age,
 		      u16 *if2_seq);
 
+void hsr_handle_san_frame(bool san, enum hsr_port_type port,
+			  struct hsr_node *node);
 void prp_handle_san_frame(bool san, enum hsr_port_type port,
 			  struct hsr_node *node);
 void prp_update_san_info(struct hsr_node *node, bool is_sup);
 
+bool hsr_is_node_in_db(struct list_head *node_db,
+		       const unsigned char addr[ETH_ALEN]);
+
 struct hsr_node {
 	struct list_head	mac_list;
 	/* Protect R/W access to seq_out */
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index 18e01791ad79..23850b16d1ea 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -21,6 +21,7 @@
  */
 #define HSR_LIFE_CHECK_INTERVAL		 2000 /* ms */
 #define HSR_NODE_FORGET_TIME		60000 /* ms */
+#define HSR_PROXY_NODE_FORGET_TIME	60000 /* ms */
 #define HSR_ANNOUNCE_INTERVAL		  100 /* ms */
 #define HSR_ENTRY_FORGET_TIME		  400 /* ms */
 
@@ -35,6 +36,7 @@
  * HSR_NODE_FORGET_TIME?
  */
 #define PRUNE_PERIOD			 3000 /* ms */
+#define PRUNE_PROXY_PERIOD		 3000 /* ms */
 #define HSR_TLV_EOT				   0  /* End of TLVs */
 #define HSR_TLV_ANNOUNCE		   22
 #define HSR_TLV_LIFE_CHECK		   23
@@ -192,11 +194,14 @@ struct hsr_priv {
 	struct rcu_head		rcu_head;
 	struct list_head	ports;
 	struct list_head	node_db;	/* Known HSR nodes */
+	struct list_head	proxy_node_db;	/* RedBox HSR proxy nodes */
 	struct hsr_self_node	__rcu *self_node;	/* MACs of slaves */
 	struct timer_list	announce_timer;	/* Supervision frame dispatch */
 	struct timer_list	prune_timer;
+	struct timer_list	prune_proxy_timer;
 	int announce_count;
 	u16 sequence_nr;
+	u16 interlink_sequence_nr; /* Interlink port seq_nr */
 	u16 sup_sequence_nr;	/* For HSRv1 separate seq_nr for supervision */
 	enum hsr_version prot_version;	/* Indicate if HSRv0, HSRv1 or PRPv1 */
 	spinlock_t seqnr_lock;	/* locking for sequence_nr */
@@ -209,6 +214,8 @@ struct hsr_priv {
 				 * of lan_id
 				 */
 	bool fwd_offloaded;	/* Forwarding offloaded to HW */
+	bool redbox;            /* Device supports HSR RedBox */
+	unsigned char		macaddress_redbox[ETH_ALEN];
 	unsigned char		sup_multicast_addr[ETH_ALEN] __aligned(sizeof(u16));
 				/* Align to u16 boundary to avoid unaligned access
 				 * in ether_addr_equal
diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index 78fe40eb9f01..898f18c6da53 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -23,6 +23,7 @@ static const struct nla_policy hsr_policy[IFLA_HSR_MAX + 1] = {
 	[IFLA_HSR_SUPERVISION_ADDR]	= { .len = ETH_ALEN },
 	[IFLA_HSR_SEQ_NR]		= { .type = NLA_U16 },
 	[IFLA_HSR_PROTOCOL]		= { .type = NLA_U8 },
+	[IFLA_HSR_INTERLINK]		= { .type = NLA_U32 },
 };
 
 /* Here, it seems a netdevice has already been allocated for us, and the
@@ -35,8 +36,8 @@ static int hsr_newlink(struct net *src_net, struct net_device *dev,
 	enum hsr_version proto_version;
 	unsigned char multicast_spec;
 	u8 proto = HSR_PROTOCOL_HSR;
-	struct net_device *link[2];
 
+	struct net_device *link[2], *interlink = NULL;
 	if (!data) {
 		NL_SET_ERR_MSG_MOD(extack, "No slave devices specified");
 		return -EINVAL;
@@ -67,6 +68,20 @@ static int hsr_newlink(struct net *src_net, struct net_device *dev,
 		return -EINVAL;
 	}
 
+	if (data[IFLA_HSR_INTERLINK])
+		interlink = __dev_get_by_index(src_net,
+					       nla_get_u32(data[IFLA_HSR_INTERLINK]));
+
+	if (interlink && interlink == link[0]) {
+		NL_SET_ERR_MSG_MOD(extack, "Interlink and Slave1 are the same");
+		return -EINVAL;
+	}
+
+	if (interlink && interlink == link[1]) {
+		NL_SET_ERR_MSG_MOD(extack, "Interlink and Slave2 are the same");
+		return -EINVAL;
+	}
+
 	if (!data[IFLA_HSR_MULTICAST_SPEC])
 		multicast_spec = 0;
 	else
@@ -96,10 +111,17 @@ static int hsr_newlink(struct net *src_net, struct net_device *dev,
 		}
 	}
 
-	if (proto == HSR_PROTOCOL_PRP)
+	if (proto == HSR_PROTOCOL_PRP) {
 		proto_version = PRP_V1;
+		if (interlink) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Interlink only works with HSR");
+			return -EINVAL;
+		}
+	}
 
-	return hsr_dev_finalize(dev, link, multicast_spec, proto_version, extack);
+	return hsr_dev_finalize(dev, link, interlink, multicast_spec,
+				proto_version, extack);
 }
 
 static void hsr_dellink(struct net_device *dev, struct list_head *head)
@@ -107,6 +129,7 @@ static void hsr_dellink(struct net_device *dev, struct list_head *head)
 	struct hsr_priv *hsr = netdev_priv(dev);
 
 	del_timer_sync(&hsr->prune_timer);
+	del_timer_sync(&hsr->prune_proxy_timer);
 	del_timer_sync(&hsr->announce_timer);
 
 	hsr_debugfs_term(hsr);
@@ -114,6 +137,7 @@ static void hsr_dellink(struct net_device *dev, struct list_head *head)
 
 	hsr_del_self_node(hsr);
 	hsr_del_nodes(&hsr->node_db);
+	hsr_del_nodes(&hsr->proxy_node_db);
 
 	unregister_netdevice_queue(dev, head);
 }
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 1b6457f357bd..af6cf64a00e0 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -55,6 +55,7 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 	protocol = eth_hdr(skb)->h_proto;
 
 	if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
+	    port->type != HSR_PT_INTERLINK &&
 	    hsr->proto_ops->invalid_dan_ingress_frame &&
 	    hsr->proto_ops->invalid_dan_ingress_frame(protocol))
 		goto finish_pass;
-- 
2.20.1

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Casper Andersson]

Hi,

On 2024-04-15 14:49 +0200, Lukasz Majewski wrote:
> - Modify frame passed Port C (Interlink) to have RedBox's source address (SA)
>   This fixes issue with connecting L2 switch to Interlink Port as switches
>   drop frames with SA other than one registered in their (internal) routing
>   tables.

You never responded to my comment regarding this on v4. The same SA
should be able to go through a whole HSR and/or PRP network without
being replaced.
https://lore.kernel.org/netdev/20240404125159.721fbc19@wsk/T/#m9f18ec6a8de3f2608908bd181a786ea2c4fbc5e7

BR,
Casper

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4812 bytes --]

Hi Casper,

I've pasted the reply here, so we can discuss the newest patch set (v5).

> Hi,
> 
> On 2024-04-15 14:49 +0200, Lukasz Majewski wrote:
> > - Modify frame passed Port C (Interlink) to have RedBox's source
> > address (SA) This fixes issue with connecting L2 switch to
> > Interlink Port as switches drop frames with SA other than one
> > registered in their (internal) routing tables.  
> 
> You never responded to my comment regarding this on v4. The same SA
> should be able to go through a whole HSR and/or PRP network without
> being replaced.
> https://lore.kernel.org/netdev/20240404125159.721fbc19@wsk/T/#m9f18ec6a8de3f2608908bd181a786ea2c4fbc5e7
> 
> BR,
> Casper

> Out of curiosity, are you planning to implement the remaining RedBox
> modes too (PRP-SAN, HSR-HSR, HSR-PRP)?
> 

Currently I'm assigned to implement HSR-SAN.

> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:
> > Changes for v3:
> >
> > - Modify frame passed Port C (Interlink) to have RedBox's source
> > address (SA) This fixes issue with connecting L2 switch to
> > Interlink Port as switches drop frames with SA other than one
> > registered in their (internal) routing tables.  
> 
> > +	/* When HSR node is used as RedBox - the frame received
> > from HSR ring
> > +	 * requires source MAC address (SA) replacement to one
> > which can be
> > +	 * recognized by SAN devices (otherwise, frames are
> > dropped by switch)
> > +	 */
> > +	if (port->type == HSR_PT_INTERLINK)
> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> > +				port->hsr->macaddress_redbox);  
> 
> I'm not really understanding the reason for this change. Can you
> explain it in more detail?

According to the HSR standard [1] the RedBox device shall work as a
"proxy" [*] between HSR network and SAN (i.e. "normal" ethernet)
devices.

This particular snippet handles the situation when frame from HSR node
is supposed to be sent to SAN network. In that case the SA of HSR
(SA_A) is replaced with SA of RedBox (SA_RB) as the MAC address of
RedBox is known and used by SAN devices.


Node A  hsr1  |======| hsr1 Node Redbox |   |
(SA_A) [**]   |	     |           eth3   |---| ethX SAN
	      |      |        	 (SA_RB)|   |  (e.g switch)


(the ====== represents duplicate link - like lan1,lan2)

If the SA_A would be passed to SAN (e.g. switch) the switch could get
confused as also RedBox MAC address would be used. Hence, all the
frames going out from "Node Redbox" have SA set to SA_RB.

According to [1] - RedBox shall have the MAC address.
This is similar to problem from [2].

> The standard does not say to modify the
> SA. However, it also does not say to *not* modify it in HSR-SAN mode
> like it does in other places. In HSR-HSR and HSR-PRP mode modifying
> SA breaks the duplicate discard.

IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two types
(and not fully compatible) networks.

> So keeping the same behavior for all
> modes would be ideal.
> 
> I imagine any HW offloaded solutions will not modify the SA, so if
> possible the SW should also behave as such.

The HW offloading in most cases works with HSR-HSR setup (i.e. it
duplicates frames automatically or discards them when recived - like
ksz9477 [3]).

I think that RedBox HW offloading would be difficult to achieve to
comply with standard. One "rough" idea would be to configure
aforementioned ksz9477 to pass all frames in its HW between SAN and HSR
network (but then it wouldn't filter them).

> 
> BR,
> Casper

Notes:

[*] - However there is no specific "guidelines" on how the "proxy"
shall be implemented.

[**] - With current approach - the SAN MAC addresses are added to
"node table" of Node A. For Node RedBox those are stored in a separate
ProxyNodeTable. I'm not sure if this is the best possible approach
[***], as ideally only MAC addresses of HSR "network" nodes shall be
visible.

[***] - I think that this "improvement" could be addressed when HSR
support is added to Linux as it is the pre-requisite to add support for
it to iproute2. Afterwards, the code can be further refined (as it
would be added to net-next anyway).

[****] - As I'm now "on the topic" - I can share full setup for busybox
to run tests included to v5 of this patch set.


Links:

[1] - IEC 62439-3:2021

[2] -
https://elixir.bootlin.com/linux/latest/source/net/hsr/hsr_framereg.c#L397

[3] -
https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/microchip/ksz9477.c#L1341


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Casper Andersson]

Hi,

Sorry for the late reply, I was awaiting confirmation on what I can say
about the hardware I have access to. They won't let me say the name :(
but I can give some details.

On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:
>> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:
>> > Changes for v3:
>> >
>> > - Modify frame passed Port C (Interlink) to have RedBox's source
>> > address (SA) This fixes issue with connecting L2 switch to
>> > Interlink Port as switches drop frames with SA other than one
>> > registered in their (internal) routing tables.  
>> 
>> > +	/* When HSR node is used as RedBox - the frame received
>> > from HSR ring
>> > +	 * requires source MAC address (SA) replacement to one
>> > which can be
>> > +	 * recognized by SAN devices (otherwise, frames are
>> > dropped by switch)
>> > +	 */
>> > +	if (port->type == HSR_PT_INTERLINK)
>> > +		ether_addr_copy(eth_hdr(skb)->h_source,
>> > +				port->hsr->macaddress_redbox);  
>> 
>> I'm not really understanding the reason for this change. Can you
>> explain it in more detail?
>
> According to the HSR standard [1] the RedBox device shall work as a
> "proxy" [*] between HSR network and SAN (i.e. "normal" ethernet)
> devices.
>
> This particular snippet handles the situation when frame from HSR node
> is supposed to be sent to SAN network. In that case the SA of HSR
> (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC address of
> RedBox is known and used by SAN devices.
>
>
> Node A  hsr1  |======| hsr1 Node Redbox |   |
> (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> 	      |      |        	 (SA_RB)|   |  (e.g switch)
>
>
> (the ====== represents duplicate link - like lan1,lan2)
>
> If the SA_A would be passed to SAN (e.g. switch) the switch could get
> confused as also RedBox MAC address would be used. Hence, all the
> frames going out from "Node Redbox" have SA set to SA_RB.
>
> According to [1] - RedBox shall have the MAC address.
> This is similar to problem from [2].

Thanks for the explanation, but I still don't quite follow in what way
the SAN gets confused. "also RedBox MAC address would be used", when
does this happen? Do you mean that some frames from Node A end up using
the RedBox MAC address so it's best if they all do?

I see there is already some address replacement going on in the HSR
interface, as you pointed out in [2]. And I get your idea of being a
proxy. If no one else is opposed to this then I'm fine with it too.

>> The standard does not say to modify the
>> SA. However, it also does not say to *not* modify it in HSR-SAN mode
>> like it does in other places. In HSR-HSR and HSR-PRP mode modifying
>> SA breaks the duplicate discard.
>
> IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two types
> (and not fully compatible) networks.
>
>> So keeping the same behavior for all
>> modes would be ideal.
>> 
>> I imagine any HW offloaded solutions will not modify the SA, so if
>> possible the SW should also behave as such.
>
> The HW offloading in most cases works with HSR-HSR setup (i.e. it
> duplicates frames automatically or discards them when recived - like
> ksz9477 [3]).
>
> I think that RedBox HW offloading would be difficult to achieve to
> comply with standard. One "rough" idea would be to configure
> aforementioned ksz9477 to pass all frames in its HW between SAN and HSR
> network (but then it wouldn't filter them).

I don't know anything about ksz9477. The hardware I have access to is
supposed to be compliant with 2016 version in an offloaded situation for
all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR). Though, I haven't
verified if the operation is fully according to standard. It does not
modify any addresses in HW.

Does the interlink port also reach the drivers? Does it call
port_hsr_join and try to join as an HSR port? Do we maybe need a
separate path or setting for configuring the interlink in the different
modes (SAN, HSR, PRP interlink)?

> Notes:
>
> [*] - However there is no specific "guidelines" on how the "proxy"
> shall be implemented.
>
> [**] - With current approach - the SAN MAC addresses are added to
> "node table" of Node A. For Node RedBox those are stored in a separate
> ProxyNodeTable. I'm not sure if this is the best possible approach
> [***], as ideally only MAC addresses of HSR "network" nodes shall be
> visible.
>
> [***] - I think that this "improvement" could be addressed when HSR
> support is added to Linux as it is the pre-requisite to add support for
> it to iproute2. Afterwards, the code can be further refined (as it
> would be added to net-next anyway).
>
> [****] - As I'm now "on the topic" - I can share full setup for busybox
> to run tests included to v5 of this patch set.
>
>
> Links:
>
> [1] - IEC 62439-3:2021
>
> [2] -
> https://elixir.bootlin.com/linux/latest/source/net/hsr/hsr_framereg.c#L397
>
> [3] -
> https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/microchip/ksz9477.c#L1341

BR,
Casper

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 7333 bytes --]

Hi Casper,

> Hi,
> 
> Sorry for the late reply, I was awaiting confirmation on what I can
> say about the hardware I have access to. They won't let me say the
> name :( but I can give some details.

Ok, good :-)

At least I'm not alone and there is another person who can validate the
code (or behaviour) on another HSR HW.

(Some parts of the specification could be double checked on another HW
as well).

> 
> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:
> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:  
> >> > Changes for v3:
> >> >
> >> > - Modify frame passed Port C (Interlink) to have RedBox's source
> >> > address (SA) This fixes issue with connecting L2 switch to
> >> > Interlink Port as switches drop frames with SA other than one
> >> > registered in their (internal) routing tables.    
> >>   
> >> > +	/* When HSR node is used as RedBox - the frame received
> >> > from HSR ring
> >> > +	 * requires source MAC address (SA) replacement to one
> >> > which can be
> >> > +	 * recognized by SAN devices (otherwise, frames are
> >> > dropped by switch)
> >> > +	 */
> >> > +	if (port->type == HSR_PT_INTERLINK)
> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> >> > +				port->hsr->macaddress_redbox);
> >> >   
> >> 
> >> I'm not really understanding the reason for this change. Can you
> >> explain it in more detail?  
> >
> > According to the HSR standard [1] the RedBox device shall work as a
> > "proxy" [*] between HSR network and SAN (i.e. "normal" ethernet)
> > devices.
> >
> > This particular snippet handles the situation when frame from HSR
> > node is supposed to be sent to SAN network. In that case the SA of
> > HSR (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC address
> > of RedBox is known and used by SAN devices.
> >
> >
> > Node A  hsr1  |======| hsr1 Node Redbox |   |
> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> > 	      |      |        	 (SA_RB)|   |  (e.g switch)
> >
> >
> > (the ====== represents duplicate link - like lan1,lan2)
> >
> > If the SA_A would be passed to SAN (e.g. switch) the switch could
> > get confused as also RedBox MAC address would be used. Hence, all
> > the frames going out from "Node Redbox" have SA set to SA_RB.
> >
> > According to [1] - RedBox shall have the MAC address.
> > This is similar to problem from [2].  
> 
> Thanks for the explanation, but I still don't quite follow in what way
> the SAN gets confused. "also RedBox MAC address would be used", when
> does this happen? Do you mean that some frames from Node A end up
> using the RedBox MAC address so it's best if they all do?

The SAN (let's say it is a switch) can communicate with RedBox or Node
A. In that way the DA is different for both (so SA on reply is also
different). On my setup I've observed frames drop (caused probably by
switch filtering of incoming traffic not matching the outgoing one).

When I only use SA of RedBox on traffic going to SAN, the problem is
gone.

IMHO, such separation (i.e. to use only RedBox's SA on traffic going to
SAN) is the "proxy" mentioned in the standard.

> 
> I see there is already some address replacement going on in the HSR
> interface, as you pointed out in [2]. And I get your idea of being a
> proxy. If no one else is opposed to this then I'm fine with it too.
> 

Ok.

> >> The standard does not say to modify the
> >> SA. However, it also does not say to *not* modify it in HSR-SAN
> >> mode like it does in other places. In HSR-HSR and HSR-PRP mode
> >> modifying SA breaks the duplicate discard.  
> >
> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two
> > types (and not fully compatible) networks.
> >  
> >> So keeping the same behavior for all
> >> modes would be ideal.
> >> 
> >> I imagine any HW offloaded solutions will not modify the SA, so if
> >> possible the SW should also behave as such.  
> >
> > The HW offloading in most cases works with HSR-HSR setup (i.e. it
> > duplicates frames automatically or discards them when recived - like
> > ksz9477 [3]).
> >
> > I think that RedBox HW offloading would be difficult to achieve to
> > comply with standard. One "rough" idea would be to configure
> > aforementioned ksz9477 to pass all frames in its HW between SAN and
> > HSR network (but then it wouldn't filter them).  
> 
> I don't know anything about ksz9477. The hardware I have access to is
> supposed to be compliant with 2016 version in an offloaded situation
> for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).

Hmm... Interesting.

As fair as I know - the ksz9477 driver from Microchip for RedBox sets
internal (i.e. in chip) vlan for Node_A, Node_B and Interlink, so _all_
packets are flowing back and forth between HSR and SAN networks ....

> Though, I haven't
> verified if the operation is fully according to standard.

You may use wireshark on device connected as SAN to redbox and then see
if there are any frames (especially supervisory ones) passed from HSR
network.

> It does not
> modify any addresses in HW.

By address - you mean the MAC addresses of nodes?

> 
> Does the interlink port also reach the drivers?

Could you be more specific in your question?

> Does it call
> port_hsr_join and try to join as an HSR port? 

No, not yet.

The community (IIRC Vladimir Oltean) suggested to first implement the
RedBox Interlink (HSR-SAN) in SW. Then, we may think about adding
offloading support for it.

> Do we maybe need a
> separate path or setting for configuring the interlink in the
> different modes (SAN, HSR, PRP interlink)?

I think that it shall be handled as an extra parameter (like we do have
now with 'supervision' or 'version') in ip link add.

However, first I would like to have the "interlink" parameter added to
iproute2 and then we can extend it to other modes if requred.

> 
> > Notes:
> >
> > [*] - However there is no specific "guidelines" on how the "proxy"
> > shall be implemented.
> >
> > [**] - With current approach - the SAN MAC addresses are added to
> > "node table" of Node A. For Node RedBox those are stored in a
> > separate ProxyNodeTable. I'm not sure if this is the best possible
> > approach [***], as ideally only MAC addresses of HSR "network"
> > nodes shall be visible.
> >
> > [***] - I think that this "improvement" could be addressed when HSR
> > support is added to Linux as it is the pre-requisite to add support
> > for it to iproute2. Afterwards, the code can be further refined (as
> > it would be added to net-next anyway).
> >
> > [****] - As I'm now "on the topic" - I can share full setup for
> > busybox to run tests included to v5 of this patch set.
> >
> >
> > Links:
> >
> > [1] - IEC 62439-3:2021
> >
> > [2] -
> > https://elixir.bootlin.com/linux/latest/source/net/hsr/hsr_framereg.c#L397
> >
> > [3] -
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/microchip/ksz9477.c#L1341
> >  
> 
> BR,
> Casper




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Casper Andersson]

On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
Hi Lukasz,

> Hi Casper,
>
>> Hi,
>> 
>> Sorry for the late reply, I was awaiting confirmation on what I can
>> say about the hardware I have access to. They won't let me say the
>> name :( but I can give some details.
>
> Ok, good :-)
>
> At least I'm not alone and there is another person who can validate the
> code (or behaviour) on another HSR HW.
>
> (Some parts of the specification could be double checked on another HW
> as well).
>
>> 
>> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:
>> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:  
>> >> > Changes for v3:
>> >> >
>> >> > - Modify frame passed Port C (Interlink) to have RedBox's source
>> >> > address (SA) This fixes issue with connecting L2 switch to
>> >> > Interlink Port as switches drop frames with SA other than one
>> >> > registered in their (internal) routing tables.    
>> >>   
>> >> > +	/* When HSR node is used as RedBox - the frame received
>> >> > from HSR ring
>> >> > +	 * requires source MAC address (SA) replacement to one
>> >> > which can be
>> >> > +	 * recognized by SAN devices (otherwise, frames are
>> >> > dropped by switch)
>> >> > +	 */
>> >> > +	if (port->type == HSR_PT_INTERLINK)
>> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
>> >> > +				port->hsr->macaddress_redbox);
>> >> >   
>> >> 
>> >> I'm not really understanding the reason for this change. Can you
>> >> explain it in more detail?  
>> >
>> > According to the HSR standard [1] the RedBox device shall work as a
>> > "proxy" [*] between HSR network and SAN (i.e. "normal" ethernet)
>> > devices.
>> >
>> > This particular snippet handles the situation when frame from HSR
>> > node is supposed to be sent to SAN network. In that case the SA of
>> > HSR (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC address
>> > of RedBox is known and used by SAN devices.
>> >
>> >
>> > Node A  hsr1  |======| hsr1 Node Redbox |   |
>> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
>> > 	      |      |        	 (SA_RB)|   |  (e.g switch)
>> >
>> >
>> > (the ====== represents duplicate link - like lan1,lan2)
>> >
>> > If the SA_A would be passed to SAN (e.g. switch) the switch could
>> > get confused as also RedBox MAC address would be used. Hence, all
>> > the frames going out from "Node Redbox" have SA set to SA_RB.
>> >
>> > According to [1] - RedBox shall have the MAC address.
>> > This is similar to problem from [2].  
>> 
>> Thanks for the explanation, but I still don't quite follow in what way
>> the SAN gets confused. "also RedBox MAC address would be used", when
>> does this happen? Do you mean that some frames from Node A end up
>> using the RedBox MAC address so it's best if they all do?
>
> The SAN (let's say it is a switch) can communicate with RedBox or Node
> A. In that way the DA is different for both (so SA on reply is also
> different). On my setup I've observed frames drop (caused probably by
> switch filtering of incoming traffic not matching the outgoing one).
>
> When I only use SA of RedBox on traffic going to SAN, the problem is
> gone.
>
> IMHO, such separation (i.e. to use only RedBox's SA on traffic going to
> SAN) is the "proxy" mentioned in the standard.
>
>> 
>> I see there is already some address replacement going on in the HSR
>> interface, as you pointed out in [2]. And I get your idea of being a
>> proxy. If no one else is opposed to this then I'm fine with it too.
>> 
>
> Ok.
>
>> >> The standard does not say to modify the
>> >> SA. However, it also does not say to *not* modify it in HSR-SAN
>> >> mode like it does in other places. In HSR-HSR and HSR-PRP mode
>> >> modifying SA breaks the duplicate discard.  
>> >
>> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two
>> > types (and not fully compatible) networks.
>> >  
>> >> So keeping the same behavior for all
>> >> modes would be ideal.
>> >> 
>> >> I imagine any HW offloaded solutions will not modify the SA, so if
>> >> possible the SW should also behave as such.  
>> >
>> > The HW offloading in most cases works with HSR-HSR setup (i.e. it
>> > duplicates frames automatically or discards them when recived - like
>> > ksz9477 [3]).
>> >
>> > I think that RedBox HW offloading would be difficult to achieve to
>> > comply with standard. One "rough" idea would be to configure
>> > aforementioned ksz9477 to pass all frames in its HW between SAN and
>> > HSR network (but then it wouldn't filter them).  
>> 
>> I don't know anything about ksz9477. The hardware I have access to is
>> supposed to be compliant with 2016 version in an offloaded situation
>> for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).
>
> Hmm... Interesting.
>
> As fair as I know - the ksz9477 driver from Microchip for RedBox sets
> internal (i.e. in chip) vlan for Node_A, Node_B and Interlink, so _all_
> packets are flowing back and forth between HSR and SAN networks ....
>
>> Though, I haven't
>> verified if the operation is fully according to standard.
>
> You may use wireshark on device connected as SAN to redbox and then see
> if there are any frames (especially supervisory ones) passed from HSR
> network.

I realized I should clarify, what I'm running is non-upstream
software. And by offloaded I mean the redbox forwarding is
offloaded. Supervision frames are still handled in SW and only sent on
HSR/PRP ports, and doesn't reach any SAN nodes. Basic operation works as
it should.

>> It does not
>> modify any addresses in HW.
>
> By address - you mean the MAC addresses of nodes?

I mean that it forwards all frames without modification (except HSR/PRP
and VLAN tags). It does not update SMAC with the proxy MAC like your
implementation does.

>> Does the interlink port also reach the drivers?
>
> Could you be more specific in your question?

Sorry, it was connected to the question below if it sets anything up in
the drivers for the interlink port. And you answered it.

>> Does it call
>> port_hsr_join and try to join as an HSR port? 
>
> No, not yet.
>
> The community (IIRC Vladimir Oltean) suggested to first implement the
> RedBox Interlink (HSR-SAN) in SW. Then, we may think about adding
> offloading support for it.
>
>> Do we maybe need a
>> separate path or setting for configuring the interlink in the
>> different modes (SAN, HSR, PRP interlink)?
>
> I think that it shall be handled as an extra parameter (like we do have
> now with 'supervision' or 'version') in ip link add.
>
> However, first I would like to have the "interlink" parameter added to
> iproute2 and then we can extend it to other modes if requred.

Alright, doing SW implementation first sounds good. From userspace it
can probably be an extra parameter. But for the driver configuration
maybe we want a port_interlink_join? (when it comes to implementing that).


I did some testing with veth interfaces (everything in SW) with your
patches. I tried to do a setup like yours
                
                  +-vethA---vethB-+
                  |               |
vethF---vethE---hsr0             hsr1
                  |               |
                  +-vethC---vethD-+

Sending traffic from vethF results in 3 copies being seen on the ring
ports. One of which ends up being forwarded back to vethF (with SMAC
updated to the proxy address). I assume this is not intended behavior.


Setup:
ip link add dev vethA type veth peer name vethB
ip link add dev vethC type veth peer name vethD
ip link add dev vethE type veth peer name vethF
ip link set up dev vethA
ip link set up dev vethB
ip link set up dev vethC
ip link set up dev vethD
ip link set up dev vethE
ip link set up dev vethF

ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink vethE supervision 45 version 1
ip link add name hsr1 type hsr slave1 vethB slave2 vethD supervision 45 version 1
ip link set dev hsr0 up
ip link set dev hsr1 up

I used Nemesis to send random UDP broadcast packets but you could use whatever:
nemesis udp -d vethF -c 10000 -i 1 

BR,
Casper

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 9786 bytes --]

Hi Casper,

> On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
> Hi Lukasz,
> 
> > Hi Casper,
> >  
> >> Hi,
> >> 
> >> Sorry for the late reply, I was awaiting confirmation on what I can
> >> say about the hardware I have access to. They won't let me say the
> >> name :( but I can give some details.  
> >
> > Ok, good :-)
> >
> > At least I'm not alone and there is another person who can validate
> > the code (or behaviour) on another HSR HW.
> >
> > (Some parts of the specification could be double checked on another
> > HW as well).
> >  
> >> 
> >> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:  
> >> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:    
> >> >> > Changes for v3:
> >> >> >
> >> >> > - Modify frame passed Port C (Interlink) to have RedBox's
> >> >> > source address (SA) This fixes issue with connecting L2
> >> >> > switch to Interlink Port as switches drop frames with SA
> >> >> > other than one registered in their (internal) routing tables.
> >> >> >      
> >> >>     
> >> >> > +	/* When HSR node is used as RedBox - the frame
> >> >> > received from HSR ring
> >> >> > +	 * requires source MAC address (SA) replacement to
> >> >> > one which can be
> >> >> > +	 * recognized by SAN devices (otherwise, frames are
> >> >> > dropped by switch)
> >> >> > +	 */
> >> >> > +	if (port->type == HSR_PT_INTERLINK)
> >> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> >> >> > +
> >> >> > port->hsr->macaddress_redbox); 
> >> >> 
> >> >> I'm not really understanding the reason for this change. Can you
> >> >> explain it in more detail?    
> >> >
> >> > According to the HSR standard [1] the RedBox device shall work
> >> > as a "proxy" [*] between HSR network and SAN (i.e. "normal"
> >> > ethernet) devices.
> >> >
> >> > This particular snippet handles the situation when frame from HSR
> >> > node is supposed to be sent to SAN network. In that case the SA
> >> > of HSR (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC
> >> > address of RedBox is known and used by SAN devices.
> >> >
> >> >
> >> > Node A  hsr1  |======| hsr1 Node Redbox |   |
> >> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> >> > 	      |      |        	 (SA_RB)|   |  (e.g switch)
> >> >
> >> >
> >> > (the ====== represents duplicate link - like lan1,lan2)
> >> >
> >> > If the SA_A would be passed to SAN (e.g. switch) the switch could
> >> > get confused as also RedBox MAC address would be used. Hence, all
> >> > the frames going out from "Node Redbox" have SA set to SA_RB.
> >> >
> >> > According to [1] - RedBox shall have the MAC address.
> >> > This is similar to problem from [2].    
> >> 
> >> Thanks for the explanation, but I still don't quite follow in what
> >> way the SAN gets confused. "also RedBox MAC address would be
> >> used", when does this happen? Do you mean that some frames from
> >> Node A end up using the RedBox MAC address so it's best if they
> >> all do?  
> >
> > The SAN (let's say it is a switch) can communicate with RedBox or
> > Node A. In that way the DA is different for both (so SA on reply is
> > also different). On my setup I've observed frames drop (caused
> > probably by switch filtering of incoming traffic not matching the
> > outgoing one).
> >
> > When I only use SA of RedBox on traffic going to SAN, the problem is
> > gone.
> >
> > IMHO, such separation (i.e. to use only RedBox's SA on traffic
> > going to SAN) is the "proxy" mentioned in the standard.
> >  
> >> 
> >> I see there is already some address replacement going on in the HSR
> >> interface, as you pointed out in [2]. And I get your idea of being
> >> a proxy. If no one else is opposed to this then I'm fine with it
> >> too. 
> >
> > Ok.
> >  
> >> >> The standard does not say to modify the
> >> >> SA. However, it also does not say to *not* modify it in HSR-SAN
> >> >> mode like it does in other places. In HSR-HSR and HSR-PRP mode
> >> >> modifying SA breaks the duplicate discard.    
> >> >
> >> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two
> >> > types (and not fully compatible) networks.
> >> >    
> >> >> So keeping the same behavior for all
> >> >> modes would be ideal.
> >> >> 
> >> >> I imagine any HW offloaded solutions will not modify the SA, so
> >> >> if possible the SW should also behave as such.    
> >> >
> >> > The HW offloading in most cases works with HSR-HSR setup (i.e. it
> >> > duplicates frames automatically or discards them when recived -
> >> > like ksz9477 [3]).
> >> >
> >> > I think that RedBox HW offloading would be difficult to achieve
> >> > to comply with standard. One "rough" idea would be to configure
> >> > aforementioned ksz9477 to pass all frames in its HW between SAN
> >> > and HSR network (but then it wouldn't filter them).    
> >> 
> >> I don't know anything about ksz9477. The hardware I have access to
> >> is supposed to be compliant with 2016 version in an offloaded
> >> situation for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).  
> >
> > Hmm... Interesting.
> >
> > As fair as I know - the ksz9477 driver from Microchip for RedBox
> > sets internal (i.e. in chip) vlan for Node_A, Node_B and Interlink,
> > so _all_ packets are flowing back and forth between HSR and SAN
> > networks .... 
> >> Though, I haven't
> >> verified if the operation is fully according to standard.  
> >
> > You may use wireshark on device connected as SAN to redbox and then
> > see if there are any frames (especially supervisory ones) passed
> > from HSR network.  
> 
> I realized I should clarify, what I'm running is non-upstream
> software.

Ok.

> And by offloaded I mean the redbox forwarding is
> offloaded. Supervision frames are still handled in SW and only sent on
> HSR/PRP ports, and doesn't reach any SAN nodes. Basic operation works
> as it should.

Ok.

> 
> >> It does not
> >> modify any addresses in HW.  
> >
> > By address - you mean the MAC addresses of nodes?  
> 
> I mean that it forwards all frames without modification (except
> HSR/PRP and VLAN tags). It does not update SMAC with the proxy MAC
> like your implementation does.

Hmm... I'm wondering how "proxy" is implemented then.
Also, what is the purpose of ProxyNodeTable in that case?

> 
> >> Does the interlink port also reach the drivers?  
> >
> > Could you be more specific in your question?  
> 
> Sorry, it was connected to the question below if it sets anything up
> in the drivers for the interlink port. And you answered it.

Ok.

> 
> >> Does it call
> >> port_hsr_join and try to join as an HSR port?   
> >
> > No, not yet.
> >
> > The community (IIRC Vladimir Oltean) suggested to first implement
> > the RedBox Interlink (HSR-SAN) in SW. Then, we may think about
> > adding offloading support for it.
> >  
> >> Do we maybe need a
> >> separate path or setting for configuring the interlink in the
> >> different modes (SAN, HSR, PRP interlink)?  
> >
> > I think that it shall be handled as an extra parameter (like we do
> > have now with 'supervision' or 'version') in ip link add.
> >
> > However, first I would like to have the "interlink" parameter added
> > to iproute2 and then we can extend it to other modes if requred.  
> 
> Alright, doing SW implementation first sounds good. From userspace it
> can probably be an extra parameter. But for the driver configuration
> maybe we want a port_interlink_join? (when it comes to implementing
> that).

IMHO, having port_interlink_join() may be useful in the future to
provide offloading support.

> 
> 
> I did some testing with veth interfaces (everything in SW) with your
> patches. I tried to do a setup like yours
>                 
>                   +-vethA---vethB-+
>                   |               |
> vethF---vethE---hsr0             hsr1
>                   |               |
>                   +-vethC---vethD-+
> 
> Sending traffic from vethF results in 3 copies being seen on the ring
> ports. One of which ends up being forwarded back to vethF (with SMAC
> updated to the proxy address). I assume this is not intended behavior.

I've reported this [2] (i.e. duplicated packets on HSR network with
veth) when I was checking hsr_ping.sh [1] script for regression.

(However, I don't see the DUP pings on my KSZ9477 setup).

> 
> 
> Setup:
> ip link add dev vethA type veth peer name vethB
> ip link add dev vethC type veth peer name vethD
> ip link add dev vethE type veth peer name vethF
> ip link set up dev vethA
> ip link set up dev vethB
> ip link set up dev vethC
> ip link set up dev vethD
> ip link set up dev vethE
> ip link set up dev vethF
> 
> ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink
> vethE supervision 45 version 1 ip link add name hsr1 type hsr slave1
> vethB slave2 vethD supervision 45 version 1 ip link set dev hsr0 up
> ip link set dev hsr1 up
> 
> I used Nemesis to send random UDP broadcast packets but you could use
> whatever: nemesis udp -d vethF -c 10000 -i 1 

Ok, I will check nemesis load as well.

Can you check the hsr_redbox.sh (from this patch set) and hsr_ping.sh ?

> 
> BR,
> Casper

Links:

[1] -
https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/net/hsr/hsr_ping.sh

[2] -
https://lore.kernel.org/linux-kernel/20240418125336.7305d545@wsk/T/#m9c54a1a31366e4d1caec8fceb4329c5dbe9cc9aa


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Casper Andersson]

On 2024-04-19 12:42 +0200, Lukasz Majewski wrote:
> Hi Casper,
>
>> On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
>> Hi Lukasz,
>> 
>> > Hi Casper,
>> >  
>> >> Hi,
>> >> 
>> >> Sorry for the late reply, I was awaiting confirmation on what I can
>> >> say about the hardware I have access to. They won't let me say the
>> >> name :( but I can give some details.  
>> >
>> > Ok, good :-)
>> >
>> > At least I'm not alone and there is another person who can validate
>> > the code (or behaviour) on another HSR HW.
>> >
>> > (Some parts of the specification could be double checked on another
>> > HW as well).
>> >  
>> >> 
>> >> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:  
>> >> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:    
>> >> >> > Changes for v3:
>> >> >> >
>> >> >> > - Modify frame passed Port C (Interlink) to have RedBox's
>> >> >> > source address (SA) This fixes issue with connecting L2
>> >> >> > switch to Interlink Port as switches drop frames with SA
>> >> >> > other than one registered in their (internal) routing tables.
>> >> >> >      
>> >> >>     
>> >> >> > +	/* When HSR node is used as RedBox - the frame
>> >> >> > received from HSR ring
>> >> >> > +	 * requires source MAC address (SA) replacement to
>> >> >> > one which can be
>> >> >> > +	 * recognized by SAN devices (otherwise, frames are
>> >> >> > dropped by switch)
>> >> >> > +	 */
>> >> >> > +	if (port->type == HSR_PT_INTERLINK)
>> >> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
>> >> >> > +
>> >> >> > port->hsr->macaddress_redbox); 
>> >> >> 
>> >> >> I'm not really understanding the reason for this change. Can you
>> >> >> explain it in more detail?    
>> >> >
>> >> > According to the HSR standard [1] the RedBox device shall work
>> >> > as a "proxy" [*] between HSR network and SAN (i.e. "normal"
>> >> > ethernet) devices.
>> >> >
>> >> > This particular snippet handles the situation when frame from HSR
>> >> > node is supposed to be sent to SAN network. In that case the SA
>> >> > of HSR (SA_A) is replaced with SA of RedBox (SA_RB) as the MAC
>> >> > address of RedBox is known and used by SAN devices.
>> >> >
>> >> >
>> >> > Node A  hsr1  |======| hsr1 Node Redbox |   |
>> >> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
>> >> > 	      |      |        	 (SA_RB)|   |  (e.g switch)
>> >> >
>> >> >
>> >> > (the ====== represents duplicate link - like lan1,lan2)
>> >> >
>> >> > If the SA_A would be passed to SAN (e.g. switch) the switch could
>> >> > get confused as also RedBox MAC address would be used. Hence, all
>> >> > the frames going out from "Node Redbox" have SA set to SA_RB.
>> >> >
>> >> > According to [1] - RedBox shall have the MAC address.
>> >> > This is similar to problem from [2].    
>> >> 
>> >> Thanks for the explanation, but I still don't quite follow in what
>> >> way the SAN gets confused. "also RedBox MAC address would be
>> >> used", when does this happen? Do you mean that some frames from
>> >> Node A end up using the RedBox MAC address so it's best if they
>> >> all do?  
>> >
>> > The SAN (let's say it is a switch) can communicate with RedBox or
>> > Node A. In that way the DA is different for both (so SA on reply is
>> > also different). On my setup I've observed frames drop (caused
>> > probably by switch filtering of incoming traffic not matching the
>> > outgoing one).
>> >
>> > When I only use SA of RedBox on traffic going to SAN, the problem is
>> > gone.
>> >
>> > IMHO, such separation (i.e. to use only RedBox's SA on traffic
>> > going to SAN) is the "proxy" mentioned in the standard.
>> >  
>> >> 
>> >> I see there is already some address replacement going on in the HSR
>> >> interface, as you pointed out in [2]. And I get your idea of being
>> >> a proxy. If no one else is opposed to this then I'm fine with it
>> >> too. 
>> >
>> > Ok.
>> >  
>> >> >> The standard does not say to modify the
>> >> >> SA. However, it also does not say to *not* modify it in HSR-SAN
>> >> >> mode like it does in other places. In HSR-HSR and HSR-PRP mode
>> >> >> modifying SA breaks the duplicate discard.    
>> >> >
>> >> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between two
>> >> > types (and not fully compatible) networks.
>> >> >    
>> >> >> So keeping the same behavior for all
>> >> >> modes would be ideal.
>> >> >> 
>> >> >> I imagine any HW offloaded solutions will not modify the SA, so
>> >> >> if possible the SW should also behave as such.    
>> >> >
>> >> > The HW offloading in most cases works with HSR-HSR setup (i.e. it
>> >> > duplicates frames automatically or discards them when recived -
>> >> > like ksz9477 [3]).
>> >> >
>> >> > I think that RedBox HW offloading would be difficult to achieve
>> >> > to comply with standard. One "rough" idea would be to configure
>> >> > aforementioned ksz9477 to pass all frames in its HW between SAN
>> >> > and HSR network (but then it wouldn't filter them).    
>> >> 
>> >> I don't know anything about ksz9477. The hardware I have access to
>> >> is supposed to be compliant with 2016 version in an offloaded
>> >> situation for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).  
>> >
>> > Hmm... Interesting.
>> >
>> > As fair as I know - the ksz9477 driver from Microchip for RedBox
>> > sets internal (i.e. in chip) vlan for Node_A, Node_B and Interlink,
>> > so _all_ packets are flowing back and forth between HSR and SAN
>> > networks .... 
>> >> Though, I haven't
>> >> verified if the operation is fully according to standard.  
>> >
>> > You may use wireshark on device connected as SAN to redbox and then
>> > see if there are any frames (especially supervisory ones) passed
>> > from HSR network.  
>> 
>> I realized I should clarify, what I'm running is non-upstream
>> software.
>
> Ok.
>
>> And by offloaded I mean the redbox forwarding is
>> offloaded. Supervision frames are still handled in SW and only sent on
>> HSR/PRP ports, and doesn't reach any SAN nodes. Basic operation works
>> as it should.
>
> Ok.
>
>> 
>> >> It does not
>> >> modify any addresses in HW.  
>> >
>> > By address - you mean the MAC addresses of nodes?  
>> 
>> I mean that it forwards all frames without modification (except
>> HSR/PRP and VLAN tags). It does not update SMAC with the proxy MAC
>> like your implementation does.
>
> Hmm... I'm wondering how "proxy" is implemented then.
> Also, what is the purpose of ProxyNodeTable in that case?

The ProxyNodeTable becomes the same as the MAC table for the interlink
port. I.e. normal MAC learning, when a frame is sent by a SAN and
received on interlink the HW learns that that SMAC is on the interlink
port (until it ages out). This table can be read out and used for
supervision frames.

Though, the NodesTable I don't think is used in HW. As I understand it's
an optional feature.

>> 
>> >> Does it call
>> >> port_hsr_join and try to join as an HSR port?   
>> >
>> > No, not yet.
>> >
>> > The community (IIRC Vladimir Oltean) suggested to first implement
>> > the RedBox Interlink (HSR-SAN) in SW. Then, we may think about
>> > adding offloading support for it.
>> >  
>> >> Do we maybe need a
>> >> separate path or setting for configuring the interlink in the
>> >> different modes (SAN, HSR, PRP interlink)?  
>> >
>> > I think that it shall be handled as an extra parameter (like we do
>> > have now with 'supervision' or 'version') in ip link add.
>> >
>> > However, first I would like to have the "interlink" parameter added
>> > to iproute2 and then we can extend it to other modes if requred.  
>> 
>> Alright, doing SW implementation first sounds good. From userspace it
>> can probably be an extra parameter. But for the driver configuration
>> maybe we want a port_interlink_join? (when it comes to implementing
>> that).
>
> IMHO, having port_interlink_join() may be useful in the future to
> provide offloading support.
>
>> 
>> 
>> I did some testing with veth interfaces (everything in SW) with your
>> patches. I tried to do a setup like yours
>>                 
>>                   +-vethA---vethB-+
>>                   |               |
>> vethF---vethE---hsr0             hsr1
>>                   |               |
>>                   +-vethC---vethD-+
>> 
>> Sending traffic from vethF results in 3 copies being seen on the ring
>> ports. One of which ends up being forwarded back to vethF (with SMAC
>> updated to the proxy address). I assume this is not intended behavior.
>
> I've reported this [2] (i.e. duplicated packets on HSR network with
> veth) when I was checking hsr_ping.sh [1] script for regression.
>
> (However, I don't see the DUP pings on my KSZ9477 setup).
>
> 
>> 
>> Setup:
>> ip link add dev vethA type veth peer name vethB
>> ip link add dev vethC type veth peer name vethD
>> ip link add dev vethE type veth peer name vethF
>> ip link set up dev vethA
>> ip link set up dev vethB
>> ip link set up dev vethC
>> ip link set up dev vethD
>> ip link set up dev vethE
>> ip link set up dev vethF
>> 
>> ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink
>> vethE supervision 45 version 1 ip link add name hsr1 type hsr slave1
>> vethB slave2 vethD supervision 45 version 1 ip link set dev hsr0 up
>> ip link set dev hsr1 up
>> 
>> I used Nemesis to send random UDP broadcast packets but you could use
>> whatever: nemesis udp -d vethF -c 10000 -i 1 
>
> Ok, I will check nemesis load as well.

Nemesis doesn't do anything specific, just generates packets. The
command above sends a packet at 1 second intervals.

> Can you check the hsr_redbox.sh (from this patch set) and hsr_ping.sh ?

Running in SW I get the same results as you, hsr_redbox.sh passes and
hsr_ping.sh fails.

I haven't tried on HW. I'll see if I can find some time for it but it
might take more time to prepare.

BR,
Casper

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 11055 bytes --]

Hi Casper,

> On 2024-04-19 12:42 +0200, Lukasz Majewski wrote:
> > Hi Casper,
> >  
> >> On 2024-04-18 17:37 +0200, Lukasz Majewski wrote:
> >> Hi Lukasz,
> >>   
> >> > Hi Casper,
> >> >    
> >> >> Hi,
> >> >> 
> >> >> Sorry for the late reply, I was awaiting confirmation on what I
> >> >> can say about the hardware I have access to. They won't let me
> >> >> say the name :( but I can give some details.    
> >> >
> >> > Ok, good :-)
> >> >
> >> > At least I'm not alone and there is another person who can
> >> > validate the code (or behaviour) on another HSR HW.
> >> >
> >> > (Some parts of the specification could be double checked on
> >> > another HW as well).
> >> >    
> >> >> 
> >> >> On 2024-04-16 15:03 +0200, Lukasz Majewski wrote:    
> >> >> >> On 2024-04-02 10:58 +0200, Lukasz Majewski wrote:      
> >> >> >> > Changes for v3:
> >> >> >> >
> >> >> >> > - Modify frame passed Port C (Interlink) to have RedBox's
> >> >> >> > source address (SA) This fixes issue with connecting L2
> >> >> >> > switch to Interlink Port as switches drop frames with SA
> >> >> >> > other than one registered in their (internal) routing
> >> >> >> > tables. 
> >> >> >>       
> >> >> >> > +	/* When HSR node is used as RedBox - the frame
> >> >> >> > received from HSR ring
> >> >> >> > +	 * requires source MAC address (SA) replacement to
> >> >> >> > one which can be
> >> >> >> > +	 * recognized by SAN devices (otherwise, frames
> >> >> >> > are dropped by switch)
> >> >> >> > +	 */
> >> >> >> > +	if (port->type == HSR_PT_INTERLINK)
> >> >> >> > +		ether_addr_copy(eth_hdr(skb)->h_source,
> >> >> >> > +
> >> >> >> > port->hsr->macaddress_redbox);   
> >> >> >> 
> >> >> >> I'm not really understanding the reason for this change. Can
> >> >> >> you explain it in more detail?      
> >> >> >
> >> >> > According to the HSR standard [1] the RedBox device shall work
> >> >> > as a "proxy" [*] between HSR network and SAN (i.e. "normal"
> >> >> > ethernet) devices.
> >> >> >
> >> >> > This particular snippet handles the situation when frame from
> >> >> > HSR node is supposed to be sent to SAN network. In that case
> >> >> > the SA of HSR (SA_A) is replaced with SA of RedBox (SA_RB) as
> >> >> > the MAC address of RedBox is known and used by SAN devices.
> >> >> >
> >> >> >
> >> >> > Node A  hsr1  |======| hsr1 Node Redbox |   |
> >> >> > (SA_A) [**]   |	     |           eth3   |---| ethX SAN
> >> >> > 	      |      |        	 (SA_RB)|   |  (e.g
> >> >> > switch)
> >> >> >
> >> >> >
> >> >> > (the ====== represents duplicate link - like lan1,lan2)
> >> >> >
> >> >> > If the SA_A would be passed to SAN (e.g. switch) the switch
> >> >> > could get confused as also RedBox MAC address would be used.
> >> >> > Hence, all the frames going out from "Node Redbox" have SA
> >> >> > set to SA_RB.
> >> >> >
> >> >> > According to [1] - RedBox shall have the MAC address.
> >> >> > This is similar to problem from [2].      
> >> >> 
> >> >> Thanks for the explanation, but I still don't quite follow in
> >> >> what way the SAN gets confused. "also RedBox MAC address would
> >> >> be used", when does this happen? Do you mean that some frames
> >> >> from Node A end up using the RedBox MAC address so it's best if
> >> >> they all do?    
> >> >
> >> > The SAN (let's say it is a switch) can communicate with RedBox or
> >> > Node A. In that way the DA is different for both (so SA on reply
> >> > is also different). On my setup I've observed frames drop (caused
> >> > probably by switch filtering of incoming traffic not matching the
> >> > outgoing one).
> >> >
> >> > When I only use SA of RedBox on traffic going to SAN, the
> >> > problem is gone.
> >> >
> >> > IMHO, such separation (i.e. to use only RedBox's SA on traffic
> >> > going to SAN) is the "proxy" mentioned in the standard.
> >> >    
> >> >> 
> >> >> I see there is already some address replacement going on in the
> >> >> HSR interface, as you pointed out in [2]. And I get your idea
> >> >> of being a proxy. If no one else is opposed to this then I'm
> >> >> fine with it too.   
> >> >
> >> > Ok.
> >> >    
> >> >> >> The standard does not say to modify the
> >> >> >> SA. However, it also does not say to *not* modify it in
> >> >> >> HSR-SAN mode like it does in other places. In HSR-HSR and
> >> >> >> HSR-PRP mode modifying SA breaks the duplicate discard.      
> >> >> >
> >> >> > IMHO, the HSR-SAN shall be regarded as a "proxy" [*] between
> >> >> > two types (and not fully compatible) networks.
> >> >> >      
> >> >> >> So keeping the same behavior for all
> >> >> >> modes would be ideal.
> >> >> >> 
> >> >> >> I imagine any HW offloaded solutions will not modify the SA,
> >> >> >> so if possible the SW should also behave as such.      
> >> >> >
> >> >> > The HW offloading in most cases works with HSR-HSR setup
> >> >> > (i.e. it duplicates frames automatically or discards them
> >> >> > when recived - like ksz9477 [3]).
> >> >> >
> >> >> > I think that RedBox HW offloading would be difficult to
> >> >> > achieve to comply with standard. One "rough" idea would be to
> >> >> > configure aforementioned ksz9477 to pass all frames in its HW
> >> >> > between SAN and HSR network (but then it wouldn't filter
> >> >> > them).      
> >> >> 
> >> >> I don't know anything about ksz9477. The hardware I have access
> >> >> to is supposed to be compliant with 2016 version in an offloaded
> >> >> situation for all modes (HSR-SAN, PRP-SAN, HSR-PRP, HSR-HSR).
> >> >>  
> >> >
> >> > Hmm... Interesting.
> >> >
> >> > As fair as I know - the ksz9477 driver from Microchip for RedBox
> >> > sets internal (i.e. in chip) vlan for Node_A, Node_B and
> >> > Interlink, so _all_ packets are flowing back and forth between
> >> > HSR and SAN networks ....   
> >> >> Though, I haven't
> >> >> verified if the operation is fully according to standard.    
> >> >
> >> > You may use wireshark on device connected as SAN to redbox and
> >> > then see if there are any frames (especially supervisory ones)
> >> > passed from HSR network.    
> >> 
> >> I realized I should clarify, what I'm running is non-upstream
> >> software.  
> >
> > Ok.
> >  
> >> And by offloaded I mean the redbox forwarding is
> >> offloaded. Supervision frames are still handled in SW and only
> >> sent on HSR/PRP ports, and doesn't reach any SAN nodes. Basic
> >> operation works as it should.  
> >
> > Ok.
> >  
> >>   
> >> >> It does not
> >> >> modify any addresses in HW.    
> >> >
> >> > By address - you mean the MAC addresses of nodes?    
> >> 
> >> I mean that it forwards all frames without modification (except
> >> HSR/PRP and VLAN tags). It does not update SMAC with the proxy MAC
> >> like your implementation does.  
> >
> > Hmm... I'm wondering how "proxy" is implemented then.
> > Also, what is the purpose of ProxyNodeTable in that case?  
> 
> The ProxyNodeTable becomes the same as the MAC table for the interlink
> port. I.e. normal MAC learning, when a frame is sent by a SAN and
> received on interlink the HW learns that that SMAC is on the interlink
> port (until it ages out). This table can be read out and used for
> supervision frames.

Yes, this is how this patch handles it.

> 
> Though, the NodesTable I don't think is used in HW. As I understand
> it's an optional feature.

Yes.

> 
> >>   
> >> >> Does it call
> >> >> port_hsr_join and try to join as an HSR port?     
> >> >
> >> > No, not yet.
> >> >
> >> > The community (IIRC Vladimir Oltean) suggested to first implement
> >> > the RedBox Interlink (HSR-SAN) in SW. Then, we may think about
> >> > adding offloading support for it.
> >> >    
> >> >> Do we maybe need a
> >> >> separate path or setting for configuring the interlink in the
> >> >> different modes (SAN, HSR, PRP interlink)?    
> >> >
> >> > I think that it shall be handled as an extra parameter (like we
> >> > do have now with 'supervision' or 'version') in ip link add.
> >> >
> >> > However, first I would like to have the "interlink" parameter
> >> > added to iproute2 and then we can extend it to other modes if
> >> > requred.    
> >> 
> >> Alright, doing SW implementation first sounds good. From userspace
> >> it can probably be an extra parameter. But for the driver
> >> configuration maybe we want a port_interlink_join? (when it comes
> >> to implementing that).  
> >
> > IMHO, having port_interlink_join() may be useful in the future to
> > provide offloading support.
> >  
> >> 
> >> 
> >> I did some testing with veth interfaces (everything in SW) with
> >> your patches. I tried to do a setup like yours
> >>                 
> >>                   +-vethA---vethB-+
> >>                   |               |
> >> vethF---vethE---hsr0             hsr1
> >>                   |               |
> >>                   +-vethC---vethD-+
> >> 
> >> Sending traffic from vethF results in 3 copies being seen on the
> >> ring ports. One of which ends up being forwarded back to vethF
> >> (with SMAC updated to the proxy address). I assume this is not
> >> intended behavior.  
> >
> > I've reported this [2] (i.e. duplicated packets on HSR network with
> > veth) when I was checking hsr_ping.sh [1] script for regression.
> >
> > (However, I don't see the DUP pings on my KSZ9477 setup).
> >
> >   
> >> 
> >> Setup:
> >> ip link add dev vethA type veth peer name vethB
> >> ip link add dev vethC type veth peer name vethD
> >> ip link add dev vethE type veth peer name vethF
> >> ip link set up dev vethA
> >> ip link set up dev vethB
> >> ip link set up dev vethC
> >> ip link set up dev vethD
> >> ip link set up dev vethE
> >> ip link set up dev vethF
> >> 
> >> ip link add name hsr0 type hsr slave1 vethA slave2 vethC interlink
> >> vethE supervision 45 version 1 ip link add name hsr1 type hsr
> >> slave1 vethB slave2 vethD supervision 45 version 1 ip link set dev
> >> hsr0 up ip link set dev hsr1 up
> >> 
> >> I used Nemesis to send random UDP broadcast packets but you could
> >> use whatever: nemesis udp -d vethF -c 10000 -i 1   
> >
> > Ok, I will check nemesis load as well.  
> 
> Nemesis doesn't do anything specific, just generates packets. The
> command above sends a packet at 1 second intervals.
> 
> > Can you check the hsr_redbox.sh (from this patch set) and
> > hsr_ping.sh ?  
> 
> Running in SW I get the same results as you, hsr_redbox.sh passes and
> hsr_ping.sh fails.

Ok.

> 
> I haven't tried on HW. I'll see if I can find some time for it but it
> might take more time to prepare.

Ok. Thanks for help.

> 
> BR,
> Casper


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi Casper,

> >
> > Hmm... I'm wondering how "proxy" is implemented then.
> > Also, what is the purpose of ProxyNodeTable in that case?  
> 
> The ProxyNodeTable becomes the same as the MAC table for the interlink
> port. I.e. normal MAC learning, when a frame is sent by a SAN and
> received on interlink the HW learns that that SMAC is on the interlink
> port (until it ages out). 

+1

> This table can be read out and used for
> supervision frames.
> 

I've go through the standard again and it looks like it is mandatory
for RedBox to send supervisory frames with MAC addresses from
ProxyNodeTable as its payload. Moreover, the RedBox MAC address shall
be also send as the second (i.e. following) payload in this frame.

The current RedBox (from net-next) needs to be extended to support it
 - I've started working on this.

> Though, the NodesTable I don't think is used in HW. As I understand
> it's an optional feature.

+1


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Paolo Abeni]

On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> Introduce RedBox support (HSR-SAN to be more precise) for HSR networks.
> Following traffic reduction optimizations have been implemented:
> - Do not send HSR supervisory frames to Port C (interlink)
> - Do not forward to HSR ring frames addressed to Port C
> - Do not forward to Port C frames from HSR ring
> - Do not send duplicate HSR frame to HSR ring when destination is Port C
> 
> The corresponding patch to modify iptable2 sources has already been sent:
> https://lore.kernel.org/netdev/20240308145729.490863-1-lukma@denx.de/T/
> 
> Testing procedure:
> ------------------
> The EVB-KSZ9477 has been used for testing on net-next branch
> (SHA1: 5fc68320c1fb3c7d456ddcae0b4757326a043e6f).
> 
> Ports 4/5 were used for SW managed HSR (hsr1) as first hsr0 for ports 1/2
> (with HW offloading for ksz9477) was created. Port 3 has been used as
> interlink port (single USB-ETH dongle).
> 
> Configuration - RedBox (EVB-KSZ9477):
> if link set lan1 down;ip link set lan2 down
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
> ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 interlink lan3 supervision 45 version 1
> ip link set lan4 up;ip link set lan5 up
> ip link set lan3 up
> ip addr add 192.168.0.11/24 dev hsr1
> ip link set hsr1 up
> 
> Configuration - DAN-H (EVB-KSZ9477):
> 
> ip link set lan1 down;ip link set lan2 down
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
> ip link add name hsr1 type hsr slave1 lan4 slave2 lan5 supervision 45 version 1
> ip link set lan4 up;ip link set lan5 up
> ip addr add 192.168.0.12/24 dev hsr1
> ip link set hsr1 up
> 
> This approach uses only SW based HSR devices (hsr1).
> 
> --------------          -----------------       ------------
> DAN-H  Port5 | <------> | Port5         |       |
>        Port4 | <------> | Port4   Port3 | <---> | PC
>              |          | (RedBox)      |       | (USB-ETH)
> EVB-KSZ9477  |          | EVB-KSZ9477   |       |
> --------------          -----------------       ------------

The above description is obsoleted by follow-up tests, right?

Thanks,

Paolo

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2818 bytes --]

Hi Paolo,

> On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> > Introduce RedBox support (HSR-SAN to be more precise) for HSR
> > networks. Following traffic reduction optimizations have been
> > implemented:
> > - Do not send HSR supervisory frames to Port C (interlink)
> > - Do not forward to HSR ring frames addressed to Port C
> > - Do not forward to Port C frames from HSR ring
> > - Do not send duplicate HSR frame to HSR ring when destination is
> > Port C
> > 
> > The corresponding patch to modify iptable2 sources has already been
> > sent:
> > https://lore.kernel.org/netdev/20240308145729.490863-1-lukma@denx.de/T/
> > 
> > Testing procedure:
> > ------------------
> > The EVB-KSZ9477 has been used for testing on net-next branch
> > (SHA1: 5fc68320c1fb3c7d456ddcae0b4757326a043e6f).
> > 
> > Ports 4/5 were used for SW managed HSR (hsr1) as first hsr0 for
> > ports 1/2 (with HW offloading for ksz9477) was created. Port 3 has
> > been used as interlink port (single USB-ETH dongle).
> > 
> > Configuration - RedBox (EVB-KSZ9477):
> > if link set lan1 down;ip link set lan2 down
> > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision
> > 45 version 1 ip link add name hsr1 type hsr slave1 lan4 slave2 lan5
> > interlink lan3 supervision 45 version 1 ip link set lan4 up;ip link
> > set lan5 up ip link set lan3 up
> > ip addr add 192.168.0.11/24 dev hsr1
> > ip link set hsr1 up
> > 
> > Configuration - DAN-H (EVB-KSZ9477):
> > 
> > ip link set lan1 down;ip link set lan2 down
> > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision
> > 45 version 1 ip link add name hsr1 type hsr slave1 lan4 slave2 lan5
> > supervision 45 version 1 ip link set lan4 up;ip link set lan5 up
> > ip addr add 192.168.0.12/24 dev hsr1
> > ip link set hsr1 up
> > 
> > This approach uses only SW based HSR devices (hsr1).
> > 
> > --------------          -----------------       ------------
> > DAN-H  Port5 | <------> | Port5         |       |
> >        Port4 | <------> | Port4   Port3 | <---> | PC
> >              |          | (RedBox)      |       | (USB-ETH)
> > EVB-KSZ9477  |          | EVB-KSZ9477   |       |
> > --------------          -----------------       ------------  
> 
> The above description is obsoleted by follow-up tests, right?
> 

No, it is still valid if one would like to use/test this code on two
KSZ9477-EVB boards.

However, I can add also information referring to hsr_redbox.sh tests as
well.

> Thanks,
> 
> Paolo
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 1/4] net: hsr: Provide RedBox support (HSR-SAN)

[Casper Andersson]

Hi,

On 2024-04-15 14:49 +0200, Lukasz Majewski wrote:
> +void hsr_handle_san_frame(bool san, enum hsr_port_type port,
> +			  struct hsr_node *node);

Function declared here but never defined or used.

[net-next PATCH v5 2/4] test: hsr: Move common code to hsr_common.sh file

[Lukasz Majewski]

Some of the code already present in the hsr_ping.sh test program can be
moved to a separate script file, so it can be reused by other HSR
functionality (like HSR-SAN) tests.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 tools/testing/selftests/net/hsr/hsr_common.sh | 96 +++++++++++++++++++
 tools/testing/selftests/net/hsr/hsr_ping.sh   | 93 +-----------------
 2 files changed, 99 insertions(+), 90 deletions(-)
 create mode 100644 tools/testing/selftests/net/hsr/hsr_common.sh

diff --git a/tools/testing/selftests/net/hsr/hsr_common.sh b/tools/testing/selftests/net/hsr/hsr_common.sh
new file mode 100644
index 000000000000..822165391573
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_common.sh
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: GPL-2.0
+# Common code for HSR testing scripts
+
+ret=0
+ksft_skip=4
+
+sec=$(date +%s)
+rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
+ns1="ns1-$rndh"
+ns2="ns2-$rndh"
+ns3="ns3-$rndh"
+
+cleanup()
+{
+	local netns
+	for netns in "$ns1" "$ns2" "$ns3" ;do
+		ip netns del $netns
+	done
+}
+
+# $1: IP address
+is_v6()
+{
+	[ -z "${1##*:*}" ]
+}
+
+do_ping()
+{
+	local netns="$1"
+	local connect_addr="$2"
+	local ping_args="-q -c 2"
+
+	if is_v6 "${connect_addr}"; then
+		$ipv6 || return 0
+		ping_args="${ping_args} -6"
+	fi
+
+	ip netns exec ${netns} ping ${ping_args} $connect_addr >/dev/null
+	if [ $? -ne 0 ] ; then
+		echo "$netns -> $connect_addr connectivity [ FAIL ]" 1>&2
+		ret=1
+		return 1
+	fi
+
+	return 0
+}
+
+do_ping_long()
+{
+	local netns="$1"
+	local connect_addr="$2"
+	local ping_args="-q -c 10"
+
+	if is_v6 "${connect_addr}"; then
+		$ipv6 || return 0
+		ping_args="${ping_args} -6"
+	fi
+
+	OUT="$(LANG=C ip netns exec ${netns} ping ${ping_args} $connect_addr | grep received)"
+	if [ $? -ne 0 ] ; then
+		echo "$netns -> $connect_addr ping [ FAIL ]" 1>&2
+		ret=1
+		return 1
+	fi
+
+	VAL="$(echo $OUT | cut -d' ' -f1-8)"
+	if [ "$VAL" != "10 packets transmitted, 10 received, 0% packet loss," ]
+	then
+		echo "$netns -> $connect_addr ping TEST [ FAIL ]"
+		echo "Expect to send and receive 10 packets and no duplicates."
+		echo "Full message: ${OUT}."
+		ret=1
+		return 1
+	fi
+
+	return 0
+}
+
+stop_if_error()
+{
+	local msg="$1"
+
+	if [ ${ret} -ne 0 ]; then
+		echo "FAIL: ${msg}" 1>&2
+		exit ${ret}
+	fi
+}
+
+check_prerequisites()
+{
+	ip -Version > /dev/null 2>&1
+	if [ $? -ne 0 ];then
+		echo "SKIP: Could not run test without ip tool"
+		exit $ksft_skip
+	fi
+}
diff --git a/tools/testing/selftests/net/hsr/hsr_ping.sh b/tools/testing/selftests/net/hsr/hsr_ping.sh
index 1c6457e54625..6fab532793a0 100755
--- a/tools/testing/selftests/net/hsr/hsr_ping.sh
+++ b/tools/testing/selftests/net/hsr/hsr_ping.sh
@@ -1,10 +1,10 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ret=0
-ksft_skip=4
 ipv6=true
 
+source ./hsr_common.sh
+
 optstring="h4"
 usage() {
 	echo "Usage: $0 [OPTION]"
@@ -27,88 +27,6 @@ while getopts "$optstring" option;do
 esac
 done
 
-sec=$(date +%s)
-rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
-ns1="ns1-$rndh"
-ns2="ns2-$rndh"
-ns3="ns3-$rndh"
-
-cleanup()
-{
-	local netns
-	for netns in "$ns1" "$ns2" "$ns3" ;do
-		ip netns del $netns
-	done
-}
-
-# $1: IP address
-is_v6()
-{
-	[ -z "${1##*:*}" ]
-}
-
-do_ping()
-{
-	local netns="$1"
-	local connect_addr="$2"
-	local ping_args="-q -c 2"
-
-	if is_v6 "${connect_addr}"; then
-		$ipv6 || return 0
-		ping_args="${ping_args} -6"
-	fi
-
-	ip netns exec ${netns} ping ${ping_args} $connect_addr >/dev/null
-	if [ $? -ne 0 ] ; then
-		echo "$netns -> $connect_addr connectivity [ FAIL ]" 1>&2
-		ret=1
-		return 1
-	fi
-
-	return 0
-}
-
-do_ping_long()
-{
-	local netns="$1"
-	local connect_addr="$2"
-	local ping_args="-q -c 10"
-
-	if is_v6 "${connect_addr}"; then
-		$ipv6 || return 0
-		ping_args="${ping_args} -6"
-	fi
-
-	OUT="$(LANG=C ip netns exec ${netns} ping ${ping_args} $connect_addr | grep received)"
-	if [ $? -ne 0 ] ; then
-		echo "$netns -> $connect_addr ping [ FAIL ]" 1>&2
-		ret=1
-		return 1
-	fi
-
-	VAL="$(echo $OUT | cut -d' ' -f1-8)"
-	if [ "$VAL" != "10 packets transmitted, 10 received, 0% packet loss," ]
-	then
-		echo "$netns -> $connect_addr ping TEST [ FAIL ]"
-		echo "Expect to send and receive 10 packets and no duplicates."
-		echo "Full message: ${OUT}."
-		ret=1
-		return 1
-	fi
-
-	return 0
-}
-
-stop_if_error()
-{
-	local msg="$1"
-
-	if [ ${ret} -ne 0 ]; then
-		echo "FAIL: ${msg}" 1>&2
-		exit ${ret}
-	fi
-}
-
 do_complete_ping_test()
 {
 	echo "INFO: Initial validation ping."
@@ -248,12 +166,7 @@ setup_hsr_interfaces()
 	ip -net "$ns3" link set hsr3 up
 }
 
-ip -Version > /dev/null 2>&1
-if [ $? -ne 0 ];then
-	echo "SKIP: Could not run test without ip tool"
-	exit $ksft_skip
-fi
-
+check_prerequisites
 trap cleanup EXIT
 
 for i in "$ns1" "$ns2" "$ns3" ;do
-- 
2.20.1

Re: [net-next PATCH v5 2/4] test: hsr: Move common code to hsr_common.sh file

[Paolo Abeni]

On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> Some of the code already present in the hsr_ping.sh test program can be
> moved to a separate script file, so it can be reused by other HSR
> functionality (like HSR-SAN) tests.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  tools/testing/selftests/net/hsr/hsr_common.sh | 96 +++++++++++++++++++
>  tools/testing/selftests/net/hsr/hsr_ping.sh   | 93 +-----------------
>  2 files changed, 99 insertions(+), 90 deletions(-)
>  create mode 100644 tools/testing/selftests/net/hsr/hsr_common.sh
> 
> diff --git a/tools/testing/selftests/net/hsr/hsr_common.sh b/tools/testing/selftests/net/hsr/hsr_common.sh
> new file mode 100644
> index 000000000000..822165391573
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_common.sh
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Common code for HSR testing scripts
> +
> +ret=0
> +ksft_skip=4
> +
> +sec=$(date +%s)
> +rndh=$(printf %x $sec)-$(mktemp -u XXXXXX)
> +ns1="ns1-$rndh"
> +ns2="ns2-$rndh"
> +ns3="ns3-$rndh"

Since you are touching the initialization code, please move it to using
the lib.sh helpers. The above will become:

setup_ns ns1 ns2 ns3

> +
> +cleanup()
> +{
> +	local netns
> +	for netns in "$ns1" "$ns2" "$ns3" ;do
> +		ip netns del $netns
> +	done
> +}

And this:

	cleanup_all_ns


Thanks,

Paolo

Re: [net-next PATCH v5 2/4] test: hsr: Move common code to hsr_common.sh file

[Paolo Abeni]

On Thu, 2024-04-18 at 10:41 +0200, Paolo Abeni wrote:
> On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> > Some of the code already present in the hsr_ping.sh test program can be
> > moved to a separate script file, so it can be reused by other HSR
> > functionality (like HSR-SAN) tests.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >  tools/testing/selftests/net/hsr/hsr_common.sh | 96 +++++++++++++++++++
> >  tools/testing/selftests/net/hsr/hsr_ping.sh   | 93 +-----------------
> >  2 files changed, 99 insertions(+), 90 deletions(-)
> >  create mode 100644 tools/testing/selftests/net/hsr/hsr_common.sh
> > 
> > diff --git a/tools/testing/selftests/net/hsr/hsr_common.sh b/tools/testing/selftests/net/hsr/hsr_common.sh
> > new file mode 100644
> > index 000000000000..822165391573
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/hsr/hsr_common.sh

Whoops, I almost forgot: you need to add hsr_common.sh in the
TEST_FILES list in the net self-tests Makefile

Thanks,

Paolo

[net-next PATCH v5 3/4] test: hsr: Extract version agnostic information from ping command output

[Lukasz Majewski]

Current code checks if ping command output match hardcoded pattern:
"10 packets transmitted, 10 received, 0% packet loss,".

Such approach will work only from one ping program version (for which
this test has been originally written).
This patch address problem when ping with different summary output
like "10 packets transmitted, 10 packets received, 0% packet" is
used to run this test - for example one from busybox (as the test
system runs in QEMU with rootfs created with buildroot).

The fix is to modify output of ping command to be agnostic to ping
version used on the platform.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 tools/testing/selftests/net/hsr/hsr_common.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/hsr/hsr_common.sh b/tools/testing/selftests/net/hsr/hsr_common.sh
index 822165391573..9ae2723f6a48 100644
--- a/tools/testing/selftests/net/hsr/hsr_common.sh
+++ b/tools/testing/selftests/net/hsr/hsr_common.sh
@@ -64,7 +64,8 @@ do_ping_long()
 	fi
 
 	VAL="$(echo $OUT | cut -d' ' -f1-8)"
-	if [ "$VAL" != "10 packets transmitted, 10 received, 0% packet loss," ]
+	SED_VAL="$(echo ${VAL} | sed -r -e 's/([0-9]{2}).*([0-9]{2}).*[[:space:]]([0-9]+%).*/\1 transmitted \2 received \3 loss/')"
+	if [ "${SED_VAL}" != "10 transmitted 10 received 0% loss" ]
 	then
 		echo "$netns -> $connect_addr ping TEST [ FAIL ]"
 		echo "Expect to send and receive 10 packets and no duplicates."
-- 
2.20.1

[net-next PATCH v5 4/4] test: hsr: Add test for HSR RedBOX (HSR-SAN) mode of operation

[Lukasz Majewski]

This patch adds hsr_redbox.sh script to test if HSR-SAN mode of operation
works correctly.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 tools/testing/selftests/net/hsr/hsr_redbox.sh | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100755 tools/testing/selftests/net/hsr/hsr_redbox.sh

diff --git a/tools/testing/selftests/net/hsr/hsr_redbox.sh b/tools/testing/selftests/net/hsr/hsr_redbox.sh
new file mode 100755
index 000000000000..6946a0c6eb17
--- /dev/null
+++ b/tools/testing/selftests/net/hsr/hsr_redbox.sh
@@ -0,0 +1,97 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ipv6=false
+
+source ./hsr_common.sh
+
+do_complete_ping_test()
+{
+	echo "INFO: Initial validation ping (HSR-SAN/RedBox)."
+	# Each node has to be able each one.
+	do_ping "${ns1}" 100.64.0.2
+	do_ping "${ns2}" 100.64.0.1
+	# Ping from SAN to hsr1 (via hsr2)
+	do_ping "${ns3}" 100.64.0.1
+	do_ping "${ns1}" 100.64.0.3
+	stop_if_error "Initial validation failed."
+
+	# Wait for MGNT HSR frames being received and nodes being
+	# merged.
+	sleep 5
+
+	echo "INFO: Longer ping test (HSR-SAN/RedBox)."
+	# Ping from SAN to hsr1 (via hsr2)
+	do_ping_long "${ns3}" 100.64.0.1
+	# Ping from hsr1 (via hsr2) to SAN
+	do_ping_long "${ns1}" 100.64.0.3
+	stop_if_error "Longer ping test failed."
+
+	echo "INFO: All good."
+}
+
+setup_hsr_interfaces()
+{
+	local HSRv="$1"
+
+	echo "INFO: preparing interfaces for HSRv${HSRv} (HSR-SAN/RedBox)."
+
+#       |NS1                     |
+#       |                        |
+#       |    /-- hsr1 --\        |
+#       | ns1eth1     ns1eth2    |
+#       |------------------------|
+#            |            |
+#            |            |
+#            |            |
+#       |------------------------|        |-----------|
+#       | ns2eth1     ns2eth2    |        |           |
+#       |    \-- hsr2 --/        |        |           |
+#       |            \           |        |           |
+#       |             ns2eth3    |--------| ns3eth1   |
+#       |             (interlink)|        |           |
+#       |NS2 (RedBOX)            |        |NS3 (SAN)  |
+#
+	# Check if iproute2 supports adding interlink port to hsrX device
+	ip link help hsr | grep -q INTERLINK
+	[ $? -ne 0 ] && { echo "iproute2: HSR interlink interface not supported!"; exit 0; }
+
+	# Create interfaces for name spaces
+	ip link add ns1eth1 netns "${ns1}" type veth peer name ns2eth1 netns "${ns2}"
+	ip link add ns1eth2 netns "${ns1}" type veth peer name ns2eth2 netns "${ns2}"
+	ip link add ns3eth1 netns "${ns3}" type veth peer name ns2eth3 netns "${ns2}"
+
+	sleep 1
+
+	ip -n "${ns1}" link set ns1eth1 up
+	ip -n "${ns1}" link set ns1eth2 up
+
+	ip -n "${ns2}" link set ns2eth1 up
+	ip -n "${ns2}" link set ns2eth2 up
+	ip -n "${ns2}" link set ns2eth3 up
+
+	ip -n "${ns3}" link set ns3eth1 up
+
+	ip -net "${ns1}" link add name hsr1 type hsr slave1 ns1eth1 slave2 ns1eth2 supervision 45 version ${HSRv} proto 0
+	ip -net "${ns2}" link add name hsr2 type hsr slave1 ns2eth1 slave2 ns2eth2 interlink ns2eth3 supervision 45 version ${HSRv} proto 0
+
+	ip -n "${ns1}" addr add 100.64.0.1/24 dev hsr1
+	ip -n "${ns2}" addr add 100.64.0.2/24 dev hsr2
+	ip -n "${ns3}" addr add 100.64.0.3/24 dev ns3eth1
+
+	ip -n "${ns1}" link set hsr1 up
+	ip -n "${ns2}" link set hsr2 up
+}
+
+check_prerequisites
+trap cleanup EXIT SIGKILL SIGTERM
+
+for i in "$ns1" "$ns2" "$ns3" ;do
+	ip netns add $i || exit $ksft_skip
+	ip -net $i link set lo up
+done
+
+setup_hsr_interfaces 1
+do_complete_ping_test
+
+exit $ret
-- 
2.20.1

Re: [net-next PATCH v5 4/4] test: hsr: Add test for HSR RedBOX (HSR-SAN) mode of operation

[Paolo Abeni]

On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> This patch adds hsr_redbox.sh script to test if HSR-SAN mode of operation
> works correctly.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  tools/testing/selftests/net/hsr/hsr_redbox.sh | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100755 tools/testing/selftests/net/hsr/hsr_redbox.sh
> 
> diff --git a/tools/testing/selftests/net/hsr/hsr_redbox.sh b/tools/testing/selftests/net/hsr/hsr_redbox.sh
> new file mode 100755
> index 000000000000..6946a0c6eb17
> --- /dev/null
> +++ b/tools/testing/selftests/net/hsr/hsr_redbox.sh

You need to add  hsr_redbox.sh to TEST_PROGS in the net self-tests
Makefile.

Thanks,

Paolo

Re: [net-next PATCH v5 0/4] net: hsr: Add support for HSR-SAN (RedBOX)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

Dear Community,

> This patch set provides v5 of HSR-SAN (RedBOX) as well as
> hsr_redbox.sh test script.
> 
> Applied on top of:
> Branch: net-next/main
> SHA1: 50aee97d1511
> 

Please be informed that without this patch series, with the current
net-next (with above credentials), the hsr_ping.sh test fails, as nodes
are not merged, so duplicate HSR frames are not filtered).

I'm going to investigate this issue now.

> Runs inside: Buildroot (2024.02.1+):
> SHA1: b31443e09cb7bb67b97ae6fb7614fe3a22889d50
> 
> Lukasz Majewski (4):
>   net: hsr: Provide RedBox support (HSR-SAN)
>   test: hsr: Move common code to hsr_common.sh file
>   test: hsr: Extract version agnostic information from ping command
>     output
>   test: hsr: Add test for HSR RedBOX (HSR-SAN) mode of operation
> 
>  include/uapi/linux/if_link.h                  |  1 +
>  net/hsr/hsr_device.c                          | 36 ++++++-
>  net/hsr/hsr_device.h                          |  4 +-
>  net/hsr/hsr_forward.c                         | 85 ++++++++++++++--
>  net/hsr/hsr_framereg.c                        | 52 ++++++++++
>  net/hsr/hsr_framereg.h                        |  6 ++
>  net/hsr/hsr_main.h                            |  7 ++
>  net/hsr/hsr_netlink.c                         | 30 +++++-
>  net/hsr/hsr_slave.c                           |  1 +
>  tools/testing/selftests/net/hsr/hsr_common.sh | 97
> +++++++++++++++++++ tools/testing/selftests/net/hsr/hsr_ping.sh   |
> 93 +----------------- tools/testing/selftests/net/hsr/hsr_redbox.sh |
> 97 +++++++++++++++++++ 12 files changed, 403 insertions(+), 106
> deletions(-) create mode 100644
> tools/testing/selftests/net/hsr/hsr_common.sh create mode 100755
> tools/testing/selftests/net/hsr/hsr_redbox.sh
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [net-next PATCH v5 0/4] net: hsr: Add support for HSR-SAN (RedBOX)

[Paolo Abeni]

On Mon, 2024-04-15 at 14:49 +0200, Lukasz Majewski wrote:
> This patch set provides v5 of HSR-SAN (RedBOX) as well as hsr_redbox.sh
> test script.
> 
> Applied on top of:
> Branch: net-next/main
> SHA1: 50aee97d1511
> 
> Runs inside: Buildroot (2024.02.1+):
> SHA1: b31443e09cb7bb67b97ae6fb7614fe3a22889d50

Please don't include the above information inside the commit message,
they are not very useful an may confuse stable teams' tools.

Instead you could add a longish overview of the series.

Thanks,

Paolo