netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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;
> +	}
> +}
> 


  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).