Message-ID: <20220201144637.l2zmchkoy4pbayxb@skbuf>
Date:   Tue, 1 Feb 2022 16:46:37 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        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>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...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 Mon, Jan 31, 2022 at 02:26:30PM -0300, Luiz Angelo Daros de Luca wrote:
> > > In my case, using an incompatible tailing tag, I just made it work
> > > hacking dsa and forcing slave interfaces to disable offloading. This
> > > way, checksum is calculated before any tag is added and offloading is
> > > skipped. But it is not a real solution.
> >
> > Not sure which one is not a "real solution", but for this specific
> > combination of DSA conduit driver and switch tag, you have to disable
> > checksum offload in the conduit driver and provide it in software. The
> > other way would be to configure the realtek switch to work with
> > DSA_TAG_8021Q and see if you can continue to offload the data path since
> > tagging would use regular 802.1Q vlans, but that means you are going to
> > lose a whole lot of management functionality offered by the native
> > Realtek tag.
> 
> Definitely not a real solution. It was just a hack to check if
> checksumming at slave device will overcome the issue. As I said,
> simply disabling checksum and doing it in SW "as usual" is not enough
> because SW checksum also sums to the end. We need to parse each
> possible transport layer to find its end or taggers must hint how many
> bytes to ignore, something like a new skb->cksum_stop_before_end.
> Another solution would be to hint the slave interface if it needs to
> checksum right there (modifying slave->vlan_features). None of that
> exists today. Is it the right way?

I think we're not getting any closer to a solution if we've started
discussing tail taggers.

See commit 37120f23ac89 ("net: dsa: tag_ksz: dont let the hardware
process the layer 4 checksum"). It proves that if you calculate the L4
checksum in software before inserting the DSA tag, it won't get
recalculated upon dev_queue_xmit() on the DSA master, since
skb_checksum_help() transitions skb->ip_summed to CHECKSUM_NONE, and the
process of inserting a header/trailer will not update the checksum, so
it will end up being correct on the receive end after the tail tag is
stripped.

Otherwise, I don't completely understand what is the end goal you're
after. Each skb is checked for netdev features when determining whether
to calculate the L4 checksum in software or not. Then even if that skb
was marked for L4 checksum offload by the stack, you can still call
skb_checksum_help() from the xmit procedure of the driver.

Do you want hardware offloading with your DSA header, or why do you say
that forcing slave interfaces to disable the offload is not a real
solution? If so, I recommend looking into a custom tagging protocol
based on tag_8021q.c, but word of warning, some elbow grease will be
required.

If you're ok with software checksumming and just want the minimum amount
of checks in the fastpath, I believe you should listen for
NETDEV_CHANGEUPPER events in your DSA master driver, where
dsa_slave_dev_check(info->upper_dev) is true. From there you should be
able to retrieve the tagging protocol used (if you can't, then export some
helpers that will do that), and enable NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
in master->features if the tag is Mediatek, clear them otherwise.
See bcmsysport.c for an example.
The timing of this notifier is such that it's pointless to mangle
master->vlan_features at that stage, since DSA has already inherited
them. So DSA slaves would still report NETIF_F_IP_CSUM, but the DSA
master would force a software calculation from the correct L3 & L4
offsets, and it would practically work.
Alternatively, I think you could move dsa_slave_setup_tagger() beneath
netdev_upper_dev_link(), and this would give the DSA master an
opportunity to modulate its master->vlan_features in a way that is
desirable to you. I don't see something that would break if you do that.

As Florian and Jakub explained, the APIs for TX checksumming are what
they are, I'm not very happy with the state of things either, but I
can't justify a DSA-specific API. With HW_CSUM, the stack gives you an
L3 and L4 offset, and that is compatible with DSA headers (not
trailers), so the onus is on the DSA master to fall back to software on
offsets it doesn't like.  One could argue that DSA should not work with
IP_CSUM | IPV6_CSUM, but I believe that there are existing drivers that
use these checksum features and that do work at least with certain DSA
tagging protocols (bcmsysport) or even look at the L3 and L4 offsets
(mvneta), meaning that they would work generically with DSA. So
practically speaking, if we issue a blanket statement that DSA shouldn't
inherit IP_CSUM | IPV6_CSUM but just HW_CSUM, that would still break
working setups. Now, we could still do that (since IP_CSUM | IPV6_CSUM
are theoretically deprecated), but then you'd have to be there and help
with some more elbow grease to fix the breakage in mvneta etc, to
convert them to HW_CSUM.

Powered by blists - more mailing lists