Linux Ethernet Bridging - Jan 2020 - [Bridge] [RFC net-next Patch v2 4/4] net: bridge: mrp: switchdev: Add HW offload

The 01/13/2020 15:00, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe
> 
> On Mon, Jan 13, 2020 at 01:46:20PM +0100, Horatiu Vultur wrote:
> > +#ifdef CONFIG_BRIDGE_MRP
> > +/* SWITCHDEV_OBJ_ID_PORT_MRP */
> > +struct switchdev_obj_port_mrp {
> > +     struct switchdev_obj obj;
> > +     struct net_device *port;
> > +     u32 ring_nr;
> > +};
> > +
> > +#define SWITCHDEV_OBJ_PORT_MRP(OBJ) \
> > +     container_of((OBJ), struct switchdev_obj_port_mrp, obj)
> > +
> > +/* SWITCHDEV_OBJ_ID_RING_TEST_MRP */
> > +struct switchdev_obj_ring_test_mrp {
> > +     struct switchdev_obj obj;
> > +     /* The value is in us and a value of 0 represents to stop */
> > +     u32 interval;
> > +     u8 max;
> > +     u32 ring_nr;
> > +};
> > +
> > +#define SWITCHDEV_OBJ_RING_TEST_MRP(OBJ) \
> > +     container_of((OBJ), struct switchdev_obj_ring_test_mrp, obj)
> > +
> > +/* SWITCHDEV_OBJ_ID_RING_ROLE_MRP */
> > +struct switchdev_obj_ring_role_mrp {
> > +     struct switchdev_obj obj;
> > +     u8 ring_role;
> > +     u32 ring_nr;
> > +};
> 
> Hi Horatiu
> 
> The structures above should give me enough information to build this,
> correct?
Hi Andrew,

You will need also these attributes to build a minimum MRP_Test frame:
SWITCHDEV_ATTR_ID_MRP_PORT_STATE,
SWITCHDEV_ATTR_ID_MRP_PORT_ROLE,
SWITCHDEV_ATTR_ID_MRP_RING_STATE,
SWITCHDEV_ATTR_ID_MRP_RING_TRANS,
> 
> Ethernet II, Src: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1), Dst: Iec_00:00:01
(01:15:4e:00:00:01)
>     Destination: Iec_00:00:01 (01:15:4e:00:00:01)
>     Source: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
>     Type: MRP (0x88e3)
> PROFINET MRP MRP_Test, MRP_Common, MRP_End
>     MRP_Version: 1
>     MRP_TLVHeader.Type: MRP_Test (0x02)
>         MRP_TLVHeader.Type: MRP_Test (0x02)
>         MRP_TLVHeader.Length: 18
>         MRP_Prio: 0x1f40 High priorities
>         MRP_SA: 7a:8b:b1:35:96:e1 (7a:8b:b1:35:96:e1)
>         MRP_PortRole: Primary ring port (0x0000)
>         MRP_RingState: Ring closed (0x0001)
>         MRP_Transition: 0x0001
>         MRP_TimeStamp [ms]: 0x000cf574             <---------- Updated
automatic
>     MRP_TLVHeader.Type: MRP_Common (0x01)
>         MRP_TLVHeader.Type: MRP_Common (0x01)
>         MRP_TLVHeader.Length: 18
>         MRP_SequenceID: 0x00e9                     <---------- Updated
automatic
>         MRP_DomainUUID: ffffffff-ffff-ffff-ffff-ffffffffffff
>     MRP_TLVHeader.Type: MRP_End (0x00)
>         MRP_TLVHeader.Type: MRP_End (0x00)
>         MRP_TLVHeader.Length: 0
> 
> There are a couple of fields i don't see. MRP_SA, MRP_Transition.
Regarding the MRP_SA, which represents the bridge MAC address, we could
get this information from listening to the notifications in the driver.
So I don't think we need a special call for this.

The same could be for MRP_Transition, which counts the number of times
the ring goes in open state. In theory we could get information by
counting in the driver how many times the ring gets in the open state.
And we get this information through the attribute
SWITCHDEV_ATTR_ID_MRP_RING_STATE.

The other fields that are missing are MRP_Prio and MRP_DomainUUID. But
these values could be set to a default values for now because they are
used by MRA(Media Redundancy Auto-manager), which is not part of this
patch series.
> 
> What are max and ring_nr used for?
The max represents the number of MRP_Test frames that can be missed
by receiver before it declares the ring open. For example if the
receiver expects a MRP_Frame every 10ms and it sets the max to 3. Then
it means that if it didn't receive a frame in 30ms, it would set that
the port didn't receive MRP_Test.
The ring_nr represents the ID of the MRP instance. For example, on a
switch which has 8 ports, there can be 4 MRP instances. Because each
instance requires 2 ports. And to be able to differences them, each
instance has it's own ID, which is this ring_nr.
> 
> Do you need to set the first value MRP_SequenceID uses? Often, in
> order to detect a reset, a random value is used to initialise the
> sequence number. Also, does the time stamp need initializing?
I couldn't see in the standard if they required an initial for
MRP_SequenceID. From what I have seen on some switches that have their
own MRP implementation, they set the initial value of MRP_SequenceID to
0 and they increase for it frame.
Regarding the timestamp, again the standard doesn't say anything about
initial value. This timestamp is used by MRM to determine the maximum
travel time of the MRP_Test frames in a ring.
> 
> Thanks
>         Andrew
-- 
/Horatiu

Hi Horatiu

It has been said a few times what the basic state machine should be in
user space. A pure software solution can use raw sockets to send and
receive MRP_Test test frames. When considering hardware acceleration,
the switchdev API you have proposed here seems quite simple. It should
not be too hard to map it to a set of netlink messages from userspace.

Yet your argument for kernel, not user space, is you are worried about
the parameters which need to be passed to the hardware offload engine.
In order to win the argument for a kernel solution, we are going to
need a better idea what you think this problem is. The MRP_Test is TLV
based. Are there other things which could be in this message? Is that
what you are worried about?

Thanks
     Andrew