[PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Rasmus Villemoes]

When using MRP with hardware that does understand the concept of
blocked or forwarding ports, but not the full MRP offload, we
currently fail to tell the hardware what state it should put the port
in when the ring is closed - resulting in a ring of forwarding ports
and all the trouble that comes with that.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---

I don't really understand why SWITCHDEV_ATTR_ID_MRP_PORT_STATE even
has to exist seperately from SWITCHDEV_ATTR_ID_PORT_STP_STATE, and
it's hard to tell what the difference might be since no kernel code
implements the former.

 net/bridge/br_mrp_switchdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
index ed547e03ace1..8a1c7953e57a 100644
--- a/net/bridge/br_mrp_switchdev.c
+++ b/net/bridge/br_mrp_switchdev.c
@@ -180,6 +180,24 @@ int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
 	int err;
 
 	err = switchdev_port_attr_set(p->dev, &attr);
+	if (err == -EOPNOTSUPP) {
+		attr.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE;
+		switch (state) {
+		case BR_MRP_PORT_STATE_DISABLED:
+		case BR_MRP_PORT_STATE_NOT_CONNECTED:
+			attr.u.stp_state = BR_STATE_DISABLED;
+			break;
+		case BR_MRP_PORT_STATE_BLOCKED:
+			attr.u.stp_state = BR_STATE_BLOCKING;
+			break;
+		case BR_MRP_PORT_STATE_FORWARDING:
+			attr.u.stp_state = BR_STATE_FORWARDING;
+			break;
+		default:
+			return err;
+		};
+		err = switchdev_port_attr_set(p->dev, &attr);
+	}
 	if (err && err != -EOPNOTSUPP)
 		br_warn(p->br, "error setting offload MRP state on port %u(%s)\n",
 			(unsigned int)p->port_no, p->dev->name);
-- 
2.23.0

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Horatiu Vultur]

The 01/18/2021 19:13, Rasmus Villemoes wrote:
> 

Hi Rasmus,

> When using MRP with hardware that does understand the concept of
> blocked or forwarding ports, but not the full MRP offload, we
> currently fail to tell the hardware what state it should put the port
> in when the ring is closed - resulting in a ring of forwarding ports
> and all the trouble that comes with that.

But why don't you implement the SWITCHDEV_ATTR_ID_MRP_PORT_STATE in your
driver? if already the HW understands the concept of block or forwarding?

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> 
> I don't really understand why SWITCHDEV_ATTR_ID_MRP_PORT_STATE even
> has to exist seperately from SWITCHDEV_ATTR_ID_PORT_STP_STATE, and
> it's hard to tell what the difference might be since no kernel code
> implements the former.

The reason was to stay away from STP, because you can't run these two
protocols at the same time. Even though in SW, we reuse port's state.
In our driver(which is not upstreamed), we currently implement
SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
SWITCHDEV_ATTR_ID_PORT_STP_STATE.

> 
>  net/bridge/br_mrp_switchdev.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/net/bridge/br_mrp_switchdev.c b/net/bridge/br_mrp_switchdev.c
> index ed547e03ace1..8a1c7953e57a 100644
> --- a/net/bridge/br_mrp_switchdev.c
> +++ b/net/bridge/br_mrp_switchdev.c
> @@ -180,6 +180,24 @@ int br_mrp_port_switchdev_set_state(struct net_bridge_port *p,
>         int err;
> 
>         err = switchdev_port_attr_set(p->dev, &attr);
> +       if (err == -EOPNOTSUPP) {
> +               attr.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE;
> +               switch (state) {
> +               case BR_MRP_PORT_STATE_DISABLED:
> +               case BR_MRP_PORT_STATE_NOT_CONNECTED:
> +                       attr.u.stp_state = BR_STATE_DISABLED;
> +                       break;
> +               case BR_MRP_PORT_STATE_BLOCKED:
> +                       attr.u.stp_state = BR_STATE_BLOCKING;
> +                       break;
> +               case BR_MRP_PORT_STATE_FORWARDING:
> +                       attr.u.stp_state = BR_STATE_FORWARDING;
> +                       break;
> +               default:
> +                       return err;
> +               };
> +               err = switchdev_port_attr_set(p->dev, &attr);
> +       }
>         if (err && err != -EOPNOTSUPP)
>                 br_warn(p->br, "error setting offload MRP state on port %u(%s)\n",
>                         (unsigned int)p->port_no, p->dev->name);
> --
> 2.23.0
> 

-- 
/Horatiu

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Vladimir Oltean]

