| Message ID | 20210216235542.2718128-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 86dd9868b8788a9063893a97649594af93cd5aa6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: tag_rtl4_a: Support also 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 | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| 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 | fail | Errors and warnings before: 0 this patch: 2 |
| 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, 55 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 2 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Wed, 17 Feb 2021 00:55:42 +0100 you wrote: > Support also transmitting frames using the custom "8899 A" > 4 byte tag. > > Qingfang came up with the solution: we need to pad the > ethernet frame to 60 bytes using eth_skb_pad(), then the > switch will happily accept frames with custom tags. > > [...] Here is the summary with links: - net: dsa: tag_rtl4_a: Support also egress tags https://git.kernel.org/netdev/net-next/c/86dd9868b878 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On 2/16/2021 5:00 PM, patchwork-bot+netdevbpf@kernel.org wrote: > Hello: > > This patch was applied to netdev/net-next.git (refs/heads/master): > > On Wed, 17 Feb 2021 00:55:42 +0100 you wrote: >> Support also transmitting frames using the custom "8899 A" >> 4 byte tag. >> >> Qingfang came up with the solution: we need to pad the >> ethernet frame to 60 bytes using eth_skb_pad(), then the >> switch will happily accept frames with custom tags. >> >> [...] > > Here is the summary with links: > - net: dsa: tag_rtl4_a: Support also egress tags > https://git.kernel.org/netdev/net-next/c/86dd9868b878 > > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html Woah not so fast, 1h05 minutes to review a patch, how are we supposed to keep up?
On Wed, Feb 17, 2021 at 12:55:42AM +0100, Linus Walleij wrote: > Support also transmitting frames using the custom "8899 A" > 4 byte tag. > > Qingfang came up with the solution: we need to pad the > ethernet frame to 60 bytes using eth_skb_pad(), then the > switch will happily accept frames with custom tags. > > Cc: Mauri Sandberg <sandberg@mailfence.com> > Reported-by: DENG Qingfang <dqfext@gmail.com> > Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c > index 2646abe5a69e..c17d39b4a1a0 100644 > --- a/net/dsa/tag_rtl4_a.c > +++ b/net/dsa/tag_rtl4_a.c > @@ -12,9 +12,7 @@ > * > * The 2 bytes tag form a 16 bit big endian word. The exact > * meaning has been guessed from packet dumps from ingress > - * frames, as no working egress traffic has been available > - * we do not know the format of the egress tags or if they > - * are even supported. > + * frames. > */ > > #include <linux/etherdevice.h> > @@ -36,17 +34,34 @@ > static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb, > struct net_device *dev) > { > - /* > - * Just let it pass thru, we don't know if it is possible > - * to tag a frame with the 0x8899 ethertype and direct it > - * to a specific port, all attempts at reverse-engineering have > - * ended up with the frames getting dropped. > - * > - * The VLAN set-up needs to restrict the frames to the right port. > - * > - * If you have documentation on the tagging format for RTL8366RB > - * (tag type A) then please contribute. > - */ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + u8 *tag; > + u16 *p; > + u16 out; Reverse Christmass tree. > + > + /* Pad out to at least 60 bytes */ > + if (unlikely(eth_skb_pad(skb))) > + return NULL; The core will do the padding for you. Turn on .tail_tag in dsa_device_ops. > + if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0) > + return NULL; > + > + netdev_dbg(dev, "add realtek tag to package to port %d\n", > + dp->index); You can remove this, now that it works. > + skb_push(skb, RTL4_A_HDR_LEN); > + > + memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN); > + tag = skb->data + 2 * ETH_ALEN; > + > + /* Set Ethertype */ > + p = (u16 *)tag; > + *p = htons(RTL4_A_ETHERTYPE); > + > + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8); Do we know what bit 9 means? Andrew
On Wed, Feb 17, 2021 at 11:29 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > + /* Pad out to at least 60 bytes */ > > + if (unlikely(eth_skb_pad(skb))) > > + return NULL; > > The core will do the padding for you. Turn on .tail_tag in > dsa_device_ops. > Please don't. .tail_tag does far more than just padding.
On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > + if (skb_cow_head(skb, RTL4_A_HDR_LEN) < 0) > + return NULL; skb_cow_head is unnecessary here. DSA core will do it for you. > + > + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8); > + /* The lower bits is the port numer */ Typo? > + out |= (u8)dp->index; > + p = (u16 *)(tag + 2); > + *p = htons(out); > + > return skb; > } > > -- > 2.29.2 >
On Wed, Feb 17, 2021 at 12:55:42AM +0100, Linus Walleij wrote: > Support also transmitting frames using the custom "8899 A" > 4 byte tag. > > Qingfang came up with the solution: we need to pad the > ethernet frame to 60 bytes using eth_skb_pad(), then the > switch will happily accept frames with custom tags. I think it's pretty frustrating to realize that 'it didn't werk' because you only tested with ping. > Cc: Mauri Sandberg <sandberg@mailfence.com> > Reported-by: DENG Qingfang <dqfext@gmail.com> Reported-by seems like a bit of an understatement. Suggested-by maybe? > Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 14 deletions(-) > > diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c > index 2646abe5a69e..c17d39b4a1a0 100644 > --- a/net/dsa/tag_rtl4_a.c > +++ b/net/dsa/tag_rtl4_a.c > @@ -12,9 +12,7 @@ > * > * The 2 bytes tag form a 16 bit big endian word. The exact > * meaning has been guessed from packet dumps from ingress > - * frames, as no working egress traffic has been available > - * we do not know the format of the egress tags or if they > - * are even supported. > + * frames. > */ > > #include <linux/etherdevice.h> > @@ -36,17 +34,34 @@ > static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb, > struct net_device *dev) > { > - /* > - * Just let it pass thru, we don't know if it is possible > - * to tag a frame with the 0x8899 ethertype and direct it > - * to a specific port, all attempts at reverse-engineering have > - * ended up with the frames getting dropped. > - * > - * The VLAN set-up needs to restrict the frames to the right port. > - * > - * If you have documentation on the tagging format for RTL8366RB > - * (tag type A) then please contribute. > - */ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + 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) > + return NULL; > + > + netdev_dbg(dev, "add realtek tag to package to port %d\n", > + dp->index); You should probably remove any sort of printing from the hot path. > + skb_push(skb, RTL4_A_HDR_LEN); > + > + memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN); > + tag = skb->data + 2 * ETH_ALEN; > + > + /* Set Ethertype */ > + p = (u16 *)tag; > + *p = htons(RTL4_A_ETHERTYPE); > + > + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8); > + /* The lower bits is the port numer */ > + out |= (u8)dp->index; > + p = (u16 *)(tag + 2); > + *p = htons(out); > + > return skb; > } > > -- > 2.29.2 >
On Wed, Feb 17, 2021 at 01:37:21PM +0800, DENG Qingfang wrote: > On Wed, Feb 17, 2021 at 11:29 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > + /* Pad out to at least 60 bytes */ > > > + if (unlikely(eth_skb_pad(skb))) > > > + return NULL; > > > > The core will do the padding for you. Turn on .tail_tag in > > dsa_device_ops. > > > > Please don't. .tail_tag does far more than just padding. Qingfang is right, this is an EtherType tagging protocol not a tail tagger. Note that there was an interesting discussion with Florian and Kuba around the Broadcom switches that require padding for short frames too: https://patchwork.kernel.org/project/netdevbpf/patch/20201101191620.589272-10-vladimir.oltean@nxp.com/
On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > + > + /* Pad out to at least 60 bytes */ > + if (unlikely(eth_skb_pad(skb))) > + return NULL; I just found that this will cause double free (eth_skb_pad will free the skb if allocation fails, and dsa_slave_xmit will still try to free it because it returns NULL. Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to avoid that.
On Wed, Feb 17, 2021 at 08:38:30PM +0800, DENG Qingfang wrote: > On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > + > > + /* Pad out to at least 60 bytes */ > > + if (unlikely(eth_skb_pad(skb))) > > + return NULL; > > I just found that this will cause double free (eth_skb_pad will free > the skb if allocation fails, and dsa_slave_xmit will still try to free > it because it returns NULL. > Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to > avoid that. Good catch, Qingfang.
On 17.02.2021 13:42, Vladimir Oltean wrote: > On Wed, Feb 17, 2021 at 08:38:30PM +0800, DENG Qingfang wrote: >> On Wed, Feb 17, 2021 at 7:55 AM Linus Walleij <linus.walleij@linaro.org> wrote: >>> + >>> + /* Pad out to at least 60 bytes */ >>> + if (unlikely(eth_skb_pad(skb))) >>> + return NULL; >> >> I just found that this will cause double free (eth_skb_pad will free >> the skb if allocation fails, and dsa_slave_xmit will still try to free >> it because it returns NULL. >> Replace eth_skb_pad(skb) with __skb_put_padto(skb, ETH_ZLEN, false) to >> avoid that. > That's really a pitfall. I had to do the same in r8169: cc6528bc9a0c ("r8169: fix potential skb double free in an error path") > Good catch, Qingfang. >
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c index 2646abe5a69e..c17d39b4a1a0 100644 --- a/net/dsa/tag_rtl4_a.c +++ b/net/dsa/tag_rtl4_a.c @@ -12,9 +12,7 @@ * * The 2 bytes tag form a 16 bit big endian word. The exact * meaning has been guessed from packet dumps from ingress - * frames, as no working egress traffic has been available - * we do not know the format of the egress tags or if they - * are even supported. + * frames. */ #include <linux/etherdevice.h> @@ -36,17 +34,34 @@ static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb, struct net_device *dev) { - /* - * Just let it pass thru, we don't know if it is possible - * to tag a frame with the 0x8899 ethertype and direct it - * to a specific port, all attempts at reverse-engineering have - * ended up with the frames getting dropped. - * - * The VLAN set-up needs to restrict the frames to the right port. - * - * If you have documentation on the tagging format for RTL8366RB - * (tag type A) then please contribute. - */ + struct dsa_port *dp = dsa_slave_to_port(dev); + 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) + return NULL; + + netdev_dbg(dev, "add realtek tag to package to port %d\n", + dp->index); + skb_push(skb, RTL4_A_HDR_LEN); + + memmove(skb->data, skb->data + RTL4_A_HDR_LEN, 2 * ETH_ALEN); + tag = skb->data + 2 * ETH_ALEN; + + /* Set Ethertype */ + p = (u16 *)tag; + *p = htons(RTL4_A_ETHERTYPE); + + out = (RTL4_A_PROTOCOL_RTL8366RB << 12) | (2 << 8); + /* The lower bits is the port numer */ + out |= (u8)dp->index; + p = (u16 *)(tag + 2); + *p = htons(out); + return skb; }
Support also transmitting frames using the custom "8899 A" 4 byte tag. Qingfang came up with the solution: we need to pad the ethernet frame to 60 bytes using eth_skb_pad(), then the switch will happily accept frames with custom tags. Cc: Mauri Sandberg <sandberg@mailfence.com> Reported-by: DENG Qingfang <dqfext@gmail.com> Fixes: efd7fe68f0c6 ("net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- net/dsa/tag_rtl4_a.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-)