[U-Boot] [PATCH 00/10] net: Fix packet corruption issue when handling asynch replies

[U-Boot] [PATCH 00/10] net: Fix packet corruption issue when handling asynch replies

[Joe Hershberger]

The issue [1] was reported by (Peter) Tran Tien Dat. Unfortunately his
fix for the issue broke notmal operation and I don't feel is a good way
to address the issue. Also, the situation was not covered in the unit
tests, so we'll add them now.

First we refactor the unit test capability of the sandbox ethernet fake
driver so that we can exercise that part of the network stack, then
add the tests where we prove that the async replies work, but that in
the process, the action expected by the user (ping in this case) is
broken.

Lastly, we correct the problem and change the unit tests to also expect
success of the user's operation.

[1] https://patchwork.ozlabs.org/patch/939617/


Joe Hershberger (10):
  net: sandbox: Move disabled flag into priv struct
  net: sandbox: Refactor sandbox send function
  net: sandbox: Make the fake eth driver response configurable
  net: sandbox: Share the priv structure with tests
  net: sandbox: Allow fake eth to handle more than 1 packet response
  net: Add an accessor to know if waiting for ARP
  net: sandbox: Add a priv ptr for tests to use
  test: eth: Add a test for ARP requests
  test: eth: Add a test for the target being pinged
  net: Don't overwrite waiting packets with asynchronous replies

 arch/sandbox/include/asm/eth.h |  85 +++++++++
 drivers/net/sandbox.c          | 415 +++++++++++++++++++++++++++++++----------
 include/net.h                  |   9 +
 net/arp.c                      |  20 +-
 net/arp.h                      |   1 +
 net/net.c                      |   8 +
 net/ping.c                     |   7 +-
 test/dm/eth.c                  | 160 ++++++++++++++++
 8 files changed, 596 insertions(+), 109 deletions(-)

-- 
2.11.0

[U-Boot] [PATCH 01/10] net: sandbox: Move disabled flag into priv struct

[Joe Hershberger]

Store the per-device data with the device.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 drivers/net/sandbox.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index b71c8f88d9..60fe065ee5 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -19,17 +19,18 @@ DECLARE_GLOBAL_DATA_PTR;
  *
  * fake_host_hwaddr: MAC address of mocked machine
  * fake_host_ipaddr: IP address of mocked machine
+ * disabled: Will not respond
  * recv_packet_buffer: buffer of the packet returned as received
  * recv_packet_length: length of the packet returned as received
  */
 struct eth_sandbox_priv {
 	uchar fake_host_hwaddr[ARP_HLEN];
 	struct in_addr fake_host_ipaddr;
+	bool disabled;
 	uchar *recv_packet_buffer;
 	int recv_packet_length;
 };
 
-static bool disabled[8] = {false};
 static bool skip_timeout;
 
 /*
@@ -40,7 +41,16 @@ static bool skip_timeout;
  */
 void sandbox_eth_disable_response(int index, bool disable)
 {
-	disabled[index] = disable;
+	struct udevice *dev;
+	struct eth_sandbox_priv *priv;
+	int ret;
+
+	ret = uclass_get_device(UCLASS_ETH, index, &dev);
+	if (ret)
+		return;
+
+	priv = dev_get_priv(dev);
+	priv->disabled = disable;
 }
 
 /*
@@ -71,8 +81,7 @@ static int sb_eth_send(struct udevice *dev, void *packet, int length)
 
 	debug("eth_sandbox: Send packet %d\n", length);
 
-	if (dev->seq >= 0 && dev->seq < ARRAY_SIZE(disabled) &&
-	    disabled[dev->seq])
+	if (priv->disabled)
 		return 0;
 
 	if (ntohs(eth->et_protlen) == PROT_ARP) {
@@ -212,6 +221,7 @@ static int sb_eth_ofdata_to_platdata(struct udevice *dev)
 		return -EINVAL;
 	}
 	memcpy(priv->fake_host_hwaddr, mac, ARP_HLEN);
+	priv->disabled = false;
 
 	return 0;
 }
-- 
2.11.0

[U-Boot] [PATCH 02/10] net: sandbox: Refactor sandbox send function

[Joe Hershberger]

Make the behavior of the send function reusable.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h |  20 +++++
 drivers/net/sandbox.c          | 176 ++++++++++++++++++++++++-----------------
 2 files changed, 124 insertions(+), 72 deletions(-)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index bfcd11b593..00062616a4 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -13,4 +13,24 @@ void sandbox_eth_disable_response(int index, bool disable);
 
 void sandbox_eth_skip_timeout(void);
 
+/*
+ * sandbox_eth_arp_req_to_reply()
+ *
+ * Check for an arp request to be sent. If so, inject a reply
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
+				 unsigned int len);
+
+/*
+ * sandbox_eth_ping_req_to_reply()
+ *
+ * Check for a ping request to be sent. If so, inject a reply
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
+				  unsigned int len);
+
 #endif /* __ETH_H */
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 60fe065ee5..5746af11a6 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -63,6 +63,108 @@ void sandbox_eth_skip_timeout(void)
 	skip_timeout = true;
 }
 
+/*
+ * sandbox_eth_arp_req_to_reply()
+ *
+ * Check for an arp request to be sent. If so, inject a reply
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
+				 unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct arp_hdr *arp;
+	struct ethernet_hdr *eth_recv;
+	struct arp_hdr *arp_recv;
+
+	if (ntohs(eth->et_protlen) != PROT_ARP)
+		return 0;
+
+	arp = packet + ETHER_HDR_SIZE;
+
+	if (ntohs(arp->ar_op) != ARPOP_REQUEST)
+		return 0;
+
+	/* store this as the assumed IP of the fake host */
+	priv->fake_host_ipaddr = net_read_ip(&arp->ar_tpa);
+
+	/* Formulate a fake response */
+	eth_recv = (void *)priv->recv_packet_buffer;
+	memcpy(eth_recv->et_dest, eth->et_src, ARP_HLEN);
+	memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
+	eth_recv->et_protlen = htons(PROT_ARP);
+
+	arp_recv = (void *)eth_recv + ETHER_HDR_SIZE;
+	arp_recv->ar_hrd = htons(ARP_ETHER);
+	arp_recv->ar_pro = htons(PROT_IP);
+	arp_recv->ar_hln = ARP_HLEN;
+	arp_recv->ar_pln = ARP_PLEN;
+	arp_recv->ar_op = htons(ARPOP_REPLY);
+	memcpy(&arp_recv->ar_sha, priv->fake_host_hwaddr, ARP_HLEN);
+	net_write_ip(&arp_recv->ar_spa, priv->fake_host_ipaddr);
+	memcpy(&arp_recv->ar_tha, &arp->ar_sha, ARP_HLEN);
+	net_copy_ip(&arp_recv->ar_tpa, &arp->ar_spa);
+
+	priv->recv_packet_length = ETHER_HDR_SIZE + ARP_HDR_SIZE;
+
+	return 1;
+}
+
+/*
+ * sandbox_eth_ping_req_to_reply()
+ *
+ * Check for a ping request to be sent. If so, inject a reply
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
+				  unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct ip_udp_hdr *ip;
+	struct icmp_hdr *icmp;
+	struct ethernet_hdr *eth_recv;
+	struct ip_udp_hdr *ipr;
+	struct icmp_hdr *icmpr;
+
+	if (ntohs(eth->et_protlen) != PROT_IP)
+		return 0;
+
+	ip = packet + ETHER_HDR_SIZE;
+
+	if (ip->ip_p != IPPROTO_ICMP)
+		return 0;
+
+	icmp = (struct icmp_hdr *)&ip->udp_src;
+
+	if (icmp->type != ICMP_ECHO_REQUEST)
+		return 0;
+
+	/* reply to the ping */
+	eth_recv = (void *)priv->recv_packet_buffer;
+	memcpy(eth_recv, packet, len);
+	ipr = (void *)eth_recv + ETHER_HDR_SIZE;
+	icmpr = (struct icmp_hdr *)&ipr->udp_src;
+	memcpy(eth_recv->et_dest, eth->et_src, ARP_HLEN);
+	memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
+	ipr->ip_sum = 0;
+	ipr->ip_off = 0;
+	net_copy_ip((void *)&ipr->ip_dst, &ip->ip_src);
+	net_write_ip((void *)&ipr->ip_src, priv->fake_host_ipaddr);
+	ipr->ip_sum = compute_ip_checksum(ipr, IP_HDR_SIZE);
+
+	icmpr->type = ICMP_ECHO_REPLY;
+	icmpr->checksum = 0;
+	icmpr->checksum = compute_ip_checksum(icmpr, ICMP_HDR_SIZE);
+
+	priv->recv_packet_length = len;
+
+	return 1;
+}
+
 static int sb_eth_start(struct udevice *dev)
 {
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
@@ -77,84 +179,14 @@ static int sb_eth_start(struct udevice *dev)
 static int sb_eth_send(struct udevice *dev, void *packet, int length)
 {
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
-	struct ethernet_hdr *eth = packet;
 
 	debug("eth_sandbox: Send packet %d\n", length);
 
 	if (priv->disabled)
 		return 0;
 
-	if (ntohs(eth->et_protlen) == PROT_ARP) {
-		struct arp_hdr *arp = packet + ETHER_HDR_SIZE;
-
-		if (ntohs(arp->ar_op) == ARPOP_REQUEST) {
-			struct ethernet_hdr *eth_recv;
-			struct arp_hdr *arp_recv;
-
-			/* store this as the assumed IP of the fake host */
-			priv->fake_host_ipaddr = net_read_ip(&arp->ar_tpa);
-			/* Formulate a fake response */
-			eth_recv = (void *)priv->recv_packet_buffer;
-			memcpy(eth_recv->et_dest, eth->et_src, ARP_HLEN);
-			memcpy(eth_recv->et_src, priv->fake_host_hwaddr,
-			       ARP_HLEN);
-			eth_recv->et_protlen = htons(PROT_ARP);
-
-			arp_recv = (void *)priv->recv_packet_buffer +
-				ETHER_HDR_SIZE;
-			arp_recv->ar_hrd = htons(ARP_ETHER);
-			arp_recv->ar_pro = htons(PROT_IP);
-			arp_recv->ar_hln = ARP_HLEN;
-			arp_recv->ar_pln = ARP_PLEN;
-			arp_recv->ar_op = htons(ARPOP_REPLY);
-			memcpy(&arp_recv->ar_sha, priv->fake_host_hwaddr,
-			       ARP_HLEN);
-			net_write_ip(&arp_recv->ar_spa, priv->fake_host_ipaddr);
-			memcpy(&arp_recv->ar_tha, &arp->ar_sha, ARP_HLEN);
-			net_copy_ip(&arp_recv->ar_tpa, &arp->ar_spa);
-
-			priv->recv_packet_length = ETHER_HDR_SIZE +
-				ARP_HDR_SIZE;
-		}
-	} else if (ntohs(eth->et_protlen) == PROT_IP) {
-		struct ip_udp_hdr *ip = packet + ETHER_HDR_SIZE;
-
-		if (ip->ip_p == IPPROTO_ICMP) {
-			struct icmp_hdr *icmp = (struct icmp_hdr *)&ip->udp_src;
-
-			if (icmp->type == ICMP_ECHO_REQUEST) {
-				struct ethernet_hdr *eth_recv;
-				struct ip_udp_hdr *ipr;
-				struct icmp_hdr *icmpr;
-
-				/* reply to the ping */
-				memcpy(priv->recv_packet_buffer, packet,
-				       length);
-				eth_recv = (void *)priv->recv_packet_buffer;
-				ipr = (void *)priv->recv_packet_buffer +
-					ETHER_HDR_SIZE;
-				icmpr = (struct icmp_hdr *)&ipr->udp_src;
-				memcpy(eth_recv->et_dest, eth->et_src,
-				       ARP_HLEN);
-				memcpy(eth_recv->et_src, priv->fake_host_hwaddr,
-				       ARP_HLEN);
-				ipr->ip_sum = 0;
-				ipr->ip_off = 0;
-				net_copy_ip((void *)&ipr->ip_dst, &ip->ip_src);
-				net_write_ip((void *)&ipr->ip_src,
-					     priv->fake_host_ipaddr);
-				ipr->ip_sum = compute_ip_checksum(ipr,
-					IP_HDR_SIZE);
-
-				icmpr->type = ICMP_ECHO_REPLY;
-				icmpr->checksum = 0;
-				icmpr->checksum = compute_ip_checksum(icmpr,
-					ICMP_HDR_SIZE);
-
-				priv->recv_packet_length = length;
-			}
-		}
-	}
+	sandbox_eth_arp_req_to_reply(dev, packet, length);
+	sandbox_eth_ping_req_to_reply(dev, packet, length);
 
 	return 0;
 }
-- 
2.11.0

[U-Boot] [PATCH 03/10] net: sandbox: Make the fake eth driver response configurable

[Joe Hershberger]

Make the send handler registerable so tests can check for different
things.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h | 17 ++++++++++++++
 drivers/net/sandbox.c          | 53 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 00062616a4..04ce266e2a 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -33,4 +33,21 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
 int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 				  unsigned int len);
 