On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> The reason was to stay away from STP, because you can't run these two
> protocols at the same time. Even though in SW, we reuse port's state.
> In our driver(which is not upstreamed), we currently implement
> SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> SWITCHDEV_ATTR_ID_PORT_STP_STATE.

And isn't Rasmus's approach reasonable, in that it allows unmodified
switchdev drivers to offload MRP port states without creating
unnecessary code churn?

Also, if it has no in-kernel users, why does it even exist as a
switchdev attribute?

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Horatiu Vultur]

The 01/18/2021 19:46, Vladimir Oltean wrote:
> 
> On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> > The reason was to stay away from STP, because you can't run these two
> > protocols at the same time. Even though in SW, we reuse port's state.
> > In our driver(which is not upstreamed), we currently implement
> > SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> > SWITCHDEV_ATTR_ID_PORT_STP_STATE.
> 
> And isn't Rasmus's approach reasonable, in that it allows unmodified
> switchdev drivers to offload MRP port states without creating
> unnecessary code churn?

I am sorry but I don't see this as the correct solution. In my opinion,
I would prefer to have 3 extra lines in the driver and have a better
view of what is happening. Than having 2 calls in the driver for
different protocols.
If it is not a problem to have STP calls when you configure the MRP,
then why not just remove SWITCHDEV_ATTR_ID_MRP_PORT_STATE?

> 
> Also, if it has no in-kernel users, why does it even exist as a
> switchdev attribute?

-- 
/Horatiu

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Vladimir Oltean]

On Mon, Jan 18, 2021 at 09:20:36PM +0100, Horatiu Vultur wrote:
> The 01/18/2021 19:46, Vladimir Oltean wrote:
> >
> > On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> > > The reason was to stay away from STP, because you can't run these two
> > > protocols at the same time. Even though in SW, we reuse port's state.
> > > In our driver(which is not upstreamed), we currently implement
> > > SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> > > SWITCHDEV_ATTR_ID_PORT_STP_STATE.
> >
> > And isn't Rasmus's approach reasonable, in that it allows unmodified
> > switchdev drivers to offload MRP port states without creating
> > unnecessary code churn?
>
> I am sorry but I don't see this as the correct solution. In my opinion,
> I would prefer to have 3 extra lines in the driver and have a better
> view of what is happening. Than having 2 calls in the driver for
> different protocols.

I think the question boils down to: is a MRP-unaware driver expected to
work with the current bridge MRP code?

> If it is not a problem to have STP calls when you configure the MRP,
> then why not just remove SWITCHDEV_ATTR_ID_MRP_PORT_STATE?

Good question, why not?

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Horatiu Vultur]

The 01/18/2021 21:27, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jan 18, 2021 at 09:20:36PM +0100, Horatiu Vultur wrote:
> > The 01/18/2021 19:46, Vladimir Oltean wrote:
> > >
> > > On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> > > > The reason was to stay away from STP, because you can't run these two
> > > > protocols at the same time. Even though in SW, we reuse port's state.
> > > > In our driver(which is not upstreamed), we currently implement
> > > > SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> > > > SWITCHDEV_ATTR_ID_PORT_STP_STATE.
> > >
> > > And isn't Rasmus's approach reasonable, in that it allows unmodified
> > > switchdev drivers to offload MRP port states without creating
> > > unnecessary code churn?
> >
> > I am sorry but I don't see this as the correct solution. In my opinion,
> > I would prefer to have 3 extra lines in the driver and have a better
> > view of what is happening. Than having 2 calls in the driver for
> > different protocols.
> 
> I think the question boils down to: is a MRP-unaware driver expected to
> work with the current bridge MRP code?

