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: <8335cddd-dc5d-4a50-913a-01b748550225@lunn.ch>
Date:   Tue, 28 Mar 2023 00:45:08 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
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

On Mon, Mar 27, 2023 at 10:53:36PM +0100, Russell King (Oracle) wrote:
> 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.

Ah, O.K.

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

O.K. I don't think phylib makes the same guarantee.

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

You are correct. If the EEE configuration is changed and an auto-neg
is not required, i was not intending to force an autoneg so the link
goes down and up again.

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

For the current hardware, the MAC gets all the parameters passed as
part of the ethtool set call. They write the delay parameters to
hardware. And then there is one bit to enable/disable the us of EEE.
So the mac_set_eee() callback should only need to pass a boolean.

>What if the LPI parameter is dependent on the
> speed the MAC is operating? Just brain-storming...

None of the current hardware needs that. And we only need to call
mac_set_eee() when the link is up, so the MAC probably knows the link
speed one way or another.

I will rework this code with a new callback.

  Andrew