On Tue, Dec 19, 2023 at 2:07 PM Maxime Chevallier maxime.chevallier@bootlin.com wrote:

So it looks like an acceptable solution would be something along the lines of what Linus is suggesting here :

https://lore.kernel.org/netdev/20231216-new-gemini-ethernet-regression-v2-2-...

If so, maybe it's worth adding a new helper for that check ?

Yeah it's a bit annoying when skb->protocol is not == ethertype of buffer.

I can certainly add a helper such as skb_eth_raw_ethertype() to <linux/if_ether.h> that will inspect the actual ethertype in skb->data.

It's the most straight-forward approach.

We could also add something like bool custom_ethertype; to struct sk_buff and set that to true if the tagger adds a custom ethertype. But I don't know how the network developers feel about that.

Yours, Linus Walleij

On Tue, Dec 19, 2023 at 05:29:32PM +0100, Maxime Chevallier wrote:

Hi Linus,

On Tue, 19 Dec 2023 15:19:45 +0100 Linus Walleij linus.walleij@linaro.org wrote:

...

On Tue, Dec 19, 2023 at 2:07 PM Maxime Chevallier maxime.chevallier@bootlin.com wrote:

...

So it looks like an acceptable solution would be something along the lines of what Linus is suggesting here :

https://lore.kernel.org/netdev/20231216-new-gemini-ethernet-regression-v2-2-...

If so, maybe it's worth adding a new helper for that check ?

Yeah it's a bit annoying when skb->protocol is not == ethertype of buffer.

I can certainly add a helper such as skb_eth_raw_ethertype() to <linux/if_ether.h> that will inspect the actual ethertype in skb->data.

It's the most straight-forward approach.

Agreed :)

If you rewrite that patch to use skb_vlan_eth_hdr() to get a struct vlan_ethhdr pointer through which h_vlan_proto and h_vlan_encapsulated_proto are accessible, I don't see much value in writing that helper. It is going to beg the question how generic should it be - should it also treat ETH_P_8021AD, should it treat nested VLANs?

At the end of the day, you are trying to cover in software the cases for which the hardware engine can perform TX checksum offloading. That is going to be hardware specific.

...

We could also add something like bool custom_ethertype; to struct sk_buff and set that to true if the tagger adds a custom ethertype. But I don't know how the network developers feel about that.

I don't think this would be OK, first because sk_buff is pretty sensitive when it comes to cache alignment, adding things for this kind of use-cases isn't necessarily a good idea. Moreover, populating this flag isn't going to be straightforward as well. I guess some ethertype would be compatible with checksum engines, while other wouldn't, so probably what 'custom_ethertype' means will depend on the MAC driver.

From my point of view the first approach would indeed be better.

I guess we should first try to answer the questions "what does skb->protocol represent?" and "does DSA use it correctly?" before even thinking about adding yet another fuzzy layer on top it.

On Wed, Dec 20, 2023 at 1:43 AM Linus Walleij linus.walleij@linaro.org wrote:

Then for *this* driver I need to check for the ethertype ETH_P_8021Q what is inside it, one level down, and that is a separate helper. And I named it skb_vlan_raw_inner_ethertype() It will retrieve the inner type no matter

include/linux/if_vlan.h

+/* This determines the inner ethertype incoded into the skb data without

• • relying on skb->protocol which is not always identical.
• */

+static inline u16 skb_vlan_raw_inner_ethertype(const struct sk_buff *skb) +{

•   struct vlan_ethhdr *vhdr;
  

•   if (!skb_pointer_if_linear(skb, 0, VLAN_ETH_HLEN))
  

•           return 0x0000U;
  

•   vhdr = vlan_eth_hdr(skb);
  

•   return ntohs(vhdr->h_vlan_encapsulated_proto);
  

+}

(We can bikeshed the name of the function. *_inner_protocol maybe.)

It does not handle nested VLANs and I don't see why it should since the immediate siblings in if_vlan.h does not, i.e. vlan_eth_hdr(), skb_vlan_eth_hdr(). It's pretty clear these helpers all go just one level down. (We can add a *_descend_*() helper the day someone needs that.)

Forget this whole discussion because in <linux/if_vlan.h> there is already vlan_get_protocol() and vlan_get_protocol_and_depth() so this problem is already solved, just better.

Yours, Linus Walleij

On Fri, Dec 29, 2023 at 05:11:48PM +0100, Romain Gantois wrote:

Thanks for telling me about DSA_LOOP, I've tested several DSA tagging protocols with the RZN1 GMAC1 hardware using this method. Here's what I found in a nutshell:

Good job exploring the complexity of the problem in depth.

