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

Maxim Uvarov
2023-08-03T16:21:39Z

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

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

>
> > + */
> > +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
>
>
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)
>
> > +#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
>