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

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

[Lukasz Majewski]

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

[PATCH v3 RFC 1/4] net: dsa: Extend the ksz_device structure to hold info about HSR ports

[Lukasz Majewski]

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

[PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Lukasz Majewski]

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

Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 02:02:07PM +0200, Lukasz Majewski wrote:
> 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/

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().

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

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

Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Lukasz Majewski]

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

Hi Vladimir,

> On Mon, Sep 04, 2023 at 02:02:07PM +0200, Lukasz Majewski wrote:
> > 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.

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

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

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

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

Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote:
> > 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). 

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

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

Re: [PATCH v3 RFC 2/4] net: dsa: Extend ksz9477 TAG setup to support HSR frames duplication

[Lukasz Majewski]

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

Hi Vladimir,

> On Tue, Sep 05, 2023 at 12:44:09PM +0200, Lukasz Majewski wrote:
> > > 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).

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

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

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

[PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Lukasz Majewski]

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

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 02:02:08PM +0200, Lukasz Majewski wrote:
> 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.

How do these 2 concepts (autonomous forwarding + software-based
elimination of some frames) work together? If software is not the sole
receiver of traffic which needs to be filtered further, and duplicates
also get forwarded to the network, does this not break the HSR ring?

What are the causes due to which self-address filtering and duplicate
elimination only work "most of the time"?

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

Call ksz9477_cfg_port_member() instead?

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

FWIW: https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/
Some coordination will be required regarding the MAC address that the
switch driver needs to program to these registers. It seems that it is
not single purpose.

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

If there is no way that SW_SRC_ADDR_FILTER can be unset after
ksz9477_reset_switch() is called, then this is dead code which should be
removed.

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

Can the code that is duplicated for the partner port be moved to the
caller?

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

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Lukasz Majewski]

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

Hi Vladimir,

> On Mon, Sep 04, 2023 at 02:02:08PM +0200, Lukasz Majewski wrote:
> > 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.  
> 
> How do these 2 concepts (autonomous forwarding + software-based
> elimination of some frames) work together? If software is not the sole
> receiver of traffic which needs to be filtered further, and duplicates
> also get forwarded to the network, does this not break the HSR ring?
> 

Autonomous forwarding is based on KSZ9477, having the HSR ports
"bridged" to send frames between them.

Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX
packet duplication discarding which will discard duplicated frames.

Last but not least the - packet loop prevention.

My understanding is as follows:

1. RX packet duplication removes copy of a frame, which is addressed to
cpu port of switch.

2. The "bridge" of HSR passes frames in-KSZ9477, which are not
addressed to this cpu host (between other HSR nodes).

3. Packet loop prevention - the HSR packet with SA of note which sent
it - is not further forwarded.

> What are the causes due to which self-address filtering and duplicate
> elimination only work "most of the time"?

Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf

> 
> > + */
> > +#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);  
> 
> Call ksz9477_cfg_port_member() instead?

+1

Thanks for the information.

> 
> > +
> > +	/* 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;  
> 
> FWIW:
> https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/
> Some coordination will be required regarding the MAC address that the
> switch driver needs to program to these registers. 

Writing of this MAC address is _required_ for PREVENTING PACKET LOOP IN
THE RING BY SELF-ADDRESS FILTERING feature.

In the ifconfig output - the lan1, lan2 and hsr0 shall all have the
same MAC address assigned.

I simply take the hsr0 mac address.

> It seems that it
> is not single purpose.

At least in the case of HSR it looks like single purpose (for the loop
prevention).

> 
> > +	}
> > +
> > +	/* 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);
> > +	}  
> 
> If there is no way that SW_SRC_ADDR_FILTER can be unset after
> ksz9477_reset_switch() is called, then this is dead code which should
> be removed.

Yes. Correct. I will remove it.

> 
> > +
> > +	/* 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;  
> 
> Can the code that is duplicated for the partner port be moved to the
> caller?

I've followed the convention from xrs700x driver, where we only make
setup when we are sure that on both HSR ports the "join" has been
called.

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




Best regards,

Lukasz Majewski

--

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

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

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 01:11:03PM +0200, Lukasz Majewski wrote:
> > > +/* 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.  
> > 
> > How do these 2 concepts (autonomous forwarding + software-based
> > elimination of some frames) work together? If software is not the sole
> > receiver of traffic which needs to be filtered further, and duplicates
> > also get forwarded to the network, does this not break the HSR ring?
> > 
> 
> Autonomous forwarding is based on KSZ9477, having the HSR ports
> "bridged" to send frames between them.
> 
> Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX
> packet duplication discarding which will discard duplicated frames.
> 
> Last but not least the - packet loop prevention.
> 
> My understanding is as follows:
> 
> 1. RX packet duplication removes copy of a frame, which is addressed to
> cpu port of switch.

Does the duplicate elimination function only do that, as you say, or is
it supposed to also eliminate duplicates for packets flooded to 2 ports
as well (the host port, and the other ring port)?

If the latter, then it will fail to eliminate duplicates for packets
that didn't visit the CPU. So the ring will rely on the self-address
filtering capability of the other devices, in order not to collapse.
I see that the software implementation also offers self-address
filtering in hsr_handle_frame() -> hsr_addr_is_self(), but I don't see
anything making that mandatory in IEC 62439-3:2018. Can we assume that
the other ring members will know how to deal with it?

> 2. The "bridge" of HSR passes frames in-KSZ9477, which are not
> addressed to this cpu host (between other HSR nodes).

How is the switch supposed to know which packets are addressed to this
CPU port and which are not? I expect separate answers for unicast,
multicast and broadcast.

> 3. Packet loop prevention - the HSR packet with SA of note which sent
> it - is not further forwarded.
> 
> > What are the causes due to which self-address filtering and duplicate
> > elimination only work "most of the time"?
> 
> Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
> https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf

Ok, so the limitation is a race condition in hardware such that, when
duplicate packets are received on member ports very close in time to
each other, the hardware fails to detect that they're duplicates.

> > > +	/* 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;  
> > 
> > FWIW:
> > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/
> > Some coordination will be required regarding the MAC address that the
> > switch driver needs to program to these registers. 
> 
> Writing of this MAC address is _required_ for PREVENTING PACKET LOOP IN
> THE RING BY SELF-ADDRESS FILTERING feature.

In case it was not clear, I was talking about coordination between you
and Oleksij. He needs to program the same register for Wake on LAN.

> In the ifconfig output - the lan1, lan2 and hsr0 shall all have the
> same MAC address assigned.
> 
> I simply take the hsr0 mac address.
> 
> > It seems that it is not single purpose.
> 
> At least in the case of HSR it looks like single purpose (for the loop
> prevention).

And for WoL, REG_SW_MAC_ADDR_0 is also single purpose. And for the MAC SA
in the generated PAUSE frames, also single purpose. Single + single + single = ?

Being a common register for multiple functions, I hope that it won't be
the users who discover than when multiple functionalities are used in
tandem (like WoL+HSR), they partially overwrite what the other has done.

So, by coordination, I mean something like a cohesive way of thinking
out the driver.

For WoL/pause frames, the linked discussion suggested that the switch
MAC address could be kept in sync with the DSA master's MAC address.

Could that also work here, and could we add a restriction to say
"Offload not supported for HSR device with a MAC address different from the DSA master"
and return -EOPNOTSUPP in ksz_hsr_join()? Then ksz9477_hsr_join() would
not modify this register.

> > > +	/* 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;  
> > 
> > Can the code that is duplicated for the partner port be moved to the
> > caller?
> 
> I've followed the convention from xrs700x driver, where we only make
> setup when we are sure that on both HSR ports the "join" has been
> called.

If code that is duplicated for both member ports can be moved to code
paths that get executed for each individual port, then the resulting
driver code may turn out to be less complex.

It's not a big deal if it can't, and maintaining a sane operating mode
in that transient state (HSR device with a single member port) is
obviously much more important. But the question was if it can, and I
don't think you've answered that?

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 04:03:55PM +0300, Vladimir Oltean wrote:
> > > What are the causes due to which self-address filtering and duplicate
> > > elimination only work "most of the time"?
> > 
> > Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
> > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
> 
> Ok, so the limitation is a race condition in hardware such that, when
> duplicate packets are received on member ports very close in time to
> each other, the hardware fails to detect that they're duplicates.

It would be good to leave at least the link as part of the comment,
if not also a short summary (in case the PDF URL gets moved/removed).

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Lukasz Majewski]

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

Hi Vladimir,

> On Tue, Sep 05, 2023 at 04:03:55PM +0300, Vladimir Oltean wrote:
> > > > What are the causes due to which self-address filtering and
> > > > duplicate elimination only work "most of the time"?  
> > > 
> > > Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
> > > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
> > >  
> > 
> > Ok, so the limitation is a race condition in hardware such that,
> > when duplicate packets are received on member ports very close in
> > time to each other, the hardware fails to detect that they're
> > duplicates.  
> 
> It would be good to leave at least the link as part of the comment,
> if not also a short summary (in case the PDF URL gets moved/removed).

Ok.


Best regards,

Lukasz Majewski

--

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

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

Re: [PATCH v3 RFC 3/4] net: dsa: hsr: Enable in KSZ9477 switch HW HSR offloading

[Lukasz Majewski]

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

Hi Vladimir,

> On Tue, Sep 05, 2023 at 01:11:03PM +0200, Lukasz Majewski wrote:
> > > > +/* 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.    
> > > 
> > > How do these 2 concepts (autonomous forwarding + software-based
> > > elimination of some frames) work together? If software is not the
> > > sole receiver of traffic which needs to be filtered further, and
> > > duplicates also get forwarded to the network, does this not break
> > > the HSR ring? 
> > 
> > Autonomous forwarding is based on KSZ9477, having the HSR ports
> > "bridged" to send frames between them.
> > 
> > Then, there is also based on HSR tag, and SA in-KSZ9477 feature RX
> > packet duplication discarding which will discard duplicated frames.
> > 
> > Last but not least the - packet loop prevention.
> > 
> > My understanding is as follows:
> > 
> > 1. RX packet duplication removes copy of a frame, which is
> > addressed to cpu port of switch.  
> 
> Does the duplicate elimination function only do that, as you say, or
> is it supposed to also eliminate duplicates for packets flooded to 2
> ports as well (the host port, and the other ring port)?
> 

In the "RX PACKET DUPLICATION DISCARDING" (from [1]) the hash of DA and
SA and seq number is used to assess if received frame is a duplicated.

----8<-------
If a matching (based on above??) frame has already been received on the
other port, the frame is dropped. If not, then standard forwarding rules
apply. 
---->8-------

> If the latter, then it will fail to eliminate duplicates for packets
> that didn't visit the CPU. So the ring will rely on the self-address
> filtering capability of the other devices, in order not to collapse.
> I see that the software implementation also offers self-address
> filtering in hsr_handle_frame() -> hsr_addr_is_self(), but I don't see
> anything making that mandatory in IEC 62439-3:2018. Can we assume that
> the other ring members will know how to deal with it?
> 
> > 2. The "bridge" of HSR passes frames in-KSZ9477, which are not
> > addressed to this cpu host (between other HSR nodes).  
> 
> How is the switch supposed to know which packets are addressed to this
> CPU port and which are not? I expect separate answers for unicast,
> multicast and broadcast.
> 
> > 3. Packet loop prevention - the HSR packet with SA of note which
> > sent it - is not further forwarded.
> >   
> > > What are the causes due to which self-address filtering and
> > > duplicate elimination only work "most of the time"?  
> > 
> > Please refer to section "KSZ9477 CHIP LIMITATIONS" in:
> > https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf
> >  
> 
> Ok, so the limitation is a race condition in hardware such that, when
> duplicate packets are received on member ports very close in time to
> each other, the hardware fails to detect that they're duplicates.

+1.

> 
> > > > +	/* 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;    
> > > 
> > > FWIW:
> > > https://lore.kernel.org/netdev/155ff37f-43d5-5fe0-6de4-c4639909553d@gmail.com/
> > > Some coordination will be required regarding the MAC address that
> > > the switch driver needs to program to these registers.   
> > 
> > Writing of this MAC address is _required_ for PREVENTING PACKET
> > LOOP IN THE RING BY SELF-ADDRESS FILTERING feature.  
> 
> In case it was not clear, I was talking about coordination between you
> and Oleksij. He needs to program the same register for Wake on LAN.

Ok. Thanks for the information.

> 
> > In the ifconfig output - the lan1, lan2 and hsr0 shall all have the
> > same MAC address assigned.
> > 
> > I simply take the hsr0 mac address.
> >   
> > > It seems that it is not single purpose.  
> > 
> > At least in the case of HSR it looks like single purpose (for the
> > loop prevention).  
> 
> And for WoL, REG_SW_MAC_ADDR_0 is also single purpose. And for the
> MAC SA in the generated PAUSE frames, also single purpose. Single +
> single + single = ?
> 
> Being a common register for multiple functions, I hope that it won't
> be the users who discover than when multiple functionalities are used
> in tandem (like WoL+HSR), they partially overwrite what the other has
> done.

This cannot be excluded, yes. However, I think that HSR ports will not
use WoL...

> 
> So, by coordination, I mean something like a cohesive way of thinking
> out the driver.
> 
> For WoL/pause frames, the linked discussion suggested that the switch
> MAC address could be kept in sync with the DSA master's MAC address.
> 
> Could that also work here, and could we add a restriction to say
> "Offload not supported for HSR device with a MAC address different
> from the DSA master" and return -EOPNOTSUPP in ksz_hsr_join()? Then
> ksz9477_hsr_join() would not modify this register.

In my case - the lanX ports (and hsr0) gets the same MAC address as it
is assigned to eth0 (the cpu port).

So, yes I can add such code to avoid problems.

> 
> > > > +	/* 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;    
> > > 
> > > Can the code that is duplicated for the partner port be moved to
> > > the caller?  
> > 
> > I've followed the convention from xrs700x driver, where we only make
> > setup when we are sure that on both HSR ports the "join" has been
> > called.  
> 
> If code that is duplicated for both member ports can be moved to code
> paths that get executed for each individual port, then the resulting
> driver code may turn out to be less complex.
> 
> It's not a big deal if it can't, and maintaining a sane operating mode
> in that transient state (HSR device with a single member port) is
> obviously much more important. But the question was if it can, and I
> don't think you've answered that?

I think that it would be possible to have single execution patch per
joining port (for example join is called on lan1 and then on lan2).

I'm concerned with writing common registers - for example
REG_HSR_PORT_MAP. If there are no side effects (like hsr port support
must be set for both at once), we can try to make single execution path.

Links:

[1] -
https://ww1.microchip.com/downloads/en/Appnotes/AN3474-KSZ9477-High-Availability-Seamless-Redundancy-Application-Note-00003474A.pdf



Best regards,

Lukasz Majewski

--

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

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

[PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Lukasz Majewski]

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

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:
> 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.

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

Thanks.

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Lukasz Majewski]

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

Hi Vladimir,

> On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:
> > 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

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

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:
> 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)

Should be squashed into patch 3/4. The split does not make the code
easier to review for me.

> ---
>  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;

move the "default: return -EOPNOTSUPP" statement from below here.

> +	}

I don't see any restriction to allow offloading a single HSR device.
Looking at patch 3/4, that will obviously not work due to some hardware
registers which are global and would be overwritten by the second HSR
device.

For example, a5psw_port_bridge_join() has a similar restriction to
offload a single bridge device.

If you return -EOPNOTSUPP, then DSA should fall back to an unoffloaded,
100% software-based HSR device, and that should work too. It would be
good if you could verify that the unoffloaded HSR works well after the
changes too.

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

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Lukasz Majewski]

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

Hi Vladimir,

> On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:
> > 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)  
> 
> Should be squashed into patch 3/4. The split does not make the code
> easier to review for me.

So you recommend to have only one patch in which the hsr_join{leave}
function from ksz_common.c and ksz9477_hsr_join{leave} from ksz9477.c
are added?

> 
> > ---
> >  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;  
> 
> move the "default: return -EOPNOTSUPP" statement from below here.
> 

Ok, I will add default statement with -EOPNOTSUPP.

> > +	}  
> 
> I don't see any restriction to allow offloading a single HSR device.

As I've written in the other response - I've followed the xrs700x.c
convention. 

Moreover, for me it seems more natural, that we only allow full HSR
support for 2 ports or none. Please be aware, that HSR supposed to
support only 2 ports, and having only one working is not recommended by
vendor.

> Looking at patch 3/4, that will obviously not work due to some
> hardware registers which are global and would be overwritten by the
> second HSR device.

I cannot guarantee that there will not be any "side effects" with this
approach. And to be honest - I would prefer to spent time on testing
recommended setups.

> 
> For example, a5psw_port_bridge_join() has a similar restriction to
> offload a single bridge device.

HSR is IMHO a bit different than plain "bridge" offloading.

> 
> If you return -EOPNOTSUPP, then DSA should fall back to an
> unoffloaded, 100% software-based HSR device, and that should work
> too. 

And then we would have one port with SW HSR and another one with HW
HSR?

>It would be good if you could verify that the unoffloaded HSR
> works well after the changes too.

I've tested on KSZ9477-EVB the SW HSR operation with two ports (and two
or three boards) and HW HSR offloading. Results are presented in the
cover-letter.

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




Best regards,

Lukasz Majewski

--

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

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

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 01:23:51PM +0200, Lukasz Majewski wrote:
> > Should be squashed into patch 3/4. The split does not make the code
> > easier to review for me.
> 
> So you recommend to have only one patch in which the hsr_join{leave}
> function from ksz_common.c and ksz9477_hsr_join{leave} from ksz9477.c
> are added?

Correct. In addition, patch 1/4 will be dropped. So there will be 2
patches, one on net/dsa/tag_ksz.c and the other on drivers/net/dsa/microchip/.

> > I don't see any restriction to allow offloading a single HSR device.
> 
> As I've written in the other response - I've followed the xrs700x.c
> convention. 

"the xrs700x.c convention"

xrs700x_hsr_join():

	/* Only ports 1 and 2 can be HSR/PRP redundant ports. */
	if (port != 1 && port != 2)
		return -EOPNOTSUPP;

