[RFC 0/9] net: dsa: mv88e6352: add 802.1q VLAN support

[RFC 0/9] net: dsa: mv88e6352: add 802.1q VLAN support

[Vivien Didelot]

This patchset brings support for the 802.1q and VLAN operations to the Marvell
88E6352 switch and compatibles.

The patch 1/9 adds more glue between switchdev and DSA.

Patches 2/9 to 4/9 add the VLAN operations to DSA and the mv88e6xxx family of
switches.

Patches 5/9 to 9/9 is the necessary configuration that I figured out during the
VLAN tests I did. So far, I was able to confirm the following scenarios:

  * Port untagged to Port untagged
  * Port untagged to Port tagged
  * Port tagged to Port tagged
  * Port untagged to CPU tagged
  * Port tagged to CPU tagged
  * CPU tagged to port tagged

>From the userspace, here's a few commands I use to start using VLANs:

    # create bridge
    ip link set dev eth0 up
    ip link add name br0 type bridge
    echo 1 > /sys/class/net/br0/bridge/vlan_filtering
    ip link set dev br0 up

    # setup switch ports
    for i in $(seq 0 4); do
      ip link set dev swp$i up master br0
      # HACK: bridge assumes the following but doesn't call ndo_bridge_setlink
      bridge vlan add vid 1 pvid untagged dev swp$i master self
    done

Best,
 -v

Vivien Didelot (9):
  net: dsa: add basic support for switchdev obj
  net: dsa: add basic support for VLAN operations
  net: dsa: mv88e6xxx: add support for VTU ops
  net: dsa: mv88e6352: add support for VLAN
  net: dsa: mv88e6352: disable mirroring
  net: dsa: mv88e6352: allow egress of unknown multicast
  net: dsa: mv88e6352: lock CPU port from learning addresses
  net: dsa: mv88e6352: set port 802.1Q mode to Secure
  net: dsa: fix EDSA frame from hwaccel frame

 drivers/net/dsa/mv88e6352.c |  12 +-
 drivers/net/dsa/mv88e6xxx.c | 303 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx.h |  39 ++++++
 include/net/dsa.h           |   7 +
 net/dsa/slave.c             | 100 ++++++++++++++-
 net/dsa/tag_edsa.c          |   5 +
 6 files changed, 446 insertions(+), 20 deletions(-)

-- 
2.4.1

[RFC 1/9] net: dsa: add basic support for switchdev obj

[Vivien Didelot]

