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

On 8/11/26 21:54, Jakub Kicinski 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.
> ---
> 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?

I am really not sure, to be honest. This patch just copies the existing 
logic for the fixed-link case, which also polls. I'm not sure things are 
any different; if this does happen -- and I agree with your logic that 
it could -- it would not be a new behavior.

I am a little reticent to include fixes to this pre-existing situation 
in this patch unless it is unmergeable without doing so, especially 
given that I think a full solution would be a substantially larger 
change, but if it is needed, I would be happy to work on adding it.
-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