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

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

[Matthias Schiffer]

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 <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/

[PATCH 2/5] net: ti: am65-cpsw-nuss: avoid errors due to imbalanced start()/stop()

[Matthias Schiffer]

The eth-uclass state machine doesn't prevent imbalanced start()/stop()
calls - to the contrary, it even provides eth_init_state_only() and
eth_halt_state_only() functions that change the state without any calls
into the driver. This means that the driver must be robust against
duplicate start() and stop() calls as well as send/recv calls while the
interface is down.

We decide not to print error messages but just to return an error in the
latter case, as trying to send packets on a disabled interface commonly
happens when the netconsole is still active after the Ethernet has been
halted during bootm.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/ti/am65-cpsw-nuss.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index b151e25d6a4..4a57e945a3a 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -328,6 +328,9 @@ static int am65_cpsw_start(struct udevice *dev)
 	struct ti_udma_drv_chan_cfg_data *dma_rx_cfg_data;
 	int ret, i;
 
+	if (common->started)
+		return 0;
+
 	ret = power_domain_on(&common->pwrdmn);
 	if (ret) {
 		dev_err(dev, "power_domain_on() failed %d\n", ret);
@@ -488,6 +491,9 @@ static int am65_cpsw_send(struct udevice *dev, void *packet, int length)
 	struct ti_udma_drv_packet_data packet_data;
 	int ret;
 
+	if (!common->started)
+		return -ENETDOWN;
+
 	packet_data.pkt_type = AM65_CPSW_CPPI_PKT_TYPE;
 	packet_data.dest_tag = priv->port_id;
 	ret = dma_send(&common->dma_tx, packet, length, &packet_data);
@@ -504,6 +510,9 @@ static int am65_cpsw_recv(struct udevice *dev, int flags, uchar **packetp)
 	struct am65_cpsw_priv *priv = dev_get_priv(dev);
 	struct am65_cpsw_common	*common = priv->cpsw_common;
 
+	if (!common->started)
+		return -ENETDOWN;
+
 	/* try to receive a new packet */
 	return dma_receive(&common->dma_rx, (void **)packetp, NULL);
 }
-- 
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 3/5] net: ti: am65-cpsw-nuss: fix error handling for "RX dma add buf failed"

[Matthias Schiffer]

The RX DMA channel has been requested at this point already, so it must
be freed.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/ti/am65-cpsw-nuss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 4a57e945a3a..65ade1afd05 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -362,7 +362,7 @@ static int am65_cpsw_start(struct udevice *dev)
 					  UDMA_RX_BUF_SIZE);
 		if (ret) {
 			dev_err(dev, "RX dma add buf failed %d\n", ret);
-			goto err_free_tx;
+			goto err_free_rx;
 		}
 	}
 
-- 
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 4/5] dma: ti: k3-udma: add missing <net.h> include

[Matthias Schiffer]

net.h is needed for PKTBUFSRX. Without this definition, the driver will
always use 4 RX buffers, causing am65-cpsw-nuss initialization to fail
when a higher number of buffers is requested.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/dma/ti/k3-udma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index ef3074aa13f..4e6f7f570c5 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -12,6 +12,7 @@
 #include <asm/io.h>
 #include <asm/bitops.h>
 #include <malloc.h>
+#include <net.h>
 #include <linux/bitops.h>
 #include <linux/dma-mapping.h>
 #include <linux/sizes.h>
-- 
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 5/5] dma: ti: k3-udma: invalidate prepared buffers before pushing to recv ring

[Matthias Schiffer]

Buffers must not have an unclean cache before being used for DMA - a
pending write-back may corrupt the next dev-to-mem transfer otherwise.

This was consistently noticeable during long TFTP transfers, when an ARP
request is answered by U-Boot in the middle of the transfer:

