| Message ID | 20220209211312.7242-2-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: add rtl8_4t tag | expand |
Re: title. Tail or trailing? On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote: > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev, > + char *tag) > { > struct dsa_port *dp = dsa_slave_to_port(dev); > - __be16 *tag; > - > - skb_push(skb, RTL8_4_TAG_LEN); > - > - dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); > - tag = dsa_etype_header_pos_tx(skb); > + __be16 *tag16 = (__be16 *)tag; Can the tail tag be aligned to an odd offset? In that case, should you access byte by byte, maybe? I'm not sure how arches handle this. > > /* Set Realtek EtherType */ > - tag[0] = htons(ETH_P_REALTEK); > + tag16[0] = htons(ETH_P_REALTEK); > > /* Set Protocol; zero REASON */ > - tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); > + tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); > > /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */ > - tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); > + tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); > > /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */ > - tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); > + tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); > +} > + > +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb, > + struct net_device *dev) > +{ > + skb_push(skb, RTL8_4_TAG_LEN); > + > + dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); > + > + rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb)); > > return skb; > } > > -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb, > - struct net_device *dev) > +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb, > + struct net_device *dev) > +{ Why don't you want to add: if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) return NULL; and then you'll make this tagging protocol useful in production too. > + rtl8_4_write_tag(skb, dev, skb_put(skb, RTL8_4_TAG_LEN)); > + > + return skb; > +}
On Wed, Feb 09, 2022 at 11:51:58PM +0200, Vladimir Oltean wrote: > Re: title. Tail or trailing? > > On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote: > > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev, > > + char *tag) > > { > > struct dsa_port *dp = dsa_slave_to_port(dev); > > - __be16 *tag; > > - > > - skb_push(skb, RTL8_4_TAG_LEN); > > - > > - dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); > > - tag = dsa_etype_header_pos_tx(skb); > > + __be16 *tag16 = (__be16 *)tag; > > Can the tail tag be aligned to an odd offset? In that case, should you > access byte by byte, maybe? I'm not sure how arches handle this. You should use get_unaligned_be16(). Andrew
> Re: title. Tail or trailing? I guess I'll stick with trailing. It looks like the most used term. > > On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote: > > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev, > > + char *tag) > > { > > struct dsa_port *dp = dsa_slave_to_port(dev); > > - __be16 *tag; > > - > > - skb_push(skb, RTL8_4_TAG_LEN); > > - > > - dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); > > - tag = dsa_etype_header_pos_tx(skb); > > + __be16 *tag16 = (__be16 *)tag; > > Can the tail tag be aligned to an odd offset? In that case, should you > access byte by byte, maybe? I'm not sure how arches handle this. I see. I'm not sure my big endian MIPS device is able to test it properly. I tried a wide range of payload sizes without any issue. But I believe we need to play safe here. Andrew Lunn, you suggested using get_unaligned_be16(). However, as using get_unaligned_be16() I will already save the tag (or at least part of it) in the stack, could it be a memcpy from stack to the buffer (and the opposite while reading the tag)? It will be less intervention than rewriting the code to deal with each byte at a time. I'm not sure if I need to or can help the compiler optimize it. In my MIPS, it seems it did a good job, replacing the memcpy with four swl/swr instructions (used to write unaligned 32-bit values). > > > > /* Set Realtek EtherType */ > > - tag[0] = htons(ETH_P_REALTEK); > > + tag16[0] = htons(ETH_P_REALTEK); > > > > /* Set Protocol; zero REASON */ > > - tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); > > + tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); > > > > /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */ > > - tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); > > + tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); > > > > /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */ > > - tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); > > + tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); > > +} > > + > > +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb, > > + struct net_device *dev) > > +{ > > + skb_push(skb, RTL8_4_TAG_LEN); > > + > > + dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); > > + > > + rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb)); > > > > return skb; > > } > > > > -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb, > > - struct net_device *dev) > > +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb, > > + struct net_device *dev) > > +{ > > Why don't you want to add: > > if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) > return NULL; > > and then you'll make this tagging protocol useful in production too. It works like a charm! Thanks! And now I have a pretty good use for this new tag: force checksum in software. Whenever the cpu ethernet driver cannot correctly deal with the checksum offloading, switch to rtl8_4t and be happy. It will work either adding 'dsa-tag-protocol = "rtl8_4t";' to the CPU port in the device-tree file or even changing the tag at runtime. Now checksum will only break if you stack two trailing tags and the first one added does not calculate the checksum and the second one is "rtl8_4t". When "rtl8_4t" does calculate the checksum, it will include the other tag in the sum. But it might even be considered a bug in the first tagger. Regards, Luiz
diff --git a/include/net/dsa.h b/include/net/dsa.h index fd1f62a6e0a8..b688ced04b0e 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -52,6 +52,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22 #define DSA_TAG_PROTO_SJA1110_VALUE 23 #define DSA_TAG_PROTO_RTL8_4_VALUE 24 +#define DSA_TAG_PROTO_RTL8_4T_VALUE 25 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -79,6 +80,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE, DSA_TAG_PROTO_SJA1110 = DSA_TAG_PROTO_SJA1110_VALUE, DSA_TAG_PROTO_RTL8_4 = DSA_TAG_PROTO_RTL8_4_VALUE, + DSA_TAG_PROTO_RTL8_4T = DSA_TAG_PROTO_RTL8_4T_VALUE, }; struct dsa_switch; diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c index 02686ad4045d..a9883b0c1d51 100644 --- a/net/dsa/tag_rtl8_4.c +++ b/net/dsa/tag_rtl8_4.c @@ -84,87 +84,123 @@ #define RTL8_4_TX GENMASK(3, 0) #define RTL8_4_RX GENMASK(10, 0) -static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb, - struct net_device *dev) +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev, + char *tag) { struct dsa_port *dp = dsa_slave_to_port(dev); - __be16 *tag; - - skb_push(skb, RTL8_4_TAG_LEN); - - dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); - tag = dsa_etype_header_pos_tx(skb); + __be16 *tag16 = (__be16 *)tag; /* Set Realtek EtherType */ - tag[0] = htons(ETH_P_REALTEK); + tag16[0] = htons(ETH_P_REALTEK); /* Set Protocol; zero REASON */ - tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); + tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB)); /* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */ - tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); + tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1)); /* Zero ALLOW; set RX (CPU->switch) forwarding port mask */ - tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); + tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index))); +} + +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb, + struct net_device *dev) +{ + skb_push(skb, RTL8_4_TAG_LEN); + + dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN); + + rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb)); return skb; } -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb, - struct net_device *dev) +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb, + struct net_device *dev) +{ + rtl8_4_write_tag(skb, dev, skb_put(skb, RTL8_4_TAG_LEN)); + + return skb; +} + +static int rtl8_4_read_tag(struct sk_buff *skb, struct net_device *dev, + char *tag) { - __be16 *tag; u16 etype; u8 reason; u8 proto; u8 port; - - if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN))) - return NULL; - - tag = dsa_etype_header_pos_rx(skb); + __be16 *tag16 = (__be16 *)tag; /* Parse Realtek EtherType */ - etype = ntohs(tag[0]); + etype = ntohs(tag16[0]); if (unlikely(etype != ETH_P_REALTEK)) { dev_warn_ratelimited(&dev->dev, "non-realtek ethertype 0x%04x\n", etype); - return NULL; + return -EPROTO; } /* Parse Protocol */ - proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag[1])); + proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag16[1])); if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) { dev_warn_ratelimited(&dev->dev, "unknown realtek protocol 0x%02x\n", proto); - return NULL; + return -EPROTO; } /* Parse REASON */ - reason = FIELD_GET(RTL8_4_REASON, ntohs(tag[1])); + reason = FIELD_GET(RTL8_4_REASON, ntohs(tag16[1])); /* Parse TX (switch->CPU) */ - port = FIELD_GET(RTL8_4_TX, ntohs(tag[3])); + port = FIELD_GET(RTL8_4_TX, ntohs(tag16[3])); skb->dev = dsa_master_find_slave(dev, 0, port); if (!skb->dev) { dev_warn_ratelimited(&dev->dev, "could not find slave for port %d\n", port); - return NULL; + return -ENOENT; } + if (reason != RTL8_4_REASON_TRAP) + dsa_default_offload_fwd_mark(skb); + + return 0; +} + +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb, + struct net_device *dev) +{ + if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN))) + return NULL; + + if (unlikely(rtl8_4_read_tag(skb, dev, dsa_etype_header_pos_rx(skb)))) + return NULL; + /* Remove tag and recalculate checksum */ skb_pull_rcsum(skb, RTL8_4_TAG_LEN); dsa_strip_etype_header(skb, RTL8_4_TAG_LEN); - if (reason != RTL8_4_REASON_TRAP) - dsa_default_offload_fwd_mark(skb); + return skb; +} + +static struct sk_buff *rtl8_4t_tag_rcv(struct sk_buff *skb, + struct net_device *dev) +{ + if (skb_linearize(skb)) + return NULL; + + if (unlikely(rtl8_4_read_tag(skb, dev, skb_tail_pointer(skb) - RTL8_4_TAG_LEN))) + return NULL; + + if (pskb_trim_rcsum(skb, skb->len - RTL8_4_TAG_LEN)) + return NULL; return skb; } +/* Ethertype version */ static const struct dsa_device_ops rtl8_4_netdev_ops = { .name = "rtl8_4", .proto = DSA_TAG_PROTO_RTL8_4, @@ -172,7 +208,28 @@ static const struct dsa_device_ops rtl8_4_netdev_ops = { .rcv = rtl8_4_tag_rcv, .needed_headroom = RTL8_4_TAG_LEN, }; -module_dsa_tag_driver(rtl8_4_netdev_ops); -MODULE_LICENSE("GPL"); +DSA_TAG_DRIVER(rtl8_4_netdev_ops); + MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4); + +/* Tail version */ +static const struct dsa_device_ops rtl8_4t_netdev_ops = { + .name = "rtl8_4t", + .proto = DSA_TAG_PROTO_RTL8_4T, + .xmit = rtl8_4t_tag_xmit, + .rcv = rtl8_4t_tag_rcv, + .needed_tailroom = RTL8_4_TAG_LEN, +}; + +DSA_TAG_DRIVER(rtl8_4t_netdev_ops); + +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4L); + +static struct dsa_tag_driver *dsa_tag_drivers[] = { + &DSA_TAG_DRIVER_NAME(rtl8_4_netdev_ops), + &DSA_TAG_DRIVER_NAME(rtl8_4t_netdev_ops), +}; +module_dsa_tag_drivers(dsa_tag_drivers); + +MODULE_LICENSE("GPL");
The switch supports the same tag both before ethertype or before CRC. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- include/net/dsa.h | 2 + net/dsa/tag_rtl8_4.c | 119 ++++++++++++++++++++++++++++++++----------- 2 files changed, 90 insertions(+), 31 deletions(-)