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

[Simon Glass]

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