[PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[George McCollister]

Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
removal, forwarding and duplication on DSA switches.
This series adds offloading to the xrs700x DSA driver.

Changes since RFC:
 * Split hsr and dsa patches. (Florian Fainelli)

Changes since v1:
 * Fixed some typos/wording. (Vladimir Oltean)
 * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
 * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
 * Don't add hsr tag for HSR v0 supervisory frames.
 * Fixed tag insertion offloading for PRP.

George McCollister (4):
  net: hsr: generate supervision frame without HSR/PRP tag
  net: hsr: add offloading support
  net: dsa: add support for offloading HSR
  net: dsa: xrs700x: add HSR offloading support

 Documentation/networking/netdev-features.rst |  21 ++++++
 drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
 drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
 include/linux/if_hsr.h                       |  27 +++++++
 include/linux/netdev_features.h              |   9 +++
 include/net/dsa.h                            |  13 ++++
 net/dsa/dsa_priv.h                           |  11 +++
 net/dsa/port.c                               |  34 +++++++++
 net/dsa/slave.c                              |  14 ++++
 net/dsa/switch.c                             |  24 ++++++
 net/dsa/tag_xrs700x.c                        |   7 +-
 net/ethtool/common.c                         |   4 +
 net/hsr/hsr_device.c                         |  46 ++----------
 net/hsr/hsr_device.h                         |   1 -
 net/hsr/hsr_forward.c                        |  33 ++++++++-
 net/hsr/hsr_forward.h                        |   1 +
 net/hsr/hsr_framereg.c                       |   2 +
 net/hsr/hsr_main.c                           |  11 +++
 net/hsr/hsr_main.h                           |   8 +-
 net/hsr/hsr_slave.c                          |  10 ++-
 20 files changed, 331 insertions(+), 56 deletions(-)
 create mode 100644 include/linux/if_hsr.h

-- 
2.11.0

[PATCH net-next v2 1/4] net: hsr: generate supervision frame without HSR/PRP tag

[George McCollister]

For a switch to offload insertion of HSR/PRP tags, frames must not be
sent to the CPU facing switch port with a tag. Generate supervision frames
(eth type ETH_P_PRP) without HSR v1 (ETH_P_HSR)/PRP tag and rely on
create_tagged_frame which inserts it later. This will allow skipping the
tag insertion for all outgoing frames in the future which is required for
HSR v1/PRP tag insertions to be offloaded.

HSR v0 supervision frames always contain tag information so insertion of
the tag can't be offloaded. IEC 62439-3 Ed.2.0 (HSR v1) specifically
notes that this was changed since v0 to allow offloading.

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
 net/hsr/hsr_device.c  | 32 ++++----------------------------
 net/hsr/hsr_forward.c |  6 +++++-
 2 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index ab953a1a0d6c..161b8da6a21d 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -242,8 +242,7 @@ static struct sk_buff *hsr_init_skb(struct hsr_port *master, u16 proto)
 	 * being, for PRP it is a trailer and for HSR it is a
 	 * header
 	 */
-	skb = dev_alloc_skb(sizeof(struct hsr_tag) +
-			    sizeof(struct hsr_sup_tag) +
+	skb = dev_alloc_skb(sizeof(struct hsr_sup_tag) +
 			    sizeof(struct hsr_sup_payload) + hlen + tlen);
 
 	if (!skb)
@@ -275,12 +274,10 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
 {
 	struct hsr_priv *hsr = master->hsr;
 	__u8 type = HSR_TLV_LIFE_CHECK;
-	struct hsr_tag *hsr_tag = NULL;
 	struct hsr_sup_payload *hsr_sp;
 	struct hsr_sup_tag *hsr_stag;
 	unsigned long irqflags;
 	struct sk_buff *skb;
-	u16 proto;
 
 	*interval = msecs_to_jiffies(HSR_LIFE_CHECK_INTERVAL);
 	if (hsr->announce_count < 3 && hsr->prot_version == 0) {
@@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
 		hsr->announce_count++;
 	}
 
-	if (!hsr->prot_version)
-		proto = ETH_P_PRP;
-	else
-		proto = ETH_P_HSR;
-
-	skb = hsr_init_skb(master, proto);
+	skb = hsr_init_skb(master, ETH_P_PRP);
 	if (!skb) {
 		WARN_ONCE(1, "HSR: Could not send supervision frame\n");
 		return;
 	}
 
-	if (hsr->prot_version > 0) {
-		hsr_tag = skb_put(skb, sizeof(struct hsr_tag));
-		hsr_tag->encap_proto = htons(ETH_P_PRP);
-		set_hsr_tag_LSDU_size(hsr_tag, HSR_V1_SUP_LSDUSIZE);
-	}
-
 	hsr_stag = skb_put(skb, sizeof(struct hsr_sup_tag));
 	set_hsr_stag_path(hsr_stag, (hsr->prot_version ? 0x0 : 0xf));
 	set_hsr_stag_HSR_ver(hsr_stag, hsr->prot_version);
@@ -315,8 +301,6 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
 	if (hsr->prot_version > 0) {
 		hsr_stag->sequence_nr = htons(hsr->sup_sequence_nr);
 		hsr->sup_sequence_nr++;
-		hsr_tag->sequence_nr = htons(hsr->sequence_nr);
-		hsr->sequence_nr++;
 	} else {
 		hsr_stag->sequence_nr = htons(hsr->sequence_nr);
 		hsr->sequence_nr++;
@@ -332,7 +316,7 @@ 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 (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN))
+	if (skb_put_padto(skb, ETH_ZLEN))
 		return;
 
 	hsr_forward_skb(skb, master);
@@ -348,8 +332,6 @@ static void send_prp_supervision_frame(struct hsr_port *master,
 	struct hsr_sup_tag *hsr_stag;
 	unsigned long irqflags;
 	struct sk_buff *skb;
-	struct prp_rct *rct;
-	u8 *tail;
 
 	skb = hsr_init_skb(master, ETH_P_PRP);
 	if (!skb) {
@@ -373,17 +355,11 @@ static void send_prp_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 (skb_put_padto(skb, ETH_ZLEN + HSR_HLEN)) {
+	if (skb_put_padto(skb, ETH_ZLEN)) {
 		spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
 		return;
 	}
 
-	tail = skb_tail_pointer(skb) - HSR_HLEN;
-	rct = (struct prp_rct *)tail;
-	rct->PRP_suffix = htons(ETH_P_PRP);
-	set_prp_LSDU_size(rct, HSR_V1_SUP_LSDUSIZE);
-	rct->sequence_nr = htons(hsr->sequence_nr);
-	hsr->sequence_nr++;
 	spin_unlock_irqrestore(&master->hsr->seqnr_lock, irqflags);
 
 	hsr_forward_skb(skb, master);
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index cadfccd7876e..c11be87daa8f 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -454,7 +454,11 @@ static void handle_std_frame(struct sk_buff *skb,
 void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
 			 struct hsr_frame_info *frame)
 {
-	if (proto == htons(ETH_P_PRP) ||
+	struct hsr_port *port = frame->port_rcv;
+	struct hsr_priv *hsr = port->hsr;
+
+	/* HSRv0 supervisory frames double as a tag so treat them as tagged. */
+	if ((!hsr->prot_version && proto == htons(ETH_P_PRP)) ||
 	    proto == htons(ETH_P_HSR)) {
 		/* HSR tagged frame :- Data or Supervision */
 		frame->skb_std = NULL;
-- 
2.11.0

Re: [PATCH net-next v2 1/4] net: hsr: generate supervision frame without HSR/PRP tag

[Vladimir Oltean]

On Thu, Feb 04, 2021 at 03:59:23PM -0600, George McCollister wrote:
> @@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  		hsr->announce_count++;
>  	}
>  
> -	if (!hsr->prot_version)
> -		proto = ETH_P_PRP;
> -	else
> -		proto = ETH_P_HSR;
> -
> -	skb = hsr_init_skb(master, proto);
> +	skb = hsr_init_skb(master, ETH_P_PRP);
>  	if (!skb) {
>  		WARN_ONCE(1, "HSR: Could not send supervision frame\n");
>  		return;
>  	}

I wonder why you aren't even more aggressive, just remove the proto
argument from hsr_init_skb and delay setting skb->protocol to
hsr_fill_tag or the whereabouts. This is probably also more correct
since as far as I can see, nobody is updating the skb->proto of
supervision frames from HSR v0 to v1 after your change.

Re: [PATCH net-next v2 1/4] net: hsr: generate supervision frame without HSR/PRP tag

[George McCollister]

On Sat, Feb 6, 2021 at 7:26 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Feb 04, 2021 at 03:59:23PM -0600, George McCollister wrote:
> > @@ -289,23 +286,12 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
> >               hsr->announce_count++;
> >       }
> >
> > -     if (!hsr->prot_version)
> > -             proto = ETH_P_PRP;
> > -     else
> > -             proto = ETH_P_HSR;
> > -
> > -     skb = hsr_init_skb(master, proto);
> > +     skb = hsr_init_skb(master, ETH_P_PRP);
> >       if (!skb) {
> >               WARN_ONCE(1, "HSR: Could not send supervision frame\n");
> >               return;
> >       }
>
> I wonder why you aren't even more aggressive, just remove the proto
> argument from hsr_init_skb and delay setting skb->protocol to
> hsr_fill_tag or the whereabouts. This is probably also more correct
> since as far as I can see, nobody is updating the skb->proto of
> supervision frames from HSR v0 to v1 after your change.

Will do. Thanks.

[PATCH net-next v2 2/4] net: hsr: add offloading support

[George McCollister]

Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
tag removal, duplicate generation and forwarding.

For HSR, insertion involves the switch adding a 6 byte HSR header after
the 14 byte Ethernet header. For PRP it adds a 6 byte trailer.

Tag removal involves automatically stripping the HSR/PRP header/trailer
in the switch. This is possible when the switch also performs auto
deduplication using the HSR/PRP header/trailer (making it no longer
required).

Forwarding involves automatically forwarding between redundant ports in
an HSR. This is crucial because delay is accumulated as a frame passes
through each node in the ring.

Duplication involves the switch automatically sending a single frame
from the CPU port to both redundant ports. This is required because the
inserted HSR/PRP header/trailer must contain the same sequence number
on the frames sent out both redundant ports.

Export is_hsr_master so DSA can tell them apart from other devices in
dsa_slave_changeupper.

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
 Documentation/networking/netdev-features.rst | 21 +++++++++++++++++++++
 include/linux/if_hsr.h                       | 27 +++++++++++++++++++++++++++
 include/linux/netdev_features.h              |  9 +++++++++
 net/ethtool/common.c                         |  4 ++++
 net/hsr/hsr_device.c                         | 14 +++-----------
 net/hsr/hsr_device.h                         |  1 -
 net/hsr/hsr_forward.c                        | 27 ++++++++++++++++++++++++---
 net/hsr/hsr_forward.h                        |  1 +
 net/hsr/hsr_framereg.c                       |  2 ++
 net/hsr/hsr_main.c                           | 11 +++++++++++
 net/hsr/hsr_main.h                           |  8 +-------
 net/hsr/hsr_slave.c                          | 10 ++++++----
 12 files changed, 109 insertions(+), 26 deletions(-)
 create mode 100644 include/linux/if_hsr.h

diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
index a2d7d7160e39..d7b15bb64deb 100644
--- a/Documentation/networking/netdev-features.rst
+++ b/Documentation/networking/netdev-features.rst
@@ -182,3 +182,24 @@ stricter than Hardware LRO.  A packet stream merged by Hardware GRO must
 be re-segmentable by GSO or TSO back to the exact original packet stream.
 Hardware GRO is dependent on RXCSUM since every packet successfully merged
 by hardware must also have the checksum verified by hardware.
+
+* hsr-tag-ins-offload
+
+This should be set for devices which insert an HSR (High-availability Seamless
+Redundancy) or PRP (Parallel Redundancy Protocol) tag automatically.
+
+* hsr-tag-rm-offload
+
+This should be set for devices which remove HSR (High-availability Seamless
+Redundancy) or PRP (Parallel Redundancy Protocol) tags automatically.
+
+* hsr-fwd-offload
+
+This should be set for devices which forward HSR (High-availability Seamless
+Redundancy) frames from one port to another in hardware.
+
+* hsr-dup-offload
+
+This should be set for devices which duplicate outgoing HSR (High-availability
+Seamless Redundancy) or PRP (Parallel Redundancy Protocol) tags automatically
+frames in hardware.
diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
new file mode 100644
index 000000000000..38bbc537d4e4
--- /dev/null
+++ b/include/linux/if_hsr.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_IF_HSR_H_
+#define _LINUX_IF_HSR_H_
+
+/* used to differentiate various protocols */
+enum hsr_version {
+	HSR_V0 = 0,
+	HSR_V1,
+	PRP_V1,
+};
+
+#if IS_ENABLED(CONFIG_HSR)
+extern bool is_hsr_master(struct net_device *dev);
+extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
+#else
+static inline bool is_hsr_master(struct net_device *dev)
+{
+	return false;
+}
+static inline int hsr_get_version(struct net_device *dev,
+				  enum hsr_version *ver)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_HSR */
+
+#endif /*_LINUX_IF_HSR_H_*/
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index c06d6aaba9df..3de38d6a0aea 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -86,6 +86,11 @@ enum {
 	NETIF_F_HW_MACSEC_BIT,		/* Offload MACsec operations */
 	NETIF_F_GRO_UDP_FWD_BIT,	/* Allow UDP GRO for forwarding */
 
+	NETIF_F_HW_HSR_TAG_INS_BIT,	/* Offload HSR tag insertion */
+	NETIF_F_HW_HSR_TAG_RM_BIT,	/* Offload HSR tag removal */
+	NETIF_F_HW_HSR_FWD_BIT,		/* Offload HSR forwarding */
+	NETIF_F_HW_HSR_DUP_BIT,		/* Offload HSR duplication */
+
 	/*
 	 * Add your fresh new feature above and remember to update
 	 * netdev_features_strings[] in net/core/ethtool.c and maybe
@@ -159,6 +164,10 @@ enum {
 #define NETIF_F_GSO_FRAGLIST	__NETIF_F(GSO_FRAGLIST)
 #define NETIF_F_HW_MACSEC	__NETIF_F(HW_MACSEC)
 #define NETIF_F_GRO_UDP_FWD	__NETIF_F(GRO_UDP_FWD)
+#define NETIF_F_HW_HSR_TAG_INS	__NETIF_F(HW_HSR_TAG_INS)
+#define NETIF_F_HW_HSR_TAG_RM	__NETIF_F(HW_HSR_TAG_RM)
+#define NETIF_F_HW_HSR_FWD	__NETIF_F(HW_HSR_FWD)
+#define NETIF_F_HW_HSR_DUP	__NETIF_F(HW_HSR_DUP)
 
 /* Finds the next feature with the highest number of the range of start till 0.
  */
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 835b9bba3e7e..c6a383dfd6c2 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -69,6 +69,10 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
 	[NETIF_F_GRO_FRAGLIST_BIT] =	 "rx-gro-list",
 	[NETIF_F_HW_MACSEC_BIT] =	 "macsec-hw-offload",
 	[NETIF_F_GRO_UDP_FWD_BIT] =	 "rx-udp-gro-forwarding",
+	[NETIF_F_HW_HSR_TAG_INS_BIT] =	 "hsr-tag-ins-offload",
+	[NETIF_F_HW_HSR_TAG_RM_BIT] =	 "hsr-tag-rm-offload",
+	[NETIF_F_HW_HSR_FWD_BIT] =	 "hsr-fwd-offload",
+	[NETIF_F_HW_HSR_DUP_BIT] =	 "hsr-dup-offload",
 };
 
 const char
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 161b8da6a21d..1ff4fa3b383f 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -418,6 +418,7 @@ static struct hsr_proto_ops hsr_ops = {
 	.send_sv_frame = send_hsr_supervision_frame,
 	.create_tagged_frame = hsr_create_tagged_frame,
 	.get_untagged_frame = hsr_get_untagged_frame,
+	.drop_frame = hsr_drop_frame,
 	.fill_frame_info = hsr_fill_frame_info,
 	.invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
 };
@@ -465,10 +466,11 @@ void hsr_dev_setup(struct net_device *dev)
 
 /* Return true if dev is a HSR master; return false otherwise.
  */
-inline bool is_hsr_master(struct net_device *dev)
+bool is_hsr_master(struct net_device *dev)
 {
 	return (dev->netdev_ops->ndo_start_xmit == hsr_dev_xmit);
 }
+EXPORT_SYMBOL(is_hsr_master);
 
 /* Default multicast address for HSR Supervision frames */
 static const unsigned char def_multicast_addr[ETH_ALEN] __aligned(2) = {
@@ -521,16 +523,6 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 
 	hsr->prot_version = protocol_version;
 
-	/* FIXME: should I modify the value of these?
-	 *
-	 * - hsr_dev->flags - i.e.
-	 *			IFF_MASTER/SLAVE?
-	 * - hsr_dev->priv_flags - i.e.
-	 *			IFF_EBRIDGE?
-	 *			IFF_TX_SKB_SHARING?
-	 *			IFF_HSR_MASTER/SLAVE?
-	 */
-
 	/* Make sure the 1st call to netif_carrier_on() gets through */
 	netif_carrier_off(hsr_dev);
 
diff --git a/net/hsr/hsr_device.h b/net/hsr/hsr_device.h
index 868373822ee4..9060c92168f9 100644
--- a/net/hsr/hsr_device.h
+++ b/net/hsr/hsr_device.h
@@ -19,6 +19,5 @@ 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);
 void hsr_check_carrier_and_operstate(struct hsr_priv *hsr);
-bool is_hsr_master(struct net_device *dev);
 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 c11be87daa8f..76c650fa7345 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -247,6 +247,8 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
 		/* set the lane id properly */
 		hsr_set_path_id(hsr_ethhdr, port);
 		return skb_clone(frame->skb_hsr, GFP_ATOMIC);
+	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
+		return skb_clone(frame->skb_std, GFP_ATOMIC);
 	}
 
 	/* Create the new skb with enough headroom to fit the HSR tag */
@@ -289,6 +291,8 @@ struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
 			return NULL;
 		}
 		return skb_clone(frame->skb_prp, GFP_ATOMIC);
+	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
+		return skb_clone(frame->skb_std, GFP_ATOMIC);
 	}
 
 	skb = skb_copy_expand(frame->skb_std, 0,
@@ -341,6 +345,14 @@ bool prp_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
 		 port->type ==  HSR_PT_SLAVE_A));
 }
 
+bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
+{
+	if (port->dev->features & NETIF_F_HW_HSR_FWD)
+		return prp_drop_frame(frame, port);
+
+	return false;
+}
+
 /* Forward the frame through all devices except:
  * - Back through the receiving device
  * - If it's a HSR frame: through a device where it has passed before
@@ -357,6 +369,7 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 {
 	struct hsr_port *port;
 	struct sk_buff *skb;
+	bool sent = false;
 
 	hsr_for_each_port(frame->port_rcv->hsr, port) {
 		struct hsr_priv *hsr = port->hsr;
@@ -372,6 +385,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		if (port->type != HSR_PT_MASTER && frame->is_local_exclusive)
 			continue;
 
+		/* If hardware duplicate generation is enabled, only send out
+		 * one port.
+		 */
+		if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
+			continue;
+
 		/* Don't send frame over port where it has been sent before.
 		 * Also fro SAN, this shouldn't be done.
 		 */
@@ -403,10 +422,12 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
 		}
 
 		skb->dev = port->dev;
-		if (port->type == HSR_PT_MASTER)
+		if (port->type == HSR_PT_MASTER) {
 			hsr_deliver_master(skb, port->dev, frame->node_src);
-		else
-			hsr_xmit(skb, port, frame);
+		} else {
+			if (!hsr_xmit(skb, port, frame))
+				sent = true;
+		}
 	}
 }
 
