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

On 8/18/26 09:14, Paolo Abeni wrote:
> 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://urldefense.com/v3/__https://netdev-ai.bots.linux.dev/sashiko/__;!!HXCxUKc!1c62Nd-KqBSJoooZYqRXvyr1xBH2SrYG0Qj-vjwRbTXeO230CuhteNJg0IKZip8kZa8eox-PjTqUg6Jbyg$
> ---
> 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?

Very happy to update the commit message here. I don't have the 10G 
version of this hardware and had avoided touching it. Longer-term, as I 
mentioned in the cover letter, I think we want to set up an interrupt 
handler for the link-state change notifications from the PCS, which 
would remove this, remove the similar polling on fixed links, and 
obviate the need for similar behavior in the 10G PCS case.

> [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?

I think probably, but I don't know enough here to be certain. As you 
note, this slightly expands the scope of the pre-existing fixed-link 
case, but I don't know enough about the pieces here to be sure of how 
best to resolve this or the rationale for the current fixed-link polling.
-Nathan

-- 
Nathan Whitehorn (he/him)
Associate Professor
Department of Physics and Astronomy
Michigan State University
Biomedical and Physical Sciences 3225
East Lansing, MI 48824
(517) 884-5563