| Message ID | 20210807120726.1063225-4-dqfext@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | qca8k bridge flags offload | expand |
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@gmail.com> > --- > 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.
On Sun, Aug 08, 2021 at 01:57:21AM +0300, Vladimir Oltean wrote: > 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. The driver currently only enables flooding to the CPU port (like MT7530 back then), so offload_fwd_mark should NOT be set until bridge flags offload is supported. > > 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.
On Mon, Aug 09, 2021 at 12:12:24AM +0800, DENG Qingfang wrote: > On Sun, Aug 08, 2021 at 01:57:21AM +0300, Vladimir Oltean wrote: > > 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. > > The driver currently only enables flooding to the CPU port (like MT7530 > back then), so offload_fwd_mark should NOT be set until bridge flags > offload is supported. Ok, I missed that. Please squash this with patch 1 then, please.
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; }
As we offload flooding and forwarding, set offload_fwd_mark according to Atheros header's type. Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- net/dsa/tag_qca.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)