On Thu, 3 Aug 2023 at 03:31, Simon Glass sjg@google.com wrote:

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 ?

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

Hi Maxim,

On Wed, 2 Aug 2023 at 08:09, Maxim Uvarov maxim.uvarov@linaro.org wrote:

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/Makefile | 2 ++ lib/lwip/apps/dns/lwip-dns.c | 46 ++++++++++++++++++++++++++++++++++++ lib/lwip/apps/dns/lwip-dns.h | 3 +++ 3 files changed, 51 insertions(+) create mode 100644 lib/lwip/apps/dns/lwip-dns.c create mode 100644 lib/lwip/apps/dns/lwip-dns.h

diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index 35f34d7afa..2815662093 100644 --- a/lib/lwip/Makefile +++ b/lib/lwip/Makefile @@ -64,3 +64,5 @@ obj-$(CONFIG_NET) += $(LWIPDIR)/netif/ethernet.o

obj-$(CONFIG_NET) += port/if.o obj-$(CONFIG_NET) += port/sys-arch.o

+obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o diff --git a/lib/lwip/apps/dns/lwip-dns.c b/lib/lwip/apps/dns/lwip-dns.c new file mode 100644 index 0000000000..04fd53bfcb --- /dev/null +++ b/lib/lwip/apps/dns/lwip-dns.c @@ -0,0 +1,46 @@ +// 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/dns.h> +#include <lwip/ip_addr.h>

+#include "../../../lwip/ulwip.h"

Can those headers go in include/ so we don't have this problem?

+static void dns_found_cb(const char *name, const ip_addr_t *ipaddr, void *callback_arg) +{

•   char *varname = (char *)callback_arg;
  

•   if (varname)
  

•           env_set(varname, ip4addr_ntoa(ipaddr));
  

•   printf("resolved %s to %s\n",  name, ip4addr_ntoa(ipaddr));
  

•   ulwip_exit(0);
  

+}

+int ulwip_dns(char *name, char *varname) +{

•   int err;
  

•   ip_addr_t ipaddr; /* not used */
  

•   ip_addr_t dns1;
  

•   ip_addr_t dns2;
  

•   ipaddr_aton(env_get("dnsip"), &dns1);
  

•   ipaddr_aton(env_get("dnsip2"), &dns2);
  

•   dns_init();
  

•   dns_setserver(0, &dns1);
  

•   dns_setserver(1, &dns2);
  

•   err = dns_gethostbyname(name, &ipaddr, dns_found_cb, varname);
  

•   if (err == ERR_OK)
  

•           dns_found_cb(name, &ipaddr, varname);
  

•   return err;
  

+} diff --git a/lib/lwip/apps/dns/lwip-dns.h b/lib/lwip/apps/dns/lwip-dns.h new file mode 100644 index 0000000000..c59f99e099 --- /dev/null +++ b/lib/lwip/apps/dns/lwip-dns.h @@ -0,0 +1,3 @@ +/* SPDX-License-Identifier: GPL-2.0+ */

+int ulwip_dns(char *name, char *varname);

Please comment all exported functions.

-- 2.30.2

Regards, Simon

Hi Maxim,

The split looks a lot easier to review!

On Wed, Aug 02, 2023 at 08:06:49PM +0600, Maxim Uvarov wrote:

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/Makefile | 1 + lib/lwip/apps/dhcp/lwip-dhcp.c | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 lib/lwip/apps/dhcp/lwip-dhcp.c

diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index 2815662093..a3521a9d18 100644 --- a/lib/lwip/Makefile +++ b/lib/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/lib/lwip/apps/dhcp/lwip-dhcp.c b/lib/lwip/apps/dhcp/lwip-dhcp.c new file mode 100644 index 0000000000..9fda43db9f --- /dev/null +++ b/lib/lwip/apps/dhcp/lwip-dhcp.c @@ -0,0 +1,53 @@ +// 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 "../../../lwip/ulwip.h"

I think we had similar comments on the previous version, you need to get rid of this '../../../'

+static struct dhcp dhcp; +static bool dhcp_is_set;

+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;
• ulwip_set_tmo(ulwip_dhcp_tmo);
• netif = netif_get_by_index(1);
• if (!dhcp_is_set) {

Why do we need dhcp_is_set? Can't we check using netif_get_client_data()?

• dhcp_set_struct(netif, &dhcp);
  

• dhcp_is_set = true;
  

• }
• err = dhcp_start(netif);
• if (err)

• printf("dhcp_start error %d\n", err);
  

• return err;

+}

2.30.2

On Thu, 3 Aug 2023 at 12:42, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Wed, Aug 02, 2023 at 08:06:50PM +0600, Maxim Uvarov wrote:

...

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/Makefile | 1 + lib/lwip/apps/tftp/Makefile | 16 +++++ lib/lwip/apps/tftp/lwip-tftp.c | 124 +++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) create mode 100644 lib/lwip/apps/tftp/Makefile create mode 100644 lib/lwip/apps/tftp/lwip-tftp.c

diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index a3521a9d18..1893162c1a 100644 --- a/lib/lwip/Makefile +++ b/lib/lwip/Makefile @@ -67,3 +67,4 @@ 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 +obj-$(CONFIG_CMD_TFTPBOOT) += apps/tftp/ diff --git a/lib/lwip/apps/tftp/Makefile b/lib/lwip/apps/tftp/Makefile new file mode 100644 index 0000000000..299c95e9c0 --- /dev/null +++ b/lib/lwip/apps/tftp/Makefile @@ -0,0 +1,16 @@

+ccflags-y += -I$(srctree)/lib/lwip/port/include +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include

-I$(srctree)/lib/lwip

...

+ccflags-y += -I$(obj)

+$(obj)/tftp.o: $(obj)/tftp.c +.PHONY: $(obj)/tftp.c +$(obj)/tftp.c:

• cp $(srctree)/lib/lwip/lwip-external/src/apps/tftp/tftp.c
  

$(obj)/tftp.c

...

• cp
  

