[RFC,net-next,v3,03/10] net: bridge: mrp: Add MRP interface used by netlink
diff mbox series

Message ID 20200124161828.12206-4-horatiu.vultur@microchip.com
State RFC
Delegated to: David Miller
Headers show
Series
  • net: bridge: mrp: Add support for Media Redundancy Protocol (MRP)
Related show

Commit Message

Horatiu Vultur Jan. 24, 2020, 4:18 p.m. UTC
Define the MRP interface that will be used by the generic netlink to offload the
calls to the HW.

For this it is required for the kernel to hold in a list all the MRP instances
that are created and all the ports that are part of the MRP rings. Therefore add
the structure 'br_mrp'. This contains the following:
- a list with all MRP instances
- pointer to the net bridge on which the MRP instance is attach to
- pointers to the net bridge ports, which represents the ring ports
- a ring nr which represents the ID of the ring.

The interface has the following functions:

br_mrp_add - it creates a br_mrp instances and adds it to the list of mrp
  instances.
br_mrp_del - it removes a br_mrp instances from the list based on the ring nr of
  the instance.
These functions are used just by the SW to know which rings and which ports are
to which ring. These function will not call eventually the switchdev API. If
there is a HW required to know about these calls then the switchdev API can be
extended.

br_mrp_add_port - adds a port to a MRP instance. This will eventually call the
  switchdev API to notify the HW that the port is part of a ring so it needs to
  process or forward MRP frames on the other port.
br_mrp_del_port - deletes a port from a MRP instance. This will eventually call
  switchdev API to notify the HW that the port is not part of a ring anymore. So
  it would not need to do special processing to MRP frames
Whenever a port is added to the MRP instance, the also the SW needs to know this
information in case the HW can't support MRP. This information is required when
the SW bridge receives MRP frames. Because in case a frame arrived on an MRP
port the SW bridge should not forward the frame.

br_mrp_port_state - changes the port state. The port can be in forwarding state,
  which means that the frames can pass through or in blocked state which means
  that the frames can't pass through except MRP frames. This will eventually
  call the switchdev API to notify the HW. This information is used also by the
  SW bridge to know how to forward frames in case the HW doesn't have this
  capability.

br_mrp_port_role - a port role can be primary or secondary. This information is
  required to be pushed to HW in case the HW can generate MRP_Test frames.
  Because the MRP_Test frames contains a file with this information. Otherwise
  the HW will not be able to generate the frames correctly.

br_mrp_ring_state - a ring can be in state open or closed. State open means that
  the mrp port stopped receiving MRP_Test frames, while closed means that the
  mrp port received MRP_Test frames. Similar with br_mrp_port_role, this
  information is pushed in HW because the MRP_Test frames contain this
  information.

For all the previous commands the userspace doesn't need to check the return
value because it is not affected if the HW supports these or not.

br_mrp_ring_role - a ring can have the following roles MRM or MRC. For the role
  MRM it is expected that the HW can terminate the MRP frames, notify the SW
  that it stopped receiving MRP_Test frames and trapp all the other MRP frames.
  While for MRC mode it is expected that the HW can forward the MRP frames only
  between the MRP ports and copy MRP_Topology frames to CPU. In case the HW
  doesn't support a role it needs to return an error code different than
  -EOPNOTSUPP.

Because the userspace doesn't know if the kernel has HW offload capabilities
it is using the return value of the netlink calls to know if there was a problem
setting the role to the HW, or it should run the role in userspace. For example
if the node doesn't have a switchdev driver than the return code is -EOPNOTSUPP
that means that the state machine can run only in SW. If the node has switchdev
support then, if the node doesn't support the role it needs to return an error
code different than -EOPNOTSUPP. In this case the entire userspace application
will stop. If the node support the role then it returns 0.

br_mrp_start_test - this starts/stops the generation of MRP_Test frames. To stop
  the generation of frames the interval needs to have a value of 0. In this case
  the userspace needs to know if the HW supports this or not. Not to have
  duplicate frames(generated by HW and SW). Because if the HW supports this then
  the SW will not generate anymore frames and will expect that the HW will
  notify when it stopped receiving MRP frames using the function
  br_mrp_port_open.

br_mrp_flush - will flush the FDB.

br_mrp_port_open - this function is used by drivers to notify the userspace via
  a netlink callback that one of the ports stopped receiving MRP_Test frames.
  This function is called only when the node has the role MRM. It is not
  supposed to be called from userspace.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 net/bridge/br_private_mrp.h | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 net/bridge/br_private_mrp.h

Comments

Andrew Lunn Jan. 24, 2020, 5:43 p.m. UTC | #1
> br_mrp_flush - will flush the FDB.

How does this differ from a normal bridge flush? I assume there is a
way for user space to flush the bridge FDB.

    Andrew
Horatiu Vultur Jan. 25, 2020, 11:37 a.m. UTC | #2
The 01/24/2020 18:43, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > br_mrp_flush - will flush the FDB.
> 
> How does this differ from a normal bridge flush? I assume there is a
> way for user space to flush the bridge FDB.

