[PATCHv5 12/13] net/lwip: replace original net commands with lwip
Maxim Uvarov
maxim.uvarov at linaro.org
Tue Aug 8 16:05:55 CEST 2023
On Thu, 3 Aug 2023 at 03:32, Simon Glass <sjg at google.com> wrote:
> Hi Maxim,
>
> On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uvarov at linaro.org> wrote:
> >
> > Replace original commands: ping, tftp, dhcp and wget.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uvarov at 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.
> > 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?
> > - 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.
>
>
I tried to make messages 1 to 1 with original to pass validation tests.
> > -
> > - 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.
>
ok.
>
> > -
> > - 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.
>
>
Yes, sure. There will be do_lwip_tftp() which parses args, and
lwip_tftp(addr, name) without
parsing args.
> >
> > /**
> > * 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.
>
>
yes, sure.
> > +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
>
More information about the U-Boot
mailing list