$(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/tftp_client.h $(obj)/tftp_client.h

...

• cp
  

$(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/tftp_common.h $(obj)/tftp_common.h

...

• cp
  

$(srctree)/lib/lwip/lwip-external/contrib/examples/tftp/tftp_example.h $(obj)/tftp_example.h

...

+obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o +obj-$(CONFIG_CMD_TFTPBOOT) += lwip-tftp.o

diff --git a/lib/lwip/apps/tftp/lwip-tftp.c

b/lib/lwip/apps/tftp/lwip-tftp.c

...

new file mode 100644 index 0000000000..511d82e600 --- /dev/null +++ b/lib/lwip/apps/tftp/lwip-tftp.c @@ -0,0 +1,124 @@ +// 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/apps/tftp_client.h" +#include "lwip/apps/tftp_server.h" +#include <tftp_example.h>

+#include <string.h>

+#include "../../../lwip/ulwip.h"

+#if LWIP_UDP

+static ulong daddr; +static ulong size;

+static void *tftp_open(const char *fname, const char *mode, u8_t

is_write)

...

+{

• LWIP_UNUSED_ARG(mode);
  

• return NULL;
  

+}

+static void tftp_close(void *handle) +{

• printf("\ndone\n");
  

• printf("Bytes transferred = %ld (0x%lx hex)\n", size, size);
  

Can you replace all the printf occurences with log_XXXX ?

...

• env_set_ulong("filesize", size);
  

• ulwip_exit(0);
  

+}

+static int tftp_read(void *handle, void *buf, int bytes) +{

• return 0;
  

+}

+static int tftp_write(void *handle, struct pbuf *p) +{

• struct pbuf *q;
  

• for (q = p; q != NULL; q = q->next) {
  

•         memcpy((void *)daddr, q->payload, q->len);
  

•         /* printf("downloaded chunk size %d, to addr 0x%lx\n",
  

q->len, daddr); */

Remove all those debug comments on the next version

...

•         daddr += q->len;
  

•         size += q->len;
  

•         printf("#");
  

• }
  

• return 0;
  

+}

+/* For TFTP client only */ +static void tftp_error(void *handle, int err, const char *msg, int size) +{

• char message[100];
  

• LWIP_UNUSED_ARG(handle);
  

• memset(message, 0, sizeof(message));
  

• MEMCPY(message, msg, LWIP_MIN(sizeof(message)-1, (size_t)size));
  

Why aren't we using the native memcpy?

...

• printf("TFTP error: %d (%s)", err, message);
  

+}

+static const struct tftp_context tftp = {

• tftp_open,
  

• tftp_close,
  

• tftp_read,
  

• tftp_write,
  

• tftp_error
  

+};

+int lwip_tftp(ulong addr, char *fname) +{

• void *f = (void *)0x1; /*fake handle*/
  

If it's fake can't it just be NULL? What does 'fake' mean? is that safe to be passed around the LWIP APIs?

Here I reuse a tftp example from lwIP. f here is a file handle to write output. I do write to memory and do not use this value. But there is a check that his value can not be NULL. So something needs to be passed here to not do modifications to the example code.

...

• err_t err;
  

• ip_addr_t srv;
  

• int ret;
  

• char *server_ip;
  

• if (!fname || addr == 0)
  

•         return CMD_RET_FAILURE;
  

• size = 0;
  

• daddr = addr;
  

• server_ip = env_get("serverip");
  

• if (!server_ip) {
  

•         printf("error: serverip variable has to be set\n");
  

•         return CMD_RET_FAILURE;
  

• }
  

• ret = ipaddr_aton(server_ip, &srv);
  

• LWIP_ASSERT("ipaddr_aton failed", ret == 1);
  

• printf("TFTP from server %s; our IP address is %s\n",
  

•                 server_ip, env_get("ipaddr"));
  

• printf("Filename '%s'.\n", fname);
  

• printf("Load address: 0x%lx\n", daddr);
  

• printf("Loading:");
  

• err = tftp_init_client(&tftp);
  

• if (!(err == ERR_OK || err == ERR_USE))
  

•         printf("tftp_init_client err: %d\n", err);
  

• err = tftp_get(f, &srv, TFTP_PORT, fname, TFTP_MODE_OCTET);
  

• /* might return different errors, like routing problems */
  

• if (err != ERR_OK) {
  

•         printf("tftp_get err=%d\n", err);
  

• }
  

• LWIP_ASSERT("tftp_get failed", err == ERR_OK);
  

• env_set_hex("fileaddr", addr);
  

• return err;
  

+} +#else +#error "UDP has to be supported"

+#endif /* LWIP_UDP */

2.30.2

On Thu, 3 Aug 2023 at 12:48, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Wed, Aug 02, 2023 at 08:06:51PM +0600, Maxim Uvarov wrote:

...

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/Makefile | 1 + lib/lwip/apps/http/Makefile | 13 ++++ lib/lwip/apps/http/lwip-wget.c | 130 +++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 lib/lwip/apps/http/Makefile create mode 100644 lib/lwip/apps/http/lwip-wget.c

diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index 1893162c1a..ec6b728c8e 100644 --- a/lib/lwip/Makefile +++ b/lib/lwip/Makefile @@ -68,3 +68,4 @@ 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 obj-$(CONFIG_CMD_TFTPBOOT) += apps/tftp/ +obj-$(CONFIG_CMD_WGET) += apps/http/ diff --git a/lib/lwip/apps/http/Makefile b/lib/lwip/apps/http/Makefile new file mode 100644 index 0000000000..7d22817e50 --- /dev/null +++ b/lib/lwip/apps/http/Makefile @@ -0,0 +1,13 @@ +ccflags-y += -I$(srctree)/lib/lwip/port/include +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include

-I$(srctree)/lib/lwip

...

+ccflags-y += -I$(obj)

+$(obj)/http_clinet.o: $(obj)/http_client.c +.PHONY: $(obj)/http_client.c +$(obj)/http_client.c:

• cp $(srctree)/lib/lwip/lwip-external/src/apps/http/http_client.c
  

$(obj)/http_client.c

...

• cp
  

$(srctree)/lib/lwip/lwip-external/src/include/lwip/apps/http_client.h $(obj)/http_client.h

...

+obj-$(CONFIG_CMD_WGET) += http_client.o +obj-$(CONFIG_CMD_WGET) += lwip-wget.o

diff --git a/lib/lwip/apps/http/lwip-wget.c

b/lib/lwip/apps/http/lwip-wget.c

...

new file mode 100644 index 0000000000..47a77250c5 --- /dev/null +++ b/lib/lwip/apps/http/lwip-wget.c @@ -0,0 +1,130 @@ +// 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 "http_client.h" +#include <ulwip.h>

+static ulong daddr; +static httpc_connection_t settings;

+#define SERVER_NAME_SIZE 200

+static err_t httpc_recv(void *arg, struct altcp_pcb *pcb, struct pbuf

*p, err_t err)

...

+{

• struct pbuf *q;
  

• LWIP_UNUSED_ARG(err);
  

I think it's better to use standard C instead of LWIP defined macros for things like that

agree no need to use lwip macro here. But what is the common practice for U-Boot for unused function args? I think compilation does not even warn about it. So naming like unused_err has to be fine here.

...

• if (!p)
  

•         return ERR_BUF;
  

• for (q = p; q != NULL; q = q->next) {
  

•         memcpy((void *)daddr, q->payload, q->len);
  

•         printf("downloaded chunk size %d, to addr 0x%lx\n",
  

q->len, daddr);

...

•         daddr += q->len;
  

• }
  

• altcp_recved(pcb, p->tot_len);
  

• pbuf_free(p);
  

• return ERR_OK;
  

+}

+static void httpc_result(void *arg, httpc_result_t httpc_result, u32_t

rx_content_len,

...

•                  u32_t srv_res, err_t err)
  

+{

• if (httpc_result == HTTPC_RESULT_OK) {
  

•         printf("\n%d bytes successfully downloaded.\n",
  

rx_content_len);

Switch those to a log_XXXX that makes sense

yep.

...

•         env_set_ulong("filesize", rx_content_len);
  

•         ulwip_exit(0);
  

• } else {
  

•         printf("\nhttp eroror: %d\n", httpc_result);
  

•         ulwip_exit(-1);
  

• }
  

+}

+/* http://hostname:port/url */ +static int parse_url(char *url, char *host, int *port) +{

• char *p, *pp;
  

• p = strstr(url, "http://");
  

• if (!p) {
  

•         printf("err: no http://!\n");
  

•         return -1;
  

• }
  

• p += strlen("http://");
  

• /* parse hostname */
  

• pp = strchr(p, ':');
  

• if (pp) {
  

+#define PORT_STR_SIZE 5

https://lore.kernel.org/u-boot/ZMJqIxmXJD4S72Ig@hades/ Do we currently support a configurable port for wget?

I might have to reply to the previous email, then update the patch. Old wget has port inside header file and it's binded to 80. Old wget does not parse url script and uses serverip variable for the server. Old wget does not use DNS. Because of lwip variant can use a url (including DNS and IPv6) there is no need to use the server ip variable and bind to a specific port. So here we parse port, host and url and pass it to lwip http_client.c example.

...

•         char portstr[PORT_STR_SIZE];
  

•         if (pp - p >= SERVER_NAME_SIZE)
  

•                 return -2;
  

•         memcpy(host, p, pp - p);
  

•         host[pp - p + 1] = '\0';
  

•         p = pp + 1;
  

•         pp = strchr(p, '/');
  

•         if (!pp) {
  

•                 printf("wrong url\n");
  

•                 return -3;
  

•         }
  

•         if (pp - p >= PORT_STR_SIZE)
  

•                    return -4;
  

•         memcpy(portstr, p, pp - p);
  

•         portstr[pp - p] = '\0';
  

•         *port = atoi(portstr);
  

• } else {
  

•         pp = strchr(p, '/');
  

•         if (!pp) {
  

•                 printf("wrong url\n");
  

•                 return -5;
  

•         }
  

•         if (pp - p >= SERVER_NAME_SIZE)
  

•                 return -6;
  

•         memcpy(host, p, pp - p);
  

•         host[pp - p + 1] = '\0';
  

•         *port = 80; /* default */
  

• }
  

• return 0;
  

+}

+int lwip_wget(ulong addr, char *url) +{

• err_t err;
  

• int port;
  

• char server_name[SERVER_NAME_SIZE];
  

• httpc_state_t *connection;
  

• daddr = addr;
  

• err = parse_url(url, server_name, &port);
  

• if (err) {
  

•         printf("error parse_url\n");
  

•         return -1;
  

• }
  

• printf("downloading %s to addr 0x%lx\n", url, addr);
  

• memset(&settings, 0, sizeof(settings));
  

• settings.result_fn = httpc_result;
  

• err = httpc_get_file_dns(server_name, port, url, &settings,
  

•                          httpc_recv, NULL,  &connection);
  

• if (err != ERR_OK) {
  

•         printf("httpc_init_connection failed\n");
  

•         return err;
  

• }
  

• env_set_hex("fileaddr", addr);
  

• return 0;
  

+}

2.30.2

On Wed, Aug 02, 2023 at 08:06:52PM +0600, Maxim Uvarov wrote:

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/Makefile | 1 + lib/lwip/apps/ping/Makefile | 11 ++++++++++ lib/lwip/apps/ping/lwip_ping.c | 37 ++++++++++++++++++++++++++++++++++ lib/lwip/apps/ping/lwip_ping.h | 24 ++++++++++++++++++++++ lib/lwip/apps/ping/ping.h | 35 ++++++++++++++++++++++++++++++++ 5 files changed, 108 insertions(+) create mode 100644 lib/lwip/apps/ping/Makefile create mode 100644 lib/lwip/apps/ping/lwip_ping.c create mode 100644 lib/lwip/apps/ping/lwip_ping.h create mode 100644 lib/lwip/apps/ping/ping.h

diff --git a/lib/lwip/Makefile b/lib/lwip/Makefile index ec6b728c8e..87ed99a230 100644 --- a/lib/lwip/Makefile +++ b/lib/lwip/Makefile @@ -67,5 +67,6 @@ 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 +obj-$(CONFIG_CMD_PING) += apps/ping/ obj-$(CONFIG_CMD_TFTPBOOT) += apps/tftp/ obj-$(CONFIG_CMD_WGET) += apps/http/ diff --git a/lib/lwip/apps/ping/Makefile b/lib/lwip/apps/ping/Makefile new file mode 100644 index 0000000000..0d0a811336 --- /dev/null +++ b/lib/lwip/apps/ping/Makefile @@ -0,0 +1,11 @@ +ccflags-y += -I$(srctree)/lib/lwip/port/include +ccflags-y += -I$(srctree)/lib/lwip/lwip-external/src/include -I$(srctree)/lib/lwip +ccflags-y += -I$(obj)

+.PHONY: $(obj)/ping.c +$(obj)/ping.o: $(obj)/ping.c +$(obj)/ping.c:

• cp $(srctree)/lib/lwip/lwip-external/contrib/apps/ping/ping.c $(obj)/ping.c

+obj-$(CONFIG_CMD_PING) += ping.o +obj-$(CONFIG_CMD_PING) += lwip_ping.o diff --git a/lib/lwip/apps/ping/lwip_ping.c b/lib/lwip/apps/ping/lwip_ping.c new file mode 100644 index 0000000000..40658ab6fd --- /dev/null +++ b/lib/lwip/apps/ping/lwip_ping.c @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#include "lwip/opt.h" +#include "lwip/ip_addr.h" +#include "ping.h"

+#include <ulwip.h>

+static ip_addr_t ip_target;

+static int ulwip_ping_tmo(void) +{

• printf("ping failed; host %s is not alive\n", ipaddr_ntoa(&ip_target));
• return 0;

Shouldnt this return != 0? Also why do we need this? Looking at ping_raw_init() it ets timeout and print fail messages

+}

+int lwip_ping_init(char *ping_addr) +{

• int err;
• err = ipaddr_aton(ping_addr, &ip_target);
• if (err == 0) {

• printf("wrong ping addr string \"%s\" \n", ping_addr);
  

• return -1;
  

• }
• ulwip_set_tmo(ulwip_ping_tmo);
• ping_init(&ip_target);
• return 0;

+} diff --git a/lib/lwip/apps/ping/lwip_ping.h b/lib/lwip/apps/ping/lwip_ping.h new file mode 100644 index 0000000000..7f08095427 --- /dev/null +++ b/lib/lwip/apps/ping/lwip_ping.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0+ */

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#ifndef LWIP_PING_H +#define LWIP_PING_H

+#include <lwip/ip_addr.h>

+/**

• • PING_USE_SOCKETS: Set to 1 to use sockets, otherwise the raw api is used
• */

Is there any support for sockets in the current series? IOW if someome sets this to 1 will it work? If not get rid of it completely

+#ifndef PING_USE_SOCKETS +#define PING_USE_SOCKETS 0 +#endif

+int lwip_ping_init(char *ping_addr);

+void ping_raw_init(void); +void ping_send_now(void);

+#endif /* LWIP_PING_H */ diff --git a/lib/lwip/apps/ping/ping.h b/lib/lwip/apps/ping/ping.h new file mode 100644 index 0000000000..0dd4bd78c7 --- /dev/null +++ b/lib/lwip/apps/ping/ping.h @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+#include "../../../lwip/ulwip.h"

Again, find a away to include this normally, without all the ../../ uglyness

+#include "lwip/prot/ip4.h"

+#define ip4_print_parts(a, b, c, d) \

• printf("%" U16_F ".%" U16_F ".%" U16_F ".%" U16_F, a, b, c, d);

+#define ip4_print(ipaddr) \

• ip4_print_parts(\

• 	(u16_t)((ipaddr) != NULL ? ip4_addr1_16(ipaddr) : 0), \
  

• 	(u16_t)((ipaddr) != NULL ? ip4_addr2_16(ipaddr) : 0), \
  

• 	(u16_t)((ipaddr) != NULL ? ip4_addr3_16(ipaddr) : 0), \
  

• 	(u16_t)((ipaddr) != NULL ? ip4_addr4_16(ipaddr) : 0))
  

+#define LWIP_DEBUG 1 /* ping_time is under ifdef*/ +#define PING_RESULT(cond) { \

• if (cond == 1) { \

• printf("host "); \
  

• ip4_print(addr); \
  

• printf(" is alive\n"); \
  

• printf(" %"U32_F" ms\n", (sys_now() - ping_time)); \
  

• ulwip_exit(0); \
  

• } else { \

• printf("ping failed; host "); \
  

• ip4_print(addr); \
  

• printf(" is not alive\n"); \
  

• ulwip_exit(-1); \
  

• } \

• } while (0);
  

+#include "lwip/ip_addr.h"

+void ping_init(const ip_addr_t *ping_addr);

2.30.2

Thanks /Ilias

Hi Maxim,

On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov maxim.uvarov@linaro.org wrote:

subject: U-Boot

commit message please (throughout series)

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/port/if.c | 256 ++++++++++++++++++++++++++ lib/lwip/port/include/arch/cc.h | 46 +++++ lib/lwip/port/include/arch/sys_arch.h | 59 ++++++ lib/lwip/port/include/limits.h | 0 lib/lwip/port/sys-arch.c | 20 ++ 5 files changed, 381 insertions(+) create mode 100644 lib/lwip/port/if.c create mode 100644 lib/lwip/port/include/arch/cc.h create mode 100644 lib/lwip/port/include/arch/sys_arch.h create mode 100644 lib/lwip/port/include/limits.h create mode 100644 lib/lwip/port/sys-arch.c

I wonder if this port/ directory should go away and this code should be in net/ ? It feels a bit odd, as lib/ code suggests it is for libraries, not the integration with them.

diff --git a/lib/lwip/port/if.c b/lib/lwip/port/if.c new file mode 100644 index 0000000000..2ed59a0c4e --- /dev/null +++ b/lib/lwip/port/if.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#include <common.h> +#include <command.h> +extern int eth_init(void); /* net.h */ +extern void string_to_enetaddr(const char *addr, uint8_t *enetaddr); /* net.h */ +extern struct in_addr net_ip; +extern u8 net_ethaddr[6];

Can that go in a header file?

+#include "lwip/debug.h" +#include "lwip/arch.h" +#include "netif/etharp.h" +#include "lwip/stats.h" +#include "lwip/def.h" +#include "lwip/mem.h" +#include "lwip/pbuf.h" +#include "lwip/sys.h" +#include "lwip/netif.h"

+#include "lwip/ip.h"

+#define IFNAME0 'e' +#define IFNAME1 '0'

+static struct pbuf *low_level_input(struct netif *netif); +static int uboot_net_use_lwip;

Can we put this stuff in the UCLASS_ETH private data instead of using statics? They require BSS which is typically not available in SPL builds.

+int ulwip_enabled(void) +{

•   return uboot_net_use_lwip;
  

+}

+/* 1 - in loop

• • 0 - no loop
• */

+static int loop_lwip;

+/* ret 1 - in loop

• • 0 - no loop
    

??

This all needs some more detail in the comments

• */

+int ulwip_in_loop(void) +{

•   return loop_lwip;
  

+}

+void ulwip_loop_set(int loop) +{

•   loop_lwip = loop;
  

+}

+static int ulwip_app_err;

+void ulwip_exit(int err) +{

•   ulwip_app_err = err;
  

•   ulwip_loop_set(0);
  

+}

+int ulwip_app_get_err(void) +{

•   return ulwip_app_err;
  

+}

+struct uboot_lwip_if { +};

+static struct netif uboot_netif;

+#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, 0)

+extern uchar *net_rx_packet; +extern int net_rx_packet_len;

header file please

+int uboot_lwip_poll(void) +{

•   struct pbuf *p;
  

•   int err;
  

•   p = low_level_input(&uboot_netif);
  

•   if (!p) {
  

•           printf("error p = low_level_input = NULL\n");
  

•           return 0;
  

•   }
  

•   err = ethernet_input(p, &uboot_netif);
  

•   if (err)
  

•           printf("ip4_input err %d\n", err);
  

log_err() ?

But what is going on here? Just return the error code, rather than suppressing it, then the caller can handle it.

•   return 0;
  

+}

+static struct pbuf *low_level_input(struct netif *netif) +{

•   struct pbuf *p, *q;
  

•   u16_t len = net_rx_packet_len;
  

•   uchar *data = net_rx_packet;
  

+#if ETH_PAD_SIZE

•   len += ETH_PAD_SIZE; /* allow room for Ethernet padding */
  

Please find a way to drop any use of #if

This can be defined to 0 if not needed, for example. Or you could have a static inline eth_pad_size() in the header file (where #if is permitted)

+#endif

•   /* We allocate a pbuf chain of pbufs from the pool. */
  

•   p = pbuf_alloc(PBUF_RAW, len, PBUF_POOL);
  

•   if (p) {
  

+#if ETH_PAD_SIZE

•           pbuf_remove_header(p, ETH_PAD_SIZE); /* drop the padding word */
  

+#endif

•           /* We iterate over the pbuf chain until we have read the entire
  

•            * packet into the pbuf.
  

•            */
  

•           for (q = p; q != NULL; q = q->next) {
  

•                   /* Read enough bytes to fill this pbuf in the chain. The
  

Comment style

/* * Read

•                    * available data in the pbuf is given by the q->len
  

•                    * variable.
  

•                    * This does not necessarily have to be a memcpy, you can also preallocate
  

•                    * pbufs for a DMA-enabled MAC and after receiving truncate it to the
  

•                    * actually received size. In this case, ensure the tot_len member of the
  

•                    * pbuf is the sum of the chained pbuf len members.
  

•                    */
  

•                   MEMCPY(q->payload, data, q->len);
  

•                   data += q->len;
  

•           }
  

•           //acknowledge that packet has been read();
  

Space after // (please fix throughout)

+#if ETH_PAD_SIZE

•           pbuf_add_header(p, ETH_PAD_SIZE); /* reclaim the padding word */
  

+#endif

•           LINK_STATS_INC(link.recv);
  

•   } else {
  

•           //drop packet();
  

•           LINK_STATS_INC(link.memerr);
  

•           LINK_STATS_INC(link.drop);
  

•   }
  

•   return p;
  

+}

+static int ethernetif_input(struct pbuf *p, struct netif *netif) +{

•   struct ethernetif *ethernetif;
  

•   ethernetif = netif->state;
  

•   /* move received packet into a new pbuf */
  

•   p = low_level_input(netif);
  

•   /* if no packet could be read, silently ignore this */
  

•   if (p) {
  

•           /* pass all packets to ethernet_input, which decides what packets it supports */
  

•           if (netif->input(p, netif) != ERR_OK) {
  

•                   LWIP_DEBUGF(NETIF_DEBUG, ("%s: IP input error\n", __func__));
  

•                   pbuf_free(p);
  

•                   p = NULL;
  

•           }
  

•   }
  

blank line before final return

•   return 0;
  

+}

+static err_t low_level_output(struct netif *netif, struct pbuf *p) +{

•   int err;
  

•   err = eth_send(p->payload, p->len);
  

•   if (err != 0) {
  

if (err)

•           printf("eth_send error %d\n", err);
  

•           return ERR_ABRT;
  

•   }
  

•   return ERR_OK;
  

+}

+err_t uboot_lwip_if_init(struct netif *netif) +{

•   struct uboot_lwip_if *uif = (struct uboot_lwip_if *)malloc(sizeof(struct uboot_lwip_if));
  

•   if (!uif) {
  

•           printf("uboot_lwip_if: out of memory\n");
  

•           return ERR_MEM;
  

•   }
  

•   netif->state = uif;
  

•   netif->name[0] = IFNAME0;
  

•   netif->name[1] = IFNAME1;
  

•   netif->hwaddr_len = ETHARP_HWADDR_LEN;
  

•   string_to_enetaddr(env_get("ethaddr"), netif->hwaddr);
  

+#if defined(CONFIG_LWIP_LIB_DEBUG)

•   printf("              MAC: %02X:%02X:%02X:%02X:%02X:%02X\n",
  

•          netif->hwaddr[0], netif->hwaddr[1], netif->hwaddr[2],
  

•          netif->hwaddr[3], netif->hwaddr[4], netif->hwaddr[5]);
  

+#endif

+#if LWIP_IPV4

•   netif->output = etharp_output;
  

+#endif /* LWIP_IPV4 */ +#if LWIP_IPV6

•   netif->output_ip6 = ethip6_output;
  

+#endif /* LWIP_IPV6 */

You may need to add accessors in the header file (as global_data.h) so you don't need the #if

•   netif->linkoutput = low_level_output;
  

•   netif->mtu = 1500;
  

•   netif->flags = NETIF_FLAG_BROADCAST | NETIF_FLAG_ETHARP | NETIF_FLAG_LINK_UP;
  

•   eth_init(); /* activate u-boot eth dev */
  

•   if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) {
  

•           printf("Initialized LWIP stack\n");
  

•   }
  

•   return ERR_OK;
  

+}

+int uboot_lwip_init(void) +{

•   ip4_addr_t ipaddr, netmask, gw;
  

•   if (uboot_net_use_lwip)
  

•           return CMD_RET_SUCCESS;
  

•   ip4_addr_set_zero(&gw);
  

•   ip4_addr_set_zero(&ipaddr);
  

•   ip4_addr_set_zero(&netmask);
  

•   ipaddr_aton(env_get("ipaddr"), &ipaddr);
  

•   ipaddr_aton(env_get("ipaddr"), &netmask);
  

•   LWIP_PORT_INIT_NETMASK(&netmask);
  

•   if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) {
  

•           printf("Starting lwIP, IP: %s\n", ip4addr_ntoa(&ipaddr));
  

•           printf("               GW: %s\n", ip4addr_ntoa(&gw));
  

•           printf("             mask: %s\n", ip4addr_ntoa(&netmask));
  

•   }
  

•   if (!netif_add(&uboot_netif, &ipaddr, &netmask, &gw,
  

•                  &uboot_netif, uboot_lwip_if_init, ethernetif_input))
  

•           printf("err: netif_add failed!\n");
  

•   netif_set_up(&uboot_netif);
  

•   netif_set_link_up(&uboot_netif);
  

+#if LWIP_IPV6

if ()

•   netif_create_ip6_linklocal_address(&uboot_netif, 1);
  

•   printf("             IPv6: %s\n", ip6addr_ntoa(netif_ip6_addr(uboot_netif, 0)));
  

+#endif /* LWIP_IPV6 */

•   uboot_net_use_lwip = 1;
  

•   return CMD_RET_SUCCESS;
  

+}

