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

Simon Glass
2023-08-08T17:54:41Z

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