[Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Florian Fainelli]

Hi all,

This patch series allows using the bridge master interface to configure
an Ethernet switch port's CPU/management port with different VLAN attributes than
those of the bridge downstream ports/members.

Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
tested this with b53 and a mockup DSA driver.

Open questions:

- if we have more than one bridge on top of a physical switch, the driver
  should keep track of that and verify that we are not going to change
  the CPU port VLAN attributes in a way that results in incompatible settings
  to be applied

- if the default behavior is to have all VLANs associated with the CPU port
  be ingressing/egressing tagged to the CPU, is this really useful?

Florian Fainelli (3):
  net: bridge: Allow bridge master device to configure switch CPU port
  net: dsa: Propagate VLAN add/del to CPU port(s)
  net: dsa: b53: Remove CPU port specific VLAN programming

 drivers/net/dsa/b53/b53_common.c | 22 ++++++--------------
 net/bridge/br_vlan.c             | 28 ++++++++++++++++++++++---
 net/dsa/slave.c                  | 45 +++++++++++++++++++++++++++++-----------
 3 files changed, 64 insertions(+), 31 deletions(-)

-- 
2.9.3

[Bridge] [RFC net-next 1/3] net: bridge: Allow bridge master device to configure switch CPU port

[Florian Fainelli]

An use case which is currently not possible with Linux bridges on top of
network switches is to configure the CPU port of the switch (inherently
presented to the user with a bridge master device) independently from
its downstream ports, with a different set of VLAN properties. The
reason as to why is that the switch driver will never get any call to
switchdev_port_obj_{add,del} with the obj->orig_dev set to the bridge
master device.

This allows CPU/management ports to e.g: receive all traffic as tagged,
whereas the downstream port may have different untagged VLAN settings.

The following happens now (assuming bridge master device is already
created):

bridge vlan add vid 2 dev port0 pvid untagged
	-> port0 (e.g: switch port 0) gets programmed
	-> CPU port gets programmed
bridge vlan add vid 2 dev br0 self
	-> CPU port gets programmed
bridge vlan add vid 2 dev port0
	-> port0 (switch port 0) gets programmed

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/bridge/br_vlan.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index b6de4f457161..b335d66d21db 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -228,7 +228,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 		err = __vlan_vid_add(dev, br, v->vid, flags);
 		if (err)
 			goto out;
+	}
 
+	if (p) {
 		/* need to work on the master vlan too */
 		if (flags & BRIDGE_VLAN_INFO_MASTER) {
 			err = br_vlan_add(br, v->vid, flags |
@@ -242,6 +244,14 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 			goto out_filt;
 		v->brvlan = masterv;
 		v->stats = masterv->stats;
+
+		/* Propagate the VLAN flags changes down to the underlying
+		 * hardware, which may have to reconfigure the physical port
+		 * associated with the bridge (e.g: CPU/management port)
+		 */
+		err = __vlan_vid_add(br->dev, br, v->vid, flags);
+		if (err)
+			goto out_filt;
 	}
 
 	/* Add the dev mac and count the vlan only if it's usable */
@@ -287,19 +297,25 @@ static int __vlan_del(struct net_bridge_vlan *v)
 	struct net_bridge_vlan *masterv = v;
 	struct net_bridge_vlan_group *vg;
 	struct net_bridge_port *p = NULL;
+	struct net_device *dev;
+	struct net_bridge *br;
 	int err = 0;
 
 	if (br_vlan_is_master(v)) {
-		vg = br_vlan_group(v->br);
+		br = v->br;
+		vg = br_vlan_group(br);
+		dev = v->br->dev;
 	} else {
 		p = v->port;
+		br = p->br;
+		dev = p->dev;
 		vg = nbp_vlan_group(v->port);
 		masterv = v->brvlan;
 	}
 
 	__vlan_delete_pvid(vg, v->vid);
-	if (p) {
-		err = __vlan_vid_del(p->dev, p->br, v->vid);
+	if (p || br_vlan_is_master(v)) {
+		err = __vlan_vid_del(dev, br, v->vid);
 		if (err)
 			goto out;
 	}
@@ -568,6 +584,12 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 			vg->num_vlans++;
 		}
 		__vlan_add_flags(vlan, flags);
+
+		/* Propagate the VLAN flags changes down to the underlying
+		 * hardware, which may have to reconfigure the physical port
+		 * associated with the bridge (e.g: CPU/management port)
+		 */
+		__vlan_vid_add(br->dev, br, vlan->vid, flags);
 		return 0;
 	}
 
-- 
2.9.3

Re: [Bridge] [RFC net-next 1/3] net: bridge: Allow bridge master device to configure switch CPU port

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> bridge vlan add vid 2 dev br0 self
> 	-> CPU port gets programmed
> bridge vlan add vid 2 dev port0
> 	-> port0 (switch port 0) gets programmed

Although this is not specific to this patch, I'd like to point out that
this seems not to be the behavior bridge expects.

The bridge manpage says:

    bridge vlan add - add a new vlan filter entry
    ...

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

So if I'm not mistaken, the switch chip must be programmed only when the
bridge command is called with the "self" attribute. Without it, only
software configuration must be made, like what happens when the driver
returns -EOPNOTSUPP.

Currently, both commands below program the hardware:

    # bridge vlan add vid 2 dev port0 [master]
    # bridge vlan add vid 2 dev port0 [master] self

Jiri, what do you think? Is there a reason for switchdev not to be
consistent with the bridge doc, or should this be fixed?

Thanks,

        Vivien

Re: [Bridge] [RFC net-next 1/3] net: bridge: Allow bridge master device to configure switch CPU port

[Toshiaki Makita]

On 2016/11/23 0:46, Vivien Didelot wrote:
> Hi Florian,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> bridge vlan add vid 2 dev br0 self
>> 	-> CPU port gets programmed
>> bridge vlan add vid 2 dev port0
>> 	-> port0 (switch port 0) gets programmed
> 
> Although this is not specific to this patch, I'd like to point out that
> this seems not to be the behavior bridge expects.
> 
> The bridge manpage says:
> 
>     bridge vlan add - add a new vlan filter entry
>     ...
> 
>        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).
> 
> So if I'm not mistaken, the switch chip must be programmed only when the
> bridge command is called with the "self" attribute. Without it, only
> software configuration must be made, like what happens when the driver
> returns -EOPNOTSUPP.
> 
> Currently, both commands below program the hardware:
> 
>     # bridge vlan add vid 2 dev port0 [master]
>     # bridge vlan add vid 2 dev port0 [master] self

