[PATCHv10 14/15] net/lwip: replace original net commands with lwip

[Maxim Uvarov]

On Wed, 4 Oct 2023 at 08:12, Simon Glass <sjg at google.com> wrote:

> Hi Sean,
>
> On Tue, 3 Oct 2023 at 11:58, Sean Edmond <seanedmond at linux.microsoft.com>
> wrote:
> >
> >
> > On 2023-09-26 2:41 a.m., Maxim Uvarov wrote:
> > > Replace original commands: ping, tftp, dhcp and wget.
> > >
> > > Signed-off-by: Maxim Uvarov<maxim.uvarov at linaro.org>
> > > ---
> > >   boot/bootmeth_efi.c | 18 +++++++---
> > >   boot/bootmeth_pxe.c | 21 ++++++-----
> > >   cmd/net.c           | 86
> +++++----------------------------------------
> > >   cmd/pxe.c           | 19 +++++-----
> > >   include/net.h       |  8 +++--
> > >   include/net/ulwip.h | 64 +++++++++++++++++++++++++++++++++
> > >   6 files changed, 113 insertions(+), 103 deletions(-)
> > >   create mode 100644 include/net/ulwip.h
> > >
> > > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
> > > index ae936c8daa..52399d627c 100644
> > > --- a/boot/bootmeth_efi.c
> > > +++ b/boot/bootmeth_efi.c
> > > @@ -20,6 +20,8 @@
> > >   #include <mapmem.h>
> > >   #include <mmc.h>
> > >   #include <net.h>
> > > +#include <net/lwip.h>
> > > +#include <net/ulwip.h>
> > >   #include <pxe_utils.h>
> > >   #include <linux/sizes.h>
> > >
> > > @@ -319,9 +321,7 @@ static int distro_efi_try_bootflow_files(struct
> udevice *dev,
> > >
> > >   static int distro_efi_read_bootflow_net(struct bootflow *bflow)
> > >   {
> > > -     char file_addr[17], fname[256];
> > > -     char *tftp_argv[] = {"tftp", file_addr, fname, NULL};
> > > -     struct cmd_tbl cmdtp = {};      /* dummy */
> > > +     char fname[256];
> > >       const char *addr_str, *fdt_addr_str;
> > >       int ret, arch, size;
> > >       ulong addr, fdt_addr;
> > > @@ -368,7 +368,6 @@ static int distro_efi_read_bootflow_net(struct
> bootflow *bflow)
> > >       if (!fdt_addr_str)
> > >               return log_msg_ret("fdt", -EINVAL);
> > >       fdt_addr = hextoul(fdt_addr_str, NULL);
> > > -     sprintf(file_addr, "%lx", fdt_addr);
> > >
> > >       /* We only allow the first prefix with PXE */
> > >       ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0);
> > > @@ -379,7 +378,16 @@ static int distro_efi_read_bootflow_net(struct
> bootflow *bflow)
> > >       if (!bflow->fdt_fname)
> > >               return log_msg_ret("fil", -ENOMEM);
> > >
> > > -     if (!do_tftpb(&cmdtp, 0, 3, tftp_argv)) {
> > > +     ret = ulwip_init();
> > > +     if (ret)
> > > +             return log_msg_ret("ulwip_init", ret);
> > > +
> > > +     ret = ulwip_tftp(fdt_addr, fname);
> > > +     if (ret)
> > > +             return log_msg_ret("ulwip_tftp", ret);
> > > +
> > > +     ret = ulwip_loop();
> > > +     if (!ret) {
> > >               bflow->fdt_size = env_get_hex("filesize", 0);
> > >               bflow->fdt_addr = fdt_addr;
> > >       } else {
> > > diff --git a/boot/bootmeth_pxe.c b/boot/bootmeth_pxe.c
> > > index 8d489a11aa..fc6aabaa18 100644
> > > --- a/boot/bootmeth_pxe.c
> > > +++ b/boot/bootmeth_pxe.c
> > > @@ -21,6 +21,8 @@
> > >   #include <mapmem.h>
> > >   #include <mmc.h>
> > >   #include <net.h>
> > > +#include <net/ulwip.h>
> > > +#include <net/lwip.h>
> > >   #include <pxe_utils.h>
> > >
> > >   static int extlinux_pxe_getfile(struct pxe_context *ctx, const char
> *file_path,
> > > @@ -116,18 +118,21 @@ static int extlinux_pxe_read_file(struct udevice
> *dev, struct bootflow *bflow,
> > >                                 const char *file_path, ulong addr,
> > >                                 ulong *sizep)
> > >   {
> > > -     char *tftp_argv[] = {"tftp", NULL, NULL, NULL};
> > > -     struct pxe_context *ctx = dev_get_priv(dev);
> > > -     char file_addr[17];
> > >       ulong size;
> > >       int ret;
> > >
> > > -     sprintf(file_addr, "%lx", addr);
> > > -     tftp_argv[1] = file_addr;
> > > -     tftp_argv[2] = (void *)file_path;
> > > +     ret = ulwip_init();
> > > +     if (ret)
> > > +             return log_msg_ret("ulwip_init", ret);
> > > +
> > > +     ret = ulwip_tftp(addr, file_path);
> > > +     if (ret)
> > > +             return log_msg_ret("ulwip_tftp", ret);
> > > +
> > > +     ret = ulwip_loop();
> > > +     if (ret)
> > > +             return log_msg_ret("ulwip_loop", ret);
> > >
> > > -     if (do_tftpb(ctx->cmdtp, 0, 3, tftp_argv))
> > > -             return -ENOENT;
> > >       ret = pxe_get_file_size(&size);
> > >       if (ret)
> > >               return log_msg_ret("tftp", ret);
> > > diff --git a/cmd/net.c b/cmd/net.c
> > > index d407d8320a..dc5a114309 100644
> > > --- a/cmd/net.c
> > > +++ b/cmd/net.c
> > > @@ -22,6 +22,7 @@
> > >   #include <net/udp.h>
> > >   #include <net/sntp.h>
> > >   #include <net/ncsi.h>
> > > +#include <net/lwip.h>
> > >
> > >   static int netboot_common(enum proto_t, struct cmd_tbl *, int, char
> * const []);
> > >
> > > @@ -40,19 +41,9 @@ U_BOOT_CMD(
> > >   #endif
> > >
> > >   #ifdef CONFIG_CMD_TFTPBOOT
> > > -int do_tftpb(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> > > -{
> > > -     int ret;
> > > -
> > > -     bootstage_mark_name(BOOTSTAGE_KERNELREAD_START, "tftp_start");
> > > -     ret = netboot_common(TFTPGET, cmdtp, argc, argv);
> > > -     bootstage_mark_name(BOOTSTAGE_KERNELREAD_STOP, "tftp_done");
> > > -     return ret;
> > > -}
> > > -
> > >   #if IS_ENABLED(CONFIG_IPV6)
> > >   U_BOOT_CMD(
> > > -     tftpboot,       4,      1,      do_tftpb,
> > > +     tftpboot,       4,      1, do_lwip_tftp,
> >
> > It looks like LWIP doesn't support TFTP with IPv6 addressing.  Perhaps
> > we need to fall back onto the existing TFTP implementation until LWIP
> > supports it?
> >
> > Note, that currently, IPv6 TFTP is enabled using the "-ipv6" argument.
> > The intention is that netboot_common() sees the argument and sets the
> > "use_ip6" variable.  It looks like the new implementation in
> > do_lwip_tftp() doesn't re-use the argument parsing in netboot_common()
> > and that it doesn't handle the addition of the "-ipv6" flag.
> >
> > I support the addition of LWIP, but I'm concerned about how abrupt
> > changes like this one will be for existing users.  The underlying stack
> > will change, with no easy way for the user to revert to the previous
> > stack. Has there been any discussion about preserving the existing
> > functionality, with an option to enable/disable LWIP stack?This would
> > give the community time to transition/validate LWIP before deprecating
> > the old u-boot networking stack.
>
> IPv6 is news to me...and surprising.
>
> Regards,
> Simon
>

For this time I enabled only ipv4 in lwip U-boot implementation and planned
to keep the original ping6 dhcp6 tftp6 on first submission. After the first
ipv4 lwip patches can be merged I planned to work on ipv6 support.  I think
it will require more time for the test environment to be set up, then
actually enabling ipv6 in the configuration header. lwIP can work in dual
stack mode. But I'm not sure that example applications support ipv6 at this
time.

BR,
Maxim.