* RE: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-25 15:31 [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding Lukasz Majewski
@ 2023-08-25 18:10 ` Tristram.Ha
2023-08-28 9:02 ` Lukasz Majewski
2023-08-25 23:44 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Tristram.Ha @ 2023-08-25 18:10 UTC (permalink / raw)
To: lukma
Cc: andrew, f.fainelli, kuba, edumazet, bigeasy, pabeni, koverskeid,
matthieu.baerts, netdev, linux-kernel, davem
> - /* And leave the HSR tag. */
> + * And leave the HSR tag.
> + *
> + * The HSRv1 supervisory frame encapsulates the v0 frame
> + * with EtherType of 0x88FB
> + */
> if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> - pull_size = sizeof(struct ethhdr);
> + if (hsr->prot_version == HSR_V1)
> + /* In the above step the DA, SA and EtherType
> + * (0x892F - HSRv1) bytes has been removed.
> + *
> + * As the HSRv1 has the HSR header added, one need
> + * to remove path_and_LSDU_size and sequence_nr fields.
> + *
> + */
> + pull_size = 4;
> + else
> + pull_size = sizeof(struct hsr_tag);
> +
> skb_pull(skb, pull_size);
> total_pull_size += pull_size;
> }
> @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
> total_pull_size += pull_size;
>
> /* get HSR sup payload */
> + if (hsr->prot_version == HSR_V1) {
> + /* In the HSRv1 supervisor frame, when
> + * one with EtherType = 0x88FB is extracted, the Node A
> + * MAC address is preceded with type and length elements of TLV
> + * data field.
> + *
> + * It needs to be removed to get the remote peer MAC address.
> + */
> + pull_size = sizeof(struct hsr_sup_tlv);
> + skb_pull(skb, pull_size);
> + total_pull_size += pull_size;
> + }
> +
> hsr_sp = (struct hsr_sup_payload *)skb->data;
I thought the fix is simply this:
if (ethhdr->h_proto == htons(ETH_P_HSR)) {
- pull_size = sizeof(struct ethhdr);
+ pull_size = sizeof(struct hsr_tag);
skb_pull(skb, pull_size);
total_pull_size += pull_size;
}
- pull_size = sizeof(struct hsr_tag);
+ pull_size = sizeof(struct hsr_sup_tag);
Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
The code in 5.15 before this refactored code uses those structures.
When using v0 the EtherType uses the PRP tag instead of the HSR tag so
the HSR related code is not executed.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-25 18:10 ` Tristram.Ha
@ 2023-08-28 9:02 ` Lukasz Majewski
2023-08-31 13:38 ` Lukasz Majewski
2023-09-04 15:54 ` Lukasz Majewski
0 siblings, 2 replies; 15+ messages in thread
From: Lukasz Majewski @ 2023-08-28 9:02 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, f.fainelli, kuba, edumazet, bigeasy, pabeni, koverskeid,
matthieu.baerts, netdev, linux-kernel, davem
[-- Attachment #1: Type: text/plain, Size: 5975 bytes --]
Hi Tristram,
> > - /* And leave the HSR tag. */
> > + * And leave the HSR tag.
> > + *
> > + * The HSRv1 supervisory frame encapsulates the v0 frame
> > + * with EtherType of 0x88FB
> > + */
> > if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > - pull_size = sizeof(struct ethhdr);
> > + if (hsr->prot_version == HSR_V1)
> > + /* In the above step the DA, SA and
> > EtherType
> > + * (0x892F - HSRv1) bytes has been removed.
> > + *
> > + * As the HSRv1 has the HSR header added,
> > one need
> > + * to remove path_and_LSDU_size and
> > sequence_nr fields.
> > + *
> > + */
> > + pull_size = 4;
> > + else
> > + pull_size = sizeof(struct hsr_tag);
> > +
> > skb_pull(skb, pull_size);
> > total_pull_size += pull_size;
> > }
> > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > hsr_frame_info *frame) total_pull_size += pull_size;
> >
> > /* get HSR sup payload */
> > + if (hsr->prot_version == HSR_V1) {
> > + /* In the HSRv1 supervisor frame, when
> > + * one with EtherType = 0x88FB is extracted, the
> > Node A
> > + * MAC address is preceded with type and length
> > elements of TLV
> > + * data field.
> > + *
> > + * It needs to be removed to get the remote peer
> > MAC address.
> > + */
> > + pull_size = sizeof(struct hsr_sup_tlv);
> > + skb_pull(skb, pull_size);
> > + total_pull_size += pull_size;
> > + }
> > +
> > hsr_sp = (struct hsr_sup_payload *)skb->data;
>
> I thought the fix is simply this:
>
> if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> - pull_size = sizeof(struct ethhdr);
> + pull_size = sizeof(struct hsr_tag);
> skb_pull(skb, pull_size);
> total_pull_size += pull_size;
> }
>
> - pull_size = sizeof(struct hsr_tag);
> + pull_size = sizeof(struct hsr_sup_tag);
>
> Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> The code in 5.15 before this refactored code uses those structures.
> When using v0 the EtherType uses the PRP tag instead of the HSR tag so
> the HSR related code is not executed.
>
This would not be enough it seems. Please find below skb->data dump
when entering hsr_handle_sup_frame() [0]:
SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00
With the newest kernel (before applying this patch) in [1] we do remove:
01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)
So we do have:
00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00
And we need to remove rest of the HSR v1 tag (4 Bytes).
Then we do have:
SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00
The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
Length (0x06), which indicate the other HSR host IP address.
When I do apply your proposed changes we would have the DA and SA
MAC addresses removed implicitly (as the struct hsr_tag and hsr_sup_tag
are 6 bytes in size) and we end up with frame starting with HSR v1 tag -
i.e.:
SKB_I100000000: 89 2f 00 34
SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SKB_I100000040: 00 00
Hence mine question - is my setup or understanding wrong (as the PRP
supervisory frame is encapsulated in HSR v1 frame)?
I do use the same kernel on two KSZ9477-EVB boards with Port[12]
connected together to work with HSR. I also to explicitly force the HSR
driver to use v1 of HSR (by default v0 is enforced).
If you don't mind - I would also like to ask a question regarding the
node_db for HSR.
Why the output of:
# cat /sys/kernel/debug/hsr/hsr0/node_table
Node Table entries for (HSR) device
MAC-Address-A, MAC-Address-B, time_in[A], time_in[B],
00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199,
Address-B port, DAN-H
0, 1
Has the MAC-Address-B equal to 00:00:00:00:00:00 ?
As I do have the same MAC addresses for both HSR ports (to facilitate
frame duplication in KSZ9477 IC removal) I would expect to have this MAC
address set to 00:10:a1:94:77:30 as well...
Is this expected? Or is there any other issue to fix?
Thanks in advance for your help and support :-)
Links:
[0] -
https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281
[1] -
https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-28 9:02 ` Lukasz Majewski
@ 2023-08-31 13:38 ` Lukasz Majewski
2023-09-04 15:54 ` Lukasz Majewski
1 sibling, 0 replies; 15+ messages in thread
From: Lukasz Majewski @ 2023-08-31 13:38 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, f.fainelli, kuba, edumazet, bigeasy, pabeni, koverskeid,
matthieu.baerts, netdev, linux-kernel, davem
[-- Attachment #1: Type: text/plain, Size: 6659 bytes --]
Hi Tristram,
> Hi Tristram,
>
> > > - /* And leave the HSR tag. */
> > > + * And leave the HSR tag.
> > > + *
> > > + * The HSRv1 supervisory frame encapsulates the v0 frame
> > > + * with EtherType of 0x88FB
> > > + */
> > > if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > > - pull_size = sizeof(struct ethhdr);
> > > + if (hsr->prot_version == HSR_V1)
> > > + /* In the above step the DA, SA and
> > > EtherType
> > > + * (0x892F - HSRv1) bytes has been
> > > removed.
> > > + *
> > > + * As the HSRv1 has the HSR header added,
> > > one need
> > > + * to remove path_and_LSDU_size and
> > > sequence_nr fields.
> > > + *
> > > + */
> > > + pull_size = 4;
> > > + else
> > > + pull_size = sizeof(struct hsr_tag);
> > > +
> > > skb_pull(skb, pull_size);
> > > total_pull_size += pull_size;
> > > }
> > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > > hsr_frame_info *frame) total_pull_size += pull_size;
> > >
> > > /* get HSR sup payload */
> > > + if (hsr->prot_version == HSR_V1) {
> > > + /* In the HSRv1 supervisor frame, when
> > > + * one with EtherType = 0x88FB is extracted, the
> > > Node A
> > > + * MAC address is preceded with type and length
> > > elements of TLV
> > > + * data field.
> > > + *
> > > + * It needs to be removed to get the remote peer
> > > MAC address.
> > > + */
> > > + pull_size = sizeof(struct hsr_sup_tlv);
> > > + skb_pull(skb, pull_size);
> > > + total_pull_size += pull_size;
> > > + }
> > > +
> > > hsr_sp = (struct hsr_sup_payload *)skb->data;
> >
> > I thought the fix is simply this:
> >
> > if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > - pull_size = sizeof(struct ethhdr);
> > + pull_size = sizeof(struct hsr_tag);
> > skb_pull(skb, pull_size);
> > total_pull_size += pull_size;
> > }
> >
> > - pull_size = sizeof(struct hsr_tag);
> > + pull_size = sizeof(struct hsr_sup_tag);
> >
> > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> > The code in 5.15 before this refactored code uses those structures.
> > When using v0 the EtherType uses the PRP tag instead of the HSR tag
> > so the HSR related code is not executed.
> >
>
> This would not be enough it seems. Please find below skb->data dump
> when entering hsr_handle_sup_frame() [0]:
>
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> With the newest kernel (before applying this patch) in [1] we do
> remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
> sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)
>
> So we do have:
>
> 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> And we need to remove rest of the HSR v1 tag (4 Bytes).
>
> Then we do have:
>
> SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
> needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
> Length (0x06), which indicate the other HSR host IP address.
>
> When I do apply your proposed changes we would have the DA and SA
> MAC addresses removed implicitly (as the struct hsr_tag and
> hsr_sup_tag are 6 bytes in size) and we end up with frame starting
> with HSR v1 tag - i.e.:
>
> SKB_I100000000: 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
>
> Hence mine question - is my setup or understanding wrong (as the PRP
> supervisory frame is encapsulated in HSR v1 frame)?
>
> I do use the same kernel on two KSZ9477-EVB boards with Port[12]
> connected together to work with HSR. I also to explicitly force the
> HSR driver to use v1 of HSR (by default v0 is enforced).
>
>
>
>
>
> If you don't mind - I would also like to ask a question regarding the
> node_db for HSR.
>
> Why the output of:
>
> # cat /sys/kernel/debug/hsr/hsr0/node_table
> Node Table entries for (HSR) device
> MAC-Address-A, MAC-Address-B, time_in[A], time_in[B],
> 00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199,
>
> Address-B port, DAN-H
> 0, 1
>
> Has the MAC-Address-B equal to 00:00:00:00:00:00 ?
>
> As I do have the same MAC addresses for both HSR ports (to facilitate
> frame duplication in KSZ9477 IC removal) I would expect to have this
> MAC address set to 00:10:a1:94:77:30 as well...
>
> Is this expected? Or is there any other issue to fix?
>
Tristram, do you have any feedback on those changes?
>
> Thanks in advance for your help and support :-)
>
> Links:
>
> [0] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281
>
> [1] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290
>
> 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
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-28 9:02 ` Lukasz Majewski
2023-08-31 13:38 ` Lukasz Majewski
@ 2023-09-04 15:54 ` Lukasz Majewski
1 sibling, 0 replies; 15+ messages in thread
From: Lukasz Majewski @ 2023-09-04 15:54 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, f.fainelli, kuba, edumazet, bigeasy, pabeni, koverskeid,
matthieu.baerts, netdev, linux-kernel, davem
[-- Attachment #1: Type: text/plain, Size: 7874 bytes --]
Hi Tristram,
> Hi Tristram,
>
> > > - /* And leave the HSR tag. */
> > > + * And leave the HSR tag.
> > > + *
> > > + * The HSRv1 supervisory frame encapsulates the v0 frame
> > > + * with EtherType of 0x88FB
> > > + */
> > > if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > > - pull_size = sizeof(struct ethhdr);
> > > + if (hsr->prot_version == HSR_V1)
> > > + /* In the above step the DA, SA and
> > > EtherType
> > > + * (0x892F - HSRv1) bytes has been
> > > removed.
> > > + *
> > > + * As the HSRv1 has the HSR header added,
> > > one need
> > > + * to remove path_and_LSDU_size and
> > > sequence_nr fields.
> > > + *
> > > + */
> > > + pull_size = 4;
> > > + else
> > > + pull_size = sizeof(struct hsr_tag);
> > > +
> > > skb_pull(skb, pull_size);
> > > total_pull_size += pull_size;
> > > }
> > > @@ -313,6 +328,19 @@ void hsr_handle_sup_frame(struct
> > > hsr_frame_info *frame) total_pull_size += pull_size;
> > >
> > > /* get HSR sup payload */
> > > + if (hsr->prot_version == HSR_V1) {
> > > + /* In the HSRv1 supervisor frame, when
> > > + * one with EtherType = 0x88FB is extracted, the
> > > Node A
> > > + * MAC address is preceded with type and length
> > > elements of TLV
> > > + * data field.
> > > + *
> > > + * It needs to be removed to get the remote peer
> > > MAC address.
> > > + */
> > > + pull_size = sizeof(struct hsr_sup_tlv);
> > > + skb_pull(skb, pull_size);
> > > + total_pull_size += pull_size;
> > > + }
> > > +
> > > hsr_sp = (struct hsr_sup_payload *)skb->data;
> >
> > I thought the fix is simply this:
> >
> > if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> > - pull_size = sizeof(struct ethhdr);
> > + pull_size = sizeof(struct hsr_tag);
> > skb_pull(skb, pull_size);
> > total_pull_size += pull_size;
> > }
> >
> > - pull_size = sizeof(struct hsr_tag);
> > + pull_size = sizeof(struct hsr_sup_tag);
> >
> > Note the sizes of hsr_tag and hsr_sup_tag are the same: 6 bytes.
> > The code in 5.15 before this refactored code uses those structures.
> > When using v0 the EtherType uses the PRP tag instead of the HSR tag
> > so the HSR related code is not executed.
> >
>
> This would not be enough it seems. Please find below skb->data dump
> when entering hsr_handle_sup_frame() [0]:
>
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> With the newest kernel (before applying this patch) in [1] we do
> remove: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f (which is equal to
> sizeof(struct ethhdr) = 6 + 6 + 2 B = 14 B)
>
> So we do have:
>
> 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> And we need to remove rest of the HSR v1 tag (4 Bytes).
>
> Then we do have:
>
> SKB_I100000010: 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> The 0x88FB is the PRP/HSRv0 supervisory frame ETH type, so the tag
> needs to be removed (6 Bytes) and then we do have TYPE (0x17) and
> Length (0x06), which indicate the other HSR host IP address.
>
> When I do apply your proposed changes we would have the DA and SA
> MAC addresses removed implicitly (as the struct hsr_tag and
> hsr_sup_tag are 6 bytes in size) and we end up with frame starting
> with HSR v1 tag - i.e.:
>
> SKB_I100000000: 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
>
> Hence mine question - is my setup or understanding wrong (as the PRP
> supervisory frame is encapsulated in HSR v1 frame)?
>
> I do use the same kernel on two KSZ9477-EVB boards with Port[12]
> connected together to work with HSR. I also to explicitly force the
> HSR driver to use v1 of HSR (by default v0 is enforced).
>
>
As I've double checked with tshark - the supervision frame format is
different for HSR v0 and v1:
HSR v0:
[Protocols in frame:eth:ethertype:hsr_prp_supervision]
HSR v1:
[Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]
In v0 the ":hsr:" is missing, hence the error.
>
>
>
> If you don't mind - I would also like to ask a question regarding the
> node_db for HSR.
>
> Why the output of:
>
> # cat /sys/kernel/debug/hsr/hsr0/node_table
> Node Table entries for (HSR) device
> MAC-Address-A, MAC-Address-B, time_in[A], time_in[B],
> 00:10:a1:94:77:30 00:00:00:00:00:00 1689193, 1689199,
>
> Address-B port, DAN-H
> 0, 1
>
> Has the MAC-Address-B equal to 00:00:00:00:00:00 ?
>
> As I do have the same MAC addresses for both HSR ports (to facilitate
> frame duplication in KSZ9477 IC removal) I would expect to have this
> MAC address set to 00:10:a1:94:77:30 as well...
>
> Is this expected? Or is there any other issue to fix?
I think that this is caused by how KSZ9477 works with HSR core.
The HSR core is responsible for setting tag for frame. It sets the
"Lane ID" part of HSR tag to 0 as it relies on HSR frames duplication
in KSZ9477:
Type: High-availability Seamless Redundancy (IEC62439 Part 3)
(0x892f)
High-availability Seamless Redundancy (IEC62439 Part 3
Chapter 5) 0000 .... .... .... = Path: 0
000. .... .... .... = Network id: 0
...0 .... .... .... = Lane id: Lane A (0)
.... 0000 0011 0100 = LSDU size: 52 [correct]
As KSZ9477 duplicates (clones) frames without any modification, only
frames for Lane id = 0 are sent. Hence the MAC-Address-B is always
equal to 00:00:00:00:00:00 as no supervisory frames are received with
Lane B id.
This however, may be different in other HSR switch IC's - especially in
those, which insert the HSR header to frames - as they know to which
HSR "lane" (egress port) the frame is delivered.
>
>
> Thanks in advance for your help and support :-)
>
> Links:
>
> [0] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L281
>
> [1] -
> https://elixir.bootlin.com/linux/v6.5-rc7/source/net/hsr/hsr_framereg.c#L290
>
> 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
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-25 15:31 [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding Lukasz Majewski
2023-08-25 18:10 ` Tristram.Ha
@ 2023-08-25 23:44 ` kernel test robot
2023-08-26 0:38 ` kernel test robot
2023-09-05 8:06 ` Sebastian Andrzej Siewior
3 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-08-25 23:44 UTC (permalink / raw)
To: Lukasz Majewski, Tristram.Ha, Eric Dumazet, davem
Cc: llvm, oe-kbuild-all, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Sebastian Andrzej Siewior, Paolo Abeni,
Kristian Overskeid, Matthieu Baerts, netdev, linux-kernel,
Lukasz Majewski
Hi Lukasz,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423
base: net-next/main
patch link: https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de
patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
config: powerpc64-randconfig-r022-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260733.G7tU8UHx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308260733.G7tU8UHx-lkp@intel.com/
All errors (new ones prefixed by >>):
>> net/hsr/hsr_framereg.c:289:8: error: expected ';' after expression
289 | * And leave the HSR tag.
| ^
| ;
>> net/hsr/hsr_framereg.c:289:5: error: use of undeclared identifier 'And'
289 | * And leave the HSR tag.
| ^
>> net/hsr/hsr_framereg.c:289:9: error: use of undeclared identifier 'leave'
289 | * And leave the HSR tag.
| ^
3 errors generated.
vim +289 net/hsr/hsr_framereg.c
249
250 /* Use the Supervision frame's info about an eventual macaddress_B for merging
251 * nodes that has previously had their macaddress_B registered as a separate
252 * node.
253 */
254 void hsr_handle_sup_frame(struct hsr_frame_info *frame)
255 {
256 struct hsr_node *node_curr = frame->node_src;
257 struct hsr_port *port_rcv = frame->port_rcv;
258 struct hsr_priv *hsr = port_rcv->hsr;
259 struct hsr_sup_payload *hsr_sp;
260 struct hsr_sup_tlv *hsr_sup_tlv;
261 struct hsr_node *node_real;
262 struct sk_buff *skb = NULL;
263 struct list_head *node_db;
264 struct ethhdr *ethhdr;
265 int i;
266 unsigned int pull_size = 0;
267 unsigned int total_pull_size = 0;
268
269 /* Here either frame->skb_hsr or frame->skb_prp should be
270 * valid as supervision frame always will have protocol
271 * header info.
272 */
273 if (frame->skb_hsr)
274 skb = frame->skb_hsr;
275 else if (frame->skb_prp)
276 skb = frame->skb_prp;
277 else if (frame->skb_std)
278 skb = frame->skb_std;
279 if (!skb)
280 return;
281
282 /* Leave the ethernet header. */
283 pull_size = sizeof(struct ethhdr);
284 skb_pull(skb, pull_size);
285 total_pull_size += pull_size;
286
287 ethhdr = (struct ethhdr *)skb_mac_header(skb);
288
> 289 * And leave the HSR tag.
290 *
291 * The HSRv1 supervisory frame encapsulates the v0 frame
292 * with EtherType of 0x88FB
293 */
294 if (ethhdr->h_proto == htons(ETH_P_HSR)) {
295 if (hsr->prot_version == HSR_V1)
296 /* In the above step the DA, SA and EtherType
297 * (0x892F - HSRv1) bytes has been removed.
298 *
299 * As the HSRv1 has the HSR header added, one need
300 * to remove path_and_LSDU_size and sequence_nr fields.
301 *
302 */
303 pull_size = 4;
304 else
305 pull_size = sizeof(struct hsr_tag);
306
307 skb_pull(skb, pull_size);
308 total_pull_size += pull_size;
309 }
310
311 /* And leave the HSR sup tag. */
312 pull_size = sizeof(struct hsr_tag);
313 skb_pull(skb, pull_size);
314 total_pull_size += pull_size;
315
316 /* get HSR sup payload */
317 if (hsr->prot_version == HSR_V1) {
318 /* In the HSRv1 supervisor frame, when
319 * one with EtherType = 0x88FB is extracted, the Node A
320 * MAC address is preceded with type and length elements of TLV
321 * data field.
322 *
323 * It needs to be removed to get the remote peer MAC address.
324 */
325 pull_size = sizeof(struct hsr_sup_tlv);
326 skb_pull(skb, pull_size);
327 total_pull_size += pull_size;
328 }
329
330 hsr_sp = (struct hsr_sup_payload *)skb->data;
331
332 /* Merge node_curr (registered on macaddress_B) into node_real */
333 node_db = &port_rcv->hsr->node_db;
334 node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A);
335 if (!node_real)
336 /* No frame received from AddrA of this node yet */
337 node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A,
338 HSR_SEQNR_START - 1, true,
339 port_rcv->type);
340 if (!node_real)
341 goto done; /* No mem */
342 if (node_real == node_curr)
343 /* Node has already been merged */
344 goto done;
345
346 /* Leave the first HSR sup payload. */
347 pull_size = sizeof(struct hsr_sup_payload);
348 skb_pull(skb, pull_size);
349 total_pull_size += pull_size;
350
351 /* Get second supervision tlv */
352 hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
353 /* And check if it is a redbox mac TLV */
354 if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
355 /* We could stop here after pushing hsr_sup_payload,
356 * or proceed and allow macaddress_B and for redboxes.
357 */
358 /* Sanity check length */
359 if (hsr_sup_tlv->HSR_TLV_length != 6)
360 goto done;
361
362 /* Leave the second HSR sup tlv. */
363 pull_size = sizeof(struct hsr_sup_tlv);
364 skb_pull(skb, pull_size);
365 total_pull_size += pull_size;
366
367 /* Get redbox mac address. */
368 hsr_sp = (struct hsr_sup_payload *)skb->data;
369
370 /* Check if redbox mac and node mac are equal. */
371 if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) {
372 /* This is a redbox supervision frame for a VDAN! */
373 goto done;
374 }
375 }
376
377 ether_addr_copy(node_real->macaddress_B, ethhdr->h_source);
378 spin_lock_bh(&node_real->seq_out_lock);
379 for (i = 0; i < HSR_PT_PORTS; i++) {
380 if (!node_curr->time_in_stale[i] &&
381 time_after(node_curr->time_in[i], node_real->time_in[i])) {
382 node_real->time_in[i] = node_curr->time_in[i];
383 node_real->time_in_stale[i] =
384 node_curr->time_in_stale[i];
385 }
386 if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i]))
387 node_real->seq_out[i] = node_curr->seq_out[i];
388 }
389 spin_unlock_bh(&node_real->seq_out_lock);
390 node_real->addr_B_port = port_rcv->type;
391
392 spin_lock_bh(&hsr->list_lock);
393 if (!node_curr->removed) {
394 list_del_rcu(&node_curr->mac_list);
395 node_curr->removed = true;
396 kfree_rcu(node_curr, rcu_head);
397 }
398 spin_unlock_bh(&hsr->list_lock);
399
400 done:
401 /* Push back here */
402 skb_push(skb, total_pull_size);
403 }
404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-25 15:31 [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding Lukasz Majewski
2023-08-25 18:10 ` Tristram.Ha
2023-08-25 23:44 ` kernel test robot
@ 2023-08-26 0:38 ` kernel test robot
2023-09-05 8:06 ` Sebastian Andrzej Siewior
3 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-08-26 0:38 UTC (permalink / raw)
To: Lukasz Majewski, Tristram.Ha, Eric Dumazet, davem
Cc: oe-kbuild-all, Andrew Lunn, Florian Fainelli, Jakub Kicinski,
Sebastian Andrzej Siewior, Paolo Abeni, Kristian Overskeid,
Matthieu Baerts, netdev, linux-kernel, Lukasz Majewski
Hi Lukasz,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lukasz-Majewski/net-hsr-Provide-fix-for-HSRv1-supervisor-frames-decoding/20230825-233423
base: net-next/main
patch link: https://lore.kernel.org/r/20230825153111.228768-1-lukma%40denx.de
patch subject: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
config: riscv-randconfig-001-20230826 (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230826/202308260833.erhVKBnc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308260833.erhVKBnc-lkp@intel.com/
All errors (new ones prefixed by >>):
net/hsr/hsr_framereg.c: In function 'hsr_handle_sup_frame':
>> net/hsr/hsr_framereg.c:289:12: error: 'And' undeclared (first use in this function)
289 | * And leave the HSR tag.
| ^~~
net/hsr/hsr_framereg.c:289:12: note: each undeclared identifier is reported only once for each function it appears in
>> net/hsr/hsr_framereg.c:289:15: error: expected ';' before 'leave'
289 | * And leave the HSR tag.
| ^~~~~~
| ;
vim +/And +289 net/hsr/hsr_framereg.c
249
250 /* Use the Supervision frame's info about an eventual macaddress_B for merging
251 * nodes that has previously had their macaddress_B registered as a separate
252 * node.
253 */
254 void hsr_handle_sup_frame(struct hsr_frame_info *frame)
255 {
256 struct hsr_node *node_curr = frame->node_src;
257 struct hsr_port *port_rcv = frame->port_rcv;
258 struct hsr_priv *hsr = port_rcv->hsr;
259 struct hsr_sup_payload *hsr_sp;
260 struct hsr_sup_tlv *hsr_sup_tlv;
261 struct hsr_node *node_real;
262 struct sk_buff *skb = NULL;
263 struct list_head *node_db;
264 struct ethhdr *ethhdr;
265 int i;
266 unsigned int pull_size = 0;
267 unsigned int total_pull_size = 0;
268
269 /* Here either frame->skb_hsr or frame->skb_prp should be
270 * valid as supervision frame always will have protocol
271 * header info.
272 */
273 if (frame->skb_hsr)
274 skb = frame->skb_hsr;
275 else if (frame->skb_prp)
276 skb = frame->skb_prp;
277 else if (frame->skb_std)
278 skb = frame->skb_std;
279 if (!skb)
280 return;
281
282 /* Leave the ethernet header. */
283 pull_size = sizeof(struct ethhdr);
284 skb_pull(skb, pull_size);
285 total_pull_size += pull_size;
286
287 ethhdr = (struct ethhdr *)skb_mac_header(skb);
288
> 289 * And leave the HSR tag.
290 *
291 * The HSRv1 supervisory frame encapsulates the v0 frame
292 * with EtherType of 0x88FB
293 */
294 if (ethhdr->h_proto == htons(ETH_P_HSR)) {
295 if (hsr->prot_version == HSR_V1)
296 /* In the above step the DA, SA and EtherType
297 * (0x892F - HSRv1) bytes has been removed.
298 *
299 * As the HSRv1 has the HSR header added, one need
300 * to remove path_and_LSDU_size and sequence_nr fields.
301 *
302 */
303 pull_size = 4;
304 else
305 pull_size = sizeof(struct hsr_tag);
306
307 skb_pull(skb, pull_size);
308 total_pull_size += pull_size;
309 }
310
311 /* And leave the HSR sup tag. */
312 pull_size = sizeof(struct hsr_tag);
313 skb_pull(skb, pull_size);
314 total_pull_size += pull_size;
315
316 /* get HSR sup payload */
317 if (hsr->prot_version == HSR_V1) {
318 /* In the HSRv1 supervisor frame, when
319 * one with EtherType = 0x88FB is extracted, the Node A
320 * MAC address is preceded with type and length elements of TLV
321 * data field.
322 *
323 * It needs to be removed to get the remote peer MAC address.
324 */
325 pull_size = sizeof(struct hsr_sup_tlv);
326 skb_pull(skb, pull_size);
327 total_pull_size += pull_size;
328 }
329
330 hsr_sp = (struct hsr_sup_payload *)skb->data;
331
332 /* Merge node_curr (registered on macaddress_B) into node_real */
333 node_db = &port_rcv->hsr->node_db;
334 node_real = find_node_by_addr_A(node_db, hsr_sp->macaddress_A);
335 if (!node_real)
336 /* No frame received from AddrA of this node yet */
337 node_real = hsr_add_node(hsr, node_db, hsr_sp->macaddress_A,
338 HSR_SEQNR_START - 1, true,
339 port_rcv->type);
340 if (!node_real)
341 goto done; /* No mem */
342 if (node_real == node_curr)
343 /* Node has already been merged */
344 goto done;
345
346 /* Leave the first HSR sup payload. */
347 pull_size = sizeof(struct hsr_sup_payload);
348 skb_pull(skb, pull_size);
349 total_pull_size += pull_size;
350
351 /* Get second supervision tlv */
352 hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
353 /* And check if it is a redbox mac TLV */
354 if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
355 /* We could stop here after pushing hsr_sup_payload,
356 * or proceed and allow macaddress_B and for redboxes.
357 */
358 /* Sanity check length */
359 if (hsr_sup_tlv->HSR_TLV_length != 6)
360 goto done;
361
362 /* Leave the second HSR sup tlv. */
363 pull_size = sizeof(struct hsr_sup_tlv);
364 skb_pull(skb, pull_size);
365 total_pull_size += pull_size;
366
367 /* Get redbox mac address. */
368 hsr_sp = (struct hsr_sup_payload *)skb->data;
369
370 /* Check if redbox mac and node mac are equal. */
371 if (!ether_addr_equal(node_real->macaddress_A, hsr_sp->macaddress_A)) {
372 /* This is a redbox supervision frame for a VDAN! */
373 goto done;
374 }
375 }
376
377 ether_addr_copy(node_real->macaddress_B, ethhdr->h_source);
378 spin_lock_bh(&node_real->seq_out_lock);
379 for (i = 0; i < HSR_PT_PORTS; i++) {
380 if (!node_curr->time_in_stale[i] &&
381 time_after(node_curr->time_in[i], node_real->time_in[i])) {
382 node_real->time_in[i] = node_curr->time_in[i];
383 node_real->time_in_stale[i] =
384 node_curr->time_in_stale[i];
385 }
386 if (seq_nr_after(node_curr->seq_out[i], node_real->seq_out[i]))
387 node_real->seq_out[i] = node_curr->seq_out[i];
388 }
389 spin_unlock_bh(&node_real->seq_out_lock);
390 node_real->addr_B_port = port_rcv->type;
391
392 spin_lock_bh(&hsr->list_lock);
393 if (!node_curr->removed) {
394 list_del_rcu(&node_curr->mac_list);
395 node_curr->removed = true;
396 kfree_rcu(node_curr, rcu_head);
397 }
398 spin_unlock_bh(&hsr->list_lock);
399
400 done:
401 /* Push back here */
402 skb_push(skb, total_pull_size);
403 }
404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-08-25 15:31 [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding Lukasz Majewski
` (2 preceding siblings ...)
2023-08-26 0:38 ` kernel test robot
@ 2023-09-05 8:06 ` Sebastian Andrzej Siewior
2023-09-05 9:55 ` Lukasz Majewski
3 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-09-05 8:06 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:
> Provide fix to decode correctly supervisory frames when HSRv1 version of
> the HSR protocol is used.
>
> Without this patch console is polluted with:
> ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
>
> as a result of destination node's A MAC address equals to:
> 00:00:00:00:00:00.
>
> cat /sys/kernel/debug/hsr/hsr0/node_table
> Node Table entries for (HSR) device
> MAC-Address-A, MAC-Address-B, time_in[A], time_in[B], Address-B
> 00:00:00:00:00:00 00:10:a1:94:77:30 400bf, 399c, 0
>
> It was caused by wrong frames decoding in the hsr_handle_sup_frame().
>
> As the supervisor frame is encapsulated in HSRv1 frame:
>
> SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> SKB_I100000040: 00 00
>
> The code had to be adjusted accordingly and the MAC-Address-A now
> has the proper address (the MAC-Address-B now has all 0's).
Was this broken by commit
eafaa88b3eb7f ("net: hsr: Add support for redbox supervision frames")
? Is this frame somehow special? I don't remember this…
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
Sebastian
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-05 8:06 ` Sebastian Andrzej Siewior
@ 2023-09-05 9:55 ` Lukasz Majewski
2023-09-11 14:57 ` Lukasz Majewski
0 siblings, 1 reply; 15+ messages in thread
From: Lukasz Majewski @ 2023-09-05 9:55 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
Hi Sebastian,
> On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:
> > Provide fix to decode correctly supervisory frames when HSRv1
> > version of the HSR protocol is used.
> >
> > Without this patch console is polluted with:
> > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
> >
> > as a result of destination node's A MAC address equals to:
> > 00:00:00:00:00:00.
> >
> > cat /sys/kernel/debug/hsr/hsr0/node_table
> > Node Table entries for (HSR) device
> > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B],
> > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30 400bf,
> > 399c, 0
> >
> > It was caused by wrong frames decoding in the
> > hsr_handle_sup_frame().
> >
> > As the supervisor frame is encapsulated in HSRv1 frame:
> >
> > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > SKB_I100000040: 00 00
> >
> > The code had to be adjusted accordingly and the MAC-Address-A now
> > has the proper address (the MAC-Address-B now has all 0's).
>
> Was this broken by commit
> eafaa88b3eb7f ("net: hsr: Add support for redbox supervision
> frames")
>
Yes, it seems so.
> ? Is this frame somehow special? I don't remember this…
>
Please refer to the whole thread - I've described this issue thoroughly
(including hex dump of frames):
https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98
In short - the HSRv1 is not recognized correctly anymore:
HSR v0:
[Protocols in frame: eth:ethertype:hsr_prp_supervision]
HSR v1:
[Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
>
> Sebastian
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-05 9:55 ` Lukasz Majewski
@ 2023-09-11 14:57 ` Lukasz Majewski
2023-09-11 15:01 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 15+ messages in thread
From: Lukasz Majewski @ 2023-09-11 14:57 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]
Hi Sebastian,
> Hi Sebastian,
>
> > On 2023-08-25 17:31:11 [+0200], Lukasz Majewski wrote:
> > > Provide fix to decode correctly supervisory frames when HSRv1
> > > version of the HSR protocol is used.
> > >
> > > Without this patch console is polluted with:
> > > ksz-switch spi1.0 lan1: hsr_addr_subst_dest: Unknown node
> > >
> > > as a result of destination node's A MAC address equals to:
> > > 00:00:00:00:00:00.
> > >
> > > cat /sys/kernel/debug/hsr/hsr0/node_table
> > > Node Table entries for (HSR) device
> > > MAC-Address-A, MAC-Address-B, time_in[A], time_in[B],
> > > Address-B 00:00:00:00:00:00 00:10:a1:94:77:30 400bf,
> > > 399c, 0
> > >
> > > It was caused by wrong frames decoding in the
> > > hsr_handle_sup_frame().
> > >
> > > As the supervisor frame is encapsulated in HSRv1 frame:
> > >
> > > SKB_I100000000: 01 15 4e 00 01 2d 00 10 a1 94 77 30 89 2f 00 34
> > > SKB_I100000010: 02 59 88 fb 00 01 84 15 17 06 00 10 a1 94 77 30
> > > SKB_I100000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > SKB_I100000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > SKB_I100000040: 00 00
> > >
> > > The code had to be adjusted accordingly and the MAC-Address-A now
> > > has the proper address (the MAC-Address-B now has all 0's).
> >
> > Was this broken by commit
> > eafaa88b3eb7f ("net: hsr: Add support for redbox supervision
> > frames")
> >
>
> Yes, it seems so.
>
> > ? Is this frame somehow special? I don't remember this…
> >
>
> Please refer to the whole thread - I've described this issue
> thoroughly (including hex dump of frames):
> https://lore.kernel.org/lkml/20230904175419.7bed196b@wsk/T/#m35cbfa4f1b8901d341fbc39659ace6a041f84c98
>
> In short - the HSRv1 is not recognized correctly anymore:
>
> HSR v0:
> [Protocols in frame: eth:ethertype:hsr_prp_supervision]
>
> HSR v1:
> [Protocols in frame: eth:ethertype:hsr:hsr_prp_supervision]
>
Have you had time to review this patch?
Your comments are more than welcome.
>
> > > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >
> > Sebastian
>
>
> 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
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-11 14:57 ` Lukasz Majewski
@ 2023-09-11 15:01 ` Sebastian Andrzej Siewior
2023-09-12 8:18 ` Lukasz Majewski
0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-09-11 15:01 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote:
> Hi Sebastian,
Hi,
>
> Have you had time to review this patch?
got distracted a few times. I need a quiet moment… Will do this week…
> Your comments are more than welcome.
Sebastian
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-11 15:01 ` Sebastian Andrzej Siewior
@ 2023-09-12 8:18 ` Lukasz Majewski
2023-09-13 16:32 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 15+ messages in thread
From: Lukasz Majewski @ 2023-09-12 8:18 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
Hi Sebastian,
> On 2023-09-11 16:57:08 [+0200], Lukasz Majewski wrote:
> > Hi Sebastian,
> Hi,
>
> >
> > Have you had time to review this patch?
>
> got distracted a few times. I need a quiet moment… Will do this week…
>
Ok. No problem. Thanks for the information.
> > Your comments are more than welcome.
>
> Sebastian
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-12 8:18 ` Lukasz Majewski
@ 2023-09-13 16:32 ` Sebastian Andrzej Siewior
2023-09-14 12:26 ` Lukasz Majewski
0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-09-13 16:32 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote:
> Hi Sebastian,
Hi Lukasz,
> Ok. No problem. Thanks for the information.
So what happens if you try this:
diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index b77f1189d19d1..6d14d935ee828 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info *frame)
/* And leave the HSR tag. */
if (ethhdr->h_proto == htons(ETH_P_HSR)) {
- pull_size = sizeof(struct ethhdr);
+ pull_size = sizeof(struct hsr_tag);
skb_pull(skb, pull_size);
total_pull_size += pull_size;
}
/* And leave the HSR sup tag. */
- pull_size = sizeof(struct hsr_tag);
+ pull_size = sizeof(struct hsr_sup_tag);
skb_pull(skb, pull_size);
total_pull_size += pull_size;
Sebastian
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-13 16:32 ` Sebastian Andrzej Siewior
@ 2023-09-14 12:26 ` Lukasz Majewski
2023-09-14 12:32 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 15+ messages in thread
From: Lukasz Majewski @ 2023-09-14 12:26 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]
Hi Sebastian Andrzej,
> On 2023-09-12 10:18:28 [+0200], Lukasz Majewski wrote:
> > Hi Sebastian,
> Hi Lukasz,
>
> > Ok. No problem. Thanks for the information.
>
> So what happens if you try this:
>
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index b77f1189d19d1..6d14d935ee828 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -288,13 +288,13 @@ void hsr_handle_sup_frame(struct hsr_frame_info
> *frame)
> /* And leave the HSR tag. */
> if (ethhdr->h_proto == htons(ETH_P_HSR)) {
> - pull_size = sizeof(struct ethhdr);
> + pull_size = sizeof(struct hsr_tag);
> skb_pull(skb, pull_size);
> total_pull_size += pull_size;
> }
>
> /* And leave the HSR sup tag. */
> - pull_size = sizeof(struct hsr_tag);
> + pull_size = sizeof(struct hsr_sup_tag);
> skb_pull(skb, pull_size);
> total_pull_size += pull_size;
>
>
> Sebastian
Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f).
Such solution has also been pointed out earlier by Tristram.
I will prepare v2 of this patch.
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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding
2023-09-14 12:26 ` Lukasz Majewski
@ 2023-09-14 12:32 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 15+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-09-14 12:32 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Tristram.Ha, Eric Dumazet, davem, Andrew Lunn, Florian Fainelli,
Jakub Kicinski, Paolo Abeni, Kristian Overskeid, Matthieu Baerts,
netdev, linux-kernel, Andreas Oetken
On 2023-09-14 14:26:50 [+0200], Lukasz Majewski wrote:
> Hi Sebastian Andrzej,
Hi,
>
> Yes, this fixes this issue (caused by: SHA1: eafaa88b3eb7f).
> Such solution has also been pointed out earlier by Tristram.
Right, now that I looked at it…
> I will prepare v2 of this patch.
don't worry, I take care of this.
> Best regards,
>
> Lukasz Majewski
Sebastian
^ permalink raw reply [flat|nested] 15+ messages in thread