Re: [PATCH] net: hsr : Provide fix for HSRv1 supervisor frames decoding

[Lukasz Majewski]

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@xxxxxxx

Attachment: pgpFrmTMgDBJZ.pgp
Description: OpenPGP digital signature