This patch adds the switchdev operations to add and delete switchdev
objects. This will be necessary to add fdb or VLAN entries.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 04ffad3..cbda00a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -363,6 +363,43 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
+static int dsa_slave_port_obj_add(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	int err;
+
+	/*
+	 * Skip the prepare phase, since currently the DSA drivers don't need to
+	 * allocate any memory for operations and they will not fail to HW
+	 * (unless something horrible goes wrong on the MDIO bus, in which case
+	 * the prepare phase wouldn't have been able to predict anyway).
+	 */
+	if (obj->trans != SWITCHDEV_TRANS_COMMIT)
+		return 0;
+
+	switch (obj->id) {
+	default:
+		err = -ENOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
+static int dsa_slave_port_obj_del(struct net_device *dev,
+				  struct switchdev_obj *obj)
+{
+	int err;
+
+	switch (obj->id) {
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+
+	return err;
+}
+
 static int dsa_slave_bridge_port_join(struct net_device *dev,
 				      struct net_device *br)
 {
@@ -702,6 +739,8 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 static const struct switchdev_ops dsa_slave_switchdev_ops = {
 	.switchdev_port_attr_get	= dsa_slave_port_attr_get,
 	.switchdev_port_attr_set	= dsa_slave_port_attr_set,
+	.switchdev_port_obj_add		= dsa_slave_port_obj_add,
+	.switchdev_port_obj_del		= dsa_slave_port_obj_del,
 };
 
 static void dsa_slave_adjust_link(struct net_device *dev)
-- 
2.4.1

[RFC 2/9] net: dsa: add basic support for VLAN operations

[Vivien Didelot]

This patch adds the glue between DSA and switchdev to add and delete
SWITCHDEV_OBJ_PORT_VLAN objects.

This will allow the DSA switch drivers implementing the port_vlan_add
and port_vlan_del functions to access the switch VLAN database through
userspace commands such as "bridge vlan".

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h |  7 +++++++
 net/dsa/slave.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63b..726357b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -302,6 +302,13 @@ struct dsa_switch_driver {
 			   const unsigned char *addr, u16 vid);
 	int	(*fdb_getnext)(struct dsa_switch *ds, int port,
 			       unsigned char *addr, bool *is_static);
+
+	/*
+	 * VLAN support
+	 */
+	int	(*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
+				 u16 bridge_flags);
+	int	(*port_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
 };
 
 void register_switch_driver(struct dsa_switch_driver *type);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index cbda00a..52ba5a1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -363,6 +363,25 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
 	return ret;
 }
 
+static int dsa_slave_port_vlans_add(struct net_device *dev,
+				    struct switchdev_obj_vlan *vlan)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int vid, err = 0;
+
+	if (!ds->drv->port_vlan_add)
+		return -ENOTSUPP;
+
+	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
+		err = ds->drv->port_vlan_add(ds, p->port, vid, vlan->flags);
+		if (err)
+			break;
+	}
+
+	return err;
+}
+
 static int dsa_slave_port_obj_add(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
@@ -378,6 +397,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 		return 0;
 
 	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_VLAN:
+		err = dsa_slave_port_vlans_add(dev, &obj->u.vlan);
+		break;
 	default:
 		err = -ENOTSUPP;
 		break;
@@ -386,12 +408,34 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
 	return err;
 }
 
+static int dsa_slave_port_vlans_del(struct net_device *dev,
+				    struct switchdev_obj_vlan *vlan)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+	int vid, err = 0;
+
+	if (!ds->drv->port_vlan_del)
+		return -ENOTSUPP;
+
+	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
+		err = ds->drv->port_vlan_del(ds, p->port, vid);
+		if (err)
+			break;
+	}
+
+	return err;
+}
+
 static int dsa_slave_port_obj_del(struct net_device *dev,
 				  struct switchdev_obj *obj)
 {
 	int err;
 
 	switch (obj->id) {
+	case SWITCHDEV_OBJ_PORT_VLAN:
+		err = dsa_slave_port_vlans_del(dev, &obj->u.vlan);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
@@ -473,6 +517,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+static int dsa_slave_vlan_noop(struct net_device *dev, __be16 proto, u16 vid)
+{
+	/* NETIF_F_HW_VLAN_CTAG_FILTER requires ndo_vlan_rx_add_vid and
+	 * ndo_vlan_rx_kill_vid, otherwise the VLAN acceleration is considered
+	 * buggy (see net/core/dev.c).
+	 */
+	return 0;
+}
+
 
 /* ethtool operations *******************************************************/
 static int
@@ -734,6 +787,10 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_fdb_dump		= dsa_slave_fdb_dump,
 	.ndo_do_ioctl		= dsa_slave_ioctl,
 	.ndo_get_iflink		= dsa_slave_get_iflink,
+	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_noop,
+	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_noop,
+	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
+	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
 };
 
 static const struct switchdev_ops dsa_slave_switchdev_ops = {
@@ -924,7 +981,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	if (slave_dev == NULL)
 		return -ENOMEM;
 
-	slave_dev->features = master->vlan_features;
+	slave_dev->features = master->vlan_features | NETIF_F_VLAN_FEATURES;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
 	eth_hw_addr_inherit(slave_dev, master);
 	slave_dev->tx_queue_len = 0;
@@ -936,7 +993,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	SET_NETDEV_DEV(slave_dev, parent);
 	slave_dev->dev.of_node = ds->pd->port_dn[port];
-	slave_dev->vlan_features = master->vlan_features;
+	slave_dev->vlan_features = slave_dev->features;
 
 	p = netdev_priv(slave_dev);
 	p->dev = slave_dev;
-- 
2.4.1

Re: [RFC 2/9] net: dsa: add basic support for VLAN operations

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This patch adds the glue between DSA and switchdev to add and delete
> SWITCHDEV_OBJ_PORT_VLAN objects.
>
> This will allow the DSA switch drivers implementing the port_vlan_add
> and port_vlan_del functions to access the switch VLAN database through
> userspace commands such as "bridge vlan".
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   include/net/dsa.h |  7 +++++++
>   net/dsa/slave.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63b..726357b 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -302,6 +302,13 @@ struct dsa_switch_driver {
>   			   const unsigned char *addr, u16 vid);
>   	int	(*fdb_getnext)(struct dsa_switch *ds, int port,
>   			       unsigned char *addr, bool *is_static);
> +
> +	/*
> +	 * VLAN support
> +	 */
> +	int	(*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
> +				 u16 bridge_flags);
> +	int	(*port_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
>   };
>
>   void register_switch_driver(struct dsa_switch_driver *type);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index cbda00a..52ba5a1 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -363,6 +363,25 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>   	return ret;
>   }
>
> +static int dsa_slave_port_vlans_add(struct net_device *dev,
> +				    struct switchdev_obj_vlan *vlan)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	int vid, err = 0;
> +
> +	if (!ds->drv->port_vlan_add)
> +		return -ENOTSUPP;
> +
> +	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
> +		err = ds->drv->port_vlan_add(ds, p->port, vid, vlan->flags);
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
> +}
> +
>   static int dsa_slave_port_obj_add(struct net_device *dev,
>   				  struct switchdev_obj *obj)
>   {
> @@ -378,6 +397,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>   		return 0;
>
>   	switch (obj->id) {
> +	case SWITCHDEV_OBJ_PORT_VLAN:
> +		err = dsa_slave_port_vlans_add(dev, &obj->u.vlan);
> +		break;
>   	default:
>   		err = -ENOTSUPP;
>   		break;
> @@ -386,12 +408,34 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>   	return err;
>   }
>
> +static int dsa_slave_port_vlans_del(struct net_device *dev,
> +				    struct switchdev_obj_vlan *vlan)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	int vid, err = 0;
> +
> +	if (!ds->drv->port_vlan_del)
> +		return -ENOTSUPP;
> +
> +	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
> +		err = ds->drv->port_vlan_del(ds, p->port, vid);
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
> +}
> +
>   static int dsa_slave_port_obj_del(struct net_device *dev,
>   				  struct switchdev_obj *obj)
>   {
>   	int err;
>
>   	switch (obj->id) {
> +	case SWITCHDEV_OBJ_PORT_VLAN:
> +		err = dsa_slave_port_vlans_del(dev, &obj->u.vlan);
> +		break;
>   	default:
>   		err = -EOPNOTSUPP;
>   		break;
> @@ -473,6 +517,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
>   	return NETDEV_TX_OK;
>   }
>
> +static int dsa_slave_vlan_noop(struct net_device *dev, __be16 proto, u16 vid)
> +{
> +	/* NETIF_F_HW_VLAN_CTAG_FILTER requires ndo_vlan_rx_add_vid and
> +	 * ndo_vlan_rx_kill_vid, otherwise the VLAN acceleration is considered
> +	 * buggy (see net/core/dev.c).
> +	 */

As Scott mentioned, just don't set NETIF_F_HW_VLAN_CTAG_FILTER.

I don't entirely understand why we would not want to filter VLANs in the switch.
Can you explain ?

Thanks,
Guenter

Re: [RFC 2/9] net: dsa: add basic support for VLAN operations

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This patch adds the glue between DSA and switchdev to add and delete
> SWITCHDEV_OBJ_PORT_VLAN objects.
>
> This will allow the DSA switch drivers implementing the port_vlan_add
> and port_vlan_del functions to access the switch VLAN database through
> userspace commands such as "bridge vlan".
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   include/net/dsa.h |  7 +++++++
>   net/dsa/slave.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fbca63b..726357b 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -302,6 +302,13 @@ struct dsa_switch_driver {
>   			   const unsigned char *addr, u16 vid);
>   	int	(*fdb_getnext)(struct dsa_switch *ds, int port,
>   			       unsigned char *addr, bool *is_static);
> +
> +	/*
> +	 * VLAN support
> +	 */
> +	int	(*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
> +				 u16 bridge_flags);
> +	int	(*port_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
>   };
>
>   void register_switch_driver(struct dsa_switch_driver *type);
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index cbda00a..52ba5a1 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -363,6 +363,25 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>   	return ret;
>   }
>
> +static int dsa_slave_port_vlans_add(struct net_device *dev,
> +				    struct switchdev_obj_vlan *vlan)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	int vid, err = 0;
> +
> +	if (!ds->drv->port_vlan_add)
> +		return -ENOTSUPP;
> +
> +	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
> +		err = ds->drv->port_vlan_add(ds, p->port, vid, vlan->flags);
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
> +}
> +
>   static int dsa_slave_port_obj_add(struct net_device *dev,
>   				  struct switchdev_obj *obj)
>   {
> @@ -378,6 +397,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>   		return 0;
>
>   	switch (obj->id) {
> +	case SWITCHDEV_OBJ_PORT_VLAN:
> +		err = dsa_slave_port_vlans_add(dev, &obj->u.vlan);
> +		break;
>   	default:
>   		err = -ENOTSUPP;
>   		break;
> @@ -386,12 +408,34 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>   	return err;
>   }
>
> +static int dsa_slave_port_vlans_del(struct net_device *dev,
> +				    struct switchdev_obj_vlan *vlan)
> +{
> +	struct dsa_slave_priv *p = netdev_priv(dev);
> +	struct dsa_switch *ds = p->parent;
> +	int vid, err = 0;
> +
> +	if (!ds->drv->port_vlan_del)
> +		return -ENOTSUPP;
> +
> +	for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
> +		err = ds->drv->port_vlan_del(ds, p->port, vid);
> +		if (err)
> +			break;
> +	}
> +
> +	return err;
> +}
> +
>   static int dsa_slave_port_obj_del(struct net_device *dev,
>   				  struct switchdev_obj *obj)
>   {
>   	int err;
>
>   	switch (obj->id) {
> +	case SWITCHDEV_OBJ_PORT_VLAN:
> +		err = dsa_slave_port_vlans_del(dev, &obj->u.vlan);
> +		break;
>   	default:
>   		err = -EOPNOTSUPP;
>   		break;
> @@ -473,6 +517,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
>   	return NETDEV_TX_OK;
>   }
>
> +static int dsa_slave_vlan_noop(struct net_device *dev, __be16 proto, u16 vid)
> +{
> +	/* NETIF_F_HW_VLAN_CTAG_FILTER requires ndo_vlan_rx_add_vid and
> +	 * ndo_vlan_rx_kill_vid, otherwise the VLAN acceleration is considered
> +	 * buggy (see net/core/dev.c).
> +	 */
> +	return 0;
> +}
> +
>
>   /* ethtool operations *******************************************************/
>   static int
> @@ -734,6 +787,10 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
>   	.ndo_fdb_dump		= dsa_slave_fdb_dump,
>   	.ndo_do_ioctl		= dsa_slave_ioctl,
>   	.ndo_get_iflink		= dsa_slave_get_iflink,
> +	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_noop,
> +	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_noop,
> +	.ndo_bridge_setlink	= switchdev_port_bridge_setlink,
> +	.ndo_bridge_dellink	= switchdev_port_bridge_dellink,
>   };
>
>   static const struct switchdev_ops dsa_slave_switchdev_ops = {
> @@ -924,7 +981,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
>   	if (slave_dev == NULL)
>   		return -ENOMEM;
>
> -	slave_dev->features = master->vlan_features;
> +	slave_dev->features = master->vlan_features | NETIF_F_VLAN_FEATURES;

Hi Vivien,

NETIF_F_VLAN_FEATURES declares that the device supports receive and transmit
tagging offload. We do this on transmit, by calling vlan_hwaccel_push_inside()
with patch 9, but not on the receive side.

I think you may need to add matching code on the receive side to remove
the VLAN tag and move it into the skb with __vlan_hwaccel_put_tag().
It may also be necessary to add the same code for the other tag handlers.

Overall I wonder if it really makes sense to add those flags. Seems to me
that would only make sense if the resulting code gets more efficient,
which at least currently is not the case. Am I missing something ?

Thanks,
Guenter

Re: [RFC 2/9] net: dsa: add basic support for VLAN operations

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 10:42 AM, Guenter Roeck linux@roeck-us.net wrote:
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This patch adds the glue between DSA and switchdev to add and delete
>> SWITCHDEV_OBJ_PORT_VLAN objects.
>>
>> This will allow the DSA switch drivers implementing the port_vlan_add
>> and port_vlan_del functions to access the switch VLAN database through
>> userspace commands such as "bridge vlan".
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>   include/net/dsa.h |  7 +++++++
>>   net/dsa/slave.c   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index fbca63b..726357b 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -302,6 +302,13 @@ struct dsa_switch_driver {
>>                 const unsigned char *addr, u16 vid);
>>      int (*fdb_getnext)(struct dsa_switch *ds, int port,
>>                     unsigned char *addr, bool *is_static);
>> +
>> +    /*
>> +     * VLAN support
>> +     */
>> +    int (*port_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
>> +                 u16 bridge_flags);
>> +    int (*port_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
>>   };
>>
>>   void register_switch_driver(struct dsa_switch_driver *type);
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index cbda00a..52ba5a1 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -363,6 +363,25 @@ static int dsa_slave_port_attr_set(struct net_device *dev,
>>      return ret;
>>   }
>>
>> +static int dsa_slave_port_vlans_add(struct net_device *dev,
>> +                    struct switchdev_obj_vlan *vlan)
>> +{
>> +    struct dsa_slave_priv *p = netdev_priv(dev);
>> +    struct dsa_switch *ds = p->parent;
>> +    int vid, err = 0;
>> +
>> +    if (!ds->drv->port_vlan_add)
>> +        return -ENOTSUPP;
>> +
>> +    for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
>> +        err = ds->drv->port_vlan_add(ds, p->port, vid, vlan->flags);
>> +        if (err)
>> +            break;
>> +    }
>> +
>> +    return err;
>> +}
>> +
>>   static int dsa_slave_port_obj_add(struct net_device *dev,
>>                    struct switchdev_obj *obj)
>>   {
>> @@ -378,6 +397,9 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>>          return 0;
>>
>>      switch (obj->id) {
>> +    case SWITCHDEV_OBJ_PORT_VLAN:
>> +        err = dsa_slave_port_vlans_add(dev, &obj->u.vlan);
>> +        break;
>>      default:
>>          err = -ENOTSUPP;
>>          break;
>> @@ -386,12 +408,34 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>>      return err;
>>   }
>>
>> +static int dsa_slave_port_vlans_del(struct net_device *dev,
>> +                    struct switchdev_obj_vlan *vlan)
>> +{
>> +    struct dsa_slave_priv *p = netdev_priv(dev);
>> +    struct dsa_switch *ds = p->parent;
>> +    int vid, err = 0;
>> +
>> +    if (!ds->drv->port_vlan_del)
>> +        return -ENOTSUPP;
>> +
>> +    for (vid = vlan->vid_start; vid <= vlan->vid_end; ++vid) {
>> +        err = ds->drv->port_vlan_del(ds, p->port, vid);
>> +        if (err)
>> +            break;
>> +    }
>> +
>> +    return err;
>> +}
>> +
>>   static int dsa_slave_port_obj_del(struct net_device *dev,
>>                    struct switchdev_obj *obj)
>>   {
>>      int err;
>>
>>      switch (obj->id) {
>> +    case SWITCHDEV_OBJ_PORT_VLAN:
>> +        err = dsa_slave_port_vlans_del(dev, &obj->u.vlan);
>> +        break;
>>      default:
>>          err = -EOPNOTSUPP;
>>          break;
>> @@ -473,6 +517,15 @@ static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff
>> *skb,
>>      return NETDEV_TX_OK;
>>   }
>>
>> +static int dsa_slave_vlan_noop(struct net_device *dev, __be16 proto, u16 vid)
>> +{
>> +    /* NETIF_F_HW_VLAN_CTAG_FILTER requires ndo_vlan_rx_add_vid and
>> +     * ndo_vlan_rx_kill_vid, otherwise the VLAN acceleration is considered
>> +     * buggy (see net/core/dev.c).
>> +     */
>> +    return 0;
>> +}
>> +
>>
>>   /* ethtool operations *******************************************************/
>>   static int
>> @@ -734,6 +787,10 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
>>      .ndo_fdb_dump       = dsa_slave_fdb_dump,
>>      .ndo_do_ioctl       = dsa_slave_ioctl,
>>      .ndo_get_iflink     = dsa_slave_get_iflink,
>> +    .ndo_vlan_rx_add_vid    = dsa_slave_vlan_noop,
>> +    .ndo_vlan_rx_kill_vid   = dsa_slave_vlan_noop,
>> +    .ndo_bridge_setlink = switchdev_port_bridge_setlink,
>> +    .ndo_bridge_dellink = switchdev_port_bridge_dellink,
>>   };
>>
>>   static const struct switchdev_ops dsa_slave_switchdev_ops = {
>> @@ -924,7 +981,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
>> *parent,
>>      if (slave_dev == NULL)
>>          return -ENOMEM;
>>
>> -    slave_dev->features = master->vlan_features;
>> +    slave_dev->features = master->vlan_features | NETIF_F_VLAN_FEATURES;
> 
> Hi Vivien,
> 
> NETIF_F_VLAN_FEATURES declares that the device supports receive and transmit
> tagging offload. We do this on transmit, by calling vlan_hwaccel_push_inside()
> with patch 9, but not on the receive side.
> 
> I think you may need to add matching code on the receive side to remove
> the VLAN tag and move it into the skb with __vlan_hwaccel_put_tag().
> It may also be necessary to add the same code for the other tag handlers.
> 
> Overall I wonder if it really makes sense to add those flags. Seems to me
> that would only make sense if the resulting code gets more efficient,
> which at least currently is not the case. Am I missing something ?

I initially added those flags because without them, bridge didn't call
my ndo_vlan_rx_add_vid. But with the switchdev abstraction, they seem
unrelevant.

I just undo this change (keeping slave_dev->{vlan_,}features to
master->vlan_features) and I am able to create VLANs through bridge.

TBH, I'm not familiar at all with these flags. Seems like I must revert
this feature changes.

Thanks,
-v

[RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Vivien Didelot]

This commit implements the port_vlan_add and port_vlan_del functions in
the dsa_switch_driver structure for Marvell 88E6xxx compatible switches.

This allows to access a switch VLAN Table Unit, and thus define VLANs
from standard userspace commands such as "bridge vlan".

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 278 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.h |  27 +++++
 2 files changed, 305 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 7fba330..49ef2f8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2,6 +2,9 @@
  * net/dsa/mv88e6xxx.c - Marvell 88e6xxx switch chip support
  * Copyright (c) 2008 Marvell Semiconductor
  *
+ * Copyright (c) 2015 CMC Electronics, Inc.
+ *	Added support for 802.1q VLAN Table Unit operations
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
@@ -1348,6 +1351,281 @@ static void mv88e6xxx_bridge_work(struct work_struct *work)
 	}
 }
 
+static int _mv88e6xxx_vtu_wait(struct dsa_switch *ds)
+{
+	return _mv88e6xxx_wait(ds, REG_GLOBAL, GLOBAL_VTU_OP,
+			       GLOBAL_VTU_OP_BUSY);
+}
+
+static int _mv88e6xxx_vtu_cmd(struct dsa_switch *ds, u16 op)
+{
+	int ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_OP, op);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_wait(ds);
+}
+
+static int _mv88e6xxx_stu_loadpurge(struct dsa_switch *ds, u8 sid, bool valid)
+{
+	int ret, data;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	data = sid & GLOBAL_VTU_SID_MASK;
+	if (valid)
+		data |= GLOBAL_VTU_VID_VALID;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID, data);
+	if (ret < 0)
+		return ret;
+
+	/* Unused (yet) data registers */
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_8_11, 0);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_STU_LOAD_PURGE);
+}
+
+static int _mv88e6xxx_vtu_getnext(struct dsa_switch *ds, u16 vid,
+				  struct mv88e6xxx_vtu_entry *entry)
+{
+	int ret, i;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID,
+				   vid & GLOBAL_VTU_VID_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_GET_NEXT);
+	if (ret < 0)
+		return ret;
+
+	ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_VID);
+	if (ret < 0)
+		return ret;
+
+	entry->vid = ret & GLOBAL_VTU_VID_MASK;
+	entry->valid = !!(ret & GLOBAL_VTU_VID_VALID);
+
+	if (entry->valid) {
+		/* Ports 0-3, offsets 0, 4, 8, 12 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < 4; ++i)
+			entry->tags[i] = (ret >> (i * 4)) & 3;
+
+		/* Ports 4-6, offsets 0, 4, 8 */
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7);
+		if (ret < 0)
+			return ret;
+
+		for (i = 4; i < 7; ++i)
+			entry->tags[i] = (ret >> ((i - 4) * 4)) & 3;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_FID);
+		if (ret < 0)
+			return ret;
+
+		entry->fid = ret & GLOBAL_VTU_FID_MASK;
+
+		ret = _mv88e6xxx_reg_read(ds, REG_GLOBAL, GLOBAL_VTU_SID);
+		if (ret < 0)
+			return ret;
+
+		entry->sid = ret & GLOBAL_VTU_SID_MASK;
+	}
+
+	return 0;
+}
+
+static int _mv88e6xxx_vtu_loadpurge(struct dsa_switch *ds,
+				    struct mv88e6xxx_vtu_entry *entry)
+{
+	u16 data = 0;
+	int ret, i;
+
+	ret = _mv88e6xxx_vtu_wait(ds);
+	if (ret < 0)
+		return ret;
+
+	if (entry->valid) {
+		/* Set Data Register, ports 0-3, offsets 0, 4, 8, 12 */
+		for (data = i = 0; i < 4; ++i)
+			data |= entry->tags[i] << (i * 4);
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_0_3,
+					   data);
+		if (ret < 0)
+			return ret;
+
+		/* Set Data Register, ports 4-6, offsets 0, 4, 8 */
+		for (data = 0, i = 4; i < 7; ++i)
+			data |= entry->tags[i] << ((i - 4) * 4);
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_4_7,
+					   data);
+		if (ret < 0)
+			return ret;
+
+		/* Unused Data Register */
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_DATA_8_11,
+					   0);
+		if (ret < 0)
+			return ret;
+
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_SID,
+					   entry->sid & GLOBAL_VTU_SID_MASK);
+		if (ret < 0)
+			return ret;
+
+		ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_FID,
+					   entry->fid & GLOBAL_VTU_FID_MASK);
+		if (ret < 0)
+			return ret;
+
+		/* Valid bit set means load, unset means purge */
+		data = GLOBAL_VTU_VID_VALID;
+	}
+
+	data |= entry->vid & GLOBAL_VTU_VID_MASK;
+	ret = _mv88e6xxx_reg_write(ds, REG_GLOBAL, GLOBAL_VTU_VID, data);
+	if (ret < 0)
+		return ret;
+
+	return _mv88e6xxx_vtu_cmd(ds, GLOBAL_VTU_OP_VTU_LOAD_PURGE);
+}
+
+int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid,
+			    u16 bridge_flags)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int prev_vid = vid ? vid - 1 : 4095;
+	int i, ret;
+
+	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
+	if (!vid)
+		return 0;
+
+	/* The DSA port-based VLAN setup reserves FID 0 to DSA_MAX_PORTS;
+	 * we will use the next FIDs for 802.1q;
+	 * thus, forbid the last DSA_MAX_PORTS VLANs.
+	 */
+	if (vid > 4095 - DSA_MAX_PORTS)
+		return -EINVAL;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	/* If the VLAN does not exist, re-initialize the entry for addition */
+	if (entry.vid != vid || !entry.valid) {
+		memset(&entry, 0, sizeof(entry));
+		entry.valid = true;
+		entry.vid = vid;
+		entry.fid = DSA_MAX_PORTS + vid;
+		entry.sid = 0; /* We don't use 802.1s (yet) */
+
+		/* A VTU entry must have a valid STU entry (undocumented).
+		 * The default STU pointer for a VTU entry is 0. If per VLAN
+		 * spanning tree is not used then only one STU entry is needed
+		 * to cover all VTU entries. Thus, validate the STU entry 0.
+		 */
+		ret = _mv88e6xxx_stu_loadpurge(ds, 0, true);
+		if (ret < 0)
+			goto unlock;
+
+		for (i = 0; i < ps->num_ports; ++i)
+			entry.tags[i] = dsa_is_cpu_port(ds, i) ?
+				GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED :
+				GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;
+	}
+
+	entry.tags[port] = bridge_flags & BRIDGE_VLAN_INFO_UNTAGGED ?
+		GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED :
+		GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED;
+
+	ret = _mv88e6xxx_vtu_loadpurge(ds, &entry);
+
+	/* Set port default VID */
+	if (bridge_flags & BRIDGE_VLAN_INFO_PVID)
+		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port),
+					   PORT_DEFAULT_VLAN,
+					   vid & PORT_DEFAULT_VLAN_MASK);
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
+int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, u16 vid)
+{
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_vtu_entry entry = { 0 };
+	int i, ret, prev_vid = vid ? vid - 1 : 4095;
+	bool keep = false;
+
+	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
+	if (!vid)
+		return 0;
+
+	mutex_lock(&ps->smi_mutex);
+	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	if (entry.vid != vid || !entry.valid) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	entry.tags[port] = GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER;
+
+	/* keep the VLAN unless all ports are excluded */
+	for (i = 0; i < ps->num_ports; ++i) {
+		if (dsa_is_cpu_port(ds, i))
+			continue;
+
+		if (entry.tags[i] != GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER) {
+			keep = true;
+			break;
+		}
+	}
+
+	entry.valid = keep;
+	ret = _mv88e6xxx_vtu_loadpurge(ds, &entry);
+	if (ret < 0)
+		goto unlock;
+
+	/* TODO reset PVID if it was this vid? */
+
+	if (!keep)
+		ret = _mv88e6xxx_update_bridge_config(ds, entry.fid);
+unlock:
+	mutex_unlock(&ps->smi_mutex);
+
+	return ret;
+}
+
 static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index e10ccdb..42032c3 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -120,6 +120,7 @@
 #define PORT_CONTROL_1		0x05
 #define PORT_BASE_VLAN		0x06
 #define PORT_DEFAULT_VLAN	0x07
+#define PORT_DEFAULT_VLAN_MASK	0xfff
 #define PORT_CONTROL_2		0x08
 #define PORT_CONTROL_2_IGNORE_FCS	BIT(15)
 #define PORT_CONTROL_2_VTU_PRI_OVERRIDE	BIT(14)
@@ -160,6 +161,10 @@
 #define GLOBAL_MAC_01		0x01
 #define GLOBAL_MAC_23		0x02
 #define GLOBAL_MAC_45		0x03
+#define GLOBAL_VTU_FID		0x02 /* 6352 only? */
+#define GLOBAL_VTU_FID_MASK	0xfff
+#define GLOBAL_VTU_SID		0x03 /* 6352 only? */
+#define GLOBAL_VTU_SID_MASK	0x3f
 #define GLOBAL_CONTROL		0x04
 #define GLOBAL_CONTROL_SW_RESET		BIT(15)
 #define GLOBAL_CONTROL_PPU_ENABLE	BIT(14)
@@ -176,9 +181,20 @@
 #define GLOBAL_CONTROL_TCAM_EN		BIT(1)
 #define GLOBAL_CONTROL_EEPROM_DONE_EN	BIT(0)
 #define GLOBAL_VTU_OP		0x05
+#define GLOBAL_VTU_OP_BUSY	BIT(15)
+#define GLOBAL_VTU_OP_FLUSH_ALL		((1 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_LOAD_PURGE	((3 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_VTU_GET_NEXT	((4 << 12) | GLOBAL_VTU_OP_BUSY)
+#define GLOBAL_VTU_OP_STU_LOAD_PURGE	((5 << 12) | GLOBAL_VTU_OP_BUSY)
 #define GLOBAL_VTU_VID		0x06
+#define GLOBAL_VTU_VID_MASK	0xfff
+#define GLOBAL_VTU_VID_VALID	BIT(12)
 #define GLOBAL_VTU_DATA_0_3	0x07
 #define GLOBAL_VTU_DATA_4_7	0x08
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNMODIFIED	0
+#define GLOBAL_VTU_DATA_MEMBER_TAG_UNTAGGED	1
+#define GLOBAL_VTU_DATA_MEMBER_TAG_TAGGED	2
+#define GLOBAL_VTU_DATA_MEMBER_TAG_NON_MEMBER	3
 #define GLOBAL_VTU_DATA_8_11	0x09
 #define GLOBAL_ATU_CONTROL	0x0a
 #define GLOBAL_ATU_CONTROL_LEARN2ALL	BIT(3)
@@ -293,6 +309,14 @@
 #define GLOBAL2_QOS_WEIGHT	0x1c
 #define GLOBAL2_MISC		0x1d
 
