Message-ID: <Z4AJ4bxLePBbbR2u@pengutronix.de>
Date: Thu, 9 Jan 2025 18:39:45 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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...

-- 
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 |

Powered by blists - more mailing lists