Message-ID: <a1b21de9-a379-f920-e7d1-07ac7f5a7e96@gmail.com>
Date:   Sat, 3 Sep 2022 10:25:45 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Kurt Kanzenbach <kurt@...utronix.de>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once



On 9/3/2022 6:24 AM, Kurt Kanzenbach wrote:
> On Thu Sep 01 2022, Vladimir Oltean wrote:
>> On Thu, Sep 01, 2022 at 08:21:33AM +0200, Kurt Kanzenbach wrote:
>>>> So from Florian's comment above, he was under case (b), different than yours.
>>>> I don't understand why you say that when ACS is set, "the STP frames are
>>>> truncated and the trailer tag is gone". Simply altering the ACS bit
>>>> isn't going to change the determination made by stmmac_rx(). My guess
>>>> based on the current input I have is that it would work fine for you
>>>> (but possibly not for Florian).
>>>
>>> I thought so too. However, altering the ACS Bit didn't help at all.
>>
>> This is curious. Could you dump the Length/Type Field (LT bits 18:16)
>> from the RDES3 for these packets? If ACS does not take effect, it would
>> mean the DWMAC doesn't think they're Length packets I guess? Also, does
>> the Error Summary say anything? In principle, the length of this packet
>> is greater than the EtherType/Length would say, by the size of the tail
>> tag. Not sure how that affects the RX parser.
> 
> That's the point. The RX parser seems to be affected by the tail
> tag. I'll have a look at the packets with ACS feature set.
> 
>>
>>> We could do some measurements e.g., with perf to determine whether
>>> removing the FCS logic has positive or negative effects?
>>
>> Yes, some IP forwarding of 60 byte frames at line rate gigabit or higher
>> should do the trick. Testing with MTU sized packets is probably not
>> going to show much of a difference.
> 
> Well, I don't see much of a difference. However, running iperf3 with
> small packets is nowhere near line rate of 100Mbit/s. Nevertheless, I
> like the following patch more, because it looks cleaner than adding more
> checks to the receive path. It fixes my problem. Florian's use case
> should also work.

LGTM, some suggestions below.

> 
>  From 5a54383167c624a90ba460531fccabbb87d40e51 Mon Sep 17 00:00:00 2001
> From: Kurt Kanzenbach <kurt@...utronix.de>
> Date: Fri, 2 Sep 2022 19:49:52 +0200
> Subject: [PATCH] net: stmmac: Disable automatic FCS/Pad stripping
> 
> The stmmac has the possibility to automatically strip the padding/FCS for IEEE
> 802.3 type frames. This feature is enabled conditionally. Therefore, the stmmac
> receive path has to have a determination logic whether the FCS has to be
> stripped in software or not.
> 
> In fact, for DSA this ACS feature is disabled and the determination logic
> doesn't check for it properly. For instance, when using DSA in combination with
> an older stmmac (pre version 4), the FCS is not stripped by hardware or software
> which is problematic.
> 
> So either add another check for DSA to the fast path or simply disable ACS
> feature completely. The latter approach has been chosen, because most of the
> time the FCS is stripped in software anyway and it removes conditionals from the
> receive fast path.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac100.h    |  2 +-
>   .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  2 +-
>   .../ethernet/stmicro/stmmac/dwmac1000_core.c  |  9 -------
>   .../ethernet/stmicro/stmmac/dwmac100_core.c   |  8 ------
>   .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 +++----------------
>   5 files changed, 6 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> index 35ab8d0bdce7..7ab791c8d355 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> @@ -56,7 +56,7 @@
>   #define MAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
>   #define MAC_CONTROL_RE		0x00000004	/* Receiver Enable */
>   
> -#define MAC_CORE_INIT (MAC_CONTROL_HBD | MAC_CONTROL_ASTP)
> +#define MAC_CORE_INIT (MAC_CONTROL_HBD)
>   
>   /* MAC FLOW CTRL defines */
>   #define MAC_FLOW_CTRL_PT_MASK	0xffff0000	/* Pause Time Mask */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> index 3c73453725f9..4296ddda8aaa 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> @@ -126,7 +126,7 @@ enum inter_frame_gap {
>   #define GMAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
>   #define GMAC_CONTROL_RE		0x00000004	/* Receiver Enable */
>   
> -#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | GMAC_CONTROL_ACS | \
> +#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | \
>   			GMAC_CONTROL_BE | GMAC_CONTROL_DCRS)
>   
>   /* GMAC Frame Filter defines */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> index fc8759f146c7..35d7c1cb1cf1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> @@ -15,7 +15,6 @@
>   #include <linux/crc32.h>
>   #include <linux/slab.h>
>   #include <linux/ethtool.h>
> -#include <net/dsa.h>
>   #include <asm/io.h>
>   #include "stmmac.h"
>   #include "stmmac_pcs.h"
> @@ -24,7 +23,6 @@
>   static void dwmac1000_core_init(struct mac_device_info *hw,
>   				struct net_device *dev)

You should be able to remove the net_device reference here since we do 
not use it anymore after the removal of netdev_uses_dsa() or 
de-referencing of priv.

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian

Powered by blists - more mailing lists