If the driver has switchdev support, then is not expected to work with
the current bridge MRP code.

For example, the Ocelot driver, it has switchdev support but no MRP
support so this is not expected to work.  The main reason is that MRP is
using as DMAC 01:15:4E:00:00:0x(where x is between 1-4) so then when
these frames will arrive to HW then they will just be flooded which is
the wrong behavior.

But, the Ocelot which is not MRP aware, it can behave as MRP node if the
callbacks are implemented. For example, in MRP you have a notion of MRC
(Client) which needs to forward MRP Test frames between 2 ports and copy
to CPU MRP TopologyChange frames and forward these frames between 2
ports. Then using some TCAM rules(match on DMAC and source port) you can
implement this because you can differentiate between Test and Topology
frames by using the last byte of the DMAC.

> 
> > If it is not a problem to have STP calls when you configure the MRP,
> > then why not just remove SWITCHDEV_ATTR_ID_MRP_PORT_STATE?
> 
> Good question, why not?

-- 
/Horatiu

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Andrew Lunn]

On Tue, Jan 19, 2021 at 09:32:40AM +0100, Horatiu Vultur wrote:
> The 01/18/2021 21:27, Vladimir Oltean wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Mon, Jan 18, 2021 at 09:20:36PM +0100, Horatiu Vultur wrote:
> > > The 01/18/2021 19:46, Vladimir Oltean wrote:
> > > >
> > > > On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> > > > > The reason was to stay away from STP, because you can't run these two
> > > > > protocols at the same time. Even though in SW, we reuse port's state.
> > > > > In our driver(which is not upstreamed), we currently implement
> > > > > SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> > > > > SWITCHDEV_ATTR_ID_PORT_STP_STATE.
> > > >
> > > > And isn't Rasmus's approach reasonable, in that it allows unmodified
> > > > switchdev drivers to offload MRP port states without creating
> > > > unnecessary code churn?
> > >
> > > I am sorry but I don't see this as the correct solution. In my opinion,
> > > I would prefer to have 3 extra lines in the driver and have a better
> > > view of what is happening. Than having 2 calls in the driver for
> > > different protocols.
> > 
> > I think the question boils down to: is a MRP-unaware driver expected to
> > work with the current bridge MRP code?
> 
> If the driver has switchdev support, then is not expected to work with
> the current bridge MRP code.

> 
> For example, the Ocelot driver, it has switchdev support but no MRP
> support so this is not expected to work.

Then ideally, we need switchdev core to be testing for the needed ops
and returning an error which prevents MRP being configured when it
cannot work.

       Andrew

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Horatiu Vultur]

The 01/19/2021 16:52, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Jan 19, 2021 at 09:32:40AM +0100, Horatiu Vultur wrote:
> > The 01/18/2021 21:27, Vladimir Oltean wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > >
> > > On Mon, Jan 18, 2021 at 09:20:36PM +0100, Horatiu Vultur wrote:
> > > > The 01/18/2021 19:46, Vladimir Oltean wrote:
> > > > >
> > > > > On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
> > > > > > The reason was to stay away from STP, because you can't run these two
> > > > > > protocols at the same time. Even though in SW, we reuse port's state.
> > > > > > In our driver(which is not upstreamed), we currently implement
> > > > > > SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
> > > > > > SWITCHDEV_ATTR_ID_PORT_STP_STATE.
> > > > >
> > > > > And isn't Rasmus's approach reasonable, in that it allows unmodified
> > > > > switchdev drivers to offload MRP port states without creating
> > > > > unnecessary code churn?
> > > >
> > > > I am sorry but I don't see this as the correct solution. In my opinion,
> > > > I would prefer to have 3 extra lines in the driver and have a better
> > > > view of what is happening. Than having 2 calls in the driver for
> > > > different protocols.
> > >
> > > I think the question boils down to: is a MRP-unaware driver expected to
> > > work with the current bridge MRP code?
> >
> > If the driver has switchdev support, then is not expected to work with
> > the current bridge MRP code.
> 
> >
> > For example, the Ocelot driver, it has switchdev support but no MRP
> > support so this is not expected to work.
> 
> Then ideally, we need switchdev core to be testing for the needed ops
> and returning an error which prevents MRP being configured when it
> cannot work.

