Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Thu, Jan 09, 2025 at 06:10:15PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 09, 2025 at 06:39:45PM +0100, Oleksij Rempel wrote:
> > On Thu, Jan 09, 2025 at 05:27:11PM +0000, Russell King (Oracle) wrote:
> > > On Thu, Jan 09, 2025 at 06:13:10PM +0100, Oleksij Rempel wrote:
> > > > On Wed, Jan 08, 2025 at 03:15:52PM +0000, Russell King (Oracle) wrote:
> > > > > On Wed, Jan 08, 2025 at 03:23:37PM +0100, Oleksij Rempel wrote:
> > > > > > Yes, otherwise every MAC driver will need to do it in the
> > > > > > ethtool_set_eee() function.
> > > > > 
> > > > > I've had several solutions, and my latest patch set actually has a
> > > > > mixture of them in there (which is why I'm eager to try and find a way
> > > > > forward on this, so I can fix the patch set):
> > > > > 
> > > > > 1. the original idea to address this in Marvell platforms was to limit
> > > > >    the LPI timer to the maximum representable value in the hardware,
> > > > >    which would be 255us. This ignores that the hardware uses a 1us
> > > > >    tick rate for the timer at 1G speeds, and 10us for 100M speeds.
> > > > >    (So it limits it to 260us, even though the hardware can do 2550us
> > > > >    at 100M speed). This limit was applied by clamping the value passed
> > > > >    in from userspace without erroring out.
> > > > > 
> > > > > 2. another solution was added the mac_validate_tx_lpi() method, and
> > > > >    implementations added _in addition_ to the above, with the idea
> > > > >    of erroring out for values > 255us on Marvell hardware.
> > > > > 
> > > > > 3. another idea was to have mac_enable_tx_lpi() error out if it wasn't
> > > > >    possible to allow e.g. falling back to a software timer (see stmmac
> > > > >    comments below.) Another reason for erroring out applies to Marvell
> > > > >    hardware, where PP2 hardware supports LPI on the GMAC but not the
> > > > >    XGMAC - so it only works at speeds at or below 2.5G. However, that
> > > > >    can be handled via the lpi_capabilities, so I don't think needs to
> > > > >    be a concern.
> > > > > 
> > > > > > The other question is, should we allow absolute maximum values, or sane
> > > > > > maximum? At some point will come the question, why the EEE is even
> > > > > > enabled?
> > > > > 
> > > > > As referenced above, stmmac uses the hardware timer for LPI timeouts up
> > > > > to and including 1048575us (STMMAC_ET_MAX). Beyond that, it uses a
> > > > > normal kernel timer which is:
> > > > > 
> > > > > - disabled (and EEE mode reset) when we have a packet to transmit, or
> > > > >   EEE is disabled
> > > > > - is re-armed when cleaning up from packet transmission (although
> > > > >   it looks like we attempt to immediately enter LPI mode, and would
> > > > >   only wait for the timer if there are more packets to queue... maybe
> > > > >   this is a bug in stmmac's implementation?) or when EEE mode is first
> > > > >   enabled with a LPI timer longer than the above value.
> > > > > 
> > > > > So, should phylink have the capability to switch to a software LPI timer
> > > > > implementation when the LPI timeout value exceeds what the hardware
> > > > > supports?
> > > > 
> > > > No, i'll list my arguments later down.
> > > > 
> > > > > To put it another way, should the stmmac solution to this be
> > > > > made generic?
> > > > 
> > > > May be partially?
> > > > 
> > > > > Note that stmmac has this software timer implementation because not
> > > > > only for the reason I've given above, but also because cores other than
> > > > > GMAC4 that support LPI do not have support for the hardware timer.
> > > > 
> > > > There seems to be a samsung ethernet driver which implements software
> > > > based timer too.
> > > > 
> > > > > > The same is about minimal value, too low value will cause strong speed
> > > > > > degradation. Should we allow set insane minimum, but use sane default
> > > > > > value?
> > > > > 
> > > > > We currently allow zero, and the behaviour of that depends on the
> > > > > hardware. For example, in the last couple of days, it's been reported
> > > > > that stmmac will never enter LPI with a value of zero.
> > > > > 
> > > > > Note that phylib defaults to zero, so imposing a minimum would cause
> > > > > a read-modify-write of the EEE settings without setting the timer to
> > > > > fail.
> > > > >
> > > > > > > Should set_eee() error out?
> > > > > > 
> > > > > > Yes, please.
> > > > > 
> > > > > If we are to convert stmmac, then we need to consider what it's doing
> > > > > (as per the above) and whether that should be generic - and if it isn't
> > > > > what we want in generic code, then how do we allow drivers to do this if
> > > > > they wish.
> > > > 
> > > >    - EEE Advertisement:  
> > > > 
> > > >      Advertising EEE capabilities is entirely dependent on the PHY. Without a
> > > > PHY, these settings cannot be determined or validated, as the PHY defines the
> > > > supported capabilities. Any attempt to configure EEE advertisement without an
> > > > attached PHY should fail immediately with an appropriate error, such as:  "EEE
> > > > advertisement configuration not applied: no PHY available to validate
> > > > capabilities."
> > > 
> > > Sorry, at this point, I give up with phylink managed EEE. What you
> > > detail above is way too much for me to get involved with, and goes
> > > well beyond simply:
> > > 
> > > 1) Fixing the cockup with the phylib-managed EEE that has caused *user*
> > >    *regressions* that we need to resolve.
> > > 
> > > 2) Providing core functionality so that newer implementations can have
> > >    a consistency of behaviour.
> > > 
> > > I have *no* interest in doing a total rewrite of kernel EEE
> > > functionality - that goes well beyond my aims here.
> > >
> > > So I'm afraid that I really lost interest in reading your email, sorry.
> >  
> > Sorry for killing your motivation. I can feel your pain...
> 
> I just don't think it's right to throw a whole new load of problems
> to be solved into the mix when we already have issues in the kernel
> caused by inappropriately merged previous patches.
> 
> Andrew had a large patch set, which added the phylib core code, and
> fixed up many drivers. This was taken by someone else, and only
> Andrew's core phylib code was merged along with the code for their
> driver, thus breaking a heck of a lot of other drivers.
> 
> Either this needs to be fixed, or why don't we just declare that we've
> broken EEE in the kernel, declare that we don't support EEE at all, and
> rip the whole sorry damn thing out and start again from scratch -
> because what you're suggesting is basically changing *everything*
> about EEE support.
> 
> Yes, what we currently do may be sub-optimal, but it's the API that
> we've presented to userspace for a long time.
> 
> I just don't think it's right to decide to pile all these new issues
> on top of the utter crap situation we currently have.
> 
> Oh, lookie... I just looked back in the git history to find the person
> who submitted the subset of Andrew's code was... YOU. YOU broke lots of
> drivers by doing so. Now you're torpedoing attempts to fix them by
> trying to make it more complicated. Sorry, but your opinion has just
> lost all credibility with me given the mess you previously created
> and haven't been bothered to try to fix up. At least *I've* been
> trying to fix you crap.

First, I want to say that I understand your frustration with the current
situation and appreciate the effort you've put into improving EEE.
However, I find the tone of your message inappropriate. Personal and
highly emotional comments make constructive communication much harder.  

While we may not be working as a team, we share a common interest in
improving the Linux kernel. I acknowledge that there have been issues
with some of my patches in the past, and I take responsibility for them.
My goal has always been to contribute meaningful improvements and learn
from any mistakes.  

Let’s focus on the shared goal of making the kernel better. A respectful
and technical discussion will benefit everyone involved and ensure
progress is made.  

Best regards,  
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |