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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Feb 2023 12:32:34 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Doug Berger <opendmb@...il.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Wei Fang <wei.fang@....com>,
        Shenwei Wang <shenwei.wang@....com>,
        Clark Wang <xiaoning.wang@....com>,
        NXP Linux Team <linux-imx@....com>,
        UNGLinuxDriver@...rochip.com, Byungho An <bh74.an@...sung.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Oleksij Rempel <linux@...pel-privat.de>
Subject: Re: [PATCH RFC 08/18] net: FEC: Fixup EEE

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!