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@xxxxxxxxx>
> ---
> 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
>