diff --git a/net/hsr/hsr_forward.h b/net/hsr/hsr_forward.h
index 618140d484ad..b6acaafa83fc 100644
--- a/net/hsr/hsr_forward.h
+++ b/net/hsr/hsr_forward.h
@@ -23,6 +23,7 @@ struct sk_buff *hsr_get_untagged_frame(struct hsr_frame_info *frame,
 struct sk_buff *prp_get_untagged_frame(struct hsr_frame_info *frame,
 				       struct hsr_port *port);
 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);
 void prp_fill_frame_info(__be16 proto, struct sk_buff *skb,
 			 struct hsr_frame_info *frame);
 void hsr_fill_frame_info(__be16 proto, struct sk_buff *skb,
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 5c97de459905..f9a8cc82ae2e 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -277,6 +277,8 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
 		skb = frame->skb_hsr;
 	else if (frame->skb_prp)
 		skb = frame->skb_prp;
+	else if (frame->skb_std)
+		skb = frame->skb_std;
 	if (!skb)
 		return;
 
diff --git a/net/hsr/hsr_main.c b/net/hsr/hsr_main.c
index 2fd1976e5b1c..f7e284f23b1f 100644
--- a/net/hsr/hsr_main.c
+++ b/net/hsr/hsr_main.c
@@ -131,6 +131,17 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
 	return NULL;
 }
 
+int hsr_get_version(struct net_device *dev, enum hsr_version *ver)
+{
+	struct hsr_priv *hsr;
+
+	hsr = netdev_priv(dev);
+	*ver = hsr->prot_version;
+
+	return 0;
+}
+EXPORT_SYMBOL(hsr_get_version);
+
 static struct notifier_block hsr_nb = {
 	.notifier_call = hsr_netdev_notify,	/* Slave event notifications */
 };
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index a9c30a608e35..a169808ee78a 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -13,6 +13,7 @@
 #include <linux/netdevice.h>
 #include <linux/list.h>
 #include <linux/if_vlan.h>
+#include <linux/if_hsr.h>
 
 /* Time constants as specified in the HSR specification (IEC-62439-3 2010)
  * Table 8.
@@ -171,13 +172,6 @@ struct hsr_port {
 	enum hsr_port_type	type;
 };
 
-/* used by driver internally to differentiate various protocols */
-enum hsr_version {
-	HSR_V0 = 0,
-	HSR_V1,
-	PRP_V1,
-};
-
 struct hsr_frame_info;
 struct hsr_node;
 
diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
index 36d5fcf09c61..c5227d42faf5 100644
--- a/net/hsr/hsr_slave.c
+++ b/net/hsr/hsr_slave.c
@@ -48,12 +48,14 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
 		goto finish_consume;
 	}
 
-	/* For HSR, only tagged frames are expected, but for PRP
-	 * there could be non tagged frames as well from Single
-	 * attached nodes (SANs).
+	/* For HSR, only tagged frames are expected (unless the device offloads
+	 * HSR tag removal), but for PRP there could be non tagged frames as
+	 * well from Single attached nodes (SANs).
 	 */
 	protocol = eth_hdr(skb)->h_proto;
-	if (hsr->proto_ops->invalid_dan_ingress_frame &&
+
+	if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
+	    hsr->proto_ops->invalid_dan_ingress_frame &&
 	    hsr->proto_ops->invalid_dan_ingress_frame(protocol))
 		goto finish_pass;
 
-- 
2.11.0

[PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[George McCollister]

Add support for offloading of HSR/PRP (IEC 62439-3) tag insertion
tag removal, duplicate generation and forwarding on DSA switches.

Add DSA_NOTIFIER_HSR_JOIN and DSA_NOTIFIER_HSR_LEAVE which trigger calls
to .port_hsr_join and .port_hsr_leave in the DSA driver for the switch.

The DSA switch driver should then set netdev feature flags for the
HSR/PRP operation that it offloads.
    NETIF_F_HW_HSR_TAG_INS
    NETIF_F_HW_HSR_TAG_RM
    NETIF_F_HW_HSR_FWD
    NETIF_F_HW_HSR_DUP

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
 include/net/dsa.h  | 13 +++++++++++++
 net/dsa/dsa_priv.h | 11 +++++++++++
 net/dsa/port.c     | 34 ++++++++++++++++++++++++++++++++++
 net/dsa/slave.c    | 14 ++++++++++++++
 net/dsa/switch.c   | 24 ++++++++++++++++++++++++
 5 files changed, 96 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 60acb9fca124..84875960b706 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -172,6 +172,10 @@ struct dsa_switch_tree {
 	list_for_each_entry((_dp), &(_dst)->ports, list)	\
 		if ((_dp)->lag_dev == (_lag))
 
+#define dsa_hsr_foreach_port(_dp, _ds, _hsr)			\
+	list_for_each_entry((_dp), &(_ds)->dst->ports, list)	\
+		if ((_dp)->ds == (_ds) && (_dp)->hsr_dev == (_hsr))
+
 static inline struct net_device *dsa_lag_dev(struct dsa_switch_tree *dst,
 					     unsigned int id)
 {
@@ -264,6 +268,7 @@ struct dsa_port {
 	struct phylink_config	pl_config;
 	struct net_device	*lag_dev;
 	bool			lag_tx_enabled;
+	struct net_device	*hsr_dev;
 
 	struct list_head list;
 
@@ -769,6 +774,14 @@ struct dsa_switch_ops {
 				 struct netdev_lag_upper_info *info);
 	int	(*port_lag_leave)(struct dsa_switch *ds, int port,
 				  struct net_device *lag);
+
+	/*
+	 * HSR integration
+	 */
+	int	(*port_hsr_join)(struct dsa_switch *ds, int port,
+				 struct net_device *hsr);
+	void	(*port_hsr_leave)(struct dsa_switch *ds, int port,
+				  struct net_device *hsr);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 263593ce94a8..bb41f8bf4f6e 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -20,6 +20,8 @@ enum {
 	DSA_NOTIFIER_BRIDGE_LEAVE,
 	DSA_NOTIFIER_FDB_ADD,
 	DSA_NOTIFIER_FDB_DEL,
+	DSA_NOTIFIER_HSR_JOIN,
+	DSA_NOTIFIER_HSR_LEAVE,
 	DSA_NOTIFIER_LAG_CHANGE,
 	DSA_NOTIFIER_LAG_JOIN,
 	DSA_NOTIFIER_LAG_LEAVE,
@@ -100,6 +102,13 @@ struct dsa_switchdev_event_work {
 	u16 vid;
 };
 
+/* DSA_NOTIFIER_HSR_* */
+struct dsa_notifier_hsr_info {
+	struct net_device *hsr;
+	int sw_index;
+	int port;
+};
+
 struct dsa_slave_priv {
 	/* Copy of CPU port xmit for faster access in slave transmit hot path */
 	struct sk_buff *	(*xmit)(struct sk_buff *skb,
@@ -183,6 +192,8 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr);
+void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
 
 static inline bool dsa_port_offloads_netdev(struct dsa_port *dp,
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 5e079a61528e..b93bda463026 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -868,3 +868,37 @@ int dsa_port_get_phy_sset_count(struct dsa_port *dp)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dsa_port_get_phy_sset_count);
+
+int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr)
+{
+	struct dsa_notifier_hsr_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.hsr = hsr,
+	};
+	int err;
+
+	dp->hsr_dev = hsr;
+
+	err = dsa_port_notify(dp, DSA_NOTIFIER_HSR_JOIN, &info);
+	if (err)
+		dp->hsr_dev = NULL;
+
+	return err;
+}
+
+void dsa_port_hsr_leave(struct dsa_port *dp, struct net_device *hsr)
+{
+	struct dsa_notifier_hsr_info info = {
+		.sw_index = dp->ds->index,
+		.port = dp->index,
+		.hsr = hsr,
+	};
+	int err;
+
+	dp->hsr_dev = NULL;
+
+	err = dsa_port_notify(dp, DSA_NOTIFIER_HSR_LEAVE, &info);
+	if (err)
+		pr_err("DSA: failed to notify DSA_NOTIFIER_HSR_LEAVE\n");
+}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b0571ab4e5a7..11d01276f11d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -17,6 +17,7 @@
 #include <net/pkt_cls.h>
 #include <net/tc_act/tc_mirred.h>
 #include <linux/if_bridge.h>
+#include <linux/if_hsr.h>
 #include <linux/netpoll.h>
 #include <linux/ptp_classify.h>
 
@@ -1935,6 +1936,19 @@ static int dsa_slave_changeupper(struct net_device *dev,
 			dsa_port_lag_leave(dp, info->upper_dev);
 			err = NOTIFY_OK;
 		}
+	} else if (is_hsr_master(info->upper_dev)) {
+		if (info->linking) {
+			err = dsa_port_hsr_join(dp, info->upper_dev);
+			if (err == -EOPNOTSUPP) {
+				NL_SET_ERR_MSG_MOD(info->info.extack,
+						   "Offloading not supported");
+				err = 0;
+			}
+			err = notifier_from_errno(err);
+		} else {
+			dsa_port_hsr_leave(dp, info->upper_dev);
+			err = NOTIFY_OK;
+		}
 	}
 
 	return err;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 5026e4143663..1e0a65b307de 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -166,6 +166,24 @@ static int dsa_switch_fdb_del(struct dsa_switch *ds,
 	return ds->ops->port_fdb_del(ds, port, info->addr, info->vid);
 }
 
+static int dsa_switch_hsr_join(struct dsa_switch *ds,
+			       struct dsa_notifier_hsr_info *info)
+{
+	if (ds->index == info->sw_index && ds->ops->port_hsr_join)
+		return ds->ops->port_hsr_join(ds, info->port, info->hsr);
+
+	return 0;
+}
+
+static int dsa_switch_hsr_leave(struct dsa_switch *ds,
+				struct dsa_notifier_hsr_info *info)
+{
+	if (ds->index == info->sw_index && ds->ops->port_hsr_leave)
+		ds->ops->port_hsr_leave(ds, info->port, info->hsr);
+
+	return 0;
+}
+
 static int dsa_switch_lag_change(struct dsa_switch *ds,
 				 struct dsa_notifier_lag_info *info)
 {
@@ -371,6 +389,12 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_FDB_DEL:
 		err = dsa_switch_fdb_del(ds, info);
 		break;
+	case DSA_NOTIFIER_HSR_JOIN:
+		err = dsa_switch_hsr_join(ds, info);
+		break;
+	case DSA_NOTIFIER_HSR_LEAVE:
+		err = dsa_switch_hsr_leave(ds, info);
+		break;
 	case DSA_NOTIFIER_LAG_CHANGE:
 		err = dsa_switch_lag_change(ds, info);
 		break;
-- 
2.11.0

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[Vladimir Oltean]

On Thu, Feb 04, 2021 at 03:59:25PM -0600, George McCollister wrote:
> @@ -1935,6 +1936,19 @@ static int dsa_slave_changeupper(struct net_device *dev,
>  			dsa_port_lag_leave(dp, info->upper_dev);
>  			err = NOTIFY_OK;
>  		}
> +	} else if (is_hsr_master(info->upper_dev)) {
> +		if (info->linking) {
> +			err = dsa_port_hsr_join(dp, info->upper_dev);
> +			if (err == -EOPNOTSUPP) {
> +				NL_SET_ERR_MSG_MOD(info->info.extack,
> +						   "Offloading not supported");
> +				err = 0;
> +			}
> +			err = notifier_from_errno(err);
> +		} else {
> +			dsa_port_hsr_leave(dp, info->upper_dev);
> +			err = NOTIFY_OK;
> +		}
>  	}
[..]
> +static int dsa_switch_hsr_join(struct dsa_switch *ds,
> +			       struct dsa_notifier_hsr_info *info)
> +{
> +	if (ds->index == info->sw_index && ds->ops->port_hsr_join)
> +		return ds->ops->port_hsr_join(ds, info->port, info->hsr);
> +
> +	return 0;
> +}
> +
> +static int dsa_switch_hsr_leave(struct dsa_switch *ds,
> +				struct dsa_notifier_hsr_info *info)
> +{
> +	if (ds->index == info->sw_index && ds->ops->port_hsr_leave)
> +		ds->ops->port_hsr_leave(ds, info->port, info->hsr);
> +
> +	return 0;
> +}
> +

If you return zero, the software fallback is never going to kick in.

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[George McCollister]

On Sat, Feb 6, 2021 at 5:29 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Feb 04, 2021 at 03:59:25PM -0600, George McCollister wrote:
> > @@ -1935,6 +1936,19 @@ static int dsa_slave_changeupper(struct net_device *dev,
> >                       dsa_port_lag_leave(dp, info->upper_dev);
> >                       err = NOTIFY_OK;
> >               }
> > +     } else if (is_hsr_master(info->upper_dev)) {
> > +             if (info->linking) {
> > +                     err = dsa_port_hsr_join(dp, info->upper_dev);
> > +                     if (err == -EOPNOTSUPP) {
> > +                             NL_SET_ERR_MSG_MOD(info->info.extack,
> > +                                                "Offloading not supported");
> > +                             err = 0;
> > +                     }
> > +                     err = notifier_from_errno(err);
> > +             } else {
> > +                     dsa_port_hsr_leave(dp, info->upper_dev);
> > +                     err = NOTIFY_OK;
> > +             }
> >       }
> [..]
> > +static int dsa_switch_hsr_join(struct dsa_switch *ds,
> > +                            struct dsa_notifier_hsr_info *info)
> > +{
> > +     if (ds->index == info->sw_index && ds->ops->port_hsr_join)
> > +             return ds->ops->port_hsr_join(ds, info->port, info->hsr);
> > +
> > +     return 0;
> > +}
> > +
> > +static int dsa_switch_hsr_leave(struct dsa_switch *ds,
> > +                             struct dsa_notifier_hsr_info *info)
> > +{
> > +     if (ds->index == info->sw_index && ds->ops->port_hsr_leave)
> > +             ds->ops->port_hsr_leave(ds, info->port, info->hsr);
> > +
> > +     return 0;
> > +}
> > +
>
> If you return zero, the software fallback is never going to kick in.

For join and leave? How is this not a problem for the bridge and lag
functions? They work the same way don't they? I figured it would be
safe to follow what they were doing.

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[Vladimir Oltean]

On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > If you return zero, the software fallback is never going to kick in.
>
> For join and leave? How is this not a problem for the bridge and lag
> functions? They work the same way don't they? I figured it would be
> safe to follow what they were doing.

I didn't say that the bridge and LAG offloading logic does the right
thing, but it is on its way there...

Those "XXX not offloaded" messages were tested with cases where the
.port_lag_join callback _is_ present, but fails (due to things like
incompatible xmit hashing policy). They were not tested with the case
where the driver does not implement .port_lag_join at all.

Doesn't mean you shouldn't do the right thing. I'll send some patches
soon, hopefully, fixing that for LAG and the bridge, you can concentrate
on HSR. For the non-offload scenario where the port is basically
standalone, we also need to disable the other bridge functions such as
address learning, otherwise it won't work properly, and that's where
I've been focusing my attention lately. You can't offload the bridge in
software, or a LAG, if you have address learning enabled. For HSR it's
even more interesting, you need to have address learning disabled even
when you offload the DANH/DANP.

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[George McCollister]

On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > > If you return zero, the software fallback is never going to kick in.
> >
> > For join and leave? How is this not a problem for the bridge and lag
> > functions? They work the same way don't they? I figured it would be
> > safe to follow what they were doing.
>
> I didn't say that the bridge and LAG offloading logic does the right
> thing, but it is on its way there...
>
> Those "XXX not offloaded" messages were tested with cases where the
> .port_lag_join callback _is_ present, but fails (due to things like
> incompatible xmit hashing policy). They were not tested with the case
> where the driver does not implement .port_lag_join at all.
>
> Doesn't mean you shouldn't do the right thing. I'll send some patches
> soon, hopefully, fixing that for LAG and the bridge, you can concentrate
> on HSR. For the non-offload scenario where the port is basically
> standalone, we also need to disable the other bridge functions such as
> address learning, otherwise it won't work properly, and that's where
> I've been focusing my attention lately. You can't offload the bridge in
> software, or a LAG, if you have address learning enabled. For HSR it's
> even more interesting, you need to have address learning disabled even
> when you offload the DANH/DANP.

Do I just return -EOPNOTSUPP instead of 0 in dsa_switch_hsr_join and
dsa_switch_hsr_leave?

I'm not sure exactly what you're saying needs to be done wrt to
address learning with HSR. The switch does address learning
internally. Are you saying the DSA address learning needs to be
disabled? If that's something I need for this patch some tips on what
to do would be appreciated because I'm a bit lost.

Thanks

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[Vladimir Oltean]

On Tue, Feb 09, 2021 at 12:37:38PM -0600, George McCollister wrote:
> On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > > > If you return zero, the software fallback is never going to kick in.
> > >
> > > For join and leave? How is this not a problem for the bridge and lag
> > > functions? They work the same way don't they? I figured it would be
> > > safe to follow what they were doing.
> >
> > I didn't say that the bridge and LAG offloading logic does the right
> > thing, but it is on its way there...
> >
> > Those "XXX not offloaded" messages were tested with cases where the
> > .port_lag_join callback _is_ present, but fails (due to things like
> > incompatible xmit hashing policy). They were not tested with the case
> > where the driver does not implement .port_lag_join at all.
> >
> > Doesn't mean you shouldn't do the right thing. I'll send some patches
> > soon, hopefully, fixing that for LAG and the bridge, you can concentrate
> > on HSR. For the non-offload scenario where the port is basically
> > standalone, we also need to disable the other bridge functions such as
> > address learning, otherwise it won't work properly, and that's where
> > I've been focusing my attention lately. You can't offload the bridge in
> > software, or a LAG, if you have address learning enabled. For HSR it's
> > even more interesting, you need to have address learning disabled even
> > when you offload the DANH/DANP.
> 
> Do I just return -EOPNOTSUPP instead of 0 in dsa_switch_hsr_join and
> dsa_switch_hsr_leave?

Yes, return -EOPNOTSUPP if the callbacks are not implemented please.

> I'm not sure exactly what you're saying needs to be done wrt to
> address learning with HSR. The switch does address learning
> internally. Are you saying the DSA address learning needs to be
> disabled?

I'm saying that when you're doing any sort of redundancy protocol, the
switch will get confused by address learning if it performs it at
physical port level, because it will see the same source MAC address
coming in from 2 (or more) different physical ports. And when it sees a
packet coming in through a port that it had already learned it should be
the destination for the MAC address because an earlier packet came
having that MAC address as source, it will think it should do
hairpinning which it's configured not to => it'll drop the packet.

Now, your switch might have some sort of concept of address learning at
logical port level, where the "logical port" would roughly correspond to
the hsr0 upper (I haven't opened the XRS700x manual to know if it does
this, sorry). Basically if you support RedBox I expect that the switch
is able to learn at the level of "this MAC address came from the HSR
ring, aka from one or both of the individual ring ports". But for
configuring that in Linux, you'd need something like:

ip link set hsr0 master br0
ip link set hsr0 type bridge_slave learning on

and then catch from DSA the switchdev notification emitted for hsr0, and
use that to configure learning on the logical port of your switch
corresponding to hsr0, instead of learning on the physical ports that
offload it.

There are similar issues related to address learning for everything
except a bridge port, basically.

> If that's something I need for this patch some tips on what
> to do would be appreciated because I'm a bit lost.

I didn't say you need to change something related to learning for this
series, because you can't anyway - DSA doesn't give you the knobs to
configure address learning yet. The series where I try to add those are
here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210209151936.97382-1-olteanv@gmail.com/

The take-away is that with those changes, a DSA driver should start its
ports in standalone mode with learning disabled and flooding of all
kinds enabled, and then treat the .port_bridge_flags callback which
should do the right thing and enable/disable address learning only when
necessary.

All I said is that address learning remaining enabled has been an issue
that prevented the non-offload scenarios from really working, but you
shouldn't be too hung up on that, and still return -EOPNOTSUPP, thereby
allowing the software fallback to kick in, even if it doesn't work.

Re: [PATCH net-next v2 3/4] net: dsa: add support for offloading HSR

[George McCollister]

On Tue, Feb 9, 2021 at 12:51 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Tue, Feb 09, 2021 at 12:37:38PM -0600, George McCollister wrote:
> > On Tue, Feb 9, 2021 at 11:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Mon, Feb 08, 2021 at 11:21:26AM -0600, George McCollister wrote:
> > > > > If you return zero, the software fallback is never going to kick in.
> > > >
> > > > For join and leave? How is this not a problem for the bridge and lag
> > > > functions? They work the same way don't they? I figured it would be
> > > > safe to follow what they were doing.
> > >
> > > I didn't say that the bridge and LAG offloading logic does the right
> > > thing, but it is on its way there...
> > >
> > > Those "XXX not offloaded" messages were tested with cases where the
> > > .port_lag_join callback _is_ present, but fails (due to things like
> > > incompatible xmit hashing policy). They were not tested with the case
> > > where the driver does not implement .port_lag_join at all.
> > >
> > > Doesn't mean you shouldn't do the right thing. I'll send some patches
> > > soon, hopefully, fixing that for LAG and the bridge, you can concentrate
> > > on HSR. For the non-offload scenario where the port is basically
> > > standalone, we also need to disable the other bridge functions such as
> > > address learning, otherwise it won't work properly, and that's where
> > > I've been focusing my attention lately. You can't offload the bridge in
> > > software, or a LAG, if you have address learning enabled. For HSR it's
> > > even more interesting, you need to have address learning disabled even
> > > when you offload the DANH/DANP.
> >
> > Do I just return -EOPNOTSUPP instead of 0 in dsa_switch_hsr_join and
> > dsa_switch_hsr_leave?
>
> Yes, return -EOPNOTSUPP if the callbacks are not implemented please.

Will do.

>
> > I'm not sure exactly what you're saying needs to be done wrt to
> > address learning with HSR. The switch does address learning
> > internally. Are you saying the DSA address learning needs to be
> > disabled?
>
> I'm saying that when you're doing any sort of redundancy protocol, the
> switch will get confused by address learning if it performs it at
> physical port level, because it will see the same source MAC address
> coming in from 2 (or more) different physical ports. And when it sees a
> packet coming in through a port that it had already learned it should be
> the destination for the MAC address because an earlier packet came
> having that MAC address as source, it will think it should do
> hairpinning which it's configured not to => it'll drop the packet.
>
> Now, your switch might have some sort of concept of address learning at
> logical port level, where the "logical port" would roughly correspond to
> the hsr0 upper (I haven't opened the XRS700x manual to know if it does
> this, sorry). Basically if you support RedBox I expect that the switch
> is able to learn at the level of "this MAC address came from the HSR
> ring, aka from one or both of the individual ring ports". But for
> configuring that in Linux, you'd need something like:

Yup, the switch has provisions to deal with this.

>
> ip link set hsr0 master br0
> ip link set hsr0 type bridge_slave learning on
>
> and then catch from DSA the switchdev notification emitted for hsr0, and
> use that to configure learning on the logical port of your switch
> corresponding to hsr0, instead of learning on the physical ports that
> offload it.
>
> There are similar issues related to address learning for everything
> except a bridge port, basically.
>
> > If that's something I need for this patch some tips on what
> > to do would be appreciated because I'm a bit lost.
>
> I didn't say you need to change something related to learning for this
> series, because you can't anyway - DSA doesn't give you the knobs to
> configure address learning yet. The series where I try to add those are
> here:
> https://patchwork.kernel.org/project/netdevbpf/cover/20210209151936.97382-1-olteanv@gmail.com/
>
> The take-away is that with those changes, a DSA driver should start its
> ports in standalone mode with learning disabled and flooding of all
> kinds enabled, and then treat the .port_bridge_flags callback which
> should do the right thing and enable/disable address learning only when
> necessary.
>
> All I said is that address learning remaining enabled has been an issue
> that prevented the non-offload scenarios from really working, but you
> shouldn't be too hung up on that, and still return -EOPNOTSUPP, thereby
> allowing the software fallback to kick in, even if it doesn't work.

Thanks for clearing that up and explaining how this will work in the future.

[PATCH net-next v2 4/4] net: dsa: xrs700x: add HSR offloading support

[George McCollister]

Add offloading for HSR/PRP (IEC 62439-3) tag insertion, tag removal
forwarding and duplication supported by the xrs7000 series switches.

Only HSR v1 and PRP v1 are supported by the xrs7000 series switches (HSR
v0 is not).

Signed-off-by: George McCollister <george.mccollister@gmail.com>
---
 drivers/net/dsa/xrs700x/xrs700x.c     | 106 ++++++++++++++++++++++++++++++++++
 drivers/net/dsa/xrs700x/xrs700x_reg.h |   5 ++
 net/dsa/tag_xrs700x.c                 |   7 ++-
 3 files changed, 117 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/xrs700x/xrs700x.c b/drivers/net/dsa/xrs700x/xrs700x.c
index 259f5e657c46..566ce9330903 100644
--- a/drivers/net/dsa/xrs700x/xrs700x.c
+++ b/drivers/net/dsa/xrs700x/xrs700x.c
@@ -7,6 +7,8 @@
 #include <net/dsa.h>
 #include <linux/if_bridge.h>
 #include <linux/of_device.h>
+#include <linux/netdev_features.h>
+#include <linux/if_hsr.h>
 #include "xrs700x.h"
 #include "xrs700x_reg.h"
 
@@ -496,6 +498,108 @@ static void xrs700x_bridge_leave(struct dsa_switch *ds, int port,
 	xrs700x_bridge_common(ds, port, bridge, false);
 }
 
+static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
+			    struct net_device *hsr)
+{
+	unsigned int val = XRS_HSR_CFG_HSR_PRP;
+	struct dsa_port *partner = NULL, *dp;
+	struct xrs700x *priv = ds->priv;
+	struct net_device *slave;
+	enum hsr_version ver;
+	int ret;
+
+	ret = hsr_get_version(hsr, &ver);
+	if (ret)
+		return ret;
+
+	if (ver == HSR_V1)
+		val |= XRS_HSR_CFG_HSR;
+	else if (ver == PRP_V1)
+		val |= XRS_HSR_CFG_PRP;
+	else
+		return -EOPNOTSUPP;
+
+	dsa_hsr_foreach_port(dp, ds, hsr) {
+		partner = dp;
+	}
+
+	/* We can't enable redundancy on the switch until both
+	 * redundant ports have signed up.
+	 */
+	if (!partner)
+		return 0;
+
+	regmap_fields_write(priv->ps_forward, partner->index,
+			    XRS_PORT_DISABLED);
+	regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
+
+	regmap_write(priv->regmap, XRS_HSR_CFG(partner->index),
+		     val | XRS_HSR_CFG_LANID_A);
+	regmap_write(priv->regmap, XRS_HSR_CFG(port),
+		     val | XRS_HSR_CFG_LANID_B);
+
+	/* Clear bits for both redundant ports (HSR only) and the CPU port to
+	 * enable forwarding.
+	 */
+	val = GENMASK(ds->num_ports - 1, 0);
+	if (ver == HSR_V1) {
+		val &= ~BIT(partner->index);
+		val &= ~BIT(port);
+	}
+	val &= ~BIT(dsa_upstream_port(ds, port));
+	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
+	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
+
+	regmap_fields_write(priv->ps_forward, partner->index,
+			    XRS_PORT_FORWARDING);
+	regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
+
+	slave = dsa_to_port(ds, port)->slave;
+
+	slave->features |= NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
+			   NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP;
+
+	return 0;
+}
+
+static void xrs700x_hsr_leave(struct dsa_switch *ds, int port,
+			      struct net_device *hsr)
+{
+	struct dsa_port *partner = NULL, *dp;
+	struct xrs700x *priv = ds->priv;
+	struct net_device *slave;
+	unsigned int val;
+
+	dsa_hsr_foreach_port(dp, ds, hsr) {
+		partner = dp;
+	}
+
+	if (!partner)
+		return;
+
+	regmap_fields_write(priv->ps_forward, partner->index,
+			    XRS_PORT_DISABLED);
+	regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
+
+	regmap_write(priv->regmap, XRS_HSR_CFG(partner->index), 0);
+	regmap_write(priv->regmap, XRS_HSR_CFG(port), 0);
+
+	/* Clear bit for the CPU port to enable forwarding. */
+	val = GENMASK(ds->num_ports - 1, 0);
+	val &= ~BIT(dsa_upstream_port(ds, port));
+	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
+	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
+
+	regmap_fields_write(priv->ps_forward, partner->index,
+			    XRS_PORT_FORWARDING);
+	regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
+
+	slave = dsa_to_port(ds, port)->slave;
+
+	slave->features &= ~(NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
+			   NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP);
+}
+
 static const struct dsa_switch_ops xrs700x_ops = {
 	.get_tag_protocol	= xrs700x_get_tag_protocol,
 	.setup			= xrs700x_setup,
@@ -509,6 +613,8 @@ static const struct dsa_switch_ops xrs700x_ops = {
 	.get_stats64		= xrs700x_get_stats64,
 	.port_bridge_join	= xrs700x_bridge_join,
 	.port_bridge_leave	= xrs700x_bridge_leave,
+	.port_hsr_join		= xrs700x_hsr_join,
+	.port_hsr_leave		= xrs700x_hsr_leave,
 };
 
 static int xrs700x_detect(struct xrs700x *priv)
diff --git a/drivers/net/dsa/xrs700x/xrs700x_reg.h b/drivers/net/dsa/xrs700x/xrs700x_reg.h
index a135d4d92b6d..470d00e07f15 100644
--- a/drivers/net/dsa/xrs700x/xrs700x_reg.h
+++ b/drivers/net/dsa/xrs700x/xrs700x_reg.h
@@ -49,6 +49,11 @@
 
 /* Port Configuration Registers - HSR/PRP */
 #define XRS_HSR_CFG(x)			(XRS_PORT_HSR_BASE(x) + 0x0)
+#define XRS_HSR_CFG_HSR_PRP		BIT(0)
+#define XRS_HSR_CFG_HSR			0
+#define XRS_HSR_CFG_PRP			BIT(8)
+#define XRS_HSR_CFG_LANID_A		0
+#define XRS_HSR_CFG_LANID_B		BIT(10)
 
 /* Port Configuration Registers - PTP */
 #define XRS_PTP_RX_SYNC_DELAY_NS_LO(x)	(XRS_PORT_PTP_BASE(x) + 0x2)
diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c
index db0ed1a5fcb7..858cdf9d2913 100644
--- a/net/dsa/tag_xrs700x.c
+++ b/net/dsa/tag_xrs700x.c
@@ -11,12 +11,17 @@
 
 static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev)
 {
-	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_port *partner, *dp = dsa_slave_to_port(dev);
 	u8 *trailer;
 
 	trailer = skb_put(skb, 1);
 	trailer[0] = BIT(dp->index);
 
+	if (dp->hsr_dev)
+		dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
+			if (partner != dp)
+				trailer[0] |= BIT(partner->index);
+
 	return skb;
 }
 
-- 
2.11.0

Re: [PATCH net-next v2 4/4] net: dsa: xrs700x: add HSR offloading support

[Vladimir Oltean]

On Thu, Feb 04, 2021 at 03:59:26PM -0600, George McCollister wrote:
> +static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
> +			    struct net_device *hsr)
> +{
> +	unsigned int val = XRS_HSR_CFG_HSR_PRP;
> +	struct dsa_port *partner = NULL, *dp;
> +	struct xrs700x *priv = ds->priv;
> +	struct net_device *slave;
> +	enum hsr_version ver;
> +	int ret;
> +
> +	ret = hsr_get_version(hsr, &ver);
> +	if (ret)
> +		return ret;
> +
> +	if (ver == HSR_V1)
> +		val |= XRS_HSR_CFG_HSR;
> +	else if (ver == PRP_V1)
> +		val |= XRS_HSR_CFG_PRP;
> +	else
> +		return -EOPNOTSUPP;
> +
> +	dsa_hsr_foreach_port(dp, ds, hsr) {
> +		partner = dp;
> +	}
> +
> +	/* We can't enable redundancy on the switch until both
> +	 * redundant ports have signed up.
> +	 */
> +	if (!partner)
> +		return 0;
> +
> +	regmap_fields_write(priv->ps_forward, partner->index,
> +			    XRS_PORT_DISABLED);
> +	regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
> +
> +	regmap_write(priv->regmap, XRS_HSR_CFG(partner->index),
> +		     val | XRS_HSR_CFG_LANID_A);
> +	regmap_write(priv->regmap, XRS_HSR_CFG(port),
> +		     val | XRS_HSR_CFG_LANID_B);
> +
> +	/* Clear bits for both redundant ports (HSR only) and the CPU port to
> +	 * enable forwarding.
> +	 */
> +	val = GENMASK(ds->num_ports - 1, 0);
> +	if (ver == HSR_V1) {
> +		val &= ~BIT(partner->index);
> +		val &= ~BIT(port);
> +	}
> +	val &= ~BIT(dsa_upstream_port(ds, port));
> +	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
> +	regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> +
> +	regmap_fields_write(priv->ps_forward, partner->index,
> +			    XRS_PORT_FORWARDING);
> +	regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
> +
> +	slave = dsa_to_port(ds, port)->slave;
> +
> +	slave->features |= NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
> +			   NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP;
> +
> +	return 0;
> +}

