[U-Boot] [PATCH v4] net: TFTP: variables cleanup and addition

[U-Boot] [PATCH v4] net: TFTP: variables cleanup and addition

[Albert ARIBAUD]

TFTP source and destination port variable names are
'tftpsrcp' and 'tftpdstp' in the code, but 'tftpsrcport'
and 'tftpdstport' in the README file. Fix the README.

Add environment variable 'tftptimeoutcountmax'. As per the
comments about the global variable tftp_timeout_count_max,
make sure tftptimeoutcountmax is nonnegative.

Introduce configuration option CONFIG_NET_TFTP_VARS,
which controls whether environment variables tftpblocksize,
tftptimeout, and tftptimoueoutcountmax are read by the TFTP
client code. CONFIG_NET_TFTP_VARS defaults to y but can be
set to n by targets with to tight size contraints.

Make bf527-ezkit set CONFIG_NET_TFTP_VARS to n to keep the
target size below limit.
---
Changes in v4:
- fixed tftp{src,dst}port name typo in README
- added control option CONFIG_NET_TFTP_VARS (default y)
- made bf527-ezkit set option to n

Changes in v3:
- fixed mixup between tctp_timeout_count_max and timeout_count_max.

Changes in v2:
- fixed env var name in commit message

 README                        | 12 ++++++++++--
 configs/bf527-ezkit_defconfig |  1 +
 net/Kconfig                   | 10 ++++++++++
 net/tftp.c                    | 17 +++++++++++++++--
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/README b/README
index c22b60b..57f25eb 100644
--- a/README
+++ b/README
@@ -5456,10 +5456,10 @@ List of environment variables (most likely not complete):
 		  unset, then it will be made silent if the U-Boot console
 		  is silent.
 
-  tftpsrcport	- If this is set, the value is used for TFTP's
+  tftpsrcp	- If this is set, the value is used for TFTP's
 		  UDP source port.
 
-  tftpdstport	- If this is set, the value is used for TFTP's UDP
+  tftpdstp	- If this is set, the value is used for TFTP's UDP
 		  destination port instead of the Well Know Port 69.
 
   tftpblocksize - Block size to use for TFTP transfers; if not set,
@@ -5473,6 +5473,14 @@ List of environment variables (most likely not complete):
 		  faster in networks with high packet loss rates or
 		  with unreliable TFTP servers.
 
+  tftptimeoutcountmax	- maximum count of TFTP timeouts (no
+		  unit, minimum value = 0). Defines how many timeouts
+		  can happen during a single file transfer before that
+		  transfer is aborted. The default is 10, and 0 means
+		  'no timeouts allowed'. Increasing this value may help
+		  downloads succeed with high packet loss rates, or with
+		  unreliable TFTP servers or client hardware.
+
   vlan		- When set to a value < 4095 the traffic over
 		  Ethernet is encapsulated/received over 802.1q
 		  VLAN tagged frames.
diff --git a/configs/bf527-ezkit_defconfig b/configs/bf527-ezkit_defconfig
index 2e75225..0cc81cc 100644
--- a/configs/bf527-ezkit_defconfig
+++ b/configs/bf527-ezkit_defconfig
@@ -4,3 +4,4 @@ CONFIG_TARGET_BF527_EZKIT=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_SPI_FLASH=y
 CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED=y
+CONFIG_NET_TFTP_VARS=n
diff --git a/net/Kconfig b/net/Kconfig
index 77a2f7e..a44a783 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -22,4 +22,14 @@ config NETCONSOLE
 	  Support the 'nc' input/output device for networked console.
 	  See README.NetConsole for details.
 
+config NET_TFTP_VARS
+	bool "Control TFTP timeout and count through environment"
+	default y
+	help
+	  If set, allows controlling the TFTP timeout through the
+	  environment variable tftptimeout, and the TFTP maximum
+	  timeout count through the variable tftptimeoutcountmax.
+	  If unset, timeout and maximum are hard-defined as 1 second
+	  and 10 timouts per TFTP transfer.
+
 endif   # if NET
diff --git a/net/tftp.c b/net/tftp.c
index 1a51131..f2889fe 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -602,7 +602,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		}
 
 		tftp_prev_block = tftp_cur_block;
-		timeout_count_max = TIMEOUT_COUNT;
+		timeout_count_max = tftp_timeout_count_max;
 		net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
 
 		store_block(tftp_cur_block - 1, pkt + 2, len);
@@ -697,12 +697,14 @@ static void tftp_timeout_handler(void)
 
 void tftp_start(enum proto_t protocol)
 {
+#if CONFIG_NET_TFTP_VARS
 	char *ep;             /* Environment pointer */
 
 	/*
 	 * Allow the user to choose TFTP blocksize and timeout.
 	 * TFTP protocol has a minimal timeout of 1 second.
 	 */
+
 	ep = getenv("tftpblocksize");
 	if (ep != NULL)
 		tftp_block_size_option = simple_strtol(ep, NULL, 10);
@@ -717,6 +719,17 @@ void tftp_start(enum proto_t protocol)
 		timeout_ms = 1000;
 	}
 
+	ep = getenv("tftptimeoutcountmax");
+	if (ep != NULL)
+		tftp_timeout_count_max = simple_strtol(ep, NULL, 10);
+
+	if (tftp_timeout_count_max < 0) {
+		printf("TFTP timeout count max (%d ms) negative, set to 0\n",
+		       tftp_timeout_count_max);
+		tftp_timeout_count_max = 0;
+	}
+#endif
+
 	debug("TFTP blocksize = %i, timeout = %ld ms\n",
 	      tftp_block_size_option, timeout_ms);
 
