| Message ID | 20230802140658.10319-12-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net/lwip: add lwip library for the network stack | expand |
On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > lib/lwip/Makefile | 2 + > lib/lwip/cmd-lwip.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 310 insertions(+) > create mode 100644 lib/lwip/cmd-lwip.c Reviewed-by: Simon Glass <sjg@chromium.org> But this should go in cmd/
Hi Maxim On Wed, Aug 02, 2023 at 08:06:56PM +0600, Maxim Uvarov wrote: > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > lib/lwip/Makefile | 2 + > + > +#include "apps/dns/lwip-dns.h" > +#include "apps/ping/lwip_ping.h" > +#include "ulwip.h" > + > +extern int uboot_lwip_init(void); > +extern int uboot_lwip_loop_is_done(void); > + Can't we have these properly defined in .h files? > +static int do_lwip_info(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + printf("TBD: %s\n", __func__); This is not an RFC, what's missing from fetching at least something meaningful? E.g the lwip version? > + return CMD_RET_SUCCESS; > +} > + > +static int do_lwip_init(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + if (!uboot_lwip_init()) > + return CMD_RET_SUCCESS; > + return CMD_RET_FAILURE; > +} > + > +static int lwip_empty_tmo(void) { return 0; }; > +int (*ulwip_tmo)(void) = lwip_empty_tmo; > +void ulwip_set_tmo(int (*tmo)(void)) > +{ > + ulwip_tmo = tmo; > +} > + > +static void ulwip_clear_tmo(void) > +{ > + ulwip_tmo = lwip_empty_tmo; > +} > + > +static void ulwip_timeout_handler(void) > +{ > + eth_halt(); > + ulwip_tmo(); > + net_set_state(NETLOOP_FAIL); /* we did not get the reply */ I am not sure what I am reading here. You use callbacks a few lines above to set a timeout function. But only set it for dhcp. On top of that the function for DHCP has a case for a *successful* asignment of ip addresses. Why are we setting the state to fail? And why are we complicating this by assigning and removing callbacks if it's only used for dhcp? > + ulwip_loop_set(0); > +} > + > +static int ulwip_loop(void) > +{ > + ulwip_loop_set(1); > + if (net_loop(LWIP) < 0) { > + ulwip_loop_set(0); > + return CMD_RET_FAILURE; > + } > + ulwip_loop_set(0); both of the cases are using ulwip_loop_set(0). Rewrite this with a ret value and dont duplicate the function calls > + return CMD_RET_SUCCESS; > +} > + > +#if defined(CONFIG_CMD_PING) > +int do_lwip_ping(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + if (argc < 2) { > + printf("argc = %d, error\n", argc); > + return CMD_RET_USAGE; > + } > + > + uboot_lwip_init(); > + > + eth_init(); /* activate u-boot eth dev */ eth_init() can fail > + > + printf("Using %s device\n", eth_get_name()); > + printf("pinging addr: %s\n", argv[1]); > + > + net_set_timeout_handler(1000UL, ulwip_timeout_handler); I think it's cleaner to use timeout functions per case instead of carryi ng around that callback mess > + > + if (lwip_ping_init(argv[1])) { > + printf("ping init fail\n"); > + return CMD_RET_FAILURE; > + } > + > + ping_send_now(); > + > + return ulwip_loop(); > +} > +#endif /* CONFIG_CMD_PING */ > + > +#if defined(CONFIG_CMD_WGET) > +extern int lwip_wget(ulong addr, char *url); > + > +int do_lwip_wget(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + char *url; > + > + if (argc < 2) { > + printf("argc = %d, error\n", argc); > + return CMD_RET_USAGE; > + } > + url = argv[1]; > + > + uboot_lwip_init(); uboot_lwip_init() needs a rework here. It prints error messages and doesn't return an error code. You need error checking on the entire function > + > + eth_init(); /* activate u-boot eth dev */ > + > + lwip_wget(image_load_addr, url); > + > + return ulwip_loop(); > +} > +#endif > + > +#if defined(CONFIG_CMD_TFTPBOOT) > +extern int lwip_tftp(ulong addr, char *filename); > + > +int do_lwip_tftp(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + char *filename; > + ulong addr; > + char *end; > + int ret; > + > + switch (argc) { > + case 1: > + filename = env_get("bootfile"); > + break; > + case 2: > + /* > + * Only one arg - accept two forms: > + * Just load address, or just boot file name. The latter > + * form must be written in a format which can not be > + * mis-interpreted as a valid number. > + */ > + addr = hextoul(argv[1], &end); > + if (end == (argv[1] + strlen(argv[1]))) { > + image_load_addr = addr; > + filename = env_get("bootfile"); > + } else { > + filename = argv[1]; > + } > + break; > + case 3: > + image_load_addr = hextoul(argv[1], NULL); > + filename = argv[2]; > + break; > + default: > + return CMD_RET_USAGE; > + } > + > + uboot_lwip_init(); > + > + eth_init(); /* activate u-boot eth dev */ similar comments here, check return codes etc > + > + ret = lwip_tftp(image_load_addr, filename); filename can be NULL > + if (ret) > + return ret; > + > + return ulwip_loop(); > +} > +#endif /* CONFIG_CMD_TFTPBOOT */ > + > +#if defined(CONFIG_CMD_DHCP) > +extern int ulwip_dhcp(void); > + > +int do_lwip_dhcp(void) > +{ > + int ret; > + char *filename; > + > + uboot_lwip_init(); > + > + ret = ulwip_dhcp(); > + > + net_set_timeout_handler(2000UL, ulwip_timeout_handler); > + > + ulwip_loop(); > + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) { > + ulwip_clear_tmo(); > + > + filename = env_get("bootfile"); > + if (!filename) { > + printf("no bootfile\n"); > + return CMD_RET_FAILURE; Why is this a failure? You just have the tftp command enabled but dont want to download anything > + } > + > + eth_init(); /* activate u-boot eth dev */ return codes etc > + net_set_timeout_handler(20000UL, ulwip_timeout_handler); > + lwip_tftp(image_load_addr, filename); > + > + ret = ulwip_loop(); > + } > + > + return ret; > +} > + > +static int _do_lwip_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + return do_lwip_dhcp(); > +} > +#endif /* CONFIG_CMD_DHCP */ > + > +#if defined(CONFIG_CMD_DNS) > +int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + int ret; > + char *name; > + char *varname; > + int LWIP_ERR_INPROGRESS = -5; This should be a define in lwip somewhere if its a documented value. If not drop the caps > + > + if (argc == 1) > + return CMD_RET_USAGE; > + > + name = argv[1]; > + > + if (argc == 3) > + varname = argv[2]; > + else > + varname = NULL; > + > + uboot_lwip_init(); > + > + ret = ulwip_dns(name, varname); > + if (ret == 0) > + return CMD_RET_SUCCESS; > + if (ret != LWIP_ERR_INPROGRESS) > + return CMD_RET_FAILURE; > + > + net_set_timeout_handler(1000UL, ulwip_timeout_handler); > + > + return ulwip_loop(); > +} > +#endif /* CONFIG_CMD_DNS */ > + > +static struct cmd_tbl cmds[] = { > + U_BOOT_CMD_MKENT(info, 1, 0, do_lwip_info, "Info and stats", ""), > + U_BOOT_CMD_MKENT(init, 1, 0, do_lwip_init, > + "initialize lwip stack", ""), > +#if defined(CONFIG_CMD_LWIP_PING) > + U_BOOT_CMD_MKENT(ping, 2, 0, do_lwip_ping, > + "send ICMP ECHO_REQUEST to network host", > + "pingAddress"), > +#endif > +#if defined(CONFIG_CMD_WGET) > + U_BOOT_CMD_MKENT(wget, 2, 0, do_lwip_wget, "", ""), > +#endif > +#if defined(CONFIG_CMD_TFTPBOOT) > + U_BOOT_CMD_MKENT(tftp, 3, 0, do_lwip_tftp, > + "boot image via network using TFTP protocol\n", > + "[loadAddress] [[hostIPaddr:]bootfilename]"), > +#endif > +#if defined(CONFIG_CMD_DHCP) > + U_BOOT_CMD_MKENT(dhcp, 1, 0, _do_lwip_dhcp, > + "boot image via network using DHCP/TFTP protocol", > + ""), > +#endif > +#if defined(CONFIG_CMD_DNS) > + U_BOOT_CMD_MKENT(dns, 3, 0, do_lwip_dns, > + "lookup dns name [and store address at variable]", > + ""), > +#endif > +}; > + > +static int do_ops(struct cmd_tbl *cmdtp, int flag, int argc, > + char *const argv[]) > +{ > + struct cmd_tbl *cp; > + > + cp = find_cmd_tbl(argv[1], cmds, ARRAY_SIZE(cmds)); > + > + argc--; > + argv++; > + > + if (cp == NULL || argc > cp->maxargs) > + return CMD_RET_USAGE; > + if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp)) > + return CMD_RET_SUCCESS; > + > + return cp->cmd(cmdtp, flag, argc, argv); > +} > + > +U_BOOT_CMD( > + lwip, 4, 1, do_ops, > + "LWIP sub system", > + "info - display info\n" > + "init - init LWIP\n" > + "ping addr - pingAddress\n" > + "wget http://IPadress/url/\n" > + "tftp [loadAddress] [[hostIPaddr:]bootfilename]\n" > + "dhcp - boot image via network using DHCP/TFTP protocol\n" > + ); > + > +/* Old command kept for compatibility. Same as 'mmc info' */ > +U_BOOT_CMD( > + lwipinfo, 1, 0, do_lwip_info, > + "display LWIP info", > + "- display LWIP stack info" > +); > -- > 2.30.2 > Regards /Ilias
On Thu, 3 Aug 2023 at 14:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Maxim > > On Wed, Aug 02, 2023 at 08:06:56PM +0600, Maxim Uvarov wrote: > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > --- > > lib/lwip/Makefile | 2 + > > > > + > > +#include "apps/dns/lwip-dns.h" > > +#include "apps/ping/lwip_ping.h" > > +#include "ulwip.h" > > + > > +extern int uboot_lwip_init(void); > > +extern int uboot_lwip_loop_is_done(void); > > + > > Can't we have these properly defined in .h files? > > > +static int do_lwip_info(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + printf("TBD: %s\n", __func__); > > This is not an RFC, what's missing from fetching at least something > meaningful? E.g the lwip version? > LWIP has statistics if it's enabled. (If compiled in.) So I think there might be a version, configured IP addresses, statistics, or some configuration (bond, bridge, vlan, ppp). Might be some network apps running in background (netcon server, htttp server). Maybe for the first version it's reasonable to drop an empty function and then add it with some function. > > > + return CMD_RET_SUCCESS; > > +} > > + > > +static int do_lwip_init(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + if (!uboot_lwip_init()) > > + return CMD_RET_SUCCESS; > > + return CMD_RET_FAILURE; > > +} > > + > > +static int lwip_empty_tmo(void) { return 0; }; > > +int (*ulwip_tmo)(void) = lwip_empty_tmo; > > +void ulwip_set_tmo(int (*tmo)(void)) > > +{ > > + ulwip_tmo = tmo; > > +} > > + > > +static void ulwip_clear_tmo(void) > > +{ > > + ulwip_tmo = lwip_empty_tmo; > > +} > > + > > +static void ulwip_timeout_handler(void) > > +{ > > + eth_halt(); > > + ulwip_tmo(); > > + net_set_state(NETLOOP_FAIL); /* we did not get the reply */ > > I am not sure what I am reading here. You use callbacks a few lines above > to set a timeout function. But only set it for dhcp. On top of that the > function for DHCP has a case for a *successful* asignment of ip addresses. > Why are we setting the state to fail? And why are we complicating this by > assigning and removing callbacks if it's only used for dhcp? > > I need two time out callbacks here: 1. Trap rx polling loop if lwip application works too long. It is used when code goes to net_loop() code to poll rx packets and nobody interrupts this loop. This timeout is used for all cmds (lwip apps). 2. Trap lwip application after specific timeout and then check some state. That is case for DHCP, where LWIP DHCP does not have callback for changing state. And I need to know when to stop polling loop. > > + ulwip_loop_set(0); > > +} > > + > > +static int ulwip_loop(void) > > +{ > > + ulwip_loop_set(1); > > + if (net_loop(LWIP) < 0) { > > + ulwip_loop_set(0); > > + return CMD_RET_FAILURE; > > + } > > + ulwip_loop_set(0); > > both of the cases are using ulwip_loop_set(0). Rewrite this with a ret > value and dont duplicate the function calls > > ok. > > + return CMD_RET_SUCCESS; > > +} > > + > > +#if defined(CONFIG_CMD_PING) > > +int do_lwip_ping(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + if (argc < 2) { > > + printf("argc = %d, error\n", argc); > > + return CMD_RET_USAGE; > > + } > > + > > + uboot_lwip_init(); > > + > > + eth_init(); /* activate u-boot eth dev */ > > eth_init() can fail > > > + > > + printf("Using %s device\n", eth_get_name()); > > + printf("pinging addr: %s\n", argv[1]); > > + > > + net_set_timeout_handler(1000UL, ulwip_timeout_handler); > > I think it's cleaner to use timeout functions per case instead of carryi ng > around that callback mess > > it's timeout varian 1 which I described before. > > + > > + if (lwip_ping_init(argv[1])) { > > + printf("ping init fail\n"); > > + return CMD_RET_FAILURE; > > + } > > + > > + ping_send_now(); > > + > > + return ulwip_loop(); > > +} > > +#endif /* CONFIG_CMD_PING */ > > + > > +#if defined(CONFIG_CMD_WGET) > > +extern int lwip_wget(ulong addr, char *url); > > + > > +int do_lwip_wget(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + char *url; > > + > > + if (argc < 2) { > > + printf("argc = %d, error\n", argc); > > + return CMD_RET_USAGE; > > + } > > + url = argv[1]; > > + > > + uboot_lwip_init(); > > uboot_lwip_init() needs a rework here. It prints error messages and > doesn't return an error code. You need error checking on the entire > function > > > + > > + eth_init(); /* activate u-boot eth dev */ > > + > > + lwip_wget(image_load_addr, url); > > + > > + return ulwip_loop(); > > +} > > +#endif > > + > > +#if defined(CONFIG_CMD_TFTPBOOT) > > +extern int lwip_tftp(ulong addr, char *filename); > > + > > +int do_lwip_tftp(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + char *filename; > > + ulong addr; > > + char *end; > > + int ret; > > + > > + switch (argc) { > > + case 1: > > + filename = env_get("bootfile"); > > + break; > > + case 2: > > + /* > > + * Only one arg - accept two forms: > > + * Just load address, or just boot file name. The latter > > + * form must be written in a format which can not be > > + * mis-interpreted as a valid number. > > + */ > > + addr = hextoul(argv[1], &end); > > + if (end == (argv[1] + strlen(argv[1]))) { > > + image_load_addr = addr; > > + filename = env_get("bootfile"); > > + } else { > > + filename = argv[1]; > > + } > > + break; > > + case 3: > > + image_load_addr = hextoul(argv[1], NULL); > > + filename = argv[2]; > > + break; > > + default: > > + return CMD_RET_USAGE; > > + } > > + > > + uboot_lwip_init(); > > + > > + eth_init(); /* activate u-boot eth dev */ > > similar comments here, check return codes etc > > > + > > + ret = lwip_tftp(image_load_addr, filename); > > filename can be NULL > > > + if (ret) > > + return ret; > > + > > + return ulwip_loop(); > > +} > > +#endif /* CONFIG_CMD_TFTPBOOT */ > > + > > +#if defined(CONFIG_CMD_DHCP) > > +extern int ulwip_dhcp(void); > > + > > +int do_lwip_dhcp(void) > > +{ > > + int ret; > > + char *filename; > > + > > + uboot_lwip_init(); > > + > > + ret = ulwip_dhcp(); > > + > > + net_set_timeout_handler(2000UL, ulwip_timeout_handler); > > + > > + ulwip_loop(); > > + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) { > > + ulwip_clear_tmo(); > > + > > + filename = env_get("bootfile"); > > + if (!filename) { > > + printf("no bootfile\n"); > > + return CMD_RET_FAILURE; > > Why is this a failure? You just have the tftp command enabled but dont > want to download anything > > thanks, if dhcp did not return filename, but only IP, then nothing to download. It's not an error. > > + } > > + > > + eth_init(); /* activate u-boot eth dev */ > > return codes etc > > > + net_set_timeout_handler(20000UL, ulwip_timeout_handler); > > + lwip_tftp(image_load_addr, filename); > > + > > + ret = ulwip_loop(); > > + } > > + > > + return ret; > > +} > > + > > +static int _do_lwip_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + return do_lwip_dhcp(); > > +} > > +#endif /* CONFIG_CMD_DHCP */ > > + > > +#if defined(CONFIG_CMD_DNS) > > +int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + int ret; > > + char *name; > > + char *varname; > > + int LWIP_ERR_INPROGRESS = -5; > > This should be a define in lwip somewhere if its a documented value. If > not drop the caps > > lib/lwip/lwip-external/src/include/lwip/err.h /** Operation in progress */ ERR_INPROGRESS = -5, Inside ./lib/lwip/cmd-lwip.c I do not include any lwip headers. But here I need to check the return value. The idea here is lwip can cache few dns requests. If request comes from cache then we can just exist with CMD_RET_SUCCESS. If there is cache mismatch then we need to go to ulwip_loop() to send request to the netwrok. > + > > + if (argc == 1) > > + return CMD_RET_USAGE; > > + > > + name = argv[1]; > > + > > + if (argc == 3) > > + varname = argv[2]; > > + else > > + varname = NULL; > > + > > + uboot_lwip_init(); > > + > > + ret = ulwip_dns(name, varname); > > + if (ret == 0) > > + return CMD_RET_SUCCESS; > > + if (ret != LWIP_ERR_INPROGRESS) > > + return CMD_RET_FAILURE; > > + > > + net_set_timeout_handler(1000UL, ulwip_timeout_handler); > > + > > + return ulwip_loop(); > > +} > > +#endif /* CONFIG_CMD_DNS */ > > + > > +static struct cmd_tbl cmds[] = { > > + U_BOOT_CMD_MKENT(info, 1, 0, do_lwip_info, "Info and stats", ""), > > + U_BOOT_CMD_MKENT(init, 1, 0, do_lwip_init, > > + "initialize lwip stack", ""), > > +#if defined(CONFIG_CMD_LWIP_PING) > > + U_BOOT_CMD_MKENT(ping, 2, 0, do_lwip_ping, > > + "send ICMP ECHO_REQUEST to network host", > > + "pingAddress"), > > +#endif > > +#if defined(CONFIG_CMD_WGET) > > + U_BOOT_CMD_MKENT(wget, 2, 0, do_lwip_wget, "", ""), > > +#endif > > +#if defined(CONFIG_CMD_TFTPBOOT) > > + U_BOOT_CMD_MKENT(tftp, 3, 0, do_lwip_tftp, > > + "boot image via network using TFTP protocol\n", > > + "[loadAddress] [[hostIPaddr:]bootfilename]"), > > +#endif > > +#if defined(CONFIG_CMD_DHCP) > > + U_BOOT_CMD_MKENT(dhcp, 1, 0, _do_lwip_dhcp, > > + "boot image via network using DHCP/TFTP protocol", > > + ""), > > +#endif > > +#if defined(CONFIG_CMD_DNS) > > + U_BOOT_CMD_MKENT(dns, 3, 0, do_lwip_dns, > > + "lookup dns name [and store address at variable]", > > + ""), > > +#endif > > +}; > > + > > +static int do_ops(struct cmd_tbl *cmdtp, int flag, int argc, > > + char *const argv[]) > > +{ > > + struct cmd_tbl *cp; > > + > > + cp = find_cmd_tbl(argv[1], cmds, ARRAY_SIZE(cmds)); > > + > > + argc--; > > + argv++; > > + > > + if (cp == NULL || argc > cp->maxargs) > > + return CMD_RET_USAGE; > > + if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp)) > > + return CMD_RET_SUCCESS; > > + > > + return cp->cmd(cmdtp, flag, argc, argv); > > +} > > + > > +U_BOOT_CMD( > > + lwip, 4, 1, do_ops, > > + "LWIP sub system", > > + "info - display info\n" > > + "init - init LWIP\n" > > + "ping addr - pingAddress\n" > > + "wget http://IPadress/url/\n" > > + "tftp [loadAddress] [[hostIPaddr:]bootfilename]\n" > > + "dhcp - boot image via network using DHCP/TFTP protocol\n" > > + ); > > + > > +/* Old command kept for compatibility. Same as 'mmc info' */ > > +U_BOOT_CMD( > > + lwipinfo, 1, 0, do_lwip_info, > > + "display LWIP info", > > + "- display LWIP stack info" > > +); > > -- > > 2.30.2 > > > > Regards > /Ilias > Thanks Ilias, If I did not reply for any comment/question then it means that it will be fixed in new version. BR, Maxim.
Hi Maxim [...] > > > +static int lwip_empty_tmo(void) { return 0; }; > > > +int (*ulwip_tmo)(void) = lwip_empty_tmo; > > > +void ulwip_set_tmo(int (*tmo)(void)) > > > +{ > > > + ulwip_tmo = tmo; > > > +} > > > + > > > +static void ulwip_clear_tmo(void) > > > +{ > > > + ulwip_tmo = lwip_empty_tmo; > > > +} > > > + > > > +static void ulwip_timeout_handler(void) > > > +{ > > > + eth_halt(); > > > + ulwip_tmo(); > > > + net_set_state(NETLOOP_FAIL); /* we did not get the reply */ > > > > I am not sure what I am reading here. You use callbacks a few lines above > > to set a timeout function. But only set it for dhcp. On top of that the > > function for DHCP has a case for a *successful* asignment of ip addresses. > > Why are we setting the state to fail? And why are we complicating this by > > assigning and removing callbacks if it's only used for dhcp? > > > > > I need two time out callbacks here: > 1. Trap rx polling loop if lwip application works too long. It is used when > code goes to net_loop() code to poll rx packets > and nobody interrupts this loop. This timeout is used for all cmds (lwip > apps). > > 2. Trap lwip application after specific timeout and then check some state. > That is case for DHCP, where LWIP DHCP does not have > callback for changing state. And I need to know when to stop polling loop. Yes but is there a reason to reassing those callback to a function ptr? Just define them and use them [...] > > > + ret = ulwip_dhcp(); > > > + > > > + net_set_timeout_handler(2000UL, ulwip_timeout_handler); > > > + > > > + ulwip_loop(); > > > + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) { > > > + ulwip_clear_tmo(); > > > + > > > + filename = env_get("bootfile"); > > > + if (!filename) { > > > + printf("no bootfile\n"); > > > + return CMD_RET_FAILURE; > > > > Why is this a failure? You just have the tftp command enabled but dont > > want to download anything > > > > thanks, if dhcp did not return filename, but only IP, then nothing to > download. It's not an error. Yes but downloading a file is not mandatory, it depends on a DHCP option. If you want to emulate this behaviour, you need to fail only if 'bootfile' is set but cant be downloaded. > > > > > + } > > > + Regards /Ilias
On Wed, 9 Aug 2023 at 00:13, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Maxim > > [...] > > > > > +static int lwip_empty_tmo(void) { return 0; }; > > > > +int (*ulwip_tmo)(void) = lwip_empty_tmo; > > > > +void ulwip_set_tmo(int (*tmo)(void)) > > > > +{ > > > > + ulwip_tmo = tmo; > > > > +} > > > > + > > > > +static void ulwip_clear_tmo(void) > > > > +{ > > > > + ulwip_tmo = lwip_empty_tmo; > > > > +} > > > > + > > > > +static void ulwip_timeout_handler(void) > > > > +{ > > > > + eth_halt(); > > > > + ulwip_tmo(); > > > > + net_set_state(NETLOOP_FAIL); /* we did not get the reply */ > > > > > > I am not sure what I am reading here. You use callbacks a few lines > above > > > to set a timeout function. But only set it for dhcp. On top of that > the > > > function for DHCP has a case for a *successful* asignment of ip > addresses. > > > Why are we setting the state to fail? And why are we complicating this > by > > > assigning and removing callbacks if it's only used for dhcp? > > > > > > > > I need two time out callbacks here: > > 1. Trap rx polling loop if lwip application works too long. It is used > when > > code goes to net_loop() code to poll rx packets > > and nobody interrupts this loop. This timeout is used for all cmds (lwip > > apps). > > > > 2. Trap lwip application after specific timeout and then check some > state. > > That is case for DHCP, where LWIP DHCP does not have > > callback for changing state. And I need to know when to stop polling > loop. > > Yes but is there a reason to reassing those callback to a function ptr? > Just define them and use them > > I added a more detailed description to this time out function for the next version. The reason here is the following - there is no scheduler. cmd just starts lwip application. But in fact it does only initialization and in most cases you need to go to the polling loop. Then after polling loop you need to return to 1. back to U-boot console. or 2. back to lwip application to print some message, or get some lwip state or run something else. This return is done with this timeout callback. Which looks logical to me to have a timeout callback in the same file as application code. > [...] > > > > > + ret = ulwip_dhcp(); > > > > + > > > > + net_set_timeout_handler(2000UL, ulwip_timeout_handler); > > > > + > > > > + ulwip_loop(); > > > > + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) { > > > > + ulwip_clear_tmo(); > > > > + > > > > + filename = env_get("bootfile"); > > > > + if (!filename) { > > > > + printf("no bootfile\n"); > > > > + return CMD_RET_FAILURE; > > > > > > Why is this a failure? You just have the tftp command enabled but dont > > > want to download anything > > > > > > thanks, if dhcp did not return filename, but only IP, then nothing to > > download. It's not an error. > > Yes but downloading a file is not mandatory, it depends on a DHCP option. > If you > want to emulate this behaviour, you need to fail only if 'bootfile' is > set but > cant be downloaded. > > > > > > > > > + } > > > > + > > > Regards > /Ilias >
diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index 87ed99a230..25299377cd 100644 --- a/lib/lwip/Makefile +++ b/lib/lwip/Makefile @@ -65,6 +65,8 @@ obj-$(CONFIG_NET) += $(LWIPDIR)/netif/ethernet.o obj-$(CONFIG_NET) += port/if.o obj-$(CONFIG_NET) += port/sys-arch.o +obj-$(CONFIG_NET) += cmd-lwip.o + obj-$(CONFIG_CMD_DHCP) += apps/dhcp/lwip-dhcp.o obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o obj-$(CONFIG_CMD_PING) += apps/ping/ diff --git a/lib/lwip/cmd-lwip.c b/lib/lwip/cmd-lwip.c new file mode 100644 index 0000000000..86b35ccff8 --- /dev/null +++ b/lib/lwip/cmd-lwip.c @@ -0,0 +1,308 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * (C) Copyright 2023 Maxim Uvarov, maxim.uvarov@linaro.org + */ + +#include <common.h> +#include <command.h> +#include <console.h> +#include <display_options.h> +#include <memalign.h> +#include <net.h> +#include <image.h> + +#include "apps/dns/lwip-dns.h" +#include "apps/ping/lwip_ping.h" +#include "ulwip.h" + +extern int uboot_lwip_init(void); +extern int uboot_lwip_loop_is_done(void); + +static int do_lwip_info(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + printf("TBD: %s\n", __func__); + return CMD_RET_SUCCESS; +} + +static int do_lwip_init(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + if (!uboot_lwip_init()) + return CMD_RET_SUCCESS; + return CMD_RET_FAILURE; +} + +static int lwip_empty_tmo(void) { return 0; }; +int (*ulwip_tmo)(void) = lwip_empty_tmo; +void ulwip_set_tmo(int (*tmo)(void)) +{ + ulwip_tmo = tmo; +} + +static void ulwip_clear_tmo(void) +{ + ulwip_tmo = lwip_empty_tmo; +} + +static void ulwip_timeout_handler(void) +{ + eth_halt(); + ulwip_tmo(); + net_set_state(NETLOOP_FAIL); /* we did not get the reply */ + ulwip_loop_set(0); +} + +static int ulwip_loop(void) +{ + ulwip_loop_set(1); + if (net_loop(LWIP) < 0) { + ulwip_loop_set(0); + return CMD_RET_FAILURE; + } + ulwip_loop_set(0); + return CMD_RET_SUCCESS; +} + +#if defined(CONFIG_CMD_PING) +int do_lwip_ping(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + if (argc < 2) { + printf("argc = %d, error\n", argc); + return CMD_RET_USAGE; + } + + uboot_lwip_init(); + + eth_init(); /* activate u-boot eth dev */ + + printf("Using %s device\n", eth_get_name()); + printf("pinging addr: %s\n", argv[1]); + + net_set_timeout_handler(1000UL, ulwip_timeout_handler); + + if (lwip_ping_init(argv[1])) { + printf("ping init fail\n"); + return CMD_RET_FAILURE; + } + + ping_send_now(); + + return ulwip_loop(); +} +#endif /* CONFIG_CMD_PING */ + +#if defined(CONFIG_CMD_WGET) +extern int lwip_wget(ulong addr, char *url); + +int do_lwip_wget(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + char *url; + + if (argc < 2) { + printf("argc = %d, error\n", argc); + return CMD_RET_USAGE; + } + url = argv[1]; + + uboot_lwip_init(); + + eth_init(); /* activate u-boot eth dev */ + + lwip_wget(image_load_addr, url); + + return ulwip_loop(); +} +#endif + +#if defined(CONFIG_CMD_TFTPBOOT) +extern int lwip_tftp(ulong addr, char *filename); + +int do_lwip_tftp(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + char *filename; + ulong addr; + char *end; + int ret; + + switch (argc) { + case 1: + filename = env_get("bootfile"); + break; + case 2: + /* + * Only one arg - accept two forms: + * Just load address, or just boot file name. The latter + * form must be written in a format which can not be + * mis-interpreted as a valid number. + */ + addr = hextoul(argv[1], &end); + if (end == (argv[1] + strlen(argv[1]))) { + image_load_addr = addr; + filename = env_get("bootfile"); + } else { + filename = argv[1]; + } + break; + case 3: + image_load_addr = hextoul(argv[1], NULL); + filename = argv[2]; + break; + default: + return CMD_RET_USAGE; + } + + uboot_lwip_init(); + + eth_init(); /* activate u-boot eth dev */ + + ret = lwip_tftp(image_load_addr, filename); + if (ret) + return ret; + + return ulwip_loop(); +} +#endif /* CONFIG_CMD_TFTPBOOT */ + +#if defined(CONFIG_CMD_DHCP) +extern int ulwip_dhcp(void); + +int do_lwip_dhcp(void) +{ + int ret; + char *filename; + + uboot_lwip_init(); + + ret = ulwip_dhcp(); + + net_set_timeout_handler(2000UL, ulwip_timeout_handler); + + ulwip_loop(); + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) { + ulwip_clear_tmo(); + + filename = env_get("bootfile"); + if (!filename) { + printf("no bootfile\n"); + return CMD_RET_FAILURE; + } + + eth_init(); /* activate u-boot eth dev */ + net_set_timeout_handler(20000UL, ulwip_timeout_handler); + lwip_tftp(image_load_addr, filename); + + ret = ulwip_loop(); + } + + return ret; +} + +static int _do_lwip_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + return do_lwip_dhcp(); +} +#endif /* CONFIG_CMD_DHCP */ + +#if defined(CONFIG_CMD_DNS) +int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + int ret; + char *name; + char *varname; + int LWIP_ERR_INPROGRESS = -5; + + if (argc == 1) + return CMD_RET_USAGE; + + name = argv[1]; + + if (argc == 3) + varname = argv[2]; + else + varname = NULL; + + uboot_lwip_init(); + + ret = ulwip_dns(name, varname); + if (ret == 0) + return CMD_RET_SUCCESS; + if (ret != LWIP_ERR_INPROGRESS) + return CMD_RET_FAILURE; + + net_set_timeout_handler(1000UL, ulwip_timeout_handler); + + return ulwip_loop(); +} +#endif /* CONFIG_CMD_DNS */ + +static struct cmd_tbl cmds[] = { + U_BOOT_CMD_MKENT(info, 1, 0, do_lwip_info, "Info and stats", ""), + U_BOOT_CMD_MKENT(init, 1, 0, do_lwip_init, + "initialize lwip stack", ""), +#if defined(CONFIG_CMD_LWIP_PING) + U_BOOT_CMD_MKENT(ping, 2, 0, do_lwip_ping, + "send ICMP ECHO_REQUEST to network host", + "pingAddress"), +#endif +#if defined(CONFIG_CMD_WGET) + U_BOOT_CMD_MKENT(wget, 2, 0, do_lwip_wget, "", ""), +#endif +#if defined(CONFIG_CMD_TFTPBOOT) + U_BOOT_CMD_MKENT(tftp, 3, 0, do_lwip_tftp, + "boot image via network using TFTP protocol\n", + "[loadAddress] [[hostIPaddr:]bootfilename]"), +#endif +#if defined(CONFIG_CMD_DHCP) + U_BOOT_CMD_MKENT(dhcp, 1, 0, _do_lwip_dhcp, + "boot image via network using DHCP/TFTP protocol", + ""), +#endif +#if defined(CONFIG_CMD_DNS) + U_BOOT_CMD_MKENT(dns, 3, 0, do_lwip_dns, + "lookup dns name [and store address at variable]", + ""), +#endif +}; + +static int do_ops(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct cmd_tbl *cp; + + cp = find_cmd_tbl(argv[1], cmds, ARRAY_SIZE(cmds)); + + argc--; + argv++; + + if (cp == NULL || argc > cp->maxargs) + return CMD_RET_USAGE; + if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp)) + return CMD_RET_SUCCESS; + + return cp->cmd(cmdtp, flag, argc, argv); +} + +U_BOOT_CMD( + lwip, 4, 1, do_ops, + "LWIP sub system", + "info - display info\n" + "init - init LWIP\n" + "ping addr - pingAddress\n" + "wget http://IPadress/url/\n" + "tftp [loadAddress] [[hostIPaddr:]bootfilename]\n" + "dhcp - boot image via network using DHCP/TFTP protocol\n" + ); + +/* Old command kept for compatibility. Same as 'mmc info' */ +U_BOOT_CMD( + lwipinfo, 1, 0, do_lwip_info, + "display LWIP info", + "- display LWIP stack info" +);
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- lib/lwip/Makefile | 2 + lib/lwip/cmd-lwip.c | 308 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 310 insertions(+) create mode 100644 lib/lwip/cmd-lwip.c