Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Duncan Hare]

On Mon, 19 Mar 2018 09:53:24 +
Calvin Johnson  wrote:

>
> > > >  void net_set_udp_header(uchar *pkt, struct in_addr dest, int
> > > > dport,
> > > > -   int sport, int len);
> > > > -
> > > > +   int sport, int len);  
> > >  
> > Why do you need this change in the set_udp_header?
> > 
> > This is a shim to bridge between the original udp to ip procedure
> > call and the extra parameters in the enhanced ip procedure call to
> > the ip layer for TCP.
> > 
> > The original udp call is unchanged, because I did not want a
> > change to a procedure call to ripple through many applications.  
> 
> For now, I'm okay with the change you made to net_set_ip_header.
> 
> -   int sport, int len);
> -
> +   int sport, int len);
> I'm concerned about above 3 line change. Here, you are decreasing
> indent and removing an empty line.  This change may not be required.

yes, I don't remember this, but its probably driven by patman messages.
These are both issues which can generated patman error messages. 
 
Duncan

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

Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Calvin Johnson]

> > It would be good to have this cosmetic change into a separate patch.
> 
> Ok. But at this stage I'm at the "Forgive me Lord for I know not what
> I do"

 . 
Let me know, if you need steps to separate this out. 
To me, it is okay even If you keep this in the same patch.


 
> > >  /* 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);
> >
> Why do you need this change in the set_udp_header?
> 
> This is a shim to bridge between the original udp to ip procedure call
> and the extra parameters in the enhanced ip procedure call to the ip
> layer for TCP.
> 
> The original udp call is unchanged, because I did not want a
> change to a procedure call to ripple through many applications.

For now, I'm okay with the change you made to net_set_ip_header.

-   int sport, int len);
-
+   int sport, int len);
I'm concerned about above 3 line change. Here, you are decreasing indent 
and removing an empty line.  This change may not be required.

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

Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Duncan Hare]

On Sat, 17 Mar 2018 16:36:14 +
Calvin Johnson  wrote:

> It would be good to have this cosmetic change into a separate patch. 

Ok. But at this stage I'm at the "Forgive me Lord for I know not what
I do" 

> IMO, better place for this definition and associated explanation
> would be above the comment describing PKTBUFSRX, i.e after
> immediately after below line. #define DEBUG_INT_STATE 0   /*
> Internal network state changes */

Done 

> >  #define IPPROTO_ICMP1  /* Internet Control Message
> > Protocol*/ #define IPPROTO_UDP  17  /* User
> > Datagram Protocol   */ +#define IPPROTO_TCP
> > 6   /* Transmission Control Protocol*/  
> 
> Better to sort IPPROTO in ascending order

But, but protocol alphabetical order is so much prettier!! (Done in
numerical order). 

> >  /* 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);  
> 
Why do you need this change in the set_udp_header? 

This is a shim to bridge between the original udp to ip procedure call
and the extra parameters in the enhanced ip procedure call to the ip
layer for TCP.

The original udp call is unchanged, because I did not want a
change to a procedure call to ripple through many applications.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Calvin Johnson]

> -Original Message-
> From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of
> d...@synoia.com
> Sent: Thursday, March 8, 2018 10:14 AM
> To: duncanch...@yahoo.com
> Cc: Duncan Hare ; Joe Hershberger
> ; u-boot@lists.denx.de
> Subject: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot



> diff --git a/include/net.h b/include/net.h
> index 455b48f6c7..7e5f5a6a5b 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 */

It would be good to have this cosmetic change into a separate patch. 

>  /*
>   *   The number of receive packet buffers, and the required packet buffer
>   *   alignment in memory.
>   *
> + *   The number 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 12  /* For TCP */
> +#endif
> +

IMO, better place for this definition and associated explanation would be above 
the comment
describing PKTBUFSRX, i.e after immediately after below line.
#define DEBUG_INT_STATE 0   /* Internal network state changes */

>  #ifdef CONFIG_SYS_RX_ETH_BUFFER
>  # define PKTBUFSRX   CONFIG_SYS_RX_ETH_BUFFER
>  #else
> @@ -354,6 +363,7 @@ 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*/

Better to sort IPPROTO in ascending order