+struct mv88e6xxx_vtu_entry {
+	u16	vid;
+	u16	fid;
+	u8	sid;
+	bool	valid;
+	u8	tags[DSA_MAX_PORTS];
+};
+
 struct mv88e6xxx_priv_state {
 	/* When using multi-chip addressing, this mutex protects
 	 * access to the indirect access registers.  (In single-chip
@@ -397,6 +421,9 @@ int mv88e6xxx_port_fdb_getnext(struct dsa_switch *ds, int port,
 int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg);
 int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
 			     int reg, int val);
+int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid,
+			    u16 bridge_flags);
+int mv88e6xxx_port_vlan_del(struct dsa_switch *ds,int port, u16 vid);
 extern struct dsa_switch_driver mv88e6131_switch_driver;
 extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
 extern struct dsa_switch_driver mv88e6352_switch_driver;
-- 
2.4.1

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

Vivien,

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This commit implements the port_vlan_add and port_vlan_del functions in
> the dsa_switch_driver structure for Marvell 88E6xxx compatible switches.
>
> This allows to access a switch VLAN Table Unit, and thus define VLANs
> from standard userspace commands such as "bridge vlan".
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---

[ ... ]

> +
> +int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid,
> +			    u16 bridge_flags)
> +{
> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> +	struct mv88e6xxx_vtu_entry entry = { 0 };
> +	int prev_vid = vid ? vid - 1 : 4095;
> +	int i, ret;
> +
> +	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
> +	if (!vid)
> +		return 0;
> +

Me puzzled ;-). I brought this and the fid question up before.
No idea if my e-mail got lost or what happened.

Can you explain why we don't need a configuration for vlan 0 ?

> +	/* The DSA port-based VLAN setup reserves FID 0 to DSA_MAX_PORTS;
> +	 * we will use the next FIDs for 802.1q;
> +	 * thus, forbid the last DSA_MAX_PORTS VLANs.
> +	 */
> +	if (vid > 4095 - DSA_MAX_PORTS)
> +		return -EINVAL;
> +
> +	mutex_lock(&ps->smi_mutex);
> +	ret = _mv88e6xxx_vtu_getnext(ds, prev_vid, &entry);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	/* If the VLAN does not exist, re-initialize the entry for addition */
> +	if (entry.vid != vid || !entry.valid) {
> +		memset(&entry, 0, sizeof(entry));
> +		entry.valid = true;
> +		entry.vid = vid;
> +		entry.fid = DSA_MAX_PORTS + vid;

I brought this up before. No idea if my e-mail got lost or what happened.

We use a fid per port, and a fid per bridge group. With VLANs, this is completely
ignored, ahd there is only a single fid per vlan for the entire switch.

Either per-port fids are unnecessary as well, or something is wrong here,
or I am missing something. Can you explain why we only need a single fid
per vlan, even if we have multiple bridge groups and the same vlan is
configured in all of them ?

Thanks,
Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Scott Feldman]

On Mon, Jun 1, 2015 at 11:50 PM, Guenter Roeck <linux@roeck-us.net> wrote:

[cut]

> I brought this up before. No idea if my e-mail got lost or what happened.
>
> We use a fid per port, and a fid per bridge group. With VLANs, this is
> completely
> ignored, ahd there is only a single fid per vlan for the entire switch.
>
> Either per-port fids are unnecessary as well, or something is wrong here,
> or I am missing something. Can you explain why we only need a single fid
> per vlan, even if we have multiple bridge groups and the same vlan is
> configured in all of them ?

That brings up an interesting point about having multiple bridges with
the same vlan configured.  I struggled with that problem with rocker
also and I don't have an answer other than "don't do that".  Or,
better put, if you have multiple bridge on the same vlan, just use one
bridge for that vlan.  Otherwise, I don't know how at the device level
to partition the vlan between the bridges.  Maybe that's what Vivien
is facing also?  I can see how this works for software-only bridges,
because they should be isolated from each other and independent.  But
when offloading to a device which sees VLAN XXX global across the
entire switch, I don't see how we can preserve the bridge boundaries.

I hope I'm not misunderstanding the issue here; if I am, I apologize.

-scott

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

On 06/02/2015 12:44 AM, Scott Feldman wrote:
> On Mon, Jun 1, 2015 at 11:50 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> [cut]
>
>> I brought this up before. No idea if my e-mail got lost or what happened.
>>
>> We use a fid per port, and a fid per bridge group. With VLANs, this is
>> completely
>> ignored, ahd there is only a single fid per vlan for the entire switch.
>>
>> Either per-port fids are unnecessary as well, or something is wrong here,
>> or I am missing something. Can you explain why we only need a single fid
>> per vlan, even if we have multiple bridge groups and the same vlan is
>> configured in all of them ?
>
> That brings up an interesting point about having multiple bridges with
> the same vlan configured.  I struggled with that problem with rocker
> also and I don't have an answer other than "don't do that".  Or,
> better put, if you have multiple bridge on the same vlan, just use one
> bridge for that vlan.  Otherwise, I don't know how at the device level
> to partition the vlan between the bridges.  Maybe that's what Vivien
> is facing also?  I can see how this works for software-only bridges,
> because they should be isolated from each other and independent.  But
> when offloading to a device which sees VLAN XXX global across the
> entire switch, I don't see how we can preserve the bridge boundaries.
>

One solution would be to use separate fids, like we do for non-vlan
bridges. That makes fid management more complex, sure, but it would work.
Otherwise we might have to explicitly disable support for more than one
bridge group on a switch, which seems a bit artificial to me.

Also, we already have cases where the switch is connected to the CPU with
more than one Ethernet port. It is easy to imagine that the user of such
a system might want to configure two bridge groups.

Thanks,
Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Andrew Lunn]

> Also, we already have cases where the switch is connected to the CPU with
> more than one Ethernet port. It is easy to imagine that the user of such
> a system might want to configure two bridge groups.
 
Hi Guenter

I think that is orthogonal.  Having multiple CPU ports should really
only be seen as increased throughput with load sharing. It makes no
different to the basic user use cases. They can all be done with a
single CPU port, but with less bandwidth.

       Andrew

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

On 06/02/2015 06:42 AM, Andrew Lunn wrote:
>> Also, we already have cases where the switch is connected to the CPU with
>> more than one Ethernet port. It is easy to imagine that the user of such
>> a system might want to configure two bridge groups.
>
> Hi Guenter
>
> I think that is orthogonal.  Having multiple CPU ports should really
> only be seen as increased throughput with load sharing. It makes no
> different to the basic user use cases. They can all be done with a
> single CPU port, but with less bandwidth.
>

Hi Andrew,

quite possibly, but for my part I usually try not to restrict use cases.
Some people may feel uncomfortable with load sharing and rather use
the separate cpu ports to connect to dedicated external ports on the switch.

Sure, that may reduce overall throughput, but it would provide a cleaner
separation of traffic and guarantee that each of the ports gets its full
bandwidth and is not starved by the other. Yes, I am sure that is all
configurable, but it adds more and more complexity for the user.

Thanks,
Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[nolan]

On 06/02/2015 12:44 AM, Scott Feldman wrote:
> That brings up an interesting point about having multiple bridges with
> the same vlan configured.  I struggled with that problem with rocker
> also and I don't have an answer other than "don't do that".  Or,
> better put, if you have multiple bridge on the same vlan, just use one
> bridge for that vlan.  Otherwise, I don't know how at the device level
> to partition the vlan between the bridges.  Maybe that's what Vivien
> is facing also?  I can see how this works for software-only bridges,
> because they should be isolated from each other and independent.  But
> when offloading to a device which sees VLAN XXX global across the
> entire switch, I don't see how we can preserve the bridge boundaries.

Scott,

I'm confused by this.  I think you're saying this config is problematic:

br0: eth0.100, eth1.100
br1: eth2.100, eth3.100

But this works fine today.

Could you clarify the issue you're referring to?

Thanks,
- nolan

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Scott Feldman]

On Tue, Jun 2, 2015 at 3:31 PM, nolan <nolan@cumulusnetworks.com> wrote:
> On 06/02/2015 12:44 AM, Scott Feldman wrote:
>>
>> That brings up an interesting point about having multiple bridges with
>> the same vlan configured.  I struggled with that problem with rocker
>> also and I don't have an answer other than "don't do that".  Or,
>> better put, if you have multiple bridge on the same vlan, just use one
>> bridge for that vlan.  Otherwise, I don't know how at the device level
>> to partition the vlan between the bridges.  Maybe that's what Vivien
>> is facing also?  I can see how this works for software-only bridges,
>> because they should be isolated from each other and independent.  But
>> when offloading to a device which sees VLAN XXX global across the
>> entire switch, I don't see how we can preserve the bridge boundaries.
>
>
> Scott,
>
> I'm confused by this.  I think you're saying this config is problematic:
>
> br0: eth0.100, eth1.100
> br1: eth2.100, eth3.100
>
>
> But this works fine today.

Ya, this is going to work because br0 and br1 are bridging untagged
traffic, but br0 and br1 have different internal VLAN ids for untagged
traffic, so there is no crosstalk between bridges.

(I'm assuming since you used the ethX.100 format, you've vconfig
created a vlan interface on ethX and added the vlan interface to brY).
The vlan interface eats the vlan tag and the bridge sees untagged
traffic.

> Could you clarify the issue you're referring to?

I'm talking about bridging tagged traffic.  E.g.:

ip link add name br0 type bridge
ip link add name br1 type bridge

ip link set dev sw1p1 master br0
ip link set dev sw1p2 master br0
ip link set dev sw1p3 master br1
ip link set dev sw1p4 master br1

bridge vlan add vid 100 dev sw1p1
bridge vlan add vid 100 dev sw1p2
bridge vlan add vid 100 dev sw1p3
bridge vlan add vid 100 dev sw1p4

Now the ports are trunking vlan 100 and the bridge/device see tagged
traffic.  If the device used floods vlan 100 pkt to all ports in vlan
100, it'll flood to a port outside the bridge.  Oops!  For the device
I'm using (rocker w/ OF-DPA) the bridging table matches on vlan ID and
mac_dst.  There is no prevision to isolate vlans per bridge.

How do you solve the above case with your hardware?

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

