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

[Jakub Kicinski <kuba@kernel.org>]

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