[PATCH 0/2] Keep PHY link during WoL sleep cycle

[PATCH 0/2] Keep PHY link during WoL sleep cycle

[Justin Chen]

First we divide the init/deinit path to allow for a partial init/deinit
during a sleep cycle. We also remove some unnecessary small functions at
the same time.

Then we modify the suspend and resume path to allow for a partial bring
down and bring up. This allow us to keep the PHY link up and to resume
network traffic much quicker.

Justin Chen (2):
  net: bcmasp: Divide init to allow partial bring up
  net: bcmasp: Keep PHY link during WoL sleep cycle

 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 246 +++++++++---------
 1 file changed, 125 insertions(+), 121 deletions(-)

-- 
2.34.1

[PATCH net-next 1/2] net: bcmasp: Divide init to allow partial bring up

[Justin Chen]

To prepare for a partial bring up of the interface during resume,
we break apart the bcmasp_netif_init() function into smaller chunks
that can be called as necessary. Also consolidate some functions that
do not need to be standalone.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 208 ++++++++----------
 1 file changed, 96 insertions(+), 112 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index ec63f50a849e..aff0a6d84126 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -344,40 +344,35 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-static void bcmasp_netif_start(struct net_device *dev)
+static void umac_reset_and_init(struct bcmasp_intf *intf,
+				const unsigned char *addr)
 {
-	struct bcmasp_intf *intf = netdev_priv(dev);
-
-	bcmasp_set_rx_mode(dev);
-	napi_enable(&intf->tx_napi);
-	napi_enable(&intf->rx_napi);
+	struct phy_device *phydev = intf->ndev->phydev;
+	u32 mac0, mac1;
 
-	bcmasp_enable_rx_irq(intf, 1);
-	bcmasp_enable_tx_irq(intf, 1);
-	bcmasp_enable_phy_irq(intf, 1);
-
-	phy_start(dev->phydev);
-}
-
-static void umac_reset(struct bcmasp_intf *intf)
-{
 	umac_wl(intf, 0x0, UMC_CMD);
 	umac_wl(intf, UMC_CMD_SW_RESET, UMC_CMD);
 	usleep_range(10, 100);
 	/* We hold the umac in reset and bring it out of
 	 * reset when phy link is up.
 	 */
-}
 
-static void umac_set_hw_addr(struct bcmasp_intf *intf,
-			     const unsigned char *addr)
-{
-	u32 mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
-		    addr[3];
-	u32 mac1 = (addr[4] << 8) | addr[5];
+	umac_wl(intf, 0x800, UMC_FRM_LEN);
+	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
+	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
+
+	mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
+		addr[3];
+	mac1 = (addr[4] << 8) | addr[5];
 
 	umac_wl(intf, mac0, UMC_MAC0);
 	umac_wl(intf, mac1, UMC_MAC1);
+
+	/* Reset shadow values since we reset the umac */
+	intf->old_duplex = -1;
+	intf->old_link = -1;
+	intf->old_pause = -1;
+	phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
 }
 
 static void umac_enable_set(struct bcmasp_intf *intf, u32 mask,
@@ -401,13 +396,6 @@ static void umac_enable_set(struct bcmasp_intf *intf, u32 mask,
 		usleep_range(1000, 2000);
 }
 
-static void umac_init(struct bcmasp_intf *intf)
-{
-	umac_wl(intf, 0x800, UMC_FRM_LEN);
-	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
-	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
-}
-
 static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
 {
 	struct bcmasp_intf_stats64 *stats = &intf->stats64;
@@ -927,6 +915,14 @@ static void bcmasp_rgmii_mode_en_set(struct bcmasp_intf *intf, bool enable)
 	rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
 }
 
+static void bcmasp_phy_hw_unprepare(struct bcmasp_intf *intf)
+{
+	if (intf->internal_phy)
+		bcmasp_ephy_enable_set(intf, false);
+	else
+		bcmasp_rgmii_mode_en_set(intf, false);
+}
+
 static void bcmasp_netif_deinit(struct net_device *dev)
 {
 	struct bcmasp_intf *intf = netdev_priv(dev);
@@ -984,11 +980,7 @@ static int bcmasp_stop(struct net_device *dev)
 
 	phy_disconnect(dev->phydev);
 
-	/* Disable internal EPHY or external PHY */
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, false);
-	else
-		bcmasp_rgmii_mode_en_set(intf, false);
+	bcmasp_phy_hw_unprepare(intf);
 
 	/* Disable the interface clocks */
 	bcmasp_core_clock_set_intf(intf, false);
@@ -998,10 +990,15 @@ static int bcmasp_stop(struct net_device *dev)
 	return 0;
 }
 
