[PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Linus Walleij]

I noticed that only port 0 worked on the RTL8366RB since we
started to use custom tags.

It turns out that the format of egress custom tags is actually
different from ingress custom tags. While the lower bits just
contain the port number in ingress tags, egress tags need to
indicate destination port by setting the bit for the
corresponding port.

It was working on port 0 because port 0 added 0x00 as port
number in the lower bits, and if you do this the packet gets
broadcasted to all ports, including the intended port.
Ooops.

Fix this and all ports work again.

Tested on the D-Link DIR-685 by sending traffic to each of
the ports in turn. It works.

Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 net/dsa/tag_rtl4_a.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index 57c46b4ab2b3..042a6cb7704a 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -54,9 +54,10 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
 	p = (__be16 *)tag;
 	*p = htons(RTL4_A_ETHERTYPE);
 
-	out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
-	/* The lower bits is the port number */
-	out |= (u8)dp->index;
+	out = (RTL4_A_PROTOCOL_RTL8366RB << RTL4_A_PROTOCOL_SHIFT);
+	/* The lower bits indicate the port number */
+	out |= BIT(dp->index);
+
 	p = (__be16 *)(tag + 2);
 	*p = htons(out);
 
-- 
2.31.1

Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Vladimir Oltean]

On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> I noticed that only port 0 worked on the RTL8366RB since we
> started to use custom tags.
> 
> It turns out that the format of egress custom tags is actually
> different from ingress custom tags. While the lower bits just
> contain the port number in ingress tags, egress tags need to
> indicate destination port by setting the bit for the
> corresponding port.
> 
> It was working on port 0 because port 0 added 0x00 as port
> number in the lower bits, and if you do this the packet gets
> broadcasted to all ports, including the intended port.
> Ooops.

Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
for a regular data packet?

> 
> Fix this and all ports work again.
> 
> Tested on the D-Link DIR-685 by sending traffic to each of
> the ports in turn. It works.
> 
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  net/dsa/tag_rtl4_a.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
> index 57c46b4ab2b3..042a6cb7704a 100644
> --- a/net/dsa/tag_rtl4_a.c
> +++ b/net/dsa/tag_rtl4_a.c
> @@ -54,9 +54,10 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
>  	p = (__be16 *)tag;
>  	*p = htons(RTL4_A_ETHERTYPE);
>  
> -	out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);

What was 2 << 8? This patch changes that part.

> -	/* The lower bits is the port number */
> -	out |= (u8)dp->index;
> +	out = (RTL4_A_PROTOCOL_RTL8366RB << RTL4_A_PROTOCOL_SHIFT);
> +	/* The lower bits indicate the port number */
> +	out |= BIT(dp->index);
> +
>  	p = (__be16 *)(tag + 2);
>  	*p = htons(out);
>  
> -- 
> 2.31.1
> 

Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Linus Walleij]

On Mon, Aug 30, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> > I noticed that only port 0 worked on the RTL8366RB since we
> > started to use custom tags.
> >
> > It turns out that the format of egress custom tags is actually
> > different from ingress custom tags. While the lower bits just
> > contain the port number in ingress tags, egress tags need to
> > indicate destination port by setting the bit for the
> > corresponding port.
> >
> > It was working on port 0 because port 0 added 0x00 as port
> > number in the lower bits, and if you do this the packet gets
> > broadcasted to all ports, including the intended port.
> > Ooops.
>
> Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> for a regular data packet?

It gets broadcast :/

> > -     out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
>
> What was 2 << 8? This patch changes that part.

It was a bit set in the ingress packets, we don't really know
what egress tag bits there are so first I just copied this
and since it turns out the bits in the lower order are not
correct I dropped this too and it works fine.

Do you want me to clarify in the commit message and
resend?

Yours,
Linus Walleij

Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Vladimir Oltean]

On Mon, Aug 30, 2021 at 11:56:22PM +0200, Linus Walleij wrote:
> On Mon, Aug 30, 2021 at 9:29 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > On Sun, Aug 29, 2021 at 01:56:19AM +0200, Linus Walleij wrote:
> > > I noticed that only port 0 worked on the RTL8366RB since we
> > > started to use custom tags.
> > >
> > > It turns out that the format of egress custom tags is actually
> > > different from ingress custom tags. While the lower bits just
> > > contain the port number in ingress tags, egress tags need to
> > > indicate destination port by setting the bit for the
> > > corresponding port.
> > >
> > > It was working on port 0 because port 0 added 0x00 as port
> > > number in the lower bits, and if you do this the packet gets
> > > broadcasted to all ports, including the intended port.
> > > Ooops.
> >
> > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > for a regular data packet?
> 
> It gets broadcast :/

