[PATCH net-next 0/5] rocker: enable by default untagged VLAN support

[PATCH net-next 0/5] rocker: enable by default untagged VLAN support

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

This patch set is a followup to Simon Horman's RFC patch:

   [PATCH/RFC net-next] rocker: by default accept untagged packets

Now, on port probe, we install untagged VLAN (vid=0) support for each port
as the default.  This is equivalent to the command:

   bridge vlan add vid 0 dev DEV self

Accepting untagged VLAN pkts is a reasonable default, but the user could
override this with:

   bridge vlan del vid 0 dev DEV self

With this, we no longer need 8021q module to install vid=0 when port interface
opens.  In fact, we don't need support for legacy VLAN ndo ops at all since
they're superseded by bridge_setlink/dellink.  So remove legacy VLAN ndo ops
support in driver.  (The legacy VLAN ndo ops are supported by bonding/team
drivers, but don't fit into the transaction model offered by switchdev, so
switching all VLAN functions to bridge_setlink/dellink switchdev support gets
us stacked driver + transaction model support).

Scott Feldman (5):
  rocker: zero allocate ports array
  rocker: cleanup vlan table on error adding vlan
  rocker: install untagged VLAN (vid=0) support for each port
  rocker: install/remove router MAC for untagged VLAN when
    joining/leaving bridge
  rocker: remove support for legacy VLAN ndo ops

 drivers/net/ethernet/rocker/rocker.c |  105 ++++++++++++++++------------------
 1 file changed, 50 insertions(+), 55 deletions(-)

-- 
1.7.10.4

[PATCH net-next 1/5] rocker: zero allocate ports array

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

When allocating the array of rocker port pointers, zero the array values so
we can test for !NULL to see if port is allocated/registered.  We'll need
this later when installing untagged VLAN support for each port, during port
probe.  It's a long story, but to install a VLAN (vid=0 for untagged, in
this case) on a port, we'll need to scan other ports to see if the VLAN
group for that VLAN has been setup.  To scan the other ports, we need to
walk the port array.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 36f7edf..ab6d871 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4936,7 +4936,7 @@ static int rocker_probe_ports(struct rocker *rocker)
 	int err;
 
 	alloc_size = sizeof(struct rocker_port *) * rocker->port_count;
-	rocker->ports = kmalloc(alloc_size, GFP_KERNEL);
+	rocker->ports = kzalloc(alloc_size, GFP_KERNEL);
 	if (!rocker->ports)
 		return -ENOMEM;
 	for (i = 0; i < rocker->port_count; i++) {
-- 
1.7.10.4

[PATCH net-next 2/5] rocker: cleanup vlan table on error adding vlan

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Basic house keeping: If there is an error adding the router MAC for this
vlan, removing the just installed VLAN table entry to leave device in same
state as before failure.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index ab6d871..76e2281 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4342,7 +4342,12 @@ static int rocker_port_vlan_add(struct rocker_port *rocker_port,
 	if (err)
 		return err;
 
-	return rocker_port_router_mac(rocker_port, trans, 0, htons(vid));
+	err = rocker_port_router_mac(rocker_port, trans, 0, htons(vid));
+	if (err)
+		rocker_port_vlan(rocker_port, trans,
+				 ROCKER_OP_FLAG_REMOVE, vid);
+
+	return err;
 }
 
 static int rocker_port_vlans_add(struct rocker_port *rocker_port,
-- 
1.7.10.4

[PATCH net-next 3/5] rocker: install untagged VLAN (vid=0) support for each port

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

On port probe, install by default untagged VLAN support.  This is
equivalent to running the command:

	bridge vlan add vid 0 dev DEV self

A user could, if they wanted, manaully removing untagged support from the
port by running the command:

	bridge vlan del vid 0 dev DEV self

But installing it by default on port initialization gives the normal
expected behavior.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 76e2281..bd56273 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -3157,6 +3157,8 @@ static int rocker_port_vlan_flood_group(struct rocker_port *rocker_port,
 
 	for (i = 0; i < rocker->port_count; i++) {
 		p = rocker->ports[i];
+		if (!p)
+			continue;
 		if (!rocker_port_is_bridged(p))
 			continue;
 		if (test_bit(ntohs(vlan_id), p->vlan_bitmap)) {
@@ -3216,7 +3218,7 @@ static int rocker_port_vlan_l2_groups(struct rocker_port *rocker_port,
 
 	for (i = 0; i < rocker->port_count; i++) {
 		p = rocker->ports[i];
-		if (test_bit(ntohs(vlan_id), p->vlan_bitmap))
+		if (p && test_bit(ntohs(vlan_id), p->vlan_bitmap))
 			ref++;
 	}
 
@@ -4882,6 +4884,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 	const struct pci_dev *pdev = rocker->pdev;
 	struct rocker_port *rocker_port;
 	struct net_device *dev;
+	u16 untagged_vid = 0;
 	int err;
 
 	dev = alloc_etherdev(sizeof(struct rocker_port));
@@ -4917,16 +4920,27 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 
 	rocker_port_set_learning(rocker_port, SWITCHDEV_TRANS_NONE);
 
-	rocker_port->internal_vlan_id =
-		rocker_port_internal_vlan_id_get(rocker_port, dev->ifindex);
 	err = rocker_port_ig_tbl(rocker_port, SWITCHDEV_TRANS_NONE, 0);
 	if (err) {
 		dev_err(&pdev->dev, "install ig port table failed\n");
 		goto err_port_ig_tbl;
 	}
 
+	rocker_port->internal_vlan_id =
+		rocker_port_internal_vlan_id_get(rocker_port, dev->ifindex);
+
+	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
+				   untagged_vid, 0);
+	if (err) {
+		netdev_err(rocker_port->dev, "install untagged VLAN failed\n");
+		goto err_untagged_vlan;
+	}
+
 	return 0;
 
+err_untagged_vlan:
+	rocker_port_ig_tbl(rocker_port, SWITCHDEV_TRANS_NONE,
+			   ROCKER_OP_FLAG_REMOVE);
 err_port_ig_tbl:
 	unregister_netdev(dev);
 err_register_netdev:
-- 
1.7.10.4

[PATCH net-next 4/5] rocker: install/remove router MAC for untagged VLAN when joining/leaving bridge

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

When the port joins a bridge, the port's internal VLAN ID needs to change
to the bridge's internal VLAN ID.  Likewise, when leaving the bridge, the
internal VLAN ID reverts back the port's original internal VLAN ID.  (The
internal VLAN ID is used by device to internally mark untagged pkts with
some VLAN, which will eventually be removed on egress...think PVID).  When
the internal VLAN ID changes, we need to update the VLAN table entries and
the router MAC entries for IP/IPv6 to reflect the new internal VLAN ID.

This patch makes use of the common rocker_port_vlan_add/del functions to
make sure the tables are updated for the current internal VLAN ID.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   42 ++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index bd56273..3eb3eba 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -5178,41 +5178,49 @@ static bool rocker_port_dev_check(const struct net_device *dev)
 static int rocker_port_bridge_join(struct rocker_port *rocker_port,
 				   struct net_device *bridge)
 {
+	u16 untagged_vid = 0;
 	int err;
 
-	rocker_port_internal_vlan_id_put(rocker_port,
-					 rocker_port->dev->ifindex);
-
-	rocker_port->bridge_dev = bridge;
+	/* Port is joining bridge, so the internal VLAN for the
+	 * port is going to change to the bridge internal VLAN.
+	 * Let's remove untagged VLAN (vid=0) from port and
+	 * re-add once internal VLAN has changed.
+	 */
 
-	/* Use bridge internal VLAN ID for untagged pkts */
-	err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
-			       ROCKER_OP_FLAG_REMOVE, 0);
+	err = rocker_port_vlan_del(rocker_port, untagged_vid, 0);
 	if (err)
 		return err;
+
+	rocker_port_internal_vlan_id_put(rocker_port,
+					 rocker_port->dev->ifindex);
 	rocker_port->internal_vlan_id =
 		rocker_port_internal_vlan_id_get(rocker_port, bridge->ifindex);
-	return rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE, 0, 0);
+
+	rocker_port->bridge_dev = bridge;
+
+	return rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
+				    untagged_vid, 0);
 }
 
 static int rocker_port_bridge_leave(struct rocker_port *rocker_port)
 {
+	u16 untagged_vid = 0;
 	int err;
 
-	rocker_port_internal_vlan_id_put(rocker_port,
-					 rocker_port->bridge_dev->ifindex);
-
-	rocker_port->bridge_dev = NULL;
-
-	/* Use port internal VLAN ID for untagged pkts */
-	err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
-			       ROCKER_OP_FLAG_REMOVE, 0);
+	err = rocker_port_vlan_del(rocker_port, untagged_vid, 0);
 	if (err)
 		return err;
+
+	rocker_port_internal_vlan_id_put(rocker_port,
+					 rocker_port->bridge_dev->ifindex);
 	rocker_port->internal_vlan_id =
 		rocker_port_internal_vlan_id_get(rocker_port,
 						 rocker_port->dev->ifindex);
-	err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE, 0, 0);
+
+	rocker_port->bridge_dev = NULL;
+
+	err = rocker_port_vlan_add(rocker_port, SWITCHDEV_TRANS_NONE,
+				   untagged_vid, 0);
 	if (err)
 		return err;
 