+/**
+ * A packet handler
+ *
+ * dev - device pointer
+ * pkt - pointer to the "sent" packet
+ * len - packet length
+ */
+typedef int sandbox_eth_tx_hand_f(struct udevice *dev, void *pkt,
+				   unsigned int len);
+
+/*
+ * Set packet handler
+ *
+ * handler - The func ptr to call on send. If NULL, set to default handler
+ */
+void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler);
+
 #endif /* __ETH_H */
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 5746af11a6..2eb1b418f5 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -10,6 +10,7 @@
 #include <dm.h>
 #include <malloc.h>
 #include <net.h>
+#include <asm/eth.h>
 #include <asm/test.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -22,6 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
  * disabled: Will not respond
  * recv_packet_buffer: buffer of the packet returned as received
  * recv_packet_length: length of the packet returned as received
+ * tx_handler - function to generate responses to sent packets
  */
 struct eth_sandbox_priv {
 	uchar fake_host_hwaddr[ARP_HLEN];
@@ -29,6 +31,7 @@ struct eth_sandbox_priv {
 	bool disabled;
 	uchar *recv_packet_buffer;
 	int recv_packet_length;
+	sandbox_eth_tx_hand_f *tx_handler;
 };
 
 static bool skip_timeout;
@@ -165,6 +168,50 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 	return 1;
 }
 
+/*
+ * sb_default_handler()
+ *
+ * perform typical responses to simple ping
+ *
+ * dev - device pointer
+ * pkt - "sent" packet buffer
+ * len - length of packet
+ */
+static int sb_default_handler(struct udevice *dev, void *packet,
+			      unsigned int len)
+{
+	sandbox_eth_arp_req_to_reply(dev, packet, len);
+	sandbox_eth_ping_req_to_reply(dev, packet, len);
+
+	return 0;
+}
+
+/*
+ * sandbox_eth_set_tx_handler()
+ *
+ * Set a custom response to a packet being sent through the sandbox eth test
+ *	driver
+ *
+ * index - interface to set the handler for
+ * handler - The func ptr to call on send. If NULL, set to default handler
+ */
+void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler)
+{
+	struct udevice *dev;
+	struct eth_sandbox_priv *priv;
+	int ret;
+
+	ret = uclass_get_device(UCLASS_ETH, index, &dev);
+	if (ret)
+		return;
+
+	priv = dev_get_priv(dev);
+	if (handler)
+		priv->tx_handler = handler;
+	else
+		priv->tx_handler = sb_default_handler;
+}
+
 static int sb_eth_start(struct udevice *dev)
 {
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
@@ -185,10 +232,7 @@ static int sb_eth_send(struct udevice *dev, void *packet, int length)
 	if (priv->disabled)
 		return 0;
 
-	sandbox_eth_arp_req_to_reply(dev, packet, length);
-	sandbox_eth_ping_req_to_reply(dev, packet, length);
-
-	return 0;
+	return priv->tx_handler(dev, packet, length);
 }
 
 static int sb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
@@ -254,6 +298,7 @@ static int sb_eth_ofdata_to_platdata(struct udevice *dev)
 	}
 	memcpy(priv->fake_host_hwaddr, mac, ARP_HLEN);
 	priv->disabled = false;
+	priv->tx_handler = sb_default_handler;
 
 	return 0;
 }
-- 
2.11.0

[U-Boot] [PATCH 04/10] net: sandbox: Share the priv structure with tests

[Joe Hershberger]

If tests want to implement tx handlers, they will likely need access to
the details in the priv structure.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h | 19 +++++++++++++++++++
 drivers/net/sandbox.c          | 19 -------------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 04ce266e2a..03c3fb2a28 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -43,6 +43,25 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 typedef int sandbox_eth_tx_hand_f(struct udevice *dev, void *pkt,
 				   unsigned int len);
 
+/**
+ * struct eth_sandbox_priv - memory for sandbox mock driver
+ *
+ * fake_host_hwaddr - MAC address of mocked machine
+ * fake_host_ipaddr - IP address of mocked machine
+ * disabled - Will not respond
+ * recv_packet_buffer - buffer of the packet returned as received
+ * recv_packet_length - length of the packet returned as received
+ * tx_handler - function to generate responses to sent packets
+ */
+struct eth_sandbox_priv {
+	uchar fake_host_hwaddr[ARP_HLEN];
+	struct in_addr fake_host_ipaddr;
+	bool disabled;
+	uchar *recv_packet_buffer;
+	int recv_packet_length;
+	sandbox_eth_tx_hand_f *tx_handler;
+};
+
 /*
  * Set packet handler
  *
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 2eb1b418f5..0af7bc1439 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -15,25 +15,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/**
- * struct eth_sandbox_priv - memory for sandbox mock driver
- *
- * fake_host_hwaddr: MAC address of mocked machine
- * fake_host_ipaddr: IP address of mocked machine
- * disabled: Will not respond
- * recv_packet_buffer: buffer of the packet returned as received
- * recv_packet_length: length of the packet returned as received
- * tx_handler - function to generate responses to sent packets
- */
-struct eth_sandbox_priv {
-	uchar fake_host_hwaddr[ARP_HLEN];
-	struct in_addr fake_host_ipaddr;
-	bool disabled;
-	uchar *recv_packet_buffer;
-	int recv_packet_length;
-	sandbox_eth_tx_hand_f *tx_handler;
-};
-
 static bool skip_timeout;
 
 /*
-- 
2.11.0

[U-Boot] [PATCH 05/10] net: sandbox: Allow fake eth to handle more than 1 packet response

[Joe Hershberger]

Use up to the max allocated receive buffers so be able to test more
complex situations.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h | 10 +++++---
 drivers/net/sandbox.c          | 57 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 03c3fb2a28..6dabbf00ab 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -49,16 +49,18 @@ typedef int sandbox_eth_tx_hand_f(struct udevice *dev, void *pkt,
  * fake_host_hwaddr - MAC address of mocked machine
  * fake_host_ipaddr - IP address of mocked machine
  * disabled - Will not respond
- * recv_packet_buffer - buffer of the packet returned as received
- * recv_packet_length - length of the packet returned as received
+ * recv_packet_buffer - buffers of the packet returned as received
+ * recv_packet_length - lengths of the packet returned as received
+ * recv_packets - number of packets returned
  * tx_handler - function to generate responses to sent packets
  */
 struct eth_sandbox_priv {
 	uchar fake_host_hwaddr[ARP_HLEN];
 	struct in_addr fake_host_ipaddr;
 	bool disabled;
-	uchar *recv_packet_buffer;
-	int recv_packet_length;
+	uchar * recv_packet_buffer[PKTBUFSRX];
+	int recv_packet_length[PKTBUFSRX];
+	int recv_packets;
 	sandbox_eth_tx_hand_f *tx_handler;
 };
 
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 0af7bc1439..5117af8a82 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -71,11 +71,15 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
 	if (ntohs(arp->ar_op) != ARPOP_REQUEST)
 		return 0;
 
