Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Feb 17, 2023 at 09:19:43AM +0100, Oleksij Rempel wrote:
> On Fri, Feb 17, 2023 at 04:42:20AM +0100, Andrew Lunn wrote:
> > @@ -3131,15 +3120,7 @@ fec_enet_set_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  		return -ENETDOWN;
> >  
> >  	p->tx_lpi_timer = edata->tx_lpi_timer;
> > -
> > -	if (!edata->eee_enabled || !edata->tx_lpi_enabled ||
> > -	    !edata->tx_lpi_timer)
> > -		ret = fec_enet_eee_mode_set(ndev, false);
> > -	else
> > -		ret = fec_enet_eee_mode_set(ndev, true);
> > -
> > -	if (ret)
> > -		return ret;
> > +	p->tx_lpi_enabled = edata->tx_lpi_enabled;
> 
> Hm.. this change have effect only after link restart. Should we do
> something like this?
> 
> 	if (phydev->link)
> 		fec_enet_eee_mode_set(ndev, phydev->eee_active);
> 
> or, execute phy_ethtool_set_eee() first and some how detect if link
> changed? Or restart link by phylib on every change?

Restarting the link on every "change" (even when nothing has changed)
is a dirty hack, and can be very annoying, leading to multiple link
restarts e.g. at boot time which can turn into several seconds of
boot delay depending on how much is configured.

I would suggest as a general principle, we should be keeping link
renegotiations to a minimum - and phylib already tries to do that in
several places.

Also note that reading phydev->eee_active without holding the phydev
mutex can be racy - and I would also ask what would prevent two calls
to fec_enet_eee_mode_set() running concurrently, once by the
adjust_link callback and once via the set_eee method. This applies
to reading phydev->link as well, so it may be best to hold the
phydev mutex around that entire if() clause.

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