Re: [U-Boot] [PATCH] TCP and wget implementation v3

[Joe Hershberger]

On Wed, Jan 3, 2018 at 3:07 PM, Duncan Hare  wrote:
>
>>>selects the LIB_RAND feature since it is required.
>
> Thanks: will be in u-boot/cmd/Kconfig
>
>>Are we lookin at a series of patches, or a concurrent set?
>
> At this time a series of three, but I'd take advice on the preferred
> procedure.

Remember that the goal is to be atomic.

You should be able to build and use U-Boot after each patch. For
instance, if you are adding a new command, the source for that
command, the Kconfig option, the Makefile change to build it, etc
should be in the same commit. Similarly, each commit should do as
little as is reasonably a distinct change.

Also, any changes to existing code that is not changing behavior but
simply making way for new functionality should be done separately.
This way it's easier to review in the mindset of "this should be
functionally equivalent", making it easier to spot issues without
getting lost in a ton of code.

Thanks,
-Joe

> (1)
> cmd/Kconfig
> cmd/net.c
>
> net/Kconfig
> net/net.c
> net/ping.c
>
> include/net.h
>
> (2)
> net/tcp.c
> net/tcp.h
> net/Makefile
>
> (3)
> net/wget.c
> net/wget.h
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] TCP and wget implementation v3

[Duncan Hare]

>>selects the LIB_RAND feature since it is required.

Thanks: will be in u-boot/cmd/Kconfig

>Are we lookin at a series of patches, or a concurrent set? 

At this time a series of three, but I'd take advice on the preferred
procedure. 
(1)
cmd/Kconfig 
cmd/net.c 

net/Kconfig
net/net.c
net/ping.c

include/net.h

(2)
net/tcp.c
net/tcp.h
net/Makefile

(3)
net/wget.c
net/wget.h

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] TCP and wget implementation v3

[Joe Hershberger]

On Tue, Dec 19, 2017 at 11:02 AM, Duncan Hare  wrote:
>>I'd like to see the shim and all changes to existing stack as a
>>separate patch to make review simpler.
>
> In this patch.
>
>>Also please make TCP and wget into 2 other separate patches.
>
> Yes, following.
>
>> Yes it should... you could have wget "selects" tcp.
> Please send a "how to" example.

Line 12 in net/Kconfig is an example. The NET_RANDOM_ETHADDR feature
selects the LIB_RAND feature since it is required.

>
>>> 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.
>
>>What settings are used in this tftp benchmark for block size? It makes
>>a huge difference to the performance.
>
> We use the default blocksize.
>
> We decided on TCP becuse it is probably the best high performance,
> reliable protocol available, and the simplicity of the http protocol.
>
> UDP has its place. Relaible fast file transfer is a poor fit for UDP.
>
> We we lookin at a series of patches, or a concurrent set? The
> previous patch include all the pvarius change for setting up
> integration into u-boot. This patch standing by itself cannot
> be integrated. At a minimun it needs the slightly changed
> version of ping.
>
> Signed-off-by: Duncan Hare 
> ---
>
>  include/net.h |  32 ++
>  net/net.c | 102
>  +++--- 2 files
>  changed, 108 insertions(+), 26 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 455b48f6c7..d231987e6e 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -15,17 +15,26 @@
>  #include 
>  #include  /* for nton* / ntoh* stuff */
>
> -#define DEBUG_LL_STATE 0   /* Link local state machine changes */
> -#define DEBUG_DEV_PKT 0/* Packets or info directed to
> the device */ -#define DEBUG_NET_PKT 0  /* Packets on
> info on the network at large */ +#define DEBUG_LL_STATE  0  /*
> Link local state machine changes */ +#define DEBUG_DEV_PKT
> 0   /* Packets or info directed to the device */ +#define
> DEBUG_NET_PKT   0   /* Packets on info on the network at large */
> #define DEBUG_INT_STATE 0   /* Internal network state changes */
>  /*
>   * 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_ICMP1  /* 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 charnet_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