[PATCH v3 RFC 0/4] net: dsa: hsr: Enable HSR HW offloading for KSZ9477

This patch series provides support for HSR HW offloading in KSZ9477
switch IC.

To test this feature:
ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
ip link set dev lan1 up
ip link set dev lan2 up
ip a add 192.168.0.1/24 dev hsr0
ip link set dev hsr0 up

To remove HSR network device:
ip link del hsr0

Test HW:
Two KSZ9477-EVB boards with HSR ports set to "Port1" and "Port2".

Performance SW used:
nuttcp -S --nofork
nuttcp -vv -T 60 -r 192.168.0.2
nuttcp -vv -T 60 -t 192.168.0.2

Code: v6.5-rc7 Linux repository
Tested HSR v0 and v1
Results:
With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
With no offloading 		       RX: 63 Mbps  TX: 63 Mbps


Lukasz Majewski (4):
  net: dsa: Extend the ksz_device structure to hold info about HSR ports
  net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication
  net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading
  net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

 drivers/net/dsa/microchip/ksz9477.c    | 103 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h    |   4 +
 drivers/net/dsa/microchip/ksz_common.c |  81 +++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   3 +
 include/linux/dsa/ksz_common.h         |   1 +
 net/dsa/tag_ksz.c                      |   5 ++
 6 files changed, 197 insertions(+)

-- 
2.20.1

Information about HSR aware ports in a DSA switch can be helpful when
one needs tags to be adjusted before the HSR frame is sent.

For example - with ksz9477 switch - the TAG needs to be adjusted to have
both HSR ports marked in tag to allow execution of HW based frame
duplication.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use struct ksz_device to store hsr_ports variable

Changes for v3:
- None
---
 drivers/net/dsa/microchip/ksz_common.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a4de58847dea..9fcafff0c01d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h

@@ -158,6 +158,9 @@ struct ksz_device {
 	bool synclko_125;
 	bool synclko_disable;
 
+	/* Bitmask indicating ports supporting HSR */
+	u16 hsr_ports;
+
 	struct vlan_table *vlan_cache;
 
 	struct ksz_port *ports;

-- 
2.20.1

The KSZ9477 has support for HSR (High-Availability Seamless Redundancy).
One of its offloading (i.e. performed in the switch IC hardware) features
is to duplicate received frame to both HSR aware switch ports.

To achieve this goal - the tail TAG needs to be modified. To be more
specific, both ports must be marked as destination (egress) ones.

Moreover, according to AN3474 application note, the lookup bit (10)
should not be set in the tail tag.

Last but not least - the NETIF_F_HW_HSR_DUP flag indicates that the device
supports HSR and assures (in HSR core code) that frame is sent only once
from HOST to switch with tail tag indicating both ports.

Information about bits to be set in tag is provided via KSZ generic
ksz_hsr_get_ports() function.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use ksz_hsr_get_ports() to obtain the bits values corresponding to
  HSR aware ports

Changes for v3:
- None
---
 drivers/net/dsa/microchip/ksz_common.c | 12 ++++++++++++
 include/linux/dsa/ksz_common.h         |  1 +
 net/dsa/tag_ksz.c                      |  5 +++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index d9d843efd111..579fde54d1e1 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c

@@ -3421,6 +3421,18 @@ static int ksz_setup_tc(struct dsa_switch *ds, int port,
 	}
 }
 
+u16 ksz_hsr_get_ports(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		return dev->hsr_ports;
+	}
+
+	return 0;
+}
+
 static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_tag_protocol	= ksz_get_tag_protocol,
 	.connect_tag_protocol   = ksz_connect_tag_protocol,

diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index 576a99ca698d..fa3d9b0f3a72 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h

@@ -50,4 +50,5 @@ ksz_tagger_data(struct dsa_switch *ds)
 	return ds->tagger_data;
 }
 
+u16 ksz_hsr_get_ports(struct dsa_switch *ds);
 #endif /* _NET_DSA_KSZ_COMMON_H_ */

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index ea100bd25939..903db95c37ee 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c

@@ -293,6 +293,11 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 	if (is_link_local_ether_addr(hdr->h_dest))
 		val |= KSZ9477_TAIL_TAG_OVERRIDE;
 
