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 <s...@chromium.org> wrote: > > Hi Dmitrii, > > On Wed, 29 Mar 2023 at 09:31, Dmitrii Merkurev <dimori...@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 <dimori...@google.com> > > Cc: Ying-Chun Liu (PaulLiu) <paul....@linaro.org> > > Cc: Simon Glass <s...@chromium.org> > > Сс: Joe Hershberger <joe.hershber...@ni.com> > > Сс: Ramon Fried <rfried....@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 <s...@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
