Linux Ethernet Bridging - Apr 2018 - [Bridge] [PATCH net-next v2 1/6] net: bridge: Publish bridge accessor functions

On 27/04/18 18:11, Ido Schimmel wrote:
> From: Petr Machata <petrm at mellanox.com>
> 
> Add a couple new functions to allow querying FDB and vlan settings of a
> bridge.
> 
> Signed-off-by: Petr Machata <petrm at mellanox.com>
> Signed-off-by: Ido Schimmel <idosch at mellanox.com>
> ---
>  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,

Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

Nikolay Aleksandrov <nikolay at cumulusnetworks.com> writes:
> On 27/04/18 18:11, Ido Schimmel wrote:
>> From: Petr Machata <petrm at mellanox.com>
>> 
>> Add a couple new functions to allow querying FDB and vlan settings of a
>> bridge.
>> 
>> Signed-off-by: Petr Machata <petrm at mellanox.com>
>> Signed-off-by: Ido Schimmel <idosch at mellanox.com>
>> ---
>>  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

On 27/04/18 19:08, Petr Machata wrote:
> Nikolay Aleksandrov <nikolay at cumulusnetworks.com> writes:
> 
>> On 27/04/18 18:11, Ido Schimmel wrote:
>>> From: Petr Machata <petrm at mellanox.com>
>>>
>>> Add a couple new functions to allow querying FDB and vlan settings
of a
>>> bridge.
>>>
>>> Signed-off-by: Petr Machata <petrm at mellanox.com>
>>> Signed-off-by: Ido Schimmel <idosch at mellanox.com>
>>> ---
>>>  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