Re: [PATCH net] net: jme: keep RX MAC clock enabled for WoL

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch turns the RX MAC clock on before arming WoL, but
  resume never turns it back off.
- [Medium] The patch's guarantee ('Keep the RX MAC clock running while
  WoL is armed') can be undone by jme_link_change_work() running at the…

--

net: jme: keep RX MAC clock enabled for WoL

This patch turns the RX MAC clock back on in jme_powersave_phy() just
before jme_clear_pm_enable_wol() arms WoL. Earlier in the suspend path the
MAC reset leaves GPREG1_RXCLKOFF set. On a JMC250 rev 03, Magic Packet
detection does not work while the RX MAC clock is gated.

> diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> index 25ec5f757c68..27ac7405f76f 100644
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -1892,6 +1892,7 @@ jme_powersave_phy(struct jme_adapter *jme)
>  		jme_set_100m_half(jme);
>  		if (jme->reg_pmcs & (PMCS_LFEN | PMCS_LREN))
>  			jme_wait_link(jme);
> +		jme_mac_rxclk_on(jme);

[Severity: Medium]
Does anything turn the RX MAC clock back off on resume?

jme_mac_rxclk_on() changes the cached copy as well as the register:

    jme->reg_gpreg1 &= ~GPREG1_RXCLKOFF;
    jwrite32f(jme, JME_GPREG1, jme->reg_gpreg1);

jme_resume() calls jme_clear_pm_disable_wol(), jme_phy_on(),
jme_set_link_ksettings() or jme_reset_phy_processor(),
jme_phy_calibration(), jme_phy_setEA(), jme_reset_link() and
jme_start_irq(). None of them calls jme_mac_rxclk_off() or
jme_reset_mac_processor().

jme_link_change_work() also skips its disable/reset block the first time
it runs after resume, because jme_suspend() already called
netif_carrier_off():

    if (netif_carrier_ok(netdev)) {
        jme_disable_rx_engine(jme);
        jme_disable_tx_engine(jme);
        jme_reset_mac_processor(jme);
        ...

After each WoL suspend/resume, if the link stays down, the RX MAC clock
keeps running while the RX engine is disabled. Before this patch it was
gated in that state.

If the link comes up, jme_check_link() writes the cached reg_gpreg1, which
no longer has RXCLKOFF set, to JME_GPREG1. jme_enable_rx_engine() then
programs RXCS, RXDBA, RXNDA and RXQDC while the clock is already running.
Its trailing jme_mac_rxclk_on() then has nothing to do.

Doesn't this skip the order that commit 854a2e7c331a ("jme: Safer MAC
processor reset sequence") introduced, where the RX engine is programmed
with the clock gated and the clock is started afterwards?

The commit message says "Keep the RX MAC clock running while WoL is
armed". However, the clock stays on after WoL is disarmed, until the next
link-down runs jme_disable_rx_engine() and jme_reset_mac_processor().

Would calling jme_mac_rxclk_off() in jme_resume(), for example right after
jme_clear_pm_disable_wol(), keep the WoL fix and restore the previous
state?

[Severity: Medium]
Can jme_link_change_work() running at the same time gate the clock again
after this call?

jme_link_change_work() calls jme_disable_rx_engine() and
jme_reset_mac_processor(). Both end in jme_mac_rxclk_off(). Neither
caller of jme_powersave_phy() waits for linkch_task to finish first.

On the suspend path, jme_suspend() only does:

    atomic_dec(&jme->link_changing);

The work uses that atomic as an ad-hoc lock:

    while (!atomic_dec_and_test(&jme->link_changing)) {
        atomic_inc(&jme->link_changing);
        ...
    }

If the work has already passed that gate, the counter just drops to -1 and
suspend continues without waiting. jme_stop_irq() masks the device
interrupt but does not flush or cancel linkch_task. schedule_work() puts
the work on system_wq, which is not freezable.

Suppose the work is preempted after its netif_carrier_ok() check.
jme_suspend() can then run the new jme_mac_rxclk_on() and arm PMCS. When
the work resumes, it resets the MAC and gates the clock. If the link is
down, the work never turns the clock back on.

On the shutdown path:

    jme_shutdown()
        jme_powersave_phy(jme);
        pci_pme_active(pdev, true);

there is no jme_stop_irq(), no link_changing gate and no
cancel_work_sync(). With the interface up, jme_set_100m_half() forces
100M/half. That drops the link, raises INTR_LINKCH and schedules
linkch_task. With the default reg_pmcs = PMCS_MFEN, jme_wait_link() is
skipped, so jme_shutdown() returns right after the clock is turned on.

If power-off happens before the link comes back up, could the work reset
the MAC and leave WoL armed with RXCLKOFF set?

jme_mac_rxclk_on(), jme_mac_rxclk_off() and jme_check_link() also do
unlocked read-modify-writes of jme->reg_gpreg1, so concurrent updates to
that cached value can be lost.

The missing synchronization with the link-change work and the atomic gate
are older than this patch. But this fix only works if nothing gates the
clock after it is turned on.

Should linkch_task be stopped from rescheduling and drained (for example
with cancel_work_sync()) before WoL is armed?

>  		jme_clear_pm_enable_wol(jme);
>  	} else {
>  		jme_phy_off(jme);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-jme-wol-submit-v1-1-9cc033e259e4%40gmail.com