Re: [PATCH net] net: phylink: fix suspend/resume with WoL enabled and link down

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

On Thu, Apr 17, 2025 at 12:49:07PM -0700, Alexander Duyck wrote:
> What I was saying is that we could add an additional state flag that
> we set before you write to the phylink_disable_state. You would
> essentially set the state to true if you want to preserve the current
> state, and if it is true you would set cur_link_statte to false in
> phylink_resolve ignoring the actual current link state.
> 
> So in phylink_stop you would set it to false, and in phylink_suspend
> you would set it to true. With that change phylink_stop could force
> the link down, whereas phylink_suspend would keep it up,
> phylink_suspend could deal with netif_carrier_off, and phylink_resume
> could deal with old_link_state.

I really don't like the idea that the netif carrier state differs from
old_link_state. These need to be the same to ensure that drivers which
use phylink in PHYLINK_NETDEV mode (which uses netif carrier for link
state tracking) vs PHYLINK_DEV mode (which uses old_link_state) see the
same behaviour, becomes much harder to guarantee if we start treating
these differently in the code depending on something other than which
PHYLINK*DEV is in use. It's really not something I want to entertain.

> > So, if the link was up, and we don't call mac_link_down() then we must
> > also *not* call phylink_mac_initial_config(). I've no idea what will
> > break with that change.
> 
> Sorry, mentioning it didn't occur to me as I have been dealing with
> the "rolling start" since the beginning. In mac_prepare I deal with
> this. Specifically the code in mac_prepare will check to see if the
> link state is currently up with the desired configuration already or
> not. If it is, it sets a flag that will keep us from doing any changes
> that would be destructive to the link. If the link is down it
> basically clears the way for a full reinitialization.

I would much rather avoid any of the "setup" calls (that means
mac_prepare(), mac_config(), mac_finish(), pcs_config() etc) and
mac_link_up() if we're going to add support for "rolling start" to
phylink.

That basically means that the MAC needs to be fully configured to
process packets before phylink_start() or phylink_resume() is called.

This, however, makes me wonder why you'd even want to use phylink in
this situation, as phylink will be doing virtually nothing for fbnic.

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