+/* placeholder, not used now */ +void uboot_lwip_destroy(void) +{

•   uboot_net_use_lwip = 0;
  

+} diff --git a/lib/lwip/port/include/arch/cc.h b/lib/lwip/port/include/arch/cc.h new file mode 100644 index 0000000000..db30d7614e --- /dev/null +++ b/lib/lwip/port/include/arch/cc.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

It would help to have a little one-line comment as to what these files are for.

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#ifndef LWIP_ARCH_CC_H +#define LWIP_ARCH_CC_H

+#include <linux/types.h> +#include <linux/kernel.h>

+#define LWIP_ERRNO_INCLUDE <errno.h>

+#define LWIP_ERRNO_STDINCLUDE 1 +#define LWIP_NO_UNISTD_H 1 +#define LWIP_TIMEVAL_PRIVATE 1

+extern unsigned int lwip_port_rand(void); +#define LWIP_RAND() (lwip_port_rand())

+/* different handling for unit test, normally not needed */ +#ifdef LWIP_NOASSERT_ON_ERROR +#define LWIP_ERROR(message, expression, handler) do { if (!(expression)) { \

•                                               handler; }} while (0)
  

+#endif

+#define LWIP_DONT_PROVIDE_BYTEORDER_FUNCTIONS

+#define LWIP_PLATFORM_ASSERT(x) do {printf("Assertion "%s" failed at line %d in %s\n", \