+	if (dev->features & NETIF_F_HW_HSR_DUP) {
+		val &= ~KSZ9477_TAIL_TAG_LOOKUP;
+		val |= ksz_hsr_get_ports(dp->ds);
+	}
+
 	*tag = cpu_to_be16(val);
 
 	return ksz_defer_xmit(dp, skb);

-- 
2.20.1

This patch provides the common KSZ (i.e. Microchip) DSA code with support
for HSR aware devices.

To be more specific - generic ksz_hsr_{join|leave} functions are provided,
now only supporting KSZ9477 IC.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- None

Changes for v3:
- Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be caught)
---
 drivers/net/dsa/microchip/ksz_common.c | 69 ++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 579fde54d1e1..91d1acaf4494 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c

@@ -16,6 +16,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
+#include <linux/if_hsr.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/of_mdio.h>

@@ -3433,6 +3434,72 @@ u16 ksz_hsr_get_ports(struct dsa_switch *ds)
 	return 0;
 }
 
+static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr)
+{
+	struct dsa_port *partner = NULL, *dp;
+	struct ksz_device *dev = ds->priv;
+	enum hsr_version ver;
+	int ret;
+
+	ret = hsr_get_version(hsr, &ver);
+	if (ret)
+		return ret;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		if (!(ver == HSR_V0 || ver == HSR_V1))
+			return -EOPNOTSUPP;
+	}
+
+	/* We can't enable redundancy on the switch until both
+	 * redundant ports have signed up.
+	 */
+	dsa_hsr_foreach_port(dp, ds, hsr) {
+		if (dp->index != port) {
+			partner = dp;
+			break;
+		}
+	}
+
+	if (!partner)
+		return 0;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		return ksz9477_hsr_join(ds, port, hsr, partner);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int ksz_hsr_leave(struct dsa_switch *ds, int port,
+			 struct net_device *hsr)
+{
+	struct dsa_port *partner = NULL, *dp;
+	struct ksz_device *dev = ds->priv;
+
+	dsa_hsr_foreach_port(dp, ds, hsr) {
+		if (dp->index != port) {
+			partner = dp;
+			break;
+		}
+	}
+
+	if (!partner)
+		return 0;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		return ksz9477_hsr_leave(ds, port, hsr, partner);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_tag_protocol	= ksz_get_tag_protocol,
 	.connect_tag_protocol   = ksz_connect_tag_protocol,

@@ -3452,6 +3519,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_sset_count		= ksz_sset_count,
 	.port_bridge_join	= ksz_port_bridge_join,
 	.port_bridge_leave	= ksz_port_bridge_leave,
+	.port_hsr_join		= ksz_hsr_join,
+	.port_hsr_leave		= ksz_hsr_leave,
 	.port_stp_state_set	= ksz_port_stp_state_set,
 	.port_pre_bridge_flags	= ksz_port_pre_bridge_flags,
 	.port_bridge_flags	= ksz_port_bridge_flags,

-- 
2.20.1

This patch adds functions for providing in KSZ9477 switch HSR
(High-availability Seamless Redundancy) hardware offloading.

According to AN3474 application note following features are provided:
- TX packet duplication from host to switch (NETIF_F_HW_HSR_DUP)
- RX packet duplication discarding
- Prevention of packet loop

For last two ones - there is a probability that some packets will not
be filtered in HW (in some special cases). Hence, the HSR core code
shall be used to discard those not caught frames.

Moreover, some switch registers adjustments are required - like setting
MAC address of HSR network interface.

Additionally, the KSZ9477 switch has been configured to forward frames
between HSR ports (1,2) members.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use struct ksz_device to store hsr ports information (not struct dsa)

Changes for v3:
- Enable in-switch forwarding of frames between HSR ports (i.e. enable
  bridging of those two ports)

- The NETIF_F_HW_HSR_FWD flag has been marked as supported by the HSR
  network device

- Remove ETH MAC address validity check as it is done earlier in the net
  driver

- Add comment regarding adding support for NETIF_F_HW_HSR_FWD flag
---
 drivers/net/dsa/microchip/ksz9477.c | 103 ++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477.h |   4 ++
 2 files changed, 107 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 83b7f2d5c1ea..c4ed89c1de48 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c

@@ -1141,6 +1141,109 @@ int ksz9477_tc_cbs_set_cinc(struct ksz_device *dev, int port, u32 val)
 	return ksz_pwrite16(dev, port, REG_PORT_MTI_CREDIT_INCREMENT, val);
 }
 
