[PATCHv5 02/13] net/lwip: integrate lwip library
Maxim Uvarov
maxim.uvarov at linaro.org
Thu Aug 3 16:18:07 CEST 2023
On Thu, 3 Aug 2023 at 03:31, Simon Glass <sjg at google.com> wrote:
> Hi Maxim,
>
> On Wed, 2 Aug 2023 at 08:09, Maxim Uvarov <maxim.uvarov at linaro.org> wrote:
> >
>
> Please can you add a commit message in each case?
>
> > ---
> > lib/Kconfig | 2 ++
> > lib/Makefile | 3 +++
> > lib/lwip/Kconfig | 67 +++++++++++++++++++++++++++++++++++++++++++++++
> > lib/lwip/Makefile | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> > net/Kconfig | 1 +
> > net/net.c | 24 +++++++++++++++++
> > 6 files changed, 163 insertions(+)
> > create mode 100644 lib/lwip/Kconfig
> > create mode 100644 lib/lwip/Makefile
> >
>
> > diff --git a/lib/Kconfig b/lib/Kconfig
> > index 3926652db6..79f7d5bc5d 100644
> > --- a/lib/Kconfig
> > +++ b/lib/Kconfig
> > @@ -1112,3 +1112,5 @@ menu "FWU Multi Bank Updates"
> > source lib/fwu_updates/Kconfig
> >
> > endmenu
> > +
> > +source lib/lwip/Kconfig
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 8d8ccc8bbc..598b5755dd 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_EFI_LOADER) += efi_loader/
> > obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += efi_selftest/
> > obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_updates/
> > obj-$(CONFIG_LZMA) += lzma/
> > +obj-$(CONFIG_LWIP_LIB) += lwip/
>
> Do we need the _LIB ?
>
> Yea, _LIB can be removed.
> > obj-$(CONFIG_BZIP2) += bzip2/
> > obj-$(CONFIG_FIT) += libfdt/
> > obj-$(CONFIG_OF_LIVE) += of_live.o
> > @@ -92,6 +93,8 @@ obj-$(CONFIG_LIBAVB) += libavb/
> > obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += libfdt/
> > obj-$(CONFIG_$(SPL_TPL_)OF_REAL) += fdtdec_common.o fdtdec.o
> >
> > +obj-y += lwip/
> > +
> > ifdef CONFIG_SPL_BUILD
> > obj-$(CONFIG_SPL_YMODEM_SUPPORT) += crc16-ccitt.o
> > obj-$(CONFIG_$(SPL_TPL_)HASH) += crc16-ccitt.o
> > diff --git a/lib/lwip/Kconfig b/lib/lwip/Kconfig
> > new file mode 100644
> > index 0000000000..5d2603701c
> > --- /dev/null
> > +++ b/lib/lwip/Kconfig
> > @@ -0,0 +1,67 @@
> > +menu "LWIP"
> > +config LWIP_LIB
> > + bool "Support LWIP library"
> > + help
> > + This option will enable the lwIP library code with
>
> s/will enable/enables/
>
> or even
>
> s/This option will enable/Enable /
>
> > + all dependencies (cmd commands implemented with lwIP
> > + library. This option is automatically enabled if CONFIG_NET=y.
> > + lwIP library (https://git.savannah.nongnu.org/git/lwip.git)
> provides
> > + network stack and application code for U-Boot cmd commands.
>
> Please see doc/... for more information
>
> > +
> > +menu "LWIP options"
> > +
> > +config LWIP_LIB_DEBUG
> > + bool "enable debug"
> > + default n
> > +
> > +config LWIP_LIB_NOASSERT
> > + bool "disable asserts"
> > + default y
> > + help
> > + Disabling asserts reduces binary size on 16k.
>
> by 16K ?
>
> > +
> > +config LWIP_LIB_TCP
> > + bool "tcp"
> > + default y
> > +
> > +config LWIP_LIB_UDP
> > + bool "udp"
> > + default y
>
> need help
>
> > +
> > +config LWIP_LIB_DNS
> > + bool "dns"
> > + default y
> > +
> > +config LWIP_LIB_DHCP
> > + bool "dhcp"
> > + default y
> > +
> > +config LWIP_LIB_LOOPBACK
> > + bool "loopback"
> > + help
> > + Increases size on 1k.
>
> by 1K
>
> > +
> > +config LWIP_LIB_SOCKET
> > + bool "socket API"
> > +
> > +config LWIP_LIB_NETCONN
> > + bool "netconn API"
> > +
> > +config LWIP_LIB_MEM_SIZE
> > + int "mem size"
> > + default 1600
> > + range 1 4096
> > + help
> > + MEM_SIZE: the size of the heap memory. If the application
> will send
> > + a lot of data that needs to be copied, this should be set
> high.
>
> Can you add more detail? What does it mean to copy data??
>
>
Actually currently this option is not used. There are 2 ways to define mem
pool memory:
1. MEM_USE_POOLS (place on the stack). 2. use malloc (or use lwip variant
of malloc with
alignments.). Now I use malloc to simplify current integration. But I
expect then later
we will want to optimize for speed and need some dma friendly memory and
will use MEM_USE_POOLS.
To simplify settings I think it's reasonable to drop it for now from
Kconfig.
> > +
> > +config LWIP_LIB_PBUF_LINK_HLEN
> > + int "pbuf link hlen"
> > + default 14
> > + range 4 1024
> > + help
> > + PBUF_LINK_HLEN: the number of bytes that should be allocated
> for a
> > + link level header. The default is 14, the standard value for
> Ethernet.
>
> Why would you change it? Please add a little more detail
>
>
I think I will remove it from here. I tried to add more options to Kconfig
to customize lwip and that was really helpful to measure size.
This exact option can be 14 or 16 or 18, according to other code and
examples. I think we don't need to change this value in our config.
And then less configuration numbers we will have, then it's better for us.
/**
* PBUF_LINK_HLEN: the number of bytes that should be allocated for a
* link level header. The default is 14, the standard value for
* Ethernet.
*/
#if !defined PBUF_LINK_HLEN || defined __DOXYGEN__
#if (defined LWIP_HOOK_VLAN_SET || LWIP_VLAN_PCP) && !defined __DOXYGEN__
#define PBUF_LINK_HLEN (18 + ETH_PAD_SIZE)
#else /* LWIP_HOOK_VLAN_SET || LWIP_VLAN_PCP */
#define PBUF_LINK_HLEN (14 + ETH_PAD_SIZE)
#endif /* LWIP_HOOK_VLAN_SET || LWIP_VLAN_PCP */
#endif
> +endmenu
> > +
> > +endmenu
> > diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile
> > new file mode 100644
> > index 0000000000..35f34d7afa
> > --- /dev/null
> > +++ b/lib/lwip/Makefile
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +#
> > +# (C) Copyright 2023 Linaro Ltd. <maxim.uvarov at linaro.org>
> > +
> > +LWIPDIR=lwip-external/src
> > +
> > +ccflags-y += -I$(srctree)/lib/lwip/port/include
> > +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include
> -I$(srctree)/lib/lwip
> > +
> > +obj-$(CONFIG_NET) += $(LWIPDIR)/core/init.o \
> > + $(LWIPDIR)/core/def.o \
> > + $(LWIPDIR)/core/dns.o \
> > + $(LWIPDIR)/core/inet_chksum.o \
> > + $(LWIPDIR)/core/ip.o \
> > + $(LWIPDIR)/core/mem.o \
> > + $(LWIPDIR)/core/memp.o \
> > + $(LWIPDIR)/core/netif.o \
> > + $(LWIPDIR)/core/pbuf.o \
> > + $(LWIPDIR)/core/raw.o \
> > + $(LWIPDIR)/core/stats.o \
> > + $(LWIPDIR)/core/sys.o \
> > + $(LWIPDIR)/core/altcp.o \
> > + $(LWIPDIR)/core/altcp_alloc.o \
> > + $(LWIPDIR)/core/altcp_tcp.o \
> > + $(LWIPDIR)/core/tcp.o \
> > + $(LWIPDIR)/core/tcp_in.o \
> > + $(LWIPDIR)/core/tcp_out.o \
> > + $(LWIPDIR)/core/timeouts.o \
> > + $(LWIPDIR)/core/udp.o
> > +
> > +# IPv4
> > +obj-$(CONFIG_NET) += $(LWIPDIR)/core/ipv4/acd.o \
> > + $(LWIPDIR)/core/ipv4/autoip.o \
> > + $(LWIPDIR)/core/ipv4/dhcp.o \
> > + $(LWIPDIR)/core/ipv4/etharp.o \
> > + $(LWIPDIR)/core/ipv4/icmp.o \
> > + $(LWIPDIR)/core/ipv4/igmp.o \
> > + $(LWIPDIR)/core/ipv4/ip4_frag.o \
> > + $(LWIPDIR)/core/ipv4/ip4.o \
> > + $(LWIPDIR)/core/ipv4/ip4_addr.o
> > +# IPv6
> > +obj-$(CONFIG_NET) += $(LWIPDIR)/core/ipv6/dhcp6.o \
> > + $(LWIPDIR)/core/ipv6/ethip6.o \
> > + $(LWIPDIR)/core/ipv6/icmp6.o \
> > + $(LWIPDIR)/core/ipv6/inet6.o \
> > + $(LWIPDIR)/core/ipv6/ip6.o \
> > + $(LWIPDIR)/core/ipv6/ip6_addr.o \
> > + $(LWIPDIR)/core/ipv6/ip6_frag.o \
> > + $(LWIPDIR)/core/ipv6/mld6.o \
> > + $(LWIPDIR)/core/ipv6/nd6.o
> > +# API
> > +obj-$(CONFIG_NET) += $(LWIPDIR)/api/api_lib.o \
> > + $(LWIPDIR)/api/api_msg.o \
> > + $(LWIPDIR)/api/err.o \
> > + $(LWIPDIR)/api/if_api.o \
> > + $(LWIPDIR)/api/netbuf.o \
> > + $(LWIPDIR)/api/netdb.o \
> > + $(LWIPDIR)/api/netifapi.o \
> > + $(LWIPDIR)/api/sockets.o \
> > + $(LWIPDIR)/api/tcpip.o
> > +
> > +# Netdevs
> > +obj-$(CONFIG_NET) += $(LWIPDIR)/netif/ethernet.o
> > +
> > +obj-$(CONFIG_NET) += port/if.o
> > +obj-$(CONFIG_NET) += port/sys-arch.o
> > diff --git a/net/Kconfig b/net/Kconfig
> > index 4215889127..c3f4a7cae7 100644
> > --- a/net/Kconfig
> > +++ b/net/Kconfig
> > @@ -5,6 +5,7 @@
> > menuconfig NET
> > bool "Networking support"
> > default y
> > + select LWIP_LIB
>
> I agree this is best in the long term, but 'imply' might be safer
> until we are happy to delete the old code.
>
>
I agree.
> >
> > if NET
> >
> > diff --git a/net/net.c b/net/net.c
> > index 43abbac7c3..d98e51cb80 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -125,6 +125,7 @@
> > #endif
> > #include "dhcpv6.h"
> > #include "net_rand.h"
> > +#include "../lib/lwip/ulwip.h"
> >
> > /** BOOTP EXTENTIONS **/
> >
> > @@ -452,7 +453,11 @@ int net_loop(enum proto_t protocol)
> > #endif
> >
> > bootstage_mark_name(BOOTSTAGE_ID_ETH_START, "eth_start");
> > +#if defined(CONFIG_LWIP_LIB)
> > + if (!ulwip_enabled() || !ulwip_in_loop())
> > +#endif
> > net_init();
> > +
> > if (eth_is_on_demand_init()) {
> > eth_halt();
> > eth_set_current();
> > @@ -649,6 +654,18 @@ restart:
> > */
> > eth_rx();
> >
> > +#if defined(CONFIG_LWIP_LIB)
>
> if ()
>
> > + if (ulwip_enabled()) {
> > + net_set_state(NETLOOP_CONTINUE);
> > + if (!ulwip_in_loop()) {
> > + if (ulwip_app_get_err())
> > + net_set_state(NETLOOP_FAIL);
> > + else
> > + net_set_state(NETLOOP_SUCCESS);
> > + goto done;
> > + }
> > + }
> > +#endif
> > /*
> > * Abort if ctrl-c was pressed.
> > */
> > @@ -1213,6 +1230,13 @@ void net_process_received_packet(uchar
> *in_packet, int len)
> > if (len < ETHER_HDR_SIZE)
> > return;
> >
> > +#if defined(CONFIG_LWIP_LIB)
>
> same
>
> > + if (ulwip_enabled()) {
> > + uboot_lwip_poll();
>
> do we need the uboot_ prefix?
>
>
lwip has lwip_ prefix. U-Boot doesn't have a prefix. I named the
connecting code between U-Boot and lwip as ulwip_ for several functions.
Here it's also has to be ulwip_
> > + return;
> > + }
> > +#endif
> > +
> > #if defined(CONFIG_API) || defined(CONFIG_EFI_LOADER)
> > if (push_packet) {
> > (*push_packet)(in_packet, len);
> > --
> > 2.30.2
> >
>
> Regards,
> Simon
>
Other comments are valuable, thanks, fixed.
More information about the U-Boot
mailing list