-- 
1.7.10.4

[PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[sfeldma]

From: Scott Feldman <sfeldma@gmail.com>

Remove support for legacy ndo ops
.ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
bridge_setlink/dellink exclusively for VLAN add/del operations.

The legacy ops are needed if using 8021q driver module to setup VLANs on
the port.  But an alternative exists in using bridge_setlink/delink to
setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
to the newer setlink/dellink ops.  VLANs can added/delete from the port,
regardless if port is bridged or not, using the bridge commands:

	bridge vlan [add|del] vid VID dev DEV self

(Yes, I agree it's confusing to use the "bridge" command to set a VLAN on a
non-bridged port).

Using setlink/dellink over legacy ops let's us handle the stacked driver
case automatically.  It's built-in.  setlink also pass additional flags
(PVID, egress untagged) that aren't available with the legacy ops.

Signed-off-by: Scott Feldman <sfeldma@gmail.com>
---
 drivers/net/ethernet/rocker/rocker.c |   34 +---------------------------------
 1 file changed, 1 insertion(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 3eb3eba..e3fb97a 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -4185,35 +4185,6 @@ static int rocker_port_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
-static int rocker_port_vlan_rx_add_vid(struct net_device *dev,
-				       __be16 proto, u16 vid)
-{
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	int err;
-
-	err = rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE, 0, vid);
-	if (err)
-		return err;
-
-	return rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
-				      0, htons(vid));
-}
-
-static int rocker_port_vlan_rx_kill_vid(struct net_device *dev,
-					__be16 proto, u16 vid)
-{
-	struct rocker_port *rocker_port = netdev_priv(dev);
-	int err;
-
-	err = rocker_port_router_mac(rocker_port, SWITCHDEV_TRANS_NONE,
-				     ROCKER_OP_FLAG_REMOVE, htons(vid));
-	if (err)
-		return err;
-
-	return rocker_port_vlan(rocker_port, SWITCHDEV_TRANS_NONE,
-				ROCKER_OP_FLAG_REMOVE, vid);
-}
-
 static int rocker_port_get_phys_port_name(struct net_device *dev,
 					  char *buf, size_t len)
 {
@@ -4235,8 +4206,6 @@ static const struct net_device_ops rocker_port_netdev_ops = {
 	.ndo_stop			= rocker_port_stop,
 	.ndo_start_xmit			= rocker_port_xmit,
 	.ndo_set_mac_address		= rocker_port_set_mac_address,
-	.ndo_vlan_rx_add_vid		= rocker_port_vlan_rx_add_vid,
-	.ndo_vlan_rx_kill_vid		= rocker_port_vlan_rx_kill_vid,
 	.ndo_bridge_getlink		= switchdev_port_bridge_getlink,
 	.ndo_bridge_setlink		= switchdev_port_bridge_setlink,
 	.ndo_bridge_dellink		= switchdev_port_bridge_dellink,
@@ -4908,8 +4877,7 @@ static int rocker_probe_port(struct rocker *rocker, unsigned int port_number)
 		       NAPI_POLL_WEIGHT);
 	rocker_carrier_init(rocker_port);
 
-	dev->features |= NETIF_F_NETNS_LOCAL |
-			 NETIF_F_HW_VLAN_CTAG_FILTER;
+	dev->features |= NETIF_F_NETNS_LOCAL;
 
 	err = register_netdev(dev);
 	if (err) {
-- 
1.7.10.4

Re: [PATCH net-next 0/5] rocker: enable by default untagged VLAN support

[David Miller]

From: sfeldma@gmail.com
Date: Mon,  1 Jun 2015 11:39:01 -0700

> From: Scott Feldman <sfeldma@gmail.com>
> 
> This patch set is a followup to Simon Horman's RFC patch:
> 
>    [PATCH/RFC net-next] rocker: by default accept untagged packets
> 
> Now, on port probe, we install untagged VLAN (vid=0) support for each port
> as the default.  This is equivalent to the command:
> 
>    bridge vlan add vid 0 dev DEV self
> 
> Accepting untagged VLAN pkts is a reasonable default, but the user could
> override this with:
> 
>    bridge vlan del vid 0 dev DEV self
> 
> With this, we no longer need 8021q module to install vid=0 when port interface
> opens.  In fact, we don't need support for legacy VLAN ndo ops at all since
> they're superseded by bridge_setlink/dellink.  So remove legacy VLAN ndo ops
> support in driver.  (The legacy VLAN ndo ops are supported by bonding/team
> drivers, but don't fit into the transaction model offered by switchdev, so
> switching all VLAN functions to bridge_setlink/dellink switchdev support gets
> us stacked driver + transaction model support).

Series applied, although I do have some trepidation about patch #5 since it
removes a capability that existed beforehand so someone might (well, they will)
get confused if they try to configure vlans that way and it no longer works.

If anyone else expresses similar objections I will probably have to
revert, just FYI.

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Toshiaki Makita]

On 2015/06/02 3:39, sfeldma@gmail.com wrote:
> From: Scott Feldman <sfeldma@gmail.com>
> 
> Remove support for legacy ndo ops
> .ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
> bridge_setlink/dellink exclusively for VLAN add/del operations.
> 
> The legacy ops are needed if using 8021q driver module to setup VLANs on
> the port.  But an alternative exists in using bridge_setlink/delink to
> setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
> to the newer setlink/dellink ops.  VLANs can added/delete from the port,
> regardless if port is bridged or not, using the bridge commands:
> 
> 	bridge vlan [add|del] vid VID dev DEV self

Hi Scott,

This doesn't look transparent with bridge.

Before this patch, I was able to add vid in the same way as software bridge:

  ip link set DEV master br0
  bridge vlan add vid VID dev DEV

Now I need to add "self", which is different from software bridge...

Toshiaki Makita

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Tue, 02 Jun 2015 13:51:06 +0900

> On 2015/06/02 3:39, sfeldma@gmail.com wrote:
>> From: Scott Feldman <sfeldma@gmail.com>
>> 
>> Remove support for legacy ndo ops
>> .ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
>> bridge_setlink/dellink exclusively for VLAN add/del operations.
>> 
>> The legacy ops are needed if using 8021q driver module to setup VLANs on
>> the port.  But an alternative exists in using bridge_setlink/delink to
>> setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
>> to the newer setlink/dellink ops.  VLANs can added/delete from the port,
>> regardless if port is bridged or not, using the bridge commands:
>> 
>> 	bridge vlan [add|del] vid VID dev DEV self
> 
> Hi Scott,
> 
> This doesn't look transparent with bridge.
> 
> Before this patch, I was able to add vid in the same way as software bridge:
> 
>   ip link set DEV master br0
>   bridge vlan add vid VID dev DEV
> 
> Now I need to add "self", which is different from software bridge...

I'm already not liking the looks of this....

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Toshiaki Makita]

On 2015/06/02 14:24, David Miller wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Date: Tue, 02 Jun 2015 13:51:06 +0900
> 
>> On 2015/06/02 3:39, sfeldma@gmail.com wrote:
>>> From: Scott Feldman <sfeldma@gmail.com>
>>>
>>> Remove support for legacy ndo ops
>>> .ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
>>> bridge_setlink/dellink exclusively for VLAN add/del operations.
>>>
>>> The legacy ops are needed if using 8021q driver module to setup VLANs on
>>> the port.  But an alternative exists in using bridge_setlink/delink to
>>> setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
>>> to the newer setlink/dellink ops.  VLANs can added/delete from the port,
>>> regardless if port is bridged or not, using the bridge commands:
>>>
>>> 	bridge vlan [add|del] vid VID dev DEV self
>>
>> Hi Scott,
>>
>> This doesn't look transparent with bridge.
>>
>> Before this patch, I was able to add vid in the same way as software bridge:
>>
>>   ip link set DEV master br0
>>   bridge vlan add vid VID dev DEV
>>
>> Now I need to add "self", which is different from software bridge...
> 
> I'm already not liking the looks of this....

I thought one of purposes of implementing ndo_vlan_rx_add_vid() is to
make "bridge vlan add (master)" for bridged-ports work like software
bridges. Sorry if I misunderstood.

Toshiaki Makita

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman]

On Mon, Jun 1, 2015 at 10:24 PM, David Miller <davem@davemloft.net> wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Date: Tue, 02 Jun 2015 13:51:06 +0900
>
>> On 2015/06/02 3:39, sfeldma@gmail.com wrote:
>>> From: Scott Feldman <sfeldma@gmail.com>
>>>
>>> Remove support for legacy ndo ops
>>> .ndo_vlan_rx_add_vid/.ndo_vlan_rx_kill_vid.  Rocker will use
>>> bridge_setlink/dellink exclusively for VLAN add/del operations.
>>>
>>> The legacy ops are needed if using 8021q driver module to setup VLANs on
>>> the port.  But an alternative exists in using bridge_setlink/delink to
>>> setup VLANs, which doesn't depend on 8021q module.  So rocker will switch
>>> to the newer setlink/dellink ops.  VLANs can added/delete from the port,
>>> regardless if port is bridged or not, using the bridge commands:
>>>
>>>      bridge vlan [add|del] vid VID dev DEV self
>>
>> Hi Scott,
>>
>> This doesn't look transparent with bridge.
>>
>> Before this patch, I was able to add vid in the same way as software bridge:
>>
>>   ip link set DEV master br0
>>   bridge vlan add vid VID dev DEV
>>
>> Now I need to add "self", which is different from software bridge...
>
> I'm already not liking the looks of this....