+/* The KSZ9477 provides following HW features to accelerate
+ * HSR frames handling:
+ *
+ * 1. TX PACKET DUPLICATION FROM HOST TO SWITCH
+ * 2. RX PACKET DUPLICATION DISCARDING
+ * 3. PREVENTING PACKET LOOP IN THE RING BY SELF-ADDRESS FILTERING
+ *
+ * Only one from point 1. has the NETIF_F* flag available.
+ *
+ * Ones from point 2 and 3 are "best effort" - i.e. those will
+ * work correctly most of the time, but it may happen that some
+ * frames will not be caught. Hence, the SW needs to handle those
+ * special cases. However, the speed up gain is considerable when
+ * above features are used.
+ *
+ * Moreover, the NETIF_F_HW_HSR_FWD feature is also enabled, as HSR frames
+ * can be forwarded in the switch fabric between HSR ports.
+ */
+#define KSZ9477_SUPPORTED_HSR_FEATURES (NETIF_F_HW_HSR_DUP | NETIF_F_HW_HSR_FWD)
+
+int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
+		     struct dsa_port *partner)
+{
+	struct ksz_device *dev = ds->priv;
+	struct net_device *slave;
+	u8 i, data;
+	int ret;
+
+	/* Program which ports shall support HSR */
+	dev->hsr_ports = BIT(port) | BIT(partner->index);
+	ksz_write32(dev, REG_HSR_PORT_MAP__4, dev->hsr_ports);
+
+	/* Forward frames between HSR ports (i.e. bridge together HSR ports) */
+	ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports,
+		   dev->hsr_ports);
+	ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4,
+		   dev->hsr_ports, dev->hsr_ports);
+
+	/* Enable discarding of received HSR frames */
+	ksz_read8(dev, REG_HSR_ALU_CTRL_0__1, &data);
+	data |= HSR_DUPLICATE_DISCARD;
+	data &= ~HSR_NODE_UNICAST;
+	ksz_write8(dev, REG_HSR_ALU_CTRL_0__1, data);
+
+	/* Self MAC address filtering for HSR frames to avoid
+	 * traverse of the HSR ring more than once.
+	 *
+	 * The HSR port (i.e. hsr0) MAC address is used.
+	 */
+	for (i = 0; i < ETH_ALEN; i++) {
+		ret = ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, hsr->dev_addr[i]);
+		if (ret)
+			return ret;
+	}
+
+	/* Enable global self-address filtering if not yet done during switch
+	 * start
+	 */
+	ksz_read8(dev, REG_SW_LUE_CTRL_1, &data);
+	if (!(data & SW_SRC_ADDR_FILTER)) {
+		data |= SW_SRC_ADDR_FILTER;
+		ksz_write8(dev, REG_SW_LUE_CTRL_1, data);
+	}
+
+	/* Enable per port self-address filtering */
+	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, true);
+	ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL,
+		     PORT_SRC_ADDR_FILTER, true);
+
+	/* Setup HW supported features for lan HSR ports */
+	slave = dsa_to_port(ds, port)->slave;
+	slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES;
+
+	slave = dsa_to_port(ds, partner->index)->slave;
+	slave->features |= KSZ9477_SUPPORTED_HSR_FEATURES;
+
+	pr_debug("%s: HSR join port: %d partner: %d port_map: 0x%x\n", __func__,
+		 port, partner->index, dev->hsr_ports);
+
+	return 0;
+}
+
+int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr,
+		      struct dsa_port *partner)
+{
+	struct ksz_device *dev = ds->priv;
+
+	/* Clear ports HSR support */
+	ksz_write32(dev, REG_HSR_PORT_MAP__4, 0);
+
+	/* Disable forwarding frames between HSR ports */
+	ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0);
+	ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4,
+		   dev->hsr_ports, 0);
+
+	/* Disable per port self-address filtering */
+	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false);
+	ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL,
+		     PORT_SRC_ADDR_FILTER, false);
+
+	return 0;
+}
+
 int ksz9477_switch_init(struct ksz_device *dev)
 {
 	u8 data8;

diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index b6f7e3c46e3f..634262efb73c 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h

@@ -58,5 +58,9 @@ int ksz9477_dsa_init(struct ksz_device *dev);
 int ksz9477_switch_init(struct ksz_device *dev);
 void ksz9477_switch_exit(struct ksz_device *dev);
 void ksz9477_port_queue_split(struct ksz_device *dev, int port);
+int ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
+		     struct dsa_port *partner);
+int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr,
+		      struct dsa_port *partner);
 
 #endif

