Message-ID: 
 <CY5PR12MB6372C564179B362B7B41D081BF8CA@CY5PR12MB6372.namprd12.prod.outlook.com>
Date: Thu, 14 Dec 2023 11:35:07 +0000
From: Jianheng Zhang <Jianheng.Zhang@...opsys.com>
To: Vladimir Oltean <olteanv@...il.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


> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: Wednesday, December 13, 2023 8:43 AM
> To: Jianheng Zhang <jianheng@...opsys.com>; Jakub Kicinski <kuba@...nel.org>
> Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>; Jose Abreu <joabreu@...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
> <lijames@...opsys.com>; Martin McKenny <mmckenny@...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://urldefense.com/v3/__https://github.com/altera-opensource/meta-intel-fpga-refdes/blob/7b050c
> a9968f5ff71598e86bb3a10bb8e011439c/recipes-connectivity/iproute2/iproute2/0003-taprio-Add-suppo
> rt-for-the-SetAndHold-and-SetAndRele.patch__;!!A4F2R9G_pg!cj_LK6iKkOj8RlYyKdNK6wv23ddmwT3_3
> ebNC0gb97xaKddBhm2B0uAZMIffG5vxyRHCcbyez2aY-JaDt-tNTg$
> 
> 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

Hi Vladimir,

Your insights into the current state of FPE support in the stmmac driver are 
indeed on point. It's accurate that the existing FPE support of stmmac relies 
on the mentioned patch. Before the emergence of the ethtool-mm framework, some 
users might have been able to use the FP function in this way. But with the 
introduction of the common ethtool-mm framework, it is more important to update 
the existing stmmac FPE implementation to support ethtool. And I now fully 
understand that it would not be appropriate to add xgmac support for FPE based 
on the current implementation. I appreciate you pointing out the problems with 
the current implementation and providing suggestions, they are incredibly 
helpful for the subsequent refactoring of stmmac's FPE functionality.  

Jianheng

Powered by blists - more mailing lists