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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (1) · Medium: 2 (1) · 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).

- [High] jme_suspend() now calls napi_disable() and
  tasklet_disable(pcc_task) and sets JME_FLAG_WOL_QUIESCE.
- [High] jme_shutdown() checks netif_running() and then calls
  napi_disable() and tasklet_disable() without holding rtnl_lock().
- [Medium] The patch keeps tasklets disabled for long periods while they
  can still be pending or newly scheduled.
- [Medium] Netpoll can queue link-change work after the new quiesce has
  run cancel_work_sync().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but neither
  jme_close() nor jme_remove_one() calls tasklet_kill(&jme->pcc_task).
- [Medium] This problem wasn't introduced by this patch, but
  jme_shutdown() arms WoL while the RX/TX engines are still enabled and…

--

net: jme: keep RX MAC clock enabled for WoL

This commit keeps the RX MAC clock ungated while WoL is armed on JMC250
rev 03 so that Magic Packet wake from S3 works, and gates the clock again
on resume. It also adds a JME_FLAG_WOL_QUIESCE flag. jme_suspend() and
jme_shutdown() now quiesce IRQs, link-change work, NAPI and tasklets
before programming WoL, and jme_resume() undoes this.

> diff --git a/drivers/net/ethernet/jme.c b/drivers/net/ethernet/jme.c
> index 25ec5f757c682..acb4d41f56e10 100644
> --- a/drivers/net/ethernet/jme.c
> +++ b/drivers/net/ethernet/jme.c
> @@ -1502,7 +1502,8 @@ jme_intr_msi(struct jme_adapter *jme, u32 intrstat)
>  		 * all other events are ignored
>  		 */
>  		jwrite32(jme, JME_IEVE, intrstat);
> -		schedule_work(&jme->linkch_task);
> +		if (!test_bit(JME_FLAG_WOL_QUIESCE, &jme->flags))
> +			schedule_work(&jme->linkch_task);

[Severity: Medium]

Can netpoll still queue linkch_task after jme_suspend() or jme_shutdown()
has already run cancel_work_sync()?

With CONFIG_NET_POLL_CONTROLLER and netconsole bound to this device,
jme_netpoll() calls the interrupt handler directly:

drivers/net/ethernet/jme.c:jme_netpoll() {
	local_irq_save(flags);
	jme_intr(dev->irq, dev);
	local_irq_restore(flags);
}

synchronize_irq() doesn't wait for this caller. netpoll_poll_dev() only
checks netif_running(), and netif_device_detach() doesn't clear that.

Suppose the netpoll CPU reads the flag as clear and is then delayed
(NMI, SMI, vCPU preemption) before schedule_work():

CPU1 (netpoll)                      CPU2 (suspend/shutdown)
jme_intr_msi()
  test_bit(WOL_QUIESCE) == 0
                                    set_bit(JME_FLAG_WOL_QUIESCE)
                                    synchronize_irq()
                                    cancel_work_sync(&jme->linkch_task)
  schedule_work(&jme->linkch_task)

In jme_shutdown(), link_changing is still 1. That means
jme_link_change_work() can run all the way through and reach
jme_reset_mac_processor()->jme_mac_rxclk_off() after WoL is armed. The
commit message says this re-gating is prevented.

In jme_suspend(), the work can overlap the RX/TX ring teardown. If it
starts after atomic_dec(&jme->link_changing), it spins in its
link_changing wait loop until resume.

>  		goto out_reenable;
>  	}

[ ... ]

> @@ -3158,6 +3161,21 @@ jme_shutdown(struct pci_dev *pdev)
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct jme_adapter *jme = netdev_priv(netdev);
>  
> +	if (netif_running(netdev)) {
> +		set_bit(JME_FLAG_WOL_QUIESCE, &jme->flags);
> +		netif_device_detach(netdev);
> +		netif_stop_queue(netdev);
> +		jme_stop_irq(jme);
> +		synchronize_irq(pdev->irq);
> +		jme_stop_irq(jme);
> +		cancel_work_sync(&jme->linkch_task);
> +		tasklet_disable(&jme->pcc_task);
> +		napi_disable(&jme->napi);

[Severity: High]

Is anything serializing this with jme_close()?

jme_shutdown() checks netif_running() and then calls napi_disable() and
tasklet_disable() without rtnl_lock(). Meanwhile jme_close() tears down
the same NAPI instance under rtnl. device_shutdown() can run during
reboot -f or kexec -e while userspace (for example NetworkManager) is
still able to close the interface.

After a completed disable, napi_disable_locked() leaves
NAPIF_STATE_SCHED and NAPIF_STATE_NPSVC set, so a second disable spins
here forever:

net/core/dev.c:napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
	...
}

The napi_disable_pending() guard in JME_NAPI_DISABLE() doesn't detect an
earlier disable that has already completed.

If jme_close() runs JME_NAPI_DISABLE() between the netif_running() check
and this napi_disable(), wouldn't the reboot hang here?

If the order is reversed, jme_close() would hang in napi_disable() while
holding rtnl. Its tasklet_kill() calls would also wait forever on
tasklets that are disabled but still scheduled.

Before this patch, jme_shutdown() didn't touch NAPI or tasklet state.
Would wrapping this block in rtnl_lock()/rtnl_unlock(), as
__igb_shutdown() does, avoid the race?

> +		tasklet_disable(&jme->txclean_task);
> +		tasklet_disable(&jme->rxclean_task);
> +		tasklet_disable(&jme->rxempty_task);
> +	}

[Severity: Medium]

Can these disabled tasklets still be pending, and would that leave
ksoftirqd spinning?