On 06/02/2015 11:53 PM, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 3:31 PM, nolan <nolan@cumulusnetworks.com> wrote:
>> On 06/02/2015 12:44 AM, Scott Feldman wrote:
>>>
>>> That brings up an interesting point about having multiple bridges with
>>> the same vlan configured.  I struggled with that problem with rocker
>>> also and I don't have an answer other than "don't do that".  Or,
>>> better put, if you have multiple bridge on the same vlan, just use one
>>> bridge for that vlan.  Otherwise, I don't know how at the device level
>>> to partition the vlan between the bridges.  Maybe that's what Vivien
>>> is facing also?  I can see how this works for software-only bridges,
>>> because they should be isolated from each other and independent.  But
>>> when offloading to a device which sees VLAN XXX global across the
>>> entire switch, I don't see how we can preserve the bridge boundaries.
>>
>>
>> Scott,
>>
>> I'm confused by this.  I think you're saying this config is problematic:
>>
>> br0: eth0.100, eth1.100
>> br1: eth2.100, eth3.100
>>
>>
>> But this works fine today.
>
> Ya, this is going to work because br0 and br1 are bridging untagged
> traffic, but br0 and br1 have different internal VLAN ids for untagged
> traffic, so there is no crosstalk between bridges.
>
> (I'm assuming since you used the ethX.100 format, you've vconfig
> created a vlan interface on ethX and added the vlan interface to brY).
> The vlan interface eats the vlan tag and the bridge sees untagged
> traffic.
>
>> Could you clarify the issue you're referring to?
>
> I'm talking about bridging tagged traffic.  E.g.:
>
> ip link add name br0 type bridge
> ip link add name br1 type bridge
>
> ip link set dev sw1p1 master br0
> ip link set dev sw1p2 master br0
> ip link set dev sw1p3 master br1
> ip link set dev sw1p4 master br1
>
> bridge vlan add vid 100 dev sw1p1
> bridge vlan add vid 100 dev sw1p2
> bridge vlan add vid 100 dev sw1p3
> bridge vlan add vid 100 dev sw1p4
>
> Now the ports are trunking vlan 100 and the bridge/device see tagged
> traffic.  If the device used floods vlan 100 pkt to all ports in vlan
> 100, it'll flood to a port outside the bridge.  Oops!  For the device
> I'm using (rocker w/ OF-DPA) the bridging table matches on vlan ID and
> mac_dst.  There is no prevision to isolate vlans per bridge.
>
> How do you solve the above case with your hardware?
>

We could use separate FIDs per vlan/bridge group, ie don't assume
that fid == vid.

A simple solution might be to just set the fid to the fid used by
the bridge. That would not support 802.1s, though.

Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Florian Fainelli]

On 02/06/15 23:53, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 3:31 PM, nolan <nolan@cumulusnetworks.com> wrote:
>> On 06/02/2015 12:44 AM, Scott Feldman wrote:
>>>
>>> That brings up an interesting point about having multiple bridges with
>>> the same vlan configured.  I struggled with that problem with rocker
>>> also and I don't have an answer other than "don't do that".  Or,
>>> better put, if you have multiple bridge on the same vlan, just use one
>>> bridge for that vlan.  Otherwise, I don't know how at the device level
>>> to partition the vlan between the bridges.  Maybe that's what Vivien
>>> is facing also?  I can see how this works for software-only bridges,
>>> because they should be isolated from each other and independent.  But
>>> when offloading to a device which sees VLAN XXX global across the
>>> entire switch, I don't see how we can preserve the bridge boundaries.
>>
>>
>> Scott,
>>
>> I'm confused by this.  I think you're saying this config is problematic:
>>
>> br0: eth0.100, eth1.100
>> br1: eth2.100, eth3.100
>>
>>
>> But this works fine today.
> 
> Ya, this is going to work because br0 and br1 are bridging untagged
> traffic, but br0 and br1 have different internal VLAN ids for untagged
> traffic, so there is no crosstalk between bridges.
> 
> (I'm assuming since you used the ethX.100 format, you've vconfig
> created a vlan interface on ethX and added the vlan interface to brY).
> The vlan interface eats the vlan tag and the bridge sees untagged
> traffic.
> 
>> Could you clarify the issue you're referring to?
> 
> I'm talking about bridging tagged traffic.  E.g.:
> 
> ip link add name br0 type bridge
> ip link add name br1 type bridge
> 
> ip link set dev sw1p1 master br0
> ip link set dev sw1p2 master br0
> ip link set dev sw1p3 master br1
> ip link set dev sw1p4 master br1
> 
> bridge vlan add vid 100 dev sw1p1
> bridge vlan add vid 100 dev sw1p2
> bridge vlan add vid 100 dev sw1p3
> bridge vlan add vid 100 dev sw1p4
> 
> Now the ports are trunking vlan 100 and the bridge/device see tagged
> traffic.  If the device used floods vlan 100 pkt to all ports in vlan
> 100, it'll flood to a port outside the bridge.  Oops!  For the device
> I'm using (rocker w/ OF-DPA) the bridging table matches on vlan ID and
> mac_dst.  There is no prevision to isolate vlans per bridge.
> 
> How do you solve the above case with your hardware?

For switches that support double-tagging, I suppose you could utilize
the fact that packets need to be normalized to include an internal outer
tag as well (for both tagged and untagged packets), and utilize that to
make sure there is no cross-talk. That way, you can have the same inner
VLAN tags on both of your bridges but the internal outer tag can be
different.

There might be a better solution, like having proper partitioning based
on whatever the switch is capable of?
-- 
Florian

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[nolan]

On 06/02/2015 11:53 PM, Scott Feldman wrote:
> I'm talking about bridging tagged traffic.  E.g.:
>
> ip link add name br0 type bridge
> ip link add name br1 type bridge
>
> ip link set dev sw1p1 master br0
> ip link set dev sw1p2 master br0
> ip link set dev sw1p3 master br1
> ip link set dev sw1p4 master br1
>
> bridge vlan add vid 100 dev sw1p1
> bridge vlan add vid 100 dev sw1p2
> bridge vlan add vid 100 dev sw1p3
> bridge vlan add vid 100 dev sw1p4
>
> Now the ports are trunking vlan 100 and the bridge/device see tagged
> traffic.  If the device used floods vlan 100 pkt to all ports in vlan
> 100, it'll flood to a port outside the bridge.  Oops!  For the device
> I'm using (rocker w/ OF-DPA) the bridging table matches on vlan ID and
> mac_dst.  There is no prevision to isolate vlans per bridge.
>
> How do you solve the above case with your hardware?

Ah, understood, thanks for clarifying.

Right now, we only support this case with vconfig-style vlans.  I don't 
see any conceptual reason it couldn't be supported in the vlan-aware 
bridge model as well.  The HW configuration would be essentially 
identical to the equivalent vconfig-style config.

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Vivien Didelot]

Guenter,

On Jun 2, 2015, at 2:50 AM, Guenter Roeck linux@roeck-us.net wrote:
> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> +    /* Bringing an interface up adds it to the VLAN 0. Ignore this. */
>> +    if (!vid)
>> +        return 0;
>> +
> 
> Me puzzled ;-). I brought this and the fid question up before.
> No idea if my e-mail got lost or what happened.
> 
> Can you explain why we don't need a configuration for vlan 0 ?

Sorry for late reply. Initially, when issuing "ip link set up dev swp0",
ndo_vlan_rx_add_vid was called to add the interface in the VLAN 0.

2 things happen here:

  * this is inconsistent with the "bridge vlan" output which doesn't seem to
    know about a VID 0;
  * VID 0 seems special for this switch: if an ingressing frame has VID 0, the
    tagged port will override the VID bits with the port default VID at egress.

Thanks,
-v

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

On Tue, Jun 02, 2015 at 09:39:50PM -0400, Vivien Didelot wrote:
> Guenter,
> 
> On Jun 2, 2015, at 2:50 AM, Guenter Roeck linux@roeck-us.net wrote:
> > On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> >> +    /* Bringing an interface up adds it to the VLAN 0. Ignore this. */
> >> +    if (!vid)
> >> +        return 0;
> >> +
> > 
> > Me puzzled ;-). I brought this and the fid question up before.
> > No idea if my e-mail got lost or what happened.
> > 
> > Can you explain why we don't need a configuration for vlan 0 ?
> 
> Sorry for late reply. Initially, when issuing "ip link set up dev swp0",
> ndo_vlan_rx_add_vid was called to add the interface in the VLAN 0.
> 
Loading the 802.1q module has the same effect.

I think this may be on purpose; it is telling the switch to accept
packets with vid==0 (and untagged packets).

> 2 things happen here:
> 
>   * this is inconsistent with the "bridge vlan" output which doesn't seem to
>     know about a VID 0;
>   * VID 0 seems special for this switch: if an ingressing frame has VID 0, the
>     tagged port will override the VID bits with the port default VID at egress.
> 
As far as I can see, the switch treats packets with vid==0 and untaged packets
as unknown if VLAN support is enabled.

Anyway, sounds odd. Sure this isn't a configuration problem somethere ?

Thanks,
Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 10:17 PM, Guenter Roeck linux@roeck-us.net wrote:
> On Tue, Jun 02, 2015 at 09:39:50PM -0400, Vivien Didelot wrote:
>> Guenter,
>> 
>> On Jun 2, 2015, at 2:50 AM, Guenter Roeck linux@roeck-us.net wrote:
>> > On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> >> +    /* Bringing an interface up adds it to the VLAN 0. Ignore this. */
>> >> +    if (!vid)
>> >> +        return 0;
>> >> +
>> > 
>> > Me puzzled ;-). I brought this and the fid question up before.
>> > No idea if my e-mail got lost or what happened.
>> > 
>> > Can you explain why we don't need a configuration for vlan 0 ?
>> 
>> Sorry for late reply. Initially, when issuing "ip link set up dev swp0",
>> ndo_vlan_rx_add_vid was called to add the interface in the VLAN 0.
>> 
> Loading the 802.1q module has the same effect.
> 
> I think this may be on purpose; it is telling the switch to accept
> packets with vid==0 (and untagged packets).
> 
>> 2 things happen here:
>> 
>>   * this is inconsistent with the "bridge vlan" output which doesn't seem to
>>     know about a VID 0;
>>   * VID 0 seems special for this switch: if an ingressing frame has VID 0, the
>>     tagged port will override the VID bits with the port default VID at egress.
>> 
> As far as I can see, the switch treats packets with vid==0 and untaged packets
> as unknown if VLAN support is enabled.

I am not sure about the untagged frames. But for tagged frames, the
documentation says that frames with vid 0 will be overridden with the port's
default VID.

> Anyway, sounds odd. Sure this isn't a configuration problem somethere ?

If I'm not mistaken, other drivers do that. e.g. Rocker deals with VID >= 1:

    for (vid = 1; vid < VLAN_N_VID; vid++)

Maybe this VID overriding feature is what we want? But it doesn't look right to
me, even more since it is not exposed to the user.

Thanks,
-v

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Guenter Roeck]

On 06/03/2015 07:56 AM, Vivien Didelot wrote:
> Hi Guenter,
>
> On Jun 2, 2015, at 10:17 PM, Guenter Roeck linux@roeck-us.net wrote:
>> On Tue, Jun 02, 2015 at 09:39:50PM -0400, Vivien Didelot wrote:
>>> Guenter,
>>>
>>> On Jun 2, 2015, at 2:50 AM, Guenter Roeck linux@roeck-us.net wrote:
>>>> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>>>>> +    /* Bringing an interface up adds it to the VLAN 0. Ignore this. */
>>>>> +    if (!vid)
>>>>> +        return 0;
>>>>> +
>>>>
>>>> Me puzzled ;-). I brought this and the fid question up before.
>>>> No idea if my e-mail got lost or what happened.
>>>>
>>>> Can you explain why we don't need a configuration for vlan 0 ?
>>>
>>> Sorry for late reply. Initially, when issuing "ip link set up dev swp0",
>>> ndo_vlan_rx_add_vid was called to add the interface in the VLAN 0.
>>>
>> Loading the 802.1q module has the same effect.
>>
>> I think this may be on purpose; it is telling the switch to accept
>> packets with vid==0 (and untagged packets).
>>
>>> 2 things happen here:
>>>
>>>    * this is inconsistent with the "bridge vlan" output which doesn't seem to
>>>      know about a VID 0;
>>>    * VID 0 seems special for this switch: if an ingressing frame has VID 0, the
>>>      tagged port will override the VID bits with the port default VID at egress.
>>>
>> As far as I can see, the switch treats packets with vid==0 and untaged packets
>> as unknown if VLAN support is enabled.
>
> I am not sure about the untagged frames. But for tagged frames, the
> documentation says that frames with vid 0 will be overridden with the port's
> default VID.
>
The documentation says that untagged frames get the port's default VID assigned.