•                               x, __LINE__, __FILE__); } while (0)
  

+static inline int atoi(const char *str)

Can we use U-Boot's version?

+{

•   int r = 0;
  

•   int i;
  

•   for (i = 0; str[i] != '\0'; ++i)
  

•           r = r * 10 + str[i] - '0';
  

•   return r;
  

+}

+#define LWIP_ERR_T int

+#endif /* LWIP_ARCH_CC_H */ diff --git a/lib/lwip/port/include/arch/sys_arch.h b/lib/lwip/port/include/arch/sys_arch.h new file mode 100644 index 0000000000..8d95146275 --- /dev/null +++ b/lib/lwip/port/include/arch/sys_arch.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#ifndef LWIP_ARCH_SYS_ARCH_H +#define LWIP_ARCH_SYS_ARCH_H

+#include "lwip/opt.h" +#include "lwip/arch.h" +#include "lwip/err.h"

+#define ERR_NEED_SCHED 123

+void sys_arch_msleep(u32_t delay_ms); +#define sys_msleep(ms) sys_arch_msleep(ms)

+#if SYS_LIGHTWEIGHT_PROT +typedef u32_t sys_prot_t; +#endif /* SYS_LIGHTWEIGHT_PROT */

+#include <errno.h>

+#define SYS_MBOX_NULL NULL +#define SYS_SEM_NULL NULL

