Re: [PATCH net-next v4 1/2] net: macb: Poll for link state changes when using the internal PCS.

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: macb: Poll for link state changes when using the internal PCS.

This sets phylink_pcs.poll on the macb SGMII PCS so that phylink polls the
PCS for link state instead of relying on an onboard PHY, which is needed
for offboard transceivers such as SFPs.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d394f1f43b68..c15a9c7e69d3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1025,6 +1025,7 @@ static int macb_mii_probe(struct net_device *dev)
>  	struct macb *bp = netdev_priv(dev);
>  
>  	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
> +	bp->phylink_sgmii_pcs.poll = true;

[Severity: Medium]
Can this silently disarm Wake-on-LAN across a suspend?

Setting poll on the SGMII PCS makes phylink arm its periodic link_poll
timer, since macb also sets phylink_config.default_an_inband = true in
macb_mii_probe(), so cfg_link_an_mode ends up as MLO_AN_INBAND:

drivers/net/phy/phylink.c:phylink_pcs_poll_start() {
	if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND)
		mod_timer(&pl->link_poll, jiffies + HZ);
}

That is reached from phylink_start() -> phylink_mac_initial_config() ->
phylink_major_config() -> phylink_pcs_poll_start().

The timer is only cancelled by phylink_stop() (timer_delete_sync of
pl->link_poll) or phylink_pcs_poll_stop(), but macb_suspend() skips
phylink_stop() entirely on the Wake-on-LAN path:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
	...
	/* Enable Receive engine */
	macb_writel(bp, NCR, tmp | MACB_BIT(RE));
	...
	enable_irq_wake(bp->queues[0].irq);
	}
	...
	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
	...
}

macb also does not use the Wake-on-LAN aware phylink_suspend(pl, true) /
phylink_resume() pair, so pl->phylink_disable_state stays 0 and
phylink_fixed_poll() keeps re-arming itself and queuing pl->resolve on
system_power_efficient_wq, which is not freezable.

If the poller then observes a PCS link change during the suspend window,
macb_mac_link_down() clears exactly the bit macb_suspend() just set:

drivers/net/ethernet/cadence/macb_main.c:macb_mac_link_down() {
	/* Disable Rx and Tx */
	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);
}

The WOL register and the queue-0 wake interrupt remain enabled, so the
driver still believes Wake-on-LAN is armed while the receive engine is off
and a magic packet or ARP frame can no longer be received.  Clocks stay on
here because macb_runtime_suspend() skips macb_clks_disable() when
device_may_wakeup(), so the register write does reach the MAC.

The opposite direction looks odd too: a poll-triggered macb_mac_link_up()
re-enables RE|TE and re-writes the per-queue IER with
bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP) after the RX queues
have been tied off or set to MACB_BIT(QUEUE_DISABLE) and NAPI has been
disabled, on an IRQ that is an armed system wake source.

phylink documents the intended behaviour for this case in
phylink_suspend():

	/* We do not call mac_link_down() here as we want the
	 * link to remain up to receive the WoL packets.
	 */

Would switching macb_suspend()/macb_resume() to phylink_suspend(bp->phylink,
true) / phylink_resume(), or otherwise stopping the poller on the
Wake-on-LAN path, be preferable now that a periodic PCS poller exists?
-- 
pw-bot: cr