[U-Boot] [PATCH] TCP and wget implementation.

Joe Hershberger joe.hershberger at ni.com
Tue Dec 5 20:35:29 UTC 2017


Hi Duncan,

The subject should not end with a '.' character.

Also, this should have been marked as v2. Please mark the next as v3.

On Wed, Nov 8, 2017 at 6:34 PM, Duncan Hare <dh at synoia.com> wrote:
> his is the interface and Kconfig files for introducing TCP and wget
> into u-boot.

Missing 'T' at the beginning?

> Interfaces are in net.c and net.h, ping is modified to the new ip send
> interface, and UDP and TCP have shim procedures call map the protocol
> interface to the ip interface.

I'd like to see the shim and all changes to existing stack as a
separate patch to make review simpler.

Also please make TCP and wget into 2 other separate patches.

> The UDP interface is unchanged, and the existing UDP programs need no
> changes.
>
> All the code is new, and not copied from any source.
>
> The wget command and TCP stack are separately configured in Kconfig,
> and provisioning wget without TCP will result in a number of error
> messages. It might be possible to have Kconfig handle dependencies,
> if so advice is welcome.

Yes it should... you could have wget "selects" tcp.

> Makefile in the net directory is modified by hand. It appears not to be
> generated by Kconfig. Again advice is welcome.

That's fine.

> The rationale behind this change is that UDP file transfers elapsed time
> is twice the sum of network latency x number of pcckets, and TCP file
> transfer times are about 4x network latency plus data transit time.
>
> In tests this reduces kernel trnasfer time from about 15 to 20 seconds
> with tftp on a raspberry pi to about 0.4 seconds with wget.

What settings are used in this tftp benchmark for block size? It makes
a huge difference to the performance.