Actually, we're now consistent with bridge man page which says master
is the default.

Want we want, I believe, is to adjust what the man page says (and the
bridge vlan command itself), by making the default master and self.
The kernel and driver are fine, it's the default in the bridge command
that needs adjusting.  Once we do this, we'll be back to transparent
with software-only bridge.

How did we get here?   So the RTM_SETLINK for PF_BRIDGE calls
rtnl_bridge_setlink().  rtnl_bridge_setlink() calls ndo_bridge_setlink
for the master (the bridge side of the port) and self (the device side
of the port), depending on if MASTER and/or SELF flags are set.  Since
the default from the iproute2 bridge vlan cmd is to only set MASTER,
only the bridge's ndo_bridge_setlink is called.  But if you dig down
into the bridge's ndo_bridge_setlink, you'll see it will call into the
port driver's ndo_vlan_rx_add_vid() to add the vlan to the device side
of the port.  So we have a MASTER cmd that is doing some SELF work.
My guess this was done to avoid having to update all the NIC drivers
from  ndo_vlan_rx_add_vid to ndo_bridge_setlink.  When you remove
ndo_vlan_rx_add_vid() from the port driver, the cmd needs to target
MASTER and SELF for both sides of the port to be called.  But the
current cmd only sets MASTER.  This is why you (currently) need to add
SELF for cmd to target the device side of the port.

On top of all of this, you can use RTM_SETLINK for PF_BRIDGE on
non-bridged ports, in which case only SELF is used to program the VLAN
on the device, using the device's ndo_bridge_setlink.  This is the
confusing part where you can set VLANs on non-bridged ports using the
bridge cmd.

To summarize, pseudo code for rtnl_bridge_setlink() is:

    rtnl_bridge_setlink()
        if MASTER
            call bridge's ndo_bridge_setlink()
                if bridge port implements ndo_vlan_rx_add_vid()
                    call ndo_vlan_rx_add_vid() on port device to set vlan
        if SELF
            call port device's ndo_bridge_setlink()

If DEV is bridged, today we have:

    bridge vlan add vid VID dev DEV                              sets
MASTER (default)
    bridge vlan add vid VID dev DEV master                   sets MASTER
    bridge vlan add vid VID dev DEV self                        sets SELF
    bridge vlan add vid VID dev DEV master self             sets MASTER and SELF

if DEV is not bridged, today we have:

    bridge vlan add vid VID dev DEV                              //
fails  (no master device)
    bridge vlan add vid VID dev DEV self                        sets SELF

What I propose is we change the bridge vlan cmd for the DEV bridged
case as such:

    bridge vlan add vid VID dev DEV                              sets
MASTER and SELF (default)
    bridge vlan add vid VID dev DEV master                   sets MASTER
    bridge vlan add vid VID dev DEV self                        sets SELF
    bridge vlan add vid VID dev DEV master self             sets MASTER and SELF

For existing users of ndo_vlan_rx_add_vid/ndo_vlan_rx_kill_vid,
nothing really changes.  If they also have an ndo_bridge_setlink,
it'll get called but they're not doing any vlan stuff there today
anyway, so it's ignored.

For rocker, we're switching to doing all vlan stuff in
ndo_bridge_setlink.  Switching to ndo_bridge_setlink for switchdev
gives us support for stacked drivers with the transaction model,
something we don't get with ndo_vlan_rx_add_vid.

If this makes sense, I'll post the follow up bridge vlan cmd change to
default to master and self.

-scott

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Jamal Hadi Salim]

On 06/02/15 03:10, Scott Feldman wrote:


> Actually, we're now consistent with bridge man page which says master
> is the default.
>
> Want we want, I believe, is to adjust what the man page says (and the
> bridge vlan command itself), by making the default master and self.
> The kernel and driver are fine, it's the default in the bridge command
> that needs adjusting.  Once we do this, we'll be back to transparent
> with software-only bridge.
>

Question to ask when looking at something of this nature:
Will it work with no suprises if you used today's unmodified app?
The default behavior shouldnt change and unfortunately it does here.

It is not just iproute2 - since this is breaking ABI expectations.
Looking at some app i wrote a while back based on analyzing kernel
expectations at the time, I see the following logic:

user can set master or self on command line.
...
....
if (user DID NOT set master_on || user set self on)
    then set self to on

iow, current behavior:
  01: master is only set if user explicitly asked.
  11: master|self when user explicitly sets both
  10: self is on by default when the user doesnt specify anything
  00: and the last option is to have none set which is not
      possible since we have defaults.

cheers,
jamal


So this is very similar to iproute2 - if nothing is set
it defaults to self.

cheers,
jamal

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman]

On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 06/02/15 03:10, Scott Feldman wrote:
>
>
>> Actually, we're now consistent with bridge man page which says master
>> is the default.
>>
>> Want we want, I believe, is to adjust what the man page says (and the
>> bridge vlan command itself), by making the default master and self.
>> The kernel and driver are fine, it's the default in the bridge command
>> that needs adjusting.  Once we do this, we'll be back to transparent
>> with software-only bridge.
>>
>
> Question to ask when looking at something of this nature:
> Will it work with no suprises if you used today's unmodified app?
> The default behavior shouldnt change and unfortunately it does here.

The default behavior does change, yes, but there shouldn't be any
surprises even if using today's unmodified app.  The reason why is no
in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
three drivers that have ndo_bridge_setlink use if to set hwmode to
VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
master to master|self, the bridge's
ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
driver's using ndo_vlan_rx_add_vid, and if they implement
ndo_bridge_setlink, they'll get called a second time but will noop
because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.

So it comes down to two choices:

1) break ABI, which is inconsequential for in-kernel drivers and
preserve (iproute2) command transparency, or

2) embrace existing behavior which is consistent with man pages but
breaks command transparency for any driver implementing
ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
can see the DSA going down this path also based on another concurrent
thread.

We're at option 2) right now.

> It is not just iproute2 - since this is breaking ABI expectations.
> Looking at some app i wrote a while back based on analyzing kernel
> expectations at the time, I see the following logic:
>
> user can set master or self on command line.
> ...
> ....
> if (user DID NOT set master_on || user set self on)
>    then set self to on
>
> iow, current behavior:
>  01: master is only set if user explicitly asked.
>  11: master|self when user explicitly sets both
>  10: self is on by default when the user doesnt specify anything
>  00: and the last option is to have none set which is not
>      possible since we have defaults.
>
> cheers,
> jamal
>
>
> So this is very similar to iproute2 - if nothing is set
> it defaults to self.

Ha, you're giving the behavior for "bridge fdb" command, where self is
the default.

For "bridge link" and "bridge vlan", the default is master.  The user
must explicitly specify "self" to act on the device side of the port.

It's unfortunate the iproute2 defaults aren't consistent between
commands.  Maybe someone knows the history here and can explain.

-scott

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[roopa]

On 6/2/15, 7:30 AM, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 06/02/15 03:10, Scott Feldman wrote:
>>
>>
>>> Actually, we're now consistent with bridge man page which says master
>>> is the default.
>>>
>>> Want we want, I believe, is to adjust what the man page says (and the
>>> bridge vlan command itself), by making the default master and self.
>>> The kernel and driver are fine, it's the default in the bridge command
>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>> with software-only bridge.
>>>
>> Question to ask when looking at something of this nature:
>> Will it work with no suprises if you used today's unmodified app?
>> The default behavior shouldnt change and unfortunately it does here.
> The default behavior does change, yes, but there shouldn't be any
> surprises even if using today's unmodified app.  The reason why is no
> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
> three drivers that have ndo_bridge_setlink use if to set hwmode to
> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
> master to master|self, the bridge's
> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
> driver's using ndo_vlan_rx_add_vid, and if they implement
> ndo_bridge_setlink, they'll get called a second time but will noop
> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>
> So it comes down to two choices:
>
> 1) break ABI, which is inconsequential for in-kernel drivers and
> preserve (iproute2) command transparency, or
>
> 2) embrace existing behavior which is consistent with man pages but
> breaks command transparency for any driver implementing
> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
> can see the DSA going down this path also based on another concurrent
> thread.
>
> We're at option 2) right now.
>
>> It is not just iproute2 - since this is breaking ABI expectations.
>> Looking at some app i wrote a while back based on analyzing kernel
>> expectations at the time, I see the following logic:
>>
>> user can set master or self on command line.
>> ...
>> ....
>> if (user DID NOT set master_on || user set self on)
>>     then set self to on
>>
>> iow, current behavior:
>>   01: master is only set if user explicitly asked.
>>   11: master|self when user explicitly sets both
>>   10: self is on by default when the user doesnt specify anything
>>   00: and the last option is to have none set which is not
>>       possible since we have defaults.
>>
>> cheers,
>> jamal
>>
>>
>> So this is very similar to iproute2 - if nothing is set
>> it defaults to self.
> Ha, you're giving the behavior for "bridge fdb" command, where self is
> the default.

