u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot
@ 2023-03-28 20:31 Dmitrii Merkurev
  2023-03-28 20:31 ` [PATCH 2/3] net: add fastboot TCP support Dmitrii Merkurev
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Dmitrii Merkurev @ 2023-03-28 20:31 UTC (permalink / raw)
  To: u-boot; +Cc: Dmitrii Merkurev, Ying-Chun Liu, Simon Glass

Make following changes to unblock TCP fastboot support:

1. Implement being a TCP server support
2. Introduce dedicated TCP traffic handler (get rid of UDP signature)
3. Ensure seq_num and ack_num are respected in net_send_tcp_packet
function (make sure existing wget_cmd code is reflected with the fix)

Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Simon Glass <sjg@chromium.org>
Сс: Joe Hershberger <joe.hershberger@ni.com>
Сс: Ramon Fried <rfried.dev@gmail.com>
---

 include/net/tcp.h |  16 +++++--
 net/tcp.c         | 115 +++++++++++++++++++++++-----------------------
 net/wget.c        |  43 ++++++++---------
 3 files changed, 90 insertions(+), 84 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 322551694f..c29d4ce24a 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -259,6 +259,7 @@ union tcp_build_pkt {
  * enum tcp_state - TCP State machine states for connection
  * @TCP_CLOSED: Need to send SYN to connect
  * @TCP_SYN_SENT: Trying to connect, waiting for SYN ACK
+ * @TCP_SYN_RECEIVED: Initial SYN received, waiting for ACK
  * @TCP_ESTABLISHED: both server & client have a connection
  * @TCP_CLOSE_WAIT: Rec FIN, passed to app for FIN, ACK rsp
  * @TCP_CLOSING: Rec FIN, sent FIN, ACK waiting for ACK
@@ -268,6 +269,7 @@ union tcp_build_pkt {
 enum tcp_state {
 	TCP_CLOSED,
 	TCP_SYN_SENT,
+	TCP_SYN_RECEIVED,
 	TCP_ESTABLISHED,
 	TCP_CLOSE_WAIT,
 	TCP_CLOSING,
@@ -283,14 +285,18 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
 /**
  * rxhand_tcp() - An incoming packet handler.
  * @pkt: pointer to the application packet
- * @dport: destination UDP port
+ * @dport: destination TCP port
  * @sip: source IP address
- * @sport: source UDP port
+ * @sport: source TCP port
+ * @tcp_seq_num: TCP sequential number
+ * @tcp_ack_num: TCP acknowledgment number
+ * @action: TCP action (SYN, ACK, FIN, etc)
  * @len: packet length
  */
-typedef void rxhand_tcp(uchar *pkt, unsigned int dport,
-			struct in_addr sip, unsigned int sport,
-			unsigned int len);
+typedef void rxhand_tcp(uchar *pkt, u16 dport,
+			struct in_addr sip, u16 sport,
+			u32 tcp_seq_num, u32 tcp_ack_num,
+			u8 action, unsigned int len);
 void tcp_set_tcp_handler(rxhand_tcp *f);
 
 void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int len);
diff --git a/net/tcp.c b/net/tcp.c
index 8d338c72e8..a713e1dd60 100644
--- a/net/tcp.c
+++ b/net/tcp.c
@@ -36,7 +36,6 @@ static u32 rmt_timestamp;
 
 static u32 tcp_seq_init;
 static u32 tcp_ack_edge;
-static u32 tcp_seq_max;
 
 static int tcp_activity_count;
 
@@ -90,9 +89,10 @@ void tcp_set_tcp_state(enum tcp_state new_state)
 	current_tcp_state = new_state;
 }
 
-static void dummy_handler(uchar *pkt, unsigned int dport,
-			  struct in_addr sip, unsigned int sport,
-			  unsigned int len)
+static void dummy_handler(uchar *pkt, u16 dport,
+			  struct in_addr sip, u16 sport,
+			  u32 tcp_seq_num, u32 tcp_ack_num,
+			  u8 action, unsigned int len)
 {
 }
 
@@ -256,7 +256,7 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
 	switch (action) {
 	case TCP_SYN:
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:SYN (%pI4, %pI4, sq=%d, ak=%d)\n",
+			   "TCP Hdr:SYN (%pI4, %pI4, sq=%u, ak=%u)\n",
 			   &net_server_ip, &net_ip,
 			   tcp_seq_num, tcp_ack_num);
 		tcp_activity_count = 0;
@@ -271,41 +271,46 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
 			current_tcp_state = TCP_SYN_SENT;
 		}
 		break;
+	case TCP_SYN | TCP_ACK:
 	case TCP_ACK:
 		pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
 		b->ip.hdr.tcp_flags = action;
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:ACK (%pI4, %pI4, s=%d, a=%d, A=%x)\n",
+			   "TCP Hdr:ACK (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
 			   &net_server_ip, &net_ip, tcp_seq_num, tcp_ack_num,
 			   action);
 		break;
 	case TCP_FIN:
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:FIN  (%pI4, %pI4, s=%d, a=%d)\n",
+			   "TCP Hdr:FIN  (%pI4, %pI4, s=%u, a=%u)\n",
 			   &net_server_ip, &net_ip, tcp_seq_num, tcp_ack_num);
 		payload_len = 0;
 		pkt_hdr_len = IP_TCP_HDR_SIZE;
 		current_tcp_state = TCP_FIN_WAIT_1;
 		break;
-
+	case TCP_RST | TCP_ACK:
+	case TCP_RST:
+		debug_cond(DEBUG_DEV_PKT,
+			   "TCP Hdr:RST  (%pI4, %pI4, s=%u, a=%u)\n",
+			   &net_server_ip, &net_ip, tcp_seq_num, tcp_ack_num);
+		current_tcp_state = TCP_CLOSED;
+		break;
 	/* Notify connection closing */
-
 	case (TCP_FIN | TCP_ACK):
 	case (TCP_FIN | TCP_ACK | TCP_PUSH):
 		if (current_tcp_state == TCP_CLOSE_WAIT)
 			current_tcp_state = TCP_CLOSING;
 
-		tcp_ack_edge++;
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:FIN ACK PSH(%pI4, %pI4, s=%d, a=%d, A=%x)\n",
+			   "TCP Hdr:FIN ACK PSH(%pI4, %pI4, s=%u, a=%u, A=%x)\n",
 			   &net_server_ip, &net_ip,
-			   tcp_seq_num, tcp_ack_edge, action);
+			   tcp_seq_num, tcp_ack_num, action);
 		fallthrough;
 	default:
 		pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
 		b->ip.hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Hdr:dft  (%pI4, %pI4, s=%d, a=%d, A=%x)\n",
+			   "TCP Hdr:dft  (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
 			   &net_server_ip, &net_ip,
 			   tcp_seq_num, tcp_ack_num, action);
 	}
@@ -313,12 +318,12 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
 	pkt_len	= pkt_hdr_len + payload_len;
 	tcp_len	= pkt_len - IP_HDR_SIZE;
 
+	tcp_ack_edge = tcp_ack_num;
 	/* TCP Header */
 	b->ip.hdr.tcp_ack = htonl(tcp_ack_edge);
 	b->ip.hdr.tcp_src = htons(sport);
 	b->ip.hdr.tcp_dst = htons(dport);
 	b->ip.hdr.tcp_seq = htonl(tcp_seq_num);
-	tcp_seq_num = tcp_seq_num + payload_len;
 
 	/*
 	 * TCP window size - TCP header variable tcp_win.
@@ -353,9 +358,8 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
  * tcp_hole() - Selective Acknowledgment (Essential for fast stream transfer)
  * @tcp_seq_num: TCP sequence start number
  * @len: the length of sequence numbers
- * @tcp_seq_max: maximum of sequence numbers
  */
-void tcp_hole(u32 tcp_seq_num, u32 len, u32 tcp_seq_max)
+void tcp_hole(u32 tcp_seq_num, u32 len)
 {
 	u32 idx_sack, sack_in;
 	u32 sack_end = TCP_SACK - 1;
@@ -496,7 +500,7 @@ void tcp_parse_options(uchar *o, int o_len)
 	}
 }
 
-static u8 tcp_state_machine(u8 tcp_flags, u32 *tcp_seq_num, int payload_len)
+static u8 tcp_state_machine(u8 tcp_flags, u32 tcp_seq_num, int payload_len)
 {
 	u8 tcp_fin = tcp_flags & TCP_FIN;
 	u8 tcp_syn = tcp_flags & TCP_SYN;
@@ -527,46 +531,44 @@ static u8 tcp_state_machine(u8 tcp_flags, u32 *tcp_seq_num, int payload_len)
 	switch  (current_tcp_state) {
 	case TCP_CLOSED:
 		debug_cond(DEBUG_INT_STATE, "TCP CLOSED %x\n", tcp_flags);
-		if (tcp_ack)
-			action = TCP_DATA;
-		else if (tcp_syn)
-			action = TCP_RST;
-		else if (tcp_fin)
+		if (tcp_syn) {
+			action = TCP_SYN | TCP_ACK;
+			tcp_seq_init = tcp_seq_num;
+			tcp_ack_edge = tcp_seq_num + 1;
+			current_tcp_state = TCP_SYN_RECEIVED;
+		} else if (tcp_ack || tcp_fin) {
 			action = TCP_DATA;
+		}
 		break;
+	case TCP_SYN_RECEIVED:
 	case TCP_SYN_SENT:
-		debug_cond(DEBUG_INT_STATE, "TCP_SYN_SENT %x, %d\n",
-			   tcp_flags, *tcp_seq_num);
+		debug_cond(DEBUG_INT_STATE, "TCP_SYN_SENT | TCP_SYN_RECEIVED %x, %u\n",
+			   tcp_flags, tcp_seq_num);
 		if (tcp_fin) {
 			action = action | TCP_PUSH;
 			current_tcp_state = TCP_CLOSE_WAIT;
-		}
-		if (tcp_syn) {
-			action = action | TCP_ACK | TCP_PUSH;
-			if (tcp_ack) {
-				tcp_seq_init = *tcp_seq_num;
-				*tcp_seq_num = *tcp_seq_num + 1;
-				tcp_seq_max = *tcp_seq_num;
-				tcp_ack_edge = *tcp_seq_num;
-				sack_idx = 0;
-				edge_a[sack_idx].se.l = *tcp_seq_num;
-				edge_a[sack_idx].se.r = *tcp_seq_num;
-				prev_len = 0;
-				current_tcp_state = TCP_ESTABLISHED;
-				for (i = 0; i < TCP_SACK; i++)
-					edge_a[i].st = NOPKT;
-			}
-		} else if (tcp_ack) {
+		} else if (tcp_ack || (tcp_syn && tcp_ack)) {
+			action |= TCP_ACK;
+			tcp_seq_init = tcp_seq_num;
+			tcp_ack_edge = tcp_seq_num + 1;
+			sack_idx = 0;
+			edge_a[sack_idx].se.l = tcp_ack_edge;
+			edge_a[sack_idx].se.r = tcp_ack_edge;
+			prev_len = 0;
+			current_tcp_state = TCP_ESTABLISHED;
+			for (i = 0; i < TCP_SACK; i++)
+				edge_a[i].st = NOPKT;
+
+			if (tcp_syn && tcp_ack)
+				action |= TCP_PUSH;
+		} else {
 			action = TCP_DATA;
 		}
-
 		break;
 	case TCP_ESTABLISHED:
 		debug_cond(DEBUG_INT_STATE, "TCP_ESTABLISHED %x\n", tcp_flags);
-		if (*tcp_seq_num > tcp_seq_max)
-			tcp_seq_max = *tcp_seq_num;
 		if (payload_len > 0) {
-			tcp_hole(*tcp_seq_num, payload_len, tcp_seq_max);
+			tcp_hole(tcp_seq_num, payload_len);
 			tcp_fin = TCP_DATA;  /* cause standalone FIN */
 		}
 
@@ -603,15 +605,14 @@ static u8 tcp_state_machine(u8 tcp_flags, u32 *tcp_seq_num, int payload_len)
 	case TCP_FIN_WAIT_1:
 		debug_cond(DEBUG_INT_STATE, "TCP_FIN_WAIT_1 (%x)\n", tcp_flags);
 		if (tcp_fin) {
+			tcp_ack_edge++;
 			action = TCP_ACK | TCP_FIN;
 			current_tcp_state = TCP_FIN_WAIT_2;
 		}
 		if (tcp_syn)
 			action = TCP_RST;
-		if (tcp_ack) {
+		if (tcp_ack)
 			current_tcp_state = TCP_CLOSED;
-			tcp_seq_num = tcp_seq_num + 1;
-		}
 		break;
 	case TCP_CLOSING:
 		debug_cond(DEBUG_INT_STATE, "TCP_CLOSING (%x)\n", tcp_flags);
@@ -640,7 +641,6 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
 	u16 tcp_rx_xsum = b->ip.hdr.ip_sum;
 	u8  tcp_action = TCP_DATA;
 	u32 tcp_seq_num, tcp_ack_num;
-	struct in_addr action_and_state;
 	int tcp_hdr_len, payload_len;
 
 	/* Verify IP header */
@@ -685,7 +685,7 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
 
 	/* Packets are not ordered. Send to app as received. */
 	tcp_action = tcp_state_machine(b->ip.hdr.tcp_flags,
-				       &tcp_seq_num, payload_len);
+				       tcp_seq_num, payload_len);
 
 	tcp_activity_count++;
 	if (tcp_activity_count > TCP_ACTIVITY) {
@@ -695,18 +695,17 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
 
 	if ((tcp_action & TCP_PUSH) || payload_len > 0) {
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Notify (action=%x, Seq=%d,Ack=%d,Pay%d)\n",
+			   "TCP Notify (action=%x, Seq=%u,Ack=%u,Pay%d)\n",
 			   tcp_action, tcp_seq_num, tcp_ack_num, payload_len);
 
-		action_and_state.s_addr = tcp_action;
-		(*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len,
-				       tcp_seq_num, action_and_state,
-				       tcp_ack_num, payload_len);
+		(*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len, b->ip.hdr.tcp_dst,
+				       b->ip.hdr.ip_src, b->ip.hdr.tcp_src, tcp_seq_num,
+				       tcp_ack_num, tcp_action, payload_len);
 
 	} else if (tcp_action != TCP_DATA) {
 		debug_cond(DEBUG_DEV_PKT,
-			   "TCP Action (action=%x,Seq=%d,Ack=%d,Pay=%d)\n",
-			   tcp_action, tcp_seq_num, tcp_ack_num, payload_len);
+			   "TCP Action (action=%x,Seq=%u,Ack=%u,Pay=%d)\n",
+			   tcp_action, tcp_ack_num, tcp_ack_edge, payload_len);
 
 		/*
 		 * Warning: Incoming Ack & Seq sequence numbers are transposed
@@ -715,6 +714,6 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
 		net_send_tcp_packet(0, ntohs(b->ip.hdr.tcp_src),
 				    ntohs(b->ip.hdr.tcp_dst),
 				    (tcp_action & (~TCP_PUSH)),
-				    tcp_seq_num, tcp_ack_num);
+				    tcp_ack_num, tcp_ack_edge);
 	}
 }
diff --git a/net/wget.c b/net/wget.c
index eebdf80eb5..2dbfeb1a1d 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -88,8 +88,8 @@ static void wget_send_stored(void)
 {
 	u8 action = retry_action;
 	int len = retry_len;
-	unsigned int tcp_ack_num = retry_tcp_ack_num + len;
-	unsigned int tcp_seq_num = retry_tcp_seq_num;
+	unsigned int tcp_ack_num = retry_tcp_seq_num + (len == 0 ? 1 : len);
+	unsigned int tcp_seq_num = retry_tcp_ack_num;
 	uchar *ptr, *offset;
 
 	switch (current_wget_state) {
@@ -130,8 +130,8 @@ static void wget_send_stored(void)
 	}
 }
 
-static void wget_send(u8 action, unsigned int tcp_ack_num,
-		      unsigned int tcp_seq_num, int len)
+static void wget_send(u8 action, unsigned int tcp_seq_num,
+		      unsigned int tcp_ack_num, int len)
 {
 	retry_action = action;
 	retry_tcp_ack_num = tcp_ack_num;
@@ -178,10 +178,8 @@ static void wget_timeout_handler(void)
 #define PKT_QUEUE_PACKET_SIZE 0x800
 
 static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
-			   struct in_addr action_and_state,
-			   unsigned int tcp_ack_num, unsigned int len)
+			   u8 action, unsigned int tcp_ack_num, unsigned int len)
 {
-	u8 action = action_and_state.s_addr;
 	uchar *pkt_in_q;
 	char *pos;
 	int hlen, i;
@@ -268,22 +266,25 @@ static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
 }
 
 /**
- * wget_handler() - handler of wget
- * @pkt: the pointer to the payload
- * @tcp_seq_num: tcp sequence number
- * @action_and_state: TCP state
- * @tcp_ack_num: tcp acknowledge number
- * @len: length of the payload
+ * wget_handler() - TCP handler of wget
+ * @pkt: pointer to the application packet
+ * @dport: destination TCP port
+ * @sip: source IP address
+ * @sport: source TCP port
+ * @tcp_seq_num: TCP sequential number
+ * @tcp_ack_num: TCP acknowledgment number
+ * @action: TCP action (SYN, ACK, FIN, etc)
+ * @len: packet length
  *
  * In the "application push" invocation, the TCP header with all
  * its information is pointed to by the packet pointer.
  */
-static void wget_handler(uchar *pkt, unsigned int tcp_seq_num,
-			 struct in_addr action_and_state,
-			 unsigned int tcp_ack_num, unsigned int len)
+static void wget_handler(uchar *pkt, u16 dport,
+			 struct in_addr sip, u16 sport,
+			 u32 tcp_seq_num, u32 tcp_ack_num,
+			 u8 action, unsigned int len)
 {
 	enum tcp_state wget_tcp_state = tcp_get_tcp_state();
-	u8 action = action_and_state.s_addr;
 
 	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
 	packets++;
@@ -294,7 +295,7 @@ static void wget_handler(uchar *pkt, unsigned int tcp_seq_num,
 		break;
 	case WGET_CONNECTING:
 		debug_cond(DEBUG_WGET,
-			   "wget: Connecting In len=%x, Seq=%x, Ack=%x\n",
+			   "wget: Connecting In len=%x, Seq=%u, Ack=%u\n",
 			   len, tcp_seq_num, tcp_ack_num);
 		if (!len) {
 			if (wget_tcp_state == TCP_ESTABLISHED) {
@@ -310,14 +311,13 @@ static void wget_handler(uchar *pkt, unsigned int tcp_seq_num,
 		}
 		break;
 	case WGET_CONNECTED:
-		debug_cond(DEBUG_WGET, "wget: Connected seq=%x, len=%x\n",
+		debug_cond(DEBUG_WGET, "wget: Connected seq=%u, len=%x\n",
 			   tcp_seq_num, len);
 		if (!len) {
 			wget_fail("Image not found, no data returned\n",
 				  tcp_seq_num, tcp_ack_num, action);
 		} else {
-			wget_connected(pkt, tcp_seq_num, action_and_state,
-				       tcp_ack_num, len);
+			wget_connected(pkt, tcp_seq_num, action, tcp_ack_num, len);
 		}
 		break;
 	case WGET_TRANSFERRING:
@@ -338,6 +338,7 @@ static void wget_handler(uchar *pkt, unsigned int tcp_seq_num,
 			wget_send(TCP_ACK, tcp_seq_num, tcp_ack_num, len);
 			fallthrough;
 		case TCP_SYN_SENT:
+		case TCP_SYN_RECEIVED:
 		case TCP_CLOSING:
 		case TCP_FIN_WAIT_1:
 		case TCP_CLOSED:
-- 
2.40.0.348.gf938b09366-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] net: add fastboot TCP support
  2023-03-28 20:31 [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Dmitrii Merkurev
@ 2023-03-28 20:31 ` Dmitrii Merkurev
  2023-04-01  6:32   ` Simon Glass
  2023-03-28 20:31 ` [PATCH 3/3] net: share fastboot boot handle logic between transports Dmitrii Merkurev
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Dmitrii Merkurev @ 2023-03-28 20:31 UTC (permalink / raw)
  To: u-boot; +Cc: Dmitrii Merkurev, Ying-Chun Liu, Simon Glass

