On Thu, 26 Apr 2018 13:50:12 +0300 Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> wrote: > On 26/04/18 12:06, Ido Schimmel wrote: > > From: Petr Machata <petrm@xxxxxxxxxxxx> > > > > When handling mirroring to a gretap or ip6gretap netdevice in mlxsw, the > > underlay address (i.e. the remote address of the tunnel) may be routed > > to a bridge. > > > > In that case, look up the resolved neighbor Ethernet address in that > > bridge's FDB. Then configure the offload to direct the mirrored traffic > > to that port, possibly with tagging. > > > > Signed-off-by: Petr Machata <petrm@xxxxxxxxxxxx> > > Signed-off-by: Ido Schimmel <idosch@xxxxxxxxxxxx> > > --- > > .../net/ethernet/mellanox/mlxsw/spectrum_span.c | 102 +++++++++++++++++++-- > > .../net/ethernet/mellanox/mlxsw/spectrum_span.h | 1 + > > 2 files changed, 97 insertions(+), 6 deletions(-) > > > > 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 > > @@ -32,6 +32,7 @@ > > * POSSIBILITY OF SUCH DAMAGE. > > */ > > > > +#include <linux/if_bridge.h> > > #include <linux/list.h> > > #include <net/arp.h> > > #include <net/gre.h> > > @@ -39,8 +40,9 @@ > > #include <net/ip6_tunnel.h> > > > > #include "spectrum.h" > > -#include "spectrum_span.h" > > #include "spectrum_ipip.h" > > +#include "spectrum_span.h" > > +#include "spectrum_switchdev.h" > > > > int mlxsw_sp_span_init(struct mlxsw_sp *mlxsw_sp) > > { > > @@ -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. I am particularly worried that any locking changes will impact the device driver. Also lock inversion issues, and implied (or not RTNL). Also what if a value is changed, there is no notification path back to the device. Having an API that only takes net_device and vlan seems preferable.