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];elsevarname = 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.