| Message ID | 20220222224758.11324-2-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: add rtl8_4t tag | expand |
Luiz Angelo Daros de Luca <luizluca@gmail.com> writes: > Realtek switches supports the same tag both before ethertype or between > payload and the CRC. > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > include/net/dsa.h | 2 + > net/dsa/tag_rtl8_4.c | 154 +++++++++++++++++++++++++++++++++---------- > 2 files changed, 121 insertions(+), 35 deletions(-) Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>
On Tue, Feb 22, 2022 at 07:47:57PM -0300, Luiz Angelo Daros de Luca wrote: > diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c > index 02686ad4045d..2e81ab49d928 100644 > --- a/net/dsa/tag_rtl8_4.c > +++ b/net/dsa/tag_rtl8_4.c > @@ -9,11 +9,6 @@ > * > * This tag header has the following format: > * > - * ------------------------------------------- > - * | MAC DA | MAC SA | 8 byte tag | Type | ... > - * ------------------------------------------- > - * _______________/ \______________________________________ > - * / \ > * 0 7|8 15 > * |-----------------------------------+-----------------------------------|--- > * | (16-bit) | ^ > @@ -58,6 +53,28 @@ > * TX/RX | TX (switch->CPU): port number the packet was received on > * | RX (CPU->switch): forwarding port mask (if ALLOW=0) > * | allowance port mask (if ALLOW=1) > + * > + * The tag can be positioned before Ethertype, using tag "rtl8_4": > + * > + * +--------+--------+------------+------+----- > + * | MAC DA | MAC SA | 8 byte tag | Type | ... > + * +--------+--------+------------+------+----- > + * > + * If checksum offload is enabled for CPU port device, it might break if the > + * driver does not use csum_start/csum_offset. Please. This is true of any DSA header. If you feel you have something to add on this topic please do so in Documentation/networking/dsa/dsa.rst under "Switch tagging protocols". Also, s/CPU port device/DSA master/. > + * > + * The tag can also appear between the end of the payload and before the CRC, > + * using tag "rtl8_4t": > + * > + * +--------+--------+------+-----+---------+------------+-----+ > + * | MAC DA | MAC SA | TYPE | ... | payload | 8-byte tag | CRC | > + * +--------+--------+------+-----+---------+------------+-----+ > + * > + * The added bytes after the payload will break most checksums, either in > + * software or hardware. To avoid this issue, if the checksum is still pending, > + * this tagger checksum the packet before adding the tag, rendering any s/checksum/checksums/ > + * checksum offload useless. If you're adding a tail tagging driver to work around checksum offload issues, this solution is about as bad as it gets. You're literally not gaining anything in performance over fixing your DSA master driver to turn off checksum offloading for unrecognized DSA tagging protocols. And on top of that, you're requiring your users to be aware of this issue and make changes to their configuration, for something that can be done automatically. Do you have another use case as well?
Em qua., 23 de fev. de 2022 às 21:04, Vladimir Oltean <olteanv@gmail.com> escreveu: > > On Tue, Feb 22, 2022 at 07:47:57PM -0300, Luiz Angelo Daros de Luca wrote: > > diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c > > index 02686ad4045d..2e81ab49d928 100644 > > --- a/net/dsa/tag_rtl8_4.c > > +++ b/net/dsa/tag_rtl8_4.c > > @@ -9,11 +9,6 @@ > > * > > * This tag header has the following format: > > * > > - * ------------------------------------------- > > - * | MAC DA | MAC SA | 8 byte tag | Type | ... > > - * ------------------------------------------- > > - * _______________/ \______________________________________ > > - * / \ > > * 0 7|8 15 > > * |-----------------------------------+-----------------------------------|--- > > * | (16-bit) | ^ > > @@ -58,6 +53,28 @@ > > * TX/RX | TX (switch->CPU): port number the packet was received on > > * | RX (CPU->switch): forwarding port mask (if ALLOW=0) > > * | allowance port mask (if ALLOW=1) > > + * > > + * The tag can be positioned before Ethertype, using tag "rtl8_4": > > + * > > + * +--------+--------+------------+------+----- > > + * | MAC DA | MAC SA | 8 byte tag | Type | ... > > + * +--------+--------+------------+------+----- > > + * > > + * If checksum offload is enabled for CPU port device, it might break if the > > + * driver does not use csum_start/csum_offset. > > Please. This is true of any DSA header. If you feel you have something > to add on this topic please do so in Documentation/networking/dsa/dsa.rst > under "Switch tagging protocols". OK. I'll remove it from this series and add that info to docs in an independent commit. > Also, s/CPU port device/DSA master/. > > > + * > > + * The tag can also appear between the end of the payload and before the CRC, > > + * using tag "rtl8_4t": > > + * > > + * +--------+--------+------+-----+---------+------------+-----+ > > + * | MAC DA | MAC SA | TYPE | ... | payload | 8-byte tag | CRC | > > + * +--------+--------+------+-----+---------+------------+-----+ > > + * > > + * The added bytes after the payload will break most checksums, either in > > + * software or hardware. To avoid this issue, if the checksum is still pending, > > + * this tagger checksum the packet before adding the tag, rendering any > > s/checksum/checksums/ > > > + * checksum offload useless. > > If you're adding a tail tagging driver to work around checksum offload > issues, this solution is about as bad as it gets. You're literally not > gaining anything in performance over fixing your DSA master driver to > turn off checksum offloading for unrecognized DSA tagging protocols. I wasn't adding it as a way to disable offload but as an alternative to keep the hardware offload enabled. However, in the end, if the HW already does not understand the tag, adding a tag after the payload will not only break checksum offload but the software checksum as well. > And on top of that, you're requiring your users to be aware of this > issue and make changes to their configuration, for something that can be > done automatically. I don't see how we could automatically detect that in an unspecific Ethernet driver. No driver expects to have some bytes after the payload it should ignore. Not even the software checksum functions consider that case. And we should not adapt all Ethernet drivers for DSA. The easier solution is to calculate the checksum before the tag was added, even if that prevents checksum offload for a possible compatible HW. After the tag was added, it is already too late for the driver to make a decision (with existing Linux code). > Do you have another use case as well? Allowing the user to change the tag position is a nice tool to detect compatible problems. I got the checksum offload but it could also help with stacking incompatible switches, when the switch does not like the added Ethertype DSA tag. 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..2e81ab49d928 100644 --- a/net/dsa/tag_rtl8_4.c +++ b/net/dsa/tag_rtl8_4.c @@ -9,11 +9,6 @@ * * This tag header has the following format: * - * ------------------------------------------- - * | MAC DA | MAC SA | 8 byte tag | Type | ... - * ------------------------------------------- - * _______________/ \______________________________________ - * / \ * 0 7|8 15 * |-----------------------------------+-----------------------------------|--- * | (16-bit) | ^ @@ -58,6 +53,28 @@ * TX/RX | TX (switch->CPU): port number the packet was received on * | RX (CPU->switch): forwarding port mask (if ALLOW=0) * | allowance port mask (if ALLOW=1) + * + * The tag can be positioned before Ethertype, using tag "rtl8_4": + * + * +--------+--------+------------+------+----- + * | MAC DA | MAC SA | 8 byte tag | Type | ... + * +--------+--------+------------+------+----- + * + * If checksum offload is enabled for CPU port device, it might break if the + * driver does not use csum_start/csum_offset. + * + * The tag can also appear between the end of the payload and before the CRC, + * using tag "rtl8_4t": + * + * +--------+--------+------+-----+---------+------------+-----+ + * | MAC DA | MAC SA | TYPE | ... | payload | 8-byte tag | CRC | + * +--------+--------+------+-----+---------+------------+-----+ + * + * The added bytes after the payload will break most checksums, either in + * software or hardware. To avoid this issue, if the checksum is still pending, + * this tagger checksum the packet before adding the tag, rendering any + * checksum offload useless. + * */ #include <linux/bitfield.h> @@ -84,87 +101,133 @@ #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 void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev, + void *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[RTL8_4_TAG_LEN / 2]; /* 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))); + + memcpy(tag, tag16, RTL8_4_TAG_LEN); +} + +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) { - __be16 *tag; + /* Calculate the checksum here if not done yet as trailing tags will + * break either software and hardware based checksum + */ + if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) + return NULL; + + 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, + void *tag) +{ + __be16 tag16[RTL8_4_TAG_LEN / 2]; 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); + memcpy(tag16, tag, RTL8_4_TAG_LEN); /* 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 +235,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");
Realtek switches supports the same tag both before ethertype or between payload and the CRC. Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- include/net/dsa.h | 2 + net/dsa/tag_rtl8_4.c | 154 +++++++++++++++++++++++++++++++++---------- 2 files changed, 121 insertions(+), 35 deletions(-)