Is it deliberate that only one slave HSR/PRP port will have the offload
ethtool features set? If yes, then I find that a bit odd from a user
point of view.

Re: [PATCH net-next v2 4/4] net: dsa: xrs700x: add HSR offloading support

[George McCollister]

On Sat, Feb 6, 2021 at 5:53 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Feb 04, 2021 at 03:59:26PM -0600, George McCollister wrote:
> > +static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
> > +                         struct net_device *hsr)
> > +{
> > +     unsigned int val = XRS_HSR_CFG_HSR_PRP;
> > +     struct dsa_port *partner = NULL, *dp;
> > +     struct xrs700x *priv = ds->priv;
> > +     struct net_device *slave;
> > +     enum hsr_version ver;
> > +     int ret;
> > +
> > +     ret = hsr_get_version(hsr, &ver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (ver == HSR_V1)
> > +             val |= XRS_HSR_CFG_HSR;
> > +     else if (ver == PRP_V1)
> > +             val |= XRS_HSR_CFG_PRP;
> > +     else
> > +             return -EOPNOTSUPP;
> > +
> > +     dsa_hsr_foreach_port(dp, ds, hsr) {
> > +             partner = dp;
> > +     }
> > +
> > +     /* We can't enable redundancy on the switch until both
> > +      * redundant ports have signed up.
> > +      */
> > +     if (!partner)
> > +             return 0;
> > +
> > +     regmap_fields_write(priv->ps_forward, partner->index,
> > +                         XRS_PORT_DISABLED);
> > +     regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
> > +
> > +     regmap_write(priv->regmap, XRS_HSR_CFG(partner->index),
> > +                  val | XRS_HSR_CFG_LANID_A);
> > +     regmap_write(priv->regmap, XRS_HSR_CFG(port),
> > +                  val | XRS_HSR_CFG_LANID_B);
> > +
> > +     /* Clear bits for both redundant ports (HSR only) and the CPU port to
> > +      * enable forwarding.
> > +      */
> > +     val = GENMASK(ds->num_ports - 1, 0);
> > +     if (ver == HSR_V1) {
> > +             val &= ~BIT(partner->index);
> > +             val &= ~BIT(port);
> > +     }
> > +     val &= ~BIT(dsa_upstream_port(ds, port));
> > +     regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
> > +     regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> > +
> > +     regmap_fields_write(priv->ps_forward, partner->index,
> > +                         XRS_PORT_FORWARDING);
> > +     regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
> > +
> > +     slave = dsa_to_port(ds, port)->slave;
> > +
> > +     slave->features |= NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
> > +                        NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP;
> > +
> > +     return 0;
> > +}
>
> Is it deliberate that only one slave HSR/PRP port will have the offload
> ethtool features set? If yes, then I find that a bit odd from a user
> point of view.

No. Good catch. This is a mistake I introduced when I added the code
for finding the partner. Originally for testing I had hacks that hard
coded the ports used and reconfigured HSR for each join.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Tobias Waldekranz]

