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

[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: 1 (1) · Low: 1

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] Incomplete fix: only the SGMII PCS gets link-state polling;
  the sibling internal 10GBASE-R PCS (`bp->phylink_usx_pcs`) is left…
- [Low] The commit carries no `Fixes:` tag although, for the in-band
  SGMII/PCS configuration without an attached PHY, it repairs a pre-…

Pre-existing issues:
- [Medium] Enabling `poll` on the SGMII PCS creates a self-retriggering
  periodic phylink resolve path that is not quiesced by macb's Wake-on-…

--

net: macb: Poll for link state changes when using the internal PCS.

This sets phylink_pcs.poll on the macb SGMII PCS so phylink arms its
periodic resolve timer and re-reads the PCS link state. That covers the
case where the link is terminated at an offboard transceiver such as an
SFP rather than an onboard PHY.

No Link: tags are present in the commit header.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d1d1c273f4bbf..d48ad5b587d09 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 *netdev)
>  	struct macb *bp = netdev_priv(netdev);
>  
>  	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
> +	bp->phylink_sgmii_pcs.poll = true;

[Severity: Medium]
Should bp->phylink_usx_pcs get the same treatment?  It is initialised on
the very next line and keeps the zero-initialised poll == false.

macb_mac_select_pcs() hands out the USX instance for 10GBASE-R:

	if (interface == PHY_INTERFACE_MODE_10GBASER)
		return &bp->phylink_usx_pcs;

and macb_usx_pcs_get_state() reads a level status bit in exactly the same
way that macb_pcs_get_state() reads PCSSTS:

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

phylink_pcs_poll_start() arms the timer only for the currently selected
PCS:

	if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND)
		mod_timer(&pl->link_poll, jiffies + HZ);

For an SFP, phylink_sfp_set_config() forces MLO_AN_INBAND, and macb has no
other link notification for this path: phylink_pcs_change() is never
called, and MACB_ISR_LINK_* exists only as a register bit definition in
macb.h, never written to IER nor handled in macb_interrupt().  The
phylink_config.poll_fixed_state / get_fixed_state pair set below is only
consulted by phylink_start() for MLO_AN_FIXED:

	if (pl->cfg_link_an_mode == MLO_AN_FIXED)
		poll |= pl->config->poll_fixed_state;

So with a 10GBASE-R module, does USX_BLOCK_LOCK coming up late leave the
carrier down permanently, and does a later loss of block lock leave a
stale carrier up until some unrelated phylink event (module removal, ip
link down/up) forces a re-resolve?

The follow-up patch in the series, "net: macb: add support for
1000BASE-X autonegotiation to PCS", extends only the SGMII PCS, so this
stays the same at the end of the series.

[Severity: Low]
Would a Fixes: tag be appropriate here?  For the in-band SGMII/PCS
configuration with no attached PHY, nothing re-sampled the PCS before
this change: phylink honours poll_fixed_state/get_fixed_state only in
MLO_AN_FIXED, macb installs no PCS link-change notification, and
pl->link_poll was never armed for the PCS path, so macb_pcs_get_state()
ran only at major-config time.

The message carries only Signed-off-by and Reviewed-by, and commit
8876769bf936 ("net: macb: convert to phylink PCS") is not referenced, so
backporters have nothing to key off if this is meant for stable.

If the intent is purely to enable a configuration that was not supported
before (direct PCS-to-SFP attachment, as described in the follow-up
patch), then net-next without a Fixes: tag is fine — could the intent be
made explicit?

[Severity: Medium]
This isn't a bug introduced by this patch, but arming the periodic poll
adds one more trigger to a path that macb_suspend() does not quiesce when
Wake-on-LAN is armed:

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

phylink_stop() is the only caller of timer_delete_sync(&pl->link_poll) and
the resolve flush.  With WoL enabled it is skipped, while
phylink_fixed_poll() keeps re-arming itself and queues the resolve on
system_power_efficient_wq, which is not WQ_FREEZABLE:

	mod_timer(t, jiffies + HZ);
	phylink_run_resolve(pl);
	...
	if (!pl->phylink_disable_state)
		queue_work(system_power_efficient_wq, &pl->resolve);

phylink_resolve() has no netif_device_present() or suspend guard, so a
polled state change calls macb_mac_link_down() (clears NCR RE|TE, masks
queue interrupts) or macb_mac_link_up() (re-enables RE|TE, rewrites
NCFGR, writes per-queue IER, wakes TX queues) on a detached netdev whose
MAC was just armed for WoL.  Can that drop a magic/ARP wake frame while
RE is clear, or re-enable non-WoL queue interrupts on a wake-armed IRQ
line and cause a spurious wake?

The same hazard already exists without this change: macb sets
phylink_config.mac_managed_pm = true, so mdio_bus_phy_suspend() returns
early and the PHY state machine keeps driving resolves during WoL
suspend, and sfp_poll() does the same for SFP setups.  So the underlying
gap looks like macb_suspend()'s WoL path not stopping link management
rather than this one-line flag.

>  	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
>  
>  	bp->phylink_config.dev = &netdev->dev;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922151429.23272-1-nwhitehorn%40pa.msu.edu