As U-Boot's arp_receive() reuses the receive buffer to prepare its
reply packet, the beginning of one of the next incoming TFTP packets
is overwritten by the ARP reply. The corrupted packet is ignored, but
the TFTP transfer stalls for a few seconds until a timeout is detected
and a retransmit is triggered.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/dma/ti/k3-udma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
index 4e6f7f570c5..8f6d396653e 100644
--- a/drivers/dma/ti/k3-udma.c
+++ b/drivers/dma/ti/k3-udma.c
@@ -2678,6 +2678,9 @@ int udma_prepare_rcv_buf(struct dma *dma, void *dst, size_t size)
 	cppi5_hdesc_set_pktlen(desc_rx, size);
 	cppi5_hdesc_attach_buf(desc_rx, dma_dst, size, dma_dst, size);
 
+	invalidate_dcache_range((unsigned long)dma_dst,
+				(unsigned long)(dma_dst + size));
+
 	flush_dcache_range((unsigned long)desc_rx,
 			   ALIGN((unsigned long)desc_rx + uc->config.hdesc_size,
 				 ARCH_DMA_MINALIGN));
-- 
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/

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

[Siddharth Vadapalli]

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/
> 

Re: [PATCH 5/5] dma: ti: k3-udma: invalidate prepared buffers before pushing to recv ring

[Sverdlin, Alexander]

Hello Matthias,

On Fri, 2024-04-26 at 10:02 +0200, Matthias Schiffer wrote:
> Buffers must not have an unclean cache before being used for DMA - a
> pending write-back may corrupt the next dev-to-mem transfer otherwise.
> 
> This was consistently noticeable during long TFTP transfers, when an ARP
> request is answered by U-Boot in the middle of the transfer:
> 
> As U-Boot's arp_receive() reuses the receive buffer to prepare its
> reply packet, the beginning of one of the next incoming TFTP packets
> is overwritten by the ARP reply. The corrupted packet is ignored, but
> the TFTP transfer stalls for a few seconds until a timeout is detected
> and a retransmit is triggered.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

thanks for the series! I've tested it on a custom AM625-based board
issuing a a couple of pings:

am65_cpsw_nuss ethernet@8000000: K3 CPSW: nuss_ver: 0x6BA01103 cpsw_ver: 0x6BA81103 ale_ver: 0x00290105 Ports:2
Net:   eth0: ethernet@8000000port@1
Autobooting in 3 seconds, press "<Esc><Esc>" to stop
U-Boot# ping 192.168.251.10
ti-udma dma-controller@485c0000: k3_dmaring Ring probed rings:150, sci-dev-id:30
ti-udma dma-controller@485c0000: dma-ring-reset-quirk: disabled
am65_cpsw_nuss_port ethernet@8000000port@1: K3 CPSW: rflow_id_base: 19
link up on port 1, speed 100, full duplex
Using ethernet@8000000port@1 device
host 192.168.251.10 is alive
U-Boot# ping 192.168.251.10
am65_cpsw_nuss_port ethernet@8000000port@1: K3 CPSW: rflow_id_base: 19
link up on port 1, speed 100, full duplex
Using ethernet@8000000port@1 device
host 192.168.251.10 is alive

This sequence looks exactly the same with your patches and without.
Therefore for the whole series:

Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

> ---
>  drivers/dma/ti/k3-udma.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 4e6f7f570c5..8f6d396653e 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -2678,6 +2678,9 @@ int udma_prepare_rcv_buf(struct dma *dma, void *dst, size_t size)
>         cppi5_hdesc_set_pktlen(desc_rx, size);
>         cppi5_hdesc_attach_buf(desc_rx, dma_dst, size, dma_dst, size);
>  
> +       invalidate_dcache_range((unsigned long)dma_dst,
> +                               (unsigned long)(dma_dst + size));
> +
>         flush_dcache_range((unsigned long)desc_rx,
>                            ALIGN((unsigned long)desc_rx + uc->config.hdesc_size,
>                                  ARCH_DMA_MINALIGN));

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

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

[Tom Rini]

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