[PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[Felix Maurer]

Hangbin recently reported that the hsr selftests were failing and noted
that the entries in the node table were not merged, i.e., had
00:00:00:00:00:00 as MacAddressB forever [1].

This failure only occured with HSRv0 because it was not sending
supervision frames anymore. While debugging this I found that we were
not really following the HSRv0 standard for the supervision frames we
sent, so I additionally made a few changes to get closer to the standard
and restore a more correct behavior we had a while ago.

The selftests can still fail because they take a while and run into the
timeout. I did not include a change of the timeout because I have more
improvements to the selftests mostly ready that change the test duration
but are net-next material.

[1]: https://lore.kernel.org/netdev/aMONxDXkzBZZRfE5@fedora/

Felix Maurer (2):
  hsr: Fix supervision frame sending on HSRv0
  hsr: Follow standard for HSRv0 supervision frames

 net/hsr/hsr_device.c  |  5 ++++-
 net/hsr/hsr_forward.c | 22 +++++++++++++++-------
 2 files changed, 19 insertions(+), 8 deletions(-)

--
2.51.0

[PATCH net 1/2] hsr: Fix supervision frame sending on HSRv0

[Felix Maurer]

On HSRv0, no supervision frames were sent. The supervison frames were
generated successfully, but failed the check for a sufficiently long mac
header, i.e., at least sizeof(struct hsr_ethhdr), in hsr_fill_frame_info()
because the mac header only contained the ethernet header.

Fix this by including the HSR header in the mac header when generating HSR
supervision frames. Note that the mac header now also includes the TLV
fields. This matches how we set the headers on rx and also the size of
struct hsrv0_ethhdr_sp.

Reported-by: Hangbin Liu <liuhangbin@gmail.com>
Closes: https://lore.kernel.org/netdev/aMONxDXkzBZZRfE5@fedora/
Fixes: 9cfb5e7f0ded ("net: hsr: fix hsr_init_sk() vs network/transport headers.")
Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 net/hsr/hsr_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index fbbc3ccf9df6..1235abb2d79f 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -320,6 +320,9 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
 	}
 
 	hsr_stag = skb_put(skb, sizeof(struct hsr_sup_tag));
+	skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
+	skb_reset_mac_len(skb);
+
 	set_hsr_stag_path(hsr_stag, (hsr->prot_version ? 0x0 : 0xf));
 	set_hsr_stag_HSR_ver(hsr_stag, hsr->prot_version);
 
-- 
2.51.0

Re: [PATCH net 1/2] hsr: Fix supervision frame sending on HSRv0

[Sebastian Andrzej Siewior]

On 2025-11-11 17:29:32 [+0100], Felix Maurer wrote:
> On HSRv0, no supervision frames were sent. The supervison frames were
> generated successfully, but failed the check for a sufficiently long mac
> header, i.e., at least sizeof(struct hsr_ethhdr), in hsr_fill_frame_info()
> because the mac header only contained the ethernet header.
> 
> Fix this by including the HSR header in the mac header when generating HSR
> supervision frames. Note that the mac header now also includes the TLV
> fields. This matches how we set the headers on rx and also the size of
> struct hsrv0_ethhdr_sp.
> 
> Reported-by: Hangbin Liu <liuhangbin@gmail.com>
> Closes: https://lore.kernel.org/netdev/aMONxDXkzBZZRfE5@fedora/
> Fixes: 9cfb5e7f0ded ("net: hsr: fix hsr_init_sk() vs network/transport headers.")
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>

Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

[PATCH net 2/2] hsr: Follow standard for HSRv0 supervision frames

[Felix Maurer]

For HSRv0, the path_id has the following meaning:
- 0000: PRP supervision frame
- 0001-1001: HSR ring identifier
- 1010-1011: Frames from PRP network (A/B, with RedBoxes)
- 1111: HSR supervision frame

