Re: [PATCHv5 02/13] net/lwip: integrate lwip library

Simon Glass
2023-08-02T21:32:08Z

Hi Maxim,

On Wed, 2 Aug 2023 at 08:09, Maxim Uvarov <maxim.uvarov@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 ?

> 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??

> +
> +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

> +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@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.

>
> 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?

> + return;
> + }
> +#endif
> +
> #if defined(CONFIG_API) || defined(CONFIG_EFI_LOADER)
> if (push_packet) {
> (*push_packet)(in_packet, len);
> --
> 2.30.2
>

Regards,
Simon