netdev - Re: [RFC/RFT 02/23] net: phylink: Plumb eee_active in mac_link

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]

Message-ID: <ZCIQYJVMoG3RUfN3@shell.armlinux.org.uk>
Date:   Mon, 27 Mar 2023 22:53:36 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [RFC/RFT 02/23] net: phylink: Plumb eee_active in mac_link_up
 call

Hi Andrew,

Thinking about this more having read the follow-on patches, I retract
my r-b tag, because there is an issue that needs solving.

On Mon, Mar 27, 2023 at 07:01:40PM +0200, Andrew Lunn wrote:
> @@ -1257,7 +1260,8 @@ static void phylink_link_up(struct phylink *pl,
>  
>  	pl->mac_ops->mac_link_up(pl->config, pl->phydev, pl->cur_link_an_mode,
>  				 pl->cur_interface, speed, duplex,
> -				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);
> +				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause,
> +				 eee_active);

In one of your later patches, you have phylib call phy_link_up() when
the state changes as a result of configuration. That will cause
phy_link_change(), which will update phylink's stored link state, and
trigger phylink to re-resolve the link.

However, phylink guarantees that mac_link_up() will only be called
if mac_link_down() was previously called. This will *not* cause
mac_link_up() to be called.

Moreover, we don't want mac_link_up() to be called because the link
hasn't gone down and to do so will violate that guarantee that
phylink makes to MAC drivers.

So, I don't think this is going to work fully as seems to be intended,
if I'm understanding things correctly.

Maybe we should have a new mac_set_eee() method which we can call
when the EEE state changes? Would we need to call it with the LPI
delay parameter and wheneever that changes, or should we rely on
the MAC to do that? What if the LPI parameter is dependent on the
speed the MAC is operating? Just brain-storming...

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