[patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Some drivers need to implement both switchdev vlan ops and
vid_add/kill ndos. For that to work in bridge code, we need to try
switchdev op first when adding/deleting vlan id.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 net/bridge/br_vlan.c | 58 ++++++++++++++++++++--------------------------------
 1 file changed, 22 insertions(+), 36 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index eae07ee..975deb9 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -72,28 +72,20 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
 			  u16 vid, u16 flags)
 {
-	const struct net_device_ops *ops = dev->netdev_ops;
+	struct switchdev_obj_port_vlan v = {
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.flags = flags,
+		.vid_begin = vid,
+		.vid_end = vid,
+	};
 	int err;
 
-	/* If driver uses VLAN ndo ops, use 8021q to install vid
-	 * on device, otherwise try switchdev ops to install vid.
+	/* Try switchdev op first. In case it is not supported, fallback to
+	 * 8021q add.
 	 */
-
-	if (ops->ndo_vlan_rx_add_vid) {
-		err = vlan_vid_add(dev, br->vlan_proto, vid);
-	} else {
-		struct switchdev_obj_port_vlan v = {
-			.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-			.flags = flags,
-			.vid_begin = vid,
-			.vid_end = vid,
-		};
-
-		err = switchdev_port_obj_add(dev, &v.obj);
-		if (err == -EOPNOTSUPP)
-			err = 0;
-	}
-
+	err = switchdev_port_obj_add(dev, &v.obj);
+	if (err == -EOPNOTSUPP)
+		return vlan_vid_add(dev, br->vlan_proto, vid);
 	return err;
 }
 
@@ -122,27 +114,21 @@ static void __vlan_del_list(struct net_bridge_vlan *v)
 static int __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
 			  u16 vid)
 {
-	const struct net_device_ops *ops = dev->netdev_ops;
-	int err = 0;
+	struct switchdev_obj_port_vlan v = {
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vid_begin = vid,
+		.vid_end = vid,
+	};
+	int err;
 
-	/* If driver uses VLAN ndo ops, use 8021q to delete vid
-	 * on device, otherwise try switchdev ops to delete vid.
+	/* Try switchdev op first. In case it is not supported, fallback to
+	 * 8021q del.
 	 */
-
-	if (ops->ndo_vlan_rx_kill_vid) {
+	err = switchdev_port_obj_del(dev, &v.obj);
+	if (err == -EOPNOTSUPP) {
 		vlan_vid_del(dev, br->vlan_proto, vid);
-	} else {
-		struct switchdev_obj_port_vlan v = {
-			.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-			.vid_begin = vid,
-			.vid_end = vid,
-		};
-
-		err = switchdev_port_obj_del(dev, &v.obj);
-		if (err == -EOPNOTSUPP)
-			err = 0;
+		return 0;
 	}
-
 	return err;
 }
 
-- 
1.9.3

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Vivien Didelot]

Hi Jiri,

On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Some drivers need to implement both switchdev vlan ops and
> vid_add/kill ndos. For that to work in bridge code, we need to try
> switchdev op first when adding/deleting vlan id.

Just curious, when would a driver need to have both operations?

I kinda have the same question regarding ndo_fdb_{add,del} and the
bridge_{get,set}link equivalent, which is a bit confusing to me.

> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
>  net/bridge/br_vlan.c | 58 ++++++++++++++++++++--------------------------------
>  1 file changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index eae07ee..975deb9 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -72,28 +72,20 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
>  static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
>  			  u16 vid, u16 flags)
>  {
> -	const struct net_device_ops *ops = dev->netdev_ops;
> +	struct switchdev_obj_port_vlan v = {
> +		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> +		.flags = flags,
> +		.vid_begin = vid,
> +		.vid_end = vid,
> +	};
>  	int err;
>  
> -	/* If driver uses VLAN ndo ops, use 8021q to install vid
> -	 * on device, otherwise try switchdev ops to install vid.
> +	/* Try switchdev op first. In case it is not supported, fallback to
> +	 * 8021q add.
>  	 */
> -
> -	if (ops->ndo_vlan_rx_add_vid) {
> -		err = vlan_vid_add(dev, br->vlan_proto, vid);
> -	} else {
> -		struct switchdev_obj_port_vlan v = {
> -			.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
> -			.flags = flags,
> -			.vid_begin = vid,
> -			.vid_end = vid,
> -		};
> -
> -		err = switchdev_port_obj_add(dev, &v.obj);
> -		if (err == -EOPNOTSUPP)
> -			err = 0;
> -	}
> -
> +	err = switchdev_port_obj_add(dev, &v.obj);
> +	if (err == -EOPNOTSUPP)
> +		return vlan_vid_add(dev, br->vlan_proto, vid);

                err = vlan_vid_add(dev, br->vlan_proto, vid);

Just being picky: the above line would have been preferred to keep a
single return path, but this does not justify a v2 though.

>  	return err;
>  }