> The raspberry pi as a sink for the kernel runs at about 10 Mbits/sec.
>
> Signed-off-by: Duncan Hare <DH at synoia.com>
> ---
>
>  cmd/Kconfig   |   5 +++
>  cmd/net.c     |  13 ++++++++
>  include/net.h |  26 ++++++++++++---
>  net/Kconfig   |   5 +++
>  net/Makefile  |   3 +-
>  net/net.c     | 100 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  net/ping.c    |   9 ++----
>  7 files changed, 132 insertions(+), 29 deletions(-)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5a6afab99b..4e5bac685e 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1035,6 +1035,11 @@ config CMD_ETHSW
>           operations such as enabling / disabling a port and
>           viewing/maintaining the filtering database (FDB)
>
> +config CMD_WGET
> +       bool "wget"
> +       help
> +         Download a kernel, or other files, from a web server over TCP.
> +
>  endmenu
>
>  menu "Misc commands"
> diff --git a/cmd/net.c b/cmd/net.c
> index d7c776aacf..f5c2d0f8ed 100644
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -110,6 +110,19 @@ U_BOOT_CMD(
>  );
>  #endif
>
> +#if defined(CONFIG_CMD_WGET)
> +static int do_wget(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       return netboot_common(WGET, cmdtp, argc, argv);
> +}
> +
> +U_BOOT_CMD(
> +       wget,   3,      1,      do_wget,
> +       "boot image via network using HTTP protocol",
> +       "[loadAddress] [[hostIPaddr:]bootfilename]"
> +);
> +#endif
> +
>  static void netboot_update_env(void)
>  {
>         char tmp[22];
> diff --git a/include/net.h b/include/net.h
> index 455b48f6c7..7787413816 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -24,8 +24,17 @@
>   *     The number of receive packet buffers, and the required packet buffer
>   *     alignment in memory.
>   *
> + *     The nuber of buffers for TCP is used to calculate a static TCP window
> + *     size, becuse TCP window size is a promise to the sending TCP to be able
> + *     to buffer up to the window size of data.
> + *     When the sending TCP has a window size of outstanding unacknowledged
> + *     data, the sending TCP will stop sending.
>   */
>
> +#if defined(CONFIG_TCP)
> +#define CONFIG_SYS_RX_ETH_BUFFER 50    /* For TCP */
> +#endif
> +
>  #ifdef CONFIG_SYS_RX_ETH_BUFFER
>  # define PKTBUFSRX     CONFIG_SYS_RX_ETH_BUFFER
>  #else
> @@ -354,6 +363,8 @@ struct vlan_ethernet_hdr {
>
>  #define IPPROTO_ICMP    1      /* Internet Control Message Protocol    */
>  #define IPPROTO_UDP    17      /* User Datagram Protocol               */
> +#define IPPROTO_TCP     6      /* Transmission Control Protocol        */
> +
>
>  /*
>   *     Internet Protocol (IP) header.
> @@ -538,7 +549,7 @@ extern int          net_restart_wrap;       /* Tried all network devices */
>
>  enum proto_t {
>         BOOTP, RARP, ARP, TFTPGET, DHCP, PING, DNS, NFS, CDP, NETCONS, SNTP,
> -       TFTPSRV, TFTPPUT, LINKLOCAL
> +       TFTPSRV, TFTPPUT, LINKLOCAL, WGET
>  };
>
>  extern char    net_boot_file_name[1024];/* Boot File name */
> @@ -596,10 +607,10 @@ int net_set_ether(uchar *xet, const uchar *dest_ethaddr, uint prot);
>  int net_update_ether(struct ethernet_hdr *et, uchar *addr, uint prot);
>
>  /* Set IP header */
> -void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source);
> +void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source,
> +                      u16  pkt_len, u8 prot);
>  void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport,
> -                               int sport, int len);
> -
> +                       int sport, int len);
>  /**
>   * compute_ip_checksum() - Compute IP checksum
>   *
> @@ -670,9 +681,16 @@ static inline void net_send_packet(uchar *pkt, int len)
>   * @param sport Source UDP port
>   * @param payload_len Length of data after the UDP header
>   */
> +int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int sport,
> +                      int payload_len, int proto, u8 action, u32 tcp_seq_num,
> +                      u32 tcp_ack_num);
> +
>  int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport,
>                         int sport, int payload_len);
>
> +int net_send_tcp_packet(int payload_len, int dport, int sport, u8 action,
> +                       u32 tcp_seq_num, u32 tcp_ack_num);
> +
>  /* Processes a received packet */
>  void net_process_received_packet(uchar *in_packet, int len);
>
> diff --git a/net/Kconfig b/net/Kconfig
> index 414c5497c7..625ad291bb 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -45,4 +45,9 @@ config BOOTP_VCI_STRING
>         default "U-Boot.arm" if ARM
>         default "U-Boot"
>
> +config TCP
> +       bool "Include Subset TCP stack for wget"
> +       help
> +         TCP protocol support for wget.
> +
>  endif   # if NET
> diff --git a/net/Makefile b/net/Makefile
> index ae54eee5af..f83df5b728 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -25,7 +25,8 @@ obj-$(CONFIG_CMD_PING) += ping.o
>  obj-$(CONFIG_CMD_RARP) += rarp.o
>  obj-$(CONFIG_CMD_SNTP) += sntp.o
>  obj-$(CONFIG_CMD_NET)  += tftp.o
> -
> +obj-$(CONFIG_TCP)      += tcp.o
> +obj-$(CONFIG_CMD_WGET) += wget.o

It looks like you are expecting to add two new source files, but they
are not here.


