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

[Vladimir Oltean]

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.

Otherwise, I completely agree to the idea that any development should be
done incrementally on top of whatever already exists, instead of putting
a curtain on and then taking it back off.