Follow the IEC 62439-3:2010 standard more closely by setting the right
path_id for HSRv0 supervision frames (actually, it is correctly set when
the frame is constructed, but hsr_set_path_id() overwrites it) and set a
fixed HSR ring identifier of 1. The ring identifier seems to be generally
unused and we ignore it anyways on reception, but some fixed identifier is
definitely better than using one identifier in one direction and a wrong
identifier in the other.

This was also the behavior before commit f266a683a480 ("net/hsr: Better
frame dispatch") which introduced the alternating path_id. This was later
moved to hsr_set_path_id() in commit 451d8123f897 ("net: prp: add packet
handling support").

The IEC 62439-3:2010 also contains 6 unused bytes after the MacAddressA in
the HSRv0 supervision frames. Adjust a TODO comment accordingly.

Fixes: f266a683a480 ("net/hsr: Better frame dispatch")
Fixes: 451d8123f897 ("net: prp: add packet handling support")
Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 net/hsr/hsr_device.c  |  2 +-
 net/hsr/hsr_forward.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 1235abb2d79f..492cbc78ab75 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -337,7 +337,7 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
 	}
 
 	hsr_stag->tlv.HSR_TLV_type = type;
-	/* TODO: Why 12 in HSRv0? */
+	/* HSRv0 has 6 unused bytes after the MAC */
 	hsr_stag->tlv.HSR_TLV_length = hsr->prot_version ?
 				sizeof(struct hsr_sup_payload) : 12;
 
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index c67c0d35921d..339f0d220212 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -262,15 +262,23 @@ static struct sk_buff *prp_fill_rct(struct sk_buff *skb,
 	return skb;
 }
 
-static void hsr_set_path_id(struct hsr_ethhdr *hsr_ethhdr,
+static void hsr_set_path_id(struct hsr_frame_info *frame,
+			    struct hsr_ethhdr *hsr_ethhdr,
 			    struct hsr_port *port)
 {
 	int path_id;
 
-	if (port->type == HSR_PT_SLAVE_A)
-		path_id = 0;
-	else
-		path_id = 1;
+	if (port->hsr->prot_version) {
+		if (port->type == HSR_PT_SLAVE_A)
+			path_id = 0;
+		else
+			path_id = 1;
+	} else {
+		if (frame->is_supervision)
+			path_id = 0xf;
+		else
+			path_id = 1;
+	}
 
 	set_hsr_tag_path(&hsr_ethhdr->hsr_tag, path_id);
 }
@@ -304,7 +312,7 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
 	else
 		hsr_ethhdr = (struct hsr_ethhdr *)pc;
 
-	hsr_set_path_id(hsr_ethhdr, port);
+	hsr_set_path_id(frame, hsr_ethhdr, port);
 	set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
 	hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
 	hsr_ethhdr->hsr_tag.encap_proto = hsr_ethhdr->ethhdr.h_proto;
@@ -330,7 +338,7 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
 			(struct hsr_ethhdr *)skb_mac_header(frame->skb_hsr);
 
 		/* set the lane id properly */
-		hsr_set_path_id(hsr_ethhdr, port);
+		hsr_set_path_id(frame, hsr_ethhdr, port);
 		return skb_clone(frame->skb_hsr, GFP_ATOMIC);
 	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
 		return skb_clone(frame->skb_std, GFP_ATOMIC);
-- 
2.51.0

Re: [PATCH net 2/2] hsr: Follow standard for HSRv0 supervision frames

[Sebastian Andrzej Siewior]

On 2025-11-11 17:29:33 [+0100], Felix Maurer wrote:
> For HSRv0, the path_id has the following meaning:
> - 0000: PRP supervision frame
> - 0001-1001: HSR ring identifier
> - 1010-1011: Frames from PRP network (A/B, with RedBoxes)
> - 1111: HSR supervision frame

Where do you have this from?
I have here IEC 62439-3:2021 (Edition 4.0 2021-12).
From the 4 bits of path_id, the three most significant bits are NetId
with 0 for HSR and 1 to 6 for the PTP network and 7 reserved.
The list significant bit for PRP indicates Redbox A/B while for HSR it
indicates port A/B.

You say HSRv0 while I don't see this mentioned at all. And you limit the
change to prot_version == 0. So maybe this was once and removed from the
standard.

Since this is limited to v0:
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Sebastian

Re: [PATCH net 2/2] hsr: Follow standard for HSRv0 supervision frames

[Felix Maurer]

On 12.11.25 11:24, Sebastian Andrzej Siewior wrote:
> On 2025-11-11 17:29:33 [+0100], Felix Maurer wrote:
>> For HSRv0, the path_id has the following meaning:
>> - 0000: PRP supervision frame
>> - 0001-1001: HSR ring identifier
>> - 1010-1011: Frames from PRP network (A/B, with RedBoxes)
>> - 1111: HSR supervision frame
> 
> Where do you have this from?
> I have here IEC 62439-3:2021 (Edition 4.0 2021-12).
> From the 4 bits of path_id, the three most significant bits are NetId
> with 0 for HSR and 1 to 6 for the PTP network and 7 reserved.
> The list significant bit for PRP indicates Redbox A/B while for HSR it
> indicates port A/B.
> 
> You say HSRv0 while I don't see this mentioned at all. And you limit the
> change to prot_version == 0. So maybe this was once and removed from the
> standard.

My description for the path_id is from IEC 62439-3:2010. As far as I
know, the HSRv0/HSRv1 terminology is only used in the kernel. AFAIK, our
version 0/1 refers to the value in the SupVersion field of the HSR
supervision frames. The SupVersion is defined as 0 in IEC 62439-3:2010
and defined as 1 in IEC 62439-3:2012 and following.

The definition for the SupVersion field also states: "Implementation of
version X of the protocol shall interpret [...] version <=X frames
exactly as specified for the version concerned." (in IEC
62439-3:{2010,2012,2016,2021})

I read from this that if we implement HSRv0 we should follow the latest
specification for this version, i.e., the latest specification with
SupVersion defined as 0 (which would be IEC 62439-3:2010). This is also
why I limited the change to prot_version == 0 (maybe we should have some
helpers like hsr_is_v{0,1}() to make these conditions a bit more self
explanatory).

> Since this is limited to v0:
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks,
   Felix

Re: [PATCH net 2/2] hsr: Follow standard for HSRv0 supervision frames

[Sebastian Andrzej Siewior]

On 2025-11-12 12:01:51 [+0100], Felix Maurer wrote:
> > You say HSRv0 while I don't see this mentioned at all. And you limit the
> > change to prot_version == 0. So maybe this was once and removed from the
> > standard.
> 
> My description for the path_id is from IEC 62439-3:2010. As far as I
> know, the HSRv0/HSRv1 terminology is only used in the kernel. AFAIK, our
> version 0/1 refers to the value in the SupVersion field of the HSR
> supervision frames. The SupVersion is defined as 0 in IEC 62439-3:2010
> and defined as 1 in IEC 62439-3:2012 and following.

This is what I assumed. I don't have any older specification, I have
here SupVersion always defined as 1.

> The definition for the SupVersion field also states: "Implementation of
> version X of the protocol shall interpret [...] version <=X frames
> exactly as specified for the version concerned." (in IEC
> 62439-3:{2010,2012,2016,2021})
> 
> I read from this that if we implement HSRv0 we should follow the latest
> specification for this version, i.e., the latest specification with
> SupVersion defined as 0 (which would be IEC 62439-3:2010). This is also
> why I limited the change to prot_version == 0 (maybe we should have some
> helpers like hsr_is_v{0,1}() to make these conditions a bit more self
> explanatory).