Thanks,
-v

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Scott Feldman]

On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> Hi Jiri,
>
> On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> Some drivers need to implement both switchdev vlan ops and
>> vid_add/kill ndos. For that to work in bridge code, we need to try
>> switchdev op first when adding/deleting vlan id.
>
> Just curious, when would a driver need to have both operations?

Ya, I was kind of curious of that myself. Is this because the driver
wants to support standalone VLANs using 802.1q module and vconfig, as
well as bridge vlans?  With the vlan support built into the bridge,
I've been working under the assumption that 802.1q module (and
vconfig) aren't needed, and vlans for a bridged and non-bridge port
can be managed using the "bridge" iproute2 cmd.

> I kinda have the same question regarding ndo_fdb_{add,del} and the
> bridge_{get,set}link equivalent, which is a bit confusing to me.

I had to look back at my commit 7f109539 to remind myself about the
vid_add/kill ndos and bridge_{get,set}link usage.   Maybe that
write-up helps?  I'm not following you on the ndo_fdb_add/del part of
your question.

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Jiri Pirko]

Sat, Oct 10, 2015 at 05:20:28AM CEST, sfeldma@gmail.com wrote:
>On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot
><vivien.didelot@savoirfairelinux.com> wrote:
>> Hi Jiri,
>>
>> On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> Some drivers need to implement both switchdev vlan ops and
>>> vid_add/kill ndos. For that to work in bridge code, we need to try
>>> switchdev op first when adding/deleting vlan id.
>>
>> Just curious, when would a driver need to have both operations?
>
>Ya, I was kind of curious of that myself. Is this because the driver
>wants to support standalone VLANs using 802.1q module and vconfig, as
>well as bridge vlans?  With the vlan support built into the bridge,
>I've been working under the assumption that 802.1q module (and
>vconfig) aren't needed, and vlans for a bridged and non-bridge port
>can be managed using the "bridge" iproute2 cmd.

Sure, but this is for standalone port device, without being bridged. In
that case if you want to use vlan on top of that, you need these ndos.

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Florian Fainelli]

2015-10-10 0:01 GMT-07:00 Jiri Pirko <jiri@resnulli.us>:
> Sat, Oct 10, 2015 at 05:20:28AM CEST, sfeldma@gmail.com wrote:
>>On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot
>><vivien.didelot@savoirfairelinux.com> wrote:
>>> Hi Jiri,
>>>
>>> On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> Some drivers need to implement both switchdev vlan ops and
>>>> vid_add/kill ndos. For that to work in bridge code, we need to try
>>>> switchdev op first when adding/deleting vlan id.
>>>
>>> Just curious, when would a driver need to have both operations?
>>
>>Ya, I was kind of curious of that myself. Is this because the driver
>>wants to support standalone VLANs using 802.1q module and vconfig, as
>>well as bridge vlans?  With the vlan support built into the bridge,
>>I've been working under the assumption that 802.1q module (and
>>vconfig) aren't needed, and vlans for a bridged and non-bridge port
>>can be managed using the "bridge" iproute2 cmd.
>
> Sure, but this is for standalone port device, without being bridged. In
> that case if you want to use vlan on top of that, you need these ndos.

Standalone VLAN configuration (that is, using vconfig/802.1q module)
is fairly limited, and only really makes sense, in its current
implementation for a host. By that, I mean, that we cannot configure
what is your default VLAN (the untagged one), and it does not matter
in the case of a host, because this is something that the other end:
the switch, has to have configured for you.

For netdevices from a switch device, I think we should really either
have the ability to use the same configuration flexibility as the
bridge VLAN filtering offers (setting PVID, untagged/default VLAN,
tagged), either outside of the bridge layer (which could be of limited
use), or just enforce the need of a bridge device to span these
specific ports of a switch.
-- 
Florian

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Vivien Didelot]

