Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification

[Ido Schimmel]

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
>