| Message ID | 20230822093614.4717-5-maxim.uvarov@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net/lwip: add lwip library for the network stack | expand |
Hi Maxim, On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov <maxim.uvarov@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.uvarov@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 ? > +*/ > +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.uvarov@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? > + > +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. > + 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. > + > + return err; > +} > -- > 2.30.2 > Regards, Simon
On Wed, 23 Aug 2023 at 00:58, Simon Glass <sjg@google.com> wrote: > Hi Maxim, > > On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov <maxim.uvarov@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.uvarov@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.uvarov@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 >
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 +*/ +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.uvarov@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; + +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)); + 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; + } + + 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); + + return err; +}
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.uvarov@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