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

[Vladimir Oltean <olteanv@gmail.com>]

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