Okay, so a packet sent to a port mask of zero behaves just the same as a
packet sent to a port mask of all ones is what you're saying?
Sounds a bit... implausible?

When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
obviously this includes the possibility of _flooding_, if the MAC
DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
to unknown destination can be practically indistinguishable from a
"broadcast" (the latter having the sense that "you've told the switch to
broadcast this packet to all ports", at least this is what is implied by
the context of your commit message).

The point is that if the destination is not unknown, the packet is not
flooded (or "broadcast" as you say). So "broadcast" would be effectively
a mischaracterization of the behavior.

Just want to make sure that the switch does indeed "broadcast" packets
with a destination port mask of zero. Also curious if by "all ports",
the CPU port itself is also included (effectively looping back the packet)?

> > > -     out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> >
> > What was 2 << 8? This patch changes that part.
> 
> It was a bit set in the ingress packets, we don't really know
> what egress tag bits there are so first I just copied this
> and since it turns out the bits in the lower order are not
> correct I dropped this too and it works fine.
> 
> Do you want me to clarify in the commit message and
> resend?

Well, it is definitely not a logical part of the change. Also, a bug fix
patch that goes to stable kernels seems like the last place to me where
you'd want to change something that you don't really know what it does...
In net-next, this extra change is more than welcome. Possibly has
something to do with hardware address learning on the CPU port, but this
is just a very wild guess based on some other Realtek tagging protocol
drivers I've looked at recently. Anyway, more than likely not just a
random number with no effect.

Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Linus Walleij]

On Tue, Aug 31, 2021 at 12:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:

> > > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > > for a regular data packet?
> >
> > It gets broadcast :/
>
> Okay, so a packet sent to a port mask of zero behaves just the same as a
> packet sent to a port mask of all ones is what you're saying?
> Sounds a bit... implausible?
>
> When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
> obviously this includes the possibility of _flooding_, if the MAC
> DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
> to unknown destination can be practically indistinguishable from a
> "broadcast" (the latter having the sense that "you've told the switch to
> broadcast this packet to all ports", at least this is what is implied by
> the context of your commit message).
>
> The point is that if the destination is not unknown, the packet is not
> flooded (or "broadcast" as you say). So "broadcast" would be effectively
> a mischaracterization of the behavior.

Oh OK sorry what I mean is that the packet appears on all ports of
the switch. Not sent to the broadcast address.

> Just want to make sure that the switch does indeed "broadcast" packets
> with a destination port mask of zero. Also curious if by "all ports",
> the CPU port itself is also included (effectively looping back the packet)?

It does not seem to appear at the CPU port. It appear on ports
0..4.

> > > > -     out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> > >
> > > What was 2 << 8? This patch changes that part.
> >
> > It was a bit set in the ingress packets, we don't really know
> > what egress tag bits there are so first I just copied this
> > and since it turns out the bits in the lower order are not
> > correct I dropped this too and it works fine.
> >
> > Do you want me to clarify in the commit message and
> > resend?
>
> Well, it is definitely not a logical part of the change. Also, a bug fix
> patch that goes to stable kernels seems like the last place to me where
> you'd want to change something that you don't really know what it does...
> In net-next, this extra change is more than welcome. Possibly has
> something to do with hardware address learning on the CPU port, but this
> is just a very wild guess based on some other Realtek tagging protocol
> drivers I've looked at recently. Anyway, more than likely not just a
> random number with no effect.

Yeah but I don't know anything else about it than that it appear
in the ingress packets which are known to have a different
format than the egress packets, the assumption that they were
the same format was wrong ... so it seems best to drop it as well.
But if you insist I can defer that to a separate patch for next.
I just can't see that it has any effect at all.

Yours,
Linus Walleij

Re: [PATCH net] net: dsa: tag_rtl4_a: Fix egress tags

[Vladimir Oltean]