On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@gmail.com> wrote:
> Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
> removal, forwarding and duplication on DSA switches.
> This series adds offloading to the xrs700x DSA driver.
>
> Changes since RFC:
>  * Split hsr and dsa patches. (Florian Fainelli)
>
> Changes since v1:
>  * Fixed some typos/wording. (Vladimir Oltean)
>  * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
>  * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
>  * Don't add hsr tag for HSR v0 supervisory frames.
>  * Fixed tag insertion offloading for PRP.
>
> George McCollister (4):
>   net: hsr: generate supervision frame without HSR/PRP tag
>   net: hsr: add offloading support
>   net: dsa: add support for offloading HSR
>   net: dsa: xrs700x: add HSR offloading support
>
>  Documentation/networking/netdev-features.rst |  21 ++++++
>  drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
>  drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
>  include/linux/if_hsr.h                       |  27 +++++++
>  include/linux/netdev_features.h              |   9 +++
>  include/net/dsa.h                            |  13 ++++
>  net/dsa/dsa_priv.h                           |  11 +++
>  net/dsa/port.c                               |  34 +++++++++
>  net/dsa/slave.c                              |  14 ++++
>  net/dsa/switch.c                             |  24 ++++++
>  net/dsa/tag_xrs700x.c                        |   7 +-
>  net/ethtool/common.c                         |   4 +
>  net/hsr/hsr_device.c                         |  46 ++----------
>  net/hsr/hsr_device.h                         |   1 -
>  net/hsr/hsr_forward.c                        |  33 ++++++++-
>  net/hsr/hsr_forward.h                        |   1 +
>  net/hsr/hsr_framereg.c                       |   2 +
>  net/hsr/hsr_main.c                           |  11 +++
>  net/hsr/hsr_main.h                           |   8 +-
>  net/hsr/hsr_slave.c                          |  10 ++-
>  20 files changed, 331 insertions(+), 56 deletions(-)
>  create mode 100644 include/linux/if_hsr.h
>
> -- 
> 2.11.0

Hi George,

I will hopefully have some more time to look into this during the coming
weeks. What follows are some random thoughts so far, I hope you can
accept the windy road :)

Broadly speaking, I gather there are two common topologies that will be
used with the XRS chip: "End-device" and "RedBox".

End-device:    RedBox:
 .-----.       .-----.
 | CPU |       | CPU |
 '--+--'       '--+--'
    |             |
.---0---.     .---0---.
|  XRS  |     |  XRS  3--- Non-redundant network
'-1---2-'     '-1---2-'
  |   |         |   |
 HSR Ring      HSR Ring

From the looks of it, this series only deals with the end-device
use-case. Is that right?

I will be targeting a RedBox setup, and I believe that means that the
remaining port has to be configured as an "interlink". (HSR/PRP is still
pretty new to me). Is that equivalent to a Linux config like this:

      br0
     /   \
   hsr0   \
   /  \    \
swp1 swp2 swp3

Or are there some additional semantics involved in forwarding between
the redundant ports and the interlink?

The chip is very rigid in the sense that most roles are statically
allocated to specific ports. I think we need to add checks for this.

Looking at the packets being generated on the redundant ports, both
regular traffic and supervision frames seem to be HSR-tagged. Are
supervision frames not supposed to be sent with an outer ethertype of
0x88fb? The manual talks about the possibility of setting up a policy
entry to bypass HSR-tagging (section 6.1.5), is this what that is for?

In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
join/leave calls somehow? My guess is all drivers are going to end up
having to do the same dance of deferring configuration until both ports
are known.

Thanks for working on this!

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[George McCollister]

On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@gmail.com> wrote:
> > Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
> > removal, forwarding and duplication on DSA switches.
> > This series adds offloading to the xrs700x DSA driver.
> >
> > Changes since RFC:
> >  * Split hsr and dsa patches. (Florian Fainelli)
> >
> > Changes since v1:
> >  * Fixed some typos/wording. (Vladimir Oltean)
> >  * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
> >  * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
> >  * Don't add hsr tag for HSR v0 supervisory frames.
> >  * Fixed tag insertion offloading for PRP.
> >
> > George McCollister (4):
> >   net: hsr: generate supervision frame without HSR/PRP tag
> >   net: hsr: add offloading support
> >   net: dsa: add support for offloading HSR
> >   net: dsa: xrs700x: add HSR offloading support
> >
> >  Documentation/networking/netdev-features.rst |  21 ++++++
> >  drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
> >  drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
> >  include/linux/if_hsr.h                       |  27 +++++++
> >  include/linux/netdev_features.h              |   9 +++
> >  include/net/dsa.h                            |  13 ++++
> >  net/dsa/dsa_priv.h                           |  11 +++
> >  net/dsa/port.c                               |  34 +++++++++
> >  net/dsa/slave.c                              |  14 ++++
> >  net/dsa/switch.c                             |  24 ++++++
> >  net/dsa/tag_xrs700x.c                        |   7 +-
> >  net/ethtool/common.c                         |   4 +
> >  net/hsr/hsr_device.c                         |  46 ++----------
> >  net/hsr/hsr_device.h                         |   1 -
> >  net/hsr/hsr_forward.c                        |  33 ++++++++-
> >  net/hsr/hsr_forward.h                        |   1 +
> >  net/hsr/hsr_framereg.c                       |   2 +
> >  net/hsr/hsr_main.c                           |  11 +++
> >  net/hsr/hsr_main.h                           |   8 +-
> >  net/hsr/hsr_slave.c                          |  10 ++-
> >  20 files changed, 331 insertions(+), 56 deletions(-)
> >  create mode 100644 include/linux/if_hsr.h
> >
> > --
> > 2.11.0
>
> Hi George,
>
> I will hopefully have some more time to look into this during the coming
> weeks. What follows are some random thoughts so far, I hope you can
> accept the windy road :)
>
> Broadly speaking, I gather there are two common topologies that will be
> used with the XRS chip: "End-device" and "RedBox".
>
> End-device:    RedBox:
>  .-----.       .-----.
>  | CPU |       | CPU |
>  '--+--'       '--+--'
>     |             |
> .---0---.     .---0---.
> |  XRS  |     |  XRS  3--- Non-redundant network
> '-1---2-'     '-1---2-'
>   |   |         |   |
>  HSR Ring      HSR Ring

