Message-ID: <Z4ARB96M6KDuPvv8@shell.armlinux.org.uk>
Date: Thu, 9 Jan 2025 18:10:15 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	UNGLinuxDriver@...rochip.com, Phil Elwell <phil@...pberrypi.org>
Subject: Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support
 with phylink integration

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists