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

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

On Wed, Jan 08, 2025 at 12:47:37PM +0000, Russell King (Oracle) wrote:
> On Wed, Jan 08, 2025 at 01:13:41PM +0100, Oleksij Rempel wrote:
> > Refactor Energy-Efficient Ethernet (EEE) support in the LAN78xx driver
> > to integrate with phylink. This includes the following changes:
> > 
> > - Use phylink_ethtool_get_eee and phylink_ethtool_set_eee to manage
> >   EEE settings, aligning with the phylink API.
> > - Add a new tx_lpi_timer variable to manage the TX LPI (Low Power Idle)
> >   request delay. Default it to 50 microseconds based on LAN7800 documentation
> >   recommendations.
> 
> phylib maintains tx_lpi_timer for you. Please use that instead.

Just using this variable directly phydev->eee_cfg.tx_lpi_timer ?

> In any case, I've been submitting phylink EEE support which will help
> driver authors get this correct, but I think it needs more feedback.
> Please can you look at my patch set previously posted which is now
> a bit out of date, review, and think about how this driver can make
> use of it.

Ack, will do. It looks like your port of lan743x to the new API
looks exactly like what I need for this driver too.

> In particular, I'd like ideas on what phylink should be doing with
> tx_lpi_timer values that are out of range of the hardware. Should it
> limit the value itself?

Yes, otherwise every MAC driver will need to do it in the
ethtool_set_eee() function.

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?

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?

> Should set_eee() error out?

Yes, please.

> I asked these  questions in the cover message but I don't think *anyone*
> reads cover messages anymore - as evidenced by my recent patch series that
> made reference to "make it sew" and Singer sewing machines. No one
> noticed. So I think patch series cover messages are now useless on
> netdev.

lol :)

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