From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
To: Mike Manning <mmanning@vyatta.att-mail.com>,
netdev@vger.kernel.org, roopa@cumulusnetworks.com
Subject: Re: [PATCH net-next v2 3/5] bridge: support binding vlan dev link state to vlan member bridge ports
Date: Thu, 18 Apr 2019 14:28:09 +0300 [thread overview]
Message-ID: <da18ddc6-f133-2fe0-ae23-8a3142a78778@cumulusnetworks.com> (raw)
In-Reply-To: <20190417181629.5791-4-mmanning@vyatta.att-mail.com>
On 17/04/2019 21:16, Mike Manning wrote:
> In the case of vlan filtering on bridges, the bridge may also have the
> corresponding vlan devices as upper devices. A vlan bridge binding mode
> is added to allow the link state of the vlan device to track only the
> state of the subset of bridge ports that are also members of the vlan,
> rather than that of all bridge ports. This mode is set with a vlan flag
> rather than a bridge sysfs so that the 8021q module is aware that it
> should not set the link state for the vlan device.
>
> If bridge vlan is configured, the bridge device event handling results
> in the link state for an upper device being set, if it is a vlan device
> with the vlan bridge binding mode enabled. This also sets a
> vlan_bridge_binding flag so that subsequent UP/DOWN/CHANGE events for
> the ports in that bridge result in a link state update of the vlan
> device if required.
>
> The link state of the vlan device is up if there is at least one bridge
> port that is a vlan member that is admin & oper up, otherwise its oper
> state is IF_OPER_LOWERLAYERDOWN.
>
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
> net/bridge/br.c | 17 ++++--
> net/bridge/br_private.h | 14 +++++
> net/bridge/br_vlan.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 178 insertions(+), 4 deletions(-)
>
Hi Mike,
One minor comment below.
> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index a5174e5001d8..a9bb5cd962c6 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -40,10 +40,15 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
> bool changed_addr;
> int err;
>
> - /* register of bridge completed, add sysfs entries */
> - if ((dev->priv_flags & IFF_EBRIDGE) && event == NETDEV_REGISTER) {
> - br_sysfs_addbr(dev);
> - return NOTIFY_DONE;
> + if (dev->priv_flags & IFF_EBRIDGE) {
> + if (event == NETDEV_REGISTER) {
> + /* register of bridge completed, add sysfs entries */
> + br_sysfs_addbr(dev);
> + return NOTIFY_DONE;
> + }
> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
> + br_vlan_bridge_event(dev, event, ptr);
> +#endif
Why the ifdef here ? You have this function defined for both cases, one when
configured with vlans and a noop for the no-vlan case.
> }
>
> /* not a port of a bridge */
> @@ -126,6 +131,10 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
> break;
> }
>
> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
> + br_vlan_port_event(p, event);
> +#endif
> +
Same question here.
> /* Events that may cause spanning tree to refresh */
> if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
> event == NETDEV_CHANGE || event == NETDEV_DOWN))
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4bea2f11da9b..334a8c496b50 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -321,6 +321,7 @@ enum net_bridge_opts {
> BROPT_MTU_SET_BY_USER,
> BROPT_VLAN_STATS_PER_PORT,
> BROPT_NO_LL_LEARN,
> + BROPT_VLAN_BRIDGE_BINDING,
> };
>
> struct net_bridge {
> @@ -895,6 +896,9 @@ int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
> int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
> void br_vlan_get_stats(const struct net_bridge_vlan *v,
> struct br_vlan_stats *stats);
> +void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
> +void br_vlan_bridge_event(struct net_device *dev, unsigned long event,
> + void *ptr);
>
> static inline struct net_bridge_vlan_group *br_vlan_group(
> const struct net_bridge *br)
> @@ -1078,6 +1082,16 @@ static inline void br_vlan_get_stats(const struct net_bridge_vlan *v,
> struct br_vlan_stats *stats)
> {
> }
> +
> +static inline void br_vlan_port_event(struct net_bridge_port *p,
> + unsigned long event)
> +{
> +}
> +
> +static inline void br_vlan_bridge_event(struct net_device *dev,
> + unsigned long event, void *ptr)
> +{
> +}
> #endif
>
> struct nf_br_ops {
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 0a02822b5667..b903689a8fc5 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -1264,3 +1264,154 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
> return 0;
> }
> EXPORT_SYMBOL_GPL(br_vlan_get_info);
> +
> +static int br_vlan_is_bind_vlan_dev(const struct net_device *dev)
> +{
> + return is_vlan_dev(dev) &&
> + !!(vlan_dev_priv(dev)->flags & VLAN_FLAG_BRIDGE_BINDING);
> +}
> +
> +static int br_vlan_is_bind_vlan_dev_fn(struct net_device *dev,
> + __always_unused void *data)
> +{
> + return br_vlan_is_bind_vlan_dev(dev);
> +}
> +
> +static bool br_vlan_has_upper_bind_vlan_dev(struct net_device *dev)
> +{
> + int found;
> +
> + rcu_read_lock();
> + found = netdev_walk_all_upper_dev_rcu(dev, br_vlan_is_bind_vlan_dev_fn,
> + NULL);
> + rcu_read_unlock();
> +
> + return !!found;
> +}
> +
> +struct br_vlan_bind_walk_data {
> + u16 vid;
> + struct net_device *result;
> +};
> +
> +static int br_vlan_match_bind_vlan_dev_fn(struct net_device *dev,
> + void *data_in)
> +{
> + struct br_vlan_bind_walk_data *data = data_in;
> + int found = 0;
> +
> + if (br_vlan_is_bind_vlan_dev(dev) &&
> + vlan_dev_priv(dev)->vlan_id == data->vid) {
> + data->result = dev;
> + found = 1;
> + }
> +
> + return found;
> +}
> +
> +static struct net_device *
> +br_vlan_get_upper_bind_vlan_dev(struct net_device *dev, u16 vid)
> +{
> + struct br_vlan_bind_walk_data data = {
> + .vid = vid,
> + };
> +
> + rcu_read_lock();
> + netdev_walk_all_upper_dev_rcu(dev, br_vlan_match_bind_vlan_dev_fn,
> + &data);
> + rcu_read_unlock();
> +
> + return data.result;
> +}
> +
> +static bool br_vlan_is_dev_up(const struct net_device *dev)
> +{
> + return !!(dev->flags & IFF_UP) && netif_oper_up(dev);
> +}
> +
> +static void br_vlan_set_vlan_dev_state(const struct net_bridge *br,
> + struct net_device *vlan_dev)
> +{
> + u16 vid = vlan_dev_priv(vlan_dev)->vlan_id;
> + struct net_bridge_vlan_group *vg;
> + struct net_bridge_port *p;
> + bool has_carrier = false;
> +
> + list_for_each_entry(p, &br->port_list, list) {
> + vg = nbp_vlan_group(p);
> + if (br_vlan_find(vg, vid) && br_vlan_is_dev_up(p->dev)) {
> + has_carrier = true;
> + break;
> + }
> + }
> +
> + if (has_carrier)
> + netif_carrier_on(vlan_dev);
> + else
> + netif_carrier_off(vlan_dev);
> +}
> +
> +static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p)
> +{
> + struct net_bridge_vlan_group *vg = nbp_vlan_group(p);
> + struct net_bridge_vlan *vlan;
> + struct net_device *vlan_dev;
> +
> + list_for_each_entry(vlan, &vg->vlan_list, vlist) {
> + vlan_dev = br_vlan_get_upper_bind_vlan_dev(p->br->dev,
> + vlan->vid);
> + if (vlan_dev) {
> + if (br_vlan_is_dev_up(p->dev))
> + netif_carrier_on(vlan_dev);
> + else
> + br_vlan_set_vlan_dev_state(p->br, vlan_dev);
> + }
> + }
> +}
> +
> +static void br_vlan_upper_change(struct net_device *dev,
> + struct net_device *upper_dev,
> + bool linking)
> +{
> + struct net_bridge *br = netdev_priv(dev);
> +
> + if (!br_vlan_is_bind_vlan_dev(upper_dev))
> + return;
> +
> + if (linking) {
> + br_vlan_set_vlan_dev_state(br, upper_dev);
> + br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
> + } else {
> + br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
> + br_vlan_has_upper_bind_vlan_dev(dev));
> + }
> +}
> +
> +/* Must be protected by RTNL. */
> +void br_vlan_bridge_event(struct net_device *dev, unsigned long event,
> + void *ptr)
> +{
> + struct netdev_notifier_changeupper_info *info;
> +
> + switch (event) {
> + case NETDEV_CHANGEUPPER:
> + info = ptr;
> + br_vlan_upper_change(dev, info->upper_dev, info->linking);
> + break;
> + }
> +}
> +
> +/* Must be protected by RTNL. */
> +void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
> +{
> + if (!br_opt_get(p->br, BROPT_VLAN_BRIDGE_BINDING))
> + return;
> +
> + switch (event) {
> + case NETDEV_CHANGE:
> + case NETDEV_DOWN:
> + case NETDEV_UP:
> + br_vlan_set_all_vlan_dev_state(p);
> + break;
> + }
> +}
>
next prev parent reply other threads:[~2019-04-18 11:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 18:16 [PATCH net-next v2 0/5] net: support binding vlan dev link state to vlan member bridge ports Mike Manning
2019-04-17 18:16 ` [PATCH net-next v2 1/5] vlan: support binding " Mike Manning
2019-04-18 11:25 ` Nikolay Aleksandrov
2019-04-17 18:16 ` [PATCH net-next v2 2/5] vlan: do not transfer link state in vlan bridge binding mode Mike Manning
2019-04-18 11:25 ` Nikolay Aleksandrov
2019-04-17 18:16 ` [PATCH net-next v2 3/5] bridge: support binding vlan dev link state to vlan member bridge ports Mike Manning
2019-04-18 11:28 ` Nikolay Aleksandrov [this message]
2019-04-18 14:42 ` Mike Manning
2019-04-17 18:16 ` [PATCH net-next v2 4/5] bridge: update vlan dev state when port added to or deleted from vlan Mike Manning
2019-04-18 11:41 ` Nikolay Aleksandrov
2019-04-17 18:16 ` [PATCH net-next v2 5/5] bridge: update vlan dev link state for bridge netdev changes Mike Manning
2019-04-18 11:45 ` Nikolay Aleksandrov
2019-04-18 11:43 ` [PATCH net-next v2 0/5] net: support binding vlan dev link state to vlan member bridge ports Nikolay Aleksandrov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=da18ddc6-f133-2fe0-ae23-8a3142a78778@cumulusnetworks.com \
--to=nikolay@cumulusnetworks.com \
--cc=mmanning@vyatta.att-mail.com \
--cc=netdev@vger.kernel.org \
--cc=roopa@cumulusnetworks.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).