-static void bcmasp_configure_port(struct bcmasp_intf *intf)
+static void bcmasp_phy_hw_prepare(struct bcmasp_intf *intf)
 {
 	u32 reg, id_mode_dis = 0;
 
+	if (intf->internal_phy)
+		bcmasp_ephy_enable_set(intf, true);
+	else
+		bcmasp_rgmii_mode_en_set(intf, true);
+
 	reg = rgmii_rl(intf, RGMII_PORT_CNTRL);
 	reg &= ~RGMII_PORT_MODE_MASK;
 
@@ -1036,26 +1033,8 @@ static void bcmasp_configure_port(struct bcmasp_intf *intf)
 	rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
 }
 
-static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
+static phy_interface_t bcmasp_phy_iface_for_connect(phy_interface_t mode)
 {
-	struct bcmasp_intf *intf = netdev_priv(dev);
-	phy_interface_t phy_iface = intf->phy_interface;
-	u32 phy_flags = PHY_BRCM_AUTO_PWRDWN_ENABLE |
-			PHY_BRCM_DIS_TXCRXC_NOENRGY |
-			PHY_BRCM_IDDQ_SUSPEND;
-	struct phy_device *phydev = NULL;
-	int ret;
-
-	/* Always enable interface clocks */
-	bcmasp_core_clock_set_intf(intf, true);
-
-	/* Enable internal PHY or external PHY before any MAC activity */
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, true);
-	else
-		bcmasp_rgmii_mode_en_set(intf, true);
-	bcmasp_configure_port(intf);
-
 	/* This is an ugly quirk but we have not been correctly
 	 * interpreting the phy_interface values and we have done that
 	 * across different drivers, so at least we are consistent in
@@ -1081,46 +1060,43 @@ static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
 	 * affected because they use different phy_interface_t values
 	 * or the Generic PHY driver.
 	 */
-	switch (phy_iface) {
+	switch (mode) {
 	case PHY_INTERFACE_MODE_RGMII:
-		phy_iface = PHY_INTERFACE_MODE_RGMII_ID;
-		break;
+		return PHY_INTERFACE_MODE_RGMII_ID;
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		phy_iface = PHY_INTERFACE_MODE_RGMII_RXID;
-		break;
+		return PHY_INTERFACE_MODE_RGMII_RXID;
 	default:
-		break;
+		return mode;
 	}
+}
 
-	if (phy_connect) {
-		phydev = of_phy_connect(dev, intf->phy_dn,
-					bcmasp_adj_link, phy_flags,
-					phy_iface);
-		if (!phydev) {
-			ret = -ENODEV;
-			netdev_err(dev, "could not attach to PHY\n");
-			goto err_phy_disable;
-		}
-
-		if (intf->internal_phy)
-			dev->phydev->irq = PHY_MAC_INTERRUPT;
-
-		/* Indicate that the MAC is responsible for PHY PM */
-		phydev->mac_managed_pm = true;
-
-		/* Set phylib's copy of the LPI timer */
-		phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
+static int bcmasp_phy_attach(struct bcmasp_intf *intf)
+{
+	u32 phy_flags = PHY_BRCM_AUTO_PWRDWN_ENABLE |
+			PHY_BRCM_DIS_TXCRXC_NOENRGY |
+			PHY_BRCM_IDDQ_SUSPEND;
+	struct phy_device *phydev;
+	phy_interface_t phy_iface;
+
+	phy_iface = bcmasp_phy_iface_for_connect(intf->phy_interface);
+	phydev = of_phy_connect(intf->ndev, intf->phy_dn,
+				bcmasp_adj_link, phy_flags,
+				phy_iface);
+	if (!phydev) {
+		netdev_err(intf->ndev, "could not attach to PHY\n");
+		return -ENODEV;
 	}
+	if (intf->internal_phy)
+		intf->ndev->phydev->irq = PHY_MAC_INTERRUPT;
 
-	umac_reset(intf);
-
-	umac_init(intf);
+	phydev->mac_managed_pm = true;
 
-	umac_set_hw_addr(intf, dev->dev_addr);
+	return 0;
+}
 
-	intf->old_duplex = -1;
-	intf->old_link = -1;
-	intf->old_pause = -1;
+static void bcmasp_netif_init(struct net_device *dev)
+{
+	struct bcmasp_intf *intf = netdev_priv(dev);
 
 	bcmasp_init_tx(intf);
 	netif_napi_add_tx(intf->ndev, &intf->tx_napi, bcmasp_tx_poll);
@@ -1132,18 +1108,13 @@ static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
 
 	intf->crc_fwd = !!(umac_rl(intf, UMC_CMD) & UMC_CMD_CRC_FWD);
 
-	bcmasp_netif_start(dev);
-
-	netif_start_queue(dev);
-
-	return 0;
+	bcmasp_set_rx_mode(dev);
+	napi_enable(&intf->tx_napi);
+	napi_enable(&intf->rx_napi);
 
-err_phy_disable:
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, false);
-	else
-		bcmasp_rgmii_mode_en_set(intf, false);
-	return ret;
+	bcmasp_enable_rx_irq(intf, 1);
+	bcmasp_enable_tx_irq(intf, 1);
+	bcmasp_enable_phy_irq(intf, 1);
 }
 
 static int bcmasp_open(struct net_device *dev)