On Oct. Friday 09 (41) 08:20 PM, Scott Feldman wrote:
> On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot
> <vivien.didelot@savoirfairelinux.com> wrote:
> > Hi Jiri,
> >
> > On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@mellanox.com>
> >>
> >> Some drivers need to implement both switchdev vlan ops and
> >> vid_add/kill ndos. For that to work in bridge code, we need to try
> >> switchdev op first when adding/deleting vlan id.
> >
> > Just curious, when would a driver need to have both operations?
> 
> Ya, I was kind of curious of that myself. Is this because the driver
> wants to support standalone VLANs using 802.1q module and vconfig, as
> well as bridge vlans?  With the vlan support built into the bridge,
> I've been working under the assumption that 802.1q module (and
> vconfig) aren't needed, and vlans for a bridged and non-bridge port
> can be managed using the "bridge" iproute2 cmd.
> 
> > I kinda have the same question regarding ndo_fdb_{add,del} and the
> > bridge_{get,set}link equivalent, which is a bit confusing to me.
> 
> I had to look back at my commit 7f109539 to remind myself about the
> vid_add/kill ndos and bridge_{get,set}link usage.   Maybe that
> write-up helps?  I'm not following you on the ndo_fdb_add/del part of
> your question.

Sorry I wasn't clear. What is confusing to me for FDB ops is that we
define net_device_ops like:

        .ndo_fdb_add            = switchdev_port_fdb_add,                           
        .ndo_fdb_del            = switchdev_port_fdb_del,
        .ndo_fdb_dump           = switchdev_port_fdb_dump,

But if I'm not mistaken, "bridge fdb" commands use the
.ndo_bridge_{get,set,del}link ops, isn't it?

Thanks,
-v

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Scott Feldman]

On Sat, Oct 10, 2015 at 9:03 AM, Vivien Didelot
<vivien.didelot@savoirfairelinux.com> wrote:
> On Oct. Friday 09 (41) 08:20 PM, Scott Feldman wrote:
>> On Fri, Oct 9, 2015 at 3:44 PM, Vivien Didelot
>> <vivien.didelot@savoirfairelinux.com> wrote:
>> > Hi Jiri,
>> >
>> > On Oct. Friday 09 (41) 01:54 PM, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@mellanox.com>
>> >>
>> >> Some drivers need to implement both switchdev vlan ops and
>> >> vid_add/kill ndos. For that to work in bridge code, we need to try
>> >> switchdev op first when adding/deleting vlan id.
>> >
>> > Just curious, when would a driver need to have both operations?
>>
>> Ya, I was kind of curious of that myself. Is this because the driver
>> wants to support standalone VLANs using 802.1q module and vconfig, as
>> well as bridge vlans?  With the vlan support built into the bridge,
>> I've been working under the assumption that 802.1q module (and
>> vconfig) aren't needed, and vlans for a bridged and non-bridge port
>> can be managed using the "bridge" iproute2 cmd.
>>
>> > I kinda have the same question regarding ndo_fdb_{add,del} and the
>> > bridge_{get,set}link equivalent, which is a bit confusing to me.
>>
>> I had to look back at my commit 7f109539 to remind myself about the
>> vid_add/kill ndos and bridge_{get,set}link usage.   Maybe that
>> write-up helps?  I'm not following you on the ndo_fdb_add/del part of
>> your question.
>
> Sorry I wasn't clear. What is confusing to me for FDB ops is that we
> define net_device_ops like:
>
>         .ndo_fdb_add            = switchdev_port_fdb_add,
>         .ndo_fdb_del            = switchdev_port_fdb_del,
>         .ndo_fdb_dump           = switchdev_port_fdb_dump,
>
> But if I'm not mistaken, "bridge fdb" commands use the
> .ndo_bridge_{get,set,del}link ops, isn't it?

No, the "bridge fdb" cmds use the .ndo_fdb_xxx ops.

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[Scott Feldman]

On Fri, Oct 9, 2015 at 4:54 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Some drivers need to implement both switchdev vlan ops and
> vid_add/kill ndos. For that to work in bridge code, we need to try
> switchdev op first when adding/deleting vlan id.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>

Acked-by: Scott Feldman <sfeldma@gmail.com>

Re: [patch net-next] bridge: try switchdev op first in __vlan_vid_add/del

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri,  9 Oct 2015 13:54:11 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Some drivers need to implement both switchdev vlan ops and
> vid_add/kill ndos. For that to work in bridge code, we need to try
> switchdev op first when adding/deleting vlan id.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>

Applied, thanks.