Oh...i did not realize this was the case either. Thats unfortunate.

>
> For "bridge link" and "bridge vlan", the default is master.  The user
> must explicitly specify "self" to act on the device side of the port.
>
> It's unfortunate the iproute2 defaults aren't consistent between
> commands.  Maybe someone knows the history here and can explain.
>
>

scott, this brings back the discussion you and i had over the revert of 
my patches.. (commit id's at the end of this email)...
which used to seamlessly offload to switchdev from bridge driver if the 
port was a switch port (similar to stp state offload).

'self' used to exist before switchdev infra came in. My suggestion was 
to use it where required...but not build the switchdev api on the 
presence of 'self'. switchdev layer should be consistent across...all 
fib/fdb/neigh layers.

I was at some point going to try to propose restore of my patches (with 
proper error handling etc), and looking at where this conversation is 
going.....it appears that it is going to be difficult.



commit 8508025c598bdee33d9afa153e9c00c7771e7d63
Author: Scott Feldman <sfeldma@gmail.com>
Date:   Sun May 10 09:48:03 2015 -0700

     bridge: revert br_dellink change back to original

     This is revert of:

     commit 68e331c785b8 ("bridge: offload bridge port attributes to 
switch asic
     if feature flag set")

     Restore br_dellink back to original and don't call into SELF port 
driver.
     rtnetlink.c:bridge_dellink() already does a call into port driver 
for SELF.

     bridge vlan add/del cmd defaults to MASTER.  From man page for 
bridge vlan
     add/del cmd:

            self   the vlan is configured on the specified physical device.
                   Required if the device is the bridge device.

            master the vlan is configured on the software bridge (default).

     Signed-off-by: Scott Feldman <sfeldma@gmail.com>
     Acked-by: Jiri Pirko <jiri@resnulli.us>
     Signed-off-by: David S. Miller <davem@davemloft.net>

commit 41c498b9359e360f08723b7605ec0c40926ec415
Author: Scott Feldman <sfeldma@gmail.com>
Date:   Sun May 10 09:47:59 2015 -0700

     bridge: restore br_setlink back to original

     This is revert of:

     commit 68e331c785b8 ("bridge: offload bridge port attributes to 
switch asic
     if feature flag set")

     Restore br_setlink back to original and don't call into SELF port 
driver.
     rtnetlink.c:bridge_setlink() already does a call into port driver 
for SELF.

     bridge set link cmd defaults to MASTER.  From man page for bridge 
link set
     cmd:

            self   link setting is configured on specified physical device

            master link setting is configured on the software bridge 
(default)

     The link setting has two values: the device-side value and the software
     bridge-side value.  These are independent and settable using the bridge
     link set cmd by specifying some combination of [master] | [self].
     Furthermore, the device-side and bridge-side settings have their own
     initial value, viewable from bridge -d link show cmd.

     Restoring br_setlink back to original makes rocker (the only 
in-kernel user
     of SELF link settings) work as first implement: two-sided values.

     It's true that when both MASTER and SELF are specified from the 
command,
     two netlink notifications are generated, one for each side of the 
settings.
     The user-space app can distiquish between the two notifications by
     observing the MASTER or SELF flag.

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

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman]

On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> wrote:
> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>
>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>
>>>
>>>> Actually, we're now consistent with bridge man page which says master
>>>> is the default.
>>>>
>>>> Want we want, I believe, is to adjust what the man page says (and the
>>>> bridge vlan command itself), by making the default master and self.
>>>> The kernel and driver are fine, it's the default in the bridge command
>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>> with software-only bridge.
>>>>
>>> Question to ask when looking at something of this nature:
>>> Will it work with no suprises if you used today's unmodified app?
>>> The default behavior shouldnt change and unfortunately it does here.
>>
>> The default behavior does change, yes, but there shouldn't be any
>> surprises even if using today's unmodified app.  The reason why is no
>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>> master to master|self, the bridge's
>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>> driver's using ndo_vlan_rx_add_vid, and if they implement
>> ndo_bridge_setlink, they'll get called a second time but will noop
>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>
>> So it comes down to two choices:
>>
>> 1) break ABI, which is inconsequential for in-kernel drivers and
>> preserve (iproute2) command transparency, or
>>
>> 2) embrace existing behavior which is consistent with man pages but
>> breaks command transparency for any driver implementing
>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>> can see the DSA going down this path also based on another concurrent
>> thread.
>>
>> We're at option 2) right now.
>>
>>> It is not just iproute2 - since this is breaking ABI expectations.
>>> Looking at some app i wrote a while back based on analyzing kernel
>>> expectations at the time, I see the following logic:
>>>
>>> user can set master or self on command line.
>>> ...
>>> ....
>>> if (user DID NOT set master_on || user set self on)
>>>     then set self to on
>>>
>>> iow, current behavior:
>>>   01: master is only set if user explicitly asked.
>>>   11: master|self when user explicitly sets both
>>>   10: self is on by default when the user doesnt specify anything
>>>   00: and the last option is to have none set which is not
>>>       possible since we have defaults.
>>>
>>> cheers,
>>> jamal
>>>
>>>
>>> So this is very similar to iproute2 - if nothing is set
>>> it defaults to self.
>>
>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>> the default.
>
>
> Oh...i did not realize this was the case either. Thats unfortunate.
>
>>
>> For "bridge link" and "bridge vlan", the default is master.  The user
>> must explicitly specify "self" to act on the device side of the port.
>>
>> It's unfortunate the iproute2 defaults aren't consistent between
>> commands.  Maybe someone knows the history here and can explain.
>>
>>
>
> scott, this brings back the discussion you and i had over the revert of my
> patches.. (commit id's at the end of this email)...
> which used to seamlessly offload to switchdev from bridge driver if the port
> was a switch port (similar to stp state offload).

Your patch tried to do the same thing that the bridge's
ndo_bridge_setlink/dellink is doing which is using the handler for
MASTER to also set SELF stuff, when SELF was not specified.  I don't
feel we should be overriding the application defaults in the kernel;
instead, we should change the application if we want different
behavior.  The kernel should treat the two sides of the port
independent (that's the basic algo in rtnetlink.c handlers for
MASTER/SELF things).  When you start doing kernel SELF things in the
MASTER path, the application has lost the ability to address each side
of the port independently.

> 'self' used to exist before switchdev infra came in. My suggestion was to
> use it where required...but not build the switchdev api on the presence of
> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
> layers.

I don't understand why you're bringing up fib/neigh because there is
no master|self form for those.

The master|self objects are bridge fdb, settings, and vlans.  To be
clear, they are PF_BRIDGE handlers for:

PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
PF_BRIDGE:RTM_DELNEIGH: del fdb entry
PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
PF_BRIDGE:RTM_DELLINK: del VLAN

The net/core/rtnetlink.c code for these _is_ consistent right now.
They all perform this same basic algorithm:

    handler()
        if (!flags || flags & MASTER)
            if (master && master->op->foo)
                master->op->foo();
        if (flags & SELF)
            if (port->op->foo)
                port->op->foo();

This lets the application set MASTER and/or SELF to address one or
both sides of the port.  The kernel only provides the mechanism; the
application decides which sides to address.

Where we got into trouble is in the case of
PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
calls into the member port's ndo_vlan_rx_add_vid(), which is really a
SELF operation because it's setting the VLAN for the device-side of
the port.  Setting the VLAN on the device side should have only been
done if SELF was specified.  Too bad it didn't work out this way; now
we have to live with that legacy.  Your setlink/dellink patches tried
to do the same thing, which is to set the SELF side of the port
settings when SELF was not specified.  If those patches went in, we'd
be stuck there as well.  Your change said if application sets MASTER
(or nothing), the kernel treats it as MASTER|SELF.

So let's not hard-code defaults into the kernel.  Let's push those
back to the app.  If we want the command:

    bridge link set dev DEV learning on

to hit MASTER and SELF by default, then we update iproute2.  If we
want the command:

   bridge vlan add vid VID dev DEV

to hit MASTER and SELF by default, then we update iproute2.  If we
want the command:

    bridge fdb add ADR dev DEV

to hit MASTER and SELF by default, then we update iproute2.

-scott

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Jamal Hadi Salim]

On 06/02/15 10:30, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 06/02/15 03:10, Scott Feldman wrote:
>>

>> Question to ask when looking at something of this nature:
>> Will it work with no suprises if you used today's unmodified app?
>> The default behavior shouldnt change and unfortunately it does here.
>
> The default behavior does change, yes, but there shouldn't be any
> surprises even if using today's unmodified app.

[..]

[....]

>
> Ha, you're giving the behavior for "bridge fdb" command, where self is
> the default.
>

Yes, sorry ;->

> For "bridge link" and "bridge vlan", the default is master.  The user
> must explicitly specify "self" to act on the device side of the port.
>


Not sure what "device side of the port" is intended to mean. But:
iproute2, user can specify device is either the bridge or bridge port.
I think that is the key.
This is used in combination with the self/master flags to decide
behavior in the kernel.
Summary, assuming flag bits master:self

user setting: 00 (none set - which is default iproute2 behavior).
Kernel behavior:
if (bridge port targeted)
         sets the vlan bitmap on the bridge port.