tasklet_disable() waits for a running instance but not for a pending
one. jme_intr_msi() also still schedules the tasklets without checking
JME_FLAG_WOL_QUIESCE:

drivers/net/ethernet/jme.c:jme_intr_msi() {
	...
	if (intrstat & INTR_TMINTR) {
		jwrite32(jme, JME_IEVE, INTR_TMINTR);
		tasklet_schedule(&jme->pcc_task);
	}
	...
}

The handler can still run after the quiesce in three ways:

- through the shared INTx line (jme_request_irq() uses IRQF_SHARED)
  when an event bit is latched in JME_IEVE
- from a handler that raced past synchronize_irq()
- from netpoll

When a scheduled tasklet has a non-zero count, tasklet_action_common()
requeues it and raises the softirq again:

kernel/softirq.c:tasklet_action_common() {
	...
		local_irq_disable();
		t->next = NULL;
		*tl_head->tail = t;
		tl_head->tail = &t->next;
		__raise_softirq_irqoff(softirq_nr);
		local_irq_enable();
	...
}

jme_suspend() keeps pcc_task disabled until jme_resume(). The CPU would
then keep running softirqs for the whole suspend period and couldn't go
idle under s2idle.

In jme_shutdown(), pcc_task, txclean_task, rxclean_task and rxempty_task
are never re-enabled. The RX engine is also left running with the RX
clock forced on. Wouldn't the spin then last for the rest of the
shutdown sequence?

Before this patch, pcc_task was never disabled, and the other tasklets
were disabled only briefly in jme_suspend().

> +
>  	jme_powersave_phy(jme);
>  	pci_pme_active(pdev, true);
>  }

[Severity: Medium]

This is a pre-existing issue and wasn't introduced by this patch.
jme_shutdown() arms WoL while the RX/TX engines are still enabled and
the descriptor ring registers still point at host memory. jme_suspend()
quiesces the MAC first:

drivers/net/ethernet/jme.c:jme_suspend() {
	...
		jme_stop_pcc_timer(jme);
		jme_disable_rx_engine(jme);
		jme_disable_tx_engine(jme);
		jme_reset_mac_processor(jme);
	...
}

jme_shutdown() does none of this before jme_powersave_phy(). Then
jme_set_100m_half() writes JME_GHC and forces the PHY to 100M half
while the engines are live. Now that NAPI and the RX tasklets are
disabled in this path, nothing drains the live RX ring either.

Could this leave the MAC in an undefined state for S5 Magic Packet
wake? The commit message notes that S5 wake wasn't validated.

> @@ -3172,11 +3190,16 @@ jme_suspend(struct device *dev)
>  	if (!netif_running(netdev))
>  		return 0;
>  
> -	atomic_dec(&jme->link_changing);
> -
> +	set_bit(JME_FLAG_WOL_QUIESCE, &jme->flags);
>  	netif_device_detach(netdev);
>  	netif_stop_queue(netdev);
>  	jme_stop_irq(jme);
> +	synchronize_irq(jme->pdev->irq);
> +	jme_stop_irq(jme);
> +	cancel_work_sync(&jme->linkch_task);
> +	tasklet_disable(&jme->pcc_task);

[Severity: High]

This isn't a bug introduced by this patch. Since pcc_task is now managed
explicitly here, though: neither jme_close() nor jme_remove_one() calls
tasklet_kill(&jme->pcc_task).

jme_close() kills only the other deferred contexts:

drivers/net/ethernet/jme.c:jme_close() {
	...
	cancel_work_sync(&jme->linkch_task);
	tasklet_kill(&jme->txclean_task);
	tasklet_kill(&jme->rxclean_task);
	tasklet_kill(&jme->rxempty_task);
	...
}

free_irq() waits for running handlers, but it doesn't flush tasklets
those handlers have already scheduled. The last pcc_task scheduled on
INTR_TMINTR may still be pending (for example, ksoftirqd starved by a
real-time task) when jme_remove_one() does this:

	unregister_netdev(netdev);
	iounmap(jme->regs);
	free_netdev(netdev);

jme_pcc_tasklet() would then dereference the freed jme_adapter and do
MMIO through the unmapped jme->regs in
jme_stop_pcc_timer()/jme_start_pcc_timer().

Should jme_close() also call tasklet_kill() on pcc_task?

> +	napi_disable(&jme->napi);

[Severity: High]

What happens if the interface is closed after jme_suspend() and before
jme_resume()?

NAPI stays disabled until jme_resume(), and jme_resume() only undoes
that when netif_running() is true. A close in that window would reach
JME_NAPI_DISABLE() in jme_close() and call napi_disable() a second
time.

After a completed disable, napi_disable_locked() leaves
NAPIF_STATE_SCHED and NAPIF_STATE_NPSVC set. Wouldn't the second call
spin forever while holding rtnl_lock and the netdev instance lock?

One path that can run during suspend is netns cleanup on netns_wq,
which is not freezable. It moves physical devices back to init_net:

netns cleanup work (netns_wq)
  __dev_change_net_namespace()
    netif_close()
      jme_close()
        JME_NAPI_DISABLE()
          napi_disable()

Even if the close got past NAPI, jme_resume() returns early for a
closed device. Neither jme_close() nor jme_open() clears
JME_FLAG_WOL_QUIESCE or re-enables pcc_task. After that, jme_intr_msi()
would never schedule link-change work or re-enable JME_IENS.

jme_shutdown() leaves the same state behind, and nothing ever undoes
it there.

> +	atomic_dec(&jme->link_changing);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261024-jme-wol-v2-submit-v2-1-7a89ef6fb4c9%40gmail.com