Re: [lwip-devel] [PATCHv5 09/13] net/lwip: implement lwip port to u-boot

[Simon Glass]

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