[PATCH net 0/1] Prevent DSA tags from breaking COE

[PATCH net 0/1] Prevent DSA tags from breaking COE

[Romain Gantois]

Hello everyone,

This is a bugfix for an issue that was recently brought up in two
reports:

https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/

The Checksum Offloading Engine of some stmmac cores (e.g. DWMAC1000)
computes an incorrect checksum when presented with DSA-tagged packets. This
causes all TCP/UDP transfers to break when the stmmac device is connected
to the CPU port of a DSA switch.

The main change introduced by this series is a new stmmac dma feature that
stmmac_mac_link_up() can check to detect cores that have DSA-incompatible
COEs. If the flag is set and the netdevice uses DSA, stmmac_xmit() will
complete checksumming in software instead of offloading it.

I've run some iperf3 tests and the TX hotpath performance doesn't seem
to be degraded by the field added to dma_features.

Best Regards,

Romain

Romain Gantois (1):
  net: stmmac: Prevent DSA tags from breaking COE

 drivers/net/ethernet/stmicro/stmmac/common.h     |  1 +
 .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c  |  1 +
 .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 +++++++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Romain Gantois]

Some stmmac cores have Checksum Offload Engines that cannot handle DSA tags
properly. These cores find the IP/TCP headers on their own and end up
computing an incorrect checksum when a DSA tag is inserted between the MAC
header and IP header.

Add an additional check on stmmac link up so that COE is deactivated
when the stmmac device is used as a DSA conduit.

Add a new dma_feature flag to allow cores to signal that their COEs can't
handle DSA tags on TX.

Fixes: 6b2c6e4a938f ("net: stmmac: propagate feature flags to vlan")
Cc: stable@vger.kernel.org
Reported-by: Richard Tresidder <rtresidd@electromag.com.au>
Closes: https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
Reported-by: Romain Gantois <romain.gantois@bootlin.com>
Closes: https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h     |  1 +
 .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c  |  1 +
 .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 +++++++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index a381dee8b52d..1469d95e77a0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -406,6 +406,7 @@ struct dma_features {
 	unsigned int rx_coe_type1;
 	unsigned int rx_coe_type2;
 	unsigned int rxfifo_over_2048;
+	unsigned int dsa_breaks_tx_coe;
 	/* TX and RX number of channels */
 	unsigned int number_rx_channel;
 	unsigned int number_tx_channel;
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
index daf79cdbd3ec..50686cdc3320 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
@@ -264,6 +264,7 @@ static int dwmac1000_get_hw_feature(void __iomem *ioaddr,
 	dma_cap->number_tx_channel = (hw_cap & DMA_HW_FEAT_TXCHCNT) >> 22;
 	/* Alternate (enhanced) DESC mode */
 	dma_cap->enh_desc = (hw_cap & DMA_HW_FEAT_ENHDESSEL) >> 24;
+	dma_cap->dsa_breaks_tx_coe = 1;
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a9b6b383e863..733348c65e04 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -42,6 +42,7 @@
 #include <net/page_pool/helpers.h>
 #include <net/pkt_cls.h>
 #include <net/xdp_sock_drv.h>
+#include <net/dsa.h>
 #include "stmmac_ptp.h"
 #include "stmmac.h"
 #include "stmmac_xdp.h"
@@ -993,8 +994,11 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 			       int speed, int duplex,
 			       bool tx_pause, bool rx_pause)
 {
-	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	unsigned int tx_queue_cnt;
 	u32 old_ctrl, ctrl;
+	int queue;
 
 	if ((priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP) &&
 	    priv->plat->serdes_powerup)
@@ -1102,6 +1106,16 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	if (priv->plat->flags & STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY)
 		stmmac_hwtstamp_correct_latency(priv, priv);
+
+	/* DSA tags break COEs on some cores. Disable TX checksum
+	 * offloading on those cores if the netdevice is a DSA conduit.
+	 */
+	if (priv->dma_cap.dsa_breaks_tx_coe && netdev_uses_dsa(ndev)) {
+		tx_queue_cnt = priv->plat->tx_queues_to_use;
+		for (queue = 0; queue < tx_queue_cnt; queue++)
+			priv->plat->tx_queues_cfg[queue].coe_unsupported = true;
+	}
+
 }
 
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
-- 
2.43.0

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Andrew Lunn]

