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

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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