[net-next,7/8] net: switchdev: Replace port attr set SDO with a notification
diff mbox series

Message ID 20190222235927.10295-8-f.fainelli@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • net: Remove switchdev_ops
Related show

Commit Message

Florian Fainelli Feb. 22, 2019, 11:59 p.m. UTC
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(-)

Comments

Ido Schimmel Feb. 23, 2019, 10:32 a.m. UTC | #1
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
>
Florian Fainelli Feb. 24, 2019, 4:47 p.m. UTC | #2
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?
Ido Schimmel Feb. 25, 2019, 9:49 a.m. UTC | #3
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.
Florian Fainelli Feb. 25, 2019, 7:47 p.m. UTC | #4
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?
Ido Schimmel Feb. 27, 2019, 12:32 p.m. UTC | #5
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!

Patch
diff mbox series

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);