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@xxxxxxxxxx> wrote: > > > On Tue, Dec 19, 2023 at 2:07 PM Maxime Chevallier > > <maxime.chevallier@xxxxxxxxxxx> 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-64c269413dfa@xxxxxxxxxx/ > > > > > > 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.
