[PATCH net-next v3 0/2] net: dsa: realtek: add rtl8_4t tag

[PATCH net-next v3 0/2] net: dsa: realtek: add rtl8_4t tag

[Luiz Angelo Daros de Luca]

This series add support for rtl8_4t tag. It is a variant of rtl8_4 tag,
with identical values but placed at the end of the packet (before CRC).

It forces checksum in software before adding the tag as those extra
bytes at the end of the packet would be summed together with the rest of
the payload. When the switch removes the tag before sending the packet
to the network, that checksum will not match.

It might be useful to diagnose or avoid checksum offload issues. With an
ethertype tag like rtl8_4, the cpu port ethernet driver must work with
cksum_start and chksum_offset in order to correctly calculate checksums.
If not, the checksum field will be broken (it will contain the fake ip
header sum).  In those cases, using 'rtl8_4t' might be an alternative
way to avoid checksum offload, either using runtime or device-tree
property.

Regards,

Luiz

v2-v3)
- updated tag documentation (file header)
- do not remove position and format from rtl8365mb_cpu
- reinstate cpu to rtl8365mb
- moved rtl8365mb_change_tag_protocol after rtl8365mb_cpu_config
- do not modify rtl8365mb_cpu_config() logic
- remove cpu arg from rtl8365mb_cpu_config(); get it from priv
- dropped tag_protocol from rtl8365mb. It is now derived from
  cpu->position.
- init cpu struct before dsa_register as default tag must be already
  defined before dsa_register()
- fix formatting issues

v1-v2)
- remove mention to tail tagger, use trailing tagger.
- use void* instead of char* for pointing to tag beginning
- use memcpy to avoid problems with unaligned tags
- calculate checksum if it still pending
- keep in-use tag protocol in memory instead of reading from switch
  register

[PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing variant

[Luiz Angelo Daros de Luca]

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(-)

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");
-- 
2.35.1

[PATCH net-next v3 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Luiz Angelo Daros de Luca]

The trailing tag is also supported by this family. The default is still
rtl8_4 but now the switch supports changing the tag to rtl8_4t.

Reintroduce the dropped cpu in struct rtl8365mb (removed by 6147631).

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 82 +++++++++++++++++++++++------
 1 file changed, 67 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 2ed592147c20..ff865af65d55 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -566,6 +566,7 @@ struct rtl8365mb_port {
  * @chip_ver: chip silicon revision
  * @port_mask: mask of all ports
  * @learn_limit_max: maximum number of L2 addresses the chip can learn
+ * @cpu: CPU tagging and CPU port configuration for this chip
  * @mib_lock: prevent concurrent reads of MIB counters
  * @ports: per-port data
  * @jam_table: chip-specific initialization jam table
@@ -580,6 +581,7 @@ struct rtl8365mb {
 	u32 chip_ver;
 	u32 port_mask;
 	u32 learn_limit_max;
+	struct rtl8365mb_cpu cpu;
 	struct mutex mib_lock;
 	struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
 	const struct rtl8365mb_jam_tbl_entry *jam_table;
@@ -770,6 +772,16 @@ static enum dsa_tag_protocol
 rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
 			   enum dsa_tag_protocol mp)
 {
+	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb_cpu *cpu;
+	struct rtl8365mb *mb;
+
+	mb = priv->chip_data;
+	cpu = &mb->cpu;
+
+	if (cpu->position == RTL8365MB_CPU_POS_BEFORE_CRC)
+		return DSA_TAG_PROTO_RTL8_4T;
+
 	return DSA_TAG_PROTO_RTL8_4;
 }
 
@@ -1725,8 +1737,10 @@ static void rtl8365mb_irq_teardown(struct realtek_priv *priv)
 	}
 }
 
-static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365mb_cpu *cpu)
+static int rtl8365mb_cpu_config(struct realtek_priv *priv)
 {
+	struct rtl8365mb *mb = priv->chip_data;
+	struct rtl8365mb_cpu *cpu = &mb->cpu;
 	u32 val;
 	int ret;
 
@@ -1752,6 +1766,42 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
 	return 0;
 }
 