else
         sets the vlan bitmap on the bridge.

******* Above is what we want to maintain unchanged.
If you are saying it doesnt change, then we are fine.

user setting: 01 (self on)
kernel behavior: no difference from default

user setting: 1x (master on, self doesnt matter)
kernel behavior:
if (bridge port targeted)
         sets the bitmap on the bridge port.
         sets the bitmap on the bridge as well. <--------
else
         sets the bitmap on the bridge.


BTW: given the vlan change are reflected from the bowels of
br_vlan_info() - is it redundant there is a call in br_afset
afterwards which says something like
"if master is set and target is bridge port then call hardware
setting thing"?

dont have much time - so i may be confusing something.

> It's unfortunate the iproute2 defaults aren't consistent between
> commands.  Maybe someone knows the history here and can explain.
>

Not sure. Too many cooks with specific use cases? There are many
thing in bridge that i wish were different.
Unfortunately when things get to this level Dave's famous "a horse has
left the barn"  principle applies. Despite my whining, over time,
even shit doesnt smell anymore. I almost feel we need an
ABI police force (refer to Jiri's talk at netconf).
It is much easier to fix kernel changes.

cheers,
jamal

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Toshiaki Makita]

On 15/06/03 (水) 4:01, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>>
>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>
>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>
>>>>
>>>>> Actually, we're now consistent with bridge man page which says master
>>>>> is the default.
>>>>>
>>>>> Want we want, I believe, is to adjust what the man page says (and the
>>>>> bridge vlan command itself), by making the default master and self.
>>>>> The kernel and driver are fine, it's the default in the bridge command
>>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>>> with software-only bridge.
>>>>>
>>>> Question to ask when looking at something of this nature:
>>>> Will it work with no suprises if you used today's unmodified app?
>>>> The default behavior shouldnt change and unfortunately it does here.
>>>
>>> The default behavior does change, yes, but there shouldn't be any
>>> surprises even if using today's unmodified app.  The reason why is no
>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>>> master to master|self, the bridge's
>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>
>>> So it comes down to two choices:
>>>
>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>> preserve (iproute2) command transparency, or
>>>
>>> 2) embrace existing behavior which is consistent with man pages but
>>> breaks command transparency for any driver implementing
>>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>>> can see the DSA going down this path also based on another concurrent
>>> thread.
>>>
>>> We're at option 2) right now.
>>>
>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>> expectations at the time, I see the following logic:
>>>>
>>>> user can set master or self on command line.
>>>> ...
>>>> ....
>>>> if (user DID NOT set master_on || user set self on)
>>>>      then set self to on
>>>>
>>>> iow, current behavior:
>>>>    01: master is only set if user explicitly asked.
>>>>    11: master|self when user explicitly sets both
>>>>    10: self is on by default when the user doesnt specify anything
>>>>    00: and the last option is to have none set which is not
>>>>        possible since we have defaults.
>>>>
>>>> cheers,
>>>> jamal
>>>>
>>>>
>>>> So this is very similar to iproute2 - if nothing is set
>>>> it defaults to self.
>>>
>>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>>> the default.
>>
>>
>> Oh...i did not realize this was the case either. Thats unfortunate.
>>
>>>
>>> For "bridge link" and "bridge vlan", the default is master.  The user
>>> must explicitly specify "self" to act on the device side of the port.
>>>
>>> It's unfortunate the iproute2 defaults aren't consistent between
>>> commands.  Maybe someone knows the history here and can explain.
>>>
>>>
>>
>> scott, this brings back the discussion you and i had over the revert of my
>> patches.. (commit id's at the end of this email)...
>> which used to seamlessly offload to switchdev from bridge driver if the port
>> was a switch port (similar to stp state offload).
>
> Your patch tried to do the same thing that the bridge's
> ndo_bridge_setlink/dellink is doing which is using the handler for
> MASTER to also set SELF stuff, when SELF was not specified.  I don't
> feel we should be overriding the application defaults in the kernel;
> instead, we should change the application if we want different
> behavior.  The kernel should treat the two sides of the port
> independent (that's the basic algo in rtnetlink.c handlers for
> MASTER/SELF things).  When you start doing kernel SELF things in the
> MASTER path, the application has lost the ability to address each side
> of the port independently.
>
>> 'self' used to exist before switchdev infra came in. My suggestion was to
>> use it where required...but not build the switchdev api on the presence of
>> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
>> layers.
>
> I don't understand why you're bringing up fib/neigh because there is
> no master|self form for those.
>
> The master|self objects are bridge fdb, settings, and vlans.  To be
> clear, they are PF_BRIDGE handlers for:
>
> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
> PF_BRIDGE:RTM_DELLINK: del VLAN
>
> The net/core/rtnetlink.c code for these _is_ consistent right now.
> They all perform this same basic algorithm:
>
>      handler()
>          if (!flags || flags & MASTER)
>              if (master && master->op->foo)
>                  master->op->foo();
>          if (flags & SELF)
>              if (port->op->foo)
>                  port->op->foo();
>
> This lets the application set MASTER and/or SELF to address one or
> both sides of the port.  The kernel only provides the mechanism; the
> application decides which sides to address.
>
> Where we got into trouble is in the case of
> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
> SELF operation because it's setting the VLAN for the device-side of
> the port.  Setting the VLAN on the device side should have only been
> done if SELF was specified.

Bridge's vlan_filtering is handled in master->op->foo(), not in 
port->op->foo().
Can't we introduce another switchdev handler that performs MASTER 
operation instead of calling SELF operation?

br_afspec()
  nbp_vlan_add()
   netdev_switch_port_vlan_add()
    rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation

I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does 
what should be done in MASTER operation.

Toshiaki Makita

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[roopa]

On 6/2/15, 12:01 PM, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> wrote:
>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>
>>>>
>>>>> Actually, we're now consistent with bridge man page which says master
>>>>> is the default.
>>>>>
>>>>> Want we want, I believe, is to adjust what the man page says (and the
>>>>> bridge vlan command itself), by making the default master and self.
>>>>> The kernel and driver are fine, it's the default in the bridge command
>>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>>> with software-only bridge.
>>>>>
>>>> Question to ask when looking at something of this nature:
>>>> Will it work with no suprises if you used today's unmodified app?
>>>> The default behavior shouldnt change and unfortunately it does here.
>>> The default behavior does change, yes, but there shouldn't be any
>>> surprises even if using today's unmodified app.  The reason why is no
>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>>> master to master|self, the bridge's
>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>
>>> So it comes down to two choices:
>>>
>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>> preserve (iproute2) command transparency, or
>>>
>>> 2) embrace existing behavior which is consistent with man pages but
>>> breaks command transparency for any driver implementing
>>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>>> can see the DSA going down this path also based on another concurrent
>>> thread.
>>>
>>> We're at option 2) right now.
>>>
>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>> expectations at the time, I see the following logic:
>>>>
>>>> user can set master or self on command line.
>>>> ...
>>>> ....
>>>> if (user DID NOT set master_on || user set self on)
>>>>      then set self to on
>>>>
>>>> iow, current behavior:
>>>>    01: master is only set if user explicitly asked.
>>>>    11: master|self when user explicitly sets both
>>>>    10: self is on by default when the user doesnt specify anything
>>>>    00: and the last option is to have none set which is not
>>>>        possible since we have defaults.
>>>>
>>>> cheers,
>>>> jamal
>>>>
>>>>
>>>> So this is very similar to iproute2 - if nothing is set
>>>> it defaults to self.
>>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>>> the default.
>>
>> Oh...i did not realize this was the case either. Thats unfortunate.
>>
>>> For "bridge link" and "bridge vlan", the default is master.  The user
>>> must explicitly specify "self" to act on the device side of the port.
>>>
>>> It's unfortunate the iproute2 defaults aren't consistent between
>>> commands.  Maybe someone knows the history here and can explain.
>>>
>>>
>> scott, this brings back the discussion you and i had over the revert of my
>> patches.. (commit id's at the end of this email)...
>> which used to seamlessly offload to switchdev from bridge driver if the port
>> was a switch port (similar to stp state offload).
> Your patch tried to do the same thing that the bridge's
> ndo_bridge_setlink/dellink is doing which is using the handler for
> MASTER to also set SELF stuff, when SELF was not specified.  I don't
> feel we should be overriding the application defaults in the kernel;
> instead, we should change the application if we want different
> behavior.  The kernel should treat the two sides of the port
> independent (that's the basic algo in rtnetlink.c handlers for
> MASTER/SELF things).  When you start doing kernel SELF things in the
> MASTER path, the application has lost the ability to address each side
> of the port independently.
>
>> 'self' used to exist before switchdev infra came in. My suggestion was to
>> use it where required...but not build the switchdev api on the presence of
>> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
>> layers.
> I don't understand why you're bringing up fib/neigh because there is
> no master|self form for those.
>
> The master|self objects are bridge fdb, settings, and vlans.  To be
> clear, they are PF_BRIDGE handlers for:
>
> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
> PF_BRIDGE:RTM_DELLINK: del VLAN
>
> The net/core/rtnetlink.c code for these _is_ consistent right now.
> They all perform this same basic algorithm:
>
>      handler()
>          if (!flags || flags & MASTER)
>              if (master && master->op->foo)
>                  master->op->foo();
>          if (flags & SELF)
>              if (port->op->foo)
>                  port->op->foo();
>
> This lets the application set MASTER and/or SELF to address one or
> both sides of the port.  The kernel only provides the mechanism; the
> application decides which sides to address.
>
> Where we got into trouble is in the case of
> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
> SELF operation because it's setting the VLAN for the device-side of
> the port.  Setting the VLAN on the device side should have only been
> done if SELF was specified.  Too bad it didn't work out this way; now
> we have to live with that legacy.  Your setlink/dellink patches tried
> to do the same thing, which is to set the SELF side of the port
> settings when SELF was not specified.  If those patches went in, we'd
> be stuck there as well.  Your change said if application sets MASTER
> (or nothing), the kernel treats it as MASTER|SELF.
I understand what you are saying.
But, the way i see it is, master/self was the old way of doing l2 offload.
It completely bypassed the bridge driver. It existed before switchdev.
And I was hoping we don't carry over that notion when we are doing 
offloads the new way.
ie involve the bridge driver. Bridge driver owns the vlan config/stp 
states etc and pushes it to hardware.
We handle rollbacks, error policies from withing the bridge driver.
This is the switchdev offload model. We started with this.

And, we did not want to break existing drivers/or apps and also have the 
option of doing offloads the old way or allow going directly to the 
switchdev driver for some attributes. So, we allowed that (and my 
patches also made sure those were not broken).

But somewhere along the way...., with the past few commits...we are 
designing switchdev offload apis around 'self|master' ie the old way of 
doing things. That is my only concern.

Also, it seems a bit inconsistent...
- stp states are offloaded from within the bridge driver but not vlans 
and other port config
- I bring up fib all the time because, the switchdev offload api design 
should be consistent with the offload api across
rest of the subsystems

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[roopa]

On 6/3/15, 8:43 AM, Toshiaki Makita wrote:
> On 15/06/03 (水) 4:01, Scott Feldman wrote:
>> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> wrote:
>>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>>>
>>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> 
>>>> wrote:
>>>>>
>>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>>
>>>>>
>>>>>> Actually, we're now consistent with bridge man page which says 
>>>>>> master
>>>>>> is the default.
>>>>>>
>>>>>> Want we want, I believe, is to adjust what the man page says (and 
>>>>>> the
>>>>>> bridge vlan command itself), by making the default master and self.
>>>>>> The kernel and driver are fine, it's the default in the bridge 
>>>>>> command
>>>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>>>> with software-only bridge.
>>>>>>
>>>>> Question to ask when looking at something of this nature:
>>>>> Will it work with no suprises if you used today's unmodified app?
>>>>> The default behavior shouldnt change and unfortunately it does here.
>>>>
>>>> The default behavior does change, yes, but there shouldn't be any
>>>> surprises even if using today's unmodified app.  The reason why is no
>>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>>>> master to master|self, the bridge's
>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>>
>>>> So it comes down to two choices:
>>>>
>>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>>> preserve (iproute2) command transparency, or
>>>>
>>>> 2) embrace existing behavior which is consistent with man pages but
>>>> breaks command transparency for any driver implementing
>>>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>>>> can see the DSA going down this path also based on another concurrent
>>>> thread.
>>>>
>>>> We're at option 2) right now.
>>>>
>>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>>> expectations at the time, I see the following logic:
>>>>>
>>>>> user can set master or self on command line.
>>>>> ...
>>>>> ....
>>>>> if (user DID NOT set master_on || user set self on)
>>>>>      then set self to on
>>>>>
>>>>> iow, current behavior:
>>>>>    01: master is only set if user explicitly asked.
>>>>>    11: master|self when user explicitly sets both
>>>>>    10: self is on by default when the user doesnt specify anything
>>>>>    00: and the last option is to have none set which is not
>>>>>        possible since we have defaults.
>>>>>
>>>>> cheers,
>>>>> jamal
>>>>>
>>>>>
>>>>> So this is very similar to iproute2 - if nothing is set
>>>>> it defaults to self.
>>>>
>>>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>>>> the default.
>>>
>>>
>>> Oh...i did not realize this was the case either. Thats unfortunate.
>>>
>>>>
>>>> For "bridge link" and "bridge vlan", the default is master. The user
>>>> must explicitly specify "self" to act on the device side of the port.
>>>>
>>>> It's unfortunate the iproute2 defaults aren't consistent between
>>>> commands.  Maybe someone knows the history here and can explain.
>>>>
>>>>
>>>
>>> scott, this brings back the discussion you and i had over the revert 
>>> of my
>>> patches.. (commit id's at the end of this email)...
>>> which used to seamlessly offload to switchdev from bridge driver if 
>>> the port
>>> was a switch port (similar to stp state offload).
>>
>> Your patch tried to do the same thing that the bridge's
>> ndo_bridge_setlink/dellink is doing which is using the handler for
>> MASTER to also set SELF stuff, when SELF was not specified.  I don't
>> feel we should be overriding the application defaults in the kernel;
>> instead, we should change the application if we want different
>> behavior.  The kernel should treat the two sides of the port
>> independent (that's the basic algo in rtnetlink.c handlers for
>> MASTER/SELF things).  When you start doing kernel SELF things in the
>> MASTER path, the application has lost the ability to address each side
>> of the port independently.
>>
>>> 'self' used to exist before switchdev infra came in. My suggestion 
>>> was to
>>> use it where required...but not build the switchdev api on the 
>>> presence of
>>> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
>>> layers.
>>
>> I don't understand why you're bringing up fib/neigh because there is
>> no master|self form for those.
>>
>> The master|self objects are bridge fdb, settings, and vlans.  To be
>> clear, they are PF_BRIDGE handlers for:
>>
>> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
>> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
>> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
>> PF_BRIDGE:RTM_DELLINK: del VLAN
>>
>> The net/core/rtnetlink.c code for these _is_ consistent right now.
>> They all perform this same basic algorithm:
>>
>>      handler()
>>          if (!flags || flags & MASTER)
>>              if (master && master->op->foo)
>>                  master->op->foo();
>>          if (flags & SELF)
>>              if (port->op->foo)
>>                  port->op->foo();
>>
>> This lets the application set MASTER and/or SELF to address one or
>> both sides of the port.  The kernel only provides the mechanism; the
>> application decides which sides to address.
>>
>> Where we got into trouble is in the case of
>> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
>> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
>> SELF operation because it's setting the VLAN for the device-side of
>> the port.  Setting the VLAN on the device side should have only been
>> done if SELF was specified.
>
> Bridge's vlan_filtering is handled in master->op->foo(), not in 
> port->op->foo().
> Can't we introduce another switchdev handler that performs MASTER 
> operation instead of calling SELF operation?
>
> br_afspec()
>  nbp_vlan_add()
>   netdev_switch_port_vlan_add()
>    rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>
> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does 
> what should be done in MASTER operation.
yes, this is what i have been alluding to (and I had commits which did 
this but got reverted).

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman]

