Re: [PATCH net-next 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 says "Blindly sending out since there will be a repost,
anyway."
---
net: bcmasp: Keep PHY link during WoL sleep cycle

We currently more or less restart all the HW on resume. Since we also
stop the PHY, it takes a while for the PHY link to be re-negotiated on
resume. Instead of doing a full restart, we keep the HW state and the
PHY link, that way we can resume network traffic with a much smaller
delay.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index aff0a6d84126..bab2a4f82e4e 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
[ ... ]
> @@ -1383,15 +1384,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);
>  
>  		/* If Wake-on-LAN is disabled, we can safely
> @@ -1400,9 +1406,6 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
>  		bcmasp_core_clock_set_intf(intf, false);
>  	}
>  
> -	if (device_may_wakeup(kdev) && intf->wolopts)
> -		bcmasp_suspend_to_wol(intf);
> -
>  	clk_disable_unprepare(intf->parent->clk);

If wake is true, phy_stop() is skipped during bcmasp_netif_deinit().
This leaves the phylib state machine active and polling on its delayed
workqueue.

At the end of bcmasp_interface_suspend(), the main MAC clock is
unconditionally disabled via clk_disable_unprepare(intf->parent->clk).

If the PHY state machine executes while this clock is disabled, will it
attempt to access PHY registers over the MDIO bus, resulting in a
synchronous external abort (bus fault) or kernel panic?

Could the active state machine also concurrently invoke the MAC's
adjust_link callback while bcmasp_interface_resume() is executing,
causing a data race on MAC registers?

>  	return 0;
>  }
> @@ -1426,8 +1429,11 @@ static void bcmasp_resume_from_wol(struct bcmasp_intf *intf)
>  
>  int bcmasp_interface_resume(struct bcmasp_intf *intf)
>  {
[ ... ]
> +	/* 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);
> +	}

If wake is true but the MAC hardware was reset (reg & UMC_CMD_RX_EN
is false), the code branches to the else block and skips calling
bcmasp_resume_from_wol().

Since bcmasp_resume_from_wol() is responsible for reversing the WoL
configuration applied during suspend (like restoring the EEE fixup state
and masking the wakeup interrupt), does skipping this teardown leave the
wakeup interrupt unmasked during normal operation and risk an interrupt
storm?

>  	bcmasp_netif_init(dev);
>  
> -	phy_start(dev->phydev);
> +	/* If HW was reset, we need to force a link re-negotiation */
> +	if (wake && !(reg & UMC_CMD_RX_EN)) {
> +		phy_restart_aneg(dev->phydev);

If the user has explicitly configured forced speed and duplex with
autonegotiation disabled, will this unconditional call to
phy_restart_aneg() override the user's configuration? It appears to
enable autonegotiation without checking if phydev->autoneg ==
AUTONEG_ENABLE.

Also, because phy_stop() was skipped during suspend, the PHY state
machine is still actively running in the background. Does calling
phy_restart_aneg() without holding phydev->lock create a data race
with the concurrent PHY state machine?

> +		phy_trigger_machine(dev->phydev);
> +	} else if (!wake) {
> +		phy_start(dev->phydev);
> +	}
>  
>  	netif_device_attach(dev);