@@ -1161,14 +1132,28 @@ static int bcmasp_open(struct net_device *dev)
 	if (ret)
 		goto err_free_mem;
 
-	ret = bcmasp_netif_init(dev, true);
-	if (ret) {
-		clk_disable_unprepare(intf->parent->clk);
-		goto err_free_mem;
-	}
+	bcmasp_core_clock_set_intf(intf, true);
+
+	bcmasp_phy_hw_prepare(intf);
+
+	ret = bcmasp_phy_attach(intf);
+	if (ret)
+		goto err_phy_attach;
+
+	umac_reset_and_init(intf, dev->dev_addr);
+
+	bcmasp_netif_init(dev);
+
+	phy_start(dev->phydev);
+
+	netif_start_queue(dev);
 
 	return ret;
 
+err_phy_attach:
+	bcmasp_phy_hw_unprepare(intf);
+	bcmasp_core_clock_set_intf(intf, false);
+	clk_disable_unprepare(intf->parent->clk);
 err_free_mem:
 	bcmasp_reclaim_free_buffers(intf);
 
@@ -1407,10 +1392,7 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
 	bcmasp_netif_deinit(dev);
 
 	if (!intf->wolopts) {
-		if (intf->internal_phy)
-			bcmasp_ephy_enable_set(intf, false);
-		else
-			bcmasp_rgmii_mode_en_set(intf, false);
+		bcmasp_phy_hw_unprepare(intf);
 
 		/* If Wake-on-LAN is disabled, we can safely
 		 * disable the network interface clocks.
@@ -1454,17 +1436,19 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
 	if (ret)
 		return ret;
 
-	ret = bcmasp_netif_init(dev, false);
-	if (ret)
-		goto out;
+	bcmasp_core_clock_set_intf(intf, true);
 
 	bcmasp_resume_from_wol(intf);
 
+	bcmasp_phy_hw_prepare(intf);
+
+	umac_reset_and_init(intf, dev->dev_addr);
+
+	bcmasp_netif_init(dev);
+
+	phy_start(dev->phydev);
+
 	netif_device_attach(dev);
 
 	return 0;
-
-out:
-	clk_disable_unprepare(intf->parent->clk);
-	return ret;
 }
-- 
2.34.1

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

[Justin Chen]

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.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 48 +++++++++++++------
 1 file changed, 34 insertions(+), 14 deletions(-)

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
@@ -923,7 +923,7 @@ static void bcmasp_phy_hw_unprepare(struct bcmasp_intf *intf)
 		bcmasp_rgmii_mode_en_set(intf, false);
 }
 
-static void bcmasp_netif_deinit(struct net_device *dev)
+static void bcmasp_netif_deinit(struct net_device *dev, bool stop_phy)
 {
 	struct bcmasp_intf *intf = netdev_priv(dev);
 	u32 reg, timeout = 1000;
@@ -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);
 
 	umac_enable_set(intf, UMC_CMD_RX_EN, 0);
 
@@ -974,7 +975,7 @@ static int bcmasp_stop(struct net_device *dev)
 	/* Stop tx from updating HW */
 	netif_tx_disable(dev);
 
-	bcmasp_netif_deinit(dev);
+	bcmasp_netif_deinit(dev, true);
 
 	bcmasp_reclaim_free_buffers(intf);
 
@@ -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);
 
 	return 0;
