thr3ads.net - Linux Ethernet Bridging - [Bridge] [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification [Feb 2019]

If this information is useful, please help other people find it:
Share via:

Ido Schimmel

2019-Feb-23 10:32 UTC

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

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 at 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
>

Florian Fainelli

2019-Feb-24 16:47 UTC

head link

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

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 at 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?
-- 
Florian

Linux Ethernet Bridging - Feb 2019 - [Bridge] [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification

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

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