On Mon, Dec 18, 2023 at 05:23:22PM +0100, Romain Gantois wrote:
> Hello everyone,
> 
> This is a bugfix for an issue that was recently brought up in two
> reports:
> 
> https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> 
> The Checksum Offloading Engine of some stmmac cores (e.g. DWMAC1000)
> computes an incorrect checksum when presented with DSA-tagged packets. This
> causes all TCP/UDP transfers to break when the stmmac device is connected
> to the CPU port of a DSA switch.

Probably a dumb question.... Does this COE also perform checksum
validation on receive? Is it also getting confused by the DSA header?

You must of tested receive, so it works somehow, but i just wounder if
something needs to be done to be on the safe side?

	  Andrew

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Romain Gantois]

On Mon, 18 Dec 2023, Andrew Lunn wrote:
...
> Probably a dumb question.... Does this COE also perform checksum
> validation on receive? Is it also getting confused by the DSA header?
> 
> You must of tested receive, so it works somehow, but i just wounder if
> something needs to be done to be on the safe side?

That's a good point, I just investigated the RX path a bit more and the MAC 
indeed has IP/TCP/UDP RX checksum offloading enabled. However, the 
external switch in my setup uses EDSA tags, which displace the "true" ethertype 
field to the end of the DSA header and replaces the "normal" ethertype with 
ETH_P_EDSA (0xdada). So to the MAC controller, the ethernet frame has an unknown 
ethertype, and so it doesn't see it as an IP frame at all. All of the 
ethtool counters related to IP stuff are at 0, which supports this.

This explains why checksum offloading doesn't break the RX path in my case. 
However, other maybe other DSA switches using different frame formats could 
cause different behavior? Considering this, I think it would be safer to change 
the dsa_breaks_tx_coe flag to a general dsa_breaks_coe flag. It makes sense to 
me to assume that if DSA tags break TX COE, then RX COE will also not work.

I'll take this into account when I send a v2.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Vladimir Oltean]

Hi Romain,

On Mon, Dec 18, 2023 at 05:23:23PM +0100, Romain Gantois wrote:
> Some stmmac cores have Checksum Offload Engines that cannot handle DSA tags
> properly. These cores find the IP/TCP headers on their own and end up
> computing an incorrect checksum when a DSA tag is inserted between the MAC
> header and IP header.
> 
> Add an additional check on stmmac link up so that COE is deactivated
> when the stmmac device is used as a DSA conduit.
> 
> Add a new dma_feature flag to allow cores to signal that their COEs can't
> handle DSA tags on TX.
> 
> Fixes: 6b2c6e4a938f ("net: stmmac: propagate feature flags to vlan")
> Cc: stable@vger.kernel.org
> Reported-by: Richard Tresidder <rtresidd@electromag.com.au>
> Closes: https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> Reported-by: Romain Gantois <romain.gantois@bootlin.com>
> Closes: https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> ---

DSA_TAG_PROTO_LAN9303, DSA_TAG_PROTO_SJA1105 and DSA_TAG_PROTO_SJA1110
construct tags with ETH_P_8021Q as EtherType. Do you still think it
would be correct to say that all DSA tags break COE on the stmmac, as
this patch assumes?

The NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM convention is not about
statically checking whether the interface using DSA, but about looking
at each packet before deciding whether to use the offload engine or to
call skb_checksum_help().

You can experiment with any tagging protocol on the stmmac driver, and
thus with the controller's response to any kind of traffic, even if the
port is not attached to a hardware switch. You need to enable the
CONFIG_NET_DSA_LOOP option, edit the return value of dsa_loop_get_protocol()
and the "netdev" field of dsa_loop_pdata. The packets need to be
analyzed on the link partner with a packet analysis tool, since there is
no switch to strip the DSA tag.