+	/* Don't allow the buffer to overrun */
+	if (priv->recv_packets >= PKTBUFSRX)
+		return 0;
+
 	/* store this as the assumed IP of the fake host */
 	priv->fake_host_ipaddr = net_read_ip(&arp->ar_tpa);
 
 	/* Formulate a fake response */
-	eth_recv = (void *)priv->recv_packet_buffer;
+	eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
 	memcpy(eth_recv->et_dest, eth->et_src, ARP_HLEN);
 	memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
 	eth_recv->et_protlen = htons(PROT_ARP);
@@ -91,7 +95,9 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
 	memcpy(&arp_recv->ar_tha, &arp->ar_sha, ARP_HLEN);
 	net_copy_ip(&arp_recv->ar_tpa, &arp->ar_spa);
 
-	priv->recv_packet_length = ETHER_HDR_SIZE + ARP_HDR_SIZE;
+	priv->recv_packet_length[priv->recv_packets] =
+		ETHER_HDR_SIZE + ARP_HDR_SIZE;
+	++priv->recv_packets;
 
 	return 1;
 }
@@ -127,8 +133,12 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 	if (icmp->type != ICMP_ECHO_REQUEST)
 		return 0;
 
+	/* Don't allow the buffer to overrun */
+	if (priv->recv_packets >= PKTBUFSRX)
+		return 0;
+
 	/* reply to the ping */
-	eth_recv = (void *)priv->recv_packet_buffer;
+	eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
 	memcpy(eth_recv, packet, len);
 	ipr = (void *)eth_recv + ETHER_HDR_SIZE;
 	icmpr = (struct icmp_hdr *)&ipr->udp_src;
@@ -144,7 +154,8 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 	icmpr->checksum = 0;
 	icmpr->checksum = compute_ip_checksum(icmpr, ICMP_HDR_SIZE);
 
-	priv->recv_packet_length = len;
+	priv->recv_packet_length[priv->recv_packets] = len;
+	++priv->recv_packets;
 
 	return 1;
 }
@@ -199,7 +210,11 @@ static int sb_eth_start(struct udevice *dev)
 
 	debug("eth_sandbox: Start\n");
 
-	priv->recv_packet_buffer = net_rx_packets[0];
+	priv->recv_packets = 0;
+	for (int i = 0; i < PKTBUFSRX; i++) {
+		priv->recv_packet_buffer[i] = net_rx_packets[i];
+		priv->recv_packet_length[i] = 0;
+	}
 
 	return 0;
 }
@@ -225,18 +240,37 @@ static int sb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
 		skip_timeout = false;
 	}
 
-	if (priv->recv_packet_length) {
-		int lcl_recv_packet_length = priv->recv_packet_length;
+	if (priv->recv_packets) {
+		int lcl_recv_packet_length = priv->recv_packet_length[0];
 
-		debug("eth_sandbox: received packet %d\n",
-		      priv->recv_packet_length);
-		priv->recv_packet_length = 0;
-		*packetp = priv->recv_packet_buffer;
+		debug("eth_sandbox: received packet[%d], %d waiting\n",
+		      lcl_recv_packet_length, priv->recv_packets - 1);
+		*packetp = priv->recv_packet_buffer[0];
 		return lcl_recv_packet_length;
 	}
 	return 0;
 }
 
+static int sb_eth_free_pkt(struct udevice *dev, uchar *packet, int length)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	int i;
+
+	if (!priv->recv_packets)
+		return 0;
+
+	--priv->recv_packets;
+	for (i = 0; i < priv->recv_packets; i++) {
+		priv->recv_packet_length[i] = priv->recv_packet_length[i + 1];
+		memcpy(priv->recv_packet_buffer[i],
+		       priv->recv_packet_buffer[i + 1],
+		       priv->recv_packet_length[i + 1]);
+	}
+	priv->recv_packet_length[priv->recv_packets] = 0;
+
+	return 0;
+}
+
 static void sb_eth_stop(struct udevice *dev)
 {
 	debug("eth_sandbox: Stop\n");
@@ -255,6 +289,7 @@ static const struct eth_ops sb_eth_ops = {
 	.start			= sb_eth_start,
 	.send			= sb_eth_send,
 	.recv			= sb_eth_recv,
+	.free_pkt		= sb_eth_free_pkt,
 	.stop			= sb_eth_stop,
 	.write_hwaddr		= sb_eth_write_hwaddr,
 };
-- 
2.11.0

[U-Boot] [PATCH 06/10] net: Add an accessor to know if waiting for ARP

[Joe Hershberger]

This single-sources the state of the ARP.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 include/net.h |  1 +
 net/arp.c     | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/net.h b/include/net.h
index f9984ae86c..63718a47f2 100644
--- a/include/net.h
+++ b/include/net.h
@@ -635,6 +635,7 @@ rxhand_f *net_get_udp_handler(void);	/* Get UDP RX packet handler */
 void net_set_udp_handler(rxhand_f *);	/* Set UDP RX packet handler */
 rxhand_f *net_get_arp_handler(void);	/* Get ARP RX packet handler */
 void net_set_arp_handler(rxhand_f *);	/* Set ARP RX packet handler */
+int arp_is_waiting(void);		/* Waiting for ARP reply? */
 void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */
 void net_set_timeout_handler(ulong, thand_f *);/* Set timeout handler */
 
diff --git a/net/arp.c b/net/arp.c
index b8a71684cd..524361cf1b 100644
--- a/net/arp.c
+++ b/net/arp.c
@@ -100,7 +100,7 @@ int arp_timeout_check(void)
 {
 	ulong t;
 
-	if (!net_arp_wait_packet_ip.s_addr)
+	if (!arp_is_waiting())
 		return 0;
 
 	t = get_timer(0);
@@ -187,8 +187,8 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 		return;
 
 	case ARPOP_REPLY:		/* arp reply */
-		/* are we waiting for a reply */
-		if (!net_arp_wait_packet_ip.s_addr)
+		/* are we waiting for a reply? */
+		if (!arp_is_waiting())
 			break;
 
 #ifdef CONFIG_KEEP_SERVERADDR
@@ -233,3 +233,8 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 		return;
 	}
 }
+
+int arp_is_waiting(void)
+{
+	return !!net_arp_wait_packet_ip.s_addr;
+}
-- 
2.11.0

[U-Boot] [PATCH 07/10] net: sandbox: Add a priv ptr for tests to use

[Joe Hershberger]

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h |  9 +++++++++
 drivers/net/sandbox.c          | 20 ++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 6dabbf00ab..6bb3f1bbfd 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -53,6 +53,7 @@ typedef int sandbox_eth_tx_hand_f(struct udevice *dev, void *pkt,
  * recv_packet_length - lengths of the packet returned as received
  * recv_packets - number of packets returned
  * tx_handler - function to generate responses to sent packets
+ * priv - a pointer to some structure a test may want to keep track of
  */
 struct eth_sandbox_priv {
 	uchar fake_host_hwaddr[ARP_HLEN];
@@ -62,6 +63,7 @@ struct eth_sandbox_priv {
 	int recv_packet_length[PKTBUFSRX];
 	int recv_packets;
 	sandbox_eth_tx_hand_f *tx_handler;
+	void *priv;
 };
 
 /*
@@ -71,4 +73,11 @@ struct eth_sandbox_priv {
  */
 void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler);
 
+/*
+ * Set priv ptr
+ *
+ * priv - priv void ptr to store in the device
+ */
+void sandbox_eth_set_priv(int index, void *priv);
+
 #endif /* __ETH_H */
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 5117af8a82..29767ef291 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -204,6 +204,26 @@ void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler)
 		priv->tx_handler = sb_default_handler;
 }
 
+/*
+ * Set priv ptr
+ *
+ * priv - priv void ptr to store in the device
+ */
+void sandbox_eth_set_priv(int index, void *priv)
+{
+	struct udevice *dev;
+	struct eth_sandbox_priv *uc_priv;
+	int ret;
+
+	ret = uclass_get_device(UCLASS_ETH, index, &dev);
+	if (ret)
+		return;
+
+	uc_priv = dev_get_priv(dev);
+
+	uc_priv->priv = priv;
+}
+
 static int sb_eth_start(struct udevice *dev)
 {
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
-- 
2.11.0

[U-Boot] [PATCH 08/10] test: eth: Add a test for ARP requests

[Joe Hershberger]

This tests that ARP requests made to this target's IP address are
responded-to by the target when it is doing other networking operations.

This currently corrupts the ongoing operation of the device if it
happens to be awaiting an ARP reply of its own to whatever serverip it
is attempting to communicate with. In the test, add an expectation that
the user operation (ping, in this case) will fail. A later patch will
address this problem.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h |  9 +++++
 drivers/net/sandbox.c          | 41 +++++++++++++++++++++
 test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 6bb3f1bbfd..7cd5b551d9 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -33,6 +33,15 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
 int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 				  unsigned int len);
 
