Re: [PATCH net-next v1 1/7] net: stmmac: xgmac: drop incomplete FPE implementation

[Furong Xu]

Hi Vladimir

On Tue, 9 Jul 2024 20:10:18 +0300, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> Hi Andrew, Furong,
> 
> On Tue, Jul 09, 2024 at 03:16:35PM +0200, Andrew Lunn wrote:
> > On Tue, Jul 09, 2024 at 04:21:19PM +0800, Furong Xu wrote:  
> > > The FPE support for xgmac is incomplete, drop it temporarily.
> > > Once FPE implementation is refactored, xgmac support will be added.  
> > 
> > This is a pretty unusual thing to do. What does the current
> > implementation do? Is there enough for it to actually work? If i was
> > doing a git bisect and landed on this patch, could i find my
> > networking is broken?
> > 
> > More normal is to build a new implementation by the side, and then
> > swap to it.
> > 
> > 	Andrew
> >   
> 
> There were 2 earlier attempts from Jianheng Zhang @ Synopsys to add FPE
> support to new hardware.
> 
> I told him that the #1 priority should be to move the stmmac driver over
> to the new standard API which uses ethtool + tc.
> https://lore.kernel.org/netdev/CY5PR12MB63726FED738099761A9B81E7BF8FA@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> https://lore.kernel.org/netdev/CY5PR12MB63727C24923AE855CFF0D425BF8EA@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> 
> I'm not sure what happened in the meantime. Jianheng must have faced
> some issue, because he never came back.
> 
> I did comment this at the time:
> 
> | 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.
> 
> So it is relatively known that the support is incomplete. But I still
> think we should push for more reviewer insight into this driver by
> having access to a selftest to get a clearer picture of how it behaves.
> For that, we need the compliance to the common API.
> 

After some searching and learning about your commits for FPE using the
generic framework, I think it is clear enough to me to implement the new
standard driver interface which uses ethtool + tc, and then the refactor
of low level FPE function can follow.
Thanks.