>  drivers/net/ethernet/stmicro/stmmac/common.h     |  1 +
>  .../net/ethernet/stmicro/stmmac/dwmac1000_dma.c  |  1 +
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c    | 16 +++++++++++++++-
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> index daf79cdbd3ec..50686cdc3320 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c
> @@ -264,6 +264,7 @@ static int dwmac1000_get_hw_feature(void __iomem *ioaddr,
>  	dma_cap->number_tx_channel = (hw_cap & DMA_HW_FEAT_TXCHCNT) >> 22;
>  	/* Alternate (enhanced) DESC mode */
>  	dma_cap->enh_desc = (hw_cap & DMA_HW_FEAT_ENHDESSEL) >> 24;
> +	dma_cap->dsa_breaks_tx_coe = 1;
>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a9b6b383e863..733348c65e04 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -42,6 +42,7 @@
>  #include <net/page_pool/helpers.h>
>  #include <net/pkt_cls.h>
>  #include <net/xdp_sock_drv.h>
> +#include <net/dsa.h>
>  #include "stmmac_ptp.h"
>  #include "stmmac.h"
>  #include "stmmac_xdp.h"
> @@ -993,8 +994,11 @@ static void stmmac_mac_link_up(struct phylink_config *config,
>  			       int speed, int duplex,
>  			       bool tx_pause, bool rx_pause)
>  {
> -	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> +	struct net_device *ndev = to_net_dev(config->dev);
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	unsigned int tx_queue_cnt;
>  	u32 old_ctrl, ctrl;
> +	int queue;
>  
>  	if ((priv->plat->flags & STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP) &&
>  	    priv->plat->serdes_powerup)
> @@ -1102,6 +1106,16 @@ static void stmmac_mac_link_up(struct phylink_config *config,
>  
>  	if (priv->plat->flags & STMMAC_FLAG_HWTSTAMP_CORRECT_LATENCY)
>  		stmmac_hwtstamp_correct_latency(priv, priv);
> +
> +	/* DSA tags break COEs on some cores. Disable TX checksum
> +	 * offloading on those cores if the netdevice is a DSA conduit.
> +	 */
> +	if (priv->dma_cap.dsa_breaks_tx_coe && netdev_uses_dsa(ndev)) {
> +		tx_queue_cnt = priv->plat->tx_queues_to_use;
> +		for (queue = 0; queue < tx_queue_cnt; queue++)
> +			priv->plat->tx_queues_cfg[queue].coe_unsupported = true;
> +	}
> +

The DSA switch driver can load after stmmac_mac_link_up() runs.
This implementation is racy anyway.

>  }
>  
>  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> -- 
> 2.43.0
> 
> 

pw-bot: cr

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Vladimir Oltean]

On Mon, Dec 18, 2023 at 05:23:22PM +0100, Romain Gantois wrote:
> I've run some iperf3 tests and the TX hotpath performance doesn't seem
> to be degraded by the field added to dma_features.

I don't know what CPU cores you are using, but if the iperf3 performance
was line rate at gigabit before and is line rate at gigabit now, you
haven't effectively measured the impact of the change (and "doesn't seem
to be degraded" is a false conclusion). You need something more CPU
intensive to see the difference, like IP forwarding of 64 byte packets.

A very simplistic way to set up IP forwarding between 2 DSA user ports
is to do this on the router board (DUT):

echo 1 > /proc/sys/net/ipv4/ip_forward
ip link set swp0 up && ip addr add 192.168.100.2/24 dev swp0
ip link set swp1 up && ip addr add 192.168.101.2/24 dev swp1

and this on the system with 2 endpoints:

ip netns add ns0
ip link set $ETH1 netns ns0
ip link set $ETH0 up && ip addr add 192.168.100.1/24 dev $ETH0
ip -n ns0 link set $ETH1 up && ip -n ns0 addr add 192.168.101.1/24 dev $ETH1
ip route add 192.168.101.0/24 via 192.168.100.2
ip -n ns0 route add 192.168.100.0/24 via 192.168.101.2
./net-next/samples/pktgen/pktgen_sample03_burst_single_flow.sh -i $ETH0 -s 64 -m 00:04:9f:05:de:0a -d 192.168.101.1 -t 2 -f 13 -c 0 -p 400 -n 0 -b 4

I compiled the commands from some notes I had lying around, I didn't
retest them.

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Maxime Chevallier]

Hi Vlad,

+ Linus Walleij

