Hi Maxim, On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uva...@linaro.org> wrote: > > Replace original commands: ping, tftp, dhcp and wget. > > Signed-off-by: Maxim Uvarov <maxim.uva...@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? > 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 > - return ret; > -} > - > #if IS_ENABLED(CONFIG_IPV6) > U_BOOT_CMD( > - tftpboot, 4, 1, do_tftpb, > + tftpboot, 4, 1, do_lwip_tftp, > "boot image via network using TFTP protocol\n" > "To use IPv6 add -ipv6 parameter or use IPv6 hostIPaddr framed " > "with [] brackets", > @@ -60,7 +51,7 @@ U_BOOT_CMD( > ); > #else > U_BOOT_CMD( > - tftpboot, 3, 1, do_tftpb, > + tftpboot, 3, 1, do_lwip_tftp, > "load file via network using TFTP protocol", > "[loadAddress] [[hostIPaddr:]bootfilename]" > ); > @@ -139,7 +130,7 @@ U_BOOT_CMD(dhcp6, 3, 1, do_dhcp6, > static int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > { > - return netboot_common(DHCP, cmdtp, argc, argv); > + return do_lwip_dhcp(); > } > > U_BOOT_CMD( > @@ -196,13 +187,11 @@ U_BOOT_CMD( > #endif > > #if defined(CONFIG_CMD_WGET) > -static int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const > argv[]) > -{ > - return netboot_common(WGET, cmdtp, argc, argv); > -} > +int do_lwip_wget(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]); > > U_BOOT_CMD( > - wget, 3, 1, do_wget, > + wget, 3, 1, do_lwip_wget, > "boot image via network using HTTP protocol", > "[loadAddress] [[hostIPaddr:]path and image name]" > ); > @@ -456,28 +445,8 @@ static int netboot_common(enum proto_t proto, struct > cmd_tbl *cmdtp, int argc, > } > > #if defined(CONFIG_CMD_PING) > -static int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, > - char *const argv[]) > -{ > - if (argc < 2) > - return CMD_RET_USAGE; > - > - net_ping_ip = string_to_ip(argv[1]); > - if (net_ping_ip.s_addr == 0) > - return CMD_RET_USAGE; > - > - 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. > - > - return CMD_RET_SUCCESS; > -} > - > U_BOOT_CMD( > - ping, 2, 1, do_ping, > + ping, 2, 1, do_lwip_ping, > "send ICMP ECHO_REQUEST to network host", > "pingAddress" > ); > @@ -601,45 +570,8 @@ U_BOOT_CMD( > #endif > > #if defined(CONFIG_CMD_DNS) > -int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > -{ > - if (argc == 1) > - return CMD_RET_USAGE; > - > - /* > - * We should check for a valid hostname: > - * - Each label must be between 1 and 63 characters long > - * - the entire hostname has a maximum of 255 characters > - * - only the ASCII letters 'a' through 'z' (case-insensitive), > - * the digits '0' through '9', and the hyphen > - * - cannot begin or end with a hyphen > - * - no other symbols, punctuation characters, or blank spaces are > - * permitted > - * but hey - this is a minimalist implmentation, so only check length > - * and let the name server deal with things. > - */ > - if (strlen(argv[1]) >= 255) { > - printf("dns error: hostname too long\n"); > - return CMD_RET_FAILURE; > - } Some info in the commit message would be helpful here. People are left to guess why you have removed this code. > - > - net_dns_resolve = argv[1]; > - > - if (argc == 3) > - net_dns_env_var = argv[2]; > - else > - net_dns_env_var = NULL; > - > - if (net_loop(DNS) < 0) { > - printf("dns lookup of %s failed, check setup\n", argv[1]); > - return CMD_RET_FAILURE; > - } > - > - return CMD_RET_SUCCESS; > -} > - > U_BOOT_CMD( > - dns, 3, 1, do_dns, > + dns, 3, 1, do_lwip_dns, > "lookup the IP of a hostname", > "hostname [envvar]" > ); > diff --git a/cmd/pxe.c b/cmd/pxe.c > index 677142520b..a31fbd7e40 100644 > --- a/cmd/pxe.c > +++ b/cmd/pxe.c > @@ -42,7 +42,7 @@ static int do_get_tftp(struct pxe_context *ctx, const char > *file_path, > num_args = 3; > } > > - if (do_tftpb(ctx->cmdtp, 0, num_args, tftp_argv)) > + if (do_lwip_tftp(ctx->cmdtp, 0, num_args, tftp_argv)) > return -ENOENT; > > ret = pxe_get_file_size(sizep); > diff --git a/include/net.h b/include/net.h > index e254df7d7f..de7baeb121 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -54,8 +54,10 @@ struct in_addr { > __be32 s_addr; > }; > > +int do_lwip_dhcp(void); > + > /** > - * do_tftpb - Run the tftpboot command > + * do_lwip_tftp - Run the tftpboot command > * > * @cmdtp: Command information for tftpboot > * @flag: Command flags (CMD_FLAG_...) > @@ -63,7 +65,7 @@ struct in_addr { > * @argv: List of arguments > * Return: result (see enum command_ret_t) > */ > -int do_tftpb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]); > +int do_lwip_tftp(struct cmd_tbl *cmdtp, int flag, int argc, char *const > argv[]); comment!! Also please can you add a direct function that doesn't need to parse args? Basically we want to be able to turn of CONFIG_CMDLINE and have things still work. > > /** > * dhcp_run() - Run DHCP on the current ethernet device > @@ -514,7 +516,7 @@ extern int net_restart_wrap; /* Tried all > network devices */ > enum proto_t { > BOOTP, RARP, ARP, TFTPGET, DHCP, DHCP6, PING, PING6, DNS, NFS, CDP, > NETCONS, SNTP, TFTPSRV, TFTPPUT, LINKLOCAL, FASTBOOT_UDP, > FASTBOOT_TCP, > - WOL, UDP, NCSI, WGET, RS > + WOL, UDP, NCSI, WGET, RS, LWIP > }; > > extern char net_boot_file_name[1024];/* Boot File name */ > diff --git a/include/net/lwip.h b/include/net/lwip.h > new file mode 100644 > index 0000000000..6686a52bfc > --- /dev/null > +++ b/include/net/lwip.h > @@ -0,0 +1,5 @@ > + > +int do_lwip_ping(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]); > +int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]); > diff --git a/lib/Makefile b/lib/Makefile > index 598b5755dd..414f171e74 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -93,8 +93,6 @@ obj-$(CONFIG_LIBAVB) += libavb/ > obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += libfdt/ > obj-$(CONFIG_$(SPL_TPL_)OF_REAL) += fdtdec_common.o fdtdec.o > > -obj-y += lwip/ > - > ifdef CONFIG_SPL_BUILD > obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16-ccitt.o > obj-$(CONFIG_$(SPL_TPL_)HASH) += crc16-ccitt.o > diff --git a/lib/lwip/ulwip.h b/lib/lwip/ulwip.h > new file mode 100644 > index 0000000000..11ca52aa1f > --- /dev/null > +++ b/lib/lwip/ulwip.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +int ulwip_enabled(void); > +int ulwip_in_loop(void); Please add a full comment for each exported function. > +int ulwip_loop_set(int loop); > +int ulwip_exit(int err); > +int uboot_lwip_poll(void); > +int ulwip_app_get_err(void); > +void ulwip_set_tmo(int (*tmo)(void)); > -- > 2.30.2 > Regards, Simon