On Wed, 23 Aug 2023 at 00:58, Simon Glass <s...@google.com> wrote:
> Hi Maxim,
>
> On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov <maxim.uva...@linaro.org>
> wrote:
> >
> > U-Boot recently got support for an alternative network stack using LWIP.
> > Replace dhcp command with the LWIP variant while keeping the output and
> > error messages identical.
> >
> > Signed-off-by: Maxim Uvarov <maxim.uva...@linaro.org>
> > ---
> > include/net/lwip.h | 12 +++++++
> > net/lwip/Makefile | 1 +
> > net/lwip/apps/dhcp/lwip-dhcp.c | 62 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 75 insertions(+)
> > create mode 100644 net/lwip/apps/dhcp/lwip-dhcp.c
> >
> > diff --git a/include/net/lwip.h b/include/net/lwip.h
> > index cda896d062..240ebba354 100644
> > --- a/include/net/lwip.h
> > +++ b/include/net/lwip.h
> > @@ -17,3 +17,15 @@ int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int
> argc,
> > * Other value < 0, if error
> > */
> > int ulwip_dns(char *name, char *varname);
> > +
> > +/**
> > + * ulwip_dhcp() - create the DHCP request to obtain IP address.
> > + *
> > + * This function creates the DHCP request to obtain IP address. If DHCP
> server
> > + * returns file name, this file will be downloaded with tftp. After
> this
> > + * function you need to invoke the polling loop to process network
> communication.
> > + *
> > + * Returns: 0 if success
> > + * Other value < 0, if error
>
> So this is an errno value from errno.h ?
>
>
No. In this version it's what lwip returned. I can replace it with:
return dhcp_start(netif) ? 0 : -EPERM;
to not provide on upper layer lwip internal things.
> > +*/
> > +int ulwip_dhcp(void);
> > diff --git a/net/lwip/Makefile b/net/lwip/Makefile
> > index 6d2c00605b..59323fb325 100644
> > --- a/net/lwip/Makefile
> > +++ b/net/lwip/Makefile
> > @@ -65,4 +65,5 @@ obj-$(CONFIG_NET) += $(LWIPDIR)/netif/ethernet.o
> > obj-$(CONFIG_NET) += port/if.o
> > obj-$(CONFIG_NET) += port/sys-arch.o
> >
> > +obj-$(CONFIG_CMD_DHCP) += apps/dhcp/lwip-dhcp.o
> > obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o
> > diff --git a/net/lwip/apps/dhcp/lwip-dhcp.c
> b/net/lwip/apps/dhcp/lwip-dhcp.c
> > new file mode 100644
> > index 0000000000..cce1e367f9
> > --- /dev/null
> > +++ b/net/lwip/apps/dhcp/lwip-dhcp.c
> > @@ -0,0 +1,62 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * (C) Copyright 2023 Linaro Ltd. <maxim.uva...@linaro.org>
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <console.h>
> > +
> > +#include <lwip/dhcp.h>
> > +#include <lwip/prot/dhcp.h>
> > +
> > +#include <net/eth.h>
> > +#include <net/ulwip.h>
> > +
> > +static struct dhcp dhcp;
>
> Can we keep the state in the uclass?
>
>
I found a good way to avoid this static. If dhcp_set_struct() is not called
this struct is malloced, later it can be accessed as code bellow.
> > +
> > +static int ulwip_dhcp_tmo(void)
> > +{
> > + switch (dhcp.state) {
> > + case DHCP_STATE_BOUND:
> > + env_set("bootfile", dhcp.boot_file_name);
> > + env_set("ipaddr", ip4addr_ntoa(&dhcp.offered_ip_addr));
> > + env_set("netmask", ip4addr_ntoa(&dhcp.offered_sn_mask));
> > + env_set("serverip", ip4addr_ntoa(&dhcp.server_ip_addr));
>
> Please check errors throughout your patches. Use the
> netif_get_client_data() as in code bellow and do not pass
>
>
yep.
> > + printf("DHCP client bound to address %s\n",
> ip4addr_ntoa(&dhcp.offered_ip_addr));
> > + break;
> > + default:
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int ulwip_dhcp(void)
> > +{
> > + int err;
> > + struct netif *netif;
> > + int eth_idx;
> > +
> > + eth_idx = eth_get_dev_index();
> > + if (eth_idx < 0) {
> > + log_err("no eth idx\n");
> > + return -1;
>
> That is -EPERM...please use a suitable error, perhaps -ENOENT?
>
> > + }
> > +
> > + netif = netif_get_by_index(eth_idx + 1);
> > + if (!netif)
> > + return -1;
> > +
> > + ulwip_set_tmo(ulwip_dhcp_tmo);
> > +
> > + if (!netif_get_client_data(netif,
> LWIP_NETIF_CLIENT_DATA_INDEX_DHCP))
> > + dhcp_set_struct(netif, &dhcp);
> > +
> > + err = dhcp_start(netif);
> > + if (err)
> > + log_err("dhcp_start error %d\n", err);
>
> Ideally the caller would print the errors. We try to have commands do
> that, since then it allows silent running for things which want it. It
> also helps with code size.
>
>
ok.
> > +
> > + return err;
> > +}
> > --
> > 2.30.2
> >
>
> Regards,
> Simon
>
