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

[Paolo Abeni <pabeni@redhat.com>]

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

This sets poll = true on the macb internal SGMII phylink PCS so phylink
arms its periodic link poll timer, letting link state changes be observed
at the PCS instead of at an onboard PHY (for example with an SFP).

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d394f1f43b685..c15a9c7e69d37 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;
>  	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;

[Severity: Low]
This isn't a bug introduced by this patch, but should phylink_usx_pcs also
get poll = true?  The commit message speaks generally about "an offboard
transceiver (e.g. an SFP)" and about polling "when the PCS is active", yet
only the SGMII PCS is updated.

macb_mac_select_pcs() returns &bp->phylink_usx_pcs for
PHY_INTERFACE_MODE_10GBASER, and phylink only arms the timer for the
currently selected PCS:

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);
}

PHY_INTERFACE_MODE_10GBASER is one of the modes accepted as in-band by
phylink_parse_mode(), so with managed = "in-band-status" the USX PCS ends up
in MLO_AN_INBAND but with poll == false.

macb_usx_pcs_get_state() is a pure register read:

	val = gem_readl(bp, USX_STATUS);
	state->link = !!(val & GEM_BIT(USX_BLOCK_LOCK));

and macb has no phylink_pcs_change()/phylink_mac_change() call anywhere, and
poll_fixed_state/get_fixed_state are only installed for SGMII.  Does that
mean USX_BLOCK_LOCK transitions are only sampled when something else (such
as an SFP loss-of-signal transition) forces a resolve?

The USX PCS behaves exactly as it did before this patch, so this is a
pre-existing gap rather than a new failure.  Could the commit message say
whether leaving the USX PCS unpolled is deliberate?

[Severity: Medium]
This is a pre-existing issue, but arming the poll timer adds one more
asynchronous context that touches the hardware, and macb_suspend() does not
stop phylink on the Wake-on-LAN path:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
	...
}

phylink_suspend() is never called either, so with WoL enabled
pl->phylink_disable_state stays 0 and phylink_resolve() keeps running (clocks
stay on because macb_suspend() skips pm_runtime_force_suspend() when
device_may_wakeup() is true).

If the PCS link state changes after the WoL arming sequence, can the resolve
undo it?  macb_mac_link_down() clears the receive engine that magic-packet
and ARP wake detection rely on:

	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);

and macb_mac_link_up() goes the other way, re-unmasking every queue while
NAPI is disabled and re-enabling RE|TE:

	queue_writel(queue, IER,
		     bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP));
	...
	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));

phylink_resolve() does not consult netif_device_present(), so nothing appears
to gate this against the suspended state.

The same 1 Hz poll already existed in the baseline for MLO_AN_FIXED SGMII:

	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
		bp->phylink_config.poll_fixed_state = true;
		bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;

and phylink_phy_change()/phylink_sfp_link_down() can already drive a resolve
during a WoL-armed suspend, so this patch only adds another trigger for the
in-band SGMII case.  Would calling phylink_suspend() (or phylink_stop()) on
the WoL path in macb_suspend() be the right fix?
-- 
This is an AI-generated review.