+typedef u32_t sys_prot_t;

+struct sys_sem; +typedef struct sys_sem *sys_sem_t; +#define sys_sem_valid(sem) (((sem) != NULL) && (*(sem) != NULL)) +#define sys_sem_set_invalid(sem) do { if ((sem) != NULL) { *(sem) = NULL; }} while (0)

+/* let sys.h use binary semaphores for mutexes */ +#define LWIP_COMPAT_MUTEX 1 +#define LWIP_COMPAT_MUTEX_ALLOWED 1

+struct sys_mbox; +typedef struct sys_mbox *sys_mbox_t; +#define sys_mbox_valid(mbox) (((mbox) != NULL) && (*(mbox) != NULL)) +#define sys_mbox_set_invalid(mbox) do { if ((mbox) != NULL) { *(mbox) = NULL; }} while (0)

+struct sys_thread; +typedef struct sys_thread *sys_thread_t;

+static inline u32_t sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) +{

•   return 0;
  

+};

+static inline err_t sys_mbox_trypost(sys_mbox_t *mbox, void *msg) +{

•   return 0;
  

+};

+#define sys_sem_signal(s)

+#endif /* LWIP_ARCH_SYS_ARCH_H */ diff --git a/lib/lwip/port/include/limits.h b/lib/lwip/port/include/limits.h new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lib/lwip/port/sys-arch.c b/lib/lwip/port/sys-arch.c new file mode 100644 index 0000000000..609eeccf8c --- /dev/null +++ b/lib/lwip/port/sys-arch.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#include <common.h> +#include <rand.h> +#include "lwip/opt.h"