-- 
2.20.1

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:

This conflicts with commit f44a90104ee5 ("net: dsa: Explicitly include
correct DT includes") from July, merged through net-next.

"New features" material for networking goes through this tree, please
submit patches that were formatted (and tested) on top of the most
recent version of the "main" branch, and use git-send-email
--subject-prefix "[RFC PATCH vN net-next]" to denote that.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/

If patches fail to apply to the target kernel, you lose the benefit of
automatic build testing (which would have highlighted a problem that
exists since v3). With RFC patches, the kbuild test robot sends build
breakage reports only to you - with normal patches it sends them to
everybody.

Please wait for more feedback before posting RFC v5. I will review in
more detail, but it will take some time.

Thanks.

Hi Vladimir,

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:

quoted

This patch provides the common KSZ (i.e. Microchip) DSA code with
support for HSR aware devices.

To be more specific - generic ksz_hsr_{join|leave} functions are
provided, now only supporting KSZ9477 IC.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- None

Changes for v3:
- Do not return -EOPNOTSUPP for only PRP_V1 (as v2 will not be
caught) ---
 drivers/net/dsa/microchip/ksz_common.c | 69
++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c

b/drivers/net/dsa/microchip/ksz_common.c index
579fde54d1e1..91d1acaf4494 100644 ---
a/drivers/net/dsa/microchip/ksz_common.c +++
b/drivers/net/dsa/microchip/ksz_common.c @@ -16,6 +16,7 @@
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
+#include <linux/if_hsr.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/of_mdio.h>  

This conflicts with commit f44a90104ee5 ("net: dsa: Explicitly include
correct DT includes") from July, merged through net-next.

"New features" material for networking goes through this tree, please
submit patches that were formatted (and tested) on top of the most
recent version of the "main" branch, and use git-send-email
--subject-prefix "[RFC PATCH vN net-next]" to denote that.

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/log/

If patches fail to apply to the target kernel, you lose the benefit of
automatic build testing (which would have highlighted a problem that
exists since v3). With RFC patches, the kbuild test robot sends build
breakage reports only to you - with normal patches it sends them to
everybody.

Thanks for the info.

Please wait for more feedback before posting RFC v5. I will review in
more detail, but it will take some time.

Ok. I will wait for your feedback and then sent RFC v4.

Thanks.

Best regards,

Lukasz Majewski

--

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

On Mon, Sep 04, 2023 at 02:02:07PM +0200, Lukasz Majewski wrote:

When CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m:

ld.lld: error: undefined symbol: ksz_hsr_get_ports
referenced by tag_ksz.c:298 (/opt/net-next/output-arm64-clang/../net/dsa/tag_ksz.c:298)
              net/dsa/tag_ksz.o:(ksz9477_xmit) in archive vmlinux.a

But before you rush to add EXPORT_SYMBOL_GPL(ksz_hsr_get_ports), be aware
that due to DSA's design, tag_ksz.ko and ksz_common.ko cannot have any
symbol dependency on each other, and if you do that, you will break
module auto-loading. More information here, there were also patches that
removed those dependencies for other tagger/switch driver pairs:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

Not to mention that there are other problems with the "dev->hsr_ports"
concept. For example, having a hsr0 over lan0 and lan1, and a hsr1 over
lan2 and lan3, would set dev->hsr_ports to GENMASK(3, 0). But you want
an xmit coming from hsr0 to get sent only to GENMASK(1, 0), and an xmit
from hsr1 only to GENMASK(3, 2).

In this particular case, the best option seems to be to delete ksz_hsr_get_ports().

No need to unset a bit which was never set.

+		val |= ksz_hsr_get_ports(dp->ds);
+	}

Would this work instead?

	struct net_device *hsr_dev = dp->hsr_dev;
	struct dsa_port *other_dp;

	dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
		val |= BIT(other_dp->index);

+
 	*tag = cpu_to_be16(val);
 
 	return ksz_defer_xmit(dp, skb);
-- 
2.20.1

On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote:

quoted

Not to mention that there are other problems with the "dev->hsr_ports"
concept. For example, having a hsr0 over lan0 and lan1, and a hsr1
over lan2 and lan3, would set dev->hsr_ports to GENMASK(3, 0).

I doubt that having two hsr{01} interfaces is possible with current
kernel.

You mean 2 hsr{01} interfaces not being able to coexist in general,
or just "offloaded" ones?

The KSZ9477 allows only to have 2 ports of 5 available as HSR
ones.

The same is with earlier chip xrs700x (but this have even bigger
constrain - there only ports 1 and 2 can support HSR). 

quoted

quoted

+	if (dev->features & NETIF_F_HW_HSR_DUP) {
+		val &= ~KSZ9477_TAIL_TAG_LOOKUP;  

No need to unset a bit which was never set.

I've explicitly followed the vendor's guidelines - the TAG_LOOKUP needs
to be cleared.

But if we can assure that it is not set here I can remove it.

Let's look at ksz9477_xmit(), filtering only for changes to "u16 val".

static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
				    struct net_device *dev)
{
	u16 val;

	val = BIT(dp->index);

	val |= FIELD_PREP(KSZ9477_TAIL_TAG_PRIO, prio);

	if (is_link_local_ether_addr(hdr->h_dest))
		val |= KSZ9477_TAIL_TAG_OVERRIDE;

	if (dev->features & NETIF_F_HW_HSR_DUP) {
		val &= ~KSZ9477_TAIL_TAG_LOOKUP;
		val |= ksz_hsr_get_ports(dp->ds);
	}
}

Is KSZ9477_TAIL_TAG_LOOKUP ever set in "val", or am I missing something?

quoted

quoted

+		val |= ksz_hsr_get_ports(dp->ds);
+	}  

