Re: [PATCH net-next v2 2/2] net: bcmasp: Keep PHY link during WoL sleep cycle

[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: bcmasp: Keep PHY link during WoL sleep cycle

This patch splits the suspend/resume paths so that when WoL is enabled
the PHY is left running and the MAC is only partially re-initialized on
resume. phy_stop() is skipped during suspend and phy_start() is replaced
with a forced relink via phy_trigger_machine() when the MAC was reset.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index e2b51ec903af..9ffd0dfa5969 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -946,7 +946,8 @@ static void bcmasp_netif_deinit(struct net_device *dev)
>  
>  	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
>  
> -	phy_stop(dev->phydev);
> +	if (stop_phy)
> +		phy_stop(dev->phydev);

Skipping phy_stop() in the WoL path means phy_stop_machine() and the
cancel_delayed_work_sync() on phydev->state_queue are no longer executed
before the ASP clock is disabled in bcmasp_interface_suspend().

The PHY state machine runs on system_power_efficient_wq, which is not
WQ_FREEZABLE, so task freeze does not stop it.  If a PHY IRQ fires (or
the polling timer expires) between clk_disable_unprepare(intf->parent->clk)
and the matching clk_prepare_enable() on resume, the state machine can
reach bcmasp_adj_link() via phy_check_link_status() ->
phy_link_up()/phy_link_down() -> phydev->adjust_link.

bcmasp_adj_link() reads and writes UMC_CMD, UMC_EEE_LPI_TIMER,
UMC_EEE_CTRL and RGMII_OOB_CNTRL, which sit behind the disabled ASP
clock.  Can those accesses hang the bus or return 0xffffffff on these
SoCs?

bcmgenet pins phydev->state = PHY_READY during WoL suspend specifically
to freeze the state machine in a no-op state.  Is an equivalent
protection needed here?

> @@ -1385,15 +1386,20 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
>  {
>  	struct device *kdev = &intf->parent->pdev->dev;
>  	struct net_device *dev = intf->ndev;
> +	bool wake;
>  
>  	if (!netif_running(dev))
>  		return 0;
>  
>  	netif_device_detach(dev);
>  
> -	bcmasp_netif_deinit(dev);
> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>  
> -	if (!intf->wolopts) {
> +	bcmasp_netif_deinit(dev, !wake);
> +
> +	if (wake) {
> +		bcmasp_suspend_to_wol(intf);
> +	} else {
>  		bcmasp_phy_hw_unprepare(intf);

[ ... ]

> @@ -1438,17 +1444,34 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
>  	if (ret)
>  		return ret;
>  
> -	bcmasp_core_clock_set_intf(intf, true);
> -
> -	bcmasp_resume_from_wol(intf);
> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>  
> -	bcmasp_phy_hw_prepare(intf);
> +	bcmasp_core_clock_set_intf(intf, true);
>  
> -	umac_reset_and_init(intf, dev->dev_addr);
> +	/* The interface might be HW reset in some suspend modes, so we may
> +	 * need to restore the UNIMAC/PHY if that is the case.
> +	 */
> +	reg = umac_rl(intf, UMC_CMD);
> +	if (wake && (reg & UMC_CMD_RX_EN)) {
> +		umac_enable_set(intf, UMC_CMD_TX_EN, 1);
> +		bcmasp_resume_from_wol(intf);
> +	} else {
> +		bcmasp_phy_hw_prepare(intf);
> +		umac_reset_and_init(intf, dev->dev_addr);
> +	}

In the wake && !(reg & UMC_CMD_RX_EN) branch (MAC was HW reset during
suspend), bcmasp_resume_from_wol() is not called.

bcmasp_suspend_to_wol() unconditionally does two things that live in
the shared parent register block, not the per-interface UMC block:

	wakeup_intr2_core_wl(intf->parent, 0xffffffff,
			     ASP_WAKEUP_INTR2_MASK_CLEAR);

	if (ndev->phydev && ndev->phydev->eee_cfg.eee_enabled &&
	    intf->parent->eee_fixup)
		intf->parent->eee_fixup(intf, true);

The wakeup interrupt mask and the EEE fixup bits (phy_lpi_overwrite in
ASP_EDPKT_SPARE_REG) are not cleared by a reset that only wipes UMC_CMD.

In this branch the wakeup IRQ remains unmasked (bcmasp_isr_wol would
then fire on subsequent MPD/FILT events during normal traffic) and the
EEE fixup stays asserted.  Should this branch also invoke the parts of
bcmasp_resume_from_wol() / eee_fixup(false) that live in the shared
register block?

Separately, bcmasp_suspend_to_wol() explicitly programs
UMC_CMD_PROMISC:

	reg |= UMC_CMD_RX_EN | UMC_CMD_PROMISC;
	umac_wl(intf, reg, UMC_CMD);

In the wake && (reg & UMC_CMD_RX_EN) fast-resume branch, the only
writes to UMC_CMD are:

	umac_enable_set(intf, UMC_CMD_TX_EN, 1);

and whatever bcmasp_resume_from_wol() does, which only touches
UMC_MPD_CTRL and ASP_WAKEUP_INTR2_MASK_SET.  bcmasp_netif_init() calls
bcmasp_set_rx_mode() which manages ASP_RX_FILTER_MDA but not
UMC_CMD_PROMISC, and bcmasp_adj_link() only reprograms PROMISC inside
its if (reg & UMC_CMD_SW_RESET) branch (not taken here).  Since
old_link/old_duplex/old_pause are preserved across the fast path,
adj_link's changed flag stays zero and it returns without rewriting
UMC_CMD.

Does this leave the MAC stuck in UMC_CMD_PROMISC after every WoL
resume, until the interface is toggled down/up?

>  
>  	bcmasp_netif_init(dev);
>  
> -	phy_start(dev->phydev);
> +	/* If HW was reset, we need to force a relink */
> +	if (wake && !(reg & UMC_CMD_RX_EN)) {
> +		mutex_lock(&dev->phydev->lock);
> +		if (dev->phydev->state == PHY_RUNNING)
> +			dev->phydev->state = PHY_UP;
> +		mutex_unlock(&dev->phydev->lock);
> +		phy_trigger_machine(dev->phydev);
> +	} else if (!wake) {
> +		phy_start(dev->phydev);
> +	}

The forced-relink branch only demotes PHY_RUNNING -> PHY_UP before
phy_trigger_machine().  Since phy_stop() was skipped during suspend,
phydev->state is whatever it was at suspend time, which may instead be
PHY_NOLINK (cable was unplugged), PHY_HALTED or PHY_ERROR.

For PHY_NOLINK, _phy_state_machine() runs phy_check_link_status() +
phy_update_stats() only; it does not trigger _phy_start_aneg() (that is
the PHY_UP case).  If the HW reset that cleared UMC_CMD_RX_EN also
touched the PHY chip (shared reset/power domain), autoneg may need to
be restarted.

Does this path handle states other than PHY_RUNNING, and does it
guarantee the PHY is reinitialized and autoneg restarted when the MAC
has been reset?

> +
>  	netif_device_attach(dev);
>  
>  	return 0;