On Thu, 4 Jun 2015, Toshiaki Makita wrote:

> Bridge's vlan_filtering is handled in master->op->foo(), not in 
> port->op->foo().
> Can't we introduce another switchdev handler that performs MASTER operation 
> instead of calling SELF operation?
>
> br_afspec()
> nbp_vlan_add()
>  netdev_switch_port_vlan_add()
>   rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>
> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does what 
> should be done in MASTER operation.

Something like this:

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 13013fe..df57ede 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -2,6 +2,7 @@
  #include <linux/netdevice.h>
  #include <linux/rtnetlink.h>
  #include <linux/slab.h>
+#include <net/switchdev.h>

  #include "br_private.h"

@@ -36,6 +37,36 @@ static void __vlan_add_flags(struct net_port_vlans *v, u16 vid, u16 flags)
  		clear_bit(vid, v->untagged_bitmap);
  }

+
+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 vlan_obj = {
+		.id = SWITCHDEV_OBJ_PORT_VLAN,
+		.u.vlan = {
+			.flags = flags,
+			.vid_start = 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.
+	 */
+
+	if (ops->ndo_vlan_rx_add_vid) {
+		err = vlan_vid_add(dev, br->vlan_proto, vid);
+	} else {
+		err = switchdev_port_obj_add(dev, &vlan_obj);
+		if (err == -EOPNOTSUPP)
+			err = 0;
+	}
+
+	return err;
+}
+
  static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
  {
  	struct net_bridge_port *p = NULL;
@@ -62,7 +93,7 @@ static int __vlan_add(struct net_port_vlans *v, u16 vid, u16 flags)
  		 * This ensures tagged traffic enters the bridge when
  		 * promiscuous mode is disabled by br_manage_promisc().
  		 */
-		err = vlan_vid_add(dev, br->vlan_proto, vid);
+		err = __vlan_vid_add(dev, br, vid, flags);
  		if (err)
  			return err;
  	}
@@ -86,6 +117,28 @@ out_filt:
  	return err;
  }

