From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 59111C001DF for ; Thu, 3 Aug 2023 08:56:24 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9828186672; Thu, 3 Aug 2023 10:56:22 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="QQ5nMB/m"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E1919866BB; Thu, 3 Aug 2023 10:56:20 +0200 (CEST) Received: from mail-ej1-x629.google.com (mail-ej1-x629.google.com [IPv6:2a00:1450:4864:20::629]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 5BF9E8666E for ; Thu, 3 Aug 2023 10:56:18 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-ej1-x629.google.com with SMTP id a640c23a62f3a-99c353a395cso92455466b.2 for ; Thu, 03 Aug 2023 01:56:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1691052978; x=1691657778; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=zH7dVRxLHeI/9MTrNGjQzJtCIVBcsvi1ALG/x5DQ3a0=; b=QQ5nMB/mzGUokbaJ9alNeNNgUfvwAIQ+sHFzCZcQY6ox7HcOBqMEAP7f4qFHCkZUrv ip8z/sVlNLRxcDabbh1JmlnOwtI5ax4ynf/vePToLarPXYDU7l7RV1sO6AOY/vk5BwaC T2APEhSPdSNer5dj/gM47WaiIaOLSVjQCaoI2UBVFb/qL1dbiCgxi3SQIgOVXNYtXk2W U81c9iyHOWowJrymbiIudYD/9y9EcHkdtVspOxGXHgbiXrEDwWzmDWeqhYEJiC5A7dd3 q1oGUXqfiH4c/yOifDg7+SHh/NASiVshajmamWVTHZuMHSrux8RROGfKtZsnCa79Q/dn i5VA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691052978; x=1691657778; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=zH7dVRxLHeI/9MTrNGjQzJtCIVBcsvi1ALG/x5DQ3a0=; b=cUDY1FKpsOcShaG00F7UAgdvfCj0rNDDJmy0RwUV0JRa7i2i7Bp7TbM9FH5cGWbBoT X16BS2AxeIvae5LZSpYYrE8qARgLN4U+K+cY7OkaPUQJwh2/BT+V4EreVUj7iIj2SlH0 2fGaxsLSF3gNDxtzJ5dVz8Jz1i16D88KBP697RRnDks+L8VTYaKA8IjEtLfJoYDk+16X hGE4GosoTJwMNCMJlFgNNs7AscVdYgWibX7ijnD42gI8Xvn+2BETt+myBKHeug3iSCXA UTXm1jsl3yr28DfHgyR9JP3vHr4ngjYx4yffVS+0RuK9Wal4jo8zwMEKEidK/zxMCGFd nqYw== X-Gm-Message-State: ABy/qLZrRa5xy1iaz4/iOQfYAvfm4tEBFU3as5uISRBXrzoqYfVyPZh2 WcQ7ffiDMOLbMx/Cs4eLLwtwDw== X-Google-Smtp-Source: APBJJlG39UG9QkrTZu3gwHQLHLlK2jLJ6rJFFlHO7Oya3F4OgfZsx7on9GpKHJfnqZLI1BUk7vW+Eg== X-Received: by 2002:a17:906:3112:b0:99b:64d0:f6c8 with SMTP id 18-20020a170906311200b0099b64d0f6c8mr6422933ejx.50.1691052977829; Thu, 03 Aug 2023 01:56:17 -0700 (PDT) Received: from hades (ppp089210246083.access.hol.gr. [89.210.246.83]) by smtp.gmail.com with ESMTPSA id sb9-20020a170906edc900b00992ae4cf3c1sm10138341ejb.186.2023.08.03.01.56.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Aug 2023 01:56:17 -0700 (PDT) Date: Thu, 3 Aug 2023 11:56:15 +0300 From: Ilias Apalodimas To: Maxim Uvarov Cc: u-boot@lists.denx.de, pbrobinson@redhat.com, joe.hershberger@ni.com, rfried.dev@gmail.com, trini@konsulko.com, goldsimon@gmx.de, lwip-devel@nongnu.org Subject: Re: [PATCHv5 11/13] net/lwip: connection between cmd and lwip apps Message-ID: References: <20230802140658.10319-1-maxim.uvarov@linaro.org> <20230802140658.10319-12-maxim.uvarov@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230802140658.10319-12-maxim.uvarov@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi Maxim On Wed, Aug 02, 2023 at 08:06:56PM +0600, Maxim Uvarov wrote: > Signed-off-by: Maxim Uvarov > --- > 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