Would this work instead?

	struct net_device *hsr_dev = dp->hsr_dev;
	struct dsa_port *other_dp;

	dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
		val |= BIT(other_dp->index);

I thought about this solution as well, but I've been afraid, that going
through the loop of all 5 ports each time we want to send single packet
will reduce the performance.

Hence, the idea with having the "hsr_ports" set once during join
function and then use this cached value afterwards.

There was a quote about "premature optimization" which I can't quite remember...

If you can see a measurable performance difference, then the list
traversal can be converted to something more efficient.

In this case, struct dsa_port :: hsr_dev can be converted to a larger
struct dsa_hsr structure, similar to struct dsa_port :: bridge.
That structure could look like this:

struct dsa_hsr {
	struct net_device *dev;
	unsigned long port_mask;
	refcount_t refcount;
};

and you could replace the list traversal with "val |= dp->hsr->port_mask".
But a more complex solution requires a justification, which in this case
is performance-related. So performance data must be gathered.

FWIW, dsa_master_find_slave() also performs a list traversal.
But similar discussions about performance improvements didn't lead anywhere.

Hi Vladimir,

On Mon, Sep 04, 2023 at 02:02:07PM +0200, Lukasz Majewski wrote:

quoted

The KSZ9477 has support for HSR (High-Availability Seamless
Redundancy). One of its offloading (i.e. performed in the switch IC
hardware) features is to duplicate received frame to both HSR aware
switch ports.

To achieve this goal - the tail TAG needs to be modified. To be more
specific, both ports must be marked as destination (egress) ones.

Moreover, according to AN3474 application note, the lookup bit (10)
should not be set in the tail tag.

Last but not least - the NETIF_F_HW_HSR_DUP flag indicates that the
device supports HSR and assures (in HSR core code) that frame is
sent only once from HOST to switch with tail tag indicating both
ports.

Information about bits to be set in tag is provided via KSZ generic
ksz_hsr_get_ports() function.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Use ksz_hsr_get_ports() to obtain the bits values corresponding to
  HSR aware ports

Changes for v3:
- None
---
 drivers/net/dsa/microchip/ksz_common.c | 12 ++++++++++++
 include/linux/dsa/ksz_common.h         |  1 +
 net/dsa/tag_ksz.c                      |  5 +++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c

b/drivers/net/dsa/microchip/ksz_common.c index
d9d843efd111..579fde54d1e1 100644 ---
a/drivers/net/dsa/microchip/ksz_common.c +++
b/drivers/net/dsa/microchip/ksz_common.c @@ -3421,6 +3421,18 @@
static int ksz_setup_tc(struct dsa_switch *ds, int port, }
 }
 
