Message-ID: <c02f1bdb-0134-4edf-b3a7-8bb5152c11d0@broadcom.com>
Date: Wed, 30 Jul 2025 11:45:53 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
 Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: Daniel Braunwarth <daniel.braunwarth@...a.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Gatien CHEVALLIER <gatien.chevallier@...s.st.com>,
 Jakub Kicinski <kuba@...nel.org>, Jon Hunter <jonathanh@...dia.com>,
 netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
 Thierry Reding <treding@...dia.com>
Subject: Re: [PATCH RFC ???net???] net: phy: realtek: fix wake-on-lan support

On 7/30/25 01:35, Russell King (Oracle) wrote:
> Implement Wake-on-Lan correctly. The existing implementation has
> multiple issues:
> 
> 1. It assumes that Wake-on-Lan can always be used, whether or not the
>     interrupt is wired, and whether or not the interrupt is capable of
>     waking the system. This breaks the ability for MAC drivers to detect
>     whether the PHY WoL is functional.
> 2. switching the interrupt pin in the .set_wol() method to PMEB mode
>     immediately silences link-state interrupts, which breaks phylib
>     when interrupts are being used rather than polling mode.
> 3. the code claiming to "reset WOL status" was doing nothing of the
>     sort. Bit 15 in page 0xd8a register 17 controls WoL reset, and
>     needs to be pulsed low to reset the WoL state. This bit was always
>     written as '1', resulting in no reset.
> 4. not resetting WoL state results in the PMEB pin remaining asserted,
>     which in turn leads to an interrupt storm. Only resetting the WoL
>     state in .set_wol() is not sufficient.
> 5. PMEB mode does not allow software detection of the wake-up event as
>     there is no status bit to indicate we received the WoL packet.
> 6. across reboots of at least the Jetson Xavier NX system, the WoL
>     configuration is preserved.
> 
> Fix all of these issues by essentially rewriting the support. We:
> 1. clear the WoL event enable register at probe time.
> 2. detect whether we can support wake-up by having a valid interrupt,
>     and the "wakeup-source" property in DT. If we can, then we mark
>     the MDIO device as wakeup capable, and associate the interrupt
>     with the wakeup source.
> 3. arrange for the get_wol() and set_wol() implementations to handle
>     the case where the MDIO device has not been marked as wakeup
>     capable (thereby returning no WoL support, and refusing to enable
>     WoL support.)
> 4. avoid switching to PMEB mode, instead using INTB mode with the
>     interrupt enable, reconfiguring the interrupt enables at suspend
>     time, and restoring their original state at resume time (we track
>     the state of the interrupt enable register in .config_intr()
>     register.)
> 5. move WoL reset from .set_wol() to the suspend function to ensure
>     that WoL state is cleared prior to suspend. This is necessary
>     after the PME interrupt has been enabled as a second WoL packet
>     will not re-raise a previously cleared PME interrupt.
> 6. when a PME interrupt (for wakeup) is asserted, pass this to the
>     PM wakeup so it knows which device woke the system.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>

This looks much better and straightforward to me, thanks!

> ---
> I've sort-of tested this on the Jetson Xavier NX platform, but it's
> been difficult because it appears that the whole interrupt/wakeup
> stuff for the SoC is foobar in mainline. One gets the choice of
> specifying the GPIO interrupt in DT and have working normal interrupt
> or the power management controller interrupt for the same line and
> having wakeup functional. You can't have both together.
> 
> I'm not sure whether this change should target the net or net-next
> tree; what we have currently in 6.16 is totally and utterly broken,
> so arguably this is a fix - but it's not a regression because 6.16
> is the first kernel that WoL "support" for RTL8211F is in. This is
> also a large change.
> 
> However, I can't see that it was tested, given all the problems
> identified above. As a result, I've taken the decision in this patch
> to not worry about breaking anyone's existing setup.
> 
> So, I have no problem with requiring "wakeup-source" to be added to DT
> for rtl8211f PHYs that are to support wake-up, meaning that they are
> properly wired to support WoL. We can't tell just from having an
> interrupt - not all interrupts on all devices may be wake-up capable.

That seems entirely reasonable to me to expect.
-- 
Florian

Powered by blists - more mailing lists