From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Brugger Date: Sat, 23 May 2020 19:39:58 +0200 Subject: [PATCH v4] net: tftp: Add client support for RFC 7440 In-Reply-To: <20200519192557.18075-1-rfried.dev@gmail.com> References: <20200519192557.18075-1-rfried.dev@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 19/05/2020 21:25, Ramon Fried wrote: > Add support for RFC 7440: "TFTP Windowsize Option". > > This optional feature allows the client and server > to negotiate a window size of consecutive blocks to send as an > alternative for replacing the single-block lockstep schema. > > windowsize can be defined statically during compilation by > setting CONFIG_TFTP_WINDOWSIZE, or defined in runtime by > setting an environment variable: "tftpwindowsize" > If not defined, the windowsize is set to 1, meaning that it > behaves as it was never defined. > > Choosing the appropriate windowsize depends on the specific > network topology, underlying NIC. > You should test various windowsize scenarios and see which > best work for you. > > Setting a windowsize too big can actually decreases performance. > > Signed-off-by: Ramon Fried > Reviewed-by: Marek Vasut > --- > v2: > * Don't send windowsize option on tftpput, as it's not implemented yet. > * Don't send NACK for every out of order block that arrives, one nack > is enough. > v3: > * Add option CONFIG_TFTP_WINDOWSIZE to kconfig with default 1. > * Fixed some spelling errors. > * Took assignment out of a loop. > * simplified variable increment. > v4: > * send ack for last packet, so the server can finish > the tranfer gracefully and not in timeout. > > README | 5 ++++ > net/Kconfig | 9 ++++++ > net/tftp.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++------ > 3 files changed, 87 insertions(+), 8 deletions(-) > > diff --git a/README b/README > index be9e6391d6..686474a2f1 100644 > --- a/README > +++ b/README > @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete): > downloads succeed with high packet loss rates, or with > unreliable TFTP servers or client hardware. > > + tftpwindowsize - if this is set, the value is used for TFTP's > + window size as described by RFC 7440. > + This means the count of blocks we can receive before > + sending ack to server. > + > vlan - When set to a value < 4095 the traffic over > Ethernet is encapsulated/received over 802.1q > VLAN tagged frames. > diff --git a/net/Kconfig b/net/Kconfig > index ac6d0cf8a6..7916ae305f 100644 > --- a/net/Kconfig > +++ b/net/Kconfig > @@ -49,4 +49,13 @@ config TFTP_BLOCKSIZE > almost-MTU block sizes. > You can also activate CONFIG_IP_DEFRAG to set a larger block. > > +config TFTP_WINDOWSIZE > + int "TFTP window size" > + default 1 > + help > + Default TFTP window size. > + RFC7440 defines an optional window size of transmits, > + before an ack response is required. > + The default TFTP implementation implies a window size of 1. > + > endif # if NET > diff --git a/net/tftp.c b/net/tftp.c > index be24e63075..72d23e1574 100644 > --- a/net/tftp.c > +++ b/net/tftp.c > @@ -5,7 +5,6 @@ > * Copyright 2011 Comelit Group SpA, > * Luca Ceresoli > */ > - > #include > #include > #include > @@ -95,6 +94,12 @@ static int tftp_tsize; > /* The number of hashes we printed */ > static short tftp_tsize_num_hash; > #endif > +/* The window size negotiated */ > +static ushort tftp_windowsize; > +/* Next block to send ack to */ > +static ushort tftp_next_ack; > +/* Last nack block we send */ > +static ushort tftp_last_nack; > #ifdef CONFIG_CMD_TFTPPUT > /* 1 if writing, else 0 */ > static int tftp_put_active; > @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN]; > * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file) > */ > > +/* When windowsize is defined to 1, > + * tftp behaves the same way as it was > + * never declared > + */ > +#ifdef CONFIG_TFTP_WINDOWSIZE > +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE > +#else > +#define TFTP_WINDOWSIZE 1 > +#endif > + > static unsigned short tftp_block_size = TFTP_BLOCK_SIZE; > static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE; > +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE; > > static inline int store_block(int block, uchar *src, unsigned int len) > { > @@ -348,6 +364,14 @@ static void tftp_send(void) > /* try for more effic. blk size */ > pkt += sprintf((char *)pkt, "blksize%c%d%c", > 0, tftp_block_size_option, 0); > + > + /* try for more effic. window size. > + * Implemented only for tftp get. > + * Don't bother sending if it's 1 > + */ > + if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1) I think it makes more sense to check: if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ) Because I understand that the tftp_state will change while the tftp_window_size_option is set or at compile time or through the environment. So we can save the check of the tftp_state if we have the default value. Regards, Matthias > + pkt += sprintf((char *)pkt, "windowsize%c%d%c", > + 0, tftp_window_size_option, 0); > len = pkt - xp; > break; > > @@ -500,7 +524,18 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > (char *)pkt + i + 6, tftp_tsize); > } > #endif > + if (strcmp((char *)pkt + i, "windowsize") == 0) { > + tftp_windowsize = > + simple_strtoul((char *)pkt + i + 11, > + NULL, 10); > + debug("windowsize = %s, %d\n", > + (char *)pkt + i + 11, tftp_windowsize); > + } > + > } > + > + tftp_next_ack = tftp_windowsize; > + > #ifdef CONFIG_CMD_TFTPPUT > if (tftp_put_active) { > /* Get ready to send the first block */ > @@ -514,12 +549,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > if (len < 2) > return; > len -= 2; > - tftp_cur_block = ntohs(*(__be16 *)pkt); > + > + if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) { > + debug("Received unexpected block: %d, expected: %d\n", > + ntohs(*(__be16 *)pkt), > + (ushort)(tftp_cur_block + 1)); > + /* > + * If one packet is dropped most likely > + * all other buffers in the window > + * that will arrive will cause a sending NACK. > + * This just overwellms the server, let's just send one. > + */ > + if (tftp_last_nack != tftp_cur_block) { > + tftp_send(); > + tftp_last_nack = tftp_cur_block; > + tftp_next_ack = (ushort)(tftp_cur_block + > + tftp_windowsize); > + } > + break; > + } > + > + tftp_cur_block++; > > update_block_number(); > > if (tftp_state == STATE_SEND_RRQ) > - debug("Server did not acknowledge timeout option!\n"); > + debug("Server did not acknowledge the options!\n"); > > if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK || > tftp_state == STATE_RECV_WRQ) { > @@ -557,10 +612,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, > * Acknowledge the block just received, which will prompt > * the remote for the next one. > */ > - tftp_send(); > + if (tftp_cur_block == tftp_next_ack) { > + tftp_send(); > + tftp_next_ack += tftp_windowsize; > + } > > - if (len < tftp_block_size) > + if (len < tftp_block_size) { > + tftp_send(); > tftp_complete(); > + } > break; > > case TFTP_ERROR: > @@ -634,6 +694,10 @@ void tftp_start(enum proto_t protocol) > if (ep != NULL) > tftp_block_size_option = simple_strtol(ep, NULL, 10); > > + ep = env_get("tftpwindowsize"); > + if (ep != NULL) > + tftp_window_size_option = simple_strtol(ep, NULL, 10); > + > ep = env_get("tftptimeout"); > if (ep != NULL) > timeout_ms = simple_strtol(ep, NULL, 10); > @@ -655,8 +719,8 @@ void tftp_start(enum proto_t protocol) > } > #endif > > - debug("TFTP blocksize = %i, timeout = %ld ms\n", > - tftp_block_size_option, timeout_ms); > + debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n", > + tftp_block_size_option, tftp_window_size_option, timeout_ms); > > tftp_remote_ip = net_server_ip; > if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) { > @@ -752,7 +816,8 @@ void tftp_start(enum proto_t protocol) > tftp_our_port = simple_strtol(ep, NULL, 10); > #endif > tftp_cur_block = 0; > - > + tftp_windowsize = 1; > + tftp_last_nack = 0; > /* zero out server ether in case the server ip has changed */ > memset(net_server_ethaddr, 0, 6); > /* Revert tftp_block_size to dflt */ >