Message ID | 20200718203146.317254-1-rfried.dev@gmail.com |
---|---|
State | Accepted |
Commit | cc6b87ecaa96325577a8fafabc0d5972b816bc6c |
Delegated to: | Tom Rini |
Headers | show |
Series | [v5] net: tftp: Add client support for RFC 7440 | expand |
On Sat, Jul 18, 2020 at 11:31:46PM +0300, 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 <rfried.dev@gmail.com> > Reviewed-by: Marek Vasut <marex@denx.de> Applied to u-boot/master, thanks!
Hello Ramon, With TFTP window size as default 1 and enabling TFTPPUT config option results in tftpboot command failure. Attached the pcap files for with and without TFTPPUT enabled. Both captures are the same except that the TFTPPUT option enables ICMP handler and for the response from the server triggers a restart of netloop operation and eventually fails as below. > tftpboot 0x30000000 192.168.1.1:Image Waiting for RPM0 LMAC0 link status... 10G_R [10G] Using rvu_pf#0 device TFTP from server 192.168.1.1; our IP address is 192.168.1.16 Filename 'Image'. Load address: 0x30000000 Loading: ################################################## 15.8 MiB 787.1 KiB/s done TFTP server died; starting again > I see the code issue as below on net/tftp.c [v2020.10] – /* * Acknowledge the block just received, which will prompt * the remote for the next one. */ if (tftp_cur_block == tftp_next_ack) { tftp_send(); tftp_next_ack += tftp_windowsize; } if (len < tftp_block_size) { //if (tftp_windowsize > 1) [Hack in use for now to work around this issue] tftp_send(); [ This causes extra ACK packet send with same block number and causes server to respond with ICMP error] tftp_complete(); } I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. I tried with all latest commits on net/tftp.c on top of v2020.10 and still see the issue. This change is introduced in commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c Author: Ramon Fried <rfried.dev@gmail.com> Date: Sat Jul 18 23:31:46 2020 +0300 net: tftp: Add client support for RFC 7440 Add support for RFC 7440: "TFTP Windowsize Option". Reverting this commit on v2020.10 also fixes the issue. I would like to know if this extra tftp_send is needed at all or only for window size > 1 Regards, Suneel On Wed, Aug 5, 2020 at 1:28 PM Tom Rini <trini@konsulko.com> wrote: > > On Sat, Jul 18, 2020 at 11:31:46PM +0300, 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 <rfried.dev@gmail.com> > > Reviewed-by: Marek Vasut <marex@denx.de> > > Applied to u-boot/master, thanks! > > -- > Tom
On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > Hello Ramon, > > With TFTP window size as default 1 and enabling TFTPPUT config option > results in tftpboot command failure. > > Attached the pcap files for with and without TFTPPUT enabled. > Both captures are the same except that the TFTPPUT option enables ICMP handler > and for the response from the server triggers a restart of netloop > operation and eventually fails as below. > > > tftpboot 0x30000000 192.168.1.1:Image > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > Using rvu_pf#0 device > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > Filename 'Image'. Load address: 0x30000000 > Loading: ################################################## 15.8 MiB > 787.1 KiB/s > done > TFTP server died; starting again > > > > I see the code issue as below on net/tftp.c [v2020.10] – > /* > * Acknowledge the block just received, which will prompt > * the remote for the next one. > */ > if (tftp_cur_block == tftp_next_ack) { > tftp_send(); > tftp_next_ack += tftp_windowsize; > } > > if (len < tftp_block_size) { > //if (tftp_windowsize > 1) [Hack in use for > now to work around this issue] > tftp_send(); [ This > causes extra ACK packet send with same block number and causes server > to respond with ICMP error] > tftp_complete(); > } > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > I tried with all latest commits on net/tftp.c on top of v2020.10 and > still see the issue. > > This change is introduced in > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > Author: Ramon Fried <rfried.dev@gmail.com> > Date: Sat Jul 18 23:31:46 2020 +0300 > > net: tftp: Add client support for RFC 7440 > > Add support for RFC 7440: "TFTP Windowsize Option". > > Reverting this commit on v2020.10 also fixes the issue. > > I would like to know if this extra tftp_send is needed at all or only > for window size > 1 > > Regards, > Suneel Thanks for spotting this, I'll check it out.
On Sat, Jan 30, 2021 at 11:26 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > Hello Ramon, > > > > With TFTP window size as default 1 and enabling TFTPPUT config option > > results in tftpboot command failure. > > > > Attached the pcap files for with and without TFTPPUT enabled. > > Both captures are the same except that the TFTPPUT option enables ICMP handler > > and for the response from the server triggers a restart of netloop > > operation and eventually fails as below. > > > > > tftpboot 0x30000000 192.168.1.1:Image > > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > > Using rvu_pf#0 device > > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > > Filename 'Image'. Load address: 0x30000000 > > Loading: ################################################## 15.8 MiB > > 787.1 KiB/s > > done > > TFTP server died; starting again > > > > > > > I see the code issue as below on net/tftp.c [v2020.10] – > > /* > > * Acknowledge the block just received, which will prompt > > * the remote for the next one. > > */ > > if (tftp_cur_block == tftp_next_ack) { > > tftp_send(); > > tftp_next_ack += tftp_windowsize; > > } > > > > if (len < tftp_block_size) { > > //if (tftp_windowsize > 1) [Hack in use for > > now to work around this issue] > > tftp_send(); [ This > > causes extra ACK packet send with same block number and causes server > > to respond with ICMP error] > > tftp_complete(); > > } > > > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > > I tried with all latest commits on net/tftp.c on top of v2020.10 and > > still see the issue. > > > > This change is introduced in > > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > > Author: Ramon Fried <rfried.dev@gmail.com> > > Date: Sat Jul 18 23:31:46 2020 +0300 > > > > net: tftp: Add client support for RFC 7440 > > > > Add support for RFC 7440: "TFTP Windowsize Option". > > > > Reverting this commit on v2020.10 also fixes the issue. > > > > I would like to know if this extra tftp_send is needed at all or only > > for window size > 1 RFC7440 is not supported by most TFTP Servers, when adding support for this feature I didn't even consider TFTPPUT. I think that the best thing to do is to only support RFC7440 on TFTPGET. In this case we should remove the tftp_send() when sending a file. I will create a fix, would you mind testing to see if it works for you ? Thanks, Ramon. > > > > Regards, > > Suneel > Thanks for spotting this, I'll check it out.
On Mon, Feb 1, 2021 at 2:27 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > On Sat, Jan 30, 2021 at 11:26 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > Hello Ramon, > > > > > > With TFTP window size as default 1 and enabling TFTPPUT config option > > > results in tftpboot command failure. > > > > > > Attached the pcap files for with and without TFTPPUT enabled. > > > Both captures are the same except that the TFTPPUT option enables ICMP handler > > > and for the response from the server triggers a restart of netloop > > > operation and eventually fails as below. > > > > > > > tftpboot 0x30000000 192.168.1.1:Image > > > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > > > Using rvu_pf#0 device > > > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > > > Filename 'Image'. Load address: 0x30000000 > > > Loading: ################################################## 15.8 MiB > > > 787.1 KiB/s > > > done > > > TFTP server died; starting again > > > > > > > > > > I see the code issue as below on net/tftp.c [v2020.10] – > > > /* > > > * Acknowledge the block just received, which will prompt > > > * the remote for the next one. > > > */ > > > if (tftp_cur_block == tftp_next_ack) { > > > tftp_send(); > > > tftp_next_ack += tftp_windowsize; > > > } > > > > > > if (len < tftp_block_size) { > > > //if (tftp_windowsize > 1) [Hack in use for > > > now to work around this issue] > > > tftp_send(); [ This > > > causes extra ACK packet send with same block number and causes server > > > to respond with ICMP error] > > > tftp_complete(); > > > } > > > > > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > > > I tried with all latest commits on net/tftp.c on top of v2020.10 and > > > still see the issue. > > > > > > This change is introduced in > > > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > > > Author: Ramon Fried <rfried.dev@gmail.com> > > > Date: Sat Jul 18 23:31:46 2020 +0300 > > > > > > net: tftp: Add client support for RFC 7440 > > > > > > Add support for RFC 7440: "TFTP Windowsize Option". > > > > > > Reverting this commit on v2020.10 also fixes the issue. > > > > > > I would like to know if this extra tftp_send is needed at all or only > > > for window size > 1 > RFC7440 is not supported by most TFTP Servers, when adding support for > this feature I didn't even consider TFTPPUT. > I think that the best thing to do is to only support RFC7440 on TFTPGET. > In this case we should remove the tftp_send() when sending a file. > I will create a fix, would you mind testing to see if it works for you ? > Thanks, > Ramon. Yes, I can test the fix. Thanks, Suneel > > > > > > Regards, > > > Suneel > > Thanks for spotting this, I'll check it out.
On Tue, Feb 2, 2021 at 6:02 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > On Mon, Feb 1, 2021 at 2:27 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > On Sat, Jan 30, 2021 at 11:26 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > > > On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > > > Hello Ramon, > > > > > > > > With TFTP window size as default 1 and enabling TFTPPUT config option > > > > results in tftpboot command failure. > > > > > > > > Attached the pcap files for with and without TFTPPUT enabled. > > > > Both captures are the same except that the TFTPPUT option enables ICMP handler > > > > and for the response from the server triggers a restart of netloop > > > > operation and eventually fails as below. > > > > > > > > > tftpboot 0x30000000 192.168.1.1:Image > > > > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > > > > Using rvu_pf#0 device > > > > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > > > > Filename 'Image'. Load address: 0x30000000 > > > > Loading: ################################################## 15.8 MiB > > > > 787.1 KiB/s > > > > done > > > > TFTP server died; starting again > > > > > > > > > > > > > I see the code issue as below on net/tftp.c [v2020.10] – > > > > /* > > > > * Acknowledge the block just received, which will prompt > > > > * the remote for the next one. > > > > */ > > > > if (tftp_cur_block == tftp_next_ack) { > > > > tftp_send(); > > > > tftp_next_ack += tftp_windowsize; > > > > } > > > > > > > > if (len < tftp_block_size) { > > > > //if (tftp_windowsize > 1) [Hack in use for > > > > now to work around this issue] > > > > tftp_send(); [ This > > > > causes extra ACK packet send with same block number and causes server > > > > to respond with ICMP error] > > > > tftp_complete(); > > > > } > > > > > > > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > > > > I tried with all latest commits on net/tftp.c on top of v2020.10 and > > > > still see the issue. > > > > > > > > This change is introduced in > > > > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > > > > Author: Ramon Fried <rfried.dev@gmail.com> > > > > Date: Sat Jul 18 23:31:46 2020 +0300 > > > > > > > > net: tftp: Add client support for RFC 7440 > > > > > > > > Add support for RFC 7440: "TFTP Windowsize Option". > > > > > > > > Reverting this commit on v2020.10 also fixes the issue. > > > > > > > > I would like to know if this extra tftp_send is needed at all or only > > > > for window size > 1 > > RFC7440 is not supported by most TFTP Servers, when adding support for > > this feature I didn't even consider TFTPPUT. > > I think that the best thing to do is to only support RFC7440 on TFTPGET. > > In this case we should remove the tftp_send() when sending a file. > > I will create a fix, would you mind testing to see if it works for you ? > > Thanks, > > Ramon. > Yes, I can test the fix. > > Thanks, > Suneel > > > > > > > > Regards, > > > > Suneel > > > Thanks for spotting this, I'll check it out. Hey Suneel Now when I dived deeper, I just figured out that __tftpboot__ doesn't work for you with window size = 1. This is very strange, my current setup is working also with this configuration. Can you please check which TFTP server you're using on the other end ? Thanks, Ramon.
On Tue, Feb 2, 2021 at 11:46 AM Ramon Fried <rfried.dev@gmail.com> wrote: > > On Tue, Feb 2, 2021 at 6:02 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > On Mon, Feb 1, 2021 at 2:27 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > > > On Sat, Jan 30, 2021 at 11:26 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > > > > > On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > > > > > Hello Ramon, > > > > > > > > > > With TFTP window size as default 1 and enabling TFTPPUT config option > > > > > results in tftpboot command failure. > > > > > > > > > > Attached the pcap files for with and without TFTPPUT enabled. > > > > > Both captures are the same except that the TFTPPUT option enables ICMP handler > > > > > and for the response from the server triggers a restart of netloop > > > > > operation and eventually fails as below. > > > > > > > > > > > tftpboot 0x30000000 192.168.1.1:Image > > > > > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > > > > > Using rvu_pf#0 device > > > > > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > > > > > Filename 'Image'. Load address: 0x30000000 > > > > > Loading: ################################################## 15.8 MiB > > > > > 787.1 KiB/s > > > > > done > > > > > TFTP server died; starting again > > > > > > > > > > > > > > > > I see the code issue as below on net/tftp.c [v2020.10] – > > > > > /* > > > > > * Acknowledge the block just received, which will prompt > > > > > * the remote for the next one. > > > > > */ > > > > > if (tftp_cur_block == tftp_next_ack) { > > > > > tftp_send(); > > > > > tftp_next_ack += tftp_windowsize; > > > > > } > > > > > > > > > > if (len < tftp_block_size) { > > > > > //if (tftp_windowsize > 1) [Hack in use for > > > > > now to work around this issue] > > > > > tftp_send(); [ This > > > > > causes extra ACK packet send with same block number and causes server > > > > > to respond with ICMP error] > > > > > tftp_complete(); > > > > > } > > > > > > > > > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > > > > > I tried with all latest commits on net/tftp.c on top of v2020.10 and > > > > > still see the issue. > > > > > > > > > > This change is introduced in > > > > > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > > > > > Author: Ramon Fried <rfried.dev@gmail.com> > > > > > Date: Sat Jul 18 23:31:46 2020 +0300 > > > > > > > > > > net: tftp: Add client support for RFC 7440 > > > > > > > > > > Add support for RFC 7440: "TFTP Windowsize Option". > > > > > > > > > > Reverting this commit on v2020.10 also fixes the issue. > > > > > > > > > > I would like to know if this extra tftp_send is needed at all or only > > > > > for window size > 1 > > > RFC7440 is not supported by most TFTP Servers, when adding support for > > > this feature I didn't even consider TFTPPUT. > > > I think that the best thing to do is to only support RFC7440 on TFTPGET. > > > In this case we should remove the tftp_send() when sending a file. > > > I will create a fix, would you mind testing to see if it works for you ? > > > Thanks, > > > Ramon. > > Yes, I can test the fix. > > > > Thanks, > > Suneel > > > > > > > > > > Regards, > > > > > Suneel > > > > Thanks for spotting this, I'll check it out. > Hey Suneel > Now when I dived deeper, I just figured out that __tftpboot__ doesn't > work for you with window size = 1. > This is very strange, my current setup is working also with this configuration. > Can you please check which TFTP server you're using on the other end ? Did it work with/without CMD_TFTPPUT config option? For me it fails only when this option is enabled. What is the response on your setup from the server for the extra ack packet sent on the last block? TFTP server is a tftpd-hpa service on ubuntu 18.04 machine with basic setup. > > Thanks, > Ramon.
On Wed, Feb 3, 2021 at 12:23 AM Suneel Garapati <suneelglinux@gmail.com> wrote: > > On Tue, Feb 2, 2021 at 11:46 AM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > On Tue, Feb 2, 2021 at 6:02 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > On Mon, Feb 1, 2021 at 2:27 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > > > > > On Sat, Jan 30, 2021 at 11:26 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > > > > > > > > On Sat, Jan 30, 2021 at 10:39 PM Suneel Garapati <suneelglinux@gmail.com> wrote: > > > > > > > > > > > > Hello Ramon, > > > > > > > > > > > > With TFTP window size as default 1 and enabling TFTPPUT config option > > > > > > results in tftpboot command failure. > > > > > > > > > > > > Attached the pcap files for with and without TFTPPUT enabled. > > > > > > Both captures are the same except that the TFTPPUT option enables ICMP handler > > > > > > and for the response from the server triggers a restart of netloop > > > > > > operation and eventually fails as below. > > > > > > > > > > > > > tftpboot 0x30000000 192.168.1.1:Image > > > > > > Waiting for RPM0 LMAC0 link status... 10G_R [10G] > > > > > > Using rvu_pf#0 device > > > > > > TFTP from server 192.168.1.1; our IP address is 192.168.1.16 > > > > > > Filename 'Image'. Load address: 0x30000000 > > > > > > Loading: ################################################## 15.8 MiB > > > > > > 787.1 KiB/s > > > > > > done > > > > > > TFTP server died; starting again > > > > > > > > > > > > > > > > > > > I see the code issue as below on net/tftp.c [v2020.10] – > > > > > > /* > > > > > > * Acknowledge the block just received, which will prompt > > > > > > * the remote for the next one. > > > > > > */ > > > > > > if (tftp_cur_block == tftp_next_ack) { > > > > > > tftp_send(); > > > > > > tftp_next_ack += tftp_windowsize; > > > > > > } > > > > > > > > > > > > if (len < tftp_block_size) { > > > > > > //if (tftp_windowsize > 1) [Hack in use for > > > > > > now to work around this issue] > > > > > > tftp_send(); [ This > > > > > > causes extra ACK packet send with same block number and causes server > > > > > > to respond with ICMP error] > > > > > > tftp_complete(); > > > > > > } > > > > > > > > > > > > I couldn’t try with tftp_windowsize > 1 as the test servers don’t support. > > > > > > I tried with all latest commits on net/tftp.c on top of v2020.10 and > > > > > > still see the issue. > > > > > > > > > > > > This change is introduced in > > > > > > commit cc6b87ecaa96325577a8fafabc0d5972b816bc6c > > > > > > Author: Ramon Fried <rfried.dev@gmail.com> > > > > > > Date: Sat Jul 18 23:31:46 2020 +0300 > > > > > > > > > > > > net: tftp: Add client support for RFC 7440 > > > > > > > > > > > > Add support for RFC 7440: "TFTP Windowsize Option". > > > > > > > > > > > > Reverting this commit on v2020.10 also fixes the issue. > > > > > > > > > > > > I would like to know if this extra tftp_send is needed at all or only > > > > > > for window size > 1 > > > > RFC7440 is not supported by most TFTP Servers, when adding support for > > > > this feature I didn't even consider TFTPPUT. > > > > I think that the best thing to do is to only support RFC7440 on TFTPGET. > > > > In this case we should remove the tftp_send() when sending a file. > > > > I will create a fix, would you mind testing to see if it works for you ? > > > > Thanks, > > > > Ramon. > > > Yes, I can test the fix. > > > > > > Thanks, > > > Suneel > > > > > > > > > > > > Regards, > > > > > > Suneel > > > > > Thanks for spotting this, I'll check it out. > > Hey Suneel > > Now when I dived deeper, I just figured out that __tftpboot__ doesn't > > work for you with window size = 1. > > This is very strange, my current setup is working also with this configuration. > > Can you please check which TFTP server you're using on the other end ? > > Did it work with/without CMD_TFTPPUT config option? > For me it fails only when this option is enabled. > What is the response on your setup from the server for the extra ack > packet sent on the last block? > TFTP server is a tftpd-hpa service on ubuntu 18.04 machine with basic setup. > > > > > Thanks, > > Ramon. OK. Found the problem. sending a patch.
On 18/07/20, 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. this commit breaks my TFTP Setup. I run into lot of timeouts and most of the times I can't load the kernel or the dtb. Using ethernet@5b040000 device TFTP from server 192.168.100.99; our IP address is 192.168.100.96 Filename 'Image'. Load address: 0x80280000 Loading: #T T T T T T T On bisecting between 2020.07 and 2020.10 I stumbled across this commit. reverting it fix the issue immediately. Some clue why the default windowsize set 1 breaks it for me? Best regards, Oliver
On 17/02/21, Oliver Graute wrote: > On 18/07/20, 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. > > this commit breaks my TFTP Setup. I run into lot of timeouts and most of > the times I can't load the kernel or the dtb. > > Using ethernet@5b040000 device > TFTP from server 192.168.100.99; our IP address is 192.168.100.96 > Filename 'Image'. > Load address: 0x80280000 > Loading: #T T T T T T T > > On bisecting between 2020.07 and 2020.10 I stumbled across this commit. > reverting it fix the issue immediately. > > Some clue why the default windowsize set 1 breaks it for me? ok found the patch that fixed the issue myself https://lists.denx.de/pipermail/u-boot/2021-February/439845.html Best regards, Oliver
diff --git a/README b/README index 2384966a39..2ebf664848 100644 --- a/README +++ b/README @@ -3473,6 +3473,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 c05b7b5532..84e970bec1 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -5,7 +5,6 @@ * Copyright 2011 Comelit Group SpA, * Luca Ceresoli <luca.ceresoli@comelit.it> */ - #include <common.h> #include <command.h> #include <efi_loader.h> @@ -98,6 +97,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; @@ -138,8 +143,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) { @@ -356,6 +372,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) + pkt += sprintf((char *)pkt, "windowsize%c%d%c", + 0, tftp_window_size_option, 0); len = pkt - xp; break; @@ -550,7 +574,17 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, (char *)pkt + i + 6, tftp_tsize); } #endif + if (strcasecmp((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 && tftp_state == STATE_OACK) { /* Get ready to send the first block */ @@ -564,7 +598,28 @@ 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++; + tftp_cur_block %= TFTP_SEQUENCE_SIZE; if (tftp_state == STATE_SEND_RRQ) debug("Server did not acknowledge any options!\n"); @@ -606,10 +661,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: @@ -683,6 +743,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); @@ -704,8 +768,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)) { @@ -801,7 +865,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 */