So, checking for ports 1 and 2 is a stronger condition than the one I'm
asking you to enforce.

The KSZ9477 is more flexible. It can enable HSR offload on any 2 ports,
not just on ports 1 and 2. But it can still be 2 ports max, no more.
You haven't copied the xrs700x convention, but you haven't adapted it,
either.

> Moreover, for me it seems more natural, that we only allow full HSR
> support for 2 ports or none. Please be aware, that HSR supposed to
> support only 2 ports, and having only one working is not recommended by
> vendor.

And where is it enforced that full HSR offload is only applied to 2
ports or none?

> > Looking at patch 3/4, that will obviously not work due to some
> > hardware registers which are global and would be overwritten by the
> > second HSR device.
> 
> I cannot guarantee that there will not be any "side effects" with this
> approach. And to be honest - I would prefer to spent time on testing
> recommended setups.

Please read my reply again, keeping in mind that by "HSR device" I mean
"hsr0" in the commands below, and not the member ports as you've assumed
when responding.

> > 
> > For example, a5psw_port_bridge_join() has a similar restriction to
> > offload a single bridge device.
> 
> HSR is IMHO a bit different than plain "bridge" offloading.

Maybe this was not clear, but by "similar" I mean: if you replace the
"bridge" word with "hsr", and you copy that code snippet from a5psw,
you get the desired outcome.