@@ -842,7 +855,7 @@ void tftp_start_server(void)
 
 	puts("Loading: *\b");
 
-	timeout_count_max = TIMEOUT_COUNT;
+	timeout_count_max = tftp_timeout_count_max;
 	timeout_count = 0;
 	timeout_ms = TIMEOUT;
 	net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
-- 
2.1.4

[U-Boot] [PATCH v4] net: TFTP: variables cleanup and addition

[Albert ARIBAUD]

On Mon, 12 Oct 2015 00:02:57 +0200, Albert ARIBAUD (3ADEV)
<albert.aribaud@3adev.fr> wrote:
> TFTP source and destination port variable names are
> 'tftpsrcp' and 'tftpdstp' in the code, but 'tftpsrcport'
> and 'tftpdstport' in the README file. Fix the README.
> 
> Add environment variable 'tftptimeoutcountmax'. As per the
> comments about the global variable tftp_timeout_count_max,
> make sure tftptimeoutcountmax is nonnegative.
> 
> Introduce configuration option CONFIG_NET_TFTP_VARS,
> which controls whether environment variables tftpblocksize,
> tftptimeout, and tftptimoueoutcountmax are read by the TFTP
> client code. CONFIG_NET_TFTP_VARS defaults to y but can be
> set to n by targets with to tight size contraints.
> 
> Make bf527-ezkit set CONFIG_NET_TFTP_VARS to n to keep the
> target size below limit.
> ---

Joe, does it work for bf527-ezkit?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4] net: TFTP: variables cleanup and addition

[Joe Hershberger]

Hi Albert,

On Sun, Oct 25, 2015 at 5:29 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> On Mon, 12 Oct 2015 00:02:57 +0200, Albert ARIBAUD (3ADEV)
> <albert.aribaud@3adev.fr> wrote:
>> TFTP source and destination port variable names are
>> 'tftpsrcp' and 'tftpdstp' in the code, but 'tftpsrcport'
>> and 'tftpdstport' in the README file. Fix the README.
>>
>> Add environment variable 'tftptimeoutcountmax'. As per the
>> comments about the global variable tftp_timeout_count_max,
>> make sure tftptimeoutcountmax is nonnegative.
>>
>> Introduce configuration option CONFIG_NET_TFTP_VARS,
>> which controls whether environment variables tftpblocksize,
>> tftptimeout, and tftptimoueoutcountmax are read by the TFTP
>> client code. CONFIG_NET_TFTP_VARS defaults to y but can be
>> set to n by targets with to tight size contraints.
>>
>> Make bf527-ezkit set CONFIG_NET_TFTP_VARS to n to keep the
>> target size below limit.
>> ---
>
> Joe, does it work for bf527-ezkit?

Apparently it is OK to break these boards. It seems it is already
broken in the now-current master.

+(bf537-stamp) u-boot.bin exceeds file size limit:
+(bf537-stamp,bf527-ezkit-v2,bf527-ezkit,bf526-ezbrd)   limit:  262144 bytes
+(bf537-stamp)   actual: 263088 bytes
+(bf537-stamp)   excess: 944 bytes
+(bf537-stamp) make[1]: *** [u-boot.bin] Error 1
+(bf527-ezkit-v2,bf527-ezkit,bf526-ezbrd) u-boot.ldr exceeds file size limit:
+(bf527-ezkit-v2)   actual: 262752 bytes
+(bf527-ezkit-v2)   excess: 608 bytes
+(bf527-ezkit-v2,bf527-ezkit,bf526-ezbrd) make[1]: *** [u-boot.ldr] Error 1
+(bf527-ezkit)   actual: 262876 bytes
+(bf527-ezkit)   excess: 732 bytes
+(bf526-ezbrd)   actual: 263056 bytes
+(bf526-ezbrd)   excess: 912 bytes
+(bct-brettl2) bfin-elf-ld.bfd: u-boot section `.bss' will not fit in
region `ram'
+(bct-brettl2) bfin-elf-ld.bfd: region `ram' overflowed by 212 bytes

The only new regression with the patches I'm testing (this includes
your v4) is this:

+(bf538f-ezkit) bfin-elf-ld.bfd: region `ram' overflowed by 256 bytes

I'm tired of fighting for a byte here or there. I agree that we should
not just bloat needlessly, but these boards are simply too close to
the limit to be reasonable. I guess they will be broken frequently
until someone turns off some features on them.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v4] net: TFTP: variables cleanup and addition

[Joe Hershberger]

On Sun, Oct 11, 2015 at 5:02 PM, Albert ARIBAUD (3ADEV)
<albert.aribaud@3adev.fr> wrote:
> TFTP source and destination port variable names are
> 'tftpsrcp' and 'tftpdstp' in the code, but 'tftpsrcport'
> and 'tftpdstport' in the README file. Fix the README.
>
> Add environment variable 'tftptimeoutcountmax'. As per the
> comments about the global variable tftp_timeout_count_max,
> make sure tftptimeoutcountmax is nonnegative.
>
> Introduce configuration option CONFIG_NET_TFTP_VARS,
> which controls whether environment variables tftpblocksize,
> tftptimeout, and tftptimoueoutcountmax are read by the TFTP
> client code. CONFIG_NET_TFTP_VARS defaults to y but can be
> set to n by targets with to tight size contraints.
>
> Make bf527-ezkit set CONFIG_NET_TFTP_VARS to n to keep the
> target size below limit.

Applied to u-boot-net/master, thanks!
-Joe