>> Anyway, sounds odd. Sure this isn't a configuration problem somethere ?
>
> If I'm not mistaken, other drivers do that. e.g. Rocker deals with VID >= 1:
>
>      for (vid = 1; vid < VLAN_N_VID; vid++)
>

Yes, but that won't help us. Again, the problem is that the switch drops untagged
packets if VLAN mode is set to secure and there is no VID table entry for VID 0
(or, rather, the port's default VID, which happens to be 0 in our case).
We'll have to solve that problem somehow. Using fallback mode isn't really a good
solution since it still treats untagged packets (and priority tagged packets
with vid==0) as membership violations.

It might make sense to set all ports to secure mode, but then we would always have
to create a VID table entry for vid=0 (or vid=default vid). Maybe we should just
do that, and/or keep 802.1q support on a port disabled unless it is explicitly
enabled by some means (such as adding an entry to the port's VLAN filter table).

Since we actually set the default VLAN to 0 in mv88e6xxx_setup_port_common(),
I wonder if you actually _see_ a problem with replaced VIDs, or if you just
assume that there would be one. Can you clarify ?

Thanks,
Guenter

Re: [RFC 3/9] net: dsa: mv88e6xxx: add support for VTU ops

[Andrew Lunn]

> +int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, u16 vid,
> +			    u16 bridge_flags)
> +{
> +	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
> +	struct mv88e6xxx_vtu_entry entry = { 0 };
> +	int prev_vid = vid ? vid - 1 : 4095;
> +	int i, ret;
> +
> +	/* Bringing an interface up adds it to the VLAN 0. Ignore this. */
> +	if (!vid)
> +		return 0;
> +
> +	/* The DSA port-based VLAN setup reserves FID 0 to DSA_MAX_PORTS;
> +	 * we will use the next FIDs for 802.1q;
> +	 * thus, forbid the last DSA_MAX_PORTS VLANs.
> +	 */
> +	if (vid > 4095 - DSA_MAX_PORTS)
> +		return -EINVAL;

I'm not sure about this. I've setup systems where i've used VLANs from
the top down to differentiate them from other VLANs going from the
bottom up. We might want to dynamically assign FIDs to VLANs rather
than have a static mapping.

     Andrew

[RFC 4/9] net: dsa: mv88e6352: add support for VLAN

[Vivien Didelot]

This commit adds support for the VTU operations to the mv88e6352 driver.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6352.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index b524bd3..838494a 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -397,6 +397,8 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.fdb_add		= mv88e6xxx_port_fdb_add,
 	.fdb_del		= mv88e6xxx_port_fdb_del,
 	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
+	.port_vlan_add		= mv88e6xxx_port_vlan_add,
+	.port_vlan_del		= mv88e6xxx_port_vlan_del,
 };
 
 MODULE_ALIAS("platform:mv88e6352");
-- 
2.4.1

[RFC 5/9] net: dsa: mv88e6352: disable mirroring

[Vivien Didelot]

Disable the mirroring policy in the monitor control register, since this
feature is not needed.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6352.c | 10 +++-------
 drivers/net/dsa/mv88e6xxx.h |  1 +
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 838494a..f541362 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -63,13 +63,9 @@ static int mv88e6352_setup_global(struct dsa_switch *ds)
 	REG_WRITE(REG_GLOBAL, GLOBAL_CONTROL,
 		  GLOBAL_CONTROL_PPU_ENABLE | GLOBAL_CONTROL_DISCARD_EXCESS);
 
-	/* Configure the upstream port, and configure the upstream
-	 * port as the port to which ingress and egress monitor frames
-	 * are to be sent.
-	 */
-	reg = upstream_port << GLOBAL_MONITOR_CONTROL_INGRESS_SHIFT |
-		upstream_port << GLOBAL_MONITOR_CONTROL_EGRESS_SHIFT |
-		upstream_port << GLOBAL_MONITOR_CONTROL_ARP_SHIFT;
+	/* Configure the upstream port, and disable policy mirroring. */
+	reg = upstream_port << GLOBAL_MONITOR_CONTROL_ARP_SHIFT |
+		GLOBAL_MONITOR_CONTROL_MIRROR_DISABLED;
 	REG_WRITE(REG_GLOBAL, GLOBAL_MONITOR_CONTROL, reg);
 
 	/* Disable remote management for now, and set the switch's
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 42032c3..f4ea66a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -237,6 +237,7 @@
 #define GLOBAL_MONITOR_CONTROL_ARP_SHIFT	4
 #define GLOBAL_MONITOR_CONTROL_MIRROR_SHIFT	0
 #define GLOBAL_MONITOR_CONTROL_ARP_DISABLED	(0xf0)
+#define GLOBAL_MONITOR_CONTROL_MIRROR_DISABLED	0x0f
 #define GLOBAL_CONTROL_2	0x1c
 #define GLOBAL_CONTROL_2_NO_CASCADE		0xe000
 #define GLOBAL_CONTROL_2_MULTIPLE_CASCADE	0xf000
-- 
2.4.1

Re: [RFC 5/9] net: dsa: mv88e6352: disable mirroring

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> Disable the mirroring policy in the monitor control register, since this
> feature is not needed.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Should this be a separate patch, unrelated to the patch set ?

If I understand correctly, this effectively disables IGMP/MLD snooping.
I think this warrants an explanation why that it not needed, not just
a statement that it is not needed.

Thanks,
Guenter

Re: [RFC 5/9] net: dsa: mv88e6352: disable mirroring

[Andrew Lunn]

On Tue, Jun 02, 2015 at 07:16:10AM -0700, Guenter Roeck wrote:
> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> >Disable the mirroring policy in the monitor control register, since this
> >feature is not needed.
> >
> >Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
> Should this be a separate patch, unrelated to the patch set ?
> 
> If I understand correctly, this effectively disables IGMP/MLD snooping.
> I think this warrants an explanation why that it not needed, not just
> a statement that it is not needed.

+1

Especially since we might want to revisit this to implement IGMP/MLD
snooping in the bridge. The hardware should be capable of it.

	 Andrew

Re: [RFC 5/9] net: dsa: mv88e6352: disable mirroring

[Vivien Didelot]

Hi Guenter, Andrew,

On Jun 2, 2015, at 10:53 AM, Andrew Lunn andrew@lunn.ch wrote:
On Tue, Jun 02, 2015 at 07:16:10AM -0700, Guenter Roeck wrote:
>> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> >Disable the mirroring policy in the monitor control register, since this
>> >feature is not needed.
>> >
>> >Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> 
>> Should this be a separate patch, unrelated to the patch set ?

Indeed, this one is an unrelated patch, sorry.

>> If I understand correctly, this effectively disables IGMP/MLD snooping.
>> I think this warrants an explanation why that it not needed, not just
>> a statement that it is not needed.
> 
> +1
> 
> Especially since we might want to revisit this to implement IGMP/MLD
> snooping in the bridge. The hardware should be capable of it.

This is something I want to disable because I can have several times
gigabit traffic on my ports. This would end up in a bottleneck on the
CPU port. Am I right?

Thanks,
-v

Re: [RFC 5/9] net: dsa: mv88e6352: disable mirroring

[Guenter Roeck]

On Tue, Jun 02, 2015 at 09:12:30PM -0400, Vivien Didelot wrote:
> Hi Guenter, Andrew,
> 
> On Jun 2, 2015, at 10:53 AM, Andrew Lunn andrew@lunn.ch wrote:
> On Tue, Jun 02, 2015 at 07:16:10AM -0700, Guenter Roeck wrote:
> >> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> >> >Disable the mirroring policy in the monitor control register, since this
> >> >feature is not needed.
> >> >
> >> >Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> >> 
> >> Should this be a separate patch, unrelated to the patch set ?
> 
> Indeed, this one is an unrelated patch, sorry.
> 
> >> If I understand correctly, this effectively disables IGMP/MLD snooping.
> >> I think this warrants an explanation why that it not needed, not just
> >> a statement that it is not needed.
> > 
> > +1
> > 
> > Especially since we might want to revisit this to implement IGMP/MLD
> > snooping in the bridge. The hardware should be capable of it.
> 
> This is something I want to disable because I can have several times
> gigabit traffic on my ports. This would end up in a bottleneck on the
> CPU port. Am I right?
> 
Not really. That should not be that much traffic. Besides, IGMP/MLD snooping
still needs to be enabled separately, as well as egress monitoring.

I don't think this has any impact on the traffic to the CPU port unless other
configuration bits are set as well.

Guenter

[RFC 6/9] net: dsa: mv88e6352: allow egress of unknown multicast

[Vivien Didelot]

This patch disables egress of unknown unicast destination addresses.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 3 ++-
 drivers/net/dsa/mv88e6xxx.h | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 49ef2f8..d2beb10 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1686,7 +1686,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	    mv88e6xxx_6185_family(ds))
 		reg = PORT_CONTROL_IGMP_MLD_SNOOP |
 		PORT_CONTROL_USE_TAG | PORT_CONTROL_USE_IP |
-		PORT_CONTROL_STATE_FORWARDING;
+		PORT_CONTROL_STATE_FORWARDING |
+		PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_UNICAST_DA;
 	if (dsa_is_cpu_port(ds, port)) {
 		if (mv88e6xxx_6095_family(ds) || mv88e6xxx_6185_family(ds))
 			reg |= PORT_CONTROL_DSA_TAG;
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index f4ea66a..412d14e 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -117,6 +117,11 @@
 #define PORT_CONTROL_STATE_BLOCKING	0x01
 #define PORT_CONTROL_STATE_LEARNING	0x02
 #define PORT_CONTROL_STATE_FORWARDING	0x03
+#define PORT_CONTROL_EGRESS_FLOODS_MASK				(0x03 << 2)
+#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_ANY_DA		(0x00 << 2)
+#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_MULTICAST_DA	(0x01 << 2)
+#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_UNICAST_DA	(0x02 << 2)
+#define PORT_CONTROL_EGRESS_FLOODS_ANY_DA			(0x03 << 2)
 #define PORT_CONTROL_1		0x05
 #define PORT_BASE_VLAN		0x06
 #define PORT_DEFAULT_VLAN	0x07
-- 
2.4.1

Re: [RFC 6/9] net: dsa: mv88e6352: allow egress of unknown multicast

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This patch disables egress of unknown unicast destination addresses.
>

Hi Vivien,

seems to me this patch is unrelated to the rest of the series.

Not sure if we really want this. If an address is in the arp cache
but has timed out from the bridge database, any unicast to that address
will no longer be sent. If the bridge database has been flushed for some
reason, such as a spanning tree reconfiguration, we'll have a hard time
to send anything.

What is the problem you are trying to solve with this patch ?

Thanks,
Guenter

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 3 ++-
>   drivers/net/dsa/mv88e6xxx.h | 5 +++++
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index 49ef2f8..d2beb10 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1686,7 +1686,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   	    mv88e6xxx_6185_family(ds))
>   		reg = PORT_CONTROL_IGMP_MLD_SNOOP |
>   		PORT_CONTROL_USE_TAG | PORT_CONTROL_USE_IP |
> -		PORT_CONTROL_STATE_FORWARDING;
> +		PORT_CONTROL_STATE_FORWARDING |
> +		PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_UNICAST_DA;
>   	if (dsa_is_cpu_port(ds, port)) {
>   		if (mv88e6xxx_6095_family(ds) || mv88e6xxx_6185_family(ds))
>   			reg |= PORT_CONTROL_DSA_TAG;
> diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
> index f4ea66a..412d14e 100644
> --- a/drivers/net/dsa/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx.h
> @@ -117,6 +117,11 @@
>   #define PORT_CONTROL_STATE_BLOCKING	0x01
>   #define PORT_CONTROL_STATE_LEARNING	0x02
>   #define PORT_CONTROL_STATE_FORWARDING	0x03
> +#define PORT_CONTROL_EGRESS_FLOODS_MASK				(0x03 << 2)
> +#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_ANY_DA		(0x00 << 2)
> +#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_MULTICAST_DA	(0x01 << 2)
> +#define PORT_CONTROL_EGRESS_FLOODS_NO_UNKNOWN_UNICAST_DA	(0x02 << 2)
> +#define PORT_CONTROL_EGRESS_FLOODS_ANY_DA			(0x03 << 2)
>   #define PORT_CONTROL_1		0x05
>   #define PORT_BASE_VLAN		0x06
>   #define PORT_DEFAULT_VLAN	0x07
>

Re: [RFC 6/9] net: dsa: mv88e6352: allow egress of unknown multicast

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 10:20 AM, Guenter Roeck linux@roeck-us.net wrote:
> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This patch disables egress of unknown unicast destination addresses.
>>
> 
> Hi Vivien,
> 
> seems to me this patch is unrelated to the rest of the series.
> 
> Not sure if we really want this. If an address is in the arp cache
> but has timed out from the bridge database, any unicast to that address
> will no longer be sent. If the bridge database has been flushed for some
> reason, such as a spanning tree reconfiguration, we'll have a hard time
> to send anything.
> 
> What is the problem you are trying to solve with this patch ?

TBH, I don't remember which one of the test cases I described in 0/9
this patch was solving... Some ARP request didn't propagate correctly
without this, IIRC.

I'll try to revert the change and do my tests again in order to isolate
the problem.

Thanks,
-v

Re: [RFC 6/9] net: dsa: mv88e6352: allow egress of unknown multicast

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 9:52 PM, Vivien Didelot vivien.didelot@savoirfairelinux.com wrote:
> On Jun 2, 2015, at 10:20 AM, Guenter Roeck linux@roeck-us.net wrote:
>> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>>> This patch disables egress of unknown unicast destination addresses.
>>>
>> 
>> Hi Vivien,
>> 
>> seems to me this patch is unrelated to the rest of the series.
>> 
>> Not sure if we really want this. If an address is in the arp cache
>> but has timed out from the bridge database, any unicast to that address
>> will no longer be sent. If the bridge database has been flushed for some
>> reason, such as a spanning tree reconfiguration, we'll have a hard time
>> to send anything.
>> 
>> What is the problem you are trying to solve with this patch ?
> 
> TBH, I don't remember which one of the test cases I described in 0/9
> this patch was solving... Some ARP request didn't propagate correctly
> without this, IIRC.
> 
> I'll try to revert the change and do my tests again in order to isolate
> the problem.

Indeed, it seems like this patch is not necessary. I removed it and I
was still able to ping a tagged and untagged port from an untagged one.

I will remove it from this serie.

Thanks,
-v

[RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Vivien Didelot]

This commit disables SA learning and refreshing for the CPU port.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 8 +++++---
 drivers/net/dsa/mv88e6xxx.h | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index d2beb10..ed49bd8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1761,10 +1761,12 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	/* Port Association Vector: when learning source addresses
 	 * of packets, add the address to the address database using
 	 * a port bitmap that has only the bit for this port set and
-	 * the other bits clear.
+	 * the other bits clear, except for the CPU port.
 	 */
-	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR,
-				   1 << port);
+	reg = BIT(port);
+	if (dsa_is_cpu_port(ds, port))
+		reg |= PORT_ASSOC_VECTOR_LOCKED_PORT;
+	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR, reg);
 	if (ret)
 		goto abort;
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 412d14e..e26eb0c 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -144,6 +144,7 @@
 #define PORT_RATE_CONTROL	0x09
 #define PORT_RATE_CONTROL_2	0x0a
 #define PORT_ASSOC_VECTOR	0x0b
