Re: [PATCHv5 11/13] net/lwip: connection between cmd and lwip apps

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/%5Cn"
• "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