For tagging protocols that change the EtherType field in the MAC header (e.g. DSA_TAG_PROTO_(DSA/EDSA/BRCM/MTK/RTL4C_A/SJA1105): On TX the tagged frames are almost always ignored by the checksum offload engine and IP header checker of the MAC device. I say "almost always" because there is an unlikely but nasty corner case where a DSA tag can be identical to an IP EtherType value. In these cases, the frame will likely fail IP header checks and be dropped by the MAC.

Yes, there are a few poorly designed DSA tagging formats where arbitrary fields overlap with what the conduit interface sees as the EtherType field. We don't design the tagging formats, as they are proprietary (except for those derived from tag_8021q), we just support them. In some cases where the switch has permitted that, we have implemented dynamic changing of tagging protocols (like 'echo edsa > /sys/class/net/eth0/dsa/tagging') in order to increase the compatibility between a particular switch and its conduit interface. And where the compatibility with the default tagging protocol was beyond broken, we accepted an alternative one through the 'dsa-tag-protocol' device tree property.

Ignoring these corner cases, the DSA frames will egress with a partial checksum and be dropped by the recipient. On RX, these frames will, once again, not be detected as IP frames by the MAC. So they will be transmitted to the CPU. However, the stmmac driver will assume (wrongly in this case) that these frames' checksums have been verified by the MAC. So it will set CHECKSUM_UNECESSARY:

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/... And so the IP/TCP checksums will not be checked at all, which is not ideal.

Yup, this all stems from the fact that DSA inherits the checksum offload features of the conduit (stmmac) from its vlan_features. People think that vlan_features are inherited only by VLAN upper interfaces, but that is not the case. Confusingly, in some cases, offloading NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM really does work (Broadcom conduit + Broadcom switch, Marvell conduit + Marvell switch, etc), so we can't remove this mechanism. But it uncovers lack of API compliance in drivers such as the stmmac, which is why it is a fragile mechanism.

There are other DSA tagging protocols which cause different issues. For example DSA_TAG_PROTO_BRCM_PREPEND, which seems to offset the whole MAC header, and DSA_TAG_PROTO_LAN9303 which sets ETH_P_8021Q as its EtherType. I haven't dug too deeply on these issues yet, since I'd rather deal with the checksumming issue before getting distracted by VLAN offloading and other stuff.

I agree that what brcm-prepend does - shifting the entire frame to the right by 4 octets - sounds problematic in general (making the conduit see the EtherType as octets [3:2] of the original MAC SA). But you also need to take a look at where those protocols are used, and if that is relevant in any way to the stmmac.

/* Broadcom BCM58xx chips have a flow accelerator on Port 8 * which requires us to use the prepended Broadcom tag type */ if (dev->chip_id == BCM58XX_DEVICE_ID && port == B53_CPU_PORT) { dev->tag_protocol = DSA_TAG_PROTO_BRCM_PREPEND; goto out; }

From what I understand, DSA_TAG_PROTO_BRCM_PREPEND is only used internally within Broadcom SoCs, so it seems likely that it's not designed with generic compatibility in mind.

As for DSA_TAG_PROTO_LAN9303, let me guess what the problem was. TX was fine, but on RX, the packets got dropped in hardware before they even reached the stmmac driver, because it declares NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER as features, and the DSA tags effectively look like unregistered VLAN traffic.

That is certainly an area where the lan9303 support can be improved. Other VLAN-based taggers like tag_8021q perform vlan_vid_add() calls on the conduit interface so that it won't drop the traffic even when it uses hardware VLAN filtering.

Among the tagging protocols I tested, the only one that didn't cause any issues was DSA_TAG_PROTO_TRAILER, which only appends stuff to the frame.

It's very curious that you say this. Tail taggers are notoriously problematic, because while the conduit will perform the checksum offload function on the packets, the checksum calculation goes until the very end of the frame. Thus, that checksum will be wrong after the switch consumes the tail tag (and does not update the L4 checksum).

There is no way to overcome that except to not inherit any checksum offload features for tail taggers. But that would break some other thing, so we opted for having this line in the xmit procedure of tail taggers:

if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) return NULL;

But apparently we have been inconsistent in applying this to trailer_xmit() as well. So DSA_TAG_PROTO_TRAILER should actually be a case of "checksum is computed, but is incorrect after tag stripping", but you say that it was the only one that worked fine.

TLDR: The simplest solution seems to be to modify the stmmac TX and RX paths to disable checksum offloading for frames that have a non-IP ethertype in their MAC header. This will fix the checksum situation for DSA tagging protocols that set non-IP and non-8021Q EtherTypes. Some edge cases like DSA_TAG_PROTO_BRCM_PREPEND and DSA_TAG_PROTO_LAN9303 will require a completely different solution if we want these MAC devices to handle them properly. Please share any thoughts you might have on this suggestion.

I think the overall idea is correct, with the small mentions of "let's ignore brcm-prepend" and "lan9303 should work, maybe it's just a case of disabling the VLAN filtering features through ethtool and testing again?".