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