| Message ID | 20190222235927.10295-8-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: Remove switchdev_ops | expand |
On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: > Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field > from all clients, which were migrated to use switchdev notification in > the previous patches. > > Add a new function switchdev_port_attr_notify() that sends the switchdev > notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on > SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking > (atomic) notifier chain accordingly. > > Drop __switchdev_port_attr_set() and update switchdev_port_attr_set() > likewise. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > net/switchdev/switchdev.c | 96 +++++++++++---------------------------- > 1 file changed, 26 insertions(+), 70 deletions(-) > > diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c > index 94400f5b8e07..a1f16836ef89 100644 > --- a/net/switchdev/switchdev.c > +++ b/net/switchdev/switchdev.c > @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device *dev, > return 0; > } > > -/** > - * switchdev_port_attr_get - Get port attribute Hmm, why do you remove it here? Can't you remove it in a separate patch? I thought we already got rid of it :p > - * > - * @dev: port device > - * @attr: attribute to get > - */ > -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr *attr) > +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt, > + struct net_device *dev, > + const struct switchdev_attr *attr, > + struct switchdev_trans *trans) > { > - const struct switchdev_ops *ops = dev->switchdev_ops; > - struct net_device *lower_dev; > - struct list_head *iter; > - struct switchdev_attr first = { > - .id = SWITCHDEV_ATTR_ID_UNDEFINED > - }; > - int err = -EOPNOTSUPP; > + int err; > + int rc; > > - if (ops && ops->switchdev_port_attr_get) > - return ops->switchdev_port_attr_get(dev, attr); > + struct switchdev_notifier_port_attr_info attr_info = { > + .attr = attr, > + .trans = trans, > + .handled = false, > + }; > > - if (attr->flags & SWITCHDEV_F_NO_RECURSE) > + if (attr & SWITCHDEV_F_DEFER) > + rc = call_switchdev_blocking_notifiers(nt, dev, > + &attr_info.info, NULL); > + else > + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); I don't believe this is needed. You're calling this function from switchdev_port_attr_set_now() which is always called from process context. switchdev_port_attr_set() takes care of that. Similar to switchdev_port_obj_add(). The event `SWITCHDEV_PORT_ATTR_SET` is therefore always blocking and drivers only need to take care of it from their blocking notifier. > + err = notifier_to_errno(rc); > + if (err) { > + WARN_ON(!attr_info.handled); > return err; > - > - /* Switch device port(s) may be stacked under > - * bond/team/vlan dev, so recurse down to get attr on > - * each port. Return -ENODATA if attr values don't > - * compare across ports. > - */ > - > - netdev_for_each_lower_dev(dev, lower_dev, iter) { > - err = switchdev_port_attr_get(lower_dev, attr); > - if (err) > - break; > - if (first.id == SWITCHDEV_ATTR_ID_UNDEFINED) > - first = *attr; > - else if (memcmp(&first, attr, sizeof(*attr))) > - return -ENODATA; > } > > - return err; > -} > -EXPORT_SYMBOL_GPL(switchdev_port_attr_get); > - > -static int __switchdev_port_attr_set(struct net_device *dev, > - const struct switchdev_attr *attr, > - struct switchdev_trans *trans) > -{ > - const struct switchdev_ops *ops = dev->switchdev_ops; > - struct net_device *lower_dev; > - struct list_head *iter; > - int err = -EOPNOTSUPP; > - > - if (ops && ops->switchdev_port_attr_set) { > - err = ops->switchdev_port_attr_set(dev, attr, trans); > - goto done; > - } > - > - if (attr->flags & SWITCHDEV_F_NO_RECURSE) > - goto done; > - > - /* Switch device port(s) may be stacked under > - * bond/team/vlan dev, so recurse down to set attr on > - * each port. > - */ > - > - netdev_for_each_lower_dev(dev, lower_dev, iter) { > - err = __switchdev_port_attr_set(lower_dev, attr, trans); > - if (err) > - break; > - } > - > -done: > - if (err == -EOPNOTSUPP && attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP) > - err = 0; > + if (!attr_info.handled) > + return -EOPNOTSUPP; > > - return err; > + return 0; > } > > static int switchdev_port_attr_set_now(struct net_device *dev, > @@ -267,7 +221,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev, > */ > > trans.ph_prepare = true; > - err = __switchdev_port_attr_set(dev, attr, &trans); > + err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr, > + &trans); > if (err) { > /* Prepare phase failed: abort the transaction. Any > * resources reserved in the prepare phase are > @@ -286,7 +241,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev, > */ > > trans.ph_prepare = false; > - err = __switchdev_port_attr_set(dev, attr, &trans); > + err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr, > + &trans); > WARN(err, "%s: Commit of attribute (id=%d) failed.\n", > dev->name, attr->id); > switchdev_trans_items_warn_destroy(dev, &trans); > -- > 2.17.1 >
Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : > On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: >> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field >> from all clients, which were migrated to use switchdev notification in >> the previous patches. >> >> Add a new function switchdev_port_attr_notify() that sends the switchdev >> notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on >> SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking >> (atomic) notifier chain accordingly. >> >> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set() >> likewise. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> net/switchdev/switchdev.c | 96 +++++++++++---------------------------- >> 1 file changed, 26 insertions(+), 70 deletions(-) >> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c >> index 94400f5b8e07..a1f16836ef89 100644 >> --- a/net/switchdev/switchdev.c >> +++ b/net/switchdev/switchdev.c >> @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device *dev, >> return 0; >> } >> >> -/** >> - * switchdev_port_attr_get - Get port attribute > > Hmm, why do you remove it here? Can't you remove it in a separate patch? > I thought we already got rid of it :p Yes it should have been removed, looks like my previous series did not that, I will send that separately. > >> - * >> - * @dev: port device >> - * @attr: attribute to get >> - */ >> -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr *attr) >> +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt, >> + struct net_device *dev, >> + const struct switchdev_attr *attr, >> + struct switchdev_trans *trans) >> { >> - const struct switchdev_ops *ops = dev->switchdev_ops; >> - struct net_device *lower_dev; >> - struct list_head *iter; >> - struct switchdev_attr first = { >> - .id = SWITCHDEV_ATTR_ID_UNDEFINED >> - }; >> - int err = -EOPNOTSUPP; >> + int err; >> + int rc; >> >> - if (ops && ops->switchdev_port_attr_get) >> - return ops->switchdev_port_attr_get(dev, attr); >> + struct switchdev_notifier_port_attr_info attr_info = { >> + .attr = attr, >> + .trans = trans, >> + .handled = false, >> + }; >> >> - if (attr->flags & SWITCHDEV_F_NO_RECURSE) >> + if (attr & SWITCHDEV_F_DEFER) >> + rc = call_switchdev_blocking_notifiers(nt, dev, >> + &attr_info.info, NULL); >> + else >> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); > > I don't believe this is needed. You're calling this function from > switchdev_port_attr_set_now() which is always called from process > context. switchdev_port_attr_set() takes care of that. Similar to > switchdev_port_obj_add(). Except for net/bridge/br_switchdev.c when we check the bridge port's flags support with PRE_BRIDGE_FLAGS. In that case we are executing from the caller (atomic) context and we can't defer otherwise that trumps the whole idea of being able to do a quick check and return that to the caller that we cannot support specific flags. How would you recommend approaching that?
On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote: > Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : > > On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: > >> - if (attr->flags & SWITCHDEV_F_NO_RECURSE) > >> + if (attr & SWITCHDEV_F_DEFER) > >> + rc = call_switchdev_blocking_notifiers(nt, dev, > >> + &attr_info.info, NULL); > >> + else > >> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); > > > > I don't believe this is needed. You're calling this function from > > switchdev_port_attr_set_now() which is always called from process > > context. switchdev_port_attr_set() takes care of that. Similar to > > switchdev_port_obj_add(). > > Except for net/bridge/br_switchdev.c when we check the bridge port's > flags support with PRE_BRIDGE_FLAGS. In that case we are executing from > the caller (atomic) context and we can't defer otherwise that trumps the > whole idea of being able to do a quick check and return that to the > caller that we cannot support specific flags. How would you recommend > approaching that? In this case you can invoke call_switchdev_notifiers() directly from br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will be gone and bridge code will invoke the notifiers directly.
On 2/25/19 1:49 AM, Ido Schimmel wrote: > On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote: >> Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : >>> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: >>>> - if (attr->flags & SWITCHDEV_F_NO_RECURSE) >>>> + if (attr & SWITCHDEV_F_DEFER) >>>> + rc = call_switchdev_blocking_notifiers(nt, dev, >>>> + &attr_info.info, NULL); >>>> + else >>>> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); >>> >>> I don't believe this is needed. You're calling this function from >>> switchdev_port_attr_set_now() which is always called from process >>> context. switchdev_port_attr_set() takes care of that. Similar to >>> switchdev_port_obj_add(). >> >> Except for net/bridge/br_switchdev.c when we check the bridge port's >> flags support with PRE_BRIDGE_FLAGS. In that case we are executing from >> the caller (atomic) context and we can't defer otherwise that trumps the >> whole idea of being able to do a quick check and return that to the >> caller that we cannot support specific flags. How would you recommend >> approaching that? > > In this case you can invoke call_switchdev_notifiers() directly from > br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will > be gone and bridge code will invoke the notifiers directly. That can be done, but it still requires the target driver (mlxsw, ocelot, dsa, etc.) to support attribute notification from blocking and non-blocking context. Are you fine with that?
On Mon, Feb 25, 2019 at 11:47:12AM -0800, Florian Fainelli wrote: > On 2/25/19 1:49 AM, Ido Schimmel wrote: > > On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote: > >> Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit : > >>> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote: > >>>> - if (attr->flags & SWITCHDEV_F_NO_RECURSE) > >>>> + if (attr & SWITCHDEV_F_DEFER) > >>>> + rc = call_switchdev_blocking_notifiers(nt, dev, > >>>> + &attr_info.info, NULL); > >>>> + else > >>>> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); > >>> > >>> I don't believe this is needed. You're calling this function from > >>> switchdev_port_attr_set_now() which is always called from process > >>> context. switchdev_port_attr_set() takes care of that. Similar to > >>> switchdev_port_obj_add(). > >> > >> Except for net/bridge/br_switchdev.c when we check the bridge port's > >> flags support with PRE_BRIDGE_FLAGS. In that case we are executing from > >> the caller (atomic) context and we can't defer otherwise that trumps the > >> whole idea of being able to do a quick check and return that to the > >> caller that we cannot support specific flags. How would you recommend > >> approaching that? > > > > In this case you can invoke call_switchdev_notifiers() directly from > > br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will > > be gone and bridge code will invoke the notifiers directly. > > That can be done, but it still requires the target driver (mlxsw, > ocelot, dsa, etc.) to support attribute notification from blocking and > non-blocking context. Are you fine with that? Yes. Sorry for the latency. I was away yesterday. Reviewed your v2 and only found one problem. Will run some tests now. Thanks!
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c index 94400f5b8e07..a1f16836ef89 100644 --- a/net/switchdev/switchdev.c +++ b/net/switchdev/switchdev.c @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device *dev, return 0; } -/** - * switchdev_port_attr_get - Get port attribute - * - * @dev: port device - * @attr: attribute to get - */ -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr *attr) +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt, + struct net_device *dev, + const struct switchdev_attr *attr, + struct switchdev_trans *trans) { - const struct switchdev_ops *ops = dev->switchdev_ops; - struct net_device *lower_dev; - struct list_head *iter; - struct switchdev_attr first = { - .id = SWITCHDEV_ATTR_ID_UNDEFINED - }; - int err = -EOPNOTSUPP; + int err; + int rc; - if (ops && ops->switchdev_port_attr_get) - return ops->switchdev_port_attr_get(dev, attr); + struct switchdev_notifier_port_attr_info attr_info = { + .attr = attr, + .trans = trans, + .handled = false, + }; - if (attr->flags & SWITCHDEV_F_NO_RECURSE) + if (attr & SWITCHDEV_F_DEFER) + rc = call_switchdev_blocking_notifiers(nt, dev, + &attr_info.info, NULL); + else + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL); + err = notifier_to_errno(rc); + if (err) { + WARN_ON(!attr_info.handled); return err; - - /* Switch device port(s) may be stacked under - * bond/team/vlan dev, so recurse down to get attr on - * each port. Return -ENODATA if attr values don't - * compare across ports. - */ - - netdev_for_each_lower_dev(dev, lower_dev, iter) { - err = switchdev_port_attr_get(lower_dev, attr); - if (err) - break; - if (first.id == SWITCHDEV_ATTR_ID_UNDEFINED) - first = *attr; - else if (memcmp(&first, attr, sizeof(*attr))) - return -ENODATA; } - return err; -} -EXPORT_SYMBOL_GPL(switchdev_port_attr_get); - -static int __switchdev_port_attr_set(struct net_device *dev, - const struct switchdev_attr *attr, - struct switchdev_trans *trans) -{ - const struct switchdev_ops *ops = dev->switchdev_ops; - struct net_device *lower_dev; - struct list_head *iter; - int err = -EOPNOTSUPP; - - if (ops && ops->switchdev_port_attr_set) { - err = ops->switchdev_port_attr_set(dev, attr, trans); - goto done; - } - - if (attr->flags & SWITCHDEV_F_NO_RECURSE) - goto done; - - /* Switch device port(s) may be stacked under - * bond/team/vlan dev, so recurse down to set attr on - * each port. - */ - - netdev_for_each_lower_dev(dev, lower_dev, iter) { - err = __switchdev_port_attr_set(lower_dev, attr, trans); - if (err) - break; - } - -done: - if (err == -EOPNOTSUPP && attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP) - err = 0; + if (!attr_info.handled) + return -EOPNOTSUPP; - return err; + return 0; } static int switchdev_port_attr_set_now(struct net_device *dev, @@ -267,7 +221,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev, */ trans.ph_prepare = true; - err = __switchdev_port_attr_set(dev, attr, &trans); + err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr, + &trans); if (err) { /* Prepare phase failed: abort the transaction. Any * resources reserved in the prepare phase are @@ -286,7 +241,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev, */ trans.ph_prepare = false; - err = __switchdev_port_attr_set(dev, attr, &trans); + err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr, + &trans); WARN(err, "%s: Commit of attribute (id=%d) failed.\n", dev->name, attr->id); switchdev_trans_items_warn_destroy(dev, &trans);
Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field from all clients, which were migrated to use switchdev notification in the previous patches. Add a new function switchdev_port_attr_notify() that sends the switchdev notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking (atomic) notifier chain accordingly. Drop __switchdev_port_attr_set() and update switchdev_port_attr_set() likewise. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/switchdev/switchdev.c | 96 +++++++++++---------------------------- 1 file changed, 26 insertions(+), 70 deletions(-)