+static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu_index,
+					 enum dsa_tag_protocol proto)
+{
+	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb_cpu *cpu;
+	struct rtl8365mb *mb;
+	int ret;
+
+	mb = priv->chip_data;
+	cpu = &mb->cpu;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_RTL8_4:
+		cpu->format = RTL8365MB_CPU_FORMAT_8BYTES;
+		cpu->position = RTL8365MB_CPU_POS_AFTER_SA;
+		break;
+	case DSA_TAG_PROTO_RTL8_4T:
+		cpu->format = RTL8365MB_CPU_FORMAT_8BYTES;
+		cpu->position = RTL8365MB_CPU_POS_BEFORE_CRC;
+		break;
+	/* The switch also supports a 4-byte format, similar to rtl4a but with
+	 * the same 0x04 8-bit version and probably 8-bit port source/dest.
+	 * There is no public doc about it. Not supported yet and it will probably
+	 * never be.
+	 */
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	ret = rtl8365mb_cpu_config(priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int rtl8365mb_switch_init(struct realtek_priv *priv)
 {
 	struct rtl8365mb *mb = priv->chip_data;
@@ -1798,13 +1848,14 @@ static int rtl8365mb_reset_chip(struct realtek_priv *priv)
 static int rtl8365mb_setup(struct dsa_switch *ds)
 {
 	struct realtek_priv *priv = ds->priv;
-	struct rtl8365mb_cpu cpu = {0};
+	struct rtl8365mb_cpu *cpu;
 	struct dsa_port *cpu_dp;
 	struct rtl8365mb *mb;
 	int ret;
 	int i;
 
 	mb = priv->chip_data;
+	cpu = &mb->cpu;
 
 	ret = rtl8365mb_reset_chip(priv);
 	if (ret) {
@@ -1827,21 +1878,14 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "no interrupt support\n");
 
 	/* Configure CPU tagging */
-	cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
 	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
-		cpu.mask |= BIT(cpu_dp->index);
+		cpu->mask |= BIT(cpu_dp->index);
 
-		if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
-			cpu.trap_port = cpu_dp->index;
+		if (cpu->trap_port == RTL8365MB_MAX_NUM_PORTS)
+			cpu->trap_port = cpu_dp->index;
 	}
-
-	cpu.enable = cpu.mask > 0;
-	cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
-	cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
-	cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
-	cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
-
-	ret = rtl8365mb_cpu_config(priv, &cpu);
+	cpu->enable = cpu->mask > 0;
+	ret = rtl8365mb_cpu_config(priv);
 	if (ret)
 		goto out_teardown_irq;
 
@@ -1853,7 +1897,7 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 			continue;
 
 		/* Forward only to the CPU */
-		ret = rtl8365mb_port_set_isolation(priv, i, cpu.mask);
+		ret = rtl8365mb_port_set_isolation(priv, i, cpu->mask);
 		if (ret)
 			goto out_teardown_irq;
 
@@ -1983,6 +2027,12 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
 		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
 
+		mb->cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
+		mb->cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
+		mb->cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
+		mb->cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
+		mb->cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
+
 		break;
 	default:
 		dev_err(priv->dev,
@@ -1996,6 +2046,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,
@@ -2014,6 +2065,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,
-- 
2.35.1

Re: [PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing variant

[Alvin Šipraga]

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>

Re: [PATCH net-next v3 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Alvin Šipraga]

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> The trailing tag is also supported by this family. The default is still
> rtl8_4 but now the switch supports changing the tag to rtl8_4t.
>
> Reintroduce the dropped cpu in struct rtl8365mb (removed by 6147631).
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 82 +++++++++++++++++++++++------
>  1 file changed, 67 insertions(+), 15 deletions(-)

Thanks Luiz!

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Re: [PATCH net-next v3 0/2] net: dsa: realtek: add rtl8_4t tag

[Vladimir Oltean]

On Tue, Feb 22, 2022 at 07:47:56PM -0300, Luiz Angelo Daros de Luca wrote:
> In those cases, using 'rtl8_4t' might be an alternative way to avoid
> checksum offload, either using runtime or device-tree property.

Good point. Can you please add one more patch that updates
Documentation/devicetree/bindings/net/dsa/dsa-port.yaml?

Re: [PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing variant

[Vladimir Oltean]

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?

Re: [PATCH net-next v3 0/2] net: dsa: realtek: add rtl8_4t tag

[Luiz Angelo Daros de Luca]

> On Tue, Feb 22, 2022 at 07:47:56PM -0300, Luiz Angelo Daros de Luca wrote:
> > In those cases, using 'rtl8_4t' might be an alternative way to avoid
> > checksum offload, either using runtime or device-tree property.
>
> Good point. Can you please add one more patch that updates
> Documentation/devicetree/bindings/net/dsa/dsa-port.yaml?

Sure

Re: [PATCH net-next v3 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing variant

[Luiz Angelo Daros de Luca]

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