+/*
+ * sandbox_eth_recv_arp_req()
+ *
+ * Inject an ARP request for this target
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_recv_arp_req(struct udevice *dev);
+
 /**
  * A packet handler
  *
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 29767ef291..2c2a2c6311 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -161,6 +161,47 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
 }
 
 /*
+ * sandbox_eth_recv_arp_req()
+ *
+ * Inject an ARP request for this target
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_recv_arp_req(struct udevice *dev)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth_recv;
+	struct arp_hdr *arp_recv;
+
+	/* Don't allow the buffer to overrun */
+	if (priv->recv_packets >= PKTBUFSRX)
+		return 0;
+
+	/* Formulate a fake request */
+	eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
+	memcpy(eth_recv->et_dest, net_bcast_ethaddr, ARP_HLEN);
+	memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
+	eth_recv->et_protlen = htons(PROT_ARP);
+
+	arp_recv = (void *)eth_recv + ETHER_HDR_SIZE;
+	arp_recv->ar_hrd = htons(ARP_ETHER);
+	arp_recv->ar_pro = htons(PROT_IP);
+	arp_recv->ar_hln = ARP_HLEN;
+	arp_recv->ar_pln = ARP_PLEN;
+	arp_recv->ar_op = htons(ARPOP_REQUEST);
+	memcpy(&arp_recv->ar_sha, priv->fake_host_hwaddr, ARP_HLEN);
+	net_write_ip(&arp_recv->ar_spa, priv->fake_host_ipaddr);
+	memcpy(&arp_recv->ar_tha, net_null_ethaddr, ARP_HLEN);
+	net_write_ip(&arp_recv->ar_tpa, net_ip);
+
+	priv->recv_packet_length[priv->recv_packets] =
+		ETHER_HDR_SIZE + ARP_HDR_SIZE;
+	++priv->recv_packets;
+
+	return 1;
+}
+
+/*
  * sb_default_handler()
  *
  * perform typical responses to simple ping
diff --git a/test/dm/eth.c b/test/dm/eth.c
index 1a7684a887..9b5f53e819 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -258,3 +258,84 @@ static int dm_test_net_retry(struct unit_test_state *uts)
 	return retval;
 }
 DM_TEST(dm_test_net_retry, DM_TESTF_SCAN_FDT);
+
+static int sb_check_arp_reply(struct udevice *dev, void *packet,
+			      unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct arp_hdr *arp;
+	struct unit_test_state *uts = priv->priv;
+
+	if (ntohs(eth->et_protlen) != PROT_ARP)
+		return 0;
+
+	arp = packet + ETHER_HDR_SIZE;
+
+	if (ntohs(arp->ar_op) != ARPOP_REPLY)
+		return 0;
+
+	/* This test would be worthless if we are not waiting */
+	ut_assert(arp_is_waiting());
+
+	/* Validate response */
+	ut_assert(memcmp(eth->et_src, net_ethaddr, ARP_HLEN) == 0);
+	ut_assert(memcmp(eth->et_dest, priv->fake_host_hwaddr, ARP_HLEN) == 0);
+	ut_assert(eth->et_protlen == htons(PROT_ARP));
+
+	ut_assert(arp->ar_hrd == htons(ARP_ETHER));
+	ut_assert(arp->ar_pro == htons(PROT_IP));
+	ut_assert(arp->ar_hln == ARP_HLEN);
+	ut_assert(arp->ar_pln == ARP_PLEN);
+	ut_assert(memcmp(&arp->ar_sha, net_ethaddr, ARP_HLEN) == 0);
+	ut_assert(net_read_ip(&arp->ar_spa).s_addr == net_ip.s_addr);
+	ut_assert(memcmp(&arp->ar_tha, priv->fake_host_hwaddr, ARP_HLEN) == 0);
+	ut_assert(net_read_ip(&arp->ar_tpa).s_addr ==
+		  string_to_ip("1.1.2.4").s_addr);
+
+	return 0;
+}
+
+static int sb_with_async_arp_handler(struct udevice *dev, void *packet,
+				     unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct arp_hdr *arp = packet + ETHER_HDR_SIZE;
+
+	/*
+	 * If we are about to generate a reply to ARP, first inject a request
+	 * from another host
+	 */
+	if (ntohs(eth->et_protlen) == PROT_ARP &&
+	    ntohs(arp->ar_op) == ARPOP_REQUEST) {
+		/* Make sure sandbox_eth_recv_arp_req() knows who is asking */
+		priv->fake_host_ipaddr = string_to_ip("1.1.2.4");
+
+		sandbox_eth_recv_arp_req(dev);
+	}
+
+	sandbox_eth_arp_req_to_reply(dev, packet, len);
+	sandbox_eth_ping_req_to_reply(dev, packet, len);
+
+	return sb_check_arp_reply(dev, packet, len);
+}
+
+static int dm_test_eth_async_arp_reply(struct unit_test_state *uts)
+{
+	net_ping_ip = string_to_ip("1.1.2.2");
+
+	sandbox_eth_set_tx_handler(0, sb_with_async_arp_handler);
+	sandbox_eth_set_priv(0, uts);
+	sandbox_eth_skip_timeout();
+
+	env_set("ethact", "eth at 10002000");
+	ut_assert(net_loop(PING) == -ETIMEDOUT);
+	ut_asserteq_str("eth at 10002000", env_get("ethact"));
+
+	sandbox_eth_set_tx_handler(0, NULL);
+
+	return 0;
+}
+
+DM_TEST(dm_test_eth_async_arp_reply, DM_TESTF_SCAN_FDT);
-- 
2.11.0

[U-Boot] [PATCH 09/10] test: eth: Add a test for the target being pinged

[Joe Hershberger]

The target will respond to pings while doing other network handling.
Make sure that the response happens and is correct.

This currently corrupts the ongoing operation of the device if it
happens to be awaiting an ARP reply of its own to whatever serverip it
is attempting to communicate with. In the test, add an expectation that
the user operation (ping, in this case) will fail. A later patch will
address this problem.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/include/asm/eth.h |  9 +++++
 drivers/net/sandbox.c          | 51 ++++++++++++++++++++++++++
 test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+)

diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
index 7cd5b551d9..66f6b4078b 100644
--- a/arch/sandbox/include/asm/eth.h
+++ b/arch/sandbox/include/asm/eth.h
@@ -42,6 +42,15 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
  */
 int sandbox_eth_recv_arp_req(struct udevice *dev);
 
+/*
+ * sandbox_eth_recv_ping_req()
+ *
+ * Inject a ping request for this target
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_recv_ping_req(struct udevice *dev);
+
 /**
  * A packet handler
  *
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 2c2a2c6311..039c1e3222 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -202,6 +202,57 @@ int sandbox_eth_recv_arp_req(struct udevice *dev)
 }
 
 /*
+ * sandbox_eth_recv_ping_req()
+ *
+ * Inject a ping request for this target
+ *
+ * returns 1 if injected, 0 if not
+ */
+int sandbox_eth_recv_ping_req(struct udevice *dev)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth_recv;
+	struct ip_udp_hdr *ipr;
+	struct icmp_hdr *icmpr;
+
+	/* Don't allow the buffer to overrun */
+	if (priv->recv_packets >= PKTBUFSRX)
+		return 0;
+
+	/* Formulate a fake ping */
+	eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
+
+	memcpy(eth_recv->et_dest, net_ethaddr, ARP_HLEN);
+	memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
+	eth_recv->et_protlen = htons(PROT_IP);
+
+	ipr = (void *)eth_recv + ETHER_HDR_SIZE;
+	ipr->ip_hl_v = 0x45;
+	ipr->ip_len = htons(IP_ICMP_HDR_SIZE);
+	ipr->ip_off = htons(IP_FLAGS_DFRAG);
+	ipr->ip_p = IPPROTO_ICMP;
+	ipr->ip_sum = 0;
+	net_write_ip(&ipr->ip_src, priv->fake_host_ipaddr);
+	net_write_ip(&ipr->ip_dst, net_ip);
+	ipr->ip_sum = compute_ip_checksum(ipr, IP_HDR_SIZE);
+
+	icmpr = (struct icmp_hdr *)&ipr->udp_src;
+
+	icmpr->type = ICMP_ECHO_REQUEST;
+	icmpr->code = 0;
+	icmpr->checksum = 0;
+	icmpr->un.echo.id = 0;
+	icmpr->un.echo.sequence = htons(1);
+	icmpr->checksum = compute_ip_checksum(icmpr, ICMP_HDR_SIZE);
+
+	priv->recv_packet_length[priv->recv_packets] =
+		ETHER_HDR_SIZE + IP_ICMP_HDR_SIZE;
+	++priv->recv_packets;
+
+	return 1;
+}
+
+/*
  * sb_default_handler()
  *
  * perform typical responses to simple ping
diff --git a/test/dm/eth.c b/test/dm/eth.c
index 9b5f53e819..77c602beaf 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -339,3 +339,84 @@ static int dm_test_eth_async_arp_reply(struct unit_test_state *uts)
 }
 
 DM_TEST(dm_test_eth_async_arp_reply, DM_TESTF_SCAN_FDT);
+
+static int sb_check_ping_reply(struct udevice *dev, void *packet,
+			       unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct ip_udp_hdr *ip;
+	struct icmp_hdr *icmp;
+	struct unit_test_state *uts = priv->priv;
+
+	if (ntohs(eth->et_protlen) != PROT_IP)
+		return 0;
+
+	ip = packet + ETHER_HDR_SIZE;
+
+	if (ip->ip_p != IPPROTO_ICMP)
+		return 0;
+
+	icmp = (struct icmp_hdr *)&ip->udp_src;
+
+	if (icmp->type != ICMP_ECHO_REPLY)
+		return 0;
+
+	/* This test would be worthless if we are not waiting */
+	ut_assert(arp_is_waiting());
+
+	/* Validate response */
+	ut_assert(memcmp(eth->et_src, net_ethaddr, ARP_HLEN) == 0);
+	ut_assert(memcmp(eth->et_dest, priv->fake_host_hwaddr, ARP_HLEN) == 0);
+	ut_assert(eth->et_protlen == htons(PROT_IP));
+
+	ut_assert(net_read_ip(&ip->ip_src).s_addr == net_ip.s_addr);
+	ut_assert(net_read_ip(&ip->ip_dst).s_addr ==
+		  string_to_ip("1.1.2.4").s_addr);
+
+	return 0;
+}
+
+static int sb_with_async_ping_handler(struct udevice *dev, void *packet,
+				      unsigned int len)
+{
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	struct ethernet_hdr *eth = packet;
+	struct arp_hdr *arp = packet + ETHER_HDR_SIZE;
+
+	/*
+	 * If we are about to generate a reply to ARP, first inject a request
+	 * from another host
+	 */
+	if (ntohs(eth->et_protlen) == PROT_ARP &&
+	    ntohs(arp->ar_op) == ARPOP_REQUEST) {
+		/* Make sure sandbox_eth_recv_arp_req() knows who is asking */
+		priv->fake_host_ipaddr = string_to_ip("1.1.2.4");
+
+		sandbox_eth_recv_ping_req(dev);
+	}
+
+	sandbox_eth_arp_req_to_reply(dev, packet, len);
+	sandbox_eth_ping_req_to_reply(dev, packet, len);
+
+	return sb_check_ping_reply(dev, packet, len);
+}
+
+static int dm_test_eth_async_ping_reply(struct unit_test_state *uts)
+{
+	net_ping_ip = string_to_ip("1.1.2.2");
+
+	sandbox_eth_set_tx_handler(0, sb_with_async_ping_handler);
+	sandbox_eth_set_priv(0, uts);
+	sandbox_eth_skip_timeout();
+
+	env_set("ethact", "eth at 10002000");
+	ut_assert(net_loop(PING) == -ETIMEDOUT);
+	ut_asserteq_str("eth at 10002000", env_get("ethact"));
+
+	sandbox_eth_set_tx_handler(0, NULL);
+
+	return 0;
+}
+
+DM_TEST(dm_test_eth_async_ping_reply, DM_TESTF_SCAN_FDT);
-- 
2.11.0

