From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net-next v2 1/6] net: bridge: Publish bridge accessor functions Date: Fri, 27 Apr 2018 19:11:09 +0300 Message-ID: <5a29ab08-450c-092c-67c2-f3775fe4ca3e@cumulusnetworks.com> References: <20180427151111.22099-1-idosch@mellanox.com> <20180427151111.22099-2-idosch@mellanox.com> <1faecfc8-bf75-6ab4-cb7d-44ef27d75bf6@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Ido Schimmel , netdev@vger.kernel.org, bridge@lists.linux-foundation.org, davem@davemloft.net, jiri@mellanox.com, stephen@networkplumber.org, mlxsw@mellanox.com To: Petr Machata Return-path: Received: from mail-wr0-f196.google.com ([209.85.128.196]:33439 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758632AbeD0QLM (ORCPT ); Fri, 27 Apr 2018 12:11:12 -0400 Received: by mail-wr0-f196.google.com with SMTP id o4-v6so2290646wrm.0 for ; Fri, 27 Apr 2018 09:11:12 -0700 (PDT) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 27/04/18 19:08, Petr Machata wrote: > Nikolay Aleksandrov writes: > >> On 27/04/18 18:11, Ido Schimmel wrote: >>> From: Petr Machata >>> >>> Add a couple new functions to allow querying FDB and vlan settings of a >>> bridge. >>> >>> Signed-off-by: Petr Machata >>> Signed-off-by: Ido Schimmel >>> --- >>> include/linux/if_bridge.h | 28 ++++++++++++++++++++++++++++ >>> net/bridge/br_fdb.c | 22 ++++++++++++++++++++++ >>> net/bridge/br_private.h | 11 +++++++++++ >>> net/bridge/br_vlan.c | 39 +++++++++++++++++++++++++++++++++++++++ >>> 4 files changed, 100 insertions(+) >>> >> >> Thanks! This looks good to me although the new exported helpers could've >> taken both bridge or port and return the result. Usually when adding a >> port-only functions we name them with nbp_ prefix instead of br_. >> >> Anyway this can be done later since the API is internal, > > The idea is that the API would accept both ports and bridges, but > currently there's no user for the parts that I didn't code up--it would > be a dead code. It's super simple to extend these if/when there are > clients, e.g.: > > modified net/bridge/br_vlan.c > @@ -1155,7 +1155,10 @@ int br_vlan_pvid_rtnl(const struct net_device *dev, u16 *p_pvid) > struct net_bridge_vlan_group *vg; > > ASSERT_RTNL(); > - if (netif_is_bridge_master(dev)) > + p = br_port_get_check_rtnl(dev); > + if (p) > + vg = nbp_vlan_group(p); > + else if (netif_is_bridge_master(dev)) > vg = br_vlan_group(netdev_priv(dev)); > else > return -EINVAL; > > I can post a follow-up with the renames if you prefer that. > > Thanks, > Petr > I know, that's why I said it can be done later. :-) No need to do it now, it was only a comment based on other accessors in the bridge code and for completeness. I'm very happy with this version and have acked/reviewed it. Cheers, Nik