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

Justin Chen
2026-04-30T23:24:34Z

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