Yes, that would be great, I had a look at the handled attribute of the
switchdev_notifier_port_attr_info but I am not sure.

But what about adding some 'if (IS_ENBABLED(NET_SWITCHDEV))' in br_mrp.c
and then calling the functions br_mrp_switchdev_ only if this is
enabled.  Then whenever the switchdev call returns an error then is
cleared that MRP can't be configured.

> 
>        Andrew

-- 
/Horatiu

Re: [PATCH net] net: mrp: use stp state as substitute for unimplemented mrp state

[Rasmus Villemoes]

On 19/01/2021 16.52, Andrew Lunn wrote:
> On Tue, Jan 19, 2021 at 09:32:40AM +0100, Horatiu Vultur wrote:
>> The 01/18/2021 21:27, Vladimir Oltean wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jan 18, 2021 at 09:20:36PM +0100, Horatiu Vultur wrote:
>>>> The 01/18/2021 19:46, Vladimir Oltean wrote:
>>>>>
>>>>> On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:
>>>>>> The reason was to stay away from STP, because you can't run these two
>>>>>> protocols at the same time. Even though in SW, we reuse port's state.
>>>>>> In our driver(which is not upstreamed), we currently implement
>>>>>> SWITCHDEV_ATTR_ID_MRP_PORT_STATE and just call the
>>>>>> SWITCHDEV_ATTR_ID_PORT_STP_STATE.
>>>>>
>>>>> And isn't Rasmus's approach reasonable, in that it allows unmodified
>>>>> switchdev drivers to offload MRP port states without creating
>>>>> unnecessary code churn?
>>>>
>>>> I am sorry but I don't see this as the correct solution. In my opinion,
>>>> I would prefer to have 3 extra lines in the driver and have a better
>>>> view of what is happening. Than having 2 calls in the driver for
>>>> different protocols.
>>>
>>> I think the question boils down to: is a MRP-unaware driver expected to
>>> work with the current bridge MRP code?
>>
>> If the driver has switchdev support, then is not expected to work with
>> the current bridge MRP code.
> 
>>
>> For example, the Ocelot driver, it has switchdev support but no MRP
>> support so this is not expected to work.
> 
> Then ideally, we need switchdev core to be testing for the needed ops
> and returning an error which prevents MRP being configured when it
> cannot work.

Why are we now discussing crippling switchdev code instead of making it
work? Yeah, this is not all that is needed to make MRP work with
existing switchdev/dsa drivers, but it's certainly part of the puzzle.
The patch at the beginning of this thread did that.

Another approach that I'd probably prefer even more, given that Horatiu
said that even in the only driver (and an out-of-tree at that) currently
knowing about SWITCHDEV_ATTR_ID_MRP_PORT_STATE actually just translates
that to the equivalent SWITCHDEV_ATTR_ID_PORT_STP_STATE, is to simply do
that translation back in br_mrp_port_switchdev_set_state() (i.e., not
having the translation as a fallback). We'd keep the netlink
IFLA_BRIDGE_MRP_PORT_STATE_STATE because that's already uapi, and there
_might_ be some future driver that would need to do something different,
but until then I don't see the point of duplicating code down the call
stack.

Rasmus