Re: [PATCH 1/5] net: eth-uclass: guard against reentrant eth_init()/eth_halt() calls
On Fri, 26 Apr 2024 10:02:24 +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() > > [...] Applied to u-boot/next, thanks! -- Tom
Re: [PATCH 1/5] net: eth-uclass: guard against reentrant eth_init()/eth_halt() calls
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
> ---
> 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/
>
[PATCH 1/5] net: eth-uclass: guard against reentrant eth_init()/eth_halt() calls
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.
Signed-off-by: Matthias Schiffer
---
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/