+#define PORT_ASSOC_VECTOR_LOCKED_PORT	BIT(13)
 #define PORT_ATU_CONTROL	0x0c
 #define PORT_PRI_OVERRIDE	0x0d
 #define PORT_ETH_TYPE		0x0f
-- 
2.4.1

Re: [RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This commit disables SA learning and refreshing for the CPU port.
>

Hi Vivien,

This patch also seems to be unrelated to the rest of the series.

Can you add an explanation why it is needed ?

With this in place, how does the CPU port SA find its way into the fdb ?
Do we assume that it will be configured statically ?
An explanation might be useful.

Thanks,
Guenter

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 8 +++++---
>   drivers/net/dsa/mv88e6xxx.h | 1 +
>   2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index d2beb10..ed49bd8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1761,10 +1761,12 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   	/* Port Association Vector: when learning source addresses
>   	 * of packets, add the address to the address database using
>   	 * a port bitmap that has only the bit for this port set and
> -	 * the other bits clear.
> +	 * the other bits clear, except for the CPU port.
>   	 */
> -	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR,
> -				   1 << port);
> +	reg = BIT(port);
> +	if (dsa_is_cpu_port(ds, port))
> +		reg |= PORT_ASSOC_VECTOR_LOCKED_PORT;
> +	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR, reg);
>   	if (ret)
>   		goto abort;
>
> diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
> index 412d14e..e26eb0c 100644
> --- a/drivers/net/dsa/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx.h
> @@ -144,6 +144,7 @@
>   #define PORT_RATE_CONTROL	0x09
>   #define PORT_RATE_CONTROL_2	0x0a
>   #define PORT_ASSOC_VECTOR	0x0b
> +#define PORT_ASSOC_VECTOR_LOCKED_PORT	BIT(13)
>   #define PORT_ATU_CONTROL	0x0c
>   #define PORT_PRI_OVERRIDE	0x0d
>   #define PORT_ETH_TYPE		0x0f
>

Re: [RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net wrote:
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This commit disables SA learning and refreshing for the CPU port.
>>
> 
> Hi Vivien,
> 
> This patch also seems to be unrelated to the rest of the series.
> 
> Can you add an explanation why it is needed ?
> 
> With this in place, how does the CPU port SA find its way into the fdb ?
> Do we assume that it will be configured statically ?
> An explanation might be useful.

Without this patch, I noticed the CPU port was stealing the SA of a PC
behind a switch port. this happened when the port was a bridge member,
as the bridge was relaying broadcast coming from one switch port to the
other switch ports in the same vlan.

Thanks,
-v

Re: [RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Guenter Roeck]

On Tue, Jun 02, 2015 at 09:06:15PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> 
> On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net wrote:
> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> >> This commit disables SA learning and refreshing for the CPU port.
> >>
> > 
> > Hi Vivien,
> > 
> > This patch also seems to be unrelated to the rest of the series.
> > 
> > Can you add an explanation why it is needed ?
> > 
> > With this in place, how does the CPU port SA find its way into the fdb ?
> > Do we assume that it will be configured statically ?
> > An explanation might be useful.
> 
> Without this patch, I noticed the CPU port was stealing the SA of a PC
> behind a switch port. this happened when the port was a bridge member,
> as the bridge was relaying broadcast coming from one switch port to the
> other switch ports in the same vlan.
> 
Makes me feel really uncomfortable. I think we may be going into the wrong
direction. The whole point of offloading bridging is to have the switch handle
forwarding, and that includes multicasts and broadcasts. Instead of doing that,
it looks like we put more and more workarounds in place.

Maybe the software bridge code needs to understand that it isn't support to
forward broadcasts to ports of an offloaded bridge, and we should let the
switch chip handle it ?

Thanks,
Guenter

Re: [RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Guenter Roeck]

On 06/02/2015 07:31 PM, Chris Healy wrote:
> Guenter,
>
> That's a very valid concern.  I have a configuration with a 6352 controlled by a low end ARM core with a 100mbps connection on the CPU port.  This switch needs to support passing multicast streams that are more than 100mbps on GigE links.  (The ARM does not need to consume the multicast, it just manages the switch.)
>

Possibly, but Vivien didn't answer my question (how the local SA address finds
its way into the switch fdb). I'll check it myself.

Thanks,
Guenter

> On Jun 3, 2015 3:24 AM, "Guenter Roeck" <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
>
>     On Tue, Jun 02, 2015 at 09:06:15PM -0400, Vivien Didelot wrote:
>      > Hi Guenter,
>      >
>      > On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net <mailto:linux@roeck-us.net> wrote:
>      > On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>      > >> This commit disables SA learning and refreshing for the CPU port.
>      > >>
>      > >
>      > > Hi Vivien,
>      > >
>      > > This patch also seems to be unrelated to the rest of the series.
>      > >
>      > > Can you add an explanation why it is needed ?
>      > >
>      > > With this in place, how does the CPU port SA find its way into the fdb ?
>      > > Do we assume that it will be configured statically ?
>      > > An explanation might be useful.
>      >
>      > Without this patch, I noticed the CPU port was stealing the SA of a PC
>      > behind a switch port. this happened when the port was a bridge member,
>      > as the bridge was relaying broadcast coming from one switch port to the
>      > other switch ports in the same vlan.
>      >
>     Makes me feel really uncomfortable. I think we may be going into the wrong
>     direction. The whole point of offloading bridging is to have the switch handle
>     forwarding, and that includes multicasts and broadcasts. Instead of doing that,
>     it looks like we put more and more workarounds in place.
>
>     Maybe the software bridge code needs to understand that it isn't support to
>     forward broadcasts to ports of an offloaded bridge, and we should let the
>     switch chip handle it ?
>
>     Thanks,
>     Guenter
>

Re: [RFC 7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

[Andrew Lunn]

On Tue, Jun 02, 2015 at 07:31:56PM -0700, Chris Healy wrote:
> Guenter,
> 
> That's a very valid concern.  I have a configuration with a 6352 controlled
> by a low end ARM core with a 100mbps connection on the CPU port.  This
> switch needs to support passing multicast streams that are more than
> 100mbps on GigE links.  (The ARM does not need to consume the multicast, it
> just manages the switch.)

Hi Chris

Thinking out load here...

There are two use cases:

1) Without bridging. The switch ports are seen as host interfaces.
   Host interfaces are expected to accept packets for there own MAC
   address and the broadcast address. Additional multicast addresses
   can be added and the ndo_set_rx_mode() method of the driver is
   called to get to hardware to accept these MAC addresses. DSA has an
   implementation of ndo_set_rx_mode(), but all it does is ask the
   kernel to do the filtering. We need to extend it to program the
   hardware to only pass frames which match the addresses on the
   lists.  This should be just adding some static forwarding
   entries. Then, so long as an application on the host does not join
   any of the multicast groups, the frames should never be passed to
   the host.

