From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6267C10F0E for ; Thu, 18 Apr 2019 14:43:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 972E9206B6 for ; Thu, 18 Apr 2019 14:43:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388879AbfDROnE (ORCPT ); Thu, 18 Apr 2019 10:43:04 -0400 Received: from mx0a-00191d01.pphosted.com ([67.231.149.140]:47660 "EHLO mx0a-00191d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388277AbfDROnD (ORCPT ); Thu, 18 Apr 2019 10:43:03 -0400 Received: from pps.filterd (m0053301.ppops.net [127.0.0.1]) by mx0a-00191d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x3IERPO9015124; Thu, 18 Apr 2019 10:43:00 -0400 Received: from tlpd255.enaf.dadc.sbc.com (sbcsmtp3.sbc.com [144.160.112.28]) by mx0a-00191d01.pphosted.com with ESMTP id 2rxrh0vufe-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 18 Apr 2019 10:42:58 -0400 Received: from enaf.dadc.sbc.com (localhost [127.0.0.1]) by tlpd255.enaf.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id x3IEgWua073288; Thu, 18 Apr 2019 09:42:33 -0500 Received: from zlp30496.vci.att.com (zlp30496.vci.att.com [135.46.181.157]) by tlpd255.enaf.dadc.sbc.com (8.14.5/8.14.5) with ESMTP id x3IEgU7O073226 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 18 Apr 2019 09:42:30 -0500 Received: from zlp30496.vci.att.com (zlp30496.vci.att.com [127.0.0.1]) by zlp30496.vci.att.com (Service) with ESMTP id 0789A400B573; Thu, 18 Apr 2019 14:42:30 +0000 (GMT) Received: from clpi183.sldc.sbc.com (unknown [135.41.1.46]) by zlp30496.vci.att.com (Service) with ESMTP id D6FEA400B570; Thu, 18 Apr 2019 14:42:29 +0000 (GMT) Received: from sldc.sbc.com (localhost [127.0.0.1]) by clpi183.sldc.sbc.com (8.14.5/8.14.5) with ESMTP id x3IEgTT1007012; Thu, 18 Apr 2019 09:42:29 -0500 Received: from mail.eng.vyatta.net (mail.eng.vyatta.net [10.156.50.82]) by clpi183.sldc.sbc.com (8.14.5/8.14.5) with ESMTP id x3IEgLq0006198; Thu, 18 Apr 2019 09:42:21 -0500 Received: from [10.156.47.194] (unknown [10.156.47.194]) by mail.eng.vyatta.net (Postfix) with ESMTPA id 7873136005E; Thu, 18 Apr 2019 07:42:20 -0700 (PDT) Reply-To: mmanning@vyatta.att-mail.com Subject: Re: [PATCH net-next v2 3/5] bridge: support binding vlan dev link state to vlan member bridge ports To: Nikolay Aleksandrov , netdev@vger.kernel.org, roopa@cumulusnetworks.com References: <20190417181629.5791-1-mmanning@vyatta.att-mail.com> <20190417181629.5791-4-mmanning@vyatta.att-mail.com> From: Mike Manning Message-ID: Date: Thu, 18 Apr 2019 15:42:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-04-18_07:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_policy_notspam policy=outbound_policy score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1904180098 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 18/04/2019 12:28, Nikolay Aleksandrov wrote: > 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 >> --- >> 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. Hi Nikolay, thank you very much for the review, I will annotate v3 appropriately. You are quite right, there is no need for these ugly inline #ifdef, as I followed your example of providing stubs 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; >> + } >> +} >>