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

[Petr Machata]

Nikolay Aleksandrov <nikolay at cumulusnetworks.com> writes:

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