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

On 19/01/2021 16.52, Andrew Lunn wrote:

On Tue, Jan 19, 2021 at 09:32:40AM +0100, Horatiu Vultur wrote:

quoted

The 01/18/2021 21:27, Vladimir Oltean wrote:

quoted

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:

quoted

The 01/18/2021 19:46, Vladimir Oltean wrote:

quoted

On Mon, Jan 18, 2021 at 07:56:18PM +0100, Horatiu Vultur wrote:

quoted

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.

quoted

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