+u32_t sys_now(void) +{

•   return get_timer(0);
  

+}

+u32_t lwip_port_rand(void) +{

•   return (u32_t)rand();
  

+}

-- 2.30.2

Regards, Simon

On Thu, 3 Aug 2023 at 22:21, Maxim Uvarov maxim.uvarov@linaro.org wrote:

On Thu, 3 Aug 2023 at 03:32, Simon Glass sjg@google.com wrote:

...

Hi Maxim,

On Wed, 2 Aug 2023 at 08:11, Maxim Uvarov maxim.uvarov@linaro.org wrote:

...

subject: U-Boot

commit message please (throughout series)

...

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

lib/lwip/port/if.c | 256 ++++++++++++++++++++++++++ lib/lwip/port/include/arch/cc.h | 46 +++++ lib/lwip/port/include/arch/sys_arch.h | 59 ++++++ lib/lwip/port/include/limits.h | 0 lib/lwip/port/sys-arch.c | 20 ++ 5 files changed, 381 insertions(+) create mode 100644 lib/lwip/port/if.c create mode 100644 lib/lwip/port/include/arch/cc.h create mode 100644 lib/lwip/port/include/arch/sys_arch.h create mode 100644 lib/lwip/port/include/limits.h create mode 100644 lib/lwip/port/sys-arch.c

I wonder if this port/ directory should go away and this code should be in net/ ? It feels a bit odd, as lib/ code suggests it is for libraries, not the integration with them.

In all lwIP examples I was port/ directory with specific files for the platform. And here I also planned to follow the same rule. And yes, I think it's better to move all this code to net/ and cmd/.

...

...

diff --git a/lib/lwip/port/if.c b/lib/lwip/port/if.c new file mode 100644 index 0000000000..2ed59a0c4e --- /dev/null +++ b/lib/lwip/port/if.c @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#include <common.h> +#include <command.h> +extern int eth_init(void); /* net.h */ +extern void string_to_enetaddr(const char *addr, uint8_t *enetaddr);

/* net.h */

...

+extern struct in_addr net_ip; +extern u8 net_ethaddr[6];

Can that go in a header file?

I tried to do as minimal changes as I could. In general these are definitions from include/net.h. And the problem that net.h has not only ethernet things, but also protocol defines which overlaps with lwip protocol defines and data structures. More clean fix will be to clean up net.h and move ip to net/ip.h udp to net/udp.h and so on. So here we can include <net.h> to get eth_init() and friends accessed.

...

...

+#include "lwip/debug.h" +#include "lwip/arch.h" +#include "netif/etharp.h" +#include "lwip/stats.h" +#include "lwip/def.h" +#include "lwip/mem.h" +#include "lwip/pbuf.h" +#include "lwip/sys.h" +#include "lwip/netif.h"

+#include "lwip/ip.h"

+#define IFNAME0 'e' +#define IFNAME1 '0'

+static struct pbuf *low_level_input(struct netif *netif); +static int uboot_net_use_lwip;

Can we put this stuff in the UCLASS_ETH private data instead of using statics? They require BSS which is typically not available in SPL builds.

yes, that will work. I expect this flag to be used for compatibility mode. I.e. if it's set before cmd then lwip controls net_loop(), if not set then an original code controls net_loop(). I can even add an argument to eth_init(). But because there are too many eth_init() calls I think I will add an entry to uclass.

moved to UCLASS_ETH with split net.h on net.h, eth.h and arp.h. And I like how it looks now, thanks!

...

...

+int ulwip_enabled(void) +{

•   return uboot_net_use_lwip;
  

+}

+/* 1 - in loop

• • 0 - no loop
• */

+static int loop_lwip;

+/* ret 1 - in loop

• • 0 - no loop
    

??

This all needs some more detail in the comments

Yes, maybe with UCLASS_ETH some of that will go away.

Yes, that goes away after moving.

...

...

• */

+int ulwip_in_loop(void) +{

•   return loop_lwip;
  

+}

+void ulwip_loop_set(int loop) +{

•   loop_lwip = loop;
  

+}

+static int ulwip_app_err;

+void ulwip_exit(int err) +{

•   ulwip_app_err = err;
  

•   ulwip_loop_set(0);
  

+}

+int ulwip_app_get_err(void) +{

•   return ulwip_app_err;
  

+}

+struct uboot_lwip_if { +};

+static struct netif uboot_netif;

+#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255,

...

+extern uchar *net_rx_packet; +extern int net_rx_packet_len;