On Tue, 19 Dec 2023 14:20:34 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> Hi Romain,
> 
> On Mon, Dec 18, 2023 at 05:23:23PM +0100, Romain Gantois wrote:
> > Some stmmac cores have Checksum Offload Engines that cannot handle DSA tags
> > properly. These cores find the IP/TCP headers on their own and end up
> > computing an incorrect checksum when a DSA tag is inserted between the MAC
> > header and IP header.
> > 
> > Add an additional check on stmmac link up so that COE is deactivated
> > when the stmmac device is used as a DSA conduit.
> > 
> > Add a new dma_feature flag to allow cores to signal that their COEs can't
> > handle DSA tags on TX.
> > 
> > Fixes: 6b2c6e4a938f ("net: stmmac: propagate feature flags to vlan")
> > Cc: stable@vger.kernel.org
> > Reported-by: Richard Tresidder <rtresidd@electromag.com.au>
> > Closes: https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> > Reported-by: Romain Gantois <romain.gantois@bootlin.com>
> > Closes: https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> > ---  
> 
> DSA_TAG_PROTO_LAN9303, DSA_TAG_PROTO_SJA1105 and DSA_TAG_PROTO_SJA1110
> construct tags with ETH_P_8021Q as EtherType. Do you still think it
> would be correct to say that all DSA tags break COE on the stmmac, as
> this patch assumes?
> 
> The NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM convention is not about
> statically checking whether the interface using DSA, but about looking
> at each packet before deciding whether to use the offload engine or to
> call skb_checksum_help().

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@linaro.org/

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

Maxime

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Linus Walleij]

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-64c269413dfa@linaro.org/
>
> 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

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Maxime Chevallier]

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-64c269413dfa@linaro.org/
> >
> > 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 :)

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

Thanks,

Maxime

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Vladimir Oltean]

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-64c269413dfa@linaro.org/
> > >
> > > 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.

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Linus Walleij]

On Tue, Dec 19, 2023 at 11:46 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Dec 19, 2023 at 05:29:32PM +0100, Maxime Chevallier wrote:

> > > 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?

I guess I should just post the patches inline. (It came from both
Erics and Maximes suggestion really.)

Actually I wrote two helpers, one to get the ethertype from the
ethernet frame which is pretty straight-forward.

include/linux/if_ether.h

+/* This determines the ethertype incoded into the skb data without
+ * relying on skb->protocol which is not always identical.
+ */
+static inline u16 skb_eth_raw_ethertype(const struct sk_buff *skb)
+{
+       struct ethhdr *hdr;
+
+       /* If we can't extract a header, return invalid type */
+       if (!skb_pointer_if_linear(skb, 0, ETH_HLEN))
+               return 0x0000U;
+
+       hdr = skb_eth_hdr(skb);
+
+       return ntohs(hdr->h_proto);
+}

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

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

Yeps and I am happy to fold these helpers inside of my driver if
they are not helpful to anyone else, or if that is the best idea for something
intended for a fix, i.e. an -rc kernel.

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

Fair point! Let's take a step back. The kerneldoc says:

 *      @protocol: Packet protocol from driver

That's a bit vague and it was in the first commit in git history :/

But Eric probably knows the right way to use protocol.

But we know for sure that VLAN uses this for the outermost protocol
ETH_P_8021Q (etc).

I wonder how the network stack reacts if we set the skb->protocol
to whatever DSA taggers put at the position of the ethertype.

For RTL taggers probably this works because they use an elaborate
custom ethertype, but e.g. net/dsa/tag_mtk.c will just put in
"ethertype" 0x0000, 0x0001 or 0x0002, the two latter which are
formally ETH_P_802_3 and ETH_P_AX25 which I think is maybe
not so good to put into skb->protocol.

Another option is to set it to the ETH_P_DSA ethertype, currently
unused in the kernel.

Now this kind of thinking makes me insecure because:
git grep '\->protocol' net/

There is just sooooo much code inspecting ->protocol in the generic
network stack that this seems like inviting disaster.

Yours,
Linus Walleij

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Linus Walleij]

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

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Household Cang]

> On Dec 18, 2023, at 11:23 AM, Romain Gantois <romain.gantois@bootlin.com> wrote:
> 
> This is a bugfix for an issue that was recently brought up in two
> reports:
> 
> https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> 
Add me in to be the 3rd report...
RK3568 GMAC0 (eth1) to MT7531BE (CPU port)
Current workaround for me is ethtool -K eth1 rx off tx off