There is also the HSR-HSR use case and HSR-PRP use case.

>
> From the looks of it, this series only deals with the end-device
> use-case. Is that right?

Correct. net/hsr doesn't support this use case right now. It will
stomp the outgoing source MAC with that of the interface for instance.
It also doesn't implement a ProxyNodeTable (though that actually
wouldn't matter if you were offloading to the xrs700x I think). Try
commenting out the ether_addr_copy() line in hsr_xmit and see if it
makes your use case work.

>
> I will be targeting a RedBox setup, and I believe that means that the
> remaining port has to be configured as an "interlink". (HSR/PRP is still
> pretty new to me). Is that equivalent to a Linux config like this:

Depends what you mean by configured as an interlink. I believe bit 9
of HSR_CFG in the switch is only supposed to be used for the HSR-HSR
and HSR-PRP use case, not HSR-SAN.

>
>       br0
>      /   \
>    hsr0   \
>    /  \    \
> swp1 swp2 swp3
>
> Or are there some additional semantics involved in forwarding between
> the redundant ports and the interlink?

That sounds right.

>
> The chip is very rigid in the sense that most roles are statically
> allocated to specific ports. I think we need to add checks for this.

Okay. I'll look into this. Though a lot of the restrictions have to do
with using the third gigabit port for an HSR/PRP interlink (not
HSR-SAN) which I'm not currently supporting anyway.

>
> Looking at the packets being generated on the redundant ports, both
> regular traffic and supervision frames seem to be HSR-tagged. Are
> supervision frames not supposed to be sent with an outer ethertype of
> 0x88fb? The manual talks about the possibility of setting up a policy
> entry to bypass HSR-tagging (section 6.1.5), is this what that is for?

This was changed between 62439-3:2010 and 62439-3:2012.
"Prefixing the supervision frames on HSR by an HSR tag to simplify the hardware
implementation and introduce a unique EtherType for HSR to simplify processing."
The Linux HSR driver calls the former HSR v0 and the later HSR v1. I'm
not sure what their intention was with this feature. The inbound
policies are pretty flexible so maybe they didn't have anything so
specific in mind.
I don't think the xrs7000 series could offload HSR v0 anyway because
the tag ether type is different.

>
> In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
> join/leave calls somehow? My guess is all drivers are going to end up
> having to do the same dance of deferring configuration until both ports
> are known.

Describe what you mean a bit more. Do you mean join and leave should
each only be called once with both hsr ports being passed in?

>
> Thanks for working on this!

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Tobias Waldekranz]

On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote:
> On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote:
>>
>> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@gmail.com> wrote:
>> > Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
>> > removal, forwarding and duplication on DSA switches.
>> > This series adds offloading to the xrs700x DSA driver.
>> >
>> > Changes since RFC:
>> >  * Split hsr and dsa patches. (Florian Fainelli)
>> >
>> > Changes since v1:
>> >  * Fixed some typos/wording. (Vladimir Oltean)
>> >  * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
>> >  * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
>> >  * Don't add hsr tag for HSR v0 supervisory frames.
>> >  * Fixed tag insertion offloading for PRP.
>> >
>> > George McCollister (4):
>> >   net: hsr: generate supervision frame without HSR/PRP tag
>> >   net: hsr: add offloading support
>> >   net: dsa: add support for offloading HSR
>> >   net: dsa: xrs700x: add HSR offloading support
>> >
>> >  Documentation/networking/netdev-features.rst |  21 ++++++
>> >  drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
>> >  drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
>> >  include/linux/if_hsr.h                       |  27 +++++++
>> >  include/linux/netdev_features.h              |   9 +++
>> >  include/net/dsa.h                            |  13 ++++
>> >  net/dsa/dsa_priv.h                           |  11 +++
>> >  net/dsa/port.c                               |  34 +++++++++
>> >  net/dsa/slave.c                              |  14 ++++
>> >  net/dsa/switch.c                             |  24 ++++++
>> >  net/dsa/tag_xrs700x.c                        |   7 +-
>> >  net/ethtool/common.c                         |   4 +
>> >  net/hsr/hsr_device.c                         |  46 ++----------
>> >  net/hsr/hsr_device.h                         |   1 -
>> >  net/hsr/hsr_forward.c                        |  33 ++++++++-
>> >  net/hsr/hsr_forward.h                        |   1 +
>> >  net/hsr/hsr_framereg.c                       |   2 +
>> >  net/hsr/hsr_main.c                           |  11 +++
>> >  net/hsr/hsr_main.h                           |   8 +-
>> >  net/hsr/hsr_slave.c                          |  10 ++-
>> >  20 files changed, 331 insertions(+), 56 deletions(-)
>> >  create mode 100644 include/linux/if_hsr.h
>> >
>> > --
>> > 2.11.0
>>
>> Hi George,
>>
>> I will hopefully have some more time to look into this during the coming
>> weeks. What follows are some random thoughts so far, I hope you can
>> accept the windy road :)
>>
>> Broadly speaking, I gather there are two common topologies that will be
>> used with the XRS chip: "End-device" and "RedBox".
>>
>> End-device:    RedBox:
>>  .-----.       .-----.
>>  | CPU |       | CPU |
>>  '--+--'       '--+--'
>>     |             |
>> .---0---.     .---0---.
>> |  XRS  |     |  XRS  3--- Non-redundant network
>> '-1---2-'     '-1---2-'
>>   |   |         |   |
>>  HSR Ring      HSR Ring
>
> There is also the HSR-HSR use case and HSR-PRP use case.

HSR-HSR is also known as a "QuadBox", yes? HSR-PRP is the same thing,
but having two PRP networks on one side and an HSR ring on the other?

>> From the looks of it, this series only deals with the end-device
>> use-case. Is that right?
>
> Correct. net/hsr doesn't support this use case right now. It will
> stomp the outgoing source MAC with that of the interface for instance.

Good to know! When would that behavior be required? Presumably it is not
overriding the SA just for fun?

> It also doesn't implement a ProxyNodeTable (though that actually
> wouldn't matter if you were offloading to the xrs700x I think). Try
> commenting out the ether_addr_copy() line in hsr_xmit and see if it
> makes your use case work.

So what is missing is basically to expand the current facility for
generating sequence numbers to maintain a table of such associations,
keyed by the SA?

Is the lack of that table the reason for enforcing that the SA match the
HSR netdev?

>> I will be targeting a RedBox setup, and I believe that means that the
>> remaining port has to be configured as an "interlink". (HSR/PRP is still
>> pretty new to me). Is that equivalent to a Linux config like this:
>
> Depends what you mean by configured as an interlink. I believe bit 9
> of HSR_CFG in the switch is only supposed to be used for the HSR-HSR
> and HSR-PRP use case, not HSR-SAN.

Interesting, section 6.4.1 of the XRS manual states: "The interlink port
can be either in HSR, PRP or normal (non-HSR, non-PRP) mode." Maybe the
term is overloaded?

>>       br0
>>      /   \
>>    hsr0   \
>>    /  \    \
>> swp1 swp2 swp3
>>
>> Or are there some additional semantics involved in forwarding between
>> the redundant ports and the interlink?
>
> That sounds right.
>
>>
>> The chip is very rigid in the sense that most roles are statically
>> allocated to specific ports. I think we need to add checks for this.
>
> Okay. I'll look into this. Though a lot of the restrictions have to do
> with using the third gigabit port for an HSR/PRP interlink (not
> HSR-SAN) which I'm not currently supporting anyway.

But nothing is stopping me from trying to setup an HSR ring between port
(2,3) or (1,3), right? And that is not supported by the chip as I
understand it from looking at table 25.

>> Looking at the packets being generated on the redundant ports, both
>> regular traffic and supervision frames seem to be HSR-tagged. Are
>> supervision frames not supposed to be sent with an outer ethertype of
>> 0x88fb? The manual talks about the possibility of setting up a policy
>> entry to bypass HSR-tagging (section 6.1.5), is this what that is for?
>
> This was changed between 62439-3:2010 and 62439-3:2012.
> "Prefixing the supervision frames on HSR by an HSR tag to simplify the hardware
> implementation and introduce a unique EtherType for HSR to simplify
> processing."

Thank you, that would have taken me a long time to figure out :)

> The Linux HSR driver calls the former HSR v0 and the later HSR v1. I'm
> not sure what their intention was with this feature. The inbound
> policies are pretty flexible so maybe they didn't have anything so
> specific in mind.

Now that I think of it, maybe you want things like LLDP to still operate
hop-by-hop over the ring?

> I don't think the xrs7000 series could offload HSR v0 anyway because
> the tag ether type is different.
>
>>
>> In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
>> join/leave calls somehow? My guess is all drivers are going to end up
>> having to do the same dance of deferring configuration until both ports
>> are known.
>
> Describe what you mean a bit more. Do you mean join and leave should
> each only be called once with both hsr ports being passed in?

Exactly. Maybe we could use `netdev_for_each_lower_dev` to figure out if
the other port has already been switched over to the new upper or
something. I find it hard to believe that there is any hardware out
there that can do something useful with a single HSR/PRP port anyway.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[George McCollister]

On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz <tobias@waldekranz.com> wrote:
>
> On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote:
> > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >>
> >> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@gmail.com> wrote:
> >> > Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
> >> > removal, forwarding and duplication on DSA switches.
> >> > This series adds offloading to the xrs700x DSA driver.
> >> >
> >> > Changes since RFC:
> >> >  * Split hsr and dsa patches. (Florian Fainelli)
> >> >
> >> > Changes since v1:
> >> >  * Fixed some typos/wording. (Vladimir Oltean)
> >> >  * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
> >> >  * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
> >> >  * Don't add hsr tag for HSR v0 supervisory frames.
> >> >  * Fixed tag insertion offloading for PRP.
> >> >
> >> > George McCollister (4):
> >> >   net: hsr: generate supervision frame without HSR/PRP tag
> >> >   net: hsr: add offloading support
> >> >   net: dsa: add support for offloading HSR
> >> >   net: dsa: xrs700x: add HSR offloading support
> >> >
> >> >  Documentation/networking/netdev-features.rst |  21 ++++++
> >> >  drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
> >> >  drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
> >> >  include/linux/if_hsr.h                       |  27 +++++++
> >> >  include/linux/netdev_features.h              |   9 +++
> >> >  include/net/dsa.h                            |  13 ++++
> >> >  net/dsa/dsa_priv.h                           |  11 +++
> >> >  net/dsa/port.c                               |  34 +++++++++
> >> >  net/dsa/slave.c                              |  14 ++++
> >> >  net/dsa/switch.c                             |  24 ++++++
> >> >  net/dsa/tag_xrs700x.c                        |   7 +-
> >> >  net/ethtool/common.c                         |   4 +
> >> >  net/hsr/hsr_device.c                         |  46 ++----------
> >> >  net/hsr/hsr_device.h                         |   1 -
> >> >  net/hsr/hsr_forward.c                        |  33 ++++++++-
> >> >  net/hsr/hsr_forward.h                        |   1 +
> >> >  net/hsr/hsr_framereg.c                       |   2 +
> >> >  net/hsr/hsr_main.c                           |  11 +++
> >> >  net/hsr/hsr_main.h                           |   8 +-
> >> >  net/hsr/hsr_slave.c                          |  10 ++-
> >> >  20 files changed, 331 insertions(+), 56 deletions(-)
> >> >  create mode 100644 include/linux/if_hsr.h
> >> >
> >> > --
> >> > 2.11.0
> >>
> >> Hi George,
> >>
> >> I will hopefully have some more time to look into this during the coming
> >> weeks. What follows are some random thoughts so far, I hope you can
> >> accept the windy road :)
> >>
> >> Broadly speaking, I gather there are two common topologies that will be
> >> used with the XRS chip: "End-device" and "RedBox".
> >>
> >> End-device:    RedBox:
> >>  .-----.       .-----.
> >>  | CPU |       | CPU |
> >>  '--+--'       '--+--'
> >>     |             |
> >> .---0---.     .---0---.
> >> |  XRS  |     |  XRS  3--- Non-redundant network
> >> '-1---2-'     '-1---2-'
> >>   |   |         |   |
> >>  HSR Ring      HSR Ring
> >
> > There is also the HSR-HSR use case and HSR-PRP use case.
>
> HSR-HSR is also known as a "QuadBox", yes? HSR-PRP is the same thing,
> but having two PRP networks on one side and an HSR ring on the other?

