[PATCHv5 11/13] net/lwip: connection between cmd and lwip apps
Maxim Uvarov
maxim.uvarov at linaro.org
Tue Aug 8 14:19:33 CEST 2023
On Thu, 3 Aug 2023 at 14:56, Ilias Apalodimas <ilias.apalodimas at 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 at 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.
More information about the U-Boot
mailing list