[PATCH v5] net: tftp: Add client support for RFC 7440

[Suneel Garapati]

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 at 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 at 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 at gmail.com>
> > Reviewed-by: Marek Vasut <marex at denx.de>
>
> Applied to u-boot/master, thanks!
>
> --
> Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tftp-1.pcapng
Type: application/x-pcapng
Size: 17324 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210130/7e4d3fc3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tftp-tftpput-1.pcapng
Type: application/x-pcapng
Size: 17248 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210130/7e4d3fc3/attachment-0001.bin>