Yes. I believe you are correct.
From the spec:
"QuadBox
Quadruple port device connecting two peer HSR rings, which behaves as
an HSR node in
each ring and is able to filter the traffic and forward it from ring to ring."

>
> >> From the looks of it, this series only deals with the end-device
> >> use-case. Is that right?
> >
> > Correct. net/hsr doesn't support this use case right now. It will
> > stomp the outgoing source MAC with that of the interface for instance.
>
> Good to know! When would that behavior be required? Presumably it is not
> overriding the SA just for fun?

Over the last few weeks I've looked over that code for way longer than
I'd like to admit and I'm still not sure. As far as I can tell, the
original authors have disappeared. My guess is it has something to do
with a configuration in which they had each redundant interface set to
a different MAC address and wanted the frames to go out with the
associated MAC address. As far as I can tell this is a violation of
the spec.

>
> > It also doesn't implement a ProxyNodeTable (though that actually
> > wouldn't matter if you were offloading to the xrs700x I think). Try
> > commenting out the ether_addr_copy() line in hsr_xmit and see if it
> > makes your use case work.
>
> So what is missing is basically to expand the current facility for
> generating sequence numbers to maintain a table of such associations,
> keyed by the SA?

For the software implementation it would also need to use the
ProxyNodeTable to prevent forwarding matching frames on the ring and
delivering them to the hsr master port. It's also supposed to drop
frames coming in on a redundant port if the source address is in the
ProxyNodeTable.

>
> Is the lack of that table the reason for enforcing that the SA match the
> HSR netdev?

Could be.

>
> >> I will be targeting a RedBox setup, and I believe that means that the
> >> remaining port has to be configured as an "interlink". (HSR/PRP is still
> >> pretty new to me). Is that equivalent to a Linux config like this:
> >
> > Depends what you mean by configured as an interlink. I believe bit 9
> > of HSR_CFG in the switch is only supposed to be used for the HSR-HSR
> > and HSR-PRP use case, not HSR-SAN.
>
> Interesting, section 6.4.1 of the XRS manual states: "The interlink port
> can be either in HSR, PRP or normal (non-HSR, non-PRP) mode." Maybe the
> term is overloaded?

Yeah I guess since it's the only port that can be used for QuadBox
they call it the interlink port even if it doesn't have the HSR/PRP
mode enabled with the interlink bit set.

>
> >>       br0
> >>      /   \
> >>    hsr0   \
> >>    /  \    \
> >> swp1 swp2 swp3
> >>
> >> Or are there some additional semantics involved in forwarding between
> >> the redundant ports and the interlink?
> >
> > That sounds right.
> >
> >>
> >> The chip is very rigid in the sense that most roles are statically
> >> allocated to specific ports. I think we need to add checks for this.
> >
> > Okay. I'll look into this. Though a lot of the restrictions have to do
> > with using the third gigabit port for an HSR/PRP interlink (not
> > HSR-SAN) which I'm not currently supporting anyway.
>
> But nothing is stopping me from trying to setup an HSR ring between port
> (2,3) or (1,3), right? And that is not supported by the chip as I
> understand it from looking at table 25.

Yeah. That's why I said I'd look into it :). Wasn't an issue for my
board since port 0 isn't connected and port 3 is used as the CPU
facing port.

>
> >> Looking at the packets being generated on the redundant ports, both
> >> regular traffic and supervision frames seem to be HSR-tagged. Are
> >> supervision frames not supposed to be sent with an outer ethertype of
> >> 0x88fb? The manual talks about the possibility of setting up a policy
> >> entry to bypass HSR-tagging (section 6.1.5), is this what that is for?
> >
> > This was changed between 62439-3:2010 and 62439-3:2012.
> > "Prefixing the supervision frames on HSR by an HSR tag to simplify the hardware
> > implementation and introduce a unique EtherType for HSR to simplify
> > processing."
>
> Thank you, that would have taken me a long time to figure out :)
>
> > The Linux HSR driver calls the former HSR v0 and the later HSR v1. I'm
> > not sure what their intention was with this feature. The inbound
> > policies are pretty flexible so maybe they didn't have anything so
> > specific in mind.
>
> Now that I think of it, maybe you want things like LLDP to still operate
> hop-by-hop over the ring?

Not sure. Would need to look into it.

>
> > I don't think the xrs7000 series could offload HSR v0 anyway because
> > the tag ether type is different.
> >
> >>
> >> In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
> >> join/leave calls somehow? My guess is all drivers are going to end up
> >> having to do the same dance of deferring configuration until both ports
> >> are known.
> >
> > Describe what you mean a bit more. Do you mean join and leave should
> > each only be called once with both hsr ports being passed in?
>
> Exactly. Maybe we could use `netdev_for_each_lower_dev` to figure out if
> the other port has already been switched over to the new upper or
> something. I find it hard to believe that there is any hardware out
> there that can do something useful with a single HSR/PRP port anyway.

If one port failed maybe it would still be useful to join one port if
the switch supported it? Maybe this couldn't ever happen anyway due
the way hsr is designed.

How were you thinking this would work? Would it just not use
dsa_port_notify() and call a switch op directly after the second
port's dsa_slave_changeupper() call? Or would we instead keep port
notifiers and calls to dsa_switch_hsr_join for each port and just make
dsa_switch_hsr_join() not call the switch op to create the HSR until
the second port called it? I'm not all that familiar with how these
dsa notifiers work and would prefer to stick with using a similar
mechanism to the bridge and lag support. It would be nice to get some
feedback from the DSA maintainers on how they would prefer it to work
if they indeed had a preference at all.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Vladimir Oltean]

On Tue, Feb 09, 2021 at 11:04:08AM -0600, George McCollister wrote:
> > >> In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
> > >> join/leave calls somehow? My guess is all drivers are going to end up
> > >> having to do the same dance of deferring configuration until both ports
> > >> are known.
> > >
> > > Describe what you mean a bit more. Do you mean join and leave should
> > > each only be called once with both hsr ports being passed in?
> >
> > Exactly. Maybe we could use `netdev_for_each_lower_dev` to figure out if
> > the other port has already been switched over to the new upper or
> > something. I find it hard to believe that there is any hardware out
> > there that can do something useful with a single HSR/PRP port anyway.
>
> If one port failed maybe it would still be useful to join one port if
> the switch supported it? Maybe this couldn't ever happen anyway due
> the way hsr is designed.
>
> How were you thinking this would work? Would it just not use
> dsa_port_notify() and call a switch op directly after the second
> port's dsa_slave_changeupper() call? Or would we instead keep port
> notifiers and calls to dsa_switch_hsr_join for each port and just make
> dsa_switch_hsr_join() not call the switch op to create the HSR until
> the second port called it? I'm not all that familiar with how these
> dsa notifiers work and would prefer to stick with using a similar
> mechanism to the bridge and lag support. It would be nice to get some
> feedback from the DSA maintainers on how they would prefer it to work
> if they indeed had a preference at all.

Even though I understand where this is coming from, I have grown to
dislike overengineered solutions. DSA is there to standardize how an
Ethernet-connected switch interacts with the network stack, not to be
the middle man that tries to offer a lending hand everywhere.
If there is a 50/50 chance that this extra logic in the DSA mid layer
will not be needed for the second switch that offloads HSR/PRP, then I'd
go with "don't do it". Just emit a hsr_join for both ports and let the
driver deal with it.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Tobias Waldekranz]

On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote:
> On Tue, Feb 9, 2021 at 8:38 AM Tobias Waldekranz <tobias@waldekranz.com> wrote:
>>
>> On Mon, Feb 08, 2021 at 15:09, George McCollister <george.mccollister@gmail.com> wrote:
>> > On Mon, Feb 8, 2021 at 2:16 PM Tobias Waldekranz <tobias@waldekranz.com> wrote:
>> >>
>> >> On Thu, Feb 04, 2021 at 15:59, George McCollister <george.mccollister@gmail.com> wrote:
>> >> > Add support for offloading HSR/PRP (IEC 62439-3) tag insertion, tag
>> >> > removal, forwarding and duplication on DSA switches.
>> >> > This series adds offloading to the xrs700x DSA driver.
>> >> >
>> >> > Changes since RFC:
>> >> >  * Split hsr and dsa patches. (Florian Fainelli)
>> >> >
>> >> > Changes since v1:
>> >> >  * Fixed some typos/wording. (Vladimir Oltean)
>> >> >  * eliminate IFF_HSR and use is_hsr_master instead. (Vladimir Oltean)
>> >> >  * Make hsr_handle_sup_frame handle skb_std as well (required when offloading)
>> >> >  * Don't add hsr tag for HSR v0 supervisory frames.
>> >> >  * Fixed tag insertion offloading for PRP.
>> >> >
>> >> > George McCollister (4):
>> >> >   net: hsr: generate supervision frame without HSR/PRP tag
>> >> >   net: hsr: add offloading support
>> >> >   net: dsa: add support for offloading HSR
>> >> >   net: dsa: xrs700x: add HSR offloading support
>> >> >
>> >> >  Documentation/networking/netdev-features.rst |  21 ++++++
>> >> >  drivers/net/dsa/xrs700x/xrs700x.c            | 106 +++++++++++++++++++++++++++
>> >> >  drivers/net/dsa/xrs700x/xrs700x_reg.h        |   5 ++
>> >> >  include/linux/if_hsr.h                       |  27 +++++++
>> >> >  include/linux/netdev_features.h              |   9 +++
>> >> >  include/net/dsa.h                            |  13 ++++
>> >> >  net/dsa/dsa_priv.h                           |  11 +++
>> >> >  net/dsa/port.c                               |  34 +++++++++
>> >> >  net/dsa/slave.c                              |  14 ++++
>> >> >  net/dsa/switch.c                             |  24 ++++++
>> >> >  net/dsa/tag_xrs700x.c                        |   7 +-
>> >> >  net/ethtool/common.c                         |   4 +
>> >> >  net/hsr/hsr_device.c                         |  46 ++----------
>> >> >  net/hsr/hsr_device.h                         |   1 -
>> >> >  net/hsr/hsr_forward.c                        |  33 ++++++++-
>> >> >  net/hsr/hsr_forward.h                        |   1 +
>> >> >  net/hsr/hsr_framereg.c                       |   2 +
>> >> >  net/hsr/hsr_main.c                           |  11 +++
>> >> >  net/hsr/hsr_main.h                           |   8 +-
>> >> >  net/hsr/hsr_slave.c                          |  10 ++-
>> >> >  20 files changed, 331 insertions(+), 56 deletions(-)
>> >> >  create mode 100644 include/linux/if_hsr.h
>> >> >
>> >> > --
>> >> > 2.11.0
>> >>
>> >> Hi George,
>> >>
>> >> I will hopefully have some more time to look into this during the coming
>> >> weeks. What follows are some random thoughts so far, I hope you can
>> >> accept the windy road :)
>> >>
>> >> Broadly speaking, I gather there are two common topologies that will be
>> >> used with the XRS chip: "End-device" and "RedBox".
>> >>
>> >> End-device:    RedBox:
>> >>  .-----.       .-----.
>> >>  | CPU |       | CPU |
>> >>  '--+--'       '--+--'
>> >>     |             |
>> >> .---0---.     .---0---.
>> >> |  XRS  |     |  XRS  3--- Non-redundant network
>> >> '-1---2-'     '-1---2-'
>> >>   |   |         |   |
>> >>  HSR Ring      HSR Ring
>> >
>> > There is also the HSR-HSR use case and HSR-PRP use case.
>>
>> HSR-HSR is also known as a "QuadBox", yes? HSR-PRP is the same thing,
>> but having two PRP networks on one side and an HSR ring on the other?
>
> Yes. I believe you are correct.
> From the spec:
> "QuadBox
> Quadruple port device connecting two peer HSR rings, which behaves as
> an HSR node in
> each ring and is able to filter the traffic and forward it from ring to ring."
>
>>
>> >> From the looks of it, this series only deals with the end-device
>> >> use-case. Is that right?
>> >
>> > Correct. net/hsr doesn't support this use case right now. It will
>> > stomp the outgoing source MAC with that of the interface for instance.
>>
>> Good to know! When would that behavior be required? Presumably it is not
>> overriding the SA just for fun?
>
> Over the last few weeks I've looked over that code for way longer than
> I'd like to admit and I'm still not sure. As far as I can tell, the
> original authors have disappeared. My guess is it has something to do
> with a configuration in which they had each redundant interface set to
> a different MAC address and wanted the frames to go out with the
> associated MAC address. As far as I can tell this is a violation of
> the spec.

