Re: [U-Boot] [PATCH v10 2/3] Adding TCP

2018-04-30 Thread Joe Hershberger
Hi Duncan,

On Sat, Apr 14, 2018 at 6:43 PM,   wrote:
> From: Duncan Hare 
>
> All the code is new, and not copied from any source.
>
> Series-changes
> The previous patch was using an old version of net/Kconfig,
> which prevented requesting options for a bootp/dhcp request.
>
> A similar issue fixed with a cmd/Kconfig.
> Items in include/net.h fixed.

Please follow the format as listed in the patman README:
"""
Series-changes: n
- Guinea pig moved into its cage
- Other changes ending with a blank line

This can appear in any commit. It lists the changes for a
particular version n of that commit. The change list is
created based on this information. Each commit gets its own
change list and also the whole thing is repeated in the cover
letter (where duplicate change lines are merged).

By adding your change lists into your commits it is easier to
keep track of what happened. When you amend a commit, remember
to update the log there and then, knowing that the script will
do the rest.
"""

When you make changes to the patches based on my feedback, add to the
list of changes for version 11 as you make the changes so you won't
forget anything.

> Signed-off-by: Duncan Hare 
> Signed-off-by: Duncan Hare 
> ---
>
> Why TCP:
>
> Currently file transfer are done using tftp or NFS both
> over udp. This requires a request to be sent from client
> (u-boot) to the boot server.
>
> For a 4 Mbyte kernel, with a 1k block size this requires
> 4,000 request for a block.
>
> Using a large block size, one greater than the Ethernet
> maximum frame size limitation, would require fragmentation,
> which u-boot supports. However missing fragment recovery
> requires timeout detection and re-transmission requests
> for missing fragments.
>
> In networks with large latency, for example: the internet,
> UDP is vwey slow. What is a 30 second transfer on a local
> boot server and LAN increase to over 3 minutes, because of
> all the requests/response traffic.
>
> This was anticipated in the evolution of the IP protocols
> and TCP was developed and then enhanced for high latency high
> bandwidth networks.
>
> The current standard is TCP with selective acknowledgment.

You make no mention in the commit log of this patch supporting
selective ack or not. Honestly, all of this description (above this
line) seems like info that should actually stay in the patch's log,
not in a note.

> Routine tcp_print_buffer() is used to print portions of
> non zero terminated buffers. If there is an existing routine
> please let me know. I'm from the world of length fields
> not zero terminated strings (zOS).
>
>
> Changes in v10: None
>
>  include/net.h |   8 +-
>  include/net/tcp.h | 218 
>  net/Kconfig   |   5 +
>  net/Makefile  |   2 +-
>  net/net.c |  51 +++-
>  net/tcp.c | 749 
> ++
>  6 files changed, 1023 insertions(+), 10 deletions(-)
>  create mode 100644 include/net/tcp.h
>  create mode 100644 net/tcp.c
>
> diff --git a/include/net.h b/include/net.h
> index 7e5f5a6a5b..e29d804a23 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -548,7 +548,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

Seems like this should be in the WGET patch, right?

>  };
>
>  extern charnet_boot_file_name[1024];/* Boot File name */
> @@ -681,11 +681,15 @@ static inline void net_send_packet(uchar *pkt, int len)
>   * @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);
> +  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/include/net/tcp.h b/include/net/tcp.h
> new file mode 100644
> index 00..81f263351e
> --- /dev/null
> +++ b/include/net/tcp.h
> @@ -0,0 +1,218 @@
> +/*
> + * TCP Support for file transfer.
> + *
> + * Copyright 2017 Duncan Hare, All rights reserved.
> + *
> + *  SPDX-License-Identifier:GPL-2.0
> + */
> +
> +#define TCP_ACTIVITY 127   /* Activity on downloading  */

What is the meaning of this number? Number of packets before the
console adds a progress mark? Please add a more verbose comment.

> +
> +struct ip_tcp_hdr {
> + 

[U-Boot] [PATCH v10 2/3] Adding TCP

2018-04-14 Thread DH
From: Duncan Hare 

All the code is new, and not copied from any source.

Series-changes
The previous patch was using an old version of net/Kconfig,
which prevented requesting options for a bootp/dhcp request.

A similar issue fixed with a cmd/Kconfig.
Items in include/net.h fixed.

Signed-off-by: Duncan Hare 
Signed-off-by: Duncan Hare 
---

Why TCP:

Currently file transfer are done using tftp or NFS both
over udp. This requires a request to be sent from client
(u-boot) to the boot server.

For a 4 Mbyte kernel, with a 1k block size this requires
4,000 request for a block.

Using a large block size, one greater than the Ethernet
maximum frame size limitation, would require fragmentation,
which u-boot supports. However missing fragment recovery
requires timeout detection and re-transmission requests
for missing fragments.

In networks with large latency, for example: the internet,
UDP is vwey slow. What is a 30 second transfer on a local
boot server and LAN increase to over 3 minutes, because of
all the requests/response traffic.

This was anticipated in the evolution of the IP protocols
and TCP was developed and then enhanced for high latency high
bandwidth networks.

The current standard is TCP with selective acknowledgment.

Routine tcp_print_buffer() is used to print portions of
non zero terminated buffers. If there is an existing routine
please let me know. I'm from the world of length fields
not zero terminated strings (zOS).


Changes in v10: None

 include/net.h |   8 +-
 include/net/tcp.h | 218 
 net/Kconfig   |   5 +
 net/Makefile  |   2 +-
 net/net.c |  51 +++-
 net/tcp.c | 749 ++
 6 files changed, 1023 insertions(+), 10 deletions(-)
 create mode 100644 include/net/tcp.h
 create mode 100644 net/tcp.c

diff --git a/include/net.h b/include/net.h
index 7e5f5a6a5b..e29d804a23 100644
--- a/include/net.h
+++ b/include/net.h
@@ -548,7 +548,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 charnet_boot_file_name[1024];/* Boot File name */
@@ -681,11 +681,15 @@ static inline void net_send_packet(uchar *pkt, int len)
  * @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);
+  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/include/net/tcp.h b/include/net/tcp.h
new file mode 100644
index 00..81f263351e
--- /dev/null
+++ b/include/net/tcp.h
@@ -0,0 +1,218 @@
+/*
+ * TCP Support for file transfer.
+ *
+ * Copyright 2017 Duncan Hare, All rights reserved.
+ *
+ *  SPDX-License-Identifier:GPL-2.0
+ */
+
+#define TCP_ACTIVITY 127   /* Activity on downloading  */
+
+struct ip_tcp_hdr {
+   u8  ip_hl_v;/* header length and version*/
+   u8  ip_tos; /* type of service  */
+   u16 ip_len; /* total length */
+   u16 ip_id;  /* identification   */
+   u16 ip_off; /* fragment offset field*/
+   u8  ip_ttl; /* time to live */
+   u8  ip_p;   /* protocol */
+   u16 ip_sum; /* checksum */
+   struct in_addr  ip_src; /* Source IP address*/
+   struct in_addr  ip_dst; /* Destination IP address   */
+   u16 tcp_src;/* TCP source port  */
+   u16 tcp_dst;/* TCP destination port */
+   u32 tcp_seq;/* TCP sequence number  */
+   u32 tcp_ack;/* TCP Acknowledgment number*/
+   u8  tcp_hlen;   /* 4 bits TCP header Length/4   */
+   /* 4 bits Reserved  */
+   /* 2 more bits reserved */
+   u8  tcp_flags;  /* see defines  */
+   u16 tcp_win;/* TCP windows size */
+   u16 tcp_xsum;   /* Checksum */
+   u16 tc