https://lore.kernel.org/netdev/m3clft2k7umjtny546ot3ayebattksibty3yyttpffvdixl65p@7dpqsr5nisbk/T/#t

Question on the patch to be built: how would I know if my setup could take advantage of the HW checksum offload? RK3658’s eth0 on stmmac is doing fine, and eth0 is not on a DSA switch. Does this mean eth1 should be able to do hw checksum offload once the stmmac driver is fixed?
—Lucas

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Vladimir Oltean]

Hi Lucas,

On Thu, Dec 21, 2023 at 02:40:34AM -0500, Household Cang wrote:
> > On Dec 18, 2023, at 11:23 AM, Romain Gantois <romain.gantois@bootlin.com> wrote:
> > 
> > This is a bugfix for an issue that was recently brought up in two
> > reports:
> > 
> > https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> > https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> > 
> Add me in to be the 3rd report...
> RK3568 GMAC0 (eth1) to MT7531BE (CPU port)
> Current workaround for me is ethtool -K eth1 rx off tx off

Is "rx off" actually required, or just "tx off"?

> https://lore.kernel.org/netdev/m3clft2k7umjtny546ot3ayebattksibty3yyttpffvdixl65p@7dpqsr5nisbk/T/#t
> 
> Question on the patch to be built: how would I know if my setup could
> take advantage of the HW checksum offload? RK3658’s eth0 on stmmac is
> doing fine, and eth0 is not on a DSA switch. Does this mean eth1
> should be able to do hw checksum offload once the stmmac driver is
> fixed?

The MT7531BE switch requires transmitted packets to have an additional
header which indicates what switch port is targeted. So the packet
structure is not the same as what eth0 transmits.

Your GMAC datasheet should explain what packets it is able to offload
L4 checksumming for, quite plainly. Probably MAC + IP + TCP yes, but
MAC + MTK DSA + IP + TCP no.

The bug is that the network stack thinks that the GMAC is able to
offload TX checksums for these MTK DSA tagged packets, so it does not
calculate the checksum in software, leaving the task up to the hardware.
My guess is that the hardware doesn't recognize the packets as something
that is offloadable, so it doesn't calculate the checksum either, and
that's the story of how you end up with packets with bad checksums.

The patch to be built should analyze the packet before passing it to a
hardware offload engine which will do nothing. The driver still declares
the NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM features because it is able to
offload checksumming for _some_ packets, but it falls back to the
software checksum helper for the rest. This includes your MTK DSA tagged
packets. They can be checksummed in software even with the DSA tag added,
because that uses the more generic mechanism with skb->csum_start and
skb->csum_offset, which DSA is compatible with, just fine. The GMAC
driver, most likely because of the lack of hardware support, does not
look at skb->csum_start and skb->csum_offset (aka it does not declare
the NETIF_F_HW_CSUM feature), so it cannot offload checksumming for your
switch traffic unless that was specifically baked into the RTL.

More details in the "Switch tagging protocols" section of
Documentation/networking/dsa/dsa.rst.

RE: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Lucas Pereira]

Dear community collaborators,

First of all, I would like to thank you for the prompt response and
the suggestions provided.

We conducted the tests as indicated, but unfortunately, the problem
persists. It seems to me that if it were a Checksum-related issue, the
behavior would be different, as the VPN and communication work
normally for several days before failing suddenly.

We have observed that the only effective ways to reestablish
communication, so far, are through a system reboot or by changing the
authentication cipher, such as switching from MD5 to SHA1.
Interestingly, when switching back to the MD5 cipher, the
communication fails to function again.

I am immensely grateful for the help received so far and would greatly
appreciate any further suggestions or recommendations that you might
offer to resolve this challenge.

Sincerely,
Lucas

________________________________
De: Vladimir Oltean <olteanv@gmail.com>
Enviado: sexta-feira, 22 de dezembro de 2023 09:30
Para: Household Cang <canghousehold@aol.com>
Cc: Romain Gantois <romain.gantois@bootlin.com>; Alexandre Torgue
<alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
David S. Miller <davem@davemloft.net>; Eric Dumazet
<edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
<pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
Miquel Raynal <miquel.raynal@bootlin.com>; Maxime Chevallier
<maxime.chevallier@bootlin.com>; Sylvain Girard
<sylvain.girard@se.com>; Pascal EBERHARD <pascal.eberhard@se.com>;
Richard Tresidder <rtresidd@electromag.com.au>; netdev@vger.kernel.org
<netdev@vger.kernel.org>; linux-stm32@st-md-mailman.stormreply.com
<linux-stm32@st-md-mailman.stormreply.com>;
linux-arm-kernel@lists.infradead.org
<linux-arm-kernel@lists.infradead.org>
Assunto: Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

