| Message ID | 20220209213044.2353153-6-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Replay and offload host VLAN entries in DSA | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | fail | Errors and warnings before: 22 this patch: 24 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 24 this patch: 27 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 27 this patch: 29 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, Feb 09, 2022 at 11:30:43PM +0200, Vladimir Oltean wrote: > - It is possible to artificially fill the VLAN table of a switch, by > walking through the entire VLAN space, adding and deleting them. > For each VLAN added on a user port, DSA will add it on shared ports > too, but for each VLAN deletion on a user port, it will remain > installed on shared ports, since DSA has no good indication of whether > the VLAN is still in use or not. If the hardware has a limited number > of VLAN table entries, this may uselessly consume that space. There's another, more important angle to this which I forgot while I was writing the commit message. If you don't have a way to delete VLANs on CPU ports, you have no way of removing yourself from a VLAN with noisy stations, once you've entered it. I'll make sure to update the commit message for v2.
Hi Vladimir, Thanks for working on this! On Wed, Feb 09, 2022 at 23:30, Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > Currently, DSA programs VLANs on shared (DSA and CPU) ports each time it > does so on user ports. This is good for basic functionality but has > several limitations: > > - the VLAN group which must reach the CPU may be radically different > from the VLAN group that must be autonomously forwarded by the switch. > In other words, the admin may want to isolate noisy stations and avoid > traffic from them going to the control processor of the switch, where > it would just waste useless cycles. The bridge already supports > independent control of VLAN groups on bridge ports and on the bridge > itself, and when VLAN-aware, it will drop packets in software anyway > if their VID isn't added as a 'self' entry towards the bridge device. > > - Replaying host FDB entries may depend, for some drivers like mv88e6xxx, > on replaying the host VLANs as well. The 2 VLAN groups are > approximately the same in most regular cases, but there are corner > cases when timing matters, and DSA's approximation of replicating > VLANs on shared ports simply does not work. > > - It is possible to artificially fill the VLAN table of a switch, by > walking through the entire VLAN space, adding and deleting them. > For each VLAN added on a user port, DSA will add it on shared ports > too, but for each VLAN deletion on a user port, it will remain > installed on shared ports, since DSA has no good indication of whether > the VLAN is still in use or not. If the hardware has a limited number > of VLAN table entries, this may uselessly consume that space. > > Now that the bridge driver emits well-balanced SWITCHDEV_OBJ_ID_PORT_VLAN > addition and removal events, DSA has a simple and straightforward task > of separating the bridge port VLANs (these have an orig_dev which is a > DSA slave interface, or a LAG interface) from the host VLANs (these have > an orig_dev which is a bridge interface), and to keep a simple reference > count of each VID on each shared port. > > Forwarding VLANs must be installed on the bridge ports and on all DSA > ports interconnecting them. We don't have a good view of the exact > topology, so we simply install forwarding VLANs on all DSA ports, which > is what has been done until now. > > Host VLANs must be installed primarily on the dedicated CPU port of each > bridge port. More subtly, they must also be installed on upstream-facing > and downstream-facing DSA ports that are connecting the bridge ports and > the CPU. This ensures that the mv88e6xxx's problem (VID of host FDB > entry may be absent from VTU) is still addressed even if that switch is > in a cross-chip setup, and it has no local CPU port. > > Therefore: > - user ports contain only bridge port (forwarding) VLANs, and no > refcounting is necessary > - DSA ports contain both forwarding and host VLANs. Refcounting is > necessary among these 2 types. > - CPU ports contain only host VLANs. Refcounting is also necessary. This is pretty much true, though this does not take foreign interfaces into account. It would be great if the condifion could be refined to: The CPU port should be a member of all VLANs where either (a) the host is a member, or (b) at least one foreign interface is a member. I.e. in a situation like this: br0 / \ swp0 tap0 If br0 is not a member of VLAN X, but tap0 is, then the CPU port still needs to be a member of X. Otherwise the (software) bridge will never get a chance to software forward it over the tunnel. > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > include/net/dsa.h | 12 +++ > net/dsa/dsa2.c | 2 + > net/dsa/dsa_priv.h | 7 ++ > net/dsa/port.c | 42 +++++++++++ > net/dsa/slave.c | 97 ++++++++++++++---------- > net/dsa/switch.c | 179 +++++++++++++++++++++++++++++++++++++++++++-- > 6 files changed, 295 insertions(+), 44 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index fd1f62a6e0a8..313295c1b0c6 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -312,6 +312,12 @@ struct dsa_port { > struct mutex addr_lists_lock; > struct list_head fdbs; > struct list_head mdbs; > + > + /* List of host VLANs that CPU and upstream-facing DSA ports > + * are members of. > + */ > + struct mutex vlans_lock; > + struct list_head vlans; > }; > > /* TODO: ideally DSA ports would have a single dp->link_dp member, > @@ -332,6 +338,12 @@ struct dsa_mac_addr { > struct list_head list; > }; > > +struct dsa_vlan { > + u16 vid; > + refcount_t refcount; > + struct list_head list; > +}; > + > struct dsa_switch { > struct device *dev; > > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index e498c927c3d0..1df8c2356463 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -453,8 +453,10 @@ static int dsa_port_setup(struct dsa_port *dp) > return 0; > > mutex_init(&dp->addr_lists_lock); > + mutex_init(&dp->vlans_lock); > INIT_LIST_HEAD(&dp->fdbs); > INIT_LIST_HEAD(&dp->mdbs); > + INIT_LIST_HEAD(&dp->vlans); > > if (ds->ops->port_setup) { > err = ds->ops->port_setup(ds, dp->index); > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 2bbfa9efe9f8..6a3878157b0a 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -34,6 +34,8 @@ enum { > DSA_NOTIFIER_HOST_MDB_DEL, > DSA_NOTIFIER_VLAN_ADD, > DSA_NOTIFIER_VLAN_DEL, > + DSA_NOTIFIER_HOST_VLAN_ADD, > + DSA_NOTIFIER_HOST_VLAN_DEL, > DSA_NOTIFIER_MTU, > DSA_NOTIFIER_TAG_PROTO, > DSA_NOTIFIER_TAG_PROTO_CONNECT, > @@ -234,6 +236,11 @@ int dsa_port_vlan_add(struct dsa_port *dp, > struct netlink_ext_ack *extack); > int dsa_port_vlan_del(struct dsa_port *dp, > const struct switchdev_obj_port_vlan *vlan); > +int dsa_port_host_vlan_add(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan, > + struct netlink_ext_ack *extack); > +int dsa_port_host_vlan_del(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan); > int dsa_port_mrp_add(const struct dsa_port *dp, > const struct switchdev_obj_mrp *mrp); > int dsa_port_mrp_del(const struct dsa_port *dp, > diff --git a/net/dsa/port.c b/net/dsa/port.c > index bd78192e0e47..cca5cf686f74 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -904,6 +904,48 @@ int dsa_port_vlan_del(struct dsa_port *dp, > return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info); > } > > +int dsa_port_host_vlan_add(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan, > + struct netlink_ext_ack *extack) > +{ > + struct dsa_notifier_vlan_info info = { > + .sw_index = dp->ds->index, > + .port = dp->index, > + .vlan = vlan, > + .extack = extack, > + }; > + struct dsa_port *cpu_dp = dp->cpu_dp; > + int err; > + > + err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_ADD, &info); > + if (err && err != -EOPNOTSUPP) > + return err; > + > + vlan_vid_add(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid); > + > + return err; > +} > + > +int dsa_port_host_vlan_del(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + struct dsa_notifier_vlan_info info = { > + .sw_index = dp->ds->index, > + .port = dp->index, > + .vlan = vlan, > + }; > + struct dsa_port *cpu_dp = dp->cpu_dp; > + int err; > + > + err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_DEL, &info); > + if (err && err != -EOPNOTSUPP) > + return err; > + > + vlan_vid_del(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid); > + > + return err; > +} > + > int dsa_port_mrp_add(const struct dsa_port *dp, > const struct switchdev_obj_mrp *mrp) > { > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 2b5b0f294233..769dabe7db91 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -348,9 +348,8 @@ static int dsa_slave_vlan_add(struct net_device *dev, > const struct switchdev_obj *obj, > struct netlink_ext_ack *extack) > { > - struct net_device *master = dsa_slave_to_master(dev); > struct dsa_port *dp = dsa_slave_to_port(dev); > - struct switchdev_obj_port_vlan vlan; > + struct switchdev_obj_port_vlan *vlan; > int err; > > if (dsa_port_skip_vlan_configuration(dp)) { > @@ -358,14 +357,14 @@ static int dsa_slave_vlan_add(struct net_device *dev, > return 0; > } > > - vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); > + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > > /* Deny adding a bridge VLAN when there is already an 802.1Q upper with > * the same VID. > */ > if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) { > rcu_read_lock(); > - err = dsa_slave_vlan_check_for_8021q_uppers(dev, &vlan); > + err = dsa_slave_vlan_check_for_8021q_uppers(dev, vlan); > rcu_read_unlock(); > if (err) { > NL_SET_ERR_MSG_MOD(extack, > @@ -374,21 +373,33 @@ static int dsa_slave_vlan_add(struct net_device *dev, > } > } > > - err = dsa_port_vlan_add(dp, &vlan, extack); > - if (err) > - return err; > + return dsa_port_vlan_add(dp, vlan, extack); > +} > + > +static int dsa_slave_host_vlan_add(struct net_device *dev, > + const struct switchdev_obj *obj, > + struct netlink_ext_ack *extack) > +{ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + struct switchdev_obj_port_vlan vlan; > > - /* We need the dedicated CPU port to be a member of the VLAN as well. > - * Even though drivers often handle CPU membership in special ways, > + if (dsa_port_skip_vlan_configuration(dp)) { > + NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); > + return 0; > + } > + > + vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); > + > + /* Even though drivers often handle CPU membership in special ways, > * it doesn't make sense to program a PVID, so clear this flag. > */ > vlan.flags &= ~BRIDGE_VLAN_INFO_PVID; > > - err = dsa_port_vlan_add(dp->cpu_dp, &vlan, extack); > - if (err) > - return err; > + /* Skip case 3 VLANs from __vlan_add() from the bridge driver */ > + if (!(vlan.flags & BRIDGE_VLAN_INFO_BRENTRY)) > + return 0; > > - return vlan_vid_add(master, htons(ETH_P_8021Q), vlan.vid); > + return dsa_port_host_vlan_add(dp, &vlan, extack); > } > > static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, > @@ -415,10 +426,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, > err = dsa_port_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); > break; > case SWITCHDEV_OBJ_ID_PORT_VLAN: > - if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) > - return -EOPNOTSUPP; > + if (netif_is_bridge_master(obj->orig_dev)) { > + if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) > + return -EOPNOTSUPP; > + > + err = dsa_slave_host_vlan_add(dev, obj, extack); > + } else { > + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) > + return -EOPNOTSUPP; > > - err = dsa_slave_vlan_add(dev, obj, extack); > + err = dsa_slave_vlan_add(dev, obj, extack); > + } > break; > case SWITCHDEV_OBJ_ID_MRP: > if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) > @@ -444,26 +462,29 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, > static int dsa_slave_vlan_del(struct net_device *dev, > const struct switchdev_obj *obj) > { > - struct net_device *master = dsa_slave_to_master(dev); > struct dsa_port *dp = dsa_slave_to_port(dev); > struct switchdev_obj_port_vlan *vlan; > - int err; > > if (dsa_port_skip_vlan_configuration(dp)) > return 0; > > vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > > - /* Do not deprogram the CPU port as it may be shared with other user > - * ports which can be members of this VLAN as well. > - */ > - err = dsa_port_vlan_del(dp, vlan); > - if (err) > - return err; > + return dsa_port_vlan_del(dp, vlan); > +} > > - vlan_vid_del(master, htons(ETH_P_8021Q), vlan->vid); > +static int dsa_slave_host_vlan_del(struct net_device *dev, > + const struct switchdev_obj *obj) > +{ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + struct switchdev_obj_port_vlan *vlan; > > - return 0; > + if (dsa_port_skip_vlan_configuration(dp)) > + return 0; > + > + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > + > + return dsa_port_host_vlan_del(dp, vlan); > } > > static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx, > @@ -489,10 +510,17 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx, > err = dsa_port_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); > break; > case SWITCHDEV_OBJ_ID_PORT_VLAN: > - if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) > - return -EOPNOTSUPP; > + if (netif_is_bridge_master(obj->orig_dev)) { > + if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) > + return -EOPNOTSUPP; > > - err = dsa_slave_vlan_del(dev, obj); > + err = dsa_slave_host_vlan_del(dev, obj); > + } else { > + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) > + return -EOPNOTSUPP; > + > + err = dsa_slave_vlan_del(dev, obj); > + } > break; > case SWITCHDEV_OBJ_ID_MRP: > if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) > @@ -1405,7 +1433,7 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > } > > /* And CPU port... */ > - ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &extack); > + ret = dsa_port_host_vlan_add(dp, &vlan, &extack); > if (ret) { > if (extack._msg) > netdev_err(dev, "CPU port %d: %s\n", dp->cpu_dp->index, > @@ -1413,7 +1441,7 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, > return ret; > } > > - return vlan_vid_add(master, proto, vid); > + return 0; > } > > static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, > @@ -1428,16 +1456,11 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, > }; > int err; > > - /* Do not deprogram the CPU port as it may be shared with other user > - * ports which can be members of this VLAN as well. > - */ > err = dsa_port_vlan_del(dp, &vlan); > if (err) > return err; > > - vlan_vid_del(master, proto, vid); > - > - return 0; > + return dsa_port_host_vlan_del(dp, &vlan); > } > > static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg) > diff --git a/net/dsa/switch.c b/net/dsa/switch.c > index 4866b58649e4..9e4570bdea2f 100644 > --- a/net/dsa/switch.c > +++ b/net/dsa/switch.c > @@ -558,6 +558,7 @@ static int dsa_switch_host_mdb_del(struct dsa_switch *ds, > return err; > } > > +/* Port VLANs match on the targeted port and on all DSA ports */ > static bool dsa_port_vlan_match(struct dsa_port *dp, > struct dsa_notifier_vlan_info *info) > { > @@ -570,6 +571,118 @@ static bool dsa_port_vlan_match(struct dsa_port *dp, > return false; > } > > +/* Host VLANs match on the targeted port's CPU port, and on all DSA ports > + * (upstream and downstream) of that switch and its upstream switches. > + */ > +static bool dsa_port_host_vlan_match(struct dsa_port *dp, > + struct dsa_notifier_vlan_info *info) > +{ > + struct dsa_port *targeted_dp, *cpu_dp; > + struct dsa_switch *targeted_ds; > + > + targeted_ds = dsa_switch_find(dp->ds->dst->index, info->sw_index); > + targeted_dp = dsa_to_port(targeted_ds, info->port); > + cpu_dp = targeted_dp->cpu_dp; > + > + if (dsa_switch_is_upstream_of(dp->ds, targeted_ds)) > + return dsa_port_is_dsa(dp) || dp == cpu_dp; > + > + return false; > +} > + > +static struct dsa_vlan *dsa_vlan_find(struct list_head *vlan_list, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + struct dsa_vlan *v; > + > + list_for_each_entry(v, vlan_list, list) > + if (v->vid == vlan->vid) > + return v; > + > + return NULL; > +} > + > +static int dsa_port_do_vlan_add(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan, > + struct netlink_ext_ack *extack) > +{ > + struct dsa_switch *ds = dp->ds; > + int port = dp->index; > + struct dsa_vlan *v; > + int err = 0; > + > + /* No need to bother with refcounting for user ports */ > + if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) > + return ds->ops->port_vlan_add(ds, port, vlan, extack); > + > + mutex_lock(&dp->vlans_lock); > + > + v = dsa_vlan_find(&dp->vlans, vlan); > + if (v) { > + refcount_inc(&v->refcount); > + goto out; > + } > + > + v = kzalloc(sizeof(*v), GFP_KERNEL); > + if (!v) { > + err = -ENOMEM; > + goto out; > + } > + > + err = ds->ops->port_vlan_add(ds, port, vlan, extack); > + if (err) { > + kfree(v); > + goto out; > + } > + > + v->vid = vlan->vid; > + refcount_set(&v->refcount, 1); > + list_add_tail(&v->list, &dp->vlans); > + > +out: > + mutex_unlock(&dp->vlans_lock); > + > + return err; > +} > + > +static int dsa_port_do_vlan_del(struct dsa_port *dp, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + struct dsa_switch *ds = dp->ds; > + int port = dp->index; > + struct dsa_vlan *v; > + int err = 0; > + > + /* No need to bother with refcounting for user ports */ > + if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) > + return ds->ops->port_vlan_del(ds, port, vlan); > + > + mutex_lock(&dp->vlans_lock); > + > + v = dsa_vlan_find(&dp->vlans, vlan); > + if (!v) { > + err = -ENOENT; > + goto out; > + } > + > + if (!refcount_dec_and_test(&v->refcount)) > + goto out; > + > + err = ds->ops->port_vlan_del(ds, port, vlan); > + if (err) { > + refcount_set(&v->refcount, 1); > + goto out; > + } > + > + list_del(&v->list); > + kfree(v); > + > +out: > + mutex_unlock(&dp->vlans_lock); > + > + return err; > +} > + > static int dsa_switch_vlan_add(struct dsa_switch *ds, > struct dsa_notifier_vlan_info *info) > { > @@ -581,8 +694,8 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, > > dsa_switch_for_each_port(dp, ds) { > if (dsa_port_vlan_match(dp, info)) { > - err = ds->ops->port_vlan_add(ds, dp->index, info->vlan, > - info->extack); > + err = dsa_port_do_vlan_add(dp, info->vlan, > + info->extack); > if (err) > return err; > } > @@ -594,15 +707,61 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, > static int dsa_switch_vlan_del(struct dsa_switch *ds, > struct dsa_notifier_vlan_info *info) > { > + struct dsa_port *dp; > + int err; > + > if (!ds->ops->port_vlan_del) > return -EOPNOTSUPP; > > - if (ds->index == info->sw_index) > - return ds->ops->port_vlan_del(ds, info->port, info->vlan); > + dsa_switch_for_each_port(dp, ds) { > + if (dsa_port_vlan_match(dp, info)) { > + err = dsa_port_do_vlan_del(dp, info->vlan); > + if (err) > + return err; > + } > + } > + > + return 0; > +} > + > +static int dsa_switch_host_vlan_add(struct dsa_switch *ds, > + struct dsa_notifier_vlan_info *info) > +{ > + struct dsa_port *dp; > + int err; > + > + if (!ds->ops->port_vlan_add) > + return -EOPNOTSUPP; > + > + dsa_switch_for_each_port(dp, ds) { > + if (dsa_port_host_vlan_match(dp, info)) { > + err = dsa_port_do_vlan_add(dp, info->vlan, > + info->extack); > + if (err) > + return err; > + } > + } > + > + return 0; > +} > + > +static int dsa_switch_host_vlan_del(struct dsa_switch *ds, > + struct dsa_notifier_vlan_info *info) > +{ > + struct dsa_port *dp; > + int err; > + > + if (!ds->ops->port_vlan_del) > + return -EOPNOTSUPP; > + > + dsa_switch_for_each_port(dp, ds) { > + if (dsa_port_host_vlan_match(dp, info)) { > + err = dsa_port_do_vlan_del(dp, info->vlan); > + if (err) > + return err; > + } > + } > > - /* Do not deprogram the DSA links as they may be used as conduit > - * for other VLAN members in the fabric. > - */ > return 0; > } > > @@ -764,6 +923,12 @@ static int dsa_switch_event(struct notifier_block *nb, > case DSA_NOTIFIER_VLAN_DEL: > err = dsa_switch_vlan_del(ds, info); > break; > + case DSA_NOTIFIER_HOST_VLAN_ADD: > + err = dsa_switch_host_vlan_add(ds, info); > + break; > + case DSA_NOTIFIER_HOST_VLAN_DEL: > + err = dsa_switch_host_vlan_del(ds, info); > + break; > case DSA_NOTIFIER_MTU: > err = dsa_switch_mtu(ds, info); > break; > -- > 2.25.1
On Sun, Feb 13, 2022 at 02:09:35AM +0100, Tobias Waldekranz wrote: > > Therefore: > > - user ports contain only bridge port (forwarding) VLANs, and no > > refcounting is necessary > > - DSA ports contain both forwarding and host VLANs. Refcounting is > > necessary among these 2 types. > > - CPU ports contain only host VLANs. Refcounting is also necessary. > > This is pretty much true, though this does not take foreign interfaces > into account. It would be great if the condifion could be refined to: > > The CPU port should be a member of all VLANs where either (a) the > host is a member, or (b) at least one foreign interface is a member. > > I.e. in a situation like this: > > br0 > / \ > swp0 tap0 > > If br0 is not a member of VLAN X, but tap0 is, then the CPU port still > needs to be a member of X. Otherwise the (software) bridge will never > get a chance to software forward it over the tunnel. This is a good observation and it can be done - just in the same way as we treat FDB entries on foregin interfaces as upstream-facing FDB entries in DSA. It also has implications upon the replay procedure, because if we want this to happen, we must replay all bridge VLAN groups, not just the port and the bridge VLANs. Again, similar to how br_switchdev_fdb_replay() replays the entire FDB. I can start working on these changes, but in parallel I'd also like an Ack from Nikolay, Roopa, Jiri, Ido on the approach from patches 1-3, since the whole refcounting thing depends on that. I think it's fairly safe, but maybe it breaks something I haven't thought of...
diff --git a/include/net/dsa.h b/include/net/dsa.h index fd1f62a6e0a8..313295c1b0c6 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -312,6 +312,12 @@ struct dsa_port { struct mutex addr_lists_lock; struct list_head fdbs; struct list_head mdbs; + + /* List of host VLANs that CPU and upstream-facing DSA ports + * are members of. + */ + struct mutex vlans_lock; + struct list_head vlans; }; /* TODO: ideally DSA ports would have a single dp->link_dp member, @@ -332,6 +338,12 @@ struct dsa_mac_addr { struct list_head list; }; +struct dsa_vlan { + u16 vid; + refcount_t refcount; + struct list_head list; +}; + struct dsa_switch { struct device *dev; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index e498c927c3d0..1df8c2356463 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -453,8 +453,10 @@ static int dsa_port_setup(struct dsa_port *dp) return 0; mutex_init(&dp->addr_lists_lock); + mutex_init(&dp->vlans_lock); INIT_LIST_HEAD(&dp->fdbs); INIT_LIST_HEAD(&dp->mdbs); + INIT_LIST_HEAD(&dp->vlans); if (ds->ops->port_setup) { err = ds->ops->port_setup(ds, dp->index); diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 2bbfa9efe9f8..6a3878157b0a 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -34,6 +34,8 @@ enum { DSA_NOTIFIER_HOST_MDB_DEL, DSA_NOTIFIER_VLAN_ADD, DSA_NOTIFIER_VLAN_DEL, + DSA_NOTIFIER_HOST_VLAN_ADD, + DSA_NOTIFIER_HOST_VLAN_DEL, DSA_NOTIFIER_MTU, DSA_NOTIFIER_TAG_PROTO, DSA_NOTIFIER_TAG_PROTO_CONNECT, @@ -234,6 +236,11 @@ int dsa_port_vlan_add(struct dsa_port *dp, struct netlink_ext_ack *extack); int dsa_port_vlan_del(struct dsa_port *dp, const struct switchdev_obj_port_vlan *vlan); +int dsa_port_host_vlan_add(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan, + struct netlink_ext_ack *extack); +int dsa_port_host_vlan_del(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan); int dsa_port_mrp_add(const struct dsa_port *dp, const struct switchdev_obj_mrp *mrp); int dsa_port_mrp_del(const struct dsa_port *dp, diff --git a/net/dsa/port.c b/net/dsa/port.c index bd78192e0e47..cca5cf686f74 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -904,6 +904,48 @@ int dsa_port_vlan_del(struct dsa_port *dp, return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info); } +int dsa_port_host_vlan_add(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan, + struct netlink_ext_ack *extack) +{ + struct dsa_notifier_vlan_info info = { + .sw_index = dp->ds->index, + .port = dp->index, + .vlan = vlan, + .extack = extack, + }; + struct dsa_port *cpu_dp = dp->cpu_dp; + int err; + + err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_ADD, &info); + if (err && err != -EOPNOTSUPP) + return err; + + vlan_vid_add(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid); + + return err; +} + +int dsa_port_host_vlan_del(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan) +{ + struct dsa_notifier_vlan_info info = { + .sw_index = dp->ds->index, + .port = dp->index, + .vlan = vlan, + }; + struct dsa_port *cpu_dp = dp->cpu_dp; + int err; + + err = dsa_port_notify(dp, DSA_NOTIFIER_HOST_VLAN_DEL, &info); + if (err && err != -EOPNOTSUPP) + return err; + + vlan_vid_del(cpu_dp->master, htons(ETH_P_8021Q), vlan->vid); + + return err; +} + int dsa_port_mrp_add(const struct dsa_port *dp, const struct switchdev_obj_mrp *mrp) { diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 2b5b0f294233..769dabe7db91 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -348,9 +348,8 @@ static int dsa_slave_vlan_add(struct net_device *dev, const struct switchdev_obj *obj, struct netlink_ext_ack *extack) { - struct net_device *master = dsa_slave_to_master(dev); struct dsa_port *dp = dsa_slave_to_port(dev); - struct switchdev_obj_port_vlan vlan; + struct switchdev_obj_port_vlan *vlan; int err; if (dsa_port_skip_vlan_configuration(dp)) { @@ -358,14 +357,14 @@ static int dsa_slave_vlan_add(struct net_device *dev, return 0; } - vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); /* Deny adding a bridge VLAN when there is already an 802.1Q upper with * the same VID. */ if (br_vlan_enabled(dsa_port_bridge_dev_get(dp))) { rcu_read_lock(); - err = dsa_slave_vlan_check_for_8021q_uppers(dev, &vlan); + err = dsa_slave_vlan_check_for_8021q_uppers(dev, vlan); rcu_read_unlock(); if (err) { NL_SET_ERR_MSG_MOD(extack, @@ -374,21 +373,33 @@ static int dsa_slave_vlan_add(struct net_device *dev, } } - err = dsa_port_vlan_add(dp, &vlan, extack); - if (err) - return err; + return dsa_port_vlan_add(dp, vlan, extack); +} + +static int dsa_slave_host_vlan_add(struct net_device *dev, + const struct switchdev_obj *obj, + struct netlink_ext_ack *extack) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + struct switchdev_obj_port_vlan vlan; - /* We need the dedicated CPU port to be a member of the VLAN as well. - * Even though drivers often handle CPU membership in special ways, + if (dsa_port_skip_vlan_configuration(dp)) { + NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); + return 0; + } + + vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); + + /* Even though drivers often handle CPU membership in special ways, * it doesn't make sense to program a PVID, so clear this flag. */ vlan.flags &= ~BRIDGE_VLAN_INFO_PVID; - err = dsa_port_vlan_add(dp->cpu_dp, &vlan, extack); - if (err) - return err; + /* Skip case 3 VLANs from __vlan_add() from the bridge driver */ + if (!(vlan.flags & BRIDGE_VLAN_INFO_BRENTRY)) + return 0; - return vlan_vid_add(master, htons(ETH_P_8021Q), vlan.vid); + return dsa_port_host_vlan_add(dp, &vlan, extack); } static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, @@ -415,10 +426,17 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, err = dsa_port_host_mdb_add(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: - if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (netif_is_bridge_master(obj->orig_dev)) { + if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) + return -EOPNOTSUPP; + + err = dsa_slave_host_vlan_add(dev, obj, extack); + } else { + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) + return -EOPNOTSUPP; - err = dsa_slave_vlan_add(dev, obj, extack); + err = dsa_slave_vlan_add(dev, obj, extack); + } break; case SWITCHDEV_OBJ_ID_MRP: if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) @@ -444,26 +462,29 @@ static int dsa_slave_port_obj_add(struct net_device *dev, const void *ctx, static int dsa_slave_vlan_del(struct net_device *dev, const struct switchdev_obj *obj) { - struct net_device *master = dsa_slave_to_master(dev); struct dsa_port *dp = dsa_slave_to_port(dev); struct switchdev_obj_port_vlan *vlan; - int err; if (dsa_port_skip_vlan_configuration(dp)) return 0; vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); - /* Do not deprogram the CPU port as it may be shared with other user - * ports which can be members of this VLAN as well. - */ - err = dsa_port_vlan_del(dp, vlan); - if (err) - return err; + return dsa_port_vlan_del(dp, vlan); +} - vlan_vid_del(master, htons(ETH_P_8021Q), vlan->vid); +static int dsa_slave_host_vlan_del(struct net_device *dev, + const struct switchdev_obj *obj) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + struct switchdev_obj_port_vlan *vlan; - return 0; + if (dsa_port_skip_vlan_configuration(dp)) + return 0; + + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); + + return dsa_port_host_vlan_del(dp, vlan); } static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx, @@ -489,10 +510,17 @@ static int dsa_slave_port_obj_del(struct net_device *dev, const void *ctx, err = dsa_port_host_mdb_del(dp, SWITCHDEV_OBJ_PORT_MDB(obj)); break; case SWITCHDEV_OBJ_ID_PORT_VLAN: - if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) - return -EOPNOTSUPP; + if (netif_is_bridge_master(obj->orig_dev)) { + if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) + return -EOPNOTSUPP; - err = dsa_slave_vlan_del(dev, obj); + err = dsa_slave_host_vlan_del(dev, obj); + } else { + if (!dsa_port_offloads_bridge_port(dp, obj->orig_dev)) + return -EOPNOTSUPP; + + err = dsa_slave_vlan_del(dev, obj); + } break; case SWITCHDEV_OBJ_ID_MRP: if (!dsa_port_offloads_bridge_dev(dp, obj->orig_dev)) @@ -1405,7 +1433,7 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, } /* And CPU port... */ - ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &extack); + ret = dsa_port_host_vlan_add(dp, &vlan, &extack); if (ret) { if (extack._msg) netdev_err(dev, "CPU port %d: %s\n", dp->cpu_dp->index, @@ -1413,7 +1441,7 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, return ret; } - return vlan_vid_add(master, proto, vid); + return 0; } static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, @@ -1428,16 +1456,11 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, }; int err; - /* Do not deprogram the CPU port as it may be shared with other user - * ports which can be members of this VLAN as well. - */ err = dsa_port_vlan_del(dp, &vlan); if (err) return err; - vlan_vid_del(master, proto, vid); - - return 0; + return dsa_port_host_vlan_del(dp, &vlan); } static int dsa_slave_restore_vlan(struct net_device *vdev, int vid, void *arg) diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 4866b58649e4..9e4570bdea2f 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -558,6 +558,7 @@ static int dsa_switch_host_mdb_del(struct dsa_switch *ds, return err; } +/* Port VLANs match on the targeted port and on all DSA ports */ static bool dsa_port_vlan_match(struct dsa_port *dp, struct dsa_notifier_vlan_info *info) { @@ -570,6 +571,118 @@ static bool dsa_port_vlan_match(struct dsa_port *dp, return false; } +/* Host VLANs match on the targeted port's CPU port, and on all DSA ports + * (upstream and downstream) of that switch and its upstream switches. + */ +static bool dsa_port_host_vlan_match(struct dsa_port *dp, + struct dsa_notifier_vlan_info *info) +{ + struct dsa_port *targeted_dp, *cpu_dp; + struct dsa_switch *targeted_ds; + + targeted_ds = dsa_switch_find(dp->ds->dst->index, info->sw_index); + targeted_dp = dsa_to_port(targeted_ds, info->port); + cpu_dp = targeted_dp->cpu_dp; + + if (dsa_switch_is_upstream_of(dp->ds, targeted_ds)) + return dsa_port_is_dsa(dp) || dp == cpu_dp; + + return false; +} + +static struct dsa_vlan *dsa_vlan_find(struct list_head *vlan_list, + const struct switchdev_obj_port_vlan *vlan) +{ + struct dsa_vlan *v; + + list_for_each_entry(v, vlan_list, list) + if (v->vid == vlan->vid) + return v; + + return NULL; +} + +static int dsa_port_do_vlan_add(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan, + struct netlink_ext_ack *extack) +{ + struct dsa_switch *ds = dp->ds; + int port = dp->index; + struct dsa_vlan *v; + int err = 0; + + /* No need to bother with refcounting for user ports */ + if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) + return ds->ops->port_vlan_add(ds, port, vlan, extack); + + mutex_lock(&dp->vlans_lock); + + v = dsa_vlan_find(&dp->vlans, vlan); + if (v) { + refcount_inc(&v->refcount); + goto out; + } + + v = kzalloc(sizeof(*v), GFP_KERNEL); + if (!v) { + err = -ENOMEM; + goto out; + } + + err = ds->ops->port_vlan_add(ds, port, vlan, extack); + if (err) { + kfree(v); + goto out; + } + + v->vid = vlan->vid; + refcount_set(&v->refcount, 1); + list_add_tail(&v->list, &dp->vlans); + +out: + mutex_unlock(&dp->vlans_lock); + + return err; +} + +static int dsa_port_do_vlan_del(struct dsa_port *dp, + const struct switchdev_obj_port_vlan *vlan) +{ + struct dsa_switch *ds = dp->ds; + int port = dp->index; + struct dsa_vlan *v; + int err = 0; + + /* No need to bother with refcounting for user ports */ + if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp))) + return ds->ops->port_vlan_del(ds, port, vlan); + + mutex_lock(&dp->vlans_lock); + + v = dsa_vlan_find(&dp->vlans, vlan); + if (!v) { + err = -ENOENT; + goto out; + } + + if (!refcount_dec_and_test(&v->refcount)) + goto out; + + err = ds->ops->port_vlan_del(ds, port, vlan); + if (err) { + refcount_set(&v->refcount, 1); + goto out; + } + + list_del(&v->list); + kfree(v); + +out: + mutex_unlock(&dp->vlans_lock); + + return err; +} + static int dsa_switch_vlan_add(struct dsa_switch *ds, struct dsa_notifier_vlan_info *info) { @@ -581,8 +694,8 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, dsa_switch_for_each_port(dp, ds) { if (dsa_port_vlan_match(dp, info)) { - err = ds->ops->port_vlan_add(ds, dp->index, info->vlan, - info->extack); + err = dsa_port_do_vlan_add(dp, info->vlan, + info->extack); if (err) return err; } @@ -594,15 +707,61 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, static int dsa_switch_vlan_del(struct dsa_switch *ds, struct dsa_notifier_vlan_info *info) { + struct dsa_port *dp; + int err; + if (!ds->ops->port_vlan_del) return -EOPNOTSUPP; - if (ds->index == info->sw_index) - return ds->ops->port_vlan_del(ds, info->port, info->vlan); + dsa_switch_for_each_port(dp, ds) { + if (dsa_port_vlan_match(dp, info)) { + err = dsa_port_do_vlan_del(dp, info->vlan); + if (err) + return err; + } + } + + return 0; +} + +static int dsa_switch_host_vlan_add(struct dsa_switch *ds, + struct dsa_notifier_vlan_info *info) +{ + struct dsa_port *dp; + int err; + + if (!ds->ops->port_vlan_add) + return -EOPNOTSUPP; + + dsa_switch_for_each_port(dp, ds) { + if (dsa_port_host_vlan_match(dp, info)) { + err = dsa_port_do_vlan_add(dp, info->vlan, + info->extack); + if (err) + return err; + } + } + + return 0; +} + +static int dsa_switch_host_vlan_del(struct dsa_switch *ds, + struct dsa_notifier_vlan_info *info) +{ + struct dsa_port *dp; + int err; + + if (!ds->ops->port_vlan_del) + return -EOPNOTSUPP; + + dsa_switch_for_each_port(dp, ds) { + if (dsa_port_host_vlan_match(dp, info)) { + err = dsa_port_do_vlan_del(dp, info->vlan); + if (err) + return err; + } + } - /* Do not deprogram the DSA links as they may be used as conduit - * for other VLAN members in the fabric. - */ return 0; } @@ -764,6 +923,12 @@ static int dsa_switch_event(struct notifier_block *nb, case DSA_NOTIFIER_VLAN_DEL: err = dsa_switch_vlan_del(ds, info); break; + case DSA_NOTIFIER_HOST_VLAN_ADD: + err = dsa_switch_host_vlan_add(ds, info); + break; + case DSA_NOTIFIER_HOST_VLAN_DEL: + err = dsa_switch_host_vlan_del(ds, info); + break; case DSA_NOTIFIER_MTU: err = dsa_switch_mtu(ds, info); break;
Currently, DSA programs VLANs on shared (DSA and CPU) ports each time it does so on user ports. This is good for basic functionality but has several limitations: - the VLAN group which must reach the CPU may be radically different from the VLAN group that must be autonomously forwarded by the switch. In other words, the admin may want to isolate noisy stations and avoid traffic from them going to the control processor of the switch, where it would just waste useless cycles. The bridge already supports independent control of VLAN groups on bridge ports and on the bridge itself, and when VLAN-aware, it will drop packets in software anyway if their VID isn't added as a 'self' entry towards the bridge device. - Replaying host FDB entries may depend, for some drivers like mv88e6xxx, on replaying the host VLANs as well. The 2 VLAN groups are approximately the same in most regular cases, but there are corner cases when timing matters, and DSA's approximation of replicating VLANs on shared ports simply does not work. - It is possible to artificially fill the VLAN table of a switch, by walking through the entire VLAN space, adding and deleting them. For each VLAN added on a user port, DSA will add it on shared ports too, but for each VLAN deletion on a user port, it will remain installed on shared ports, since DSA has no good indication of whether the VLAN is still in use or not. If the hardware has a limited number of VLAN table entries, this may uselessly consume that space. Now that the bridge driver emits well-balanced SWITCHDEV_OBJ_ID_PORT_VLAN addition and removal events, DSA has a simple and straightforward task of separating the bridge port VLANs (these have an orig_dev which is a DSA slave interface, or a LAG interface) from the host VLANs (these have an orig_dev which is a bridge interface), and to keep a simple reference count of each VID on each shared port. Forwarding VLANs must be installed on the bridge ports and on all DSA ports interconnecting them. We don't have a good view of the exact topology, so we simply install forwarding VLANs on all DSA ports, which is what has been done until now. Host VLANs must be installed primarily on the dedicated CPU port of each bridge port. More subtly, they must also be installed on upstream-facing and downstream-facing DSA ports that are connecting the bridge ports and the CPU. This ensures that the mv88e6xxx's problem (VID of host FDB entry may be absent from VTU) is still addressed even if that switch is in a cross-chip setup, and it has no local CPU port. Therefore: - user ports contain only bridge port (forwarding) VLANs, and no refcounting is necessary - DSA ports contain both forwarding and host VLANs. Refcounting is necessary among these 2 types. - CPU ports contain only host VLANs. Refcounting is also necessary. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- include/net/dsa.h | 12 +++ net/dsa/dsa2.c | 2 + net/dsa/dsa_priv.h | 7 ++ net/dsa/port.c | 42 +++++++++++ net/dsa/slave.c | 97 ++++++++++++++---------- net/dsa/switch.c | 179 +++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 295 insertions(+), 44 deletions(-)