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/ (local)

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