Hi Lucas,

On Thu, Dec 21, 2023 at 02:40:34AM -0500, Household Cang wrote:
> > On Dec 18, 2023, at 11:23 AM, Romain Gantois <romain.gantois@bootlin.com> wrote:
> >
> > This is a bugfix for an issue that was recently brought up in two
> > reports:
> >
> > https://lore.kernel.org/netdev/c57283ed-6b9b-b0e6-ee12-5655c1c54495@bootlin.com/
> > https://lore.kernel.org/netdev/e5c6c75f-2dfa-4e50-a1fb-6bf4cdb617c2@electromag.com.au/
> >
> Add me in to be the 3rd report...
> RK3568 GMAC0 (eth1) to MT7531BE (CPU port)
> Current workaround for me is ethtool -K eth1 rx off tx off

Is "rx off" actually required, or just "tx off"?

> https://lore.kernel.org/netdev/m3clft2k7umjtny546ot3ayebattksibty3yyttpffvdixl65p@7dpqsr5nisbk/T/#t
>
> Question on the patch to be built: how would I know if my setup could
> take advantage of the HW checksum offload? RK3658’s eth0 on stmmac is
> doing fine, and eth0 is not on a DSA switch. Does this mean eth1
> should be able to do hw checksum offload once the stmmac driver is
> fixed?

The MT7531BE switch requires transmitted packets to have an additional
header which indicates what switch port is targeted. So the packet
structure is not the same as what eth0 transmits.

Your GMAC datasheet should explain what packets it is able to offload
L4 checksumming for, quite plainly. Probably MAC + IP + TCP yes, but
MAC + MTK DSA + IP + TCP no.

The bug is that the network stack thinks that the GMAC is able to
offload TX checksums for these MTK DSA tagged packets, so it does not
calculate the checksum in software, leaving the task up to the hardware.
My guess is that the hardware doesn't recognize the packets as something
that is offloadable, so it doesn't calculate the checksum either, and
that's the story of how you end up with packets with bad checksums.

The patch to be built should analyze the packet before passing it to a
hardware offload engine which will do nothing. The driver still declares
the NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM features because it is able to
offload checksumming for _some_ packets, but it falls back to the
software checksum helper for the rest. This includes your MTK DSA tagged
packets. They can be checksummed in software even with the DSA tag added,
because that uses the more generic mechanism with skb->csum_start and
skb->csum_offset, which DSA is compatible with, just fine. The GMAC
driver, most likely because of the lack of hardware support, does not
look at skb->csum_start and skb->csum_offset (aka it does not declare
the NETIF_F_HW_CSUM feature), so it cannot offload checksumming for your
switch traffic unless that was specifically baked into the RTL.

More details in the "Switch tagging protocols" section of
Documentation/networking/dsa/dsa.rst.

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Vladimir Oltean]

On Fri, Dec 22, 2023 at 10:22:21AM -0300, Lucas Pereira wrote:
> Dear community collaborators,
> 
> First of all, I would like to thank you for the prompt response and
> the suggestions provided.
> 
> We conducted the tests as indicated, but unfortunately, the problem
> persists. It seems to me that if it were a Checksum-related issue, the
> behavior would be different, as the VPN and communication work
> normally for several days before failing suddenly.
> 
> We have observed that the only effective ways to reestablish
> communication, so far, are through a system reboot or by changing the
> authentication cipher, such as switching from MD5 to SHA1.
> Interestingly, when switching back to the MD5 cipher, the
> communication fails to function again.
> 
> I am immensely grateful for the help received so far and would greatly
> appreciate any further suggestions or recommendations that you might
> offer to resolve this challenge.
> 
> Sincerely,
> Lucas

Are you responding to the right thread? This is about on-board Ethernet
switch chips attached to Synopsys MAC hardware IPs.

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Household Cang]

> On Dec 22, 2023, at 7:30 AM, Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> Is "rx off" actually required, or just "tx off”?