@@ -1426,8 +1429,11 @@ static void bcmasp_resume_from_wol(struct bcmasp_intf *intf)
 
 int bcmasp_interface_resume(struct bcmasp_intf *intf)
 {
+	struct device *kdev = &intf->parent->pdev->dev;
 	struct net_device *dev = intf->ndev;
+	bool wake;
 	int ret;
+	u32 reg;
 
 	if (!netif_running(dev))
 		return 0;
@@ -1436,17 +1442,31 @@ 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);
+	}
 
 	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);
+		phy_trigger_machine(dev->phydev);
+	} else if (!wake) {
+		phy_start(dev->phydev);
+	}
 
 	netif_device_attach(dev);
 
-- 
2.34.1

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

[Andrew Lunn]

On Tue, Apr 28, 2026 at 03:08:58PM -0700, Justin Chen wrote:
> 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.

So you are just interested in getting networking working faster. This
is independent of WoL? Clearly, if you have WoL enabled you need to
keep the PHY powered, but faster networking should be orthogonal to
WoL.

> -	bcmasp_netif_deinit(dev);
> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>  
> -	if (!intf->wolopts) {
> +	bcmasp_netif_deinit(dev, !wake);

So given your commit message, this i don't understand. For the fast
restarting of networking, it does not matter if WoL is enabled, or if
the PHY is capable of waking the system.

    Andrew

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

[Justin Chen]

On 4/28/26 3:23 PM, Andrew Lunn wrote:
> On Tue, Apr 28, 2026 at 03:08:58PM -0700, Justin Chen wrote:
>> 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.
> 
> So you are just interested in getting networking working faster. This
> is independent of WoL? Clearly, if you have WoL enabled you need to
> keep the PHY powered, but faster networking should be orthogonal to
> WoL.
> 

The thought process was if we are keeping the PHY up for WoL, then 
keeping the link will not cost us anything. But if WoL is disabled, then 
we incur a power cost for keeping the PHY powered and PHY link up. So it 
made sense to only enable this when WoL is enabled. But yes, I should 
explain this in the commit message as to how WoL is related these changes.

Thanks,
Justin

>> -	bcmasp_netif_deinit(dev);
>> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>>   
>> -	if (!intf->wolopts) {
>> +	bcmasp_netif_deinit(dev, !wake);
> 
> So given your commit message, this i don't understand. For the fast
> restarting of networking, it does not matter if WoL is enabled, or if
> the PHY is capable of waking the system.
> 
>      Andrew

Re: [PATCH 0/2] Keep PHY link during WoL sleep cycle

[Florian Fainelli]

On 4/28/2026 3:08 PM, Justin Chen wrote:
> First we divide the init/deinit path to allow for a partial init/deinit
> during a sleep cycle. We also remove some unnecessary small functions at
> the same time.
> 
> Then we modify the suspend and resume path to allow for a partial bring
> down and bring up. This allow us to keep the PHY link up and to resume
> network traffic much quicker.

Since you are going to re-submit this should be prefixed with 
"net-next", thanks!
-- 
Florian

Re: [PATCH net-next 1/2] net: bcmasp: Divide init to allow partial bring up

[Jakub Kicinski]

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: Divide init to allow partial bring up

To prepare for a partial bring up of the interface during resume,
we break apart the bcmasp_netif_init() function into smaller chunks
that can be called as necessary.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index ec63f50a849e..aff0a6d84126 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
[ ... ]
> -static void umac_reset(struct bcmasp_intf *intf)
> +static void umac_reset_and_init(struct bcmasp_intf *intf,
> +				const unsigned char *addr)
>  {
> +	struct phy_device *phydev = intf->ndev->phydev;
> +	u32 mac0, mac1;
>  
>  	umac_wl(intf, 0x0, UMC_CMD);
>  	umac_wl(intf, UMC_CMD_SW_RESET, UMC_CMD);
>  	usleep_range(10, 100);
>  	/* We hold the umac in reset and bring it out of
>  	 * reset when phy link is up.
>  	 */
> -}
>  
> -static void umac_set_hw_addr(struct bcmasp_intf *intf,
> -			     const unsigned char *addr)
> -{
> -	u32 mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
> -		    addr[3];
> -	u32 mac1 = (addr[4] << 8) | addr[5];
> +	umac_wl(intf, 0x800, UMC_FRM_LEN);
> +	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
> +	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
> +
> +	mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
> +		addr[3];
> +	mac1 = (addr[4] << 8) | addr[5];
>  
>  	umac_wl(intf, mac0, UMC_MAC0);
>  	umac_wl(intf, mac1, UMC_MAC1);
> +
> +	/* Reset shadow values since we reset the umac */
> +	intf->old_duplex = -1;
> +	intf->old_link = -1;
> +	intf->old_pause = -1;
> +	phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
>  }

Since UMC_CMD_SW_RESET was just asserted above, won't reading
UMC_EEE_LPI_TIMER here return 0 or a hardware default value, effectively
overwriting the bootloader-configured EEE TX LPI timer?

Also, since umac_reset_and_init() is now called unconditionally during
bcmasp_interface_resume(), does this dangerously overwrite
phydev->eee_cfg.tx_lpi_timer without holding mutex_lock(&phydev->lock)?
This seems like it could create a data race with concurrent accesses, for
example from ethtool.

[ ... ]
> +static void bcmasp_netif_init(struct net_device *dev)
> +{
> +	struct bcmasp_intf *intf = netdev_priv(dev);
>  
>  	bcmasp_init_tx(intf);
>  	netif_napi_add_tx(intf->ndev, &intf->tx_napi, bcmasp_tx_poll);

This existed prior to this patch, but since bcmasp_netif_init() and
bcmasp_netif_deinit() are called directly from bcmasp_interface_resume()
and bcmasp_interface_suspend() (which execute asynchronously without
rtnl_lock), can modifying the global dev->napi_list via netif_napi_add_tx()
and netif_napi_del() cause a race condition with concurrent list readers?

Suspend and resume paths typically rely on napi_enable() and napi_disable()
rather than adding and deleting the NAPI structures entirely.

[ ... ]
> @@ -1454,17 +1436,19 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
>  	if (ret)
>  		return ret;
>  
> -	ret = bcmasp_netif_init(dev, false);
> -	if (ret)
> -		goto out;
> +	bcmasp_core_clock_set_intf(intf, true);
>  
>  	bcmasp_resume_from_wol(intf);
>  
> +	bcmasp_phy_hw_prepare(intf);
> +
> +	umac_reset_and_init(intf, dev->dev_addr);
> +

If Wake-on-LAN is disabled, bcmasp_interface_suspend() un-prepares the PHY
by asserting reset and disabling clocks. By calling bcmasp_resume_from_wol()
before bcmasp_phy_hw_prepare(), if EEE is enabled and invokes eee_fixup(),
won't the MDIO access timeout or cause a bus error because the PHY hardware
isn't prepared yet?

Additionally, won't any WOL state changes (like clearing
UMC_MPD_CTRL_MPD_EN) be immediately wiped out by the MAC software reset
inside the subsequent umac_reset_and_init() call?

I noticed this is fixed later in the series by commit 6994d859a69dd
(net: bcmasp: Keep PHY link during WoL sleep cycle), but does leaving it
like this here break bisection?

> +	bcmasp_netif_init(dev);
> +
> +	phy_start(dev->phydev);
> +
>  	netif_device_attach(dev);
>  
>  	return 0;
> -
> -out:
> -	clk_disable_unprepare(intf->parent->clk);
> -	return ret;
>  }
-- 
pw-bot: cr

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

[Jakub Kicinski]

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);

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