Alright, so maybe a "Works for Me (TM)" solution initially, where any
devince stacking was out of scope.

>>
>> > It also doesn't implement a ProxyNodeTable (though that actually
>> > wouldn't matter if you were offloading to the xrs700x I think). Try
>> > commenting out the ether_addr_copy() line in hsr_xmit and see if it
>> > makes your use case work.
>>
>> So what is missing is basically to expand the current facility for
>> generating sequence numbers to maintain a table of such associations,
>> keyed by the SA?
>
> For the software implementation it would also need to use the
> ProxyNodeTable to prevent forwarding matching frames on the ring and
> delivering them to the hsr master port. It's also supposed to drop
> frames coming in on a redundant port if the source address is in the
> ProxyNodeTable.

This whole thing sounds an awful lot like an FDB. I suppose an option
would be to implement the RedBox/QuadBox roles in the bridge, perhaps by
building on the work done for MRP? Feel free to tell me I'm crazy :)

>>
>> Is the lack of that table the reason for enforcing that the SA match the
>> HSR netdev?
>
> Could be.
>
>>
>> >> I will be targeting a RedBox setup, and I believe that means that the
>> >> remaining port has to be configured as an "interlink". (HSR/PRP is still
>> >> pretty new to me). Is that equivalent to a Linux config like this:
>> >
>> > Depends what you mean by configured as an interlink. I believe bit 9
>> > of HSR_CFG in the switch is only supposed to be used for the HSR-HSR
>> > and HSR-PRP use case, not HSR-SAN.
>>
>> Interesting, section 6.4.1 of the XRS manual states: "The interlink port
>> can be either in HSR, PRP or normal (non-HSR, non-PRP) mode." Maybe the
>> term is overloaded?
>
> Yeah I guess since it's the only port that can be used for QuadBox
> they call it the interlink port even if it doesn't have the HSR/PRP
> mode enabled with the interlink bit set.

Right, that makes sense.

>>
>> >>       br0
>> >>      /   \
>> >>    hsr0   \
>> >>    /  \    \
>> >> swp1 swp2 swp3
>> >>
>> >> Or are there some additional semantics involved in forwarding between
>> >> the redundant ports and the interlink?
>> >
>> > That sounds right.
>> >
>> >>
>> >> The chip is very rigid in the sense that most roles are statically
>> >> allocated to specific ports. I think we need to add checks for this.
>> >
>> > Okay. I'll look into this. Though a lot of the restrictions have to do
>> > with using the third gigabit port for an HSR/PRP interlink (not
>> > HSR-SAN) which I'm not currently supporting anyway.
>>
>> But nothing is stopping me from trying to setup an HSR ring between port
>> (2,3) or (1,3), right? And that is not supported by the chip as I
>> understand it from looking at table 25.
>
> Yeah. That's why I said I'd look into it :). Wasn't an issue for my
> board since port 0 isn't connected and port 3 is used as the CPU
> facing port.

Fair enough :)

>>
>> >> Looking at the packets being generated on the redundant ports, both
>> >> regular traffic and supervision frames seem to be HSR-tagged. Are
>> >> supervision frames not supposed to be sent with an outer ethertype of
>> >> 0x88fb? The manual talks about the possibility of setting up a policy
>> >> entry to bypass HSR-tagging (section 6.1.5), is this what that is for?
>> >
>> > This was changed between 62439-3:2010 and 62439-3:2012.
>> > "Prefixing the supervision frames on HSR by an HSR tag to simplify the hardware
>> > implementation and introduce a unique EtherType for HSR to simplify
>> > processing."
>>
>> Thank you, that would have taken me a long time to figure out :)
>>
>> > The Linux HSR driver calls the former HSR v0 and the later HSR v1. I'm
>> > not sure what their intention was with this feature. The inbound
>> > policies are pretty flexible so maybe they didn't have anything so
>> > specific in mind.
>>
>> Now that I think of it, maybe you want things like LLDP to still operate
>> hop-by-hop over the ring?
>
> Not sure. Would need to look into it.
>
>>
>> > I don't think the xrs7000 series could offload HSR v0 anyway because
>> > the tag ether type is different.
>> >
>> >>
>> >> In the DSA layer (dsa_slave_changeupper), could we merge the two HSR
>> >> join/leave calls somehow? My guess is all drivers are going to end up
>> >> having to do the same dance of deferring configuration until both ports
>> >> are known.
>> >
>> > Describe what you mean a bit more. Do you mean join and leave should
>> > each only be called once with both hsr ports being passed in?
>>
>> Exactly. Maybe we could use `netdev_for_each_lower_dev` to figure out if
>> the other port has already been switched over to the new upper or
>> something. I find it hard to believe that there is any hardware out
>> there that can do something useful with a single HSR/PRP port anyway.
>
> If one port failed maybe it would still be useful to join one port if
> the switch supported it? Maybe this couldn't ever happen anyway due
> the way hsr is designed.
>
> How were you thinking this would work? Would it just not use
> dsa_port_notify() and call a switch op directly after the second
> port's dsa_slave_changeupper() call? Or would we instead keep port

Yeah this is what I was thinking. But I understand where Vladimir is
coming from. Fundamentally, I am also on the library side of the
"library vs. framework" spectrum.

> notifiers and calls to dsa_switch_hsr_join for each port and just make
> dsa_switch_hsr_join() not call the switch op to create the HSR until
> the second port called it? I'm not all that familiar with how these
> dsa notifiers work and would prefer to stick with using a similar
> mechanism to the bridge and lag support. It would be nice to get some
> feedback from the DSA maintainers on how they would prefer it to work
> if they indeed had a preference at all.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Vladimir Oltean]

On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote:
> On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote:
> >> > It also doesn't implement a ProxyNodeTable (though that actually
> >> > wouldn't matter if you were offloading to the xrs700x I think). Try
> >> > commenting out the ether_addr_copy() line in hsr_xmit and see if it
> >> > makes your use case work.
> >>
> >> So what is missing is basically to expand the current facility for
> >> generating sequence numbers to maintain a table of such associations,
> >> keyed by the SA?
> >
> > For the software implementation it would also need to use the
> > ProxyNodeTable to prevent forwarding matching frames on the ring and
> > delivering them to the hsr master port. It's also supposed to drop
> > frames coming in on a redundant port if the source address is in the
> > ProxyNodeTable.
> 
> This whole thing sounds an awful lot like an FDB. I suppose an option
> would be to implement the RedBox/QuadBox roles in the bridge, perhaps by
> building on the work done for MRP? Feel free to tell me I'm crazy :)

As far as I understand, the VDAN needs to generate supervision frames on
behalf of all nodes that it proxies. Therefore, implementing the
RedBox/QuadBox in the bridge is probably not practical. What I was
discussing with George though is that maybe we can make hsr a consumer
of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its
assisted_learning_on_cpu_port functionality, and that would be how it
populates its proxy node table. A RedBox becomes a bridge with one HSR
interface and one or more standalone interfaces, and a QuadBox becomes a
bridge with two HSR interfaces. How does that sound?

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Tobias Waldekranz]

On Wed, Feb 10, 2021 at 23:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote:
>> On Tue, Feb 09, 2021 at 11:04, George McCollister <george.mccollister@gmail.com> wrote:
>> >> > It also doesn't implement a ProxyNodeTable (though that actually
>> >> > wouldn't matter if you were offloading to the xrs700x I think). Try
>> >> > commenting out the ether_addr_copy() line in hsr_xmit and see if it
>> >> > makes your use case work.
>> >>
>> >> So what is missing is basically to expand the current facility for
>> >> generating sequence numbers to maintain a table of such associations,
>> >> keyed by the SA?
>> >
>> > For the software implementation it would also need to use the
>> > ProxyNodeTable to prevent forwarding matching frames on the ring and
>> > delivering them to the hsr master port. It's also supposed to drop
>> > frames coming in on a redundant port if the source address is in the
>> > ProxyNodeTable.
>> 
>> This whole thing sounds an awful lot like an FDB. I suppose an option
>> would be to implement the RedBox/QuadBox roles in the bridge, perhaps by
>> building on the work done for MRP? Feel free to tell me I'm crazy :)
>
> As far as I understand, the VDAN needs to generate supervision frames on
> behalf of all nodes that it proxies. Therefore, implementing the
> RedBox/QuadBox in the bridge is probably not practical. What I was
> discussing with George though is that maybe we can make hsr a consumer
> of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its
> assisted_learning_on_cpu_port functionality, and that would be how it
> populates its proxy node table.

Is it not easier to just implement learning in the HSR layer? Seeing as
you need to look up the table for each packet anyway, you might as well
add a new entry on a miss. Otherwise you run the risk of filling up your
proxy table with entries that never egress the HSR device. Perhaps not
likely on this particular device, but on a 48-port switch with HSR
offloading it might be.

This should also work for more exotic configs with multiple macvlans for
example:

macvlan0 macvlan1
      \  /
      hsr0
      /  \
   swp1  swp2

> A RedBox becomes a bridge with one HSR
> interface and one or more standalone interfaces, and a QuadBox becomes a
> bridge with two HSR interfaces. How does that sound?

Yeah that is the straight forward solution, and what I tried to describe
earlier in the thread with this illustration:

     >> >>       br0
     >> >>      /   \
     >> >>    hsr0   \
     >> >>    /  \    \
     >> >> swp1 swp2 swp3

I just wanted to double check that we had not overlooked a better
solution outside of the existing HSR code.

Re: [PATCH net-next v2 0/4] add HSR offloading support for DSA switches

[Vladimir Oltean]

On Sat, Feb 13, 2021 at 12:52:36AM +0100, Tobias Waldekranz wrote:
> On Wed, Feb 10, 2021 at 23:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Wed, Feb 10, 2021 at 10:10:14PM +0100, Tobias Waldekranz wrote:
> >> This whole thing sounds an awful lot like an FDB. I suppose an option
> >> would be to implement the RedBox/QuadBox roles in the bridge, perhaps by
> >> building on the work done for MRP? Feel free to tell me I'm crazy :)
> >
> > As far as I understand, the VDAN needs to generate supervision frames on
> > behalf of all nodes that it proxies. Therefore, implementing the
> > RedBox/QuadBox in the bridge is probably not practical. What I was
> > discussing with George though is that maybe we can make hsr a consumer
> > of SWITCHDEV_FDB_ADD_TO_DEVICE events, similar to DSA with its
> > assisted_learning_on_cpu_port functionality, and that would be how it
> > populates its proxy node table.
> 
> Is it not easier to just implement learning in the HSR layer? Seeing as
> you need to look up the table for each packet anyway, you might as well
> add a new entry on a miss. Otherwise you run the risk of filling up your
> proxy table with entries that never egress the HSR device. Perhaps not
> likely on this particular device, but on a 48-port switch with HSR
> offloading it might be.

In the HSR layer, sure, I didn't mean to suggest otherwise, I thought
you wanted to, when you said "I suppose an option would be to implement
the RedBox/QuadBox roles in the bridge".

So then the SWITCHDEV_FDB_ADD_TO_DEVICE events might be too much.
Learning / populating the proxy node table can be probably done from the
xmit function, with the only potential issue that the first packets will
probably be lost, since no supervision frames have yet been transmitted
for those proxied nodes.

> This should also work for more exotic configs with multiple macvlans for
> example:
> 
> macvlan0 macvlan1
>       \  /
>       hsr0
>       /  \
>    swp1  swp2

Yes, I don't think macvlan uses switchdev.

> > A RedBox becomes a bridge with one HSR
> > interface and one or more standalone interfaces, and a QuadBox becomes a
> > bridge with two HSR interfaces. How does that sound?
> 
> Yeah that is the straight forward solution, and what I tried to describe
> earlier in the thread with this illustration:
> 
>      >> >>       br0
>      >> >>      /   \
>      >> >>    hsr0   \
>      >> >>    /  \    \
>      >> >> swp1 swp2 swp3
> 
> I just wanted to double check that we had not overlooked a better
> solution outside of the existing HSR code.

I'm not aware of a better solution, but I'm also interested if there is one.