| Message ID | 20210118181319.25419-1-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mrp: use stp state as substitute for unimplemented mrp state | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 >
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?
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?
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?
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?
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
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
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
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);
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(+)