On Tue, Aug 31, 2021 at 08:35:05PM +0200, Linus Walleij wrote:
> On Tue, Aug 31, 2021 at 12:20 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > > > Does it get broadcast, or forwarded by MAC DA/VLAN ID as you'd expect
> > > > for a regular data packet?
> > >
> > > It gets broadcast :/
> >
> > Okay, so a packet sent to a port mask of zero behaves just the same as a
> > packet sent to a port mask of all ones is what you're saying?
> > Sounds a bit... implausible?
> >
> > When I phrased the question whether it gets "forwarded by MAC DA/VLAN ID",
> > obviously this includes the possibility of _flooding_, if the MAC
> > DA/VLAN ID is unknown to the FDB. The behavior of flooding a packet due
> > to unknown destination can be practically indistinguishable from a
> > "broadcast" (the latter having the sense that "you've told the switch to
> > broadcast this packet to all ports", at least this is what is implied by
> > the context of your commit message).
> >
> > The point is that if the destination is not unknown, the packet is not
> > flooded (or "broadcast" as you say). So "broadcast" would be effectively
> > a mischaracterization of the behavior.
> 
> Oh OK sorry what I mean is that the packet appears on all ports of
> the switch. Not sent to the broadcast address.

Yes, but why (due to which hardware decision does this behavior take place)?

I was not hung up on the "broadcast" word. That was used a bit imprecisely,
but I got over that. I was curious as to _why_ would the packets be
delivered to all ports of the switch. After all, you told the switch to
send the packet to _no_ port :-/

The reason why I'm so interested about this is because other switches
(mt7530) treat a destination port mask of 0x0 as "look up the FDB"
(reported by Qingfang here):
https://patchwork.kernel.org/project/netdevbpf/patch/20210825083832.2425886-3-dqfext@gmail.com/#24407683

This means it would be possible to implement the bridge TX forwarding
offload feature:
https://patchwork.kernel.org/project/netdevbpf/cover/20210722155542.2897921-1-vladimir.oltean@nxp.com/

I just wanted to know what type of packets were you testing with. If you
were testing with a unidirectional stream (where the switch has no
opportunity to learn the destination MAC on a particular port), then it
is much more likely that what's happening in your case is that the
packets were flooded, and not simply "broadcast". Pick a different MAC
DA, which _is_ learned in the FDB, and the packets would not be
"broadcast" (actually flooded) at all.

This is still my hypothesis about what was going on.

> > Just want to make sure that the switch does indeed "broadcast" packets
> > with a destination port mask of zero. Also curious if by "all ports",
> > the CPU port itself is also included (effectively looping back the packet)?
> 
> It does not seem to appear at the CPU port. It appear on ports
> 0..4.

Which again would be consistent with my theory.

> > > > > -     out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
> > > >
> > > > What was 2 << 8? This patch changes that part.
> > >
> > > It was a bit set in the ingress packets, we don't really know
> > > what egress tag bits there are so first I just copied this
> > > and since it turns out the bits in the lower order are not
> > > correct I dropped this too and it works fine.
> > >
> > > Do you want me to clarify in the commit message and
> > > resend?
> >
> > Well, it is definitely not a logical part of the change. Also, a bug fix
> > patch that goes to stable kernels seems like the last place to me where
> > you'd want to change something that you don't really know what it does...
> > In net-next, this extra change is more than welcome. Possibly has
> > something to do with hardware address learning on the CPU port, but this
> > is just a very wild guess based on some other Realtek tagging protocol
> > drivers I've looked at recently. Anyway, more than likely not just a
> > random number with no effect.
> 
> Yeah but I don't know anything else about it than that it appear
> in the ingress packets which are known to have a different
> format than the egress packets, the assumption that they were
> the same format was wrong ... so it seems best to drop it as well.
> But if you insist I can defer that to a separate patch for next.
> I just can't see that it has any effect at all.

I was about to say that you can do as you wish, but I see you've resent
already. In general it is good to keep a change to the point, and
documented well and clear.

Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags

[Vladimir Oltean]

