| Message ID | 20190213220638.1552-8-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: Get rid of switchdev_port_attr_get() | expand |
On Wed, Feb 13, 2019 at 02:06:36PM -0800, Florian Fainelli wrote: > Now that all switchdev drivers have been converted to checking the > bridge port flags during the prepare phase of the > switchdev_port_attr_set() when the process > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, we can avoid calling > switchdev_port_attr_get() with > SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > net/bridge/br_switchdev.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index db9e8ab96d48..8f88f8a1a7fa 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -64,29 +64,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, > { > struct switchdev_attr attr = { > .orig_dev = p->dev, > - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, > + .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, > + .u.brport_flags = flags, > }; > int err; > > if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD) > return 0; > > - err = switchdev_port_attr_get(p->dev, &attr); > - if (err == -EOPNOTSUPP) > - return 0; > - if (err) > + err = switchdev_port_attr_set(p->dev, &attr); > + if (err && err != -EOPNOTSUPP) > return err; > > - /* Check if specific bridge flag attribute offload is supported */ > - if (!(attr.u.brport_flags_support & mask)) { > + if (err == -EOPNOTSUPP) { > br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", > (unsigned int)p->port_no, p->dev->name); > - return -EOPNOTSUPP; > + return err; > } I see that you return -EOPNOTSUPP from drivers in case of unsupported flags. I believe this is problematic (I'll test soon). The same return code is used by: 1. Switch drivers to indicate unsupported flags 2. switchdev code to indicate unsupported netdev (no switchdev ops) I guess that with this patch any attempt to set bridge port flags on veth/dummy device will result in an error. > > attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS; > attr.flags = SWITCHDEV_F_DEFER; > - attr.u.brport_flags = flags; > + > err = switchdev_port_attr_set(p->dev, &attr); > if (err) { > br_warn(p->br, "error setting offload flag on port %u(%s)\n", > -- > 2.17.1 >
On Thu, Feb 14, 2019 at 01:20:02PM +0200, Ido Schimmel wrote: > On Wed, Feb 13, 2019 at 02:06:36PM -0800, Florian Fainelli wrote: > > Now that all switchdev drivers have been converted to checking the > > bridge port flags during the prepare phase of the > > switchdev_port_attr_set() when the process > > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, we can avoid calling > > switchdev_port_attr_get() with > > SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT. > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > net/bridge/br_switchdev.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > > index db9e8ab96d48..8f88f8a1a7fa 100644 > > --- a/net/bridge/br_switchdev.c > > +++ b/net/bridge/br_switchdev.c > > @@ -64,29 +64,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, > > { > > struct switchdev_attr attr = { > > .orig_dev = p->dev, > > - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, > > + .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, > > + .u.brport_flags = flags, > > }; > > int err; > > > > if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD) > > return 0; > > > > - err = switchdev_port_attr_get(p->dev, &attr); > > - if (err == -EOPNOTSUPP) > > - return 0; > > - if (err) > > + err = switchdev_port_attr_set(p->dev, &attr); > > + if (err && err != -EOPNOTSUPP) > > return err; > > > > - /* Check if specific bridge flag attribute offload is supported */ > > - if (!(attr.u.brport_flags_support & mask)) { > > + if (err == -EOPNOTSUPP) { > > br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", > > (unsigned int)p->port_no, p->dev->name); > > - return -EOPNOTSUPP; > > + return err; > > } > > I see that you return -EOPNOTSUPP from drivers in case of unsupported > flags. I believe this is problematic (I'll test soon). The same return > code is used by: > > 1. Switch drivers to indicate unsupported flags > 2. switchdev code to indicate unsupported netdev (no switchdev ops) > > I guess that with this patch any attempt to set bridge port flags on > veth/dummy device will result in an error. Yea, that's the case. You can test with tools/testing/selftests/net/forwarding/bridge_vlan_aware.sh and other bridge-related tests we have there. Another problem is that during PORT_PRE_BRIDGE_FLAGS you pass 'flags' and not 'mask'. This breaks mlxsw (and probably others as well) given BR_BCAST_FLOOD is set by default. > > > > > attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS; > > attr.flags = SWITCHDEV_F_DEFER; > > - attr.u.brport_flags = flags; > > + > > err = switchdev_port_attr_set(p->dev, &attr); > > if (err) { > > br_warn(p->br, "error setting offload flag on port %u(%s)\n", > > -- > > 2.17.1 > >
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index db9e8ab96d48..8f88f8a1a7fa 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -64,29 +64,27 @@ int br_switchdev_set_port_flag(struct net_bridge_port *p, { struct switchdev_attr attr = { .orig_dev = p->dev, - .id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT, + .id = SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, + .u.brport_flags = flags, }; int err; if (mask & ~BR_PORT_FLAGS_HW_OFFLOAD) return 0; - err = switchdev_port_attr_get(p->dev, &attr); - if (err == -EOPNOTSUPP) - return 0; - if (err) + err = switchdev_port_attr_set(p->dev, &attr); + if (err && err != -EOPNOTSUPP) return err; - /* Check if specific bridge flag attribute offload is supported */ - if (!(attr.u.brport_flags_support & mask)) { + if (err == -EOPNOTSUPP) { br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", (unsigned int)p->port_no, p->dev->name); - return -EOPNOTSUPP; + return err; } attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS; attr.flags = SWITCHDEV_F_DEFER; - attr.u.brport_flags = flags; + err = switchdev_port_attr_set(p->dev, &attr); if (err) { br_warn(p->br, "error setting offload flag on port %u(%s)\n",
Now that all switchdev drivers have been converted to checking the bridge port flags during the prepare phase of the switchdev_port_attr_set() when the process SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS, we can avoid calling switchdev_port_attr_get() with SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/bridge/br_switchdev.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)