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

[Lukasz Majewski <lukma@denx.de>]

[-- 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 --]