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