[Justin Chen]

On 4/29/26 4:20 PM, Jakub Kicinski wrote:
> 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?
> 

Just tested this. Added an infinite loop after we call 
clk_disable_unprepare() and don't see the state machine running, so I 
think we are good here.

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

In the case that the MAC hardware is reset, there is no WoL 
configuration to reverse since it was HW reset. Interrupts are also 
reset, so the HW default of everything being masked means this isn't an 
issue.

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

Will fix. I think need to just put the phy state back to PHY_UP state 
and have the state machine take care of relinking whatever the 
configuration is set at.

Justin

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

Re: [PATCH net-next 1/2] net: bcmasp: Divide init to allow partial bring up

[Justin Chen]

On 4/29/26 4:20 PM, Jakub Kicinski wrote:
> 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: Divide init to allow partial bring up
> 
> To prepare for a partial bring up of the interface during resume,
> we break apart the bcmasp_netif_init() function into smaller chunks
> that can be called as necessary.
> 
>> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
>> index ec63f50a849e..aff0a6d84126 100644
>> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
>> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> [ ... ]
>> -static void umac_reset(struct bcmasp_intf *intf)
>> +static void umac_reset_and_init(struct bcmasp_intf *intf,
>> +				const unsigned char *addr)
>>   {
>> +	struct phy_device *phydev = intf->ndev->phydev;
>> +	u32 mac0, mac1;
>>   
>>   	umac_wl(intf, 0x0, UMC_CMD);
>>   	umac_wl(intf, UMC_CMD_SW_RESET, UMC_CMD);
>>   	usleep_range(10, 100);
>>   	/* We hold the umac in reset and bring it out of
>>   	 * reset when phy link is up.
>>   	 */
>> -}
>>   
>> -static void umac_set_hw_addr(struct bcmasp_intf *intf,
>> -			     const unsigned char *addr)
>> -{
>> -	u32 mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
>> -		    addr[3];
>> -	u32 mac1 = (addr[4] << 8) | addr[5];
>> +	umac_wl(intf, 0x800, UMC_FRM_LEN);
>> +	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
>> +	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
>> +
>> +	mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
>> +		addr[3];
>> +	mac1 = (addr[4] << 8) | addr[5];
>>   
>>   	umac_wl(intf, mac0, UMC_MAC0);
>>   	umac_wl(intf, mac1, UMC_MAC1);
>> +
>> +	/* Reset shadow values since we reset the umac */
>> +	intf->old_duplex = -1;
>> +	intf->old_link = -1;
>> +	intf->old_pause = -1;
>> +	phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
>>   }
> 
> Since UMC_CMD_SW_RESET was just asserted above, won't reading
> UMC_EEE_LPI_TIMER here return 0 or a hardware default value, effectively
> overwriting the bootloader-configured EEE TX LPI timer?
> 
> Also, since umac_reset_and_init() is now called unconditionally during
> bcmasp_interface_resume(), does this dangerously overwrite
> phydev->eee_cfg.tx_lpi_timer without holding mutex_lock(&phydev->lock)?
> This seems like it could create a data race with concurrent accesses, for
> example from ethtool.
> 

