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

[Justin Chen <justin.chen@broadcom.com>]

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