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

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