| Message ID | 20210828235619.249757-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: tag_rtl4_a: Fix egress tags | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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 >
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
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.
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
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.
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);
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(-)