Re: [lwip-devel] [PATCHv5 12/13] net/lwip: replace original net commands with lwip

[Simon Glass]

Hi Maxim,

On Tue, 8 Aug 2023 at 08:06, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>
>
> On Thu, 3 Aug 2023 at 03:32, Simon Glass <sjg@google.com> wrote:
>>
>> Hi Maxim,
>>
>> On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>> >
>> > Replace original commands: ping, tftp, dhcp and wget.
>> >
>> > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> > ---
>> >  boot/bootmeth_efi.c |  2 +-
>> >  boot/bootmeth_pxe.c |  2 +-
>> >  cmd/net.c           | 86 +++++----------------------------------------
>> >  cmd/pxe.c           |  2 +-
>> >  include/net.h       |  8 +++--
>> >  include/net/lwip.h  |  5 +++
>> >  lib/Makefile        |  2 --
>> >  lib/lwip/ulwip.h    |  9 +++++
>> >  8 files changed, 31 insertions(+), 85 deletions(-)
>> >  create mode 100644 include/net/lwip.h
>> >  create mode 100644 lib/lwip/ulwip.h
>> >
>> > diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
>> > index af31fbfc85..83334991bb 100644
>> > --- a/boot/bootmeth_efi.c
>> > +++ b/boot/bootmeth_efi.c
>> > @@ -340,7 +340,7 @@ 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)) {
>> > +       if (!do_lwip_tftp(&cmdtp, 0, 3, tftp_argv)) {
>>
>> For these two (efi and pxe) I would really like to avoid passing a
>> command, as you can probably tell. Is there a direct function we can
>> call with the appropriate ages?
>>
>
> yes, just lwip_tftp(addr, name) make code simpler here. If that's ok to 
> replace, then I will do that.

Yes please.

>
>
>>
>> >                 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 ce986bd260..881d2167a6 100644
>> > --- a/boot/bootmeth_pxe.c
>> > +++ b/boot/bootmeth_pxe.c
>> > @@ -123,7 +123,7 @@ static int extlinux_pxe_read_file(struct udevice *dev, 
>> > struct bootflow *bflow,
>> >         tftp_argv[1] = file_addr;
>> >         tftp_argv[2] = (void *)file_path;
>> >
>> > -       if (do_tftpb(ctx->cmdtp, 0, 3, tftp_argv))
>> > +       if (do_lwip_tftp(ctx->cmdtp, 0, 3, tftp_argv))
>> >                 return -ENOENT;
>> >         ret = pxe_get_file_size(&size);
>> >         if (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");
>>
>> Please don't remove these...we need the timing
>>
>
>  bootstage_mark_name() is needed only for tft cmd? I.e. add this to function 
> which parses arguments.
> Or it's also need to account time in pxe and efi?

We should really account for all time, as you say.

[..]

>> > -
>> > -       if (net_loop(PING) < 0) {
>> > -               printf("ping failed; host %s is not alive\n", argv[1]);
>> > -               return CMD_RET_FAILURE;
>> > -       }
>> > -
>> > -       printf("host %s is alive\n", argv[1]);
>>
>> Does lwip print the same messages? That would be useful information
>> for the commit message.
>>
>
> I tried to make messages 1 to 1 with original to pass validation tests.

OK, please can you add that to the commit message?

Regards,
Simon