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

Jakub Kicinski
2026-04-29T23:20:30Z

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