>  # Disable this warning as it is triggered by:
>  # sprintf(buf, index ? "foo%d" : "foo", index)
>  # and this is intentional usage.
> diff --git a/net/net.c b/net/net.c
> index 4259c9e321..0ce3413cfc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -107,6 +107,12 @@
>  #if defined(CONFIG_CMD_SNTP)
>  #include "sntp.h"
>  #endif
> +#if defined(CONFIG_TCP)
> +#include "tcp.h"
> +#endif
> +#if defined(CONFIG_CMD_WGET)
> +#include "wget.h"
> +#endif
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -181,6 +187,7 @@ int         net_ntp_time_offset;
>  static uchar net_pkt_buf[(PKTBUFSRX+1) * PKTSIZE_ALIGN + PKTALIGN];
>  /* Receive packets */
>  uchar *net_rx_packets[PKTBUFSRX];
> +
>  /* Current UDP RX packet handler */
>  static rxhand_f *udp_packet_handler;
>  /* Current ARP RX packet handler */
> @@ -381,6 +388,9 @@ void net_init(void)
>
>                 /* Only need to setup buffer pointers once. */
>                 first_call = 0;
> +#if defined(CONFIG_TCP)
> +                       tcp_set_tcp_state(TCP_CLOSED);
> +#endif
>         }
>
>         net_init_loop();
> @@ -484,6 +494,11 @@ restart:
>                         nfs_start();
>                         break;
>  #endif
> +#if defined(CONFIG_CMD_WGET)
> +               case WGET:
> +                       wget_start();
> +                       break;
> +#endif
>  #if defined(CONFIG_CMD_CDP)
>                 case CDP:
>                         cdp_start();
> @@ -777,11 +792,41 @@ void net_set_timeout_handler(ulong iv, thand_f *f)
>  }
>
>  int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, int sport,
> -               int payload_len)
> +                       int payload_len)
> +{
> +       return net_send_ip_packet(ether, dest, dport, sport, payload_len,
> +                                 IPPROTO_UDP, 0, 0, 0);
> +}
> +
> +#if defined(CONFIG_TCP)
> +int net_send_tcp_packet(int payload_len, int dport, int sport, u8 action,
> +                       u32 tcp_seq_num, u32 tcp_ack_num)
> +{
> +       return net_send_ip_packet(net_server_ethaddr, net_server_ip, dport,
> +                                 sport, payload_len, IPPROTO_TCP, action,
> +                                 tcp_seq_num, tcp_ack_num);
> +}
> +#endif
> +
> +int net_send_ip_packet(uchar *ether, struct in_addr dest, int dport, int sport,
> +                      int payload_len, int proto, u8 action, u32 tcp_seq_num,
> +                      u32 tcp_ack_num)
>  {
>         uchar *pkt;
>         int eth_hdr_size;
>         int pkt_hdr_size;
> +       if (proto == IPPROTO_UDP) {
> +               debug_cond(DEBUG_DEV_PKT,
> +                          "UDP Send  (to=%pI4, from=%pI4, len=%d)\n",
> +                          &dest, &net_ip, payload_len);
> +#if defined(CONFIG_TCP)
> +
> +       } else {
> +               debug_cond(DEBUG_DEV_PKT,
> +                          "TCP Send  (%pI4, %pI4, len=%d, A=%x)\n",
> +                          &dest, &net_ip, payload_len, action);
> +#endif
> +       }
>
>         /* make sure the net_tx_packet is initialized (net_init() was called) */
>         assert(net_tx_packet != NULL);
> @@ -799,9 +844,22 @@ int net_send_udp_packet(uchar *ether, struct in_addr dest, int dport, int sport,
>         pkt = (uchar *)net_tx_packet;
>
>         eth_hdr_size = net_set_ether(pkt, ether, PROT_IP);
> -       pkt += eth_hdr_size;
> -       net_set_udp_header(pkt, dest, dport, sport, payload_len);
> -       pkt_hdr_size = eth_hdr_size + IP_UDP_HDR_SIZE;
> +#if defined(CONFIG_TCP)
> +       if (proto == IPPROTO_UDP) {
> +#endif
> +               net_set_udp_header(pkt + eth_hdr_size, dest,
> +                                  dport, sport, payload_len);
> +               pkt_hdr_size = IP_UDP_HDR_SIZE;
> +               eth_hdr_size = eth_hdr_size + pkt_hdr_size;
> +
> +#if defined(CONFIG_TCP)
> +       } else {
> +               pkt_hdr_size = eth_hdr_size +
> +               tcp_set_tcp_header(pkt + eth_hdr_size, dport, sport,
> +                                  payload_len, action,
> +                                  tcp_seq_num, tcp_ack_num);
> +       }
> +#endif
>
>         /* if MAC address was not discovered yet, do an ARP request */
>         if (memcmp(ether, net_null_ethaddr, 6) == 0) {
> @@ -1157,9 +1215,6 @@ void net_process_received_packet(uchar *in_packet, int len)
>                 /* Can't deal with anything except IPv4 */
>                 if ((ip->ip_hl_v & 0xf0) != 0x40)
>                         return;
> -               /* Can't deal with IP options (headers != 20 bytes) */
> -               if ((ip->ip_hl_v & 0x0f) > 0x05)
> -                       return;
>                 /* Check the Checksum of the header */
>                 if (!ip_checksum_ok((uchar *)ip, IP_HDR_SIZE)) {
>                         debug("checksum bad\n");
> @@ -1205,9 +1260,19 @@ void net_process_received_packet(uchar *in_packet, int len)
>                  * we send a tftp packet to a dead connection, or when
>                  * there is no server at the other end.
>                  */
> +
>                 if (ip->ip_p == IPPROTO_ICMP) {
>                         receive_icmp(ip, len, src_ip, et);
>                         return;
> +#if defined(CONFIG_TCP)
> +               } else if (ip->ip_p == IPPROTO_TCP) {
> +                       debug_cond(DEBUG_DEV_PKT,
> +                                  "TCP PH (to=%pI4, from=%pI4, len=%d)\n",
> +                                  &dst_ip, &src_ip, len);
> +
> +                               rxhand_tcp_f((union tcp_build_pkt *)ip, len);
> +                       return;
> +#endif
>                 } else if (ip->ip_p != IPPROTO_UDP) {   /* Only UDP packets */
>                         return;
>                 }
> @@ -1426,25 +1491,28 @@ int net_update_ether(struct ethernet_hdr *et, uchar *addr, uint prot)
>         }
>  }
>
> -void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source)
> +void net_set_ip_header(uchar *pkt, struct in_addr dest, struct in_addr source,
> +                      u16  pkt_len, u8 prot)
>  {
>         struct ip_udp_hdr *ip = (struct ip_udp_hdr *)pkt;
>
>         /*
> -        *      Construct an IP header.
> +        *      Construct an IP header.
>          */
>         /* IP_HDR_SIZE / 4 (not including UDP) */
>         ip->ip_hl_v  = 0x45;
>         ip->ip_tos   = 0;
> -       ip->ip_len   = htons(IP_HDR_SIZE);
> +       ip->ip_len   = htons(pkt_len);
>         ip->ip_id    = htons(net_ip_id++);
> -       ip->ip_off   = htons(IP_FLAGS_DFRAG);   /* Don't fragment */
> +       ip->ip_off   = htons(IP_FLAGS_DFRAG);   /* Don't fragment */
>         ip->ip_ttl   = 255;
> +       ip->ip_p     = prot;
>         ip->ip_sum   = 0;
>         /* already in network byte order */
>         net_copy_ip((void *)&ip->ip_src, &source);
>         /* already in network byte order */
>         net_copy_ip((void *)&ip->ip_dst, &dest);
> +       ip->ip_sum  = compute_ip_checksum(ip, IP_HDR_SIZE);
>  }
>
>  void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport, int sport,
> @@ -1460,11 +1528,8 @@ void net_set_udp_header(uchar *pkt, struct in_addr dest, int dport, int sport,
>         if (len & 1)
>                 pkt[IP_UDP_HDR_SIZE + len] = 0;
>
> -       net_set_ip_header(pkt, dest, net_ip);
> -       ip->ip_len   = htons(IP_UDP_HDR_SIZE + len);
> -       ip->ip_p     = IPPROTO_UDP;
> -       ip->ip_sum   = compute_ip_checksum(ip, IP_HDR_SIZE);
> -
> +       net_set_ip_header(pkt, dest, net_ip, IP_UDP_HDR_SIZE + len,
> +                         IPPROTO_UDP);
>         ip->udp_src  = htons(sport);
>         ip->udp_dst  = htons(dport);
>         ip->udp_len  = htons(UDP_HDR_SIZE + len);
> @@ -1485,7 +1550,8 @@ void copy_filename(char *dst, const char *src, int size)
>
>  #if    defined(CONFIG_CMD_NFS)         || \
>         defined(CONFIG_CMD_SNTP)        || \
> -       defined(CONFIG_CMD_DNS)
> +       defined(CONFIG_CMD_DNS)         || \
> +       defined(CONFIG_CMD_WGET)
>  /*
>   * make port a little random (1024-17407)
>   * This keeps the math somewhat trivial to compute, and seems to work with
> diff --git a/net/ping.c b/net/ping.c
> index 9508cf1160..254b646193 100644
> --- a/net/ping.c
> +++ b/net/ping.c
> @@ -20,16 +20,11 @@ struct in_addr net_ping_ip;
>  static void set_icmp_header(uchar *pkt, struct in_addr dest)
>  {
>         /*
> -        *      Construct an IP and ICMP header.
> +        *      Construct an ICMP header.
>          */
> -       struct ip_hdr *ip = (struct ip_hdr *)pkt;
>         struct icmp_hdr *icmp = (struct icmp_hdr *)(pkt + IP_HDR_SIZE);
>
> -       net_set_ip_header(pkt, dest, net_ip);
> -
> -       ip->ip_len   = htons(IP_ICMP_HDR_SIZE);
> -       ip->ip_p     = IPPROTO_ICMP;
> -       ip->ip_sum   = compute_ip_checksum(ip, IP_HDR_SIZE);
> +       net_set_ip_header(pkt, dest, net_ip, IP_ICMP_HDR_SIZE, IPPROTO_ICMP);
>
>         icmp->type = ICMP_ECHO_REQUEST;
>         icmp->code = 0;
> --
> 2.11.0


More information about the U-Boot mailing list