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

[Simon Glass]

Hi Maxim,

On Tue, 8 Aug 2023 at 04:07, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>
>
>
> 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
>>>

[..]

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

I mean that you can follow what global_data.h does.

gd_malloc_start() is an example of using a header-file construct to
avoid #ifdef in the source.

[..]

>>> > +#if LWIP_IPV6
>>>
>>> if ()
>>>
>
> No, we are using lwip header files. it will not work if LWIP_IPV6 set to 0.

In general we should always include all header files needed for the
source, and this should be harmless. We don't normally #ifdef header
files.

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

No...simple_strtoul() is similar so you could call that from your atoi().

Regards,
Simon