Re: [PATCH net v2 09/10] net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

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

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

- [Medium] The unbalanced wol_clk disable on resume that this patch
  targets can still happen when enabling the clock fails.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is serious.
- [Medium] The timeout check in bcm_sysport_suspend_to_wol() is inverted.

--

net: systemport: Fix unbalanced Wake-on-LAN clock disable in resume

bcm_sysport_suspend() prepares and enables the Wake-on-LAN clock only
when both device_may_wakeup() and priv->wolopts are true. However,
bcm_sysport_resume() disabled it whenever priv->wolopts was set. This
patch makes resume use the same check as suspend so that
clk_disable_unprepare() is not called unbalanced.

[Severity: Medium]
Does this fully balance the wol_clk enable and disable? In
bcm_sysport_suspend(), the return value of clk_prepare_enable() is
ignored:

    if (device_may_wakeup(d) && priv->wolopts) {
        clk_prepare_enable(priv->wol_clk);
        ret = bcm_sysport_suspend_to_wol(priv);
    }

When clk_prepare_enable() fails, it undoes its own prepare, which leaves
wol_clk neither prepared nor enabled. Suspend still returns 0 as long as
bcm_sysport_suspend_to_wol() succeeds.

On resume, the new check above is true again. clk_disable_unprepare()
then runs on a clock that was never enabled. The common clock framework
would warn with "already disabled" and "already unprepared". WoL would
also be armed without its clock for that cycle.

Should the return value be checked in suspend, and then either skip WoL
or record in a flag that resume tests whether the clock was enabled?

 
 	umac_reset(priv);

[Severity: High]
This isn't a bug introduced by this patch, but there is a related wol_clk
imbalance in the suspend error path. When bcm_sysport_suspend_to_wol()
returns -ETIMEDOUT, bcm_sysport_suspend() has already enabled wol_clk. It
still gates priv->clk and then returns the error:

    if (device_may_wakeup(d) && priv->wolopts) {
        clk_prepare_enable(priv->wol_clk);
        ret = bcm_sysport_suspend_to_wol(priv);
    }

    clk_disable_unprepare(priv->clk);

    return ret;

After a failed ->suspend(), the PM core leaves dev->power.is_suspended
false and does not call bcm_sysport_resume(). The wol_clk enable leaks,
and every later cycle enables it again but disables it only once.

The error path in bcm_sysport_suspend_to_wol() only calls
mpd_enable_set(priv, false). RBUF_ACPI_EN and the RXCHK filter match
bits stay set.

The device is also left with the core clock off, the netdev detached,
the rings freed and NAPI disabled, while netif_running() is still true.
Can the next suspend or interface down then hang?

    bcm_sysport_suspend() or bcm_sysport_stop()
      bcm_sysport_netif_stop()
        napi_disable(&priv->napi)

napi_disable() on a NAPI that is already disabled waits for
NAPIF_STATE_SCHED to clear, with no timeout. Nothing will ever clear it.

Right now -ETIMEDOUT is only returned when RBUF_WOL_MODE appears on the
very last poll (see below). A corrected timeout check would send every
real timeout down this path.

Should this error path undo the wol_clk enable and restore the device
state before it returns?

[Severity: Medium]
This is also a pre-existing issue, but is the timeout check in
bcm_sysport_suspend_to_wol() inverted?

    unsigned int timeout = 1000;
    ...
    do {
        reg = rbuf_readl(priv, RBUF_STATUS);
        if (reg & RBUF_WOL_MODE)
            break;

        udelay(10);
    } while (timeout-- > 0);

    /* Do not leave the UniMAC RBUF matching only MPD packets */
    if (!timeout) {

On a real timeout, the final "0 > 0" test fails and the post-decrement
wraps timeout to UINT_MAX, so !timeout is false. MPD stays enabled,
UniMAC RX is turned on, and 0 is returned even though RBUF never entered
WoL mode.

It also goes wrong the other way. If RBUF_WOL_MODE is seen on the last
iteration, the loop was entered with timeout == 0 and breaks with it
still 0. A successful WoL entry is then reported as -ETIMEDOUT, which
takes the failed-suspend path described above.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922232440.598918-1-florian.fainelli%40broadcom.com