[U-Boot] [PATCH 10/10] net: Don't overwrite waiting packets with asynchronous replies

[Joe Hershberger]

Peter originally sent a fix, but it breaks a number of other things.
This addresses the original reported issue in a different way.

That report was:

> U-Boot has 1 common buffer to send Ethernet frames, pointed to by
> net_tx_packet.  When sending to an IP address without knowing the MAC
> address, U-Boot makes an ARP request (using the arp_tx_packet buffer)
> to find out the MAC address of the IP addressr. When a matching ARP
> reply is received, U-Boot continues sending the frame stored in the
> net_tx_packet buffer.
>
> However, in the mean time, if U-Boot needs to send out any network
> packets (e.g. replying ping packets or ARP requests for its own IP
> address etc.), it will use the net_tx_packet buffer to prepare the
> new packet. Thus this buffer is no longer the original packet meant
> to be transmitted after the ARP reply. The original packet will be
> lost.

This instead uses the ARP tx buffer to send async replies in the case
where we are actively waiting for an ARP reply.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Reported-by: Tran Tien Dat <peter.trantiendat@gmail.com>
---

 include/net.h | 8 ++++++++
 net/arp.c     | 9 +++++----
 net/arp.h     | 1 +
 net/net.c     | 8 ++++++++
 net/ping.c    | 7 +++++--
 test/dm/eth.c | 6 ++----
 6 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/include/net.h b/include/net.h
index 63718a47f2..03d1056bc5 100644
--- a/include/net.h
+++ b/include/net.h
@@ -654,6 +654,14 @@ static inline void net_set_state(enum net_loop_state state)
 	net_state = state;
 }
 
+/*
+ * net_get_async_tx_pkt_buf - Get a packet buffer that is not in use for
+ *			      sending an asynchronous reply
+ *
+ * returns - ptr to packet buffer
+ */
+uchar * net_get_async_tx_pkt_buf(void);
+
 /* Transmit a packet */
 static inline void net_send_packet(uchar *pkt, int len)
 {
diff --git a/net/arp.c b/net/arp.c
index 524361cf1b..5af65dffd6 100644
--- a/net/arp.c
+++ b/net/arp.c
@@ -34,8 +34,7 @@ uchar	       *arp_wait_packet_ethaddr;
 int		arp_wait_tx_packet_size;
 ulong		arp_wait_timer_start;
 int		arp_wait_try;
-
-static uchar   *arp_tx_packet;	/* THE ARP transmit packet */
+uchar	       *arp_tx_packet; /* THE ARP transmit packet */
 static uchar	arp_tx_packet_buf[PKTSIZE_ALIGN + PKTALIGN];
 
 void arp_init(void)
@@ -126,6 +125,7 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 	struct arp_hdr *arp;
 	struct in_addr reply_ip_addr;
 	int eth_hdr_size;
+	uchar *tx_packet;
 
 	/*
 	 * We have to deal with two types of ARP packets:
@@ -182,8 +182,9 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 		    (net_read_ip(&arp->ar_spa).s_addr & net_netmask.s_addr))
 			udelay(5000);
 #endif
-		memcpy(net_tx_packet, et, eth_hdr_size + ARP_HDR_SIZE);
-		net_send_packet(net_tx_packet, eth_hdr_size + ARP_HDR_SIZE);
+		tx_packet = net_get_async_tx_pkt_buf();
+		memcpy(tx_packet, et, eth_hdr_size + ARP_HDR_SIZE);
+		net_send_packet(tx_packet, eth_hdr_size + ARP_HDR_SIZE);
 		return;
 
 	case ARPOP_REPLY:		/* arp reply */
diff --git a/net/arp.h b/net/arp.h
index afb86958f3..25b3c00d5c 100644
--- a/net/arp.h
+++ b/net/arp.h
@@ -20,6 +20,7 @@ extern uchar *arp_wait_packet_ethaddr;
 extern int arp_wait_tx_packet_size;
 extern ulong arp_wait_timer_start;
 extern int arp_wait_try;
+extern uchar *arp_tx_packet;
 
 void arp_init(void);
 void arp_request(void);
diff --git a/net/net.c b/net/net.c
index f35695b4fc..bc3b66837d 100644
--- a/net/net.c
+++ b/net/net.c
@@ -799,6 +799,14 @@ void net_set_timeout_handler(ulong iv, thand_f *f)
 	}
 }
 
+uchar *net_get_async_tx_pkt_buf(void)
+{
+	if (arp_is_waiting())
+		return arp_tx_packet; /* If we are waiting, we already sent */
+	else
+		return net_tx_packet;
+}
+
 int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, int sport,
 		int payload_len)
 {
diff --git a/net/ping.c b/net/ping.c
index 3e5461a36a..821d35d01d 100644
--- a/net/ping.c
+++ b/net/ping.c
@@ -84,6 +84,7 @@ void ping_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 	struct icmp_hdr *icmph = (struct icmp_hdr *)&ip->udp_src;
 	struct in_addr src_ip;
 	int eth_hdr_size;
+	uchar *tx_packet;
 
 	switch (icmph->type) {
 	case ICMP_ECHO_REPLY:
@@ -107,8 +108,10 @@ void ping_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
 		icmph->type = ICMP_ECHO_REPLY;
 		icmph->checksum = 0;
 		icmph->checksum = compute_ip_checksum(icmph, len - IP_HDR_SIZE);
-		memcpy(net_tx_packet, et, eth_hdr_size + len);
-		net_send_packet(net_tx_packet, eth_hdr_size + len);
+
+		tx_packet = net_get_async_tx_pkt_buf();
+		memcpy(tx_packet, et, eth_hdr_size + len);
+		net_send_packet(tx_packet, eth_hdr_size + len);
 		return;
 /*	default:
 		return;*/
diff --git a/test/dm/eth.c b/test/dm/eth.c
index 77c602beaf..e9ac046431 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -327,10 +327,9 @@ static int dm_test_eth_async_arp_reply(struct unit_test_state *uts)
 
 	sandbox_eth_set_tx_handler(0, sb_with_async_arp_handler);
 	sandbox_eth_set_priv(0, uts);
-	sandbox_eth_skip_timeout();
 
 	env_set("ethact", "eth at 10002000");
-	ut_assert(net_loop(PING) == -ETIMEDOUT);
+	ut_assertok(net_loop(PING));
 	ut_asserteq_str("eth at 10002000", env_get("ethact"));
 
 	sandbox_eth_set_tx_handler(0, NULL);
@@ -408,10 +407,9 @@ static int dm_test_eth_async_ping_reply(struct unit_test_state *uts)
 
 	sandbox_eth_set_tx_handler(0, sb_with_async_ping_handler);
 	sandbox_eth_set_priv(0, uts);
-	sandbox_eth_skip_timeout();
 
 	env_set("ethact", "eth at 10002000");
-	ut_assert(net_loop(PING) == -ETIMEDOUT);
+	ut_assertok(net_loop(PING));
 	ut_asserteq_str("eth at 10002000", env_get("ethact"));
 
 	sandbox_eth_set_tx_handler(0, NULL);
-- 
2.11.0

[U-Boot] [PATCH 01/10] net: sandbox: Move disabled flag into priv struct

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Store the per-device data with the device.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  drivers/net/sandbox.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 02/10] net: sandbox: Refactor sandbox send function

[Simon Glass]

Hi Joe,

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Make the behavior of the send function reusable.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  20 +++++
>  drivers/net/sandbox.c          | 176 ++++++++++++++++++++++++-----------------
>  2 files changed, 124 insertions(+), 72 deletions(-)
>
> diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
> index bfcd11b593..00062616a4 100644
> --- a/arch/sandbox/include/asm/eth.h
> +++ b/arch/sandbox/include/asm/eth.h
> @@ -13,4 +13,24 @@ void sandbox_eth_disable_response(int index, bool disable);
>
>  void sandbox_eth_skip_timeout(void);
>
> +/*
> + * sandbox_eth_arp_req_to_reply()
> + *
> + * Check for an arp request to be sent. If so, inject a reply

comments for args?

> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
> +                                unsigned int len);
> +
> +/*
> + * sandbox_eth_ping_req_to_reply()
> + *
> + * Check for a ping request to be sent. If so, inject a reply
> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
> +                                 unsigned int len);
> +
>  #endif /* __ETH_H */
> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
> index 60fe065ee5..5746af11a6 100644
> --- a/drivers/net/sandbox.c
> +++ b/drivers/net/sandbox.c
> @@ -63,6 +63,108 @@ void sandbox_eth_skip_timeout(void)
>         skip_timeout = true;
>  }
>
> +/*
> + * sandbox_eth_arp_req_to_reply()
> + *
> + * Check for an arp request to be sent. If so, inject a reply
> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
> +                                unsigned int len)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth = packet;
> +       struct arp_hdr *arp;
> +       struct ethernet_hdr *eth_recv;
> +       struct arp_hdr *arp_recv;
> +
> +       if (ntohs(eth->et_protlen) != PROT_ARP)
> +               return 0;

It seems odd to put this check here? Why not check it before calling
this function?

> +
> +       arp = packet + ETHER_HDR_SIZE;
> +
> +       if (ntohs(arp->ar_op) != ARPOP_REQUEST)
> +               return 0;
> +
> +       /* store this as the assumed IP of the fake host */
> +       priv->fake_host_ipaddr = net_read_ip(&arp->ar_tpa);
> +
> +       /* Formulate a fake response */
> +       eth_recv = (void *)priv->recv_packet_buffer;
> +       memcpy(eth_recv->et_dest, eth->et_src, ARP_HLEN);
> +       memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
> +       eth_recv->et_protlen = htons(PROT_ARP);
> +
> +       arp_recv = (void *)eth_recv + ETHER_HDR_SIZE;
> +       arp_recv->ar_hrd = htons(ARP_ETHER);
> +       arp_recv->ar_pro = htons(PROT_IP);
> +       arp_recv->ar_hln = ARP_HLEN;
> +       arp_recv->ar_pln = ARP_PLEN;
> +       arp_recv->ar_op = htons(ARPOP_REPLY);
> +       memcpy(&arp_recv->ar_sha, priv->fake_host_hwaddr, ARP_HLEN);
> +       net_write_ip(&arp_recv->ar_spa, priv->fake_host_ipaddr);
> +       memcpy(&arp_recv->ar_tha, &arp->ar_sha, ARP_HLEN);
> +       net_copy_ip(&arp_recv->ar_tpa, &arp->ar_spa);
> +
> +       priv->recv_packet_length = ETHER_HDR_SIZE + ARP_HDR_SIZE;
> +
> +       return 1;
> +}
> +