+u16 ksz_hsr_get_ports(struct dsa_switch *ds)
+{
+	struct ksz_device *dev = ds->priv;
+
+	switch (dev->chip_id) {
+	case KSZ9477_CHIP_ID:
+		return dev->hsr_ports;
+	}
+
+	return 0;
+}  

When CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m:

ld.lld: error: undefined symbol: ksz_hsr_get_ports
referenced by tag_ksz.c:298
(/opt/net-next/output-arm64-clang/../net/dsa/tag_ksz.c:298)
net/dsa/tag_ksz.o:(ksz9477_xmit) in archive vmlinux.a

But before you rush to add EXPORT_SYMBOL_GPL(ksz_hsr_get_ports), be
aware that due to DSA's design, tag_ksz.ko and ksz_common.ko cannot
have any symbol dependency on each other, and if you do that, you
will break module auto-loading. More information here, there were
also patches that removed those dependencies for other tagger/switch
driver pairs:
https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/

Ok. I will look on that

Not to mention that there are other problems with the "dev->hsr_ports"
concept. For example, having a hsr0 over lan0 and lan1, and a hsr1
over lan2 and lan3, would set dev->hsr_ports to GENMASK(3, 0).

I doubt that having two hsr{01} interfaces is possible with current
kernel.

The KSZ9477 allows only to have 2 ports of 5 available as HSR
ones.

The same is with earlier chip xrs700x (but this have even bigger
constrain - there only ports 1 and 2 can support HSR). 

But
you want an xmit coming from hsr0 to get sent only to GENMASK(1, 0),
and an xmit from hsr1 only to GENMASK(3, 2).

In this particular case, the best option seems to be to delete
ksz_hsr_get_ports().

Please see my below comment.

quoted

+
 static const struct dsa_switch_ops ksz_switch_ops = {
 	.get_tag_protocol	= ksz_get_tag_protocol,
 	.connect_tag_protocol   = ksz_connect_tag_protocol,

diff --git a/include/linux/dsa/ksz_common.h

b/include/linux/dsa/ksz_common.h index 576a99ca698d..fa3d9b0f3a72
100644 --- a/include/linux/dsa/ksz_common.h

+++ b/include/linux/dsa/ksz_common.h

@@ -50,4 +50,5 @@ ksz_tagger_data(struct dsa_switch *ds)
 	return ds->tagger_data;
 }
 
+u16 ksz_hsr_get_ports(struct dsa_switch *ds);
 #endif /* _NET_DSA_KSZ_COMMON_H_ */

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index ea100bd25939..903db95c37ee 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c

@@ -293,6 +293,11 @@ static struct sk_buff *ksz9477_xmit(struct

sk_buff *skb, if (is_link_local_ether_addr(hdr->h_dest))
 		val |= KSZ9477_TAIL_TAG_OVERRIDE;
 
+	if (dev->features & NETIF_F_HW_HSR_DUP) {
+		val &= ~KSZ9477_TAIL_TAG_LOOKUP;  

No need to unset a bit which was never set.

I've explicitly followed the vendor's guidelines - the TAG_LOOKUP needs
to be cleared.

But if we can assure that it is not set here I can remove it.

quoted

+		val |= ksz_hsr_get_ports(dp->ds);
+	}  

Would this work instead?

	struct net_device *hsr_dev = dp->hsr_dev;
	struct dsa_port *other_dp;

	dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
		val |= BIT(other_dp->index);

I thought about this solution as well, but I've been afraid, that going
through the loop of all 5 ports each time we want to send single packet
will reduce the performance.

Hence, the idea with having the "hsr_ports" set once during join
function and then use this cached value afterwards.

quoted

+
 	*tag = cpu_to_be16(val);
 
 	return ksz_defer_xmit(dp, skb);
-- 
2.20.1
  

Best regards,

Lukasz Majewski

--

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

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:

It's hard for me to put this in the proper perspective in this email,
since ksz9477_hsr_leave() is implemented in a different patch, so I'm
just going to reproduce it here:

int ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr,
		      struct dsa_port *partner)
{
	struct ksz_device *dev = ds->priv;

	/* Clear ports HSR support */
	ksz_write32(dev, REG_HSR_PORT_MAP__4, 0);

	/* Disable forwarding frames between HSR ports */
	ksz_prmw32(dev, port, REG_PORT_VLAN_MEMBERSHIP__4, dev->hsr_ports, 0);
	ksz_prmw32(dev, partner->index, REG_PORT_VLAN_MEMBERSHIP__4,
		   dev->hsr_ports, 0);

