Subject: Re: [RFC/RFT 02/23] net: phylink: Plumb eee_active in mac_link_up call

From: "Russell King (Oracle)" <[email-masked]>
Date: Mon, 27 Mar 2023 22:53:36 +0100

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!