Based on the explanation, the limit to prot_version is the reasonable
thing to do.

> Thanks,
>    Felix

Sebastian

Re: [PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[Hangbin Liu]

On Tue, Nov 11, 2025 at 05:29:31PM +0100, Felix Maurer wrote:
> Hangbin recently reported that the hsr selftests were failing and noted
> that the entries in the node table were not merged, i.e., had
> 00:00:00:00:00:00 as MacAddressB forever [1].
> 
> This failure only occured with HSRv0 because it was not sending
> supervision frames anymore. While debugging this I found that we were
> not really following the HSRv0 standard for the supervision frames we
> sent, so I additionally made a few changes to get closer to the standard
> and restore a more correct behavior we had a while ago.
> 
> The selftests can still fail because they take a while and run into the
> timeout. I did not include a change of the timeout because I have more
> improvements to the selftests mostly ready that change the test duration
> but are net-next material.
> 
> [1]: https://lore.kernel.org/netdev/aMONxDXkzBZZRfE5@fedora/
> 
> Felix Maurer (2):
>   hsr: Fix supervision frame sending on HSRv0
>   hsr: Follow standard for HSRv0 supervision frames
> 
>  net/hsr/hsr_device.c  |  5 ++++-
>  net/hsr/hsr_forward.c | 22 +++++++++++++++-------
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> --
> 2.51.0
> 

Tested-by: Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[Sebastian Andrzej Siewior]

On 2025-11-11 17:29:31 [+0100], Felix Maurer wrote:
> Hangbin recently reported that the hsr selftests were failing and noted
> that the entries in the node table were not merged, i.e., had
> 00:00:00:00:00:00 as MacAddressB forever [1].
> 
> This failure only occured with HSRv0 because it was not sending
> supervision frames anymore. While debugging this I found that we were
> not really following the HSRv0 standard for the supervision frames we
> sent, so I additionally made a few changes to get closer to the standard
> and restore a more correct behavior we had a while ago.

Thank you. I meant to look into this…

> The selftests can still fail because they take a while and run into the
> timeout. I did not include a change of the timeout because I have more
> improvements to the selftests mostly ready that change the test duration
> but are net-next material.

I added a -W10 to ping and it passes while -W5 fails. I can't remember
that this happen before but whatever. Sure, fix the testsuite via -next
if you have more things in order to improve it.

Sebastian

Re: [PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[Felix Maurer]

On 12.11.25 11:03, Sebastian Andrzej Siewior wrote:
> On 2025-11-11 17:29:31 [+0100], Felix Maurer wrote:
>> The selftests can still fail because they take a while and run into the
>> timeout. I did not include a change of the timeout because I have more
>> improvements to the selftests mostly ready that change the test duration
>> but are net-next material.
> 
> I added a -W10 to ping and it passes while -W5 fails. I can't remember
> that this happen before but whatever. Sure, fix the testsuite via -next
> if you have more things in order to improve it.

I was referring to the timeout for the net/hsr tests in
tools/testing/selftests/net/hsr/settings, set to 50 seconds at the
moment. I didn't observe that -W10 is needed but I'll take a look at it.

Thanks,
   Felix

Re: [PATCH net 0/2] hsr: Send correct HSRv0 supervision frames

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 11 Nov 2025 17:29:31 +0100 you wrote:
> Hangbin recently reported that the hsr selftests were failing and noted
> that the entries in the node table were not merged, i.e., had
> 00:00:00:00:00:00 as MacAddressB forever [1].
> 
> This failure only occured with HSRv0 because it was not sending
> supervision frames anymore. While debugging this I found that we were
> not really following the HSRv0 standard for the supervision frames we
> sent, so I additionally made a few changes to get closer to the standard
> and restore a more correct behavior we had a while ago.
> 
> [...]

Here is the summary with links:
  - [net,1/2] hsr: Fix supervision frame sending on HSRv0
    https://git.kernel.org/netdev/net/c/96a3a03abf3d
  - [net,2/2] hsr: Follow standard for HSRv0 supervision frames
    https://git.kernel.org/netdev/net/c/b2c26c82f7a9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html