On Mon, Mar 01, 2021 at 02:58:59PM +0100, Linus Walleij wrote:
> On Sun, Feb 28, 2021 at 6:08 PM DENG Qingfang <dqfext@gmail.com> wrote:
> 
> > Commit 86dd9868b878 has several issues, but was accepted too soon
> > before anyone could take a look.
> >
> > - Double free. dsa_slave_xmit() will free the skb if the xmit function
> >   returns NULL, but the skb is already freed by eth_skb_pad(). Use
> >   __skb_put_padto() to avoid that.
> > - Unnecessary allocation. It has been done by DSA core since commit
> >   a3b0b6479700.
> > - A u16 pointer points to skb data. It should be __be16 for network
> >   byte order.
> > - Typo in comments. "numer" -> "number".
> >
> > Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> 
> Ooops I send patches before properly going through the mailbox.
> Oh well things like that happen.
> 
> David: ignore my patches to the same tagger and apply this instead!
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij

Last time I checked, performance/timing sensitive code is impacted by
netdev_dbg calls even if dynamic debugging isn't turned on. However,
neither your patches nor Qingfang's have removed that netdev_dbg line.
Is there any good reason to keep it?

Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags

[Linus Walleij]

On Sun, Feb 28, 2021 at 6:08 PM DENG Qingfang <dqfext@gmail.com> wrote:

> Commit 86dd9868b878 has several issues, but was accepted too soon
> before anyone could take a look.
>
> - Double free. dsa_slave_xmit() will free the skb if the xmit function
>   returns NULL, but the skb is already freed by eth_skb_pad(). Use
>   __skb_put_padto() to avoid that.
> - Unnecessary allocation. It has been done by DSA core since commit
>   a3b0b6479700.
> - A u16 pointer points to skb data. It should be __be16 for network
>   byte order.
> - Typo in comments. "numer" -> "number".
>
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>

Ooops I send patches before properly going through the mailbox.
Oh well things like that happen.

David: ignore my patches to the same tagger and apply this instead!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH net] net: dsa: tag_rtl4_a: fix egress tags

[Florian Fainelli]

On 2/28/2021 9:08 AM, DENG Qingfang wrote:
> Commit 86dd9868b878 has several issues, but was accepted too soon
> before anyone could take a look.
> 
> - Double free. dsa_slave_xmit() will free the skb if the xmit function
>   returns NULL, but the skb is already freed by eth_skb_pad(). Use
>   __skb_put_padto() to avoid that.
> - Unnecessary allocation. It has been done by DSA core since commit
>   a3b0b6479700.
> - A u16 pointer points to skb data. It should be __be16 for network
>   byte order.
> - Typo in comments. "numer" -> "number".
> 
> Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net] net: dsa: tag_rtl4_a: fix egress tags

[DENG Qingfang]

Commit 86dd9868b878 has several issues, but was accepted too soon
before anyone could take a look.

- Double free. dsa_slave_xmit() will free the skb if the xmit function
  returns NULL, but the skb is already freed by eth_skb_pad(). Use
  __skb_put_padto() to avoid that.
- Unnecessary allocation. It has been done by DSA core since commit
  a3b0b6479700.
- A u16 pointer points to skb data. It should be __be16 for network
  byte order.
- Typo in comments. "numer" -> "number".

Fixes: 86dd9868b878 ("net: dsa: tag_rtl4_a: Support also egress tags")
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 net/dsa/tag_rtl4_a.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
index c17d39b4a1a0..e9176475bac8 100644
--- a/net/dsa/tag_rtl4_a.c
+++ b/net/dsa/tag_rtl4_a.c
@@ -35,14 +35,12 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	__be16 *p;
 	u8 *tag;
-	u16 *p;
 	u16 out;
 
 	/* Pad out to at least 60 bytes */
-	if (unlikely(eth_skb_pad(skb)))
-		return NULL;
-	if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0)
+	if (unlikely(__skb_put_padto(skb, ETH_ZLEN, false)))
 		return NULL;
 
 	netdev_dbg(dev, "add realtek tag to package to port %d\n",
@@ -53,13 +51,13 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
 	tag = skb->data + 2 * ETH_ALEN;
 
 	/* Set Ethertype */
-	p = (u16 *)tag;
+	p = (__be16 *)tag;
 	*p = htons(RTL4_A_ETHERTYPE);
 
 	out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8);
-	/* The lower bits is the port numer */
+	/* The lower bits is the port number */
 	out |= (u8)dp->index;
-	p = (u16 *)(tag + 2);
+	p = (__be16 *)(tag + 2);
 	*p = htons(out);
 
 	return skb;
-- 
2.25.1