Re: [RFC PATCH 4/4] net: dsa: tag_edsa: support reception of packets from lag devices

From: Vladimir Oltean <olteanv@gmail.com>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
	netdev@vger.kernel.org
Subject: Re: [RFC PATCH 4/4] net: dsa: tag_edsa: support reception of packets from lag devices
Date: Wed, 28 Oct 2020 14:05:15 +0200	[thread overview]
Message-ID: <20201028120515.gf4yco64qlcwoou2@skbuf> (raw)
In-Reply-To: <20201027105117.23052-5-tobias@waldekranz.com>

On Tue, Oct 27, 2020 at 11:51:17AM +0100, Tobias Waldekranz wrote:
> Packets ingressing on a LAG that egress on the CPU port, which are not
> classified as management, will have a FORWARD tag that does not
> contain the normal source device/port tuple. Instead the trunk bit
> will be set, and the port field holds the LAG id.
> 
> Since the exact source port information is not available in the tag,
> frames are injected directly on the LAG interface and thus do never
> pass through any DSA port interface on ingress.
> 
> Management frames (TO_CPU) are not affected and will pass through the
> DSA port interface as usual.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  net/dsa/dsa.c      | 23 +++++++++++++----------
>  net/dsa/tag_edsa.c | 12 +++++++++++-
>  2 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 2131bf2b3a67..b84e5f0be049 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -220,7 +220,6 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	}
>  
>  	skb = nskb;
> -	p = netdev_priv(skb->dev);
>  	skb_push(skb, ETH_HLEN);
>  	skb->pkt_type = PACKET_HOST;
>  	skb->protocol = eth_type_trans(skb, skb->dev);
> @@ -234,17 +233,21 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  		skb = nskb;
>  	}
>  
> -	s = this_cpu_ptr(p->stats64);
> -	u64_stats_update_begin(&s->syncp);
> -	s->rx_packets++;
> -	s->rx_bytes += skb->len;
> -	u64_stats_update_end(&s->syncp);
> +	if (dsa_slave_dev_check(skb->dev)) {
> +		p = netdev_priv(skb->dev);
> +		s = this_cpu_ptr(p->stats64);
> +		u64_stats_update_begin(&s->syncp);
> +		s->rx_packets++;
> +		s->rx_bytes += skb->len;
> +		u64_stats_update_end(&s->syncp);
>  
> -	if (dsa_skb_defer_rx_timestamp(p, skb))
> -		return 0;
> -
> -	gro_cells_receive(&p->gcells, skb);
> +		if (dsa_skb_defer_rx_timestamp(p, skb))
> +			return 0;
>  
> +		gro_cells_receive(&p->gcells, skb);
> +	} else {
> +		netif_rx(skb);
> +	}
>  	return 0;
>  }
>  

On one hand, I feel pretty yucky about this change.
On the other hand, I wonder if it might be useful under some conditions
for drivers with DSA_TAG_PROTO_NONE? For example, once the user bridges
all slave interfaces, then that bridge will start being able to send and
receive traffic, despite none of the individual switch ports being able
to do that. Then, you could even go off and bridge a "foreign" interface,
and that would again work properly. That use case is not supported today,
but is very useful.

Thoughts?