[..]

> +       sandbox_eth_arp_req_to_reply(dev, packet, length);
> +       sandbox_eth_ping_req_to_reply(dev, packet, length);

Here functions are called unconditionally, but presumably only at most
one function will do anything.

>
>         return 0;
>  }
> --
> 2.11.0
>


Regards,
Simon

[U-Boot] [PATCH 03/10] net: sandbox: Make the fake eth driver response configurable

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Make the send handler registerable so tests can check for different
> things.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 17 ++++++++++++++
>  drivers/net/sandbox.c          | 53 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 66 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 04/10] net: sandbox: Share the priv structure with tests

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> If tests want to implement tx handlers, they will likely need access to
> the details in the priv structure.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 19 +++++++++++++++++++
>  drivers/net/sandbox.c          | 19 -------------------
>  2 files changed, 19 insertions(+), 19 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 05/10] net: sandbox: Allow fake eth to handle more than 1 packet response

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Use up to the max allocated receive buffers so be able to test more
> complex situations.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 10 +++++---
>  drivers/net/sandbox.c          | 57 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 52 insertions(+), 15 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 06/10] net: Add an accessor to know if waiting for ARP

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> This single-sources the state of the ARP.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  include/net.h |  1 +
>  net/arp.c     | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 07/10] net: sandbox: Add a priv ptr for tests to use

[Simon Glass]

Hi,

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:

Missing commit message - what is this for?


> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++++++
>  drivers/net/sandbox.c          | 20 ++++++++++++++++++++
>  2 files changed, 29 insertions(+)

Regards,
Simon

[U-Boot] [PATCH 08/10] test: eth: Add a test for ARP requests

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> This tests that ARP requests made to this target's IP address are
> responded-to by the target when it is doing other networking operations.
>
> This currently corrupts the ongoing operation of the device if it
> happens to be awaiting an ARP reply of its own to whatever serverip it
> is attempting to communicate with. In the test, add an expectation that
> the user operation (ping, in this case) will fail. A later patch will
> address this problem.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++
>  drivers/net/sandbox.c          | 41 +++++++++++++++++++++
>  test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)

Nice!

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 09/10] test: eth: Add a test for the target being pinged

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> The target will respond to pings while doing other network handling.
> Make sure that the response happens and is correct.
>
> This currently corrupts the ongoing operation of the device if it
> happens to be awaiting an ARP reply of its own to whatever serverip it
> is attempting to communicate with. In the test, add an expectation that
> the user operation (ping, in this case) will fail. A later patch will
> address this problem.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++
>  drivers/net/sandbox.c          | 51 ++++++++++++++++++++++++++
>  test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 10/10] net: Don't overwrite waiting packets with asynchronous replies

[Simon Glass]

On 24 July 2018 at 15:40, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Peter originally sent a fix, but it breaks a number of other things.
> This addresses the original reported issue in a different way.
>
> That report was:
>
>> U-Boot has 1 common buffer to send Ethernet frames, pointed to by
>> net_tx_packet.  When sending to an IP address without knowing the MAC
>> address, U-Boot makes an ARP request (using the arp_tx_packet buffer)
>> to find out the MAC address of the IP addressr. When a matching ARP
>> reply is received, U-Boot continues sending the frame stored in the
>> net_tx_packet buffer.
>>
>> However, in the mean time, if U-Boot needs to send out any network
>> packets (e.g. replying ping packets or ARP requests for its own IP
>> address etc.), it will use the net_tx_packet buffer to prepare the
>> new packet. Thus this buffer is no longer the original packet meant
>> to be transmitted after the ARP reply. The original packet will be
>> lost.
>
> This instead uses the ARP tx buffer to send async replies in the case
> where we are actively waiting for an ARP reply.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> Reported-by: Tran Tien Dat <peter.trantiendat@gmail.com>
> ---
>
>  include/net.h | 8 ++++++++
>  net/arp.c     | 9 +++++----
>  net/arp.h     | 1 +
>  net/net.c     | 8 ++++++++
>  net/ping.c    | 7 +++++--
>  test/dm/eth.c | 6 ++----
>  6 files changed, 29 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 01/10] net: sandbox: Move disabled flag into priv struct

[Bin Meng]

On Wed, Jul 25, 2018 at 5:40 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Store the per-device data with the device.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  drivers/net/sandbox.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 02/10] net: sandbox: Refactor sandbox send function

[Bin Meng]

Hi Joe,

On Wed, Jul 25, 2018 at 5:46 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Make the behavior of the send function reusable.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  20 +++++
>  drivers/net/sandbox.c          | 176 ++++++++++++++++++++++++-----------------
>  2 files changed, 124 insertions(+), 72 deletions(-)
>
> diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
> index bfcd11b593..00062616a4 100644
> --- a/arch/sandbox/include/asm/eth.h
> +++ b/arch/sandbox/include/asm/eth.h
> @@ -13,4 +13,24 @@ void sandbox_eth_disable_response(int index, bool disable);
>
>  void sandbox_eth_skip_timeout(void);
>
> +/*
> + * sandbox_eth_arp_req_to_reply()
> + *
> + * Check for an arp request to be sent. If so, inject a reply
> + *
> + * returns 1 if injected, 0 if not

The meaning of return code 1/0 seems odd. Normally 0 means OK. Can we
use -Exxx for the error condition?

> + */
> +int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
> +                                unsigned int len);
> +
> +/*
> + * sandbox_eth_ping_req_to_reply()
> + *
> + * Check for a ping request to be sent. If so, inject a reply
> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
> +                                 unsigned int len);
> +
>  #endif /* __ETH_H */

[snip]

Regards,
Bin

[U-Boot] [PATCH 03/10] net: sandbox: Make the fake eth driver response configurable

[Bin Meng]

On Wed, Jul 25, 2018 at 5:47 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Make the send handler registerable so tests can check for different
> things.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 17 ++++++++++++++
>  drivers/net/sandbox.c          | 53 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 66 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 04/10] net: sandbox: Share the priv structure with tests

[Bin Meng]

On Wed, Jul 25, 2018 at 5:42 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> If tests want to implement tx handlers, they will likely need access to
> the details in the priv structure.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 19 +++++++++++++++++++
>  drivers/net/sandbox.c          | 19 -------------------
>  2 files changed, 19 insertions(+), 19 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 05/10] net: sandbox: Allow fake eth to handle more than 1 packet response

[Bin Meng]

On Wed, Jul 25, 2018 at 5:41 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Use up to the max allocated receive buffers so be able to test more
> complex situations.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h | 10 +++++---
>  drivers/net/sandbox.c          | 57 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 52 insertions(+), 15 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 06/10] net: Add an accessor to know if waiting for ARP

[Bin Meng]

Hi Joe,

On Wed, Jul 25, 2018 at 5:45 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> This single-sources the state of the ARP.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  include/net.h |  1 +
>  net/arp.c     | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index f9984ae86c..63718a47f2 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -635,6 +635,7 @@ rxhand_f *net_get_udp_handler(void);        /* Get UDP RX packet handler */
>  void net_set_udp_handler(rxhand_f *);  /* Set UDP RX packet handler */
>  rxhand_f *net_get_arp_handler(void);   /* Get ARP RX packet handler */
>  void net_set_arp_handler(rxhand_f *);  /* Set ARP RX packet handler */
> +int arp_is_waiting(void);              /* Waiting for ARP reply? */

Can we use 'bool' instead of 'int'?

