On Mon, Mar 15, 2021 at 08:54:13PM +0100, Tobias Waldekranz wrote:
> There are four kinds of events that have an inpact on VLAN
impact
> configuration of DSA ports:
>
> - Adding VLAN uppers
> (ip link add dev swp0.1 link swp0 type vlan id 1)
(..)
> +static bool dsa_8021q_uppers_are_coherent(struct dsa_switch_tree *dst,
> + struct net_device *br,
> + bool seen_vlan_upper,
have_8021q_uppers_in_bridge maybe?
> + unsigned long *upper_vids,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_device *lower, *upper;
> + struct list_head *liter, *uiter;
It doesn't hurt to name them lower_iter, upper_iter?
> + struct dsa_slave_priv *priv;
> + bool seen_offloaded = false;
> + u16 vid;
> +
> + netdev_for_each_lower_dev(br, lower, liter) {
> + priv = dsa_slave_dev_lower_find(lower);
> + if (!priv || priv->dp->ds->dst != dst)
> + /* Ignore ports that are not related to us in
> + * any way.
> + */
> + continue;
So "priv" is the lower of a bridge port...
> +
> + if (is_vlan_dev(lower)) {
> + seen_vlan_upper = true;
> + continue;
> + }
But in the code path below, that bridge port is not a VLAN... So it must
be a LAG or a HSR ring....
> + if (dsa_port_offloads_bridge(priv->dp, br) &&
> + dsa_port_offloads_bridge_port(priv->dp, lower))
> + seen_offloaded = true;
> + else
> + /* Non-offloaded uppers can to whatever they
s/can to/can do/
> + * want.
> + */
> + continue;
Which is offloaded..
> + netdev_for_each_upper_dev_rcu(lower, upper, uiter) {
> + if (!is_vlan_dev(upper))
> + continue;
So this iterates through VLAN uppers of offloaded LAGs and HSR rings?
Does it also iterate through 8021q uppers of "priv" somehow?
> + vid = vlan_dev_vlan_id(upper);
> + if (!test_bit(vid, upper_vids)) {
> + set_bit(vid, upper_vids);
> + continue;
> + }
> +
> + NL_SET_ERR_MSG_MOD(extack,
> + "Multiple VLAN interfaces cannot use
> the same VID");
> + return false;
> + }
> + }
> +
> + if (seen_offloaded && seen_vlan_upper) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "VLAN interfaces cannot share bridge with
> offloaded port");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static bool dsa_bridge_vlans_are_coherent(struct net_device *br,
> + u16 new_vid, unsigned long
> *upper_vids,
const unsigned long *upper_vids
> + struct netlink_ext_ack *extack)
> +{
> + u16 vid;
> +
> + if (new_vid && test_bit(new_vid, upper_vids))
> + goto err;
> +
> + for_each_set_bit(vid, upper_vids, VLAN_N_VID) {
> + struct bridge_vlan_info br_info;
> +
> + if (br_vlan_get_info(br, vid, &br_info))
You should only error out if VLAN filtering is enabled/turning on in the
bridge, no?
> + /* Error means that the VID does not exist,
> + * which is what we want to ensure.
> + */
> + continue;
> +
> + goto err;
> + }
> +
> + return true;
> +
> +err:
> + NL_SET_ERR_MSG_MOD(extack, "No bridge VID may be used on a related VLAN
> interface");
> + return false;
> +}
> +
> +/**
> + * dsa_bridge_is_coherent - Verify that DSA tree accepts a bridge config.
> + * @dst: Tree to verify against.
> + * @br: Bridge netdev to verify.
> + * @mod: Description of the modification to introduce.
> + * @extack: Netlink extended ack for error reporting.
> + *
> + * Verify that the VLAN config of @br, its offloaded ports belonging
> + * to @dst and their VLAN uppers, can be correctly offloaded after
> + * introducing the change described by @mod. If this is not the case,
> + * an error is reported via @extack.
> + *
> + * Return: true if the config can be offloaded, false otherwise.
> + */
> +bool dsa_bridge_is_coherent(struct dsa_switch_tree *dst, struct net_device
> *br,
> + struct dsa_bridge_mod *mod,
> + struct netlink_ext_ack *extack)
> +{
> + unsigned long *upper_vids = NULL;
initialization with NULL is pointless.
> + bool filter;
> +
> + if (mod->filter)
> + filter = *mod->filter;
> + else
> + filter = br && br_vlan_enabled(br);
> +
> + if (!dsa_bridge_filtering_is_coherent(dst, filter, extack))
> + goto err;
> +
> + if (!filter)
> + return true;
> +
> + upper_vids = bitmap_zalloc(VLAN_N_VID, GFP_KERNEL);
> + if (!upper_vids) {
> + WARN_ON_ONCE(1);
if (WARN_ON_ONCE(!upper_vids))
> + goto err;
> + }
> +
> + if (mod->upper_vid)
> + set_bit(mod->upper_vid, upper_vids);
> +
> + if (!dsa_8021q_uppers_are_coherent(dst, br, mod->bridge_upper,
> + upper_vids, extack))
> + goto err_free;
> +
> + if (!dsa_bridge_vlans_are_coherent(br, mod->br_vid, upper_vids, extack))
> + goto err_free;
> +
> + kfree(upper_vids);
> + return true;
> +
> +err_free:
> + kfree(upper_vids);
> +err:
> + return false;
> +}
> +
> /**
> * dsa_tree_notify - Execute code for all switches in a DSA switch tree.
> * @dst: collection of struct dsa_switch devices to notify.
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 9d4b0e9b1aa1..8d8d307df437 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -361,6 +369,27 @@ int dsa_switch_register_notifier(struct dsa_switch *ds);
> void dsa_switch_unregister_notifier(struct dsa_switch *ds);
>
> /* dsa2.c */
> +
> +/**
> + * struct dsa_bridge_mod - Modification of bridge related config
> + * @filter: If non-NULL, the new state of VLAN filtering.
> + * @br_vid: If non-zero, this VID will be added to the bridge.
> + * @upper_vid: If non-zero, a VLAN upper using this VID will be added to
> + * a bridge port.
> + * @bridge_upper: If non-NULL, a VLAN upper will be added to the bridge.
I would name this "add_8021q_upper_to_bridge". Longer name, but clearer.
> + *
> + * Describes a bridge related modification that is about to be applied.
> + */
> +struct dsa_bridge_mod {
> + bool *filter;
> + u16 br_vid;
> + u16 upper_vid;
> + bool bridge_upper;
> +};
Frankly this is a bit ugly, but I have no better idea, and the structure
is good enough for describing a state transition. Fully describing the
state is a lot more difficult, due to the need to list all bridges which
may span a DSA switch tree.
> +bool dsa_bridge_is_coherent(struct dsa_switch_tree *dst, struct net_device
> *br,
> + struct dsa_bridge_mod *mod,
> + struct netlink_ext_ack *extack);
> void dsa_lag_map(struct dsa_switch_tree *dst, struct net_device *lag);
> void dsa_lag_unmap(struct dsa_switch_tree *dst, struct net_device *lag);
> int dsa_tree_notify(struct dsa_switch_tree *dst, unsigned long e, void *v);
(...)
> -static struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device
> *dev)
> +struct dsa_slave_priv *dsa_slave_dev_lower_find(struct net_device *dev)
> {
> struct netdev_nested_priv priv = {
> .data = NULL,
> };
>
> + if (dsa_slave_dev_check(dev))
> + return netdev_priv(dev);
> +
> netdev_walk_all_lower_dev_rcu(dev, dsa_lower_dev_walk, &priv);
>
> return (struct dsa_slave_priv *)priv.data;
Ah, so that's what you did there. I don't like it. If the function is
called "lower_find" and you come back with "dev" itself, I think that
would qualify as "unexpected". Could you create a new function called
dsa_slave_find_in_lowers_or_self, or something like that, which calls
dsa_slave_dev_lower_find with the extra identity check?
