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

[Lukasz Majewski]

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




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: pgpvLCtT36T3n.pgp
Description: OpenPGP digital signature