Re: [PATCH net 1/2] net: mrp: fix definitions of MRP test packets

[Horatiu Vultur]

Hi Andrew,

The 12/23/2020 19:41, Andrew Lunn wrote:
> 
> > > @@ -96,7 +96,7 @@ struct br_mrp_ring_test_hdr {
> > >         __be16 state;
> > >         __be16 transitions;
> > >         __be32 timestamp;
> > > -};
> > > +} __attribute__((__packed__));
> >
> > Yes, I agree that this should be packed but it also needs to be 32 bit
> > alligned, so extra 2 bytes are needed.
> 
> The full structure is:
> 
> struct br_mrp_ring_test_hdr {
>         __be16 prio;
>         __u8 sa[ETH_ALEN];
>         __be16 port_role;
>         __be16 state;
>         __be16 transitions;
>         __be32 timestamp;
> };
> 
> If i'm looking at this correctly, the byte offsets are:
> 
> 0-1   prio
> 2-7   sa
> 8-9   port_role
> 10-11 state
> 12-13 transition
> 
> With packed you get
> 
> 14-17 timestamp
> 
> which is not 32 bit aligned.
> 
> Do you mean the whole structure must be 32 bit aligned? We need to add
> two reserved bytes to the end of the structure?

Sorry, I looked too fast over this. First some info, in front of the
'br_mrp_ring_test_hdr', there is 'br_mrp_tlv_hdr' which is 2
bytes. So the frame will look something like this:

... |---------|----------------|----------------------|------------| ....
... | MRP Ver | br_mrp_tlv_hdr | br_mrp_ring_test_hdr | Common TLV | ....
... |---------|----------------|----------------------|------------| ....

The standard says that for br_mrp_tlv_hdr + br_mrp_ring_test, 32 bit
alignment shall be ensured. So my understanding was that it needs to be
at word boundary(4 bytes).

So based on this, if we use packed then we get the following offsets
0	type
1	length
2-3	prio
4-9	sa
10-11	port_role
12-13	state
14-15	transition
16-19	timestamp.

Which is 20 bytes, that fits correctly. So my understanding is we need to
use packed, to remove the hole between transition and timestamp as
Rasmus suggested and should NOT use these 2 extra bytes that I
mentioned because it would not be aligned anymore.

Here you can find few more details about MRP[1]

> 
>     Andrew

[1] https://www.youtube.com/watch?v=R3vQYfwiJ2M

-- 
/Horatiu