Em qua., 23 de fev. de 2022 às 21:04, Vladimir Oltean
[off-list ref] escreveu:

On Tue, Feb 22, 2022 at 07:47:57PM -0300, Luiz Angelo Daros de Luca wrote:

quoted

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/.

quoted

+ *
+ * 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/

quoted

+ * 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