Re: [RFC net-next 3/3] net: dsa: tag_qca: set offload_fwd_mark

[Vladimir Oltean]

On Sat, Aug 07, 2021 at 08:07:26PM +0800, DENG Qingfang wrote:
> As we offload flooding and forwarding, set offload_fwd_mark according to
> Atheros header's type.
> 
> Signed-off-by: DENG Qingfang <dqfext@xxxxxxxxx>
> ---
>  net/dsa/tag_qca.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index 6e3136990491..ee5c1fdfef47 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -50,7 +50,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  {
> -	u8 ver;
> +	u8 ver, type;
>  	u16  hdr;
>  	int port;
>  	__be16 *phdr;
> @@ -82,6 +82,15 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  	if (!skb->dev)
>  		return NULL;
>  
> +	type = (hdr & QCA_HDR_RECV_TYPE_MASK) >> QCA_HDR_RECV_TYPE_S;
> +	switch (type) {
> +	case 0x00: /* Normal packet */
> +	case 0x19: /* Flooding to CPU */
> +	case 0x1a: /* Forwarding to CPU */
> +		dsa_default_offload_fwd_mark(skb);
> +		break;
> +	}
> +
>  	return skb;
>  }
>  
> -- 
> 2.25.1
> 

In this day and age, I consider this commit to be a bug fix, since the
software bridge, seeing an skb with offload_fwd_mark = false on an
offloaded port, will think it hasn't been forwarded and do that job
itself. So all broadcast and multicast traffic flooded to the CPU will
end up being transmitted with duplicates on the other bridge ports.

When the qca8k tagger was added in 2016 in commit cafdc45c949b
("net-next: dsa: add Qualcomm tag RX/TX handler"), the offload_fwd_mark
framework was already there, but no DSA driver was using it - the first
commit I can find that uses offload_fwd_mark in DSA is f849772915e5
("net: dsa: lan9303: lan9303_rcv set skb->offload_fwd_mark") in 2017,
and then quite a few more followed suit. But you could still blame
commit cafdc45c949b.

Curious, I also see that the gswip driver is in the same situation: it
implements .port_bridge_join but does not set skb->offload_fwd_mark.
I've copied Hauke Mehrtens to make him aware. I would rather not send
the patch myself because I would do a rather lousy job and set it
unconditionally to 'true', but the hardware can probably do better in
informing the tagger about whether a frame was received only by the host
or not, since it has an 8 byte header on RX.

For the record, I've checked the other tagging drivers too, to see who
else does not set skb->offload_fwd_mark, and they all correspond to
switch drivers which don't implement .port_bridge_join, which in that
case would be the correct thing to do.