Known limitations are
1. fastboot reboot doesn't work (answering OK but not rebooting)
2. flashing isn't supported (TCP transport only limitation)

The command syntax is
fastboot tcp

Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Simon Glass <sjg@chromium.org>
Сс: Joe Hershberger <joe.hershberger@ni.com>
Сс: Ramon Fried <rfried.dev@gmail.com>
---

 MAINTAINERS                                |   6 +-
 cmd/fastboot.c                             |  25 +++-
 drivers/fastboot/Kconfig                   |   7 +
 drivers/fastboot/fb_common.c               |   1 -
 include/net.h                              |   3 +-
 include/net/fastboot_tcp.h                 |  14 ++
 include/net/{fastboot.h => fastboot_udp.h} |   2 +-
 net/Makefile                               |   3 +-
 net/fastboot_tcp.c                         | 143 +++++++++++++++++++++
 net/{fastboot.c => fastboot_udp.c}         |   4 +-
 net/net.c                                  |  17 ++-
 11 files changed, 210 insertions(+), 15 deletions(-)
 create mode 100644 include/net/fastboot_tcp.h
 rename include/net/{fastboot.h => fastboot_udp.h} (91%)
 create mode 100644 net/fastboot_tcp.c
 rename net/{fastboot.c => fastboot_udp.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 91d40ea4b6..501d1147d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -978,10 +978,12 @@ F:	cmd/fastboot.c
 F:	doc/android/fastboot*.rst
 F:	include/fastboot.h
 F:	include/fastboot-internal.h
-F:	include/net/fastboot.h
+F:	include/net/fastboot_tcp.h
+F:	include/net/fastboot_udp.h
 F:	drivers/fastboot/
 F:	drivers/usb/gadget/f_fastboot.c
-F:	net/fastboot.c
+F:	net/fastboot_tcp.c
+F:	net/fastboot_udp.c
 F:	test/dm/fastboot.c
 
 FPGA
diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index 97dc02ce74..3d5ff951eb 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -26,7 +26,7 @@ static int do_fastboot_udp(int argc, char *const argv[],
 		return CMD_RET_FAILURE;
 	}
 
-	err = net_loop(FASTBOOT);
+	err = net_loop(FASTBOOT_UDP);
 
 	if (err < 0) {
 		printf("fastboot udp error: %d\n", err);
@@ -36,6 +36,26 @@ static int do_fastboot_udp(int argc, char *const argv[],
 	return CMD_RET_SUCCESS;
 }
 
+static int do_fastboot_tcp(int argc, char *const argv[],
+			   uintptr_t buf_addr, size_t buf_size)
+{
+	int err;
+
+	if (!IS_ENABLED(CONFIG_TCP_FUNCTION_FASTBOOT)) {
+		pr_err("Fastboot TCP not enabled\n");
+		return CMD_RET_FAILURE;
+	}
+
+	err = net_loop(FASTBOOT_TCP);
+
+	if (err < 0) {
+		printf("fastboot tcp error: %d\n", err);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 static int do_fastboot_usb(int argc, char *const argv[],
 			   uintptr_t buf_addr, size_t buf_size)
 {
@@ -141,7 +161,8 @@ NXTARG:
 
 	if (!strcmp(argv[1], "udp"))
 		return do_fastboot_udp(argc, argv, buf_addr, buf_size);
-
+	if (!strcmp(argv[1], "tcp"))
+		return do_fastboot_tcp(argc, argv, buf_addr, buf_size);
 	if (!strcmp(argv[1], "usb")) {
 		argv++;
 		argc--;
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
index eefa34779c..c07df8369e 100644
--- a/drivers/fastboot/Kconfig
+++ b/drivers/fastboot/Kconfig
@@ -28,6 +28,13 @@ config UDP_FUNCTION_FASTBOOT_PORT
 	help
 	  The fastboot protocol requires a UDP port number.
 
+config TCP_FUNCTION_FASTBOOT
+	depends on NET
+	select FASTBOOT
+	bool "Enable fastboot protocol over TCP"
+	help
+	  This enables the fastboot protocol over TCP.
+
 if FASTBOOT
 
 config FASTBOOT_BUF_ADDR
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
index 57b6182c46..dde3cda78f 100644
--- a/drivers/fastboot/fb_common.c
+++ b/drivers/fastboot/fb_common.c
@@ -15,7 +15,6 @@
 #include <command.h>
 #include <env.h>
 #include <fastboot.h>
-#include <net/fastboot.h>
 
 /**
  * fastboot_buf_addr - base address of the fastboot download buffer
diff --git a/include/net.h b/include/net.h
index 399af5e064..63daab3731 100644
--- a/include/net.h
+++ b/include/net.h
@@ -505,7 +505,8 @@ extern int		net_restart_wrap;	/* Tried all network devices */
 
 enum proto_t {
 	BOOTP, RARP, ARP, TFTPGET, DHCP, PING, PING6, DNS, NFS, CDP, NETCONS,
-	SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP, NCSI, WGET
+	SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT_UDP, FASTBOOT_TCP, WOL,
+	UDP, NCSI, WGET
 };
 
 extern char	net_boot_file_name[1024];/* Boot File name */
diff --git a/include/net/fastboot_tcp.h b/include/net/fastboot_tcp.h
new file mode 100644
index 0000000000..6cf29d52e9
--- /dev/null
+++ b/include/net/fastboot_tcp.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (C) 2023 The Android Open Source Project
+ */
+
+#ifndef __NET_FASTBOOT_TCP_H__
+#define __NET_FASTBOOT_TCP_H__
+
+/**
+ * Wait for incoming tcp fastboot comands.
+ */
+void fastboot_tcp_start_server(void);
+
+#endif /* __NET_FASTBOOT_TCP_H__ */
diff --git a/include/net/fastboot.h b/include/net/fastboot_udp.h
similarity index 91%
rename from include/net/fastboot.h
rename to include/net/fastboot_udp.h
index 68602095d2..82145bcb9c 100644
--- a/include/net/fastboot.h
+++ b/include/net/fastboot_udp.h
@@ -14,7 +14,7 @@
 /**
  * Wait for incoming fastboot comands.
  */
-void fastboot_start_server(void);
+void fastboot_udp_start_server(void);
 
 /**********************************************************************/
 
diff --git a/net/Makefile b/net/Makefile
index bea000b206..67fc77257d 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -26,7 +26,8 @@ obj-$(CONFIG_CMD_PCAP) += pcap.o
 obj-$(CONFIG_CMD_RARP) += rarp.o
 obj-$(CONFIG_CMD_SNTP) += sntp.o
 obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
-obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot.o
+obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot_udp.o
+obj-$(CONFIG_TCP_FUNCTION_FASTBOOT)  += fastboot_tcp.o
 obj-$(CONFIG_CMD_WOL)  += wol.o
 obj-$(CONFIG_PROT_UDP) += udp.o
 obj-$(CONFIG_PROT_TCP) += tcp.o
diff --git a/net/fastboot_tcp.c b/net/fastboot_tcp.c
new file mode 100644
index 0000000000..b4bbd815c8
--- /dev/null
+++ b/net/fastboot_tcp.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright (C) 2023 The Android Open Source Project
+ */
+
+#include <common.h>
+#include <fastboot.h>
+#include <net.h>
+#include <net/fastboot_tcp.h>
+#include <net/tcp.h>
+
+static char command[FASTBOOT_COMMAND_LEN] = {0};
+static char response[FASTBOOT_RESPONSE_LEN] = {0};
+
+static const unsigned short handshake_length = 4;
+static const uchar *handshake = "FB01";
+
+static u16 curr_sport;
+static u16 curr_dport;
+static u32 curr_tcp_seq_num;
+static u32 curr_tcp_ack_num;
+static unsigned int curr_request_len;
+static enum fastboot_tcp_state {
+	FASTBOOT_CLOSED,
+	FASTBOOT_CONNECTED,
+	FASTBOOT_DISCONNECTING
+} state = FASTBOOT_CLOSED;
+
+static void fastboot_tcp_answer(u8 action, unsigned int len)
+{
+	const u32 response_seq_num = curr_tcp_ack_num;
+	const u32 response_ack_num = curr_tcp_seq_num +
+		  (curr_request_len > 0 ? curr_request_len : 1);
+
+	net_send_tcp_packet(len, htons(curr_sport), htons(curr_dport),
+			    action, response_seq_num, response_ack_num);
+}
+
+static void fastboot_tcp_reset(void)
+{
+	fastboot_tcp_answer(TCP_RST, 0);
+	state = FASTBOOT_CLOSED;
+}
+
+static void fastboot_tcp_send_packet(u8 action, const uchar *data, unsigned int len)
+{
+	uchar *pkt = net_get_async_tx_pkt_buf();
+
+	memset((void *)pkt, 0, PKTSIZE);
+	pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
+	memcpy(pkt, data, len);
+	fastboot_tcp_answer(action, len);
+	memset((void *)pkt, 0, PKTSIZE);
+}
+
+static void fastboot_tcp_send_message(const char *message, unsigned int len)
+{
+	__be64 len_be = __cpu_to_be64(len);
+	uchar *pkt = net_get_async_tx_pkt_buf();
+
+	memset((void *)pkt, 0, PKTSIZE);
+	pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
+	// Put first 8 bytes as a big endian message length
+	memcpy(pkt, &len_be, 8);
+	pkt += 8;
+	memcpy(pkt, message, len);
+	fastboot_tcp_answer(TCP_ACK | TCP_PUSH, len + 8);
+	memset((void *)pkt, 0, PKTSIZE);
+}
+
+static void fastboot_tcp_handler_ipv4(uchar *pkt, u16 dport,
+				      struct in_addr sip, u16 sport,
+				      u32 tcp_seq_num, u32 tcp_ack_num,
+				      u8 action, unsigned int len)
+{
+	u64 command_size;
+	u8 tcp_fin = action & TCP_FIN;
+	u8 tcp_push = action & TCP_PUSH;
+
+	curr_sport = sport;
+	curr_dport = dport;
+	curr_tcp_seq_num = tcp_seq_num;
+	curr_tcp_ack_num = tcp_ack_num;
+	curr_request_len = len;
+
+	switch (state) {
+	case FASTBOOT_CLOSED:
+		if (tcp_push) {
+			if (len != handshake_length ||
+			    strlen(pkt) != handshake_length ||
+			    memcmp(pkt, handshake, handshake_length) != 0) {
+				fastboot_tcp_reset();
+				break;
+			}
+			fastboot_tcp_send_packet(TCP_ACK | TCP_PUSH,
+						 handshake, handshake_length);
+			state = FASTBOOT_CONNECTED;
+		}
+		break;
+	case FASTBOOT_CONNECTED:
+		if (tcp_fin) {
+			fastboot_tcp_answer(TCP_FIN | TCP_ACK, 0);
+			state = FASTBOOT_DISCONNECTING;
+			break;
+		}
+		if (tcp_push) {
+			// First 8 bytes is big endian message length
+			command_size = __be64_to_cpu(*(u64 *)pkt);
+			len -= 8;
+			pkt += 8;
+
+			// Only single packet messages are supported ATM
+			if (strlen(pkt) != command_size) {
+				fastboot_tcp_reset();
+				break;
+			}
+			strlcpy(command, pkt, len + 1);
+			fastboot_handle_command(command, response);
+			fastboot_tcp_send_message(response, strlen(response));
+		}
+		break;
+	case FASTBOOT_DISCONNECTING:
+		if (tcp_push)
+			state = FASTBOOT_CLOSED;
+		break;
+	}
+
+	memset(command, 0, FASTBOOT_COMMAND_LEN);
+	memset(response, 0, FASTBOOT_RESPONSE_LEN);
+	curr_sport = 0;
+	curr_dport = 0;
+	curr_tcp_seq_num = 0;
+	curr_tcp_ack_num = 0;
+	curr_request_len = 0;
+}
+
+void fastboot_tcp_start_server(void)
+{
+	printf("Using %s device\n", eth_get_name());
+	printf("Listening for fastboot command on tcp %pI4\n", &net_ip);
+
+	tcp_set_tcp_handler(fastboot_tcp_handler_ipv4);
+}
diff --git a/net/fastboot.c b/net/fastboot_udp.c
similarity index 99%
rename from net/fastboot.c
rename to net/fastboot_udp.c
index e9569d88d2..27e779d8e0 100644
--- a/net/fastboot.c
+++ b/net/fastboot_udp.c
@@ -7,7 +7,7 @@
 #include <command.h>
 #include <fastboot.h>
 #include <net.h>
-#include <net/fastboot.h>
+#include <net/fastboot_udp.h>
 
 enum {
 	FASTBOOT_ERROR = 0,
@@ -300,7 +300,7 @@ static void fastboot_handler(uchar *packet, unsigned int dport,
 	}
 }
 
-void fastboot_start_server(void)
+void fastboot_udp_start_server(void)
 {
 	printf("Using %s device\n", eth_get_name());
 	printf("Listening for fastboot command on %pI4\n", &net_ip);
diff --git a/net/net.c b/net/net.c
index c9a749f6cc..41c3cac26e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -93,7 +93,8 @@
 #include <net.h>
 #include <net6.h>
 #include <ndisc.h>
-#include <net/fastboot.h>
+#include <net/fastboot_udp.h>
+#include <net/fastboot_tcp.h>
 #include <net/tftp.h>
 #include <net/ncsi.h>
 #if defined(CONFIG_CMD_PCAP)
@@ -498,9 +499,14 @@ restart:
 			tftp_start_server();
 			break;
 #endif
-#ifdef CONFIG_UDP_FUNCTION_FASTBOOT
-		case FASTBOOT:
-			fastboot_start_server();
+#if defined(CONFIG_UDP_FUNCTION_FASTBOOT)
+		case FASTBOOT_UDP:
+			fastboot_udp_start_server();
+			break;
+#endif
+#if defined(CONFIG_TCP_FUNCTION_FASTBOOT)
+		case FASTBOOT_TCP:
+			fastboot_tcp_start_server();
 			break;
 #endif
 #if defined(CONFIG_CMD_DHCP)
@@ -1491,7 +1497,8 @@ common:
 		/* Fall through */
 
 	case NETCONS:
-	case FASTBOOT:
+	case FASTBOOT_UDP:
+	case FASTBOOT_TCP:
 	case TFTPSRV:
 		if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
 			if (!memcmp(&net_link_local_ip6, &net_null_addr_ip6,
-- 
2.40.0.348.gf938b09366-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] net: share fastboot boot handle logic between transports
  2023-03-28 20:31 [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Dmitrii Merkurev
  2023-03-28 20:31 ` [PATCH 2/3] net: add fastboot TCP support Dmitrii Merkurev
@ 2023-03-28 20:31 ` Dmitrii Merkurev
  2023-04-01  6:32   ` Simon Glass
  2023-03-29  9:08 ` [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Paul Liu
  2023-04-01  6:32 ` Simon Glass
  3 siblings, 1 reply; 10+ messages in thread
From: Dmitrii Merkurev @ 2023-03-28 20:31 UTC (permalink / raw)
  To: u-boot; +Cc: Dmitrii Merkurev, Ying-Chun Liu, Simon Glass

Introduce reboot, boot and continue commands support to
TCP fastboot by moving existing UDP logic into the common module.

Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Simon Glass <sjg@chromium.org>
Сс: Joe Hershberger <joe.hershberger@ni.com>
Сс: Ramon Fried <rfried.dev@gmail.com>
Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
---

 drivers/fastboot/fb_common.c | 32 ++++++++++++++++++++++++++++++++
 include/fastboot.h           |  9 +++++++++
 net/fastboot_tcp.c           |  5 ++++-
 net/fastboot_udp.c           | 29 +----------------------------
 4 files changed, 46 insertions(+), 29 deletions(-)

diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
index dde3cda78f..621146bc6b 100644
--- a/drivers/fastboot/fb_common.c
+++ b/drivers/fastboot/fb_common.c
@@ -15,6 +15,7 @@
 #include <command.h>
 #include <env.h>
 #include <fastboot.h>
+#include <net.h>
 
 /**
  * fastboot_buf_addr - base address of the fastboot download buffer
@@ -155,6 +156,37 @@ void fastboot_boot(void)
 	}
 }
 
+/**
+ * fastboot_handle_boot() - Shared implementation of system reaction to
+ * fastboot commands
+ *
+ * Making desceisions about device boot state (stay in fastboot, reboot
+ * to bootloader, reboot to OS, etc).
+ */
+void fastboot_handle_boot(int command, bool success)
+{
+	if (!success)
+		return;
+
+	switch (command) {
+	case FASTBOOT_COMMAND_BOOT:
+		fastboot_boot();
+		net_set_state(NETLOOP_SUCCESS);
+		break;
+
+	case FASTBOOT_COMMAND_CONTINUE:
+		net_set_state(NETLOOP_SUCCESS);
+		break;
+
+	case FASTBOOT_COMMAND_REBOOT:
+	case FASTBOOT_COMMAND_REBOOT_BOOTLOADER:
+	case FASTBOOT_COMMAND_REBOOT_FASTBOOTD:
+	case FASTBOOT_COMMAND_REBOOT_RECOVERY:
+		do_reset(NULL, 0, 0, NULL);
+		break;
+	}
+}
+
 /**
  * fastboot_set_progress_callback() - set progress callback
  *
diff --git a/include/fastboot.h b/include/fastboot.h
index 07f4c8fa71..296451f89d 100644
--- a/include/fastboot.h
+++ b/include/fastboot.h
@@ -123,6 +123,15 @@ void fastboot_init(void *buf_addr, u32 buf_size);
  */
 void fastboot_boot(void);
 
+/**
+ * fastboot_handle_boot() - Shared implementation of system reaction to
+ * fastboot commands
+ *
+ * Making desceisions about device boot state (stay in fastboot, reboot
+ * to bootloader, reboot to OS, etc).
+ */
+void fastboot_handle_boot(int command, bool success);
+
 /**
  * fastboot_handle_command() - Handle fastboot command
  *
diff --git a/net/fastboot_tcp.c b/net/fastboot_tcp.c
index b4bbd815c8..f3f087db81 100644
--- a/net/fastboot_tcp.c
+++ b/net/fastboot_tcp.c
@@ -73,6 +73,7 @@ static void fastboot_tcp_handler_ipv4(uchar *pkt, u16 dport,
 				      u32 tcp_seq_num, u32 tcp_ack_num,
 				      u8 action, unsigned int len)
 {
+	int fastboot_command_id;
 	u64 command_size;
 	u8 tcp_fin = action & TCP_FIN;
 	u8 tcp_push = action & TCP_PUSH;
@@ -115,8 +116,10 @@ static void fastboot_tcp_handler_ipv4(uchar *pkt, u16 dport,
 				break;
 			}
 			strlcpy(command, pkt, len + 1);
-			fastboot_handle_command(command, response);
+			fastboot_command_id = fastboot_handle_command(command, response);
 			fastboot_tcp_send_message(response, strlen(response));
+			fastboot_handle_boot(fastboot_command_id,
+					     strncmp("OKAY", response, 4) == 0);
 		}
 		break;
 	case FASTBOOT_DISCONNECTING:
diff --git a/net/fastboot_udp.c b/net/fastboot_udp.c
index 27e779d8e0..389a5b45f5 100644
--- a/net/fastboot_udp.c
+++ b/net/fastboot_udp.c
@@ -209,39 +209,12 @@ static void fastboot_send(struct fastboot_header header, char *fastboot_data,
 	net_send_udp_packet(net_server_ethaddr, fastboot_remote_ip,
 			    fastboot_remote_port, fastboot_our_port, len);
 
-	/* Continue boot process after sending response */
-	if (!strncmp("OKAY", response, 4)) {
-		switch (cmd) {
-		case FASTBOOT_COMMAND_BOOT:
-			boot_downloaded_image();
-			break;
-
-		case FASTBOOT_COMMAND_CONTINUE:
-			net_set_state(NETLOOP_SUCCESS);
-			break;
-
-		case FASTBOOT_COMMAND_REBOOT:
-		case FASTBOOT_COMMAND_REBOOT_BOOTLOADER:
-		case FASTBOOT_COMMAND_REBOOT_FASTBOOTD:
-		case FASTBOOT_COMMAND_REBOOT_RECOVERY:
-			do_reset(NULL, 0, 0, NULL);
-			break;
-		}
-	}
+	fastboot_handle_boot(cmd, strncmp("OKAY", response, 4) == 0);
 
 	if (!strncmp("OKAY", response, 4) || !strncmp("FAIL", response, 4))
 		cmd = -1;
 }
 
-/**
- * boot_downloaded_image() - Boots into downloaded image.
- */
-static void boot_downloaded_image(void)
-{
-	fastboot_boot();
-	net_set_state(NETLOOP_SUCCESS);
-}
-
 /**
  * fastboot_handler() - Incoming UDP packet handler.
  *
-- 
2.40.0.348.gf938b09366-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot
  2023-03-28 20:31 [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Dmitrii Merkurev
  2023-03-28 20:31 ` [PATCH 2/3] net: add fastboot TCP support Dmitrii Merkurev
  2023-03-28 20:31 ` [PATCH 3/3] net: share fastboot boot handle logic between transports Dmitrii Merkurev
@ 2023-03-29  9:08 ` Paul Liu
  2023-04-01  6:32 ` Simon Glass
  3 siblings, 0 replies; 10+ messages in thread
From: Paul Liu @ 2023-03-29  9:08 UTC (permalink / raw)
  To: Dmitrii Merkurev; +Cc: u-boot, Simon Glass

On Wed, 29 Mar 2023 at 04:31, Dmitrii Merkurev <dimorinny@google.com> wrote:

> Make following changes to unblock TCP fastboot support:
>
> 1. Implement being a TCP server support
> 2. Introduce dedicated TCP traffic handler (get rid of UDP signature)
> 3. Ensure seq_num and ack_num are respected in net_send_tcp_packet
> function (make sure existing wget_cmd code is reflected with the fix)
>
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Сс: Joe Hershberger <joe.hershberger@ni.com>
> Сс: Ramon Fried <rfried.dev@gmail.com>
> ---


Reviewed-by: Ying-Chun Liu (PaulLiu)

I've built it and ran the unit test of the TCP stack. The code looks good
to me.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot
  2023-03-28 20:31 [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Dmitrii Merkurev
                   ` (2 preceding siblings ...)
  2023-03-29  9:08 ` [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Paul Liu
@ 2023-04-01  6:32 ` Simon Glass
  2023-04-01 18:54   ` Ramon Fried
  3 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2023-04-01  6:32 UTC (permalink / raw)
  To: Dmitrii Merkurev; +Cc: u-boot, Ying-Chun Liu

On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
>
> Make following changes to unblock TCP fastboot support:
>
> 1. Implement being a TCP server support
> 2. Introduce dedicated TCP traffic handler (get rid of UDP signature)
> 3. Ensure seq_num and ack_num are respected in net_send_tcp_packet
> function (make sure existing wget_cmd code is reflected with the fix)
>
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Сс: Joe Hershberger <joe.hershberger@ni.com>
> Сс: Ramon Fried <rfried.dev@gmail.com>
> ---
>
>  include/net/tcp.h |  16 +++++--
>  net/tcp.c         | 115 +++++++++++++++++++++++-----------------------
>  net/wget.c        |  43 ++++++++---------
>  3 files changed, 90 insertions(+), 84 deletions(-)
>
Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: add fastboot TCP support
  2023-03-28 20:31 ` [PATCH 2/3] net: add fastboot TCP support Dmitrii Merkurev
@ 2023-04-01  6:32   ` Simon Glass
  2023-04-12 18:55     ` Dmitrii Merkurev
  0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2023-04-01  6:32 UTC (permalink / raw)
  To: Dmitrii Merkurev; +Cc: u-boot, Ying-Chun Liu

Hi Dmitrii,

On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
>
> Known limitations are
> 1. fastboot reboot doesn't work (answering OK but not rebooting)
> 2. flashing isn't supported (TCP transport only limitation)
>
> The command syntax is
> fastboot tcp
>
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Сс: Joe Hershberger <joe.hershberger@ni.com>
> Сс: Ramon Fried <rfried.dev@gmail.com>
> ---
>
>  MAINTAINERS                                |   6 +-
>  cmd/fastboot.c                             |  25 +++-
>  drivers/fastboot/Kconfig                   |   7 +
>  drivers/fastboot/fb_common.c               |   1 -
>  include/net.h                              |   3 +-
>  include/net/fastboot_tcp.h                 |  14 ++
>  include/net/{fastboot.h => fastboot_udp.h} |   2 +-
>  net/Makefile                               |   3 +-
>  net/fastboot_tcp.c                         | 143 +++++++++++++++++++++
>  net/{fastboot.c => fastboot_udp.c}         |   4 +-
>  net/net.c                                  |  17 ++-
>  11 files changed, 210 insertions(+), 15 deletions(-)
>  create mode 100644 include/net/fastboot_tcp.h
>  rename include/net/{fastboot.h => fastboot_udp.h} (91%)
>  create mode 100644 net/fastboot_tcp.c
>  rename net/{fastboot.c => fastboot_udp.c} (99%)

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

nits and a question below

>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 91d40ea4b6..501d1147d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -978,10 +978,12 @@ F:        cmd/fastboot.c
>  F:     doc/android/fastboot*.rst
>  F:     include/fastboot.h
>  F:     include/fastboot-internal.h
> -F:     include/net/fastboot.h
> +F:     include/net/fastboot_tcp.h
> +F:     include/net/fastboot_udp.h
>  F:     drivers/fastboot/
>  F:     drivers/usb/gadget/f_fastboot.c
> -F:     net/fastboot.c
> +F:     net/fastboot_tcp.c
> +F:     net/fastboot_udp.c
>  F:     test/dm/fastboot.c
>
>  FPGA
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index 97dc02ce74..3d5ff951eb 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -26,7 +26,7 @@ static int do_fastboot_udp(int argc, char *const argv[],
>                 return CMD_RET_FAILURE;
>         }
>
> -       err = net_loop(FASTBOOT);
> +       err = net_loop(FASTBOOT_UDP);
>
>         if (err < 0) {
>                 printf("fastboot udp error: %d\n", err);
> @@ -36,6 +36,26 @@ static int do_fastboot_udp(int argc, char *const argv[],
>         return CMD_RET_SUCCESS;
>  }
>
> +static int do_fastboot_tcp(int argc, char *const argv[],
> +                          uintptr_t buf_addr, size_t buf_size)
> +{
> +       int err;
> +
> +       if (!IS_ENABLED(CONFIG_TCP_FUNCTION_FASTBOOT)) {
> +               pr_err("Fastboot TCP not enabled\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       err = net_loop(FASTBOOT_TCP);
> +
> +       if (err < 0) {
> +               printf("fastboot tcp error: %d\n", err);
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
>  static int do_fastboot_usb(int argc, char *const argv[],
>                            uintptr_t buf_addr, size_t buf_size)
>  {
> @@ -141,7 +161,8 @@ NXTARG:
>
>         if (!strcmp(argv[1], "udp"))
>                 return do_fastboot_udp(argc, argv, buf_addr, buf_size);
> -
> +       if (!strcmp(argv[1], "tcp"))
> +               return do_fastboot_tcp(argc, argv, buf_addr, buf_size);
>         if (!strcmp(argv[1], "usb")) {
>                 argv++;
>                 argc--;
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index eefa34779c..c07df8369e 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -28,6 +28,13 @@ config UDP_FUNCTION_FASTBOOT_PORT
>         help
>           The fastboot protocol requires a UDP port number.
>
> +config TCP_FUNCTION_FASTBOOT
> +       depends on NET
> +       select FASTBOOT
> +       bool "Enable fastboot protocol over TCP"
> +       help
> +         This enables the fastboot protocol over TCP.

Please can you add some more help, like a link to the protocol and
what it is used for?

> +
>  if FASTBOOT
>
>  config FASTBOOT_BUF_ADDR
> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> index 57b6182c46..dde3cda78f 100644
> --- a/drivers/fastboot/fb_common.c
> +++ b/drivers/fastboot/fb_common.c
> @@ -15,7 +15,6 @@
>  #include <command.h>
>  #include <env.h>
>  #include <fastboot.h>
> -#include <net/fastboot.h>
>
>  /**
>   * fastboot_buf_addr - base address of the fastboot download buffer
> diff --git a/include/net.h b/include/net.h
> index 399af5e064..63daab3731 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -505,7 +505,8 @@ extern int          net_restart_wrap;       /* Tried all network devices */
>
>  enum proto_t {
>         BOOTP, RARP, ARP, TFTPGET, DHCP, PING, PING6, DNS, NFS, CDP, NETCONS,
> -       SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP, NCSI, WGET
> +       SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT_UDP, FASTBOOT_TCP, WOL,
> +       UDP, NCSI, WGET
>  };
>
>  extern char    net_boot_file_name[1024];/* Boot File name */
> diff --git a/include/net/fastboot_tcp.h b/include/net/fastboot_tcp.h
> new file mode 100644
> index 0000000000..6cf29d52e9
> --- /dev/null
> +++ b/include/net/fastboot_tcp.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Copyright (C) 2023 The Android Open Source Project
> + */
> +
> +#ifndef __NET_FASTBOOT_TCP_H__
> +#define __NET_FASTBOOT_TCP_H__
> +
> +/**
> + * Wait for incoming tcp fastboot comands.
> + */
> +void fastboot_tcp_start_server(void);
> +
> +#endif /* __NET_FASTBOOT_TCP_H__ */
> diff --git a/include/net/fastboot.h b/include/net/fastboot_udp.h
> similarity index 91%
> rename from include/net/fastboot.h
> rename to include/net/fastboot_udp.h
> index 68602095d2..82145bcb9c 100644
> --- a/include/net/fastboot.h
> +++ b/include/net/fastboot_udp.h
> @@ -14,7 +14,7 @@
>  /**
>   * Wait for incoming fastboot comands.
>   */
> -void fastboot_start_server(void);
> +void fastboot_udp_start_server(void);

Please add a comment

>
>  /**********************************************************************/
>
> diff --git a/net/Makefile b/net/Makefile
> index bea000b206..67fc77257d 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -26,7 +26,8 @@ obj-$(CONFIG_CMD_PCAP) += pcap.o
>  obj-$(CONFIG_CMD_RARP) += rarp.o
>  obj-$(CONFIG_CMD_SNTP) += sntp.o
>  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> -obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot.o
> +obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot_udp.o
> +obj-$(CONFIG_TCP_FUNCTION_FASTBOOT)  += fastboot_tcp.o
>  obj-$(CONFIG_CMD_WOL)  += wol.o
>  obj-$(CONFIG_PROT_UDP) += udp.o
>  obj-$(CONFIG_PROT_TCP) += tcp.o
> diff --git a/net/fastboot_tcp.c b/net/fastboot_tcp.c
> new file mode 100644
> index 0000000000..b4bbd815c8
> --- /dev/null
> +++ b/net/fastboot_tcp.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * Copyright (C) 2023 The Android Open Source Project
> + */
> +
> +#include <common.h>
> +#include <fastboot.h>
> +#include <net.h>
> +#include <net/fastboot_tcp.h>
> +#include <net/tcp.h>
> +
> +static char command[FASTBOOT_COMMAND_LEN] = {0};
> +static char response[FASTBOOT_RESPONSE_LEN] = {0};
> +
> +static const unsigned short handshake_length = 4;
> +static const uchar *handshake = "FB01";
> +
> +static u16 curr_sport;
> +static u16 curr_dport;
> +static u32 curr_tcp_seq_num;
> +static u32 curr_tcp_ack_num;
> +static unsigned int curr_request_len;
> +static enum fastboot_tcp_state {
> +       FASTBOOT_CLOSED,
> +       FASTBOOT_CONNECTED,
> +       FASTBOOT_DISCONNECTING
> +} state = FASTBOOT_CLOSED;
> +
> +static void fastboot_tcp_answer(u8 action, unsigned int len)
> +{
> +       const u32 response_seq_num = curr_tcp_ack_num;
> +       const u32 response_ack_num = curr_tcp_seq_num +
> +                 (curr_request_len > 0 ? curr_request_len : 1);
> +
> +       net_send_tcp_packet(len, htons(curr_sport), htons(curr_dport),
> +                           action, response_seq_num, response_ack_num);
> +}
> +
> +static void fastboot_tcp_reset(void)
> +{
> +       fastboot_tcp_answer(TCP_RST, 0);
> +       state = FASTBOOT_CLOSED;
> +}
> +
> +static void fastboot_tcp_send_packet(u8 action, const uchar *data, unsigned int len)
> +{
> +       uchar *pkt = net_get_async_tx_pkt_buf();
> +
> +       memset((void *)pkt, 0, PKTSIZE);

'\0'

Can you drop cast on these?

> +       pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
> +       memcpy(pkt, data, len);
> +       fastboot_tcp_answer(action, len);
> +       memset((void *)pkt, 0, PKTSIZE);
> +}
> +
> +static void fastboot_tcp_send_message(const char *message, unsigned int len)
> +{
> +       __be64 len_be = __cpu_to_be64(len);
> +       uchar *pkt = net_get_async_tx_pkt_buf();
> +
> +       memset((void *)pkt, 0, PKTSIZE);
> +       pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
> +       // Put first 8 bytes as a big endian message length
> +       memcpy(pkt, &len_be, 8);
> +       pkt += 8;
> +       memcpy(pkt, message, len);
> +       fastboot_tcp_answer(TCP_ACK | TCP_PUSH, len + 8);
> +       memset((void *)pkt, 0, PKTSIZE);
> +}
> +
> +static void fastboot_tcp_handler_ipv4(uchar *pkt, u16 dport,
> +                                     struct in_addr sip, u16 sport,
> +                                     u32 tcp_seq_num, u32 tcp_ack_num,
> +                                     u8 action, unsigned int len)
> +{
> +       u64 command_size;

Wow, that can really be that big? Or are you using u64 just because
that is the size of the field?

> +       u8 tcp_fin = action & TCP_FIN;
> +       u8 tcp_push = action & TCP_PUSH;
> +
> +       curr_sport = sport;
> +       curr_dport = dport;
> +       curr_tcp_seq_num = tcp_seq_num;
> +       curr_tcp_ack_num = tcp_ack_num;
> +       curr_request_len = len;
> +
> +       switch (state) {
> +       case FASTBOOT_CLOSED:
> +               if (tcp_push) {
> +                       if (len != handshake_length ||
> +                           strlen(pkt) != handshake_length ||
> +                           memcmp(pkt, handshake, handshake_length) != 0) {
> +                               fastboot_tcp_reset();
> +                               break;
> +                       }
> +                       fastboot_tcp_send_packet(TCP_ACK | TCP_PUSH,
> +                                                handshake, handshake_length);
> +                       state = FASTBOOT_CONNECTED;
> +               }
> +               break;
> +       case FASTBOOT_CONNECTED:
> +               if (tcp_fin) {
> +                       fastboot_tcp_answer(TCP_FIN | TCP_ACK, 0);
> +                       state = FASTBOOT_DISCONNECTING;
> +                       break;
> +               }
> +               if (tcp_push) {
> +                       // First 8 bytes is big endian message length
> +                       command_size = __be64_to_cpu(*(u64 *)pkt);
> +                       len -= 8;
> +                       pkt += 8;
> +
> +                       // Only single packet messages are supported ATM
> +                       if (strlen(pkt) != command_size) {
> +                               fastboot_tcp_reset();
> +                               break;
> +                       }
> +                       strlcpy(command, pkt, len + 1);
> +                       fastboot_handle_command(command, response);
> +                       fastboot_tcp_send_message(response, strlen(response));
> +               }
> +               break;
> +       case FASTBOOT_DISCONNECTING:
> +               if (tcp_push)
> +                       state = FASTBOOT_CLOSED;
> +               break;
> +       }
> +
> +       memset(command, 0, FASTBOOT_COMMAND_LEN);
> +       memset(response, 0, FASTBOOT_RESPONSE_LEN);
> +       curr_sport = 0;
> +       curr_dport = 0;
> +       curr_tcp_seq_num = 0;
> +       curr_tcp_ack_num = 0;
> +       curr_request_len = 0;
> +}
> +
> +void fastboot_tcp_start_server(void)
> +{
> +       printf("Using %s device\n", eth_get_name());
> +       printf("Listening for fastboot command on tcp %pI4\n", &net_ip);
> +
> +       tcp_set_tcp_handler(fastboot_tcp_handler_ipv4);
> +}
> diff --git a/net/fastboot.c b/net/fastboot_udp.c
> similarity index 99%
> rename from net/fastboot.c
> rename to net/fastboot_udp.c
> index e9569d88d2..27e779d8e0 100644
> --- a/net/fastboot.c
> +++ b/net/fastboot_udp.c
> @@ -7,7 +7,7 @@
>  #include <command.h>
>  #include <fastboot.h>
>  #include <net.h>
> -#include <net/fastboot.h>
> +#include <net/fastboot_udp.h>
>
>  enum {
>         FASTBOOT_ERROR = 0,
> @@ -300,7 +300,7 @@ static void fastboot_handler(uchar *packet, unsigned int dport,
>         }
>  }
>
> -void fastboot_start_server(void)
> +void fastboot_udp_start_server(void)
>  {
>         printf("Using %s device\n", eth_get_name());
>         printf("Listening for fastboot command on %pI4\n", &net_ip);
> diff --git a/net/net.c b/net/net.c
> index c9a749f6cc..41c3cac26e 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -93,7 +93,8 @@
>  #include <net.h>
>  #include <net6.h>
>  #include <ndisc.h>
> -#include <net/fastboot.h>
> +#include <net/fastboot_udp.h>
> +#include <net/fastboot_tcp.h>
>  #include <net/tftp.h>
>  #include <net/ncsi.h>
>  #if defined(CONFIG_CMD_PCAP)
> @@ -498,9 +499,14 @@ restart:
>                         tftp_start_server();
>                         break;
>  #endif
> -#ifdef CONFIG_UDP_FUNCTION_FASTBOOT
> -               case FASTBOOT:
> -                       fastboot_start_server();
> +#if defined(CONFIG_UDP_FUNCTION_FASTBOOT)
> +               case FASTBOOT_UDP:
> +                       fastboot_udp_start_server();
> +                       break;
> +#endif
> +#if defined(CONFIG_TCP_FUNCTION_FASTBOOT)
> +               case FASTBOOT_TCP:
> +                       fastboot_tcp_start_server();
>                         break;
>  #endif
>  #if defined(CONFIG_CMD_DHCP)
> @@ -1491,7 +1497,8 @@ common:
>                 /* Fall through */
>
>         case NETCONS:
> -       case FASTBOOT:
> +       case FASTBOOT_UDP:
> +       case FASTBOOT_TCP:
>         case TFTPSRV:
>                 if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
>                         if (!memcmp(&net_link_local_ip6, &net_null_addr_ip6,
> --
> 2.40.0.348.gf938b09366-goog
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] net: share fastboot boot handle logic between transports
  2023-03-28 20:31 ` [PATCH 3/3] net: share fastboot boot handle logic between transports Dmitrii Merkurev
@ 2023-04-01  6:32   ` Simon Glass
  2023-04-01 18:55     ` Ramon Fried
  0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2023-04-01  6:32 UTC (permalink / raw)
  To: Dmitrii Merkurev; +Cc: u-boot, Ying-Chun Liu

On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
>
> Introduce reboot, boot and continue commands support to
> TCP fastboot by moving existing UDP logic into the common module.
>
> Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Simon Glass <sjg@chromium.org>
> Сс: Joe Hershberger <joe.hershberger@ni.com>
> Сс: Ramon Fried <rfried.dev@gmail.com>
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> ---
>
>  drivers/fastboot/fb_common.c | 32 ++++++++++++++++++++++++++++++++
>  include/fastboot.h           |  9 +++++++++
>  net/fastboot_tcp.c           |  5 ++++-
>  net/fastboot_udp.c           | 29 +----------------------------
>  4 files changed, 46 insertions(+), 29 deletions(-)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot
  2023-04-01  6:32 ` Simon Glass
@ 2023-04-01 18:54   ` Ramon Fried
  0 siblings, 0 replies; 10+ messages in thread
From: Ramon Fried @ 2023-04-01 18:54 UTC (permalink / raw)
  To: Simon Glass; +Cc: Dmitrii Merkurev, u-boot, Ying-Chun Liu

On Sat, Apr 1, 2023 at 9:36 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
> >
> > Make following changes to unblock TCP fastboot support:
> >
> > 1. Implement being a TCP server support
> > 2. Introduce dedicated TCP traffic handler (get rid of UDP signature)
> > 3. Ensure seq_num and ack_num are respected in net_send_tcp_packet
> > function (make sure existing wget_cmd code is reflected with the fix)
> >
> > Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> > Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > Cc: Simon Glass <sjg@chromium.org>
> > Сс: Joe Hershberger <joe.hershberger@ni.com>
> > Сс: Ramon Fried <rfried.dev@gmail.com>
> > ---
> >
> >  include/net/tcp.h |  16 +++++--
> >  net/tcp.c         | 115 +++++++++++++++++++++++-----------------------
> >  net/wget.c        |  43 ++++++++---------
> >  3 files changed, 90 insertions(+), 84 deletions(-)
> >
> Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] net: share fastboot boot handle logic between transports
  2023-04-01  6:32   ` Simon Glass
@ 2023-04-01 18:55     ` Ramon Fried
  0 siblings, 0 replies; 10+ messages in thread
From: Ramon Fried @ 2023-04-01 18:55 UTC (permalink / raw)
  To: Simon Glass; +Cc: Dmitrii Merkurev, u-boot, Ying-Chun Liu

On Sat, Apr 1, 2023 at 9:36 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
> >
> > Introduce reboot, boot and continue commands support to
> > TCP fastboot by moving existing UDP logic into the common module.
> >
> > Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > Cc: Simon Glass <sjg@chromium.org>
> > Сс: Joe Hershberger <joe.hershberger@ni.com>
> > Сс: Ramon Fried <rfried.dev@gmail.com>
> > Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> > ---
> >
> >  drivers/fastboot/fb_common.c | 32 ++++++++++++++++++++++++++++++++
> >  include/fastboot.h           |  9 +++++++++
> >  net/fastboot_tcp.c           |  5 ++++-
> >  net/fastboot_udp.c           | 29 +----------------------------
> >  4 files changed, 46 insertions(+), 29 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] net: add fastboot TCP support
  2023-04-01  6:32   ` Simon Glass
@ 2023-04-12 18:55     ` Dmitrii Merkurev
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitrii Merkurev @ 2023-04-12 18:55 UTC (permalink / raw)
  To: Simon Glass; +Cc: u-boot, Ying-Chun Liu

Thank you for the comments. Fixed everything you mentioned in uploaded v2.

> Wow, that can really be that big? Or are you using u64 just because
> that is the size of the field?

I don't think any fastboot message can be that big. You're right,
using u64 to fit the field. Here is more information about packet
format https://chromium.googlesource.com/aosp/platform/system/core/+/refs/heads/upstream/fastboot/#fastboot-data
On Sat, Apr 1, 2023 at 7:32 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Dmitrii,
>
> On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimorinny@google.com> wrote:
> >
> > Known limitations are
> > 1. fastboot reboot doesn't work (answering OK but not rebooting)
> > 2. flashing isn't supported (TCP transport only limitation)
> >
> > The command syntax is
> > fastboot tcp
> >
> > Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> > Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > Cc: Simon Glass <sjg@chromium.org>
> > Сс: Joe Hershberger <joe.hershberger@ni.com>
> > Сс: Ramon Fried <rfried.dev@gmail.com>
> > ---
> >
> >  MAINTAINERS                                |   6 +-
> >  cmd/fastboot.c                             |  25 +++-
> >  drivers/fastboot/Kconfig                   |   7 +
> >  drivers/fastboot/fb_common.c               |   1 -
> >  include/net.h                              |   3 +-
> >  include/net/fastboot_tcp.h                 |  14 ++
> >  include/net/{fastboot.h => fastboot_udp.h} |   2 +-
> >  net/Makefile                               |   3 +-
> >  net/fastboot_tcp.c                         | 143 +++++++++++++++++++++
> >  net/{fastboot.c => fastboot_udp.c}         |   4 +-
> >  net/net.c                                  |  17 ++-
> >  11 files changed, 210 insertions(+), 15 deletions(-)
> >  create mode 100644 include/net/fastboot_tcp.h
> >  rename include/net/{fastboot.h => fastboot_udp.h} (91%)
> >  create mode 100644 net/fastboot_tcp.c
> >  rename net/{fastboot.c => fastboot_udp.c} (99%)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> nits and a question below
>
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 91d40ea4b6..501d1147d9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -978,10 +978,12 @@ F:        cmd/fastboot.c
> >  F:     doc/android/fastboot*.rst
> >  F:     include/fastboot.h
> >  F:     include/fastboot-internal.h
> > -F:     include/net/fastboot.h
> > +F:     include/net/fastboot_tcp.h
> > +F:     include/net/fastboot_udp.h
> >  F:     drivers/fastboot/
> >  F:     drivers/usb/gadget/f_fastboot.c
> > -F:     net/fastboot.c
> > +F:     net/fastboot_tcp.c
> > +F:     net/fastboot_udp.c
> >  F:     test/dm/fastboot.c
> >
> >  FPGA
> > diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> > index 97dc02ce74..3d5ff951eb 100644
> > --- a/cmd/fastboot.c
> > +++ b/cmd/fastboot.c
> > @@ -26,7 +26,7 @@ static int do_fastboot_udp(int argc, char *const argv[],
> >                 return CMD_RET_FAILURE;
> >         }
> >
> > -       err = net_loop(FASTBOOT);
> > +       err = net_loop(FASTBOOT_UDP);
> >
> >         if (err < 0) {
> >                 printf("fastboot udp error: %d\n", err);
> > @@ -36,6 +36,26 @@ static int do_fastboot_udp(int argc, char *const argv[],
> >         return CMD_RET_SUCCESS;
> >  }
> >
> > +static int do_fastboot_tcp(int argc, char *const argv[],
> > +                          uintptr_t buf_addr, size_t buf_size)
> > +{
> > +       int err;
> > +
> > +       if (!IS_ENABLED(CONFIG_TCP_FUNCTION_FASTBOOT)) {
> > +               pr_err("Fastboot TCP not enabled\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       err = net_loop(FASTBOOT_TCP);
> > +
> > +       if (err < 0) {
> > +               printf("fastboot tcp error: %d\n", err);
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> >  static int do_fastboot_usb(int argc, char *const argv[],
> >                            uintptr_t buf_addr, size_t buf_size)
> >  {
> > @@ -141,7 +161,8 @@ NXTARG:
> >
> >         if (!strcmp(argv[1], "udp"))
> >                 return do_fastboot_udp(argc, argv, buf_addr, buf_size);
> > -
> > +       if (!strcmp(argv[1], "tcp"))
> > +               return do_fastboot_tcp(argc, argv, buf_addr, buf_size);
> >         if (!strcmp(argv[1], "usb")) {
> >                 argv++;
> >                 argc--;
> > diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> > index eefa34779c..c07df8369e 100644
> > --- a/drivers/fastboot/Kconfig
> > +++ b/drivers/fastboot/Kconfig
> > @@ -28,6 +28,13 @@ config UDP_FUNCTION_FASTBOOT_PORT
> >         help
> >           The fastboot protocol requires a UDP port number.
> >
> > +config TCP_FUNCTION_FASTBOOT
> > +       depends on NET
> > +       select FASTBOOT
> > +       bool "Enable fastboot protocol over TCP"
> > +       help
> > +         This enables the fastboot protocol over TCP.
>
> Please can you add some more help, like a link to the protocol and
> what it is used for?
>
> > +
> >  if FASTBOOT
> >
> >  config FASTBOOT_BUF_ADDR
> > diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> > index 57b6182c46..dde3cda78f 100644
> > --- a/drivers/fastboot/fb_common.c
> > +++ b/drivers/fastboot/fb_common.c
> > @@ -15,7 +15,6 @@
> >  #include <command.h>
> >  #include <env.h>
> >  #include <fastboot.h>
> > -#include <net/fastboot.h>
> >
> >  /**
> >   * fastboot_buf_addr - base address of the fastboot download buffer
> > diff --git a/include/net.h b/include/net.h
> > index 399af5e064..63daab3731 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -505,7 +505,8 @@ extern int          net_restart_wrap;       /* Tried all network devices */
> >
> >  enum proto_t {
> >         BOOTP, RARP, ARP, TFTPGET, DHCP, PING, PING6, DNS, NFS, CDP, NETCONS,
> > -       SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT, WOL, UDP, NCSI, WGET
> > +       SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT_UDP, FASTBOOT_TCP, WOL,
> > +       UDP, NCSI, WGET
> >  };
> >
> >  extern char    net_boot_file_name[1024];/* Boot File name */
> > diff --git a/include/net/fastboot_tcp.h b/include/net/fastboot_tcp.h
> > new file mode 100644
> > index 0000000000..6cf29d52e9
> > --- /dev/null
> > +++ b/include/net/fastboot_tcp.h
> > @@ -0,0 +1,14 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (C) 2023 The Android Open Source Project
> > + */
> > +
> > +#ifndef __NET_FASTBOOT_TCP_H__
> > +#define __NET_FASTBOOT_TCP_H__
> > +
> > +/**
> > + * Wait for incoming tcp fastboot comands.
> > + */
> > +void fastboot_tcp_start_server(void);
> > +
> > +#endif /* __NET_FASTBOOT_TCP_H__ */
> > diff --git a/include/net/fastboot.h b/include/net/fastboot_udp.h
> > similarity index 91%
> > rename from include/net/fastboot.h
> > rename to include/net/fastboot_udp.h
> > index 68602095d2..82145bcb9c 100644
> > --- a/include/net/fastboot.h
> > +++ b/include/net/fastboot_udp.h
> > @@ -14,7 +14,7 @@
> >  /**
> >   * Wait for incoming fastboot comands.
> >   */
> > -void fastboot_start_server(void);
> > +void fastboot_udp_start_server(void);
>
> Please add a comment
>
> >
> >  /**********************************************************************/
> >
> > diff --git a/net/Makefile b/net/Makefile
> > index bea000b206..67fc77257d 100644
> > --- a/net/Makefile
> > +++ b/net/Makefile
> > @@ -26,7 +26,8 @@ obj-$(CONFIG_CMD_PCAP) += pcap.o
> >  obj-$(CONFIG_CMD_RARP) += rarp.o
> >  obj-$(CONFIG_CMD_SNTP) += sntp.o
> >  obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> > -obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot.o
> > +obj-$(CONFIG_UDP_FUNCTION_FASTBOOT)  += fastboot_udp.o
> > +obj-$(CONFIG_TCP_FUNCTION_FASTBOOT)  += fastboot_tcp.o
> >  obj-$(CONFIG_CMD_WOL)  += wol.o
> >  obj-$(CONFIG_PROT_UDP) += udp.o
> >  obj-$(CONFIG_PROT_TCP) += tcp.o
> > diff --git a/net/fastboot_tcp.c b/net/fastboot_tcp.c
> > new file mode 100644
> > index 0000000000..b4bbd815c8
> > --- /dev/null
> > +++ b/net/fastboot_tcp.c
> > @@ -0,0 +1,143 @@
> > +// SPDX-License-Identifier: BSD-2-Clause
> > +/*
> > + * Copyright (C) 2023 The Android Open Source Project
> > + */
> > +
> > +#include <common.h>
> > +#include <fastboot.h>
> > +#include <net.h>
> > +#include <net/fastboot_tcp.h>
> > +#include <net/tcp.h>
> > +
> > +static char command[FASTBOOT_COMMAND_LEN] = {0};
> > +static char response[FASTBOOT_RESPONSE_LEN] = {0};
> > +
> > +static const unsigned short handshake_length = 4;
> > +static const uchar *handshake = "FB01";
> > +
> > +static u16 curr_sport;
> > +static u16 curr_dport;
> > +static u32 curr_tcp_seq_num;
> > +static u32 curr_tcp_ack_num;
> > +static unsigned int curr_request_len;
> > +static enum fastboot_tcp_state {
> > +       FASTBOOT_CLOSED,
> > +       FASTBOOT_CONNECTED,
> > +       FASTBOOT_DISCONNECTING
> > +} state = FASTBOOT_CLOSED;
> > +
> > +static void fastboot_tcp_answer(u8 action, unsigned int len)
> > +{
> > +       const u32 response_seq_num = curr_tcp_ack_num;
> > +       const u32 response_ack_num = curr_tcp_seq_num +
> > +                 (curr_request_len > 0 ? curr_request_len : 1);
> > +
> > +       net_send_tcp_packet(len, htons(curr_sport), htons(curr_dport),
> > +                           action, response_seq_num, response_ack_num);
> > +}
> > +
> > +static void fastboot_tcp_reset(void)
> > +{
> > +       fastboot_tcp_answer(TCP_RST, 0);
> > +       state = FASTBOOT_CLOSED;
> > +}
> > +
> > +static void fastboot_tcp_send_packet(u8 action, const uchar *data, unsigned int len)
> > +{
> > +       uchar *pkt = net_get_async_tx_pkt_buf();
> > +
> > +       memset((void *)pkt, 0, PKTSIZE);
>
> '\0'
>
> Can you drop cast on these?
>
> > +       pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
> > +       memcpy(pkt, data, len);
> > +       fastboot_tcp_answer(action, len);
> > +       memset((void *)pkt, 0, PKTSIZE);
> > +}
> > +
> > +static void fastboot_tcp_send_message(const char *message, unsigned int len)
> > +{
> > +       __be64 len_be = __cpu_to_be64(len);
> > +       uchar *pkt = net_get_async_tx_pkt_buf();
> > +
> > +       memset((void *)pkt, 0, PKTSIZE);
> > +       pkt += net_eth_hdr_size() + IP_TCP_HDR_SIZE + TCP_TSOPT_SIZE + 2;
> > +       // Put first 8 bytes as a big endian message length
> > +       memcpy(pkt, &len_be, 8);
> > +       pkt += 8;
> > +       memcpy(pkt, message, len);
> > +       fastboot_tcp_answer(TCP_ACK | TCP_PUSH, len + 8);
> > +       memset((void *)pkt, 0, PKTSIZE);
> > +}
> > +
> > +static void fastboot_tcp_handler_ipv4(uchar *pkt, u16 dport,
> > +                                     struct in_addr sip, u16 sport,
> > +                                     u32 tcp_seq_num, u32 tcp_ack_num,
> > +                                     u8 action, unsigned int len)
> > +{
> > +       u64 command_size;
>
> Wow, that can really be that big? Or are you using u64 just because
> that is the size of the field?
>
> > +       u8 tcp_fin = action & TCP_FIN;
> > +       u8 tcp_push = action & TCP_PUSH;
> > +
> > +       curr_sport = sport;
> > +       curr_dport = dport;
> > +       curr_tcp_seq_num = tcp_seq_num;
> > +       curr_tcp_ack_num = tcp_ack_num;
> > +       curr_request_len = len;
> > +
> > +       switch (state) {
> > +       case FASTBOOT_CLOSED:
> > +               if (tcp_push) {
> > +                       if (len != handshake_length ||
> > +                           strlen(pkt) != handshake_length ||
> > +                           memcmp(pkt, handshake, handshake_length) != 0) {
> > +                               fastboot_tcp_reset();
> > +                               break;
> > +                       }
> > +                       fastboot_tcp_send_packet(TCP_ACK | TCP_PUSH,
> > +                                                handshake, handshake_length);
> > +                       state = FASTBOOT_CONNECTED;
> > +               }
> > +               break;
> > +       case FASTBOOT_CONNECTED:
> > +               if (tcp_fin) {
> > +                       fastboot_tcp_answer(TCP_FIN | TCP_ACK, 0);
> > +                       state = FASTBOOT_DISCONNECTING;
> > +                       break;
> > +               }
> > +               if (tcp_push) {
> > +                       // First 8 bytes is big endian message length
> > +                       command_size = __be64_to_cpu(*(u64 *)pkt);
> > +                       len -= 8;
> > +                       pkt += 8;
> > +
> > +                       // Only single packet messages are supported ATM
> > +                       if (strlen(pkt) != command_size) {
> > +                               fastboot_tcp_reset();
> > +                               break;
> > +                       }
> > +                       strlcpy(command, pkt, len + 1);
> > +                       fastboot_handle_command(command, response);
> > +                       fastboot_tcp_send_message(response, strlen(response));
> > +               }
> > +               break;
> > +       case FASTBOOT_DISCONNECTING:
> > +               if (tcp_push)
> > +                       state = FASTBOOT_CLOSED;
> > +               break;
> > +       }
> > +
> > +       memset(command, 0, FASTBOOT_COMMAND_LEN);
> > +       memset(response, 0, FASTBOOT_RESPONSE_LEN);
> > +       curr_sport = 0;
> > +       curr_dport = 0;
> > +       curr_tcp_seq_num = 0;
> > +       curr_tcp_ack_num = 0;
> > +       curr_request_len = 0;
> > +}
> > +
> > +void fastboot_tcp_start_server(void)
> > +{
> > +       printf("Using %s device\n", eth_get_name());
> > +       printf("Listening for fastboot command on tcp %pI4\n", &net_ip);
> > +
> > +       tcp_set_tcp_handler(fastboot_tcp_handler_ipv4);
> > +}
> > diff --git a/net/fastboot.c b/net/fastboot_udp.c
> > similarity index 99%
> > rename from net/fastboot.c
> > rename to net/fastboot_udp.c
> > index e9569d88d2..27e779d8e0 100644
> > --- a/net/fastboot.c
> > +++ b/net/fastboot_udp.c
> > @@ -7,7 +7,7 @@
> >  #include <command.h>
> >  #include <fastboot.h>
> >  #include <net.h>
> > -#include <net/fastboot.h>
> > +#include <net/fastboot_udp.h>
> >
> >  enum {
> >         FASTBOOT_ERROR = 0,
> > @@ -300,7 +300,7 @@ static void fastboot_handler(uchar *packet, unsigned int dport,
> >         }
> >  }
> >
> > -void fastboot_start_server(void)
> > +void fastboot_udp_start_server(void)
> >  {
> >         printf("Using %s device\n", eth_get_name());
> >         printf("Listening for fastboot command on %pI4\n", &net_ip);
> > diff --git a/net/net.c b/net/net.c
> > index c9a749f6cc..41c3cac26e 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -93,7 +93,8 @@
> >  #include <net.h>
> >  #include <net6.h>
> >  #include <ndisc.h>
> > -#include <net/fastboot.h>
> > +#include <net/fastboot_udp.h>
> > +#include <net/fastboot_tcp.h>
> >  #include <net/tftp.h>
> >  #include <net/ncsi.h>
> >  #if defined(CONFIG_CMD_PCAP)
> > @@ -498,9 +499,14 @@ restart:
> >                         tftp_start_server();
> >                         break;
> >  #endif
> > -#ifdef CONFIG_UDP_FUNCTION_FASTBOOT
> > -               case FASTBOOT:
> > -                       fastboot_start_server();
> > +#if defined(CONFIG_UDP_FUNCTION_FASTBOOT)
> > +               case FASTBOOT_UDP:
> > +                       fastboot_udp_start_server();
> > +                       break;
> > +#endif
> > +#if defined(CONFIG_TCP_FUNCTION_FASTBOOT)
> > +               case FASTBOOT_TCP:
> > +                       fastboot_tcp_start_server();
> >                         break;
> >  #endif
> >  #if defined(CONFIG_CMD_DHCP)
> > @@ -1491,7 +1497,8 @@ common:
> >                 /* Fall through */
> >
> >         case NETCONS:
> > -       case FASTBOOT:
> > +       case FASTBOOT_UDP:
> > +       case FASTBOOT_TCP:
> >         case TFTPSRV:
> >                 if (IS_ENABLED(CONFIG_IPV6) && use_ip6) {
> >                         if (!memcmp(&net_link_local_ip6, &net_null_addr_ip6,
> > --
> > 2.40.0.348.gf938b09366-goog
> >
>
> Regards,
> Simon

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-04-13 14:57 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-28 20:31 [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Dmitrii Merkurev
2023-03-28 20:31 ` [PATCH 2/3] net: add fastboot TCP support Dmitrii Merkurev
2023-04-01  6:32   ` Simon Glass
2023-04-12 18:55     ` Dmitrii Merkurev
2023-03-28 20:31 ` [PATCH 3/3] net: share fastboot boot handle logic between transports Dmitrii Merkurev
2023-04-01  6:32   ` Simon Glass
2023-04-01 18:55     ` Ramon Fried
2023-03-29  9:08 ` [PATCH 1/3] net: support being a TCP server to unblock TCP fastboot Paul Liu
2023-04-01  6:32 ` Simon Glass
2023-04-01 18:54   ` Ramon Fried

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).