Before adjusted the ethtool -K, the client upload speed is 880Mbps (download speed 0.6Mbps). RK3568 is acting as a router, so client is sending to eth1 via DSA user port, rx used here, and then tx on eth0. So this might suggest only tx needs to be turned off on eth1.

> 
> The MT7531BE switch requires transmitted packets to have an additional
> header which indicates what switch port is targeted. So the packet
> structure is not the same as what eth0 transmits.
> 

I understand. How many bytes are the DSA header for MT, 8 bytes?

> Your GMAC datasheet should explain what packets it is able to offload
> L4 checksumming for, quite plainly. Probably MAC + IP + TCP yes, but
> MAC + MTK DSA + IP + TCP no.

I hardly could find this in the data sheet for RK3568. From the DMA mapping, it insinuates a correct ether type needs to be detected after the MAC header, besides an inner and an outer VLAN tag, if there are any.

>  The driver still declares
> the NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM features because it is able to
> offload checksumming for _some_ packets, but it falls back to the
> software checksum helper for the rest. This includes your MTK DSA tagged
> packets.

I guess the end verdict regarding whether the hardware supports checksum offloading on DSA frames is a NO. So this is going to use some precious CPU power I am looking to fully dedicate to ipsec. Though I am pursuing crypto hw offloading at the same time with baylibre.

Lucas.

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Romain Gantois]

On Tue, 19 Dec 2023, Vladimir Oltean wrote:
> DSA_TAG_PROTO_LAN9303, DSA_TAG_PROTO_SJA1105 and DSA_TAG_PROTO_SJA1110
> construct tags with ETH_P_8021Q as EtherType. Do you still think it
> would be correct to say that all DSA tags break COE on the stmmac, as
> this patch assumes?
> 
> The NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM convention is not about
> statically checking whether the interface using DSA, but about looking
> at each packet before deciding whether to use the offload engine or to
> call skb_checksum_help().
> 
> You can experiment with any tagging protocol on the stmmac driver, and
> thus with the controller's response to any kind of traffic, even if the
> port is not attached to a hardware switch. You need to enable the
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:

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.

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/stmmac/stmmac_main.c#L5493
 
And so the IP/TCP checksums will not be checked at all, 
which is not ideal.

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.

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.

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.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE

[Vladimir Oltean]

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/stmmac/stmmac_main.c#L5493
>  
> 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?".

Re: [PATCH net 0/1] Prevent DSA tags from breaking COE

[Richard Tresidder]

Richard Tresidder


Hi All
    Just wondering if from a custom platform point of view it might be better to use vlan tagging instead of DSA in the case where the MAC can't handle DSA tags in the header.
The marvel chip can use VLAN tagging to perform basically the same thing is my understanding, you just have to nominate a vlan id (untag outbound) to a port and trunk on cpu interface..
Just considering this as from my understanding the STMMAC does understand VLAN tags and will correctly generate the CRC.
I don't think this would stop us using VLAN for the ports in general, they'd just have a "default" tag for each port thats used for untagged packets on the port.
You could still apply additional VLAN's over those same ports..

I don't suppose anyone has some CPU usage info on software CRC vs this method?
I'm not sure if this is just an IP CRC issue or a TCP CRC issue also ( can the stmmac offload the TCP CRC also? )
If this is only an IP issue and the TCP CRC is never offloaded then it probably won't make much difference in reality..

Thanks
    Richard Tresidder

On 19/12/2023 5:50 pm, Romain Gantois wrote:

> On Mon, 18 Dec 2023, Andrew Lunn wrote:
> ...
>> Probably a dumb question.... Does this COE also perform checksum
>> validation on receive? Is it also getting confused by the DSA header?
>>
>> You must of tested receive, so it works somehow, but i just wounder if
>> something needs to be done to be on the safe side?
> That's a good point, I just investigated the RX path a bit more and the MAC
> indeed has IP/TCP/UDP RX checksum offloading enabled. However, the
> external switch in my setup uses EDSA tags, which displace the "true" ethertype
> field to the end of the DSA header and replaces the "normal" ethertype with
> ETH_P_EDSA (0xdada). So to the MAC controller, the ethernet frame has an unknown
> ethertype, and so it doesn't see it as an IP frame at all. All of the
> ethtool counters related to IP stuff are at 0, which supports this.
>
> This explains why checksum offloading doesn't break the RX path in my case.
> However, other maybe other DSA switches using different frame formats could
> cause different behavior? Considering this, I think it would be safer to change
> the dsa_breaks_tx_coe flag to a general dsa_breaks_coe flag. It makes sense to
> me to assume that if DSA tags break TX COE, then RX COE will also not work.
>
> I'll take this into account when I send a v2.
>
> Best Regards,
>

