Message ID | 20190210175105.31629-7-f.fainelli@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: Remove switchdev_ops | expand |
On Sun, Feb 10, 2019 at 09:50:55AM -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(), we can move straight to trying to set the > desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS. > > Acked-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > net/bridge/br_switchdev.c | 20 +++----------------- > 1 file changed, 3 insertions(+), 17 deletions(-) > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index db9e8ab96d48..939f300522c5 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -64,29 +64,15 @@ 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_BRIDGE_FLAGS, > + .flags = SWITCHDEV_F_DEFER, How does this work? You defer the operation, so the driver cannot veto it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT which is not deferred. > + .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) > - return err; > - > - /* Check if specific bridge flag attribute offload is supported */ > - if (!(attr.u.brport_flags_support & mask)) { > - br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", > - (unsigned int)p->port_no, p->dev->name); > - return -EOPNOTSUPP; > - } > - > - 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.19.1 >
Le 2/10/19 à 11:05 AM, Ido Schimmel a écrit : > On Sun, Feb 10, 2019 at 09:50:55AM -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(), we can move straight to trying to set the >> desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS. >> >> Acked-by: Jiri Pirko <jiri@mellanox.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> net/bridge/br_switchdev.c | 20 +++----------------- >> 1 file changed, 3 insertions(+), 17 deletions(-) >> >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >> index db9e8ab96d48..939f300522c5 100644 >> --- a/net/bridge/br_switchdev.c >> +++ b/net/bridge/br_switchdev.c >> @@ -64,29 +64,15 @@ 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_BRIDGE_FLAGS, >> + .flags = SWITCHDEV_F_DEFER, > > How does this work? You defer the operation, so the driver cannot veto > it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT > which is not deferred. I missed that indeed, how would you feel about splitting the attribute setting into different phases: - checking that the attribute setting is supported (caller context, so possibly atomic) - allocating and committing resources (deferred context) For pretty much any DSA driver, we will have to be in sleepable context anyway because of MDIO, SPI, I2C, whatever transport layer. Not sure how to best approach this now...
On Sun, Feb 10, 2019 at 11:34:14AM -0800, Florian Fainelli wrote: > Le 2/10/19 à 11:05 AM, Ido Schimmel a écrit : > > On Sun, Feb 10, 2019 at 09:50:55AM -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(), we can move straight to trying to set the > >> desired flag through SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS. > >> > >> Acked-by: Jiri Pirko <jiri@mellanox.com> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> net/bridge/br_switchdev.c | 20 +++----------------- > >> 1 file changed, 3 insertions(+), 17 deletions(-) > >> > >> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > >> index db9e8ab96d48..939f300522c5 100644 > >> --- a/net/bridge/br_switchdev.c > >> +++ b/net/bridge/br_switchdev.c > >> @@ -64,29 +64,15 @@ 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_BRIDGE_FLAGS, > >> + .flags = SWITCHDEV_F_DEFER, > > > > How does this work? You defer the operation, so the driver cannot veto > > it. This is why we have SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT > > which is not deferred. > > I missed that indeed, how would you feel about splitting the attribute > setting into different phases: > > - checking that the attribute setting is supported (caller context, so > possibly atomic) > - allocating and committing resources (deferred context) Yes, this is what I suggested in the other thread. Lets continue discussion there. We are doing that when processing route notifications (for example), but we are missing a check in caller context that resources can actually be allocated. That's part of a bigger task to track resources in mlxsw. > > For pretty much any DSA driver, we will have to be in sleepable context > anyway because of MDIO, SPI, I2C, whatever transport layer. > > Not sure how to best approach this now... > -- > Florian
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index db9e8ab96d48..939f300522c5 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -64,29 +64,15 @@ 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_BRIDGE_FLAGS, + .flags = SWITCHDEV_F_DEFER, + .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) - return err; - - /* Check if specific bridge flag attribute offload is supported */ - if (!(attr.u.brport_flags_support & mask)) { - br_warn(p->br, "bridge flag offload is not supported %u(%s)\n", - (unsigned int)p->port_no, p->dev->name); - return -EOPNOTSUPP; - } - - 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",