Re: [PATCH net] dsa: tag_dsa: Handle !CONFIG_BRIDGE_VLAN_FILTERING case

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Sun, Oct 03, 2021 at 05:51:41PM +0200, Andrew Lunn wrote:
> If CONFIG_BRIDGE_VLAN_FILTERING is disabled, br_vlan_enabled() is
> replaced with a stub which returns -EINVAL.

br_vlan_enabled() returns bool, so it cannot hold -EINVAL. The stub for
that returns false. We negate that false, make it true, and then call
br_vlan_get_pvid_rcu() which returns -EINVAL because of _its_ stub
implementation.

> As a result the tagger
> drops the frame. Rather than drop the frame, use a pvid of 0.
> 
> Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/dsa/tag_dsa.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e5127b7d1c6a..8daa8b7787c0 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -149,7 +149,8 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  		 * inject packets to hardware using the bridge's pvid, since
>  		 * that's where the packets ingressed from.
>  		 */
> -		if (!br_vlan_enabled(br)) {
> +		if (IS_ENABLED(CONFIG_BRIDGE_VLAN_FILTERING) &&
> +		    !br_vlan_enabled(br)) {
>  			/* Safe because __dev_queue_xmit() runs under
>  			 * rcu_read_lock_bh()
>  			 */
> -- 
> 2.33.0
> 

So this got me thinking. We shouldn't behave differently when VLAN
filtering is disabled vs when it is disabled and compiled out.

In fact it is actually wrong to inject into the switch using the
bridge's pvid, if VLAN awareness is turned off. We should be able to
send and receive packets in that mode regardless of whether a pvid
exists for the bridge device or not. That is also what we document in
Documentation/networking/switchdev.rst.

So if VLAN 0 does that trick, perfect, we should just delete the entire
"if (!br_vlan_enabled(br))" block.