From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petr Machata Subject: Re: [PATCH net-next 6/6] mlxsw: spectrum_span: Allow bridge for gretap mirror Date: Thu, 26 Apr 2018 16:03:03 +0300 Message-ID: References: <20180426090637.25262-1-idosch@mellanox.com> <20180426090637.25262-7-idosch@mellanox.com> <1e3e120f-821a-2d99-b720-0585ce9c1803@cumulusnetworks.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Ido Schimmel , netdev@vger.kernel.org, bridge@lists.linux-foundation.org, davem@davemloft.net, stephen@networkplumber.org, jiri@mellanox.com, mlxsw@mellanox.com To: Nikolay Aleksandrov Return-path: Received: from mail-ve1eur01on0067.outbound.protection.outlook.com ([104.47.1.67]:39873 "EHLO EUR01-VE1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755207AbeDZND1 (ORCPT ); Thu, 26 Apr 2018 09:03:27 -0400 In-Reply-To: <1e3e120f-821a-2d99-b720-0585ce9c1803@cumulusnetworks.com> (Nikolay Aleksandrov's message of "Thu, 26 Apr 2018 13:50:12 +0300") Sender: netdev-owner@vger.kernel.org List-ID: Nikolay Aleksandrov writes: > On 26/04/18 12:06, Ido Schimmel wrote: >> From: Petr Machata >> >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c >> index 65a77708ff61..92fb81839852 100644 >> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c >> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c >> @@ -167,6 +169,79 @@ mlxsw_sp_span_entry_unoffloadable(struct mlxsw_sp_span_parms *sparmsp) >> return 0; >> } >> +static struct net_device * >> +mlxsw_sp_span_entry_bridge_8021q(const struct net_device *br_dev, >> + unsigned char *dmac, >> + u16 *p_vid) >> +{ >> + struct net_bridge_vlan_group *vg = br_vlan_group_rtnl(br_dev); >> + u16 pvid = br_vlan_group_pvid(vg); >> + struct net_device *edev = NULL; >> + struct net_bridge_vlan *v; >> + > > Hi, > These structures are not really exported anywhere outside of br_private.h > Instead of passing them around and risking someone else actually trying to > dereference an incomplete type, why don't you add just 2 new helpers - > br_vlan_pvid(netdevice) and br_vlan_info(netdevice, vid, *bridge_vlan_info) > > br_vlan_info can return the exported and already available type "bridge_vlan_info" > (defined in uapi/linux/if_bridge.h) into the bridge_vlan_info arg > and br_vlan_pvid is obvious - return the current dev pvid if available. > > Another bonus you'll avoid dealing with the bridge's vlan internals. All right, I'll do it like that. > >> + if (pvid) >> + edev = br_fdb_find_port_hold(br_dev, dmac, pvid); >> + if (!edev) >> + return NULL; >> + >> + /* RTNL prevents edev from being removed. */ >> + dev_put(edev); > > Minor point here - since this is always called in rtnl, the dev_hold/put dance isn't needed, > unless you plan to use the fdb_find_port_hold outside of rtnl of course. In case that is > not planned why don't you do br_fdb_find_port_rtnl() instead with RCU inside (thus not blocking > learning) and ASSERT_RTNL() to avoid some complexity ? OK, sounds good. I'll spin a v2. Thanks, Petr