> 
>  /*
>   *   Internet Protocol (IP) header.
> @@ -596,10 +606,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);

Why do you need this change in the set_udp_header? 

>  /**
>   * compute_ip_checksum() - Compute IP checksum
>   *
> @@ -670,6 +680,9 @@ 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);
> +

Place it above comments for net_send_udp_packet.

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

Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Calvin Johnson]

Hi Duncan,

> -Original Message-
> From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Joe
> Hershberger
> Sent: Friday, March 9, 2018 12:23 AM
> To: Duncan Hare <d...@synoia.com>
> Cc: Duncan Hare <dh...@synoia.com>; Joe Hershberger
> <joe.hershber...@ni.com>; Duncan Hare <duncanch...@yahoo.com>; u-boot
> <u-boot@lists.denx.de>
> Subject: Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot
> 
> Hi Duncan,
> 
> Still some issues, but getting closer to parsable.
> 
> The subject of this should be something like, "net: Adjust UDP
> implementation to prepare for TCP support"
> 
> On Wed, Mar 7, 2018 at 10:43 PM,  <d...@synoia.com> wrote:
> > From: Duncan Hare <duncanch...@yahoo.com>
> >
> >>>>>>
> > <<<<
> 
> I think these are coming from your commit log. You should remove them.
> 
> >
> > cover-letter:
> 
> You need to use the exact tags in the documentation. That means that
> you need to capitalize the 'C'.

Whenever, I like to see how my patches appear in mail, I would send it ONLY to 
my mail id
and then compare with patches in the mailing list from experts .
Once I'm confident that my patches are following the guidelines, I'll send them 
to mailing list. 
Still, there can be mistakes , but most of them will be resolved by my own 
review.

Regards
Calvin

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

Re: [U-Boot] [PATCH v8 1/3] Adding TCP and wget into u-boot

[Joe Hershberger]

Hi Duncan,

Still some issues, but getting closer to parsable.

The subject of this should be something like, "net: Adjust UDP
implementation to prepare for TCP support"

On Wed, Mar 7, 2018 at 10:43 PM,   wrote:
> From: Duncan Hare 
>
>>
> 

I think these are coming from your commit log. You should remove them.

>
> cover-letter:

You need to use the exact tags in the documentation. That means that
you need to capitalize the 'C'.

> Why netboot:

The first line here is the subject for the series, so you should have
the line that you used to have as the patches' titles in v7.

> Central management, including logs and change control,
> coupled with with enhanced security and unauthorized
> change detection and remediation by exposing a
> small attack surface.
>
> 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.
>
> UDP is ideally suited to fast single packet exchanges,
> inquiry/response, for example dns, becuse of the lack of
> connection overhead.
>
> UDP as a file transport mechanism is slow, even in low
> latency networks, because file transfer with udp requires
> poll/response mechanism to provide transfer integrity.
>
> In networks with large latency, for example: the internet,
> UDP is even slower. 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.
>
> In our testing we have reduce kernel transmission time to
> around 0.4 seconds for a 4Mbyte kernel, with a 100 Mbps
> downlink.
>
> Why http and wget:
>
> HTTP is the most efficient file retrieval protocol in common
> use. The client send a single request, after TCP connection,
> to receive a file of any length.
>
> WGET is the application which implements http file transfer
> outside browsers as a file transfer protocol. Versions of
> wget exists on many operating systems.
> END
>

This stuff is great for a cover letter. I would recommend that you
also include the relevant TCP section and wget section in those
patches.

> Signed-off-by: Duncan Hare 
> ---
>
> Changed in this patch:
> include/net.h
> net/net.c

This isn't really needed since the patch already lists the files
changed... see below about 12 lines.

>
> Added a protocol parameter to ip packet sending in net.c
> Added UDP protocol for current applications to minimize
> code changes to existing net apps.

This stuff should not be in a patman tag. The default (untagged) is
what ends up in the commit message. In other words, don't put this
stuff in the "Commit-notes:" tag. In patman, "notes" means that you
are providing notes to the reviewer that you do not want to end up in
git.

>
> All the code is new, and not copied from any source.
>
>
> Changes in v8:
> Initial changes for adding TCP
>
>  include/net.h | 25 +++--
>  net/net.c | 52 ++--
>  net/ping.c|  9 ++---
>  3 files changed, 55 insertions(+), 31 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 455b48f6c7..7e5f5a6a5b 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 number 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