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

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: <aAdmhIcBDDIskr3J@shell.armlinux.org.uk>
Date: Tue, 22 Apr 2025 10:51:00 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Joakim Zhang <qiangqing.zhang@....com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] net: phylink: fix suspend/resume with WoL enabled
 and link down

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!