static int ksz_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr,
			/* optionally pass the extack argument from the caller */)
{
	struct ksz_device *dev = ds->priv;

	/* We only support 1 HSR device */
	if (dev->hsr_dev && hsr != dev->hsr_dev) {
		NL_SET_ERR_MSG_MOD(extack,
				   "Offload supported for a single HSR");
		return -EOPNOTSUPP;
	}

	dev->hsr_dev = hsr;

	...

	return 0;
}

I did not imply that HSR is not different than bridge offloading.
I don't see how that is even related to the discussion.

> > If you return -EOPNOTSUPP, then DSA should fall back to an
> > unoffloaded, 100% software-based HSR device, and that should work
> > too. 
> 
> And then we would have one port with SW HSR and another one with HW
> HSR?

No. One HSR device (hsr0, with 2 member ports) with offload and one
HSR device (hsr1, with 2 member ports) without offload (see (b) below).

> >It would be good if you could verify that the unoffloaded HSR
> > works well after the changes too.
> 
> I've tested on KSZ9477-EVB the SW HSR operation with two ports (and two
> or three boards) and HW HSR offloading. Results are presented in the
> cover-letter.

"results in the cover letter"

Results:
With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
With no offloading                     RX: 63 Mbps  TX: 63 Mbps

What was the setup for the "no offloading" case?

