Re: [PATCH net-next 6/6] mlxsw: spectrum_span: Allow bridge for gretap mirror

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 26 Apr 2018 13:50:12 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> On 26/04/18 12:06, Ido Schimmel wrote:
> > From: Petr Machata <petrm@mellanox.com>
> > 
> > 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@mellanox.com>
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> > ---
> >   .../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.