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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This commit adds support for the VTU operations to the mv88e6352 driver.
Signed-off-by: Vivien Didelot <[email protected]>
---
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
Disable the mirroring policy in the monitor control register, since this
feature is not needed.
Signed-off-by: Vivien Didelot <[email protected]>
---
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
This patch disables egress of unknown unicast destination addresses.
Signed-off-by: Vivien Didelot <[email protected]>
---
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
This commit disables SA learning and refreshing for the CPU port.
Signed-off-by: Vivien Didelot <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
[ ... ]
> +
> +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
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 <[email protected]>
> ---
> 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
On Mon, Jun 1, 2015 at 11:50 PM, Guenter Roeck <[email protected]> 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
> +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
On 06/02/2015 12:44 AM, Scott Feldman wrote:
> On Mon, Jun 1, 2015 at 11:50 PM, Guenter Roeck <[email protected]> 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
> 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
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 <[email protected]>
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
>
> 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
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
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 <[email protected]>
> ---
> 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
Hi Guenter,
On Jun 2, 2015, at 10:31 AM, Guenter Roeck [email protected] 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 <[email protected]>
>> ---
>> 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
Hi Guenter,
On Jun 2, 2015, at 10:42 AM, Guenter Roeck [email protected] 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 <[email protected]>
>> ---
>> 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
Hi Guenter,
On Jun 2, 2015, at 10:24 AM, Guenter Roeck [email protected] 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
Hi Guenter, Andrew,
On Jun 2, 2015, at 10:53 AM, Andrew Lunn [email protected] 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 <[email protected]>
>>
>> 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
Guenter,
On Jun 2, 2015, at 2:50 AM, Guenter Roeck [email protected] 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
Hi Guenter,
On Jun 2, 2015, at 10:20 AM, Guenter Roeck [email protected] 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
On Tue, Jun 02, 2015 at 09:39:50PM -0400, Vivien Didelot wrote:
> Guenter,
>
> On Jun 2, 2015, at 2:50 AM, Guenter Roeck [email protected] 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
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 [email protected] 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
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 [email protected] 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 <[email protected]>
> >>
> >> 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
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" <[email protected] <mailto:[email protected]>> 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 [email protected] <mailto:[email protected]> 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
>
On Tue, Jun 2, 2015 at 3:31 PM, nolan <[email protected]> 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?
On 06/02/2015 11:53 PM, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 3:31 PM, nolan <[email protected]> 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
Hi Guenter,
On Jun 2, 2015, at 10:17 PM, Guenter Roeck [email protected] 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 [email protected] 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
On 06/03/2015 07:56 AM, Vivien Didelot wrote:
> Hi Guenter,
>
> On Jun 2, 2015, at 10:17 PM, Guenter Roeck [email protected] 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 [email protected] 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
On 02/06/15 23:53, Scott Feldman wrote:
> On Tue, Jun 2, 2015 at 3:31 PM, nolan <[email protected]> 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
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
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.
Hi Guenter,
On Jun 2, 2015, at 9:52 PM, Vivien Didelot [email protected] wrote:
> On Jun 2, 2015, at 10:20 AM, Guenter Roeck [email protected] 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