(a) kernel did not contain the offloading patch set

(b) the testing was on hsr1, in the situation below:

ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45 version 1 # offloaded
ip link add name hsr1 type hsr slave1 lan3 slave2 lan4 supervision 45 version 1 # unoffloaded

(d) in some other way (please detail)

I was specifically asking about situation (b).

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Andrew Lunn]

> > And then we would have one port with SW HSR and another one with HW
> > HSR?
> 
> No. One HSR device (hsr0, with 2 member ports) with offload and one
> HSR device (hsr1, with 2 member ports) without offload (see (b) below).

I just wanted to comment that offloading is about taking what Linux
can do in software and getting the hardware to do it. Linux should
happily allow two HSR devices, working in software. If you can only
offload one of them, Linux should continue to do the other one in
software.

So please do follow what Vladimir is suggesting.

	Andrew

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Lukasz Majewski]

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

Hi Vladimir,

> On Tue, Sep 05, 2023 at 01:23:51PM +0200, Lukasz Majewski wrote:
> > > Should be squashed into patch 3/4. The split does not make the
> > > code easier to review for me.  
> > 
> > So you recommend to have only one patch in which the hsr_join{leave}
> > function from ksz_common.c and ksz9477_hsr_join{leave} from
> > ksz9477.c are added?  
> 
> Correct. In addition, patch 1/4 will be dropped. So there will be 2
> patches, one on net/dsa/tag_ksz.c and the other on
> drivers/net/dsa/microchip/.