header file please

ok, the same thing here, it's from net.h

...

...

+int uboot_lwip_poll(void) +{

•   struct pbuf *p;
  

•   int err;
  

•   p = low_level_input(&uboot_netif);
  

•   if (!p) {
  

•           printf("error p = low_level_input = NULL\n");
  

•           return 0;
  

•   }
  

•   err = ethernet_input(p, &uboot_netif);
  

•   if (err)
  

•           printf("ip4_input err %d\n", err);
  

log_err() ?

But what is going on here? Just return the error code, rather than suppressing it, then the caller can handle it.

Looked more detail - current version ethernet_input() (lwip master) always returns ERR_OK (0). When I wrote I added a return code check for non function. So I expect that it can be considered as a bug if some time later we receive something non 0.

But in general the poll() function has to be void. I will correct it.

...

...

•   return 0;
  

+}

+static struct pbuf *low_level_input(struct netif *netif) +{

•   struct pbuf *p, *q;
  

•   u16_t len = net_rx_packet_len;
  

•   uchar *data = net_rx_packet;
  

+#if ETH_PAD_SIZE

•   len += ETH_PAD_SIZE; /* allow room for Ethernet padding */
  

Please find a way to drop any use of #if

This can be defined to 0 if not needed, for example. Or you could have a static inline eth_pad_size() in the header file (where #if is permitted)

let's drop for n

...

+#endif

...

•   /* We allocate a pbuf chain of pbufs from the pool. */
  

•   p = pbuf_alloc(PBUF_RAW, len, PBUF_POOL);
  

•   if (p) {
  

+#if ETH_PAD_SIZE

•           pbuf_remove_header(p, ETH_PAD_SIZE); /* drop the
  

padding word */

...

+#endif

•           /* We iterate over the pbuf chain until we have read
  

the entire

...

•            * packet into the pbuf.
  

•            */
  

•           for (q = p; q != NULL; q = q->next) {
  

•                   /* Read enough bytes to fill this pbuf in the
  

chain. The

Comment style

/*

• Read

...

•                    * available data in the pbuf is given by the
  

q->len

...

•                    * variable.
  

•                    * This does not necessarily have to be a
  

memcpy, you can also preallocate

...

•                    * pbufs for a DMA-enabled MAC and after
  

receiving truncate it to the

...

•                    * actually received size. In this case, ensure
  

the tot_len member of the

...

•                    * pbuf is the sum of the chained pbuf len
  

members.

...

•                    */
  

•                   MEMCPY(q->payload, data, q->len);
  

•                   data += q->len;
  

•           }
  

•           //acknowledge that packet has been read();
  

Space after // (please fix throughout)

...

+#if ETH_PAD_SIZE

•           pbuf_add_header(p, ETH_PAD_SIZE); /* reclaim the
  

padding word */

...

+#endif

•           LINK_STATS_INC(link.recv);
  

•   } else {
  

•           //drop packet();
  

•           LINK_STATS_INC(link.memerr);
  

•           LINK_STATS_INC(link.drop);
  

•   }
  

•   return p;
  

+}

+static int ethernetif_input(struct pbuf *p, struct netif *netif) +{

•   struct ethernetif *ethernetif;
  

•   ethernetif = netif->state;
  

•   /* move received packet into a new pbuf */
  

•   p = low_level_input(netif);
  

•   /* if no packet could be read, silently ignore this */
  

•   if (p) {
  

•           /* pass all packets to ethernet_input, which decides
  

what packets it supports */

...

•           if (netif->input(p, netif) != ERR_OK) {
  

•                   LWIP_DEBUGF(NETIF_DEBUG, ("%s: IP input
  

error\n", __func__));

...

•                   pbuf_free(p);
  

•                   p = NULL;
  

•           }
  

•   }
  

blank line before final return

...

•   return 0;
  

+}

+static err_t low_level_output(struct netif *netif, struct pbuf *p) +{

•   int err;
  

•   err = eth_send(p->payload, p->len);
  

•   if (err != 0) {
  

if (err)

...

•           printf("eth_send error %d\n", err);
  

•           return ERR_ABRT;
  

•   }
  

•   return ERR_OK;
  

+}

+err_t uboot_lwip_if_init(struct netif *netif) +{

•   struct uboot_lwip_if *uif = (struct uboot_lwip_if
  

*)malloc(sizeof(struct uboot_lwip_if));

...

•   if (!uif) {
  

•           printf("uboot_lwip_if: out of memory\n");
  

•           return ERR_MEM;
  

•   }
  

•   netif->state = uif;
  

•   netif->name[0] = IFNAME0;
  

•   netif->name[1] = IFNAME1;
  

•   netif->hwaddr_len = ETHARP_HWADDR_LEN;
  

•   string_to_enetaddr(env_get("ethaddr"), netif->hwaddr);
  

+#if defined(CONFIG_LWIP_LIB_DEBUG)

•   printf("              MAC: %02X:%02X:%02X:%02X:%02X:%02X\n",
  

•          netif->hwaddr[0], netif->hwaddr[1], netif->hwaddr[2],
  

•          netif->hwaddr[3], netif->hwaddr[4], netif->hwaddr[5]);
  

+#endif

+#if LWIP_IPV4

•   netif->output = etharp_output;
  

+#endif /* LWIP_IPV4 */ +#if LWIP_IPV6

•   netif->output_ip6 = ethip6_output;
  

+#endif /* LWIP_IPV6 */

You may need to add accessors in the header file (as global_data.h) so you don't need the #if

I did not understand the comment about global_data.h. lwiIP as I understand can work in 3 modes 1. ipv4 2.ipv6 3. ipv4+ipv6. If we want optimization for size I think we need to pass this to Kconfig because lwip has some structs under ifdefs.

...

•   netif->linkoutput = low_level_output;
  

...

•   netif->mtu = 1500;
  

•   netif->flags = NETIF_FLAG_BROADCAST | NETIF_FLAG_ETHARP |
  

NETIF_FLAG_LINK_UP;

...

•   eth_init(); /* activate u-boot eth dev */
  

•   if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) {
  

•           printf("Initialized LWIP stack\n");
  

•   }
  

•   return ERR_OK;
  

+}

+int uboot_lwip_init(void) +{

•   ip4_addr_t ipaddr, netmask, gw;
  

•   if (uboot_net_use_lwip)
  

•           return CMD_RET_SUCCESS;
  

•   ip4_addr_set_zero(&gw);
  

•   ip4_addr_set_zero(&ipaddr);
  

•   ip4_addr_set_zero(&netmask);
  

•   ipaddr_aton(env_get("ipaddr"), &ipaddr);
  

•   ipaddr_aton(env_get("ipaddr"), &netmask);
  

•   LWIP_PORT_INIT_NETMASK(&netmask);
  

•   if (IS_ENABLED(CONFIG_LWIP_LIB_DEBUG)) {
  

•           printf("Starting lwIP, IP: %s\n",
  

ip4addr_ntoa(&ipaddr));

...

•           printf("               GW: %s\n", ip4addr_ntoa(&gw));
  

•           printf("             mask: %s\n",
  

ip4addr_ntoa(&netmask));

...

•   }
  

•   if (!netif_add(&uboot_netif, &ipaddr, &netmask, &gw,
  

•                  &uboot_netif, uboot_lwip_if_init,
  

ethernetif_input))

...

•           printf("err: netif_add failed!\n");
  

•   netif_set_up(&uboot_netif);
  

•   netif_set_link_up(&uboot_netif);
  

+#if LWIP_IPV6

if ()

No, we are using lwip header files. it will not work if LWIP_IPV6 set to 0.

...

•   netif_create_ip6_linklocal_address(&uboot_netif, 1);
  

...

•   printf("             IPv6: %s\n",
  

ip6addr_ntoa(netif_ip6_addr(uboot_netif, 0)));

...

+#endif /* LWIP_IPV6 */

•   uboot_net_use_lwip = 1;
  

•   return CMD_RET_SUCCESS;
  

+}

+/* placeholder, not used now */ +void uboot_lwip_destroy(void) +{

•   uboot_net_use_lwip = 0;
  

+} diff --git a/lib/lwip/port/include/arch/cc.h

b/lib/lwip/port/include/arch/cc.h

...

new file mode 100644 index 0000000000..db30d7614e --- /dev/null +++ b/lib/lwip/port/include/arch/cc.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

It would help to have a little one-line comment as to what these files are for.

...

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#ifndef LWIP_ARCH_CC_H +#define LWIP_ARCH_CC_H

+#include <linux/types.h> +#include <linux/kernel.h>

+#define LWIP_ERRNO_INCLUDE <errno.h>

+#define LWIP_ERRNO_STDINCLUDE 1 +#define LWIP_NO_UNISTD_H 1 +#define LWIP_TIMEVAL_PRIVATE 1

+extern unsigned int lwip_port_rand(void); +#define LWIP_RAND() (lwip_port_rand())

+/* different handling for unit test, normally not needed */ +#ifdef LWIP_NOASSERT_ON_ERROR +#define LWIP_ERROR(message, expression, handler) do { if

(!(expression)) { \

...

•                                               handler; }} while
  

(0)

...

+#endif

+#define LWIP_DONT_PROVIDE_BYTEORDER_FUNCTIONS

+#define LWIP_PLATFORM_ASSERT(x) do {printf("Assertion "%s" failed at

line %d in %s\n", \

...

•                               x, __LINE__, __FILE__); } while (0)
  

