Date:   Sat, 22 Jan 2022 00:49:49 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Alvin Šipraga <ALSI@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple
 cpu ports, non cpu extint

On Fri, Jan 21, 2022 at 06:51:14PM -0300, Luiz Angelo Daros de Luca wrote:
> The code is from the OpenWrt tree.
> https://github.com/openwrt/openwrt/tree/master/target/linux/ramips/files/drivers/net/ethernet/ralink
> 
> I only patched it to accept Jumbo Frames (it was dropping incoming
> packets with MTU 1508)
> https://patchwork.ozlabs.org/project/openwrt/list/?series=279773
> 
> > If that DSA
> > master is a DSA switch itself, could you please unroll the chain all the
> > way with more links to drivers? No matter whether upstream or downstream,
> > just what you use.
> 
> OpenWrt (soc mt7620a) eth0 (mtk_eth_soc) connected to internal SoC
> MT7530 switch port 6 (, mediatek,mt7620-gsw).
> MT7530 port 5 connected to RTL8367S port 7 (RGMII).
> 
> The internal SoC switch is behaving as an unmanaged switch, with no
> vlans. It would be just extra overhead to have it working as a DSA
> switch, specially
> as those two switches tags are not compatible. I still have the
> swconfig driver installed but I was only using it for some debugging
> (checking metrics). I think that the state the bootloader leaves that
> switchis enough to make it forward packets to the Realtek switch. In
> device-tree conf, I'm directly using that eth0 as the CPU port.

There could be value in managing the internal switch with DSA too, for
example in a situation like this:

 +-------------------------------------------------+
 |  SoC                                            |
 |                                                 |
 |  +----------------+--------+---------------+    |
 |  |                |        |               |    |
 |  | Internal       |        |               |    |
 |  |  switch        +--------+               |    |
 |  | (dsa,member = <0 0>;)                   |    |
 |  | +-------+ +-------+ +-------+ +-------+ |    |
 |  | |       | |       | |       | |       | |    |
 |  | | sw0p0 | | sw0p1 | | sw0p2 | | sw0p3 | |    |
 |  | |       | |       | |       | |       | |    |
 +--+-+-------+-+-------+-+-------+-+-------+-+----+

 +----+--------+------------------+
 |    |        |                  |
 |    +--------+                  |
 | External switch                |
 | (dsa,member = <1 0>;)          |
 |  +-------+ +-------+ +-------+ |
 |  |       | |       | |       | |
 |  | sw1p0 | | sw1p1 | | sw1p2 | |
 |  |       | |       | |       | |
 +--+-------+-+-------+-+-------+-+

where you'd create a bridge spanning all of sw0p1, sw0p2, sw0p3, sw1p0,
sw1p1, sw1p2. Forwarding between the internal and the external switch is
done in software, and that deals with the "impedance matching" between
the tagging protocols too - first the packet is stripped of the DSA tag
of the ingress switch, then the DSA tag of the egress switch is added.
With a transparent internal switch (no driver), ports sw0p1, sw0p2,
sw0p3 are dead, since if you'd connect them to a PHY, they'd spit out
DSA-tagged packets from the external switch.

> > I hate to guess, but since both you and Arınç have mentioned the
> > mt7620a/mt7621 SoCs,
> 
> Sorry for the incomplete answer. If it helps, this is my device
> https://github.com/luizluca/openwrt/blob/tplink_c5v4_dsa/target/linux/ramips/dts/mt7620a_tplink_archer-c5-v4.dts
> 
> I try to keep my remote branch updated, although it has some dirty changes:
> https://github.com/luizluca/openwrt/tree/tplink_c5v4_dsa
> 
> > I'd guess that the top-most DSA driver in both cases is "mediatek,eth-mac" (drivers/net/ethernet/mediatek/mtk_eth_soc.c).
> 
> Not in my case. The driver I use also supports mt7621 but the upstream
> driver skipped the mt7620a support.
> 
> > If so, this would confirm my suspicions, since it sets its vlan_features
> > to include NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. Please confirm that
> > master->vlan_features contains these 2 bits.
> 
> Yes.

Ok. See the discussion with Lino Sanfilippo here:
https://lore.kernel.org/netdev/YPAzZXaC%2FEn3s4ly@lunn.ch/
Basically, the moving parts of this mechanism are:

- when the DSA master doesn't understand DSA tags, the transmit
  checksums must be calculated in software.

- this is already supported, we just need to make sure that the DSA
  slave->features does not include any checksum offload bits
  (NETIF_F_HW_CSUM, NETIF_F_IP_CSUM, NETIF_F_IPV6_CSUM), otherwise that
  will be delegated to the device driver. The place that checks that
  condition and calculates the checksum in software is validate_xmit_skb() ->
  skb_csum_hwoffload_help().

- the checksum is evaluated on the skb before the DSA tag is even
  inserted, and is preserved when DSA inserts the header, and is
  therefore still correct by the time the skb reaches the DSA master
  driver. A DSA-unaware master doesn't have to do anything for this
  packet, the IP header checksum will still be correct despite the
  hardware not recognizing the IP header.

- the way DSA populates slave->features is by inheriting master->vlan_features
  (vlan_features means "netdev features which are inheritable by VLAN
  upper interfaces"). This directly makes or breaks what happens in
  validate_xmit_skb() on a DSA slave interface.

- the problem occurs when the DSA master puts checksum offload bits in
  both dev->features and dev->vlan_features. The master thinks this
  means: "I can offload IP checksumming for myself and for VLAN upper
  interfaces (I can recognize the IP header past the VLAN header)."
  Little does it know that DSA assumes this means it can also offload
  checksumming in the presence of switch tags.

So just stop inheriting NETIF_F_HW_CSUM and friends from
master->vlan_features, right?

Well, you can't help but wonder a bit how come it's 2022 and we could
still have an obvious omission like that? And at the same time: but why
does the mt7530 DSA driver work with the same DSA master, but not rtl8365mb?
The answer to both, I think, is "some DSA masters do understand a
particular DSA switch tag, particularly the one from the same vendor".
So if we stop inheriting the checksum offload bits from vlan_features,
we introduce a performance regression for those.

We should instead ask the DSA master "for this DSA tagging protocol,
what netdev features can DSA inherit"? Because of the variability per
tagging protocol, this probably needs to be done through a new netdev
operation, I don't know of any cleaner way.
The complicated part is that we'd need to correctly identify the pairs
of DSA master drivers and tagging protocols where some features can be
safely inherited. Then, it's not too clear whether we want this new ndo
to cover other functionality as well, or if netdev features are enough.

So the sad news for you is that this is pretty much "net-next" material,
even if it fixes what is essentially a design shortcoming. If we're
quick, we could start doing this right as net-next reopens, and that
would give other developers maximum opportunity to fix up the
performance regressions caused by lack of TX checksumming.

> > > Oh, this DSA driver still does not implement vlan nor bridge offload.
> > > Maybe it would matter.
> >
> > It doesn't matter. The vlan_features is a confusing name for what it
> > really does here. I'll explain in a bit once you clarify the other
> > things I asked for.
> 
> That is good news as we can deal with it independently. I wish to
> focus on that afterwards.
> 
> Regards,
> 
> Luiz

Powered by blists - more mailing lists