Proxy ARP NetNS Awareness

[Household Cang]

Last light the Linux Librechat was focused on digging the kernel code surrounding /net/ipv4/arp.c and /net/core/net_namespace.c to answer whether the proxy_arp feature enabled by sysctl is namespace aware.

After many hours of tracing from namespace-generating unshare --net command all the way to the kernel net_namespace.c gave us some clues that the main ns and new ns converged at arp_net_init() in arp.c. And I am currently stuck on this line 1497

proc_create_net("arp", 0444, net->proc_net, &arp_seq_ops,
            sizeof(struct neigh_seq_state))

It is unknown whether this function creates a "view" to the ARP neighbor table such that each netns has a different view to the neighbor table, OR each netns maintains its own neighbor table. Either way, the implication is whether proxy_arp enabled by sysctl is restricted to the current netns.

If proxy_arp is retricted to the current netns, then the Debian documentation on wireless bridge-less pseudo-bridge https://wiki.debian.org/BridgeNetworkConnections may be wrong in insinuating that the proxy_arp feature in the modern kernel can replace parprouted userspace program.

The current documentation with sysctl net-related options are really vague in terms of netns interaction. arp_ignore, arp_announce, arp_filter did not do a good job of disambiguating whether any of these arp features can or cannot work across namespaces, or the reasons for the behavior.

From the arp_filter option description, this line "IP addresses are owned by the complete host on Linux, not by particular interfaces." is a single-netns statement, but highly suggests that arp operations are per namespace.

When a virtual ethernet pair is created between two netns, parprouted userspace program can relay arps across namespaces, but the kernel proxy_arp cannot.

Thank you for any insights and feel free to forward to subject matter experts.
I really want to get to the bottom of this.

Lucas

Re: Proxy ARP NetNS Awareness

[Nicolas Dichtel]

Le 17/10/2025 à 08:56, Household Cang a écrit :
> Last light the Linux Librechat was focused on digging the kernel code surrounding /net/ipv4/arp.c and /net/core/net_namespace.c to answer whether the proxy_arp feature enabled by sysctl is namespace aware.
> 
> After many hours of tracing from namespace-generating unshare --net command all the way to the kernel net_namespace.c gave us some clues that the main ns and new ns converged at arp_net_init() in arp.c. And I am currently stuck on this line 1497
> 
> proc_create_net("arp", 0444, net->proc_net, &arp_seq_ops,
>             sizeof(struct neigh_seq_state))
> 
> It is unknown whether this function creates a "view" to the ARP neighbor table such that each netns has a different view to the neighbor table, OR each netns maintains its own neighbor table. Either way, the implication is whether proxy_arp enabled by sysctl is restricted to the current netns.
Each netns maintains its own neighbor tables.
The sysctl proxy_arp is per netns.

Nicolas

> 
> If proxy_arp is retricted to the current netns, then the Debian documentation on wireless bridge-less pseudo-bridge https://wiki.debian.org/BridgeNetworkConnections may be wrong in insinuating that the proxy_arp feature in the modern kernel can replace parprouted userspace program.
> 
> The current documentation with sysctl net-related options are really vague in terms of netns interaction. arp_ignore, arp_announce, arp_filter did not do a good job of disambiguating whether any of these arp features can or cannot work across namespaces, or the reasons for the behavior.
> 
> From the arp_filter option description, this line "IP addresses are owned by the complete host on Linux, not by particular interfaces." is a single-netns statement, but highly suggests that arp operations are per namespace.
> 
> When a virtual ethernet pair is created between two netns, parprouted userspace program can relay arps across namespaces, but the kernel proxy_arp cannot.
> 
> Thank you for any insights and feel free to forward to subject matter experts.
> I really want to get to the bottom of this.
> 
> Lucas
>