+static inline int atoi(const char *str)

Can we use U-Boot's version?

#include <stdlib.h> /* getenv, atoi */ compiles but generates error on linking. I guess there is no atoi in U-Boot.

...

+{

...

•   int r = 0;
  

•   int i;
  

•   for (i = 0; str[i] != '\0'; ++i)
  

•           r = r * 10 + str[i] - '0';
  

•   return r;
  

+}

+#define LWIP_ERR_T int

+#endif /* LWIP_ARCH_CC_H */ diff --git a/lib/lwip/port/include/arch/sys_arch.h

b/lib/lwip/port/include/arch/sys_arch.h

...

new file mode 100644 index 0000000000..8d95146275 --- /dev/null +++ b/lib/lwip/port/include/arch/sys_arch.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#ifndef LWIP_ARCH_SYS_ARCH_H +#define LWIP_ARCH_SYS_ARCH_H

+#include "lwip/opt.h" +#include "lwip/arch.h" +#include "lwip/err.h"

+#define ERR_NEED_SCHED 123

+void sys_arch_msleep(u32_t delay_ms); +#define sys_msleep(ms) sys_arch_msleep(ms)

+#if SYS_LIGHTWEIGHT_PROT +typedef u32_t sys_prot_t; +#endif /* SYS_LIGHTWEIGHT_PROT */

+#include <errno.h>

+#define SYS_MBOX_NULL NULL +#define SYS_SEM_NULL NULL

+typedef u32_t sys_prot_t;

+struct sys_sem; +typedef struct sys_sem *sys_sem_t; +#define sys_sem_valid(sem) (((sem) != NULL) && (*(sem) != NULL)) +#define sys_sem_set_invalid(sem) do { if ((sem) != NULL) { *(sem) =

NULL; }} while (0)

...

+/* let sys.h use binary semaphores for mutexes */ +#define LWIP_COMPAT_MUTEX 1 +#define LWIP_COMPAT_MUTEX_ALLOWED 1

+struct sys_mbox; +typedef struct sys_mbox *sys_mbox_t; +#define sys_mbox_valid(mbox) (((mbox) != NULL) && (*(mbox) != NULL)) +#define sys_mbox_set_invalid(mbox) do { if ((mbox) != NULL) { *(mbox)

= NULL; }} while (0)

...

+struct sys_thread; +typedef struct sys_thread *sys_thread_t;

+static inline u32_t sys_arch_sem_wait(sys_sem_t *sem, u32_t timeout) +{

•   return 0;
  

+};

+static inline err_t sys_mbox_trypost(sys_mbox_t *mbox, void *msg) +{

•   return 0;
  

+};

+#define sys_sem_signal(s)

+#endif /* LWIP_ARCH_SYS_ARCH_H */ diff --git a/lib/lwip/port/include/limits.h

b/lib/lwip/port/include/limits.h

...

new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lib/lwip/port/sys-arch.c b/lib/lwip/port/sys-arch.c new file mode 100644 index 0000000000..609eeccf8c --- /dev/null +++ b/lib/lwip/port/sys-arch.c @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: GPL-2.0

+/*

• • (C) Copyright 2023 Linaro Ltd. maxim.uvarov@linaro.org
• */

+#include <common.h> +#include <rand.h> +#include "lwip/opt.h"

+u32_t sys_now(void) +{

•   return get_timer(0);
  

+}

+u32_t lwip_port_rand(void) +{

•   return (u32_t)rand();
  

+}

-- 2.30.2

Regards, Simon

Hi Maxim

On Wed, 2 Aug 2023 at 17:09, Maxim Uvarov maxim.uvarov@linaro.org wrote:

Signed-off-by: Maxim Uvarov maxim.uvarov@linaro.org

You need to try and be a bit more formal. This is past the RFC stage, you need commit messages that make sense.

Thanks /Ilias

---

.gitignore | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/.gitignore b/.gitignore index 3a4d056edf..9f2935f917 100644 --- a/.gitignore +++ b/.gitignore @@ -108,3 +108,12 @@ __pycache__

# moveconfig database /moveconfig.db

+lib/lwip/lwip-external +lib/lwip/apps/ping/ping.c +lib/lwip/apps/http/http_client.c +lib/lwip/apps/http/http_client.h +lib/lwip/apps/tftp/tftp.c +lib/lwip/apps/tftp/tftp_client.h +lib/lwip/apps/tftp/tftp_common.h

+lib/lwip/apps/tftp/tftp_example.h

2.30.2