>  void net_set_icmp_handler(rxhand_icmp_f *f); /* Set ICMP RX handler */
>  void net_set_timeout_handler(ulong, thand_f *);/* Set timeout handler */
>
> diff --git a/net/arp.c b/net/arp.c
> index b8a71684cd..524361cf1b 100644
> --- a/net/arp.c
> +++ b/net/arp.c
> @@ -100,7 +100,7 @@ int arp_timeout_check(void)
>  {
>         ulong t;
>
> -       if (!net_arp_wait_packet_ip.s_addr)
> +       if (!arp_is_waiting())
>                 return 0;
>
>         t = get_timer(0);
> @@ -187,8 +187,8 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
>                 return;
>
>         case ARPOP_REPLY:               /* arp reply */
> -               /* are we waiting for a reply */
> -               if (!net_arp_wait_packet_ip.s_addr)
> +               /* are we waiting for a reply? */
> +               if (!arp_is_waiting())
>                         break;
>
>  #ifdef CONFIG_KEEP_SERVERADDR
> @@ -233,3 +233,8 @@ void arp_receive(struct ethernet_hdr *et, struct ip_udp_hdr *ip, int len)
>                 return;
>         }
>  }
> +
> +int arp_is_waiting(void)
> +{
> +       return !!net_arp_wait_packet_ip.s_addr;
> +}
> --

Regards,
Bin

[U-Boot] [PATCH 07/10] net: sandbox: Add a priv ptr for tests to use

[Bin Meng]

Hi Joe,

On Wed, Jul 25, 2018 at 5:45 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++++++
>  drivers/net/sandbox.c          | 20 ++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
> index 6dabbf00ab..6bb3f1bbfd 100644
> --- a/arch/sandbox/include/asm/eth.h
> +++ b/arch/sandbox/include/asm/eth.h
> @@ -53,6 +53,7 @@ typedef int sandbox_eth_tx_hand_f(struct udevice *dev, void *pkt,
>   * recv_packet_length - lengths of the packet returned as received
>   * recv_packets - number of packets returned
>   * tx_handler - function to generate responses to sent packets
> + * priv - a pointer to some structure a test may want to keep track of
>   */
>  struct eth_sandbox_priv {
>         uchar fake_host_hwaddr[ARP_HLEN];
> @@ -62,6 +63,7 @@ struct eth_sandbox_priv {
>         int recv_packet_length[PKTBUFSRX];
>         int recv_packets;
>         sandbox_eth_tx_hand_f *tx_handler;
> +       void *priv;
>  };
>
>  /*
> @@ -71,4 +73,11 @@ struct eth_sandbox_priv {
>   */
>  void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler);
>
> +/*
> + * Set priv ptr
> + *
> + * priv - priv void ptr to store in the device
> + */
> +void sandbox_eth_set_priv(int index, void *priv);
> +
>  #endif /* __ETH_H */
> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
> index 5117af8a82..29767ef291 100644
> --- a/drivers/net/sandbox.c
> +++ b/drivers/net/sandbox.c
> @@ -204,6 +204,26 @@ void sandbox_eth_set_tx_handler(int index, sandbox_eth_tx_hand_f *handler)
>                 priv->tx_handler = sb_default_handler;
>  }
>
> +/*
> + * Set priv ptr
> + *
> + * priv - priv void ptr to store in the device
> + */
> +void sandbox_eth_set_priv(int index, void *priv)
> +{
> +       struct udevice *dev;
> +       struct eth_sandbox_priv *uc_priv;

Can we use 'priv' instead? As the name uc_prive seems to mean uclass's
prive, which isn't the truth.

> +       int ret;
> +
> +       ret = uclass_get_device(UCLASS_ETH, index, &dev);
> +       if (ret)
> +               return;
> +
> +       uc_priv = dev_get_priv(dev);
> +
> +       uc_priv->priv = priv;
> +}
> +
>  static int sb_eth_start(struct udevice *dev)
>  {
>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
> --

Regards,
Bin

[U-Boot] [PATCH 08/10] test: eth: Add a test for ARP requests

[Bin Meng]

Hi Joe,

On Wed, Jul 25, 2018 at 5:43 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> This tests that ARP requests made to this target's IP address are
> responded-to by the target when it is doing other networking operations.
>
> This currently corrupts the ongoing operation of the device if it
> happens to be awaiting an ARP reply of its own to whatever serverip it
> is attempting to communicate with. In the test, add an expectation that
> the user operation (ping, in this case) will fail. A later patch will
> address this problem.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++
>  drivers/net/sandbox.c          | 41 +++++++++++++++++++++
>  test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

But please see comments below:

> diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
> index 6bb3f1bbfd..7cd5b551d9 100644
> --- a/arch/sandbox/include/asm/eth.h
> +++ b/arch/sandbox/include/asm/eth.h
> @@ -33,6 +33,15 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void *packet,
>  int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
>                                   unsigned int len);
>
> +/*
> + * sandbox_eth_recv_arp_req()
> + *
> + * Inject an ARP request for this target
> + *
> + * returns 1 if injected, 0 if not

again the return codes, can we use -Exxx instead? I see the return
error is not checked anywhere, maybe we should just make it return
void?

> + */
> +int sandbox_eth_recv_arp_req(struct udevice *dev);
> +
>  /**
>   * A packet handler
>   *
> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
> index 29767ef291..2c2a2c6311 100644
> --- a/drivers/net/sandbox.c
> +++ b/drivers/net/sandbox.c
> @@ -161,6 +161,47 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
>  }
>
>  /*
> + * sandbox_eth_recv_arp_req()
> + *
> + * Inject an ARP request for this target
> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_recv_arp_req(struct udevice *dev)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth_recv;
> +       struct arp_hdr *arp_recv;
> +
> +       /* Don't allow the buffer to overrun */
> +       if (priv->recv_packets >= PKTBUFSRX)
> +               return 0;
> +
> +       /* Formulate a fake request */
> +       eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
> +       memcpy(eth_recv->et_dest, net_bcast_ethaddr, ARP_HLEN);
> +       memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
> +       eth_recv->et_protlen = htons(PROT_ARP);
> +
> +       arp_recv = (void *)eth_recv + ETHER_HDR_SIZE;
> +       arp_recv->ar_hrd = htons(ARP_ETHER);
> +       arp_recv->ar_pro = htons(PROT_IP);
> +       arp_recv->ar_hln = ARP_HLEN;
> +       arp_recv->ar_pln = ARP_PLEN;
> +       arp_recv->ar_op = htons(ARPOP_REQUEST);
> +       memcpy(&arp_recv->ar_sha, priv->fake_host_hwaddr, ARP_HLEN);
> +       net_write_ip(&arp_recv->ar_spa, priv->fake_host_ipaddr);
> +       memcpy(&arp_recv->ar_tha, net_null_ethaddr, ARP_HLEN);
> +       net_write_ip(&arp_recv->ar_tpa, net_ip);
> +
> +       priv->recv_packet_length[priv->recv_packets] =
> +               ETHER_HDR_SIZE + ARP_HDR_SIZE;
> +       ++priv->recv_packets;
> +
> +       return 1;
> +}
> +
> +/*
>   * sb_default_handler()
>   *
>   * perform typical responses to simple ping
> diff --git a/test/dm/eth.c b/test/dm/eth.c
> index 1a7684a887..9b5f53e819 100644
> --- a/test/dm/eth.c
> +++ b/test/dm/eth.c
> @@ -258,3 +258,84 @@ static int dm_test_net_retry(struct unit_test_state *uts)
>         return retval;
>  }
>  DM_TEST(dm_test_net_retry, DM_TESTF_SCAN_FDT);
> +
> +static int sb_check_arp_reply(struct udevice *dev, void *packet,
> +                             unsigned int len)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth = packet;
> +       struct arp_hdr *arp;
> +       struct unit_test_state *uts = priv->priv;

This is not used anywhere. What is this for?

> +
> +       if (ntohs(eth->et_protlen) != PROT_ARP)
> +               return 0;
> +
> +       arp = packet + ETHER_HDR_SIZE;
> +
> +       if (ntohs(arp->ar_op) != ARPOP_REPLY)
> +               return 0;
> +
> +       /* This test would be worthless if we are not waiting */
> +       ut_assert(arp_is_waiting());
> +
> +       /* Validate response */
> +       ut_assert(memcmp(eth->et_src, net_ethaddr, ARP_HLEN) == 0);
> +       ut_assert(memcmp(eth->et_dest, priv->fake_host_hwaddr, ARP_HLEN) == 0);
> +       ut_assert(eth->et_protlen == htons(PROT_ARP));
> +
> +       ut_assert(arp->ar_hrd == htons(ARP_ETHER));
> +       ut_assert(arp->ar_pro == htons(PROT_IP));
> +       ut_assert(arp->ar_hln == ARP_HLEN);
> +       ut_assert(arp->ar_pln == ARP_PLEN);
> +       ut_assert(memcmp(&arp->ar_sha, net_ethaddr, ARP_HLEN) == 0);
> +       ut_assert(net_read_ip(&arp->ar_spa).s_addr == net_ip.s_addr);
> +       ut_assert(memcmp(&arp->ar_tha, priv->fake_host_hwaddr, ARP_HLEN) == 0);
> +       ut_assert(net_read_ip(&arp->ar_tpa).s_addr ==
> +                 string_to_ip("1.1.2.4").s_addr);
> +
> +       return 0;
> +}
> +
> +static int sb_with_async_arp_handler(struct udevice *dev, void *packet,
> +                                    unsigned int len)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth = packet;
> +       struct arp_hdr *arp = packet + ETHER_HDR_SIZE;
> +
> +       /*
> +        * If we are about to generate a reply to ARP, first inject a request
> +        * from another host
> +        */
> +       if (ntohs(eth->et_protlen) == PROT_ARP &&
> +           ntohs(arp->ar_op) == ARPOP_REQUEST) {
> +               /* Make sure sandbox_eth_recv_arp_req() knows who is asking */
> +               priv->fake_host_ipaddr = string_to_ip("1.1.2.4");
> +
> +               sandbox_eth_recv_arp_req(dev);
> +       }
> +
> +       sandbox_eth_arp_req_to_reply(dev, packet, len);
> +       sandbox_eth_ping_req_to_reply(dev, packet, len);
> +
> +       return sb_check_arp_reply(dev, packet, len);
> +}
> +
> +static int dm_test_eth_async_arp_reply(struct unit_test_state *uts)
> +{
> +       net_ping_ip = string_to_ip("1.1.2.2");
> +
> +       sandbox_eth_set_tx_handler(0, sb_with_async_arp_handler);
> +       sandbox_eth_set_priv(0, uts);

Looks this is not needed since it was not used anywhere.

> +       sandbox_eth_skip_timeout();
> +
> +       env_set("ethact", "eth at 10002000");
> +       ut_assert(net_loop(PING) == -ETIMEDOUT);
> +       ut_asserteq_str("eth at 10002000", env_get("ethact"));
> +
> +       sandbox_eth_set_tx_handler(0, NULL);
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_eth_async_arp_reply, DM_TESTF_SCAN_FDT);
> --

