Re: [PATCH 1/5] net: eth-uclass: guard against reentrant eth_init()/eth_halt() calls

[Siddharth Vadapalli <s-vadapalli@ti.com>]

On Fri, Apr 26, 2024 at 10:02:24AM +0200, Matthias Schiffer wrote:
> With netconsole, any log message can result in an eth_init(), possibly
> causing an reentrant call into eth_init() if a driver's ops print
> anything:
> 
>     eth_init() -> driver.start() -> printf() -> netconsole -> eth_init()
>     eth_halt() -> driver.stop() -> printf() -> netconsole -> eth_init()
> 
> Rather than expecting every single Ethernet driver to handle this case,
> prevent the reentrant calls in eth_init() and eth_halt().
> 
> The issue was noticed on an AM62x board, where a bootm after
> simultaneous netconsole and TFTP would result in a crash.

Link: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1350550/sk-am62a-lp-am65_cpsw_nuss_port-error-in-u-boot-while-using-netconsole-functionality/

> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  net/eth-uclass.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3d0ec91dfa4..193218a328b 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -48,6 +48,8 @@ struct eth_uclass_priv {
>  
>  /* eth_errno - This stores the most recent failure code from DM functions */
>  static int eth_errno;
> +/* Are we currently in eth_init() or eth_halt()? */
> +static bool in_init_halt;
>  
>  /* board-specific Ethernet Interface initializations. */
>  __weak int board_interface_eth_init(struct udevice *dev,
> @@ -285,11 +287,19 @@ U_BOOT_ENV_CALLBACK(ethaddr, on_ethaddr);
>  
>  int eth_init(void)
>  {
> -	char *ethact = env_get("ethact");
> -	char *ethrotate = env_get("ethrotate");
>  	struct udevice *current = NULL;
>  	struct udevice *old_current;
>  	int ret = -ENODEV;
> +	char *ethrotate;
> +	char *ethact;
> +
> +	if (in_init_halt)
> +		return -EBUSY;
> +
> +	in_init_halt = true;
> +
> +	ethact = env_get("ethact");
> +	ethrotate = env_get("ethrotate");
>  
>  	/*
>  	 * When 'ethrotate' variable is set to 'no' and 'ethact' variable
> @@ -298,8 +308,10 @@ int eth_init(void)
>  	if ((ethrotate != NULL) && (strcmp(ethrotate, "no") == 0)) {
>  		if (ethact) {
>  			current = eth_get_dev_by_name(ethact);
> -			if (!current)
> -				return -EINVAL;
> +			if (!current) {
> +				ret = -EINVAL;
> +				goto end;
> +			}
>  		}
>  	}
>  
> @@ -307,7 +319,8 @@ int eth_init(void)
>  		current = eth_get_dev();
>  		if (!current) {
>  			log_err("No ethernet found.\n");
> -			return -ENODEV;
> +			ret = -ENODEV;
> +			goto end;
>  		}
>  	}
>  
> @@ -324,7 +337,8 @@ int eth_init(void)
>  
>  					priv->state = ETH_STATE_ACTIVE;
>  					priv->running = true;
> -					return 0;
> +					ret = 0;
> +					goto end;
>  				}
>  			} else {
>  				ret = eth_errno;
> @@ -344,6 +358,8 @@ int eth_init(void)
>  		current = eth_get_dev();
>  	} while (old_current != current);
>  
> +end:
> +	in_init_halt = false;
>  	return ret;
>  }
>  
> @@ -352,17 +368,25 @@ void eth_halt(void)
>  	struct udevice *current;
>  	struct eth_device_priv *priv;
>  
> +	if (in_init_halt)
> +		return;
> +
> +	in_init_halt = true;
> +
>  	current = eth_get_dev();
>  	if (!current)
> -		return;
> +		goto end;
>  
>  	priv = dev_get_uclass_priv(current);
>  	if (!priv || !priv->running)
> -		return;
> +		goto end;
>  
>  	eth_get_ops(current)->stop(current);
>  	priv->state = ETH_STATE_PASSIVE;
>  	priv->running = false;
> +
> +end:
> +	in_init_halt = false;
>  }
>  
>  int eth_is_active(struct udevice *dev)
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
>