Re: [Linux-stm32] [PATCH RFC net-next 6/7] net: stmmac: add helpers to indicate WoL enable status

[Gatien CHEVALLIER <gatien.chevallier@foss.st.com>]

On 7/29/25 16:44, Russell King (Oracle) wrote:
> On Tue, Jul 29, 2025 at 03:10:56PM +0200, Gatien CHEVALLIER wrote:
>>
>>
>> On 7/29/25 14:45, Russell King (Oracle) wrote:
>>> On Tue, Jul 29, 2025 at 10:03:22AM +0100, Russell King (Oracle) wrote:
>>>> With Thierry's .dts patch, PHY interrupts completely stop working, so
>>>> we don't get link change notifications anymore - and we still don't
>>>> seem to be capable of waking the system up with the PHY interrupt
>>>> being asserted.
>>>>
>>>> Without Thierry's .dts patch, as I predicted, enabling WoL at the
>>>> PHY results in Bad Stuff happening - the code in the realtek driver
>>>> for WoL is quite simply broken and wrong.
>>>>
>>>> Switching the pin from INTB mode to PMEB mode results in:
>>>> - No link change interrupts once WoL is enabled
>>>> - The interrupt output being stuck at active level, causing an
>>>>     interrupt storm and the interrupt is eventually disabled.
>>>>     The PHY can be configured to pulse the PMEB or hold at an active
>>>>     level until the WoL is cleared - and by default it's the latter.
>>>>
>>>> So, switching the interrupt pin to PMEB mode is simply wrong and
>>>> breaks phylib. I guess the original WoL support was only tested on
>>>> a system which didn't use the PHY interrupt, only using the interrupt
>>>> pin for wake-up purposes.
>>>>
>>>> I was working on the realtek driver to fix this, but it's pointless
>>>> spending time on this until the rest of the system can wake up -
>>>> and thus the changes can be tested. This is where I got to (and
>>>> includes work from both Thierry and myself, so please don't pick
>>>> this up as-is, because I can guarantee that you'll get the sign-offs
>>>> wrong! It's a work-in-progress, and should be a series for submission.)
>>>
>>> Okay, with this patch, wake-up now works on the PHY interrupt line, but
>>> because normal interrupts aren't processed, the interrupt output from
>>> the PHY is stuck at active level, so the system immediately wakes up
>>> from suspend.
>>>
>>
>> If I'm following correctly, you do not use the PMEB mode and share
>> the same pin for WoL and regular interrupts (INTB mode)?
> 
> As the driver is currently structured, switching the pin to PMEB mode
> in .set_wol() breaks phylib, since as soon as one enables WoL at the
> PHY, link state interrupts are no longer delivered.
> 
> It may be appropriate to switch the pin to PMEB mode in the suspend
> method while waiting for a wakeup but we need code in place to deal
> with the resulting interrupt storm (by clearing the wakeup) and that
> code is missing.
> 
> The other approach would be to leave the pin in INTB mode, and
> reconfigure the interrupt enable register (INER) to allow WoL
> interrupts, maybe disabling link state interrupts (more on that
> below.) This has the advantage that reading the interrupt status
> register clears the interrupt - and that code already exists so we
> avoid the interrupt storm.
> 
>>> Without the normal interrupt problem solved, there's nothing further
>>> I can do on this.
>>>
>>> Some of the open questions are:
>>> - whether we should configure the WoL interrupt in the suspend/resume
>>>     function
>>
>> For the LAN8742 PHY with which I worked with, the recommendation when
>> using the same pin for WoL and regular interrupt management is to mask
>> regular interrupt and enable the WoL IIRC.
> 
> That's only really appropriate if the MAC isn't involved in WoL. Let's
> say that the PHY can support magic-packet WoL, but the MAC can also
> support unicast frame WoL, and the user has enabled both.
> 

Oops, just saw that I mispronounced your name in a previous message,
sorry for that.

For STMMAC:
I'm a bit lost there. I may be missing something. I thought that using
PHY WoL (therefore having STMMAC_FLAG_USE_PHY_WOL) superseded the MAC
WoL usage.

In stmmac_hw_init():

if (priv->plat->pmt) {
         dev_info(priv->device, "Wake-Up On Lan supported\n");
         device_set_wakeup_capable(priv->device, 1);
}

but above:

priv->plat->pmt = priv->dma_cap.pmt_remote_wake_up &&
!(priv->plat->flags & STMMAC_FLAG_USE_PHY_WOL);

Then, in stmmac_set_wol(), it's either set the WoL for the
PHY or the MAC?

Can you point me to the part I'm missing please?

> The system goes to sleep (e.g. overnight) and during the night, there's
> a hiccup which causes the link to drop and re-establish at a slower
> speed.
> 
> Since the MAC has not been reconfigured (because the link state
> interrupt is disabled, and thus won't wake the system) the MAC can now
> no longer receive unicast frames to check whether they match the
> despired system wakeup condition.
> 
> So, this poses another question: do we really want to support
> simultaneous PHY and MAC level WoL support, or should we only allow
> one or other device to support WoL?
> 
> If we explicitly deny the WoL at both approach, then we don't have
> to care about link state interrupts, because the PHY will be able
> to cope with the different link speed without needing to wake the
> iystem to reconfigure the network interface for the new link
> parameters.
> 
>> This prevents the PHY from waking up from undesired events while still
>> being able use the WoL capability and should be done in suspend() /
>> resume() callbacks. I guess this means also that you share the same
>> interrupt handler that must manage both WoL events and regular events.
>>
>> On the other hand, on the stm32mp135f-dk, the nPME pin (equivalent to
>> PMEB IIUC) is wired and is different from the nINT pin. Therefore, I
>> guess it should not be done during suspend()/resume() and it really
>> depends on how the PHY is wired. Because if a WoL event is received at
>> runtime, then the PHY must clear the flags otherwise the WoL event won't
>> trigger a system wakeup afterwards.
>>
>> I need to look at how the PHYs can handle two different interrupts.
> 
> The RTL8211F only has one pin (pin 31) which can be used in INTB mode
> or PMEB mode. You can't have independent interrupt and wakeup signals
> with this PHY.
>