Ok.

> 
> > > I don't see any restriction to allow offloading a single HSR
> > > device.  
> > 
> > As I've written in the other response - I've followed the xrs700x.c
> > convention.   
> 
> "the xrs700x.c convention"
> 
> xrs700x_hsr_join():
> 
> 	/* Only ports 1 and 2 can be HSR/PRP redundant ports. */
> 	if (port != 1 && port != 2)
> 		return -EOPNOTSUPP;
> 
> So, checking for ports 1 and 2 is a stronger condition than the one
> I'm asking you to enforce.
> 
> The KSZ9477 is more flexible. It can enable HSR offload on any 2
> ports, not just on ports 1 and 2. But it can still be 2 ports max, no
> more. You haven't copied the xrs700x convention, but you haven't
> adapted it, either.

Ok. I've misuderstood your suggestion. You were asking about having one
hsr offloaded setup (hsr0 => lan1 + lan2) and in the same time
non-offloaded setup (hsr1 => lan3 + lan4).

> 
> > Moreover, for me it seems more natural, that we only allow full HSR
> > support for 2 ports or none. Please be aware, that HSR supposed to
> > support only 2 ports, and having only one working is not
> > recommended by vendor.  
> 
> And where is it enforced that full HSR offload is only applied to 2
> ports or none?

In the ksz_jsr_join() at ksz_common.c -> we exit from this function
when !partner.

> 
> > > Looking at patch 3/4, that will obviously not work due to some
> > > hardware registers which are global and would be overwritten by
> > > the second HSR device.  
> > 
> > I cannot guarantee that there will not be any "side effects" with
> > this approach. And to be honest - I would prefer to spent time on
> > testing recommended setups.  
> 
> Please read my reply again, keeping in mind that by "HSR device" I
> mean "hsr0" in the commands below, and not the member ports as you've
> assumed when responding.

Yes. I do get your point.

> 
> > > 
> > > For example, a5psw_port_bridge_join() has a similar restriction to
> > > offload a single bridge device.  
> > 
> > HSR is IMHO a bit different than plain "bridge" offloading.  
> 
> Maybe this was not clear, but by "similar" I mean: if you replace the
> "bridge" word with "hsr", and you copy that code snippet from a5psw,
> you get the desired outcome.
> 
> static int ksz_hsr_join(struct dsa_switch *ds, int port, struct
> net_device *hsr, /* optionally pass the extack argument from the
> caller */) {
> 	struct ksz_device *dev = ds->priv;
> 
> 	/* We only support 1 HSR device */
> 	if (dev->hsr_dev && hsr != dev->hsr_dev) {
> 		NL_SET_ERR_MSG_MOD(extack,
> 				   "Offload supported for a single
> HSR"); return -EOPNOTSUPP;
> 	}
> 

Ok.

> 	dev->hsr_dev = hsr;
> 
> 	...
> 
> 	return 0;
> }
> 
> I did not imply that HSR is not different than bridge offloading.
> I don't see how that is even related to the discussion.
> 
> > > If you return -EOPNOTSUPP, then DSA should fall back to an
> > > unoffloaded, 100% software-based HSR device, and that should work
> > > too.   
> > 
> > And then we would have one port with SW HSR and another one with HW
> > HSR?  
> 
> No. One HSR device (hsr0, with 2 member ports) with offload and one
> HSR device (hsr1, with 2 member ports) without offload (see (b)
> below).
> 
> > >It would be good if you could verify that the unoffloaded HSR
> > > works well after the changes too.  
> > 
> > I've tested on KSZ9477-EVB the SW HSR operation with two ports (and
> > two or three boards) and HW HSR offloading. Results are presented
> > in the cover-letter.  
> 
> "results in the cover letter"
> 
> Results:
> With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
> With no offloading                     RX: 63 Mbps  TX: 63 Mbps
> 
> What was the setup for the "no offloading" case?
> 

