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
| ||
|
Message-ID: <20231213004311.nptkcubaxuiineec@skbuf> Date: Wed, 13 Dec 2023 02:43:11 +0200 From: Vladimir Oltean <olteanv@...il.com> To: Jianheng Zhang <Jianheng.Zhang@...opsys.com>, Jakub Kicinski <kuba@...nel.org> Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <Jose.Abreu@...opsys.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, James Li <James.Li1@...opsys.com>, Martin McKenny <Martin.McKenny@...opsys.com>, "open list:STMMAC ETHERNET DRIVER" <netdev@...r.kernel.org>, "moderated list:ARM/STM32 ARCHITECTURE" <linux-stm32@...md-mailman.stormreply.com>, "moderated list:ARM/STM32 ARCHITECTURE" <linux-arm-kernel@...ts.infradead.org>, open list <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v2 net-next] net: stmmac: xgmac3+: add FPE handshaking support On Tue, Dec 12, 2023 at 03:23:47PM -0800, Jakub Kicinski wrote: > On Tue, 12 Dec 2023 14:05:02 +0000 Jianheng Zhang wrote: > > Adds the HW specific support for Frame Preemption handshaking on XGMAC3+ > > cores. > > > > Signed-off-by: Jianheng Zhang <Jianheng.Zhang@...opsys.com> > > I defer to Vladimir on whether to trust that the follow up with > the ethtool API support will come later (and not require rewrite > of existing code); or we should request that it's part of the same > series. > -- > pw-bot: needs-ack > Here's the thing - my understanding of what Synopsys is doing is that they use TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE as implicit hints to the stmmac driver that FPE should be enabled. Hold/Release is merely one use case for frame preemption. The "fp" argument in the tc framework gives you access to the rest: preemption without scheduling, preemption with scheduling but without hold/release. And the ethtool-mm framework gives you compatibility with LLDP, so you can adjust the minimum fragment sizes according to the link partner. Roger Quadros is adding am65-cpsw support for FPE using the generic framework, and the TI device has an erratum that the minimum fragment size that can be received cannot be lower than 124 bytes. LLDP allows link partners to discover that and still interoperate - which is very important, because if first-gen TSN hardware, with all its pre-standard quirks, does not deliver, there may not be a second-gen. By using LLDP, you can also enable the FPE handshake based on user space input - when the LLDP daemon gets an LLDPDU with an Additional Ethernet Capabilities TLV, rather than during each and every stmmac_mac_link_up(), and hoping for someone to respond. Depending on your hardware design, this can even lead to power savings, because you can power on your pMAC only when LLDP says the link partner is also capable, and it will be required. Ethtool also gives you the ability to report standardized stats per eMAC and per pMAC, and standardized stats for the MAC Merge layer itself. Also, the FPE state machine from the stmmac driver is so chatty and spams the kernel log so bad, because it has nowhere else to report its current (fragile) state. The ethtool MAC Merge layer gives the driver a way to report its verification state ("FPE Handshake" as Synopsys calls it) in a standardized enum. A small note that the mainline iproute2 does not even expose the TC_TAPRIO_CMD_SET_AND_HOLD and TC_TAPRIO_CMD_SET_AND_RELEASE netlink attribute values. To quote from the manpage (which is not out of date with the code; I checked - again): 'The only supported <command> is "S", which means "SetGateStates"'. So I can only guess that Synopsys and anyone else who wants to turn on FPE on stmmac has to patch their iproute2. A Github code search made me land here: https://github.com/altera-opensource/meta-intel-fpga-refdes/blob/7b050ca9968f5ff71598e86bb3a10bb8e011439c/recipes-connectivity/iproute2/iproute2/0003-taprio-Add-support-for-the-SetAndHold-and-SetAndRele.patch In principle there's nothing wrong with not sharing patches on community software with the rest of the world. But I cannot help but get the impression that stmmac support for FPE is abandonware / extremely low priority / code written to tick the boxes. It's not in the best state even in the "good" case where the XGMAC3+ support gets accepted. Jianheng, please contradict me by showing what testing is currently conducted with this implementation. If none or close to that, let's get it to a more "observable" state, where at least others' tests could be reused. Even this very patch is slightly strange - it is not brand new hardware support, but it fills in some more FPE ops in dwxlgmac2_ops - when dwxgmac3_fpe_configure() was already there. So this suggests the existing support was incomplete. How complete is it now? No way to tell. There is a selftest to tell, but we can't run it because the driver doesn't integrate with those kernel APIs. There are long periods of radio silence from Synopsys engineers in upstream, and as maintainers we simply don't know what stmmac's FPE implementation does and what it doesn't do. If a future user gets into trouble, having a "known good" bisection point, by means of a selftest that passes, is going to allow even non-expert maintainers like us provide some help, even if Synopsys engineers go radio silent again. It may be that Jianheng just needs a little nudge to help the management prioritize, by getting a NACK. It's a simple "help us help you" situation: the framework is there and it is a gateway for better Linux user space support for your platform, you just need to use it. And what better time to integrate with new API than with new hardware... :) Because it's not as if FPE on XGMAC3+ ever worked in mainline given my reading of the code. So why would users not start learning to use it with what is becoming the common tool set for everybody else. Allow me to change the "needs-ack" into: pw-bot: cr
Powered by blists - more mailing lists