Hmm yea, let me remove this and put into bcmasp_open. We only want to 
get the timer value on open, not every suspend cycle.


> [ ... ]
>> +static void bcmasp_netif_init(struct net_device *dev)
>> +{
>> +	struct bcmasp_intf *intf = netdev_priv(dev);
>>   
>>   	bcmasp_init_tx(intf);
>>   	netif_napi_add_tx(intf->ndev, &intf->tx_napi, bcmasp_tx_poll);
> 
> This existed prior to this patch, but since bcmasp_netif_init() and
> bcmasp_netif_deinit() are called directly from bcmasp_interface_resume()
> and bcmasp_interface_suspend() (which execute asynchronously without
> rtnl_lock), can modifying the global dev->napi_list via netif_napi_add_tx()
> and netif_napi_del() cause a race condition with concurrent list readers?
> 
> Suspend and resume paths typically rely on napi_enable() and napi_disable()
> rather than adding and deleting the NAPI structures entirely.
> 

Not sure about this, but such a fix shouldn't be part of a restructure 
commit like this one. So I'll investigate further and fix in another 
patch if needed.

> [ ... ]
>> @@ -1454,17 +1436,19 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = bcmasp_netif_init(dev, false);
>> -	if (ret)
>> -		goto out;
>> +	bcmasp_core_clock_set_intf(intf, true);
>>   
>>   	bcmasp_resume_from_wol(intf);
>>   
>> +	bcmasp_phy_hw_prepare(intf);
>> +
>> +	umac_reset_and_init(intf, dev->dev_addr);
>> +
> 
> If Wake-on-LAN is disabled, bcmasp_interface_suspend() un-prepares the PHY
> by asserting reset and disabling clocks. By calling bcmasp_resume_from_wol()
> before bcmasp_phy_hw_prepare(), if EEE is enabled and invokes eee_fixup(),
> won't the MDIO access timeout or cause a bus error because the PHY hardware
> isn't prepared yet?
> 

eee_fixup() doesn't use a MDIO accesses.

> Additionally, won't any WOL state changes (like clearing
> UMC_MPD_CTRL_MPD_EN) be immediately wiped out by the MAC software reset
> inside the subsequent umac_reset_and_init() call?
> 

Yes, but this was the original (incorrect) logic and as mentioned below, 
it is required in the next commit where we do not always reset the UNIMAC.

Justin

> I noticed this is fixed later in the series by commit 6994d859a69dd
> (net: bcmasp: Keep PHY link during WoL sleep cycle), but does leaving it
> like this here break bisection?
> 
>> +	bcmasp_netif_init(dev);
>> +
>> +	phy_start(dev->phydev);
>> +
>>   	netif_device_attach(dev);
>>   
>>   	return 0;
>> -
>> -out:
>> -	clk_disable_unprepare(intf->parent->clk);
>> -	return ret;
>>   }