I used two boards. I've used the same kernel with and without HSR
offloading patches.

Cables were connected to port1 and port2 on each board. Non HSR network
was connected to port3.

> (a) kernel did not contain the offloading patch set
> 

Yes.

> (b) the testing was on hsr1, in the situation below:
> 
> ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45
> version 1 # offloaded ip link add name hsr1 type hsr slave1 lan3
> slave2 lan4 supervision 45 version 1 # unoffloaded
> 

I did not setup two hsr devices on the same board. I've used two boards
with hsr0 setup on each.

> (d) in some other way (please detail)
> 
> I was specifically asking about situation (b).

I did not tested the hsr0, hsr1 as my (real) use case is with KSZ9477
having only 3 ports available for connection (port 1,2,3).


Best regards,

Lukasz Majewski

--

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

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

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 03:47:44PM +0200, Lukasz Majewski wrote:
> > > Moreover, for me it seems more natural, that we only allow full HSR
> > > support for 2 ports or none. Please be aware, that HSR supposed to
> > > support only 2 ports, and having only one working is not
> > > recommended by vendor.  
> > 
> > And where is it enforced that full HSR offload is only applied to 2
> > ports or none?
> 
> In the ksz_jsr_join() at ksz_common.c -> we exit from this function
> when !partner.

And what about 4 or 6 ports being placed as members of 2 or 3 HSR devices?
How does the !partner condition help there?

> > Results:
> > With KSZ9477 offloading support added: RX: 100 Mbps TX: 98 Mbps
> > With no offloading                     RX: 63 Mbps  TX: 63 Mbps
> > 
> > What was the setup for the "no offloading" case?
> > 
> 
> I used two boards. I've used the same kernel with and without HSR
> offloading patches.
> 
> Cables were connected to port1 and port2 on each board. Non HSR network
> was connected to port3.
> 
> > (a) kernel did not contain the offloading patch set
> > 
> 
> Yes.

Ok, then it would be good to check that your patch set does not break
something that used to work (software HSR - easiest to check with a
second pair of ports, but if not possible, it can also be emulated by
returning -EOPNOTSUPP in .port_hsr_join on an artificial other condition).

> > (b) the testing was on hsr1, in the situation below:
> > 
> > ip link add name hsr0 type hsr slave1 lan1 slave2 lan2 supervision 45
> > version 1 # offloaded ip link add name hsr1 type hsr slave1 lan3
> > slave2 lan4 supervision 45 version 1 # unoffloaded
> 
> I did not setup two hsr devices on the same board. I've used two boards
> with hsr0 setup on each.

Ok, but can you?

> > (d) in some other way (please detail)
> > 
> > I was specifically asking about situation (b).
> 
> I did not tested the hsr0, hsr1 as my (real) use case is with KSZ9477
> having only 3 ports available for connection (port 1,2,3).

ksz_chip_data :: port_cnt is set to 7 for KSZ9477. I guess that the
limitation of having only 3 user ports for testing is specific to your
board, and not to the entire switch as may be seen on other boards,
correct?

It doesn't mean that the "single HSR device" restriction shouldn't be
enforced.

Re: [PATCH v3 RFC 4/4] net: dsa: hsr: Provide generic HSR ksz_hsr_{join|leave} functions

[Vladimir Oltean]

On Mon, Sep 04, 2023 at 02:02:09PM +0200, Lukasz Majewski wrote:
> +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;
> +}

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.