lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
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