Regards,
Bin

[U-Boot] [PATCH 09/10] test: eth: Add a test for the target being pinged

[Bin Meng]

Hi Joe,

On Wed, Jul 25, 2018 at 5:42 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> The target will respond to pings while doing other network handling.
> Make sure that the response happens and is correct.
>
> This currently corrupts the ongoing operation of the device if it
> happens to be awaiting an ARP reply of its own to whatever serverip it
> is attempting to communicate with. In the test, add an expectation that
> the user operation (ping, in this case) will fail. A later patch will
> address this problem.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/include/asm/eth.h |  9 +++++
>  drivers/net/sandbox.c          | 51 ++++++++++++++++++++++++++
>  test/dm/eth.c                  | 81 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

But please see comments below.

> diff --git a/arch/sandbox/include/asm/eth.h b/arch/sandbox/include/asm/eth.h
> index 7cd5b551d9..66f6b4078b 100644
> --- a/arch/sandbox/include/asm/eth.h
> +++ b/arch/sandbox/include/asm/eth.h
> @@ -42,6 +42,15 @@ int sandbox_eth_ping_req_to_reply(struct udevice *dev, void *packet,
>   */
>  int sandbox_eth_recv_arp_req(struct udevice *dev);
>
> +/*
> + * sandbox_eth_recv_ping_req()
> + *
> + * Inject a ping request for this target
> + *
> + * returns 1 if injected, 0 if not

again the return code issue

> + */
> +int sandbox_eth_recv_ping_req(struct udevice *dev);
> +
>  /**
>   * A packet handler
>   *
> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
> index 2c2a2c6311..039c1e3222 100644
> --- a/drivers/net/sandbox.c
> +++ b/drivers/net/sandbox.c
> @@ -202,6 +202,57 @@ int sandbox_eth_recv_arp_req(struct udevice *dev)
>  }
>
>  /*
> + * sandbox_eth_recv_ping_req()
> + *
> + * Inject a ping request for this target
> + *
> + * returns 1 if injected, 0 if not
> + */
> +int sandbox_eth_recv_ping_req(struct udevice *dev)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth_recv;
> +       struct ip_udp_hdr *ipr;
> +       struct icmp_hdr *icmpr;
> +
> +       /* Don't allow the buffer to overrun */
> +       if (priv->recv_packets >= PKTBUFSRX)
> +               return 0;
> +
> +       /* Formulate a fake ping */
> +       eth_recv = (void *)priv->recv_packet_buffer[priv->recv_packets];
> +
> +       memcpy(eth_recv->et_dest, net_ethaddr, ARP_HLEN);
> +       memcpy(eth_recv->et_src, priv->fake_host_hwaddr, ARP_HLEN);
> +       eth_recv->et_protlen = htons(PROT_IP);
> +
> +       ipr = (void *)eth_recv + ETHER_HDR_SIZE;
> +       ipr->ip_hl_v = 0x45;
> +       ipr->ip_len = htons(IP_ICMP_HDR_SIZE);
> +       ipr->ip_off = htons(IP_FLAGS_DFRAG);
> +       ipr->ip_p = IPPROTO_ICMP;
> +       ipr->ip_sum = 0;
> +       net_write_ip(&ipr->ip_src, priv->fake_host_ipaddr);
> +       net_write_ip(&ipr->ip_dst, net_ip);
> +       ipr->ip_sum = compute_ip_checksum(ipr, IP_HDR_SIZE);
> +
> +       icmpr = (struct icmp_hdr *)&ipr->udp_src;
> +
> +       icmpr->type = ICMP_ECHO_REQUEST;
> +       icmpr->code = 0;
> +       icmpr->checksum = 0;
> +       icmpr->un.echo.id = 0;
> +       icmpr->un.echo.sequence = htons(1);
> +       icmpr->checksum = compute_ip_checksum(icmpr, ICMP_HDR_SIZE);
> +
> +       priv->recv_packet_length[priv->recv_packets] =
> +               ETHER_HDR_SIZE + IP_ICMP_HDR_SIZE;
> +       ++priv->recv_packets;
> +
> +       return 1;
> +}
> +
> +/*
>   * sb_default_handler()
>   *
>   * perform typical responses to simple ping
> diff --git a/test/dm/eth.c b/test/dm/eth.c
> index 9b5f53e819..77c602beaf 100644
> --- a/test/dm/eth.c
> +++ b/test/dm/eth.c
> @@ -339,3 +339,84 @@ static int dm_test_eth_async_arp_reply(struct unit_test_state *uts)
>  }
>
>  DM_TEST(dm_test_eth_async_arp_reply, DM_TESTF_SCAN_FDT);
> +
> +static int sb_check_ping_reply(struct udevice *dev, void *packet,
> +                              unsigned int len)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth = packet;
> +       struct ip_udp_hdr *ip;
> +       struct icmp_hdr *icmp;
> +       struct unit_test_state *uts = priv->priv;

This is not used anywhere.

> +
> +       if (ntohs(eth->et_protlen) != PROT_IP)
> +               return 0;
> +
> +       ip = packet + ETHER_HDR_SIZE;
> +
> +       if (ip->ip_p != IPPROTO_ICMP)
> +               return 0;
> +
> +       icmp = (struct icmp_hdr *)&ip->udp_src;
> +
> +       if (icmp->type != ICMP_ECHO_REPLY)
> +               return 0;
> +
> +       /* This test would be worthless if we are not waiting */
> +       ut_assert(arp_is_waiting());
> +
> +       /* Validate response */
> +       ut_assert(memcmp(eth->et_src, net_ethaddr, ARP_HLEN) == 0);
> +       ut_assert(memcmp(eth->et_dest, priv->fake_host_hwaddr, ARP_HLEN) == 0);
> +       ut_assert(eth->et_protlen == htons(PROT_IP));
> +
> +       ut_assert(net_read_ip(&ip->ip_src).s_addr == net_ip.s_addr);
> +       ut_assert(net_read_ip(&ip->ip_dst).s_addr ==
> +                 string_to_ip("1.1.2.4").s_addr);
> +
> +       return 0;
> +}
> +
> +static int sb_with_async_ping_handler(struct udevice *dev, void *packet,
> +                                     unsigned int len)
> +{
> +       struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       struct ethernet_hdr *eth = packet;
> +       struct arp_hdr *arp = packet + ETHER_HDR_SIZE;
> +
> +       /*
> +        * If we are about to generate a reply to ARP, first inject a request
> +        * from another host
> +        */
> +       if (ntohs(eth->et_protlen) == PROT_ARP &&
> +           ntohs(arp->ar_op) == ARPOP_REQUEST) {
> +               /* Make sure sandbox_eth_recv_arp_req() knows who is asking */
> +               priv->fake_host_ipaddr = string_to_ip("1.1.2.4");
> +
> +               sandbox_eth_recv_ping_req(dev);
> +       }
> +
> +       sandbox_eth_arp_req_to_reply(dev, packet, len);
> +       sandbox_eth_ping_req_to_reply(dev, packet, len);
> +
> +       return sb_check_ping_reply(dev, packet, len);
> +}
> +
> +static int dm_test_eth_async_ping_reply(struct unit_test_state *uts)
> +{
> +       net_ping_ip = string_to_ip("1.1.2.2");
> +
> +       sandbox_eth_set_tx_handler(0, sb_with_async_ping_handler);
> +       sandbox_eth_set_priv(0, uts);

ditto

> +       sandbox_eth_skip_timeout();
> +
> +       env_set("ethact", "eth at 10002000");
> +       ut_assert(net_loop(PING) == -ETIMEDOUT);
> +       ut_asserteq_str("eth at 10002000", env_get("ethact"));
> +
> +       sandbox_eth_set_tx_handler(0, NULL);
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_eth_async_ping_reply, DM_TESTF_SCAN_FDT);
> --

Regards,
Bin

[U-Boot] [PATCH 10/10] net: Don't overwrite waiting packets with asynchronous replies

[Bin Meng]

On Wed, Jul 25, 2018 at 5:44 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Peter originally sent a fix, but it breaks a number of other things.
> This addresses the original reported issue in a different way.
>
> That report was:
>
> > U-Boot has 1 common buffer to send Ethernet frames, pointed to by
> > net_tx_packet.  When sending to an IP address without knowing the MAC
> > address, U-Boot makes an ARP request (using the arp_tx_packet buffer)
> > to find out the MAC address of the IP addressr. When a matching ARP
> > reply is received, U-Boot continues sending the frame stored in the
> > net_tx_packet buffer.
> >
> > However, in the mean time, if U-Boot needs to send out any network
> > packets (e.g. replying ping packets or ARP requests for its own IP
> > address etc.), it will use the net_tx_packet buffer to prepare the
> > new packet. Thus this buffer is no longer the original packet meant
> > to be transmitted after the ARP reply. The original packet will be
> > lost.
>
> This instead uses the ARP tx buffer to send async replies in the case
> where we are actively waiting for an ARP reply.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> Reported-by: Tran Tien Dat <peter.trantiendat@gmail.com>
> ---
>
>  include/net.h | 8 ++++++++
>  net/arp.c     | 9 +++++----
>  net/arp.h     | 1 +
>  net/net.c     | 8 ++++++++
>  net/ping.c    | 7 +++++--
>  test/dm/eth.c | 6 ++----
>  6 files changed, 29 insertions(+), 10 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>