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

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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