Messages in this thread | | | Date | Thu, 31 Aug 2023 15:38:20 +0200 | From | Lukasz Majewski <> | Subject | Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding |
| |
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 [unhandled content-type:application/pgp-signature] | |