2) With bridging, things are a bit different. Interfaces in a bridge
   are expected to be in promiscuous mode, receiving everything and
   passing it to the bridge. With the hardware bridging support
   Guenter added, we can off load unicast forwarding to the hardware.
   However, we currently don't have full support for off-loading of
   multicast. This falls into at a few different pieces:

   a) Get the hardware to do a dumb flood to all ports in the bridge
      group. However, the host is a member of the bridge, so it will
      still get a copy of all the packets. It has to, there could be
      members of the multicast groups on interfaces not accelerated by
      the hardware.

   b) Add limited IGMP snooping, so that the host bridge knows if it
      needs to see multicast frames for a specific MAC address from
      DSA interfaces or not, and program this into the hardware to
      reduce the load on the host.

   c) Add full IGMP snooping, so that the hardware no longer performs
      dumb flooding, but only forwards out ports where there has been
      an interest in the frames.

   Until we get at least b) implemented, i would expect all multicast
   packets to hit the host. In order to be correct in the general
   case, they have to.

   Andrew

[RFC 8/9] net: dsa: mv88e6352: set port 802.1Q mode to Secure

[Vivien Didelot]

This commit changes the 802.1Q mode of each port from Disabled to
Secure. This enables the VLAN support, by checking the VTU entries on
ingress.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 14 +++++++-------
 drivers/net/dsa/mv88e6xxx.h |  5 +++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index ed49bd8..35243d8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1723,13 +1723,11 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 			goto abort;
 	}
 
-	/* Port Control 2: don't force a good FCS, set the maximum
-	 * frame size to 10240 bytes, don't let the switch add or
-	 * strip 802.1q tags, don't discard tagged or untagged frames
-	 * on this port, do a destination address lookup on all
-	 * received packets as usual, disable ARP mirroring and don't
-	 * send a copy of all transmitted/received frames on this port
-	 * to the CPU.
+	/* Port Control 2: don't force a good FCS, set the maximum frame size to
+	 * 10240 bytes, enable secure 802.1q tags, don't discard tagged or
+	 * untagged frames on this port, do a destination address lookup on all
+	 * received packets as usual, disable ARP mirroring and don't send a
+	 * copy of all transmitted/received frames on this port to the CPU.
 	 */
 	reg = 0;
 	if (mv88e6xxx_6352_family(ds) || mv88e6xxx_6351_family(ds) ||
@@ -1751,6 +1749,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 			reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
 	}
 
+	reg |= PORT_CONTROL_2_8021Q_SECURE;
+
 	if (reg) {
 		ret = _mv88e6xxx_reg_write(ds, REG_PORT(port),
 					   PORT_CONTROL_2, reg);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index e26eb0c..02528aa 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -134,6 +134,11 @@
 #define PORT_CONTROL_2_JUMBO_1522	(0x00 << 12)
 #define PORT_CONTROL_2_JUMBO_2048	(0x01 << 12)
 #define PORT_CONTROL_2_JUMBO_10240	(0x02 << 12)
+#define PORT_CONTROL_2_8021Q_MASK	(0x03 << 10)
+#define PORT_CONTROL_2_8021Q_DISABLED	(0x00 << 10)
+#define PORT_CONTROL_2_8021Q_FALLBACK	(0x01 << 10)
+#define PORT_CONTROL_2_8021Q_CHECK	(0x02 << 10)
+#define PORT_CONTROL_2_8021Q_SECURE	(0x03 << 10)
 #define PORT_CONTROL_2_DISCARD_TAGGED	BIT(9)
 #define PORT_CONTROL_2_DISCARD_UNTAGGED	BIT(8)
 #define PORT_CONTROL_2_MAP_DA		BIT(7)
-- 
2.4.1

Re: [RFC 8/9] net: dsa: mv88e6352: set port 802.1Q mode to Secure

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This commit changes the 802.1Q mode of each port from Disabled to
> Secure. This enables the VLAN support, by checking the VTU entries on
> ingress.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 14 +++++++-------
>   drivers/net/dsa/mv88e6xxx.h |  5 +++++
>   2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index ed49bd8..35243d8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1723,13 +1723,11 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   			goto abort;
>   	}
>
> -	/* Port Control 2: don't force a good FCS, set the maximum
> -	 * frame size to 10240 bytes, don't let the switch add or
> -	 * strip 802.1q tags, don't discard tagged or untagged frames
> -	 * on this port, do a destination address lookup on all
> -	 * received packets as usual, disable ARP mirroring and don't
> -	 * send a copy of all transmitted/received frames on this port
> -	 * to the CPU.
> +	/* Port Control 2: don't force a good FCS, set the maximum frame size to
> +	 * 10240 bytes, enable secure 802.1q tags, don't discard tagged or
> +	 * untagged frames on this port, do a destination address lookup on all
> +	 * received packets as usual, disable ARP mirroring and don't send a
> +	 * copy of all transmitted/received frames on this port to the CPU.
>   	 */
>   	reg = 0;
>   	if (mv88e6xxx_6352_family(ds) || mv88e6xxx_6351_family(ds) ||
> @@ -1751,6 +1749,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   			reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
>   	}
>
> +	reg |= PORT_CONTROL_2_8021Q_SECURE;
> +

Hi Vivien,

Unless I misunderstand the documentation, this effectively disables VLAN
support on non-bridge ports, especially since the ndo_ functions to add VLAN
entries to such ports are not implemented. Is that intentional, or am I
missing something ?

Thanks,
Guenter

Re: [RFC 8/9] net: dsa: mv88e6352: set port 802.1Q mode to Secure

[Vivien Didelot]

Hi Guenter,

On Jun 2, 2015, at 10:31 AM, Guenter Roeck linux@roeck-us.net wrote:
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This commit changes the 802.1Q mode of each port from Disabled to
>> Secure. This enables the VLAN support, by checking the VTU entries on
>> ingress.
>>
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>   drivers/net/dsa/mv88e6xxx.c | 14 +++++++-------
>>   drivers/net/dsa/mv88e6xxx.h |  5 +++++
>>   2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
>> index ed49bd8..35243d8 100644
>> --- a/drivers/net/dsa/mv88e6xxx.c
>> +++ b/drivers/net/dsa/mv88e6xxx.c
>> @@ -1723,13 +1723,11 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds,
>> int port)
>>              goto abort;
>>      }
>>
>> -    /* Port Control 2: don't force a good FCS, set the maximum
>> -     * frame size to 10240 bytes, don't let the switch add or
>> -     * strip 802.1q tags, don't discard tagged or untagged frames
>> -     * on this port, do a destination address lookup on all
>> -     * received packets as usual, disable ARP mirroring and don't
>> -     * send a copy of all transmitted/received frames on this port
>> -     * to the CPU.
>> +    /* Port Control 2: don't force a good FCS, set the maximum frame size to
>> +     * 10240 bytes, enable secure 802.1q tags, don't discard tagged or
>> +     * untagged frames on this port, do a destination address lookup on all
>> +     * received packets as usual, disable ARP mirroring and don't send a
>> +     * copy of all transmitted/received frames on this port to the CPU.
>>       */
>>      reg = 0;
>>      if (mv88e6xxx_6352_family(ds) || mv88e6xxx_6351_family(ds) ||
>> @@ -1751,6 +1749,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int
>> port)
>>              reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
>>      }
>>
>> +    reg |= PORT_CONTROL_2_8021Q_SECURE;
>> +
> 
> Hi Vivien,
> 
> Unless I misunderstand the documentation, this effectively disables VLAN
> support on non-bridge ports, especially since the ndo_ functions to add VLAN
> entries to such ports are not implemented. Is that intentional, or am I
> missing something ?

Indeed, I intentionaly set the port mode to Secure to work on 802.1q.
For both cases, the Fallback mode should be enough; this mode checks the
VTU for a valid entry, otherwise checks the port-based VLAN map.

Supporting port-based VLAN looks like another tricky thread.

Ideally, this must be configurable. In my case I do need strict 802.1q.
Can ethtool/iproute2 can do something about the port 802.1q mode?

Thanks,
-v

Re: [RFC 8/9] net: dsa: mv88e6352: set port 802.1Q mode to Secure

[Guenter Roeck]

On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This commit changes the 802.1Q mode of each port from Disabled to
> Secure. This enables the VLAN support, by checking the VTU entries on
> ingress.
>
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 14 +++++++-------
>   drivers/net/dsa/mv88e6xxx.h |  5 +++++
>   2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index ed49bd8..35243d8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1723,13 +1723,11 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   			goto abort;
>   	}
>
> -	/* Port Control 2: don't force a good FCS, set the maximum
> -	 * frame size to 10240 bytes, don't let the switch add or
> -	 * strip 802.1q tags, don't discard tagged or untagged frames
> -	 * on this port, do a destination address lookup on all
> -	 * received packets as usual, disable ARP mirroring and don't
> -	 * send a copy of all transmitted/received frames on this port
> -	 * to the CPU.
> +	/* Port Control 2: don't force a good FCS, set the maximum frame size to
> +	 * 10240 bytes, enable secure 802.1q tags, don't discard tagged or
> +	 * untagged frames on this port, do a destination address lookup on all
> +	 * received packets as usual, disable ARP mirroring and don't send a
> +	 * copy of all transmitted/received frames on this port to the CPU.
>   	 */
>   	reg = 0;
>   	if (mv88e6xxx_6352_family(ds) || mv88e6xxx_6351_family(ds) ||
> @@ -1751,6 +1749,8 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   			reg |= PORT_CONTROL_2_FORWARD_UNKNOWN;
>   	}
>
> +	reg |= PORT_CONTROL_2_8021Q_SECURE;
> +

Vivien,

With this patch, my non-VLAN configuration fails; it appears that untagged
packets are no longer received. I found two possible solutions:
- Use PORT_CONTROL_2_8021Q_FALLBACK
- Explicitly add a VLAN entry for vid=0.

Guenter

[RFC 9/9] net: dsa: fix EDSA frame from hwaccel frame

[Vivien Didelot]

If the underlying network device features NETIF_F_HW_VLAN_CTAG_TX,
an EDSA frame is prepended with a 802.1q header once queued.

To fix this, push the VLAN tag to the payload if present, before
checking the frame protocol.

[note: we may prefer to access directly VLAN TCI from hwaccel frames,
but this approach is simpler.]

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/tag_edsa.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 9aeda59..c1c9548 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 #include "dsa_priv.h"
@@ -24,6 +25,10 @@ static netdev_tx_t edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.tx_packets++;
 	dev->stats.tx_bytes += skb->len;
 
+	skb = vlan_hwaccel_push_inside(skb);
+	if (unlikely(!skb))
+		return -ENOMEM;
+
 	/*
 	 * Convert the outermost 802.1q tag to a DSA tag and prepend
 	 * a DSA ethertype field is the packet is tagged, or insert
-- 
2.4.1