	/* Disable per port self-address filtering */
	ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL, PORT_SRC_ADDR_FILTER, false);
	ksz_port_cfg(dev, partner->index, REG_PORT_LUE_CTRL,
		     PORT_SRC_ADDR_FILTER, false);

	return 0;
}

The code pattern from ksz_hsr_leave() is to disable HSR offload in both
member ports, after *both* member ports have left the HSR device, correct?

So it means that after this set of commands:

ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1
ip link set dev lan1 up
ip link set dev lan2 up
ip link set lan1 nomaster

lan1 will still have HSR offload enabled, and forwarding enabled towards
lan2, correct? That's not good, because lan1 is now a standalone port
and should operate as such.

Hi Vladimir,

On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote:

quoted

quoted

Not to mention that there are other problems with the
"dev->hsr_ports" concept. For example, having a hsr0 over lan0
and lan1, and a hsr1 over lan2 and lan3, would set dev->hsr_ports
to GENMASK(3, 0).  

I doubt that having two hsr{01} interfaces is possible with current
kernel.  

You mean 2 hsr{01} interfaces not being able to coexist in general,
or just "offloaded" ones?

The KSZ9477 IC only allows to have two its ports from 5 available to be
configured as HSR ones (so the HW offloading would work).

And having single hsr0 with lan[12] is the used case on which I'm
focused (with offloading or pure SW).

quoted

The KSZ9477 allows only to have 2 ports of 5 available as HSR
ones.

The same is with earlier chip xrs700x (but this have even bigger
constrain - there only ports 1 and 2 can support HSR).   

quoted

quoted

quoted

+	if (dev->features & NETIF_F_HW_HSR_DUP) {
+		val &= ~KSZ9477_TAIL_TAG_LOOKUP;    

No need to unset a bit which was never set.  

I've explicitly followed the vendor's guidelines - the TAG_LOOKUP
needs to be cleared.

But if we can assure that it is not set here I can remove it.  

Let's look at ksz9477_xmit(), filtering only for changes to "u16 val".

static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
				    struct net_device *dev)
{
	u16 val;

	val = BIT(dp->index);

	val |= FIELD_PREP(KSZ9477_TAIL_TAG_PRIO, prio);

	if (is_link_local_ether_addr(hdr->h_dest))
		val |= KSZ9477_TAIL_TAG_OVERRIDE;

	if (dev->features & NETIF_F_HW_HSR_DUP) {
		val &= ~KSZ9477_TAIL_TAG_LOOKUP;
		val |= ksz_hsr_get_ports(dp->ds);
	}
}

Is KSZ9477_TAIL_TAG_LOOKUP ever set in "val", or am I missing
something?

No, it looks like you are not. The clearance of KSZ9477_TAIL_TAG_LOOKUP
seems to be an overkill.

quoted

quoted

quoted

+		val |= ksz_hsr_get_ports(dp->ds);
+	}    

Would this work instead?

	struct net_device *hsr_dev = dp->hsr_dev;
	struct dsa_port *other_dp;

	dsa_hsr_foreach_port(other_dp, dp->ds, hsr_dev)
		val |= BIT(other_dp->index);
  

I thought about this solution as well, but I've been afraid, that
going through the loop of all 5 ports each time we want to send
single packet will reduce the performance.

Hence, the idea with having the "hsr_ports" set once during join
function and then use this cached value afterwards.  

There was a quote about "premature optimization" which I can't quite
remember...

Yes, using caching by default instead of list iterating is the
"premature optimization" .... :-)

If you can see a measurable performance difference, then the list
traversal can be converted to something more efficient.

In this case, struct dsa_port :: hsr_dev can be converted to a larger
struct dsa_hsr structure, similar to struct dsa_port :: bridge.
That structure could look like this:

struct dsa_hsr {
	struct net_device *dev;
	unsigned long port_mask;
	refcount_t refcount;
};

and you could replace the list traversal with "val |=
dp->hsr->port_mask". But a more complex solution requires a
justification, which in this case is performance-related. So
performance data must be gathered.

FWIW, dsa_master_find_slave() also performs a list traversal.
But similar discussions about performance improvements didn't lead
anywhere.

The iteration over hsr ports would simplify the code. I will use it and
provide feedback if I find performance drop.

Thanks for the feedback.


Best regards,

Lukasz Majewski

--

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