+static void __vlan_vid_del(struct net_device *dev, struct net_bridge *br,
+			  u16 vid)
+{
+	const struct net_device_ops *ops = dev->netdev_ops;
+	struct switchdev_obj vlan_obj = {
+		.id = SWITCHDEV_OBJ_PORT_VLAN,
+		.u.vlan = {
+			.vid_start = vid,
+			.vid_end = vid,
+		},
+	};
+
+	/* If driver uses VLAN ndo ops, use 8021q to delete vid
+	 * on device, otherwise try switchdev ops to delete vid.
+	 */
+
+	if (ops->ndo_vlan_rx_kill_vid)
+		vlan_vid_del(dev, br->vlan_proto, vid);
+	else
+		switchdev_port_obj_del(dev, &vlan_obj);
+}
+
  static int __vlan_del(struct net_port_vlans *v, u16 vid)
  {
  	if (!test_bit(vid, v->vlan_bitmap))
@@ -96,7 +149,7 @@ static int __vlan_del(struct net_port_vlans *v, u16 vid)

  	if (v->port_idx) {
  		struct net_bridge_port *p = v->parent.port;
-		vlan_vid_del(p->dev, p->br->vlan_proto, vid);
+		__vlan_vid_del(p->dev, p->br, vid);
  	}

  	clear_bit(vid, v->vlan_bitmap);

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Toshiaki Makita]

On 15/06/04 (木) 15:05, Scott Feldman wrote:
> On Thu, 4 Jun 2015, Toshiaki Makita wrote:
>
>> Bridge's vlan_filtering is handled in master->op->foo(), not in
>> port->op->foo().
>> Can't we introduce another switchdev handler that performs MASTER
>> operation instead of calling SELF operation?
>>
>> br_afspec()
>> nbp_vlan_add()
>>  netdev_switch_port_vlan_add()
>>   rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>>
>> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does
>> what should be done in MASTER operation.
>
> Something like this:

This looks very close to what I'm suggesting, although I'm not sure if
we need switchdev_port_obj_add() to be exclusive with vlan_vid_add().
For rocker, keeping NETIF_F_HW_VLAN_[CTAG/STAG]_FILTER dropped makes
vlan_vid_add() essentially noop.

Toshiaki Makita

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Toshiaki Makita]

On 15/06/04 (木) 3:41, roopa wrote:
> On 6/3/15, 8:43 AM, Toshiaki Makita wrote:
>> On 15/06/03 (水) 4:01, Scott Feldman wrote:
>>> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> wrote:
>>>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>>>>
>>>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>>>>> wrote:
>>>>>>
>>>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>>>
>>>>>>
>>>>>>> Actually, we're now consistent with bridge man page which says
>>>>>>> master
>>>>>>> is the default.
>>>>>>>
>>>>>>> Want we want, I believe, is to adjust what the man page says (and
>>>>>>> the
>>>>>>> bridge vlan command itself), by making the default master and self.
>>>>>>> The kernel and driver are fine, it's the default in the bridge
>>>>>>> command
>>>>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>>>>> with software-only bridge.
>>>>>>>
>>>>>> Question to ask when looking at something of this nature:
>>>>>> Will it work with no suprises if you used today's unmodified app?
>>>>>> The default behavior shouldnt change and unfortunately it does here.
>>>>>
>>>>> The default behavior does change, yes, but there shouldn't be any
>>>>> surprises even if using today's unmodified app.  The reason why is no
>>>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>>>>> master to master|self, the bridge's
>>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>>>
>>>>> So it comes down to two choices:
>>>>>
>>>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>>>> preserve (iproute2) command transparency, or
>>>>>
>>>>> 2) embrace existing behavior which is consistent with man pages but
>>>>> breaks command transparency for any driver implementing
>>>>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>>>>> can see the DSA going down this path also based on another concurrent
>>>>> thread.
>>>>>
>>>>> We're at option 2) right now.
>>>>>
>>>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>>>> expectations at the time, I see the following logic:
>>>>>>
>>>>>> user can set master or self on command line.
>>>>>> ...
>>>>>> ....
>>>>>> if (user DID NOT set master_on || user set self on)
>>>>>>      then set self to on
>>>>>>
>>>>>> iow, current behavior:
>>>>>>    01: master is only set if user explicitly asked.
>>>>>>    11: master|self when user explicitly sets both
>>>>>>    10: self is on by default when the user doesnt specify anything
>>>>>>    00: and the last option is to have none set which is not
>>>>>>        possible since we have defaults.
>>>>>>
>>>>>> cheers,
>>>>>> jamal
>>>>>>
>>>>>>
>>>>>> So this is very similar to iproute2 - if nothing is set
>>>>>> it defaults to self.
>>>>>
>>>>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>>>>> the default.
>>>>
>>>>
>>>> Oh...i did not realize this was the case either. Thats unfortunate.
>>>>
>>>>>
>>>>> For "bridge link" and "bridge vlan", the default is master. The user
>>>>> must explicitly specify "self" to act on the device side of the port.
>>>>>
>>>>> It's unfortunate the iproute2 defaults aren't consistent between
>>>>> commands.  Maybe someone knows the history here and can explain.
>>>>>
>>>>>
>>>>
>>>> scott, this brings back the discussion you and i had over the revert
>>>> of my
>>>> patches.. (commit id's at the end of this email)...
>>>> which used to seamlessly offload to switchdev from bridge driver if
>>>> the port
>>>> was a switch port (similar to stp state offload).
>>>
>>> Your patch tried to do the same thing that the bridge's
>>> ndo_bridge_setlink/dellink is doing which is using the handler for
>>> MASTER to also set SELF stuff, when SELF was not specified.  I don't
>>> feel we should be overriding the application defaults in the kernel;
>>> instead, we should change the application if we want different
>>> behavior.  The kernel should treat the two sides of the port
>>> independent (that's the basic algo in rtnetlink.c handlers for
>>> MASTER/SELF things).  When you start doing kernel SELF things in the
>>> MASTER path, the application has lost the ability to address each side
>>> of the port independently.
>>>
>>>> 'self' used to exist before switchdev infra came in. My suggestion
>>>> was to
>>>> use it where required...but not build the switchdev api on the
>>>> presence of
>>>> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
>>>> layers.
>>>
>>> I don't understand why you're bringing up fib/neigh because there is
>>> no master|self form for those.
>>>
>>> The master|self objects are bridge fdb, settings, and vlans.  To be
>>> clear, they are PF_BRIDGE handlers for:
>>>
>>> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
>>> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
>>> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
>>> PF_BRIDGE:RTM_DELLINK: del VLAN
>>>
>>> The net/core/rtnetlink.c code for these _is_ consistent right now.
>>> They all perform this same basic algorithm:
>>>
>>>      handler()
>>>          if (!flags || flags & MASTER)
>>>              if (master && master->op->foo)
>>>                  master->op->foo();
>>>          if (flags & SELF)
>>>              if (port->op->foo)
>>>                  port->op->foo();
>>>
>>> This lets the application set MASTER and/or SELF to address one or
>>> both sides of the port.  The kernel only provides the mechanism; the
>>> application decides which sides to address.
>>>
>>> Where we got into trouble is in the case of
>>> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
>>> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
>>> SELF operation because it's setting the VLAN for the device-side of
>>> the port.  Setting the VLAN on the device side should have only been
>>> done if SELF was specified.
>>
>> Bridge's vlan_filtering is handled in master->op->foo(), not in
>> port->op->foo().
>> Can't we introduce another switchdev handler that performs MASTER
>> operation instead of calling SELF operation?
>>
>> br_afspec()
>>  nbp_vlan_add()
>>   netdev_switch_port_vlan_add()
>>    rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>>
>> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does
>> what should be done in MASTER operation.
> yes, this is what i have been alluding to (and I had commits which did
> this but got reverted).

If SELF operation support is needed, then my suggetion is maybe the same 
as yours.

I mean,

(a). bridge vlan add vid VID dev DEV
(b). bridge vlan add vid VID dev DEV self

(a) should work anyway, IMHO.
If only (b) works and (a) does not work, then it does not look 
transparent (this is current behavior).
Maybe it's possible both (a) and (b) work in the same way for 
switchdev... (I presume this is your proposal).

But actually I would be a bit suprised if (b) works in the same way as 
(a), since the only usage of "bridge vlan self" used to be for bridge 
device itself.. i.e.,

   bridge vlan add vid VID dev br0 self

Toshiaki Makita

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[roopa]

On 6/4/15, 8:04 AM, Toshiaki Makita wrote:
> On 15/06/04 (木) 3:41, roopa wrote:
>> On 6/3/15, 8:43 AM, Toshiaki Makita wrote:
>>> On 15/06/03 (水) 4:01, Scott Feldman wrote:
>>>> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@cumulusnetworks.com> 
>>>> wrote:
>>>>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>>>>>
>>>>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>>>>
>>>>>>>
>>>>>>>> Actually, we're now consistent with bridge man page which says
>>>>>>>> master
>>>>>>>> is the default.
>>>>>>>>
>>>>>>>> Want we want, I believe, is to adjust what the man page says (and
>>>>>>>> the
>>>>>>>> bridge vlan command itself), by making the default master and 
>>>>>>>> self.
>>>>>>>> The kernel and driver are fine, it's the default in the bridge
>>>>>>>> command
>>>>>>>> that needs adjusting.  Once we do this, we'll be back to 
>>>>>>>> transparent
>>>>>>>> with software-only bridge.
>>>>>>>>
>>>>>>> Question to ask when looking at something of this nature:
>>>>>>> Will it work with no suprises if you used today's unmodified app?
>>>>>>> The default behavior shouldnt change and unfortunately it does 
>>>>>>> here.
>>>>>>
>>>>>> The default behavior does change, yes, but there shouldn't be any
>>>>>> surprises even if using today's unmodified app.  The reason why 
>>>>>> is no
>>>>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>>>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>>>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes 
>>>>>> from
>>>>>> master to master|self, the bridge's
>>>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>>>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>>>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>>>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>>>>
>>>>>> So it comes down to two choices:
>>>>>>
>>>>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>>>>> preserve (iproute2) command transparency, or
>>>>>>
>>>>>> 2) embrace existing behavior which is consistent with man pages but
>>>>>> breaks command transparency for any driver implementing
>>>>>> ndo_bridge_setlink for VLAN setup, which currently is just 
>>>>>> rocker.  I
>>>>>> can see the DSA going down this path also based on another 
>>>>>> concurrent
>>>>>> thread.
>>>>>>
>>>>>> We're at option 2) right now.
>>>>>>
>>>>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>>>>> expectations at the time, I see the following logic:
>>>>>>>
>>>>>>> user can set master or self on command line.
>>>>>>> ...
>>>>>>> ....
>>>>>>> if (user DID NOT set master_on || user set self on)
>>>>>>>      then set self to on
>>>>>>>
>>>>>>> iow, current behavior:
>>>>>>>    01: master is only set if user explicitly asked.
>>>>>>>    11: master|self when user explicitly sets both
>>>>>>>    10: self is on by default when the user doesnt specify anything
>>>>>>>    00: and the last option is to have none set which is not
>>>>>>>        possible since we have defaults.
>>>>>>>
>>>>>>> cheers,
>>>>>>> jamal
>>>>>>>
>>>>>>>
>>>>>>> So this is very similar to iproute2 - if nothing is set
>>>>>>> it defaults to self.
>>>>>>
>>>>>> Ha, you're giving the behavior for "bridge fdb" command, where 
>>>>>> self is
>>>>>> the default.
>>>>>
>>>>>
>>>>> Oh...i did not realize this was the case either. Thats unfortunate.
>>>>>
>>>>>>
>>>>>> For "bridge link" and "bridge vlan", the default is master. The user
>>>>>> must explicitly specify "self" to act on the device side of the 
>>>>>> port.
>>>>>>
>>>>>> It's unfortunate the iproute2 defaults aren't consistent between
>>>>>> commands.  Maybe someone knows the history here and can explain.
>>>>>>
>>>>>>
>>>>>
>>>>> scott, this brings back the discussion you and i had over the revert
>>>>> of my
>>>>> patches.. (commit id's at the end of this email)...
>>>>> which used to seamlessly offload to switchdev from bridge driver if
>>>>> the port
>>>>> was a switch port (similar to stp state offload).
>>>>
>>>> Your patch tried to do the same thing that the bridge's
>>>> ndo_bridge_setlink/dellink is doing which is using the handler for
>>>> MASTER to also set SELF stuff, when SELF was not specified. I don't
>>>> feel we should be overriding the application defaults in the kernel;
>>>> instead, we should change the application if we want different
>>>> behavior.  The kernel should treat the two sides of the port
>>>> independent (that's the basic algo in rtnetlink.c handlers for
>>>> MASTER/SELF things).  When you start doing kernel SELF things in the
>>>> MASTER path, the application has lost the ability to address each side
>>>> of the port independently.
>>>>
>>>>> 'self' used to exist before switchdev infra came in. My suggestion
>>>>> was to
>>>>> use it where required...but not build the switchdev api on the
>>>>> presence of
>>>>> 'self'. switchdev layer should be consistent across...all 
>>>>> fib/fdb/neigh
>>>>> layers.
>>>>
>>>> I don't understand why you're bringing up fib/neigh because there is
>>>> no master|self form for those.
>>>>
>>>> The master|self objects are bridge fdb, settings, and vlans.  To be
>>>> clear, they are PF_BRIDGE handlers for:
>>>>
>>>> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
>>>> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
>>>> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
>>>> PF_BRIDGE:RTM_DELLINK: del VLAN
>>>>
>>>> The net/core/rtnetlink.c code for these _is_ consistent right now.
>>>> They all perform this same basic algorithm:
>>>>
>>>>      handler()
>>>>          if (!flags || flags & MASTER)
>>>>              if (master && master->op->foo)
>>>>                  master->op->foo();
>>>>          if (flags & SELF)
>>>>              if (port->op->foo)
>>>>                  port->op->foo();
>>>>
>>>> This lets the application set MASTER and/or SELF to address one or
>>>> both sides of the port.  The kernel only provides the mechanism; the
>>>> application decides which sides to address.
>>>>
>>>> Where we got into trouble is in the case of
>>>> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
>>>> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
>>>> SELF operation because it's setting the VLAN for the device-side of
>>>> the port.  Setting the VLAN on the device side should have only been
>>>> done if SELF was specified.
>>>
>>> Bridge's vlan_filtering is handled in master->op->foo(), not in
>>> port->op->foo().
>>> Can't we introduce another switchdev handler that performs MASTER
>>> operation instead of calling SELF operation?
>>>
>>> br_afspec()
>>>  nbp_vlan_add()
>>>   netdev_switch_port_vlan_add()
>>>    rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>>>
>>> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does
>>> what should be done in MASTER operation.
>> yes, this is what i have been alluding to (and I had commits which did
>> this but got reverted).
>
> If SELF operation support is needed, then my suggetion is maybe the 
> same as yours.
>
> I mean,
>
> (a). bridge vlan add vid VID dev DEV
> (b). bridge vlan add vid VID dev DEV self
>
> (a) should work anyway, IMHO.
> If only (b) works and (a) does not work, then it does not look 
> transparent (this is current behavior).
> Maybe it's possible both (a) and (b) work in the same way for 
> switchdev... (I presume this is your proposal).
yes, correct. Exactly what i was saying.

>
> But actually I would be a bit suprised if (b) works in the same way as 
> (a), since the only usage of "bridge vlan self" used to be for bridge 
> device itself.. i.e.,
correct again. We dont want only (b) to work the same way as (a). That 
will break existing semantics of self.

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Jamal Hadi Salim]

Full quote below. So what is the consensus on this topic?
I read the emails but i dont see a resolution.

cheers,
jamal

On 06/03/15 08:08, Jamal Hadi Salim wrote:
> On 06/02/15 10:30, Scott Feldman wrote:
>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>> wrote:
>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>
>
>>> Question to ask when looking at something of this nature:
>>> Will it work with no suprises if you used today's unmodified app?
>>> The default behavior shouldnt change and unfortunately it does here.
>>
>> The default behavior does change, yes, but there shouldn't be any
>> surprises even if using today's unmodified app.
>
> [..]
>
> [....]
>
>>
>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>> the default.
>>
>
> Yes, sorry ;->
>
>> For "bridge link" and "bridge vlan", the default is master.  The user
>> must explicitly specify "self" to act on the device side of the port.
>>
>
>
> Not sure what "device side of the port" is intended to mean. But:
> iproute2, user can specify device is either the bridge or bridge port.
> I think that is the key.
> This is used in combination with the self/master flags to decide
> behavior in the kernel.
> Summary, assuming flag bits master:self
>
> user setting: 00 (none set - which is default iproute2 behavior).
> Kernel behavior:
> if (bridge port targeted)
>          sets the vlan bitmap on the bridge port.
> else
>          sets the vlan bitmap on the bridge.
>
> ******* Above is what we want to maintain unchanged.
> If you are saying it doesnt change, then we are fine.
>
> user setting: 01 (self on)
> kernel behavior: no difference from default
>
> user setting: 1x (master on, self doesnt matter)
> kernel behavior:
> if (bridge port targeted)
>          sets the bitmap on the bridge port.
>          sets the bitmap on the bridge as well. <--------
> else
>          sets the bitmap on the bridge.
>
>
> BTW: given the vlan change are reflected from the bowels of
> br_vlan_info() - is it redundant there is a call in br_afset
> afterwards which says something like
> "if master is set and target is bridge port then call hardware
> setting thing"?
>
> dont have much time - so i may be confusing something.
>
>> It's unfortunate the iproute2 defaults aren't consistent between
>> commands.  Maybe someone knows the history here and can explain.
>>
>
> Not sure. Too many cooks with specific use cases? There are many
> thing in bridge that i wish were different.
> Unfortunately when things get to this level Dave's famous "a horse has
> left the barn"  principle applies. Despite my whining, over time,
> even shit doesnt smell anymore. I almost feel we need an
> ABI police force (refer to Jiri's talk at netconf).
> It is much easier to fix kernel changes.
>
> cheers,
> jamal

Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops

[Scott Feldman]

On Thu, Jun 11, 2015 at 6:00 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Full quote below. So what is the consensus on this topic?
> I read the emails but i dont see a resolution.

I think I dropped the ball on this one.  I'll go ahead and submit the
last patch I posted to this email thread to resume the conversation.
I think we're close; just need to make sure we have buy-in.