Actually this is intended behavior, which keeps backward compatibility.
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7f1095394918c7058ff81c96c3bab3a897e97a9d

Thanks,
Toshiaki Makita

[Bridge] [RFC net-next 2/3] net: dsa: Propagate VLAN add/del to CPU port(s)

[Florian Fainelli]

Now that the bridge layer can call into switchdev to signal programming
requests targeting the bridge master device itself, allow the switch
drivers to implement separate programming of downstream and
upstream/management ports.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d0c7bce88743..18288261b964 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -223,35 +223,30 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 	return 0;
 }
 
-static int dsa_slave_port_vlan_add(struct net_device *dev,
+static int dsa_slave_port_vlan_add(struct dsa_switch *ds, int port,
 				   const struct switchdev_obj_port_vlan *vlan,
 				   struct switchdev_trans *trans)
 {
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
 
 	if (switchdev_trans_ph_prepare(trans)) {
 		if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
 			return -EOPNOTSUPP;
 
-		return ds->ops->port_vlan_prepare(ds, p->port, vlan, trans);
+		return ds->ops->port_vlan_prepare(ds, port, vlan, trans);
 	}
 
-	ds->ops->port_vlan_add(ds, p->port, vlan, trans);
+	ds->ops->port_vlan_add(ds, port, vlan, trans);
 
 	return 0;
 }
 
-static int dsa_slave_port_vlan_del(struct net_device *dev,
+static int dsa_slave_port_vlan_del(struct dsa_switch *ds, int port,
 				   const struct switchdev_obj_port_vlan *vlan)
 {
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
-
 	if (!ds->ops->port_vlan_del)
 		return -EOPNOTSUPP;
 
-	return ds->ops->port_vlan_del(ds, p->port, vlan);
+	return ds->ops->port_vlan_del(ds, port, vlan);
 }
 
 static int dsa_slave_port_vlan_dump(struct net_device *dev,
@@ -465,8 +460,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 				  const struct switchdev_obj *obj,
 				  struct switchdev_trans *trans)
 {
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int port = p->port;
 	int err;
 
+	/* Here we may be called with an orig_dev which is different from dev,
+	 * on purpose, to receive request coming from e.g the bridge master
+	 * device. Although there are no network device associated with CPU/DSA
+	 * ports, we may still have programming operation for these ports.
+	 */
+	if (obj->orig_dev == p->bridge_dev) {
+		ds = ds->dst->ds[0];
+		port = ds->dst->cpu_port;
+	}
+
 	/* For the prepare phase, ensure the full set of changes is feasable in
 	 * one go in order to signal a failure properly. If an operation is not
 	 * supported, return -EOPNOTSUPP.
@@ -483,7 +491,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 					     trans);
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		err = dsa_slave_port_vlan_add(dev,
+		err = dsa_slave_port_vlan_add(ds, port,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj),
 					      trans);
 		break;
@@ -498,8 +506,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 static int dsa_slave_port_obj_del(struct net_device *dev,
 				  const struct switchdev_obj *obj)
 {
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int port = p->port;
 	int err;
 
+	/* Here we may be called with an orig_dev which is different from dev,
+	 * on purpose, to receive request coming from e.g the bridge master
+	 * device. Although there are no network device associated with CPU/DSA
+	 * ports, we may still have programming operation for these ports.
+	 */
+	if (obj->orig_dev == p->bridge_dev) {
+		ds = ds->dst->ds[0];
+		port = ds->dst->cpu_port;
+	}
+
 	switch (obj->id) {
 	case SWITCHDEV_OBJ_ID_PORT_FDB:
 		err = dsa_slave_port_fdb_del(dev,
@@ -509,7 +530,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev,
 		err = dsa_slave_port_mdb_del(dev, SWITCHDEV_OBJ_PORT_MDB(obj));
 		break;
 	case SWITCHDEV_OBJ_ID_PORT_VLAN:
-		err = dsa_slave_port_vlan_del(dev,
+		err = dsa_slave_port_vlan_del(ds, port,
 					      SWITCHDEV_OBJ_PORT_VLAN(obj));
 		break;
 	default:
-- 
2.9.3

Re: [Bridge] [RFC net-next 2/3] net: dsa: Propagate VLAN add/del to CPU port(s)

[Vivien Didelot]

Hi Florian,

Open question: will we need to do the same for FDB and MDB objects?

Florian Fainelli <f.fainelli@gmail.com> writes:

> Now that the bridge layer can call into switchdev to signal programming
> requests targeting the bridge master device itself, allow the switch
> drivers to implement separate programming of downstream and
> upstream/management ports.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/dsa/slave.c | 45 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index d0c7bce88743..18288261b964 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -223,35 +223,30 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
>  	return 0;
>  }
>  
> -static int dsa_slave_port_vlan_add(struct net_device *dev,
> +static int dsa_slave_port_vlan_add(struct dsa_switch *ds, int port,
>  				   const struct switchdev_obj_port_vlan *vlan,
>  				   struct switchdev_trans *trans)
>  {
> -	struct dsa_slave_priv *p = netdev_priv(dev);
> -	struct dsa_switch *ds = p->parent;
>  

Extra newline ^.

>  	if (switchdev_trans_ph_prepare(trans)) {
>  		if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
>  			return -EOPNOTSUPP;
>  
> -		return ds->ops->port_vlan_prepare(ds, p->port, vlan, trans);
> +		return ds->ops->port_vlan_prepare(ds, port, vlan, trans);
>  	}
>  
> -	ds->ops->port_vlan_add(ds, p->port, vlan, trans);
> +	ds->ops->port_vlan_add(ds, port, vlan, trans);
>  
>  	return 0;
>  }
>  
> -static int dsa_slave_port_vlan_del(struct net_device *dev,
> +static int dsa_slave_port_vlan_del(struct dsa_switch *ds, int port,
>  				   const struct switchdev_obj_port_vlan *vlan)
>  {
> -	struct dsa_slave_priv *p = netdev_priv(dev);
> -	struct dsa_switch *ds = p->parent;
> -
>  	if (!ds->ops->port_vlan_del)
>  		return -EOPNOTSUPP;
>  
> -	return ds->ops->port_vlan_del(ds, p->port, vlan);
> +	return ds->ops->port_vlan_del(ds, port, vlan);
>  }
>  
>  static int dsa_slave_port_vlan_dump(struct net_device *dev,
> @@ -465,8 +460,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>  				  const struct switchdev_obj *obj,
>  				  struct switchdev_trans *trans)
>  {
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	int port = p->port;
>  	int err;
>  
> +	/* Here we may be called with an orig_dev which is different from dev,
> +	 * on purpose, to receive request coming from e.g the bridge master
> +	 * device. Although there are no network device associated with CPU/DSA
> +	 * ports, we may still have programming operation for these ports.
> +	 */
> +	if (obj->orig_dev == p->bridge_dev) {
> +		ds = ds->dst->ds[0];
> +		port = ds->dst->cpu_port;
> +	}
> +
>  	/* For the prepare phase, ensure the full set of changes is feasable in
>  	 * one go in order to signal a failure properly. If an operation is not
>  	 * supported, return -EOPNOTSUPP.
> @@ -483,7 +491,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>  					     trans);
>  		break;
>  	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> -		err = dsa_slave_port_vlan_add(dev,
> +		err = dsa_slave_port_vlan_add(ds, port,
>  					      SWITCHDEV_OBJ_PORT_VLAN(obj),
>  					      trans);

Note that dsa_slave_port_vlan_add() will be called N times, N being the
number of bridge ports. This is not an issue for the moment though.
Programming it only once requires caching, so leave it for an eventual
future patch.

When issuing the following command (lan0 being a member of br0):

    # bridge vlan add vid 42 dev lan0

the CPU port is also programmed as tagged in VLAN 42. Is that expected?

Thanks,

        Vivien

Re: [Bridge] [RFC net-next 2/3] net: dsa: Propagate VLAN add/del to CPU port(s)

[Florian Fainelli]

On 11/22/2016 08:50 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> Open question: will we need to do the same for FDB and MDB objects?

(overlooked that question early this week), I do expect that this could
be helpful for FDB and MBD objects as well, yes.

> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> Now that the bridge layer can call into switchdev to signal programming
>> requests targeting the bridge master device itself, allow the switch
>> drivers to implement separate programming of downstream and
>> upstream/management ports.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  net/dsa/slave.c | 45 +++++++++++++++++++++++++++++++++------------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index d0c7bce88743..18288261b964 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -223,35 +223,30 @@ static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
>>  	return 0;
>>  }
>>  
>> -static int dsa_slave_port_vlan_add(struct net_device *dev,
>> +static int dsa_slave_port_vlan_add(struct dsa_switch *ds, int port,
>>  				   const struct switchdev_obj_port_vlan *vlan,
>>  				   struct switchdev_trans *trans)
>>  {
>> -	struct dsa_slave_priv *p = netdev_priv(dev);
>> -	struct dsa_switch *ds = p->parent;
>>  
> 
> Extra newline ^.
> 
>>  	if (switchdev_trans_ph_prepare(trans)) {
>>  		if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
>>  			return -EOPNOTSUPP;
>>  
>> -		return ds->ops->port_vlan_prepare(ds, p->port, vlan, trans);
>> +		return ds->ops->port_vlan_prepare(ds, port, vlan, trans);
>>  	}
>>  
>> -	ds->ops->port_vlan_add(ds, p->port, vlan, trans);
>> +	ds->ops->port_vlan_add(ds, port, vlan, trans);
>>  
>>  	return 0;
>>  }
>>  
>> -static int dsa_slave_port_vlan_del(struct net_device *dev,
>> +static int dsa_slave_port_vlan_del(struct dsa_switch *ds, int port,
>>  				   const struct switchdev_obj_port_vlan *vlan)
>>  {
>> -	struct dsa_slave_priv *p = netdev_priv(dev);
>> -	struct dsa_switch *ds = p->parent;
>> -
>>  	if (!ds->ops->port_vlan_del)
>>  		return -EOPNOTSUPP;
>>  
>> -	return ds->ops->port_vlan_del(ds, p->port, vlan);
>> +	return ds->ops->port_vlan_del(ds, port, vlan);
>>  }
>>  
>>  static int dsa_slave_port_vlan_dump(struct net_device *dev,
>> @@ -465,8 +460,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>>  				  const struct switchdev_obj *obj,
>>  				  struct switchdev_trans *trans)
>>  {
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_switch *ds = p->parent;
>> +	int port = p->port;
>>  	int err;
>>  
>> +	/* Here we may be called with an orig_dev which is different from dev,
>> +	 * on purpose, to receive request coming from e.g the bridge master
>> +	 * device. Although there are no network device associated with CPU/DSA
>> +	 * ports, we may still have programming operation for these ports.
>> +	 */
>> +	if (obj->orig_dev == p->bridge_dev) {
>> +		ds = ds->dst->ds[0];
>> +		port = ds->dst->cpu_port;
>> +	}
>> +
>>  	/* For the prepare phase, ensure the full set of changes is feasable in
>>  	 * one go in order to signal a failure properly. If an operation is not
>>  	 * supported, return -EOPNOTSUPP.
>> @@ -483,7 +491,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>>  					     trans);
>>  		break;
>>  	case SWITCHDEV_OBJ_ID_PORT_VLAN:
>> -		err = dsa_slave_port_vlan_add(dev,
>> +		err = dsa_slave_port_vlan_add(ds, port,
>>  					      SWITCHDEV_OBJ_PORT_VLAN(obj),
>>  					      trans);
> 
> Note that dsa_slave_port_vlan_add() will be called N times, N being the
> number of bridge ports. This is not an issue for the moment though.
> Programming it only once requires caching, so leave it for an eventual
> future patch.
> 
> When issuing the following command (lan0 being a member of br0):
> 
>     # bridge vlan add vid 42 dev lan0
> 
> the CPU port is also programmed as tagged in VLAN 42. Is that expected?

The first time the VLAN id is programmed to either lan0 or br0, and it
did not exist prior to that call, it also gets populated into the bridge
VLAN database, which is why both the lan0 interface and the CPU port get
programmed.
-- 
Florian

[Bridge] [RFC net-next 3/3] net: dsa: b53: Remove CPU port specific VLAN programming

[Florian Fainelli]

Now that DSA calls into the switch driver to program the CPU port's VLAN
attributes, we can get rid of the code that dealt with adding/removing
the CPU port to a downstream facing port VLAN membership.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 7717b19dc806..6577286a2721 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -951,7 +951,6 @@ static void b53_vlan_add(struct dsa_switch *ds, int port,
 	struct b53_device *dev = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
-	unsigned int cpu_port = dev->cpu_port;
 	struct b53_vlan *vl;
 	u16 vid;
 
@@ -960,11 +959,11 @@ static void b53_vlan_add(struct dsa_switch *ds, int port,
 
 		b53_get_vlan_entry(dev, vid, vl);
 
-		vl->members |= BIT(port) | BIT(cpu_port);
+		vl->members |= BIT(port);
 		if (untagged)
-			vl->untag |= BIT(port) | BIT(cpu_port);
+			vl->untag |= BIT(port);
 		else
-			vl->untag &= ~(BIT(port) | BIT(cpu_port));
+			vl->untag &= ~BIT(port);
 
 		b53_set_vlan_entry(dev, vid, vl);
 		b53_fast_age_vlan(dev, vid);
@@ -973,8 +972,6 @@ static void b53_vlan_add(struct dsa_switch *ds, int port,
 	if (pvid) {
 		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
 			    vlan->vid_end);
-		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(cpu_port),
-			    vlan->vid_end);
 		b53_fast_age_vlan(dev, vid);
 	}
 }
@@ -984,7 +981,6 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 {
 	struct b53_device *dev = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
-	unsigned int cpu_port = dev->cpu_port;
 	struct b53_vlan *vl;
 	u16 vid;
 	u16 pvid;
@@ -997,8 +993,6 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 		b53_get_vlan_entry(dev, vid, vl);
 
 		vl->members &= ~BIT(port);
-		if ((vl->members & BIT(cpu_port)) == BIT(cpu_port))
-			vl->members = 0;
 
 		if (pvid == vid) {
 			if (is5325(dev) || is5365(dev))
@@ -1007,18 +1001,14 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 				pvid = 0;
 		}
 
-		if (untagged) {
+		if (untagged)
 			vl->untag &= ~(BIT(port));
-			if ((vl->untag & BIT(cpu_port)) == BIT(cpu_port))
-				vl->untag = 0;
-		}
 
 		b53_set_vlan_entry(dev, vid, vl);
 		b53_fast_age_vlan(dev, vid);
 	}
 
 	b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), pvid);
-	b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(cpu_port), pvid);
 	b53_fast_age_vlan(dev, pvid);
 
 	return 0;
@@ -1396,8 +1386,8 @@ static void b53_br_leave(struct dsa_switch *ds, int port)
 		b53_write16(dev, B53_VLAN_PAGE, B53_JOIN_ALL_VLAN_EN, reg);
 	} else {
 		b53_get_vlan_entry(dev, pvid, vl);
-		vl->members |= BIT(port) | BIT(dev->cpu_port);
-		vl->untag |= BIT(port) | BIT(dev->cpu_port);
+		vl->members |= BIT(port);
+		vl->untag |= BIT(port);
 		b53_set_vlan_entry(dev, pvid, vl);
 	}
 }
-- 
2.9.3

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> This patch series allows using the bridge master interface to configure
> an Ethernet switch port's CPU/management port with different VLAN attributes than
> those of the bridge downstream ports/members.
>
> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
> tested this with b53 and a mockup DSA driver.

Patchset looks fine to me overall. I'm cooking a patch similar to 3/3
for mv88e6xxx to put on top of this patchset.

Minor comments in individual patchs will follow.

> Open questions:
>
> - if we have more than one bridge on top of a physical switch, the driver
>   should keep track of that and verify that we are not going to change
>   the CPU port VLAN attributes in a way that results in incompatible settings
>   to be applied

In mv88e6xxx, mv88e6xxx_port_check_hw_vlan() does that. It needs a small
adjustment though.

> - if the default behavior is to have all VLANs associated with the CPU port
>   be ingressing/egressing tagged to the CPU, is this really useful?

I have no strong opinion on this. Intuitively I'd expect the CPU port to
be excluded until I add it myself, but I didn't think much about it.

Thanks,

        Vivien

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Ido Schimmel]

Hi Florian,

On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
> Hi all,
> 
> This patch series allows using the bridge master interface to configure
> an Ethernet switch port's CPU/management port with different VLAN attributes than
> those of the bridge downstream ports/members.
> 
> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
> tested this with b53 and a mockup DSA driver.

We'll need to add a check in mlxsw and ignore any VLAN configuration for
the bridge device itself. Otherwise, any configuration done on br0 will
be propagated to all of its slaves, which is incorrect.

> 
> Open questions:
> 
> - if we have more than one bridge on top of a physical switch, the driver
>   should keep track of that and verify that we are not going to change
>   the CPU port VLAN attributes in a way that results in incompatible settings
>   to be applied
> 
> - if the default behavior is to have all VLANs associated with the CPU port
>   be ingressing/egressing tagged to the CPU, is this really useful?

First of all, I want to be sure that when we say "CPU port", we're
talking about the same thing. In mlxsw, the CPU port is a pipe between
the device and the host, through which all packets trapped to the host
go through. So, when a packet is trapped, the driver reads its Rx
descriptor, checks through which port it ingressed, resolves its netdev,
sets skb->dev accordingly and injects it to the Rx path via
netif_receive_skb(). The CPU port itself isn't represented using a
netdev.

Given the above, having VLAN filters (or STP) on the CPU port itself
isn't really helpful (we do have them for physical ports of course...).
So, mlxsw will not benefit from this patchset and if we've the same
concept of "CPU port", then I'm not sure why you don't just enable all
the VLANs on it?

Also, how are you going to set the VLAN filters for the CPU port when
you don't offload a bridge, but instead vlan devices between which you
route packets? You lose your abstraction of CPU port...

Thanks!

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Florian Fainelli]

On 11/22/2016 09:41 AM, Ido Schimmel wrote:
> Hi Florian,
> 
> On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series allows using the bridge master interface to configure
>> an Ethernet switch port's CPU/management port with different VLAN attributes than
>> those of the bridge downstream ports/members.
>>
>> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
>> tested this with b53 and a mockup DSA driver.
> 
> We'll need to add a check in mlxsw and ignore any VLAN configuration for
> the bridge device itself. Otherwise, any configuration done on br0 will
> be propagated to all of its slaves, which is incorrect.
> 
>>
>> Open questions:
>>
>> - if we have more than one bridge on top of a physical switch, the driver
>>   should keep track of that and verify that we are not going to change
>>   the CPU port VLAN attributes in a way that results in incompatible settings
>>   to be applied
>>
>> - if the default behavior is to have all VLANs associated with the CPU port
>>   be ingressing/egressing tagged to the CPU, is this really useful?
> 
> First of all, I want to be sure that when we say "CPU port", we're
> talking about the same thing. In mlxsw, the CPU port is a pipe between
> the device and the host, through which all packets trapped to the host
> go through. So, when a packet is trapped, the driver reads its Rx
> descriptor, checks through which port it ingressed, resolves its netdev,
> sets skb->dev accordingly and injects it to the Rx path via
> netif_receive_skb(). The CPU port itself isn't represented using a
> netdev.

In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
premise, this driver plus the DSA tag protocol hook do exactly the same
things as you just describe.

> 
> Given the above, having VLAN filters (or STP) on the CPU port itself
> isn't really helpful (we do have them for physical ports of course...).
> So, mlxsw will not benefit from this patchset and if we've the same
> concept of "CPU port", then I'm not sure why you don't just enable all
> the VLANs on it?

We do enable all VLANs on the CPU port (at least with b53, but I think
mv88e6xxx does it too), but compared to e.g: mlxsw, we trap all traffic
by default, and actually, quite often (always actually, until we add IP
routing offloads) the CPU is involved in the LAN/WAN routing, so it is
not infrequent to have the following packet flow:

LAN port -> VLAN 1 -> eth0.1 -> NAT/routing -> eth0.2 -> VLAN 2 -> WAN port

In that case, having the ability to define the per-port membership for
VLANs, including the CPU, kind of helps, especially if there are
private/guests VLAN on either the LAN or WAN segments that the CPU does
not necessarily need to play a role in.

NB: this scheme works because in most configurations that we support
today, the CPU port's speed is greater or equal than the speed of the
downstream/front panel ports.

> 
> Also, how are you going to set the VLAN filters for the CPU port when
> you don't offload a bridge, but instead vlan devices between which you
> route packets? You lose your abstraction of CPU port...

As far as I can tell today, this is not particularly helpful with DSA,
where we start with all traffic going to the CPU (each DSA created
network device is segregated from the other) and only then we require
having bridge VLAN filtering enabled in the kernel, and configuring
bridge VLAN membership to have a proper VLAN-based scheme.

If you did configure VLAN membership with e.g: port0.<vid> we could
support that just fine, but that programming interface does not allow
configuring the default VLAN, and in our case, it matters a bit to
support the LAN/WAN routing scenario described. We could agree that all
untagged traffic should go to VLAN 0 or 1 for instance, but that could
then, vary on a per-driver/HW basis.

Hope this clarifies things a bit!
-- 
Florian

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Ido Schimmel]

Hi Florian,

On Tue, Nov 22, 2016 at 09:56:30AM -0800, Florian Fainelli wrote:
> On 11/22/2016 09:41 AM, Ido Schimmel wrote:
> > Hi Florian,
> > 
> > On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
> >> Hi all,
> >>
> >> This patch series allows using the bridge master interface to configure
> >> an Ethernet switch port's CPU/management port with different VLAN attributes than
> >> those of the bridge downstream ports/members.
> >>
> >> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
> >> tested this with b53 and a mockup DSA driver.
> > 
> > We'll need to add a check in mlxsw and ignore any VLAN configuration for
> > the bridge device itself. Otherwise, any configuration done on br0 will
> > be propagated to all of its slaves, which is incorrect.
> > 
> >>
> >> Open questions:
> >>
> >> - if we have more than one bridge on top of a physical switch, the driver
> >>   should keep track of that and verify that we are not going to change
> >>   the CPU port VLAN attributes in a way that results in incompatible settings
> >>   to be applied
> >>
> >> - if the default behavior is to have all VLANs associated with the CPU port
> >>   be ingressing/egressing tagged to the CPU, is this really useful?
> > 
> > First of all, I want to be sure that when we say "CPU port", we're
> > talking about the same thing. In mlxsw, the CPU port is a pipe between
> > the device and the host, through which all packets trapped to the host
> > go through. So, when a packet is trapped, the driver reads its Rx
> > descriptor, checks through which port it ingressed, resolves its netdev,
> > sets skb->dev accordingly and injects it to the Rx path via
> > netif_receive_skb(). The CPU port itself isn't represented using a
> > netdev.
> 
> In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
> premise, this driver plus the DSA tag protocol hook do exactly the same
> things as you just describe.

Thanks for the detailed explanation! I also took the time to read
dsa.txt, however I still don't quite understand the motivation for
VLAN filtering on the CPU port. In which cases would you like to prevent
packets from going to the host due to their VLAN header? This change
would make sense to me if you only had a limited number of VLANs you
could enable on the CPU port, but from your response I understand that
this isn't the case.

FWIW, I don't have a problem with patches if they are useful for you,
I'm just trying to understand the use case. We can easily patch mlxsw to
ignore calls with orig_dev=br0.

Thanks!

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Florian Fainelli]

On 11/23/2016 05:48 AM, Ido Schimmel wrote:
> Hi Florian,
> 
> On Tue, Nov 22, 2016 at 09:56:30AM -0800, Florian Fainelli wrote:
>> On 11/22/2016 09:41 AM, Ido Schimmel wrote:
>>> Hi Florian,
>>>
>>> On Mon, Nov 21, 2016 at 11:09:22AM -0800, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series allows using the bridge master interface to configure
>>>> an Ethernet switch port's CPU/management port with different VLAN attributes than
>>>> those of the bridge downstream ports/members.
>>>>
>>>> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
>>>> tested this with b53 and a mockup DSA driver.
>>>
>>> We'll need to add a check in mlxsw and ignore any VLAN configuration for
>>> the bridge device itself. Otherwise, any configuration done on br0 will
>>> be propagated to all of its slaves, which is incorrect.
>>>
>>>>
>>>> Open questions:
>>>>
>>>> - if we have more than one bridge on top of a physical switch, the driver
>>>>   should keep track of that and verify that we are not going to change
>>>>   the CPU port VLAN attributes in a way that results in incompatible settings
>>>>   to be applied
>>>>
>>>> - if the default behavior is to have all VLANs associated with the CPU port
>>>>   be ingressing/egressing tagged to the CPU, is this really useful?
>>>
>>> First of all, I want to be sure that when we say "CPU port", we're
>>> talking about the same thing. In mlxsw, the CPU port is a pipe between
>>> the device and the host, through which all packets trapped to the host
>>> go through. So, when a packet is trapped, the driver reads its Rx
>>> descriptor, checks through which port it ingressed, resolves its netdev,
>>> sets skb->dev accordingly and injects it to the Rx path via
>>> netif_receive_skb(). The CPU port itself isn't represented using a
>>> netdev.
>>
>> In the case of DSA, the CPU port is a normal Ethernet MAC driver, but in
>> premise, this driver plus the DSA tag protocol hook do exactly the same
>> things as you just describe.
> 
> Thanks for the detailed explanation! I also took the time to read
> dsa.txt, however I still don't quite understand the motivation for
> VLAN filtering on the CPU port. In which cases would you like to prevent
> packets from going to the host due to their VLAN header? This change
> would make sense to me if you only had a limited number of VLANs you
> could enable on the CPU port, but from your response I understand that
> this isn't the case.

It's not much about VLAN filtering per-se, but more about the default
VLAN membership of the CPU port, in the absence of any explicit
configuration. As an user, I find it a little inconvenient to have to
create one VLAN interface per VLAN that I am adding to the bridge to be
able to terminate that traffic properly towards the host/CPU/management
interface, especially when this VLAN is untagged.

This is really the motivation for these patches: if there is only one
VLAN configured, and it's the default VLAN for all ports, then the
bridge master interface also terminates this VLAN with the same
properties as those added by default (typically with default_pvid: VID 1
untagged, unless changed of course).

If you don't want that as an user, you now have the ability to change
it, and make this VLAN (or any other for that matter) to be terminated
differently at the host/CPU/management port level than how it is
egressing at the downstream ports part of that VLAN too.

Maybe it's a bit overkill...

> 
> FWIW, I don't have a problem with patches if they are useful for you,
> I'm just trying to understand the use case. We can easily patch mlxsw to
> ignore calls with orig_dev=br0.

OK, if I resubmit, I will try to take care of mlxsw and rocker as well.

Thanks!
-- 
Florian

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Florian Fainelli]

On 11/22/2016 02:08 PM, Jiri Pirko wrote:
> Tue, Nov 22, 2016 at 06:48:29PM CET, andrew@lunn.ch wrote:
>> Hi Ido
>>
>>> First of all, I want to be sure that when we say "CPU port", we're
>>> talking about the same thing. In mlxsw, the CPU port is a pipe between
>>> the device and the host, through which all packets trapped to the host
>>> go through. So, when a packet is trapped, the driver reads its Rx
>>> descriptor, checks through which port it ingressed, resolves its netdev,
>>> sets skb->dev accordingly and injects it to the Rx path via
>>> netif_receive_skb(). The CPU port itself isn't represented using a
>>> netdev.
>>
>> With DSA, we have a real physical ethernet network interface for the
>> 'cpu' port. It connects to one of the ports of the switch. Frames on
> 
> Every port should be visible as a netdevice, including cpu port.
> Would it make sence to have representors for those?

The CPU port is kind of already visible with DSA since you need the
switch to be attached to a normal Ethernet MAC driver (later referenced
as eth0 for simplicity). Since eth0 is going to potentially receive/send
switch tagged traffic, and the model is to terminate the interfaces at
the port level, this interface does not really have any meaningful use
from a data exchange, apart from multiplexing/demultiplexing switch tags
(when enabled).

If we did create a "cpu" network device, this interface would not be
able to send/receive traffic either, because the per-port network
interfaces are terminated at their level, and the conduit interface is
just used for transmitting/receiving switch tagged traffic. It does have
value as a controlling interface only though.

As a controlling interface, this can be helpful, but we need to decide
which side of the switch this CPU interface would represent, is it the
switch's view of the CPU port, or is the Ethernet MAC view's of the
switch's CPU port, attached to it (especially true with discrete switch
chips).

If we did use eth0 as a controlling interface, we need to somehow be
able to overload (in an objected oriented fashioned) the netdev_ops,
ethtool_ops and switchdev_ops for that interface so as to make it
participate in the switch configuration (we actually do this already for
ethtool statistics, but this is ugly).
-- 
Florian