Hi,

If I seen corectly the normal bridge flush will clear the entire FDB for
all the ports of the bridge. In this case it is require to clear FDB
entries only for the ring ports. In the next series I will add a better
description of this function and update also the implementation.

The user space doesn't know and doesn't contain a FDB. The user space
will just call the kernel(via netlink interface) to clear the FDB. And
the netlink call will eventually call this function.

> 
>     Andrew
Andrew Lunn Jan. 25, 2020, 3:20 p.m. UTC | #3
On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
> The 01/24/2020 18:43, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > br_mrp_flush - will flush the FDB.
> > 
> > How does this differ from a normal bridge flush? I assume there is a
> > way for user space to flush the bridge FDB.
> 
> Hi,
> 
> If I seen corectly the normal bridge flush will clear the entire FDB for
> all the ports of the bridge. In this case it is require to clear FDB
> entries only for the ring ports.

Maybe it would be better to extend the current bridge netlink call to
be able to pass an optional interface to be flushed?  I'm not sure it
is a good idea to have two APIs doing very similar things.

   Andrew
Allan W. Nielsen Jan. 25, 2020, 7:16 p.m. UTC | #4
On 25.01.2020 16:20, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>> The 01/24/2020 18:43, Andrew Lunn wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > > br_mrp_flush - will flush the FDB.
>> >
>> > How does this differ from a normal bridge flush? I assume there is a
>> > way for user space to flush the bridge FDB.
>>
>> Hi,
>>
>> If I seen corectly the normal bridge flush will clear the entire FDB for
>> all the ports of the bridge. In this case it is require to clear FDB
>> entries only for the ring ports.
>
>Maybe it would be better to extend the current bridge netlink call to
>be able to pass an optional interface to be flushed?  I'm not sure it
>is a good idea to have two APIs doing very similar things.
I agree.

And when looking at this again, I start to think that we should have
extended the existing netlink interface with new commands, instead of
adding a generic netlink.

/Allan
Horatiu Vultur Jan. 26, 2020, 1:28 p.m. UTC | #5
The 01/25/2020 20:16, Allan W. Nielsen wrote:
> On 25.01.2020 16:20, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
> > > The 01/24/2020 18:43, Andrew Lunn wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > >
> > > > > br_mrp_flush - will flush the FDB.
> > > >
> > > > How does this differ from a normal bridge flush? I assume there is a
> > > > way for user space to flush the bridge FDB.
> > > 
> > > Hi,
> > > 
> > > If I seen corectly the normal bridge flush will clear the entire FDB for
> > > all the ports of the bridge. In this case it is require to clear FDB
> > > entries only for the ring ports.
> > 
> > Maybe it would be better to extend the current bridge netlink call to
> > be able to pass an optional interface to be flushed?  I'm not sure it
> > is a good idea to have two APIs doing very similar things.
> I agree.
I would look over this.

> 
> And when looking at this again, I start to think that we should have
> extended the existing netlink interface with new commands, instead of
> adding a generic netlink.
We could do also that. The main reason why I have added a new generic
netlink was that I thought it would be clearer what commands are for MRP
configuration. But if you think that we should go forward by extending
existing netlink interface, that is perfectly fine for me.

> 
> /Allan
>
Andrew Lunn Jan. 26, 2020, 3:39 p.m. UTC | #6
> We could do also that. The main reason why I have added a new generic
> netlink was that I thought it would be clearer what commands are for MRP
> configuration.

The naming makes this clear, having _MRP_ in the attribute names etc.

But it would be good have input from the Bridge maintainers.

    Andrew
Nikolay Aleksandrov Feb. 20, 2020, 9:08 a.m. UTC | #7
On 26/01/2020 15:28, Horatiu Vultur wrote:
> The 01/25/2020 20:16, Allan W. Nielsen wrote:
>> On 25.01.2020 16:20, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>>>> The 01/24/2020 18:43, Andrew Lunn wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>>> br_mrp_flush - will flush the FDB.
>>>>>
>>>>> How does this differ from a normal bridge flush? I assume there is a
>>>>> way for user space to flush the bridge FDB.
>>>>
>>>> Hi,
>>>>
>>>> If I seen corectly the normal bridge flush will clear the entire FDB for
>>>> all the ports of the bridge. In this case it is require to clear FDB
>>>> entries only for the ring ports.
>>>
>>> Maybe it would be better to extend the current bridge netlink call to
>>> be able to pass an optional interface to be flushed?  I'm not sure it
>>> is a good idea to have two APIs doing very similar things.
>> I agree.
> I would look over this.
> 

There's already a way to flush FDBs per-port - IFLA_BRPORT_FLUSH.

>>
>> And when looking at this again, I start to think that we should have
>> extended the existing netlink interface with new commands, instead of
>> adding a generic netlink.
> We could do also that. The main reason why I have added a new generic
> netlink was that I thought it would be clearer what commands are for MRP
> configuration. But if you think that we should go forward by extending
> existing netlink interface, that is perfectly fine for me.
> 
>>
>> /Allan
>>
> 

I don't mind extending the current netlink interface but the bridge already has
a huge (the largest) set of options and each time we add a new option we have
to adjust RTNL_MAX_TYPE. If you do decide to go this way maybe look into nesting
all the MRP options under one master MRP element into the bridge options, example:
[IFLA_BR_MRP]
  [IFLA_BR_MRP_X]
  [IFLA_BR_MRP_Y]
  ...

To avoid increasing the stack usage and bumping the max rtnl type too much.

Cheers,
 Nik
Allan W. Nielsen Feb. 20, 2020, 1 p.m. UTC | #8
On 20.02.2020 11:08, Nikolay Aleksandrov wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>On 26/01/2020 15:28, Horatiu Vultur wrote:
>> The 01/25/2020 20:16, Allan W. Nielsen wrote:
>>> On 25.01.2020 16:20, Andrew Lunn wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On Sat, Jan 25, 2020 at 12:37:26PM +0100, Horatiu Vultur wrote:
>>>>> The 01/24/2020 18:43, Andrew Lunn wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>>> br_mrp_flush - will flush the FDB.
>>>>>>
>>>>>> How does this differ from a normal bridge flush? I assume there is a
>>>>>> way for user space to flush the bridge FDB.
>>>>>
>>>>> Hi,
>>>>>
>>>>> If I seen corectly the normal bridge flush will clear the entire FDB for
>>>>> all the ports of the bridge. In this case it is require to clear FDB
>>>>> entries only for the ring ports.
>>>>
>>>> Maybe it would be better to extend the current bridge netlink call to
>>>> be able to pass an optional interface to be flushed?  I'm not sure it
>>>> is a good idea to have two APIs doing very similar things.
>>> I agree.
>> I would look over this.
>>
>
>There's already a way to flush FDBs per-port - IFLA_BRPORT_FLUSH.
>
>>>
>>> And when looking at this again, I start to think that we should have
>>> extended the existing netlink interface with new commands, instead of
>>> adding a generic netlink.
>> We could do also that. The main reason why I have added a new generic
>> netlink was that I thought it would be clearer what commands are for MRP
>> configuration. But if you think that we should go forward by extending
>> existing netlink interface, that is perfectly fine for me.
>>
>>>
>>> /Allan
>>>
>>
>
>I don't mind extending the current netlink interface but the bridge already has
>a huge (the largest) set of options and each time we add a new option we have
>to adjust RTNL_MAX_TYPE. If you do decide to go this way maybe look into nesting
>all the MRP options under one master MRP element into the bridge options, example:
>[IFLA_BR_MRP]
>  [IFLA_BR_MRP_X]
>  [IFLA_BR_MRP_Y]
>  ...
Ahh, did not see this mail before responsing to the other one.

We can make it part of the BR netlink then.

/Allan

Patch
diff mbox series

diff --git a/net/bridge/br_private_mrp.h b/net/bridge/br_private_mrp.h
new file mode 100644
index 000000000000..bea4ece4411c
--- /dev/null
+++ b/net/bridge/br_private_mrp.h
@@ -0,0 +1,42 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _BR_PRIVATE_MRP_H_
+#define _BR_PRIVATE_MRP_H_
+
+#include "br_private.h"
+#include <uapi/linux/mrp_bridge.h>
+
+struct br_mrp {
+	/* list of mrp instances */
+	struct list_head		list;
+
+	struct net_bridge		*br;
+	struct net_bridge_port		*p_port;
+	struct net_bridge_port		*s_port;
+
+	u32				ring_nr;
+};
+
+/* br_mrp.c */
+int br_mrp_add(struct net_bridge *br, u32 ring_nr);
+int br_mrp_add_port(struct net_bridge *br, u32 ring_nr,
+		    struct net_bridge_port *p);
+int br_mrp_del(struct net_bridge *br, u32 ring_nr);
+int br_mrp_del_port(struct net_bridge_port *p);
+int br_mrp_set_port_state(struct net_bridge_port *p,
+			  enum br_mrp_port_state_type state);
+int br_mrp_set_port_role(struct net_bridge_port *p, u32 ring_nr,
+			 enum br_mrp_port_role_type role);
+int br_mrp_set_ring_state(struct net_bridge *br, u32 ring_nr,
+			  enum br_mrp_ring_state_type state);
+int br_mrp_set_ring_role(struct net_bridge *br, u32 ring_nr,
+			 enum br_mrp_ring_role_type role);
+int br_mrp_start_test(struct net_bridge *br, u32 ring_nr, u32 interval,
+		      u8 max_miss);
+int br_mrp_flush(struct net_bridge *br, u32 ring_nr);
+
+/* br_mrp_netlink.c */
+void br_mrp_port_open(struct net_device *dev, u8 loc);
+
+#endif /* _BR_PRIVATE_MRP_H */
+