Re: [PATCHv6 09/14] net/lwip: implement lwIP port to U-Boot
Hi all,
could you please remove the lwip-devel list from CC? I am interested in these
mails, but the more you dive into U-Boot specific things here, the less people
on our lwip list will be interested in this topic.
Thanks,
Simon
Am 18. August 2023 14:53:43 MESZ schrieb Maxim Uvarov :
>On Wed, 16 Aug 2023 at 15:01, Ilias Apalodimas
>wrote:
>
>> On Mon, Aug 14, 2023 at 07:32:48PM +0600, Maxim Uvarov wrote:
>> > Implement network lwIP interface connected to the U-boot.
>> > Keep original file structure widely used fro lwIP ports.
>> > (i.e. port/if.c port/sys-arch.c).
>>
>> What the patch does is obvious. Try to describe *why* we need this
>>
>> >
>> > Signed-off-by: Maxim Uvarov
>> > ---
>> > net/eth-uclass.c | 8 +
>> > net/lwip/port/if.c| 260 ++
>> > net/lwip/port/include/arch/cc.h | 39
>> > net/lwip/port/include/arch/sys_arch.h | 56 ++
>> > net/lwip/port/include/limits.h| 0
>> > net/lwip/port/sys-arch.c | 20 ++
>> > 6 files changed, 383 insertions(+)
>> > create mode 100644 net/lwip/port/if.c
>> > create mode 100644 net/lwip/port/include/arch/cc.h
>> > create mode 100644 net/lwip/port/include/arch/sys_arch.h
>> > create mode 100644 net/lwip/port/include/limits.h
>> > create mode 100644 net/lwip/port/sys-arch.c
>> >
>> > diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> > index c393600fab..6625f6d8a5 100644
>> > --- a/net/eth-uclass.c
>> > +++ b/net/eth-uclass.c
>> > @@ -32,6 +32,7 @@ DECLARE_GLOBAL_DATA_PTR;
>> > struct eth_device_priv {
>> > enum eth_state_t state;
>> > bool running;
>> > + ulwip ulwip;
>> > };
>> >
>> > /**
>> > @@ -347,6 +348,13 @@ int eth_init(void)
>> > return ret;
>> > }
>> >
>> > +struct ulwip *eth_lwip_priv(struct udevice *current)
>> > +{
>> > + struct eth_device_priv *priv = dev_get_uclass_priv(current);
>> > +
>> > + return &priv->ulwip;
>> > +}
>> > +
>> > void eth_halt(void)
>> > {
>> > struct udevice *current;
>> > diff --git a/net/lwip/port/if.c b/net/lwip/port/if.c
>> > new file mode 100644
>> > index 00..625a9c10bf
>> > --- /dev/null
>> > +++ b/net/lwip/port/if.c
>> > @@ -0,0 +1,260 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>> > +
>> > +/*
>> > + * (C) Copyright 2023 Linaro Ltd.
>> > + */
>> > +
>> > +#include
>> > +#include
>> > +#include
>> > +#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/ethip6.h"
>> > +
>> > +#include "lwip/ip.h"
>> > +
>> > +#define IFNAME0 'e'
>> > +#define IFNAME1 '0'
>>
>> Why is this needed and how was 'e0' chosen?
>> Dont we have a device name in the udevice struct?
>>
>>
>If we assume that we have lwip netif on top of an active U-Boot eth device,
>then it can be any name.
>
> /** descriptive abbreviation */
>
> char name[2];
>
>./net/lwip/lwip-external/contrib/examples/ethernetif/ethernetif.c:#define
>IFNAME0 'e'
>./net/lwip/lwip-external/contrib/examples/ethernetif/ethernetif.c:#define
>IFNAME1 'n'
>./net/lwip/lwip-external/contrib/ports/unix/port/netif/tapif.c:#define
>IFNAME0 't'
>./net/lwip/lwip-external/contrib/ports/unix/port/netif/tapif.c:#define
>IFNAME1 'p'
>./net/lwip/lwip-external/contrib/ports/unix/port/netif/vdeif.c:#define
>IFNAME0 'v'
>./net/lwip/lwip-external/contrib/ports/unix/port/netif/vdeif.c:#define
>IFNAME1 'd'
>./net/lwip/lwip-external/contrib/ports/win32/pcapif.c:#define IFNAME0
>'e'
>./net/lwip/lwip-external/contrib/ports/win32/pcapif.c:#define IFNAME1
>'0'
>./net/lwip/lwip-external/src/netif/bridgeif.c:#define IFNAME0 'b'
>./net/lwip/lwip-external/src/netif/bridgeif.c:#define IFNAME1 'r'
>./net/lwip/port/if.c:#define IFNAME0 'u'
>./net/lwip/port/if.c:#define IFNAME1 '0'
>
>
>
>> > +
>> > +static struct pbuf *low_level_input(struct netif *netif);
>> > +
>> > +int ulwip_enabled(void)
>> > +{
>> > + struct ulwip *ulwip;
>> > +
>> > + ulwip = eth_lwip_priv(eth_get_dev());
>>
>> eth_get_dev() can return NULL. There are various locations of this call
>> that needs fixing
>>
>
>
>ok.
>
>
>>
>> > + return ulwip->init_done;
>> > +}
>> > +
>> > +
>> > +struct ulwip_if {
>> > +};
>>
>> Why the forward declaration?
>>
>> > +
>> > +#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, 0)
>>
>> Why are we limiting the netmask to a class C network?
>>
>>
>that can be completely removed.
>
>
>> > +
>> > +void ulwip_poll(void)
>> > +{
>> > + struct pbuf *p;
>> > + int err;
>> > + struct netif *netif = netif_get_by_index(1);
>>
>> First of all netif can be NULL. Apart from that always requesting index 1
>> feels wrong. We should do something similar to eth_get_dev() and get the
>> *active* device correlation to an index
>>
>>
>I expect that it will
Re: [PATCHv6 09/14] net/lwip: implement lwIP port to U-Boot
On Wed, 16 Aug 2023 at 15:01, Ilias Apalodimas
wrote:
> On Mon, Aug 14, 2023 at 07:32:48PM +0600, Maxim Uvarov wrote:
> > Implement network lwIP interface connected to the U-boot.
> > Keep original file structure widely used fro lwIP ports.
> > (i.e. port/if.c port/sys-arch.c).
>
> What the patch does is obvious. Try to describe *why* we need this
>
> >
> > Signed-off-by: Maxim Uvarov
> > ---
> > net/eth-uclass.c | 8 +
> > net/lwip/port/if.c| 260 ++
> > net/lwip/port/include/arch/cc.h | 39
> > net/lwip/port/include/arch/sys_arch.h | 56 ++
> > net/lwip/port/include/limits.h| 0
> > net/lwip/port/sys-arch.c | 20 ++
> > 6 files changed, 383 insertions(+)
> > create mode 100644 net/lwip/port/if.c
> > create mode 100644 net/lwip/port/include/arch/cc.h
> > create mode 100644 net/lwip/port/include/arch/sys_arch.h
> > create mode 100644 net/lwip/port/include/limits.h
> > create mode 100644 net/lwip/port/sys-arch.c
> >
> > diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> > index c393600fab..6625f6d8a5 100644
> > --- a/net/eth-uclass.c
> > +++ b/net/eth-uclass.c
> > @@ -32,6 +32,7 @@ DECLARE_GLOBAL_DATA_PTR;
> > struct eth_device_priv {
> > enum eth_state_t state;
> > bool running;
> > + ulwip ulwip;
> > };
> >
> > /**
> > @@ -347,6 +348,13 @@ int eth_init(void)
> > return ret;
> > }
> >
> > +struct ulwip *eth_lwip_priv(struct udevice *current)
> > +{
> > + struct eth_device_priv *priv = dev_get_uclass_priv(current);
> > +
> > + return &priv->ulwip;
> > +}
> > +
> > void eth_halt(void)
> > {
> > struct udevice *current;
> > diff --git a/net/lwip/port/if.c b/net/lwip/port/if.c
> > new file mode 100644
> > index 00..625a9c10bf
> > --- /dev/null
> > +++ b/net/lwip/port/if.c
> > @@ -0,0 +1,260 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * (C) Copyright 2023 Linaro Ltd.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#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/ethip6.h"
> > +
> > +#include "lwip/ip.h"
> > +
> > +#define IFNAME0 'e'
> > +#define IFNAME1 '0'
>
> Why is this needed and how was 'e0' chosen?
> Dont we have a device name in the udevice struct?
>
>
If we assume that we have lwip netif on top of an active U-Boot eth device,
then it can be any name.
/** descriptive abbreviation */
char name[2];
./net/lwip/lwip-external/contrib/examples/ethernetif/ethernetif.c:#define
IFNAME0 'e'
./net/lwip/lwip-external/contrib/examples/ethernetif/ethernetif.c:#define
IFNAME1 'n'
./net/lwip/lwip-external/contrib/ports/unix/port/netif/tapif.c:#define
IFNAME0 't'
./net/lwip/lwip-external/contrib/ports/unix/port/netif/tapif.c:#define
IFNAME1 'p'
./net/lwip/lwip-external/contrib/ports/unix/port/netif/vdeif.c:#define
IFNAME0 'v'
./net/lwip/lwip-external/contrib/ports/unix/port/netif/vdeif.c:#define
IFNAME1 'd'
./net/lwip/lwip-external/contrib/ports/win32/pcapif.c:#define IFNAME0
'e'
./net/lwip/lwip-external/contrib/ports/win32/pcapif.c:#define IFNAME1
'0'
./net/lwip/lwip-external/src/netif/bridgeif.c:#define IFNAME0 'b'
./net/lwip/lwip-external/src/netif/bridgeif.c:#define IFNAME1 'r'
./net/lwip/port/if.c:#define IFNAME0 'u'
./net/lwip/port/if.c:#define IFNAME1 '0'
> > +
> > +static struct pbuf *low_level_input(struct netif *netif);
> > +
> > +int ulwip_enabled(void)
> > +{
> > + struct ulwip *ulwip;
> > +
> > + ulwip = eth_lwip_priv(eth_get_dev());
>
> eth_get_dev() can return NULL. There are various locations of this call
> that needs fixing
>
ok.
>
> > + return ulwip->init_done;
> > +}
> > +
> > +
> > +struct ulwip_if {
> > +};
>
> Why the forward declaration?
>
> > +
> > +#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, 0)
>
> Why are we limiting the netmask to a class C network?
>
>
that can be completely removed.
> > +
> > +void ulwip_poll(void)
> > +{
> > + struct pbuf *p;
> > + int err;
> > + struct netif *netif = netif_get_by_index(1);
>
> First of all netif can be NULL. Apart from that always requesting index 1
> feels wrong. We should do something similar to eth_get_dev() and get the
> *active* device correlation to an index
>
>
I expect that it will be 1 lwip netif for all eth defs. And only active eth
dev works with lwip.
This can be reconsidered but might be not a part of the initial patch set.
> > +
> > + p = low_level_input(netif);
> > + if (!p) {
> > + log_err("error p = low_level_input = NULL\n");
>
> This looks like a debug message.
> 'Network interface undefined' or something else, which is more readable.
>
> > + return;
> > + }
> > +
> > + /* ethern
Re: [PATCHv6 09/14] net/lwip: implement lwIP port to U-Boot
On Mon, Aug 14, 2023 at 07:32:48PM +0600, Maxim Uvarov wrote:
> Implement network lwIP interface connected to the U-boot.
> Keep original file structure widely used fro lwIP ports.
> (i.e. port/if.c port/sys-arch.c).
What the patch does is obvious. Try to describe *why* we need this
>
> Signed-off-by: Maxim Uvarov
> ---
> net/eth-uclass.c | 8 +
> net/lwip/port/if.c| 260 ++
> net/lwip/port/include/arch/cc.h | 39
> net/lwip/port/include/arch/sys_arch.h | 56 ++
> net/lwip/port/include/limits.h| 0
> net/lwip/port/sys-arch.c | 20 ++
> 6 files changed, 383 insertions(+)
> create mode 100644 net/lwip/port/if.c
> create mode 100644 net/lwip/port/include/arch/cc.h
> create mode 100644 net/lwip/port/include/arch/sys_arch.h
> create mode 100644 net/lwip/port/include/limits.h
> create mode 100644 net/lwip/port/sys-arch.c
>
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index c393600fab..6625f6d8a5 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -32,6 +32,7 @@ DECLARE_GLOBAL_DATA_PTR;
> struct eth_device_priv {
> enum eth_state_t state;
> bool running;
> + ulwip ulwip;
> };
>
> /**
> @@ -347,6 +348,13 @@ int eth_init(void)
> return ret;
> }
>
> +struct ulwip *eth_lwip_priv(struct udevice *current)
> +{
> + struct eth_device_priv *priv = dev_get_uclass_priv(current);
> +
> + return &priv->ulwip;
> +}
> +
> void eth_halt(void)
> {
> struct udevice *current;
> diff --git a/net/lwip/port/if.c b/net/lwip/port/if.c
> new file mode 100644
> index 00..625a9c10bf
> --- /dev/null
> +++ b/net/lwip/port/if.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * (C) Copyright 2023 Linaro Ltd.
> + */
> +
> +#include
> +#include
> +#include
> +#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/ethip6.h"
> +
> +#include "lwip/ip.h"
> +
> +#define IFNAME0 'e'
> +#define IFNAME1 '0'
Why is this needed and how was 'e0' chosen?
Dont we have a device name in the udevice struct?
> +
> +static struct pbuf *low_level_input(struct netif *netif);
> +
> +int ulwip_enabled(void)
> +{
> + struct ulwip *ulwip;
> +
> + ulwip = eth_lwip_priv(eth_get_dev());
eth_get_dev() can return NULL. There are various locations of this call
that needs fixing
> + return ulwip->init_done;
> +}
> +
> +
> +struct ulwip_if {
> +};
Why the forward declaration?
> +
> +#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, 0)
Why are we limiting the netmask to a class C network?
> +
> +void ulwip_poll(void)
> +{
> + struct pbuf *p;
> + int err;
> + struct netif *netif = netif_get_by_index(1);
First of all netif can be NULL. Apart from that always requesting index 1
feels wrong. We should do something similar to eth_get_dev() and get the
*active* device correlation to an index
> +
> + p = low_level_input(netif);
> + if (!p) {
> + log_err("error p = low_level_input = NULL\n");
This looks like a debug message.
'Network interface undefined' or something else, which is more readable.
> + return;
> + }
> +
> + /* ethernet_input always returns ERR_OK */
> + err = ethernet_input(p, netif);
> + if (err)
> + log_err("ip4_input err %d\n", err);
> +
> + return;
> +}
> +
> +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;
> +
> + /* We allocate a pbuf chain of pbufs from the pool. */
> + p = pbuf_alloc(PBUF_RAW, len, PBUF_POOL);
> + if (p) {
if (!p) and reverse the logic
> + /* 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
> + * 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();
> +
> + LINK_STATS_INC(link.recv);
> + } else {
> +
[PATCHv6 09/14] net/lwip: implement lwIP port to U-Boot
Implement network lwIP interface connected to the U-boot.
Keep original file structure widely used fro lwIP ports.
(i.e. port/if.c port/sys-arch.c).
Signed-off-by: Maxim Uvarov
---
net/eth-uclass.c | 8 +
net/lwip/port/if.c| 260 ++
net/lwip/port/include/arch/cc.h | 39
net/lwip/port/include/arch/sys_arch.h | 56 ++
net/lwip/port/include/limits.h| 0
net/lwip/port/sys-arch.c | 20 ++
6 files changed, 383 insertions(+)
create mode 100644 net/lwip/port/if.c
create mode 100644 net/lwip/port/include/arch/cc.h
create mode 100644 net/lwip/port/include/arch/sys_arch.h
create mode 100644 net/lwip/port/include/limits.h
create mode 100644 net/lwip/port/sys-arch.c
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index c393600fab..6625f6d8a5 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -32,6 +32,7 @@ DECLARE_GLOBAL_DATA_PTR;
struct eth_device_priv {
enum eth_state_t state;
bool running;
+ ulwip ulwip;
};
/**
@@ -347,6 +348,13 @@ int eth_init(void)
return ret;
}
+struct ulwip *eth_lwip_priv(struct udevice *current)
+{
+ struct eth_device_priv *priv = dev_get_uclass_priv(current);
+
+ return &priv->ulwip;
+}
+
void eth_halt(void)
{
struct udevice *current;
diff --git a/net/lwip/port/if.c b/net/lwip/port/if.c
new file mode 100644
index 00..625a9c10bf
--- /dev/null
+++ b/net/lwip/port/if.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * (C) Copyright 2023 Linaro Ltd.
+ */
+
+#include
+#include
+#include
+#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/ethip6.h"
+
+#include "lwip/ip.h"
+
+#define IFNAME0 'e'
+#define IFNAME1 '0'
+
+static struct pbuf *low_level_input(struct netif *netif);
+
+int ulwip_enabled(void)
+{
+ struct ulwip *ulwip;
+
+ ulwip = eth_lwip_priv(eth_get_dev());
+ return ulwip->init_done;
+}
+
+int ulwip_in_loop(void)
+{
+ struct ulwip *ulwip;
+
+ ulwip = eth_lwip_priv(eth_get_dev());
+ return ulwip->loop;
+}
+
+void ulwip_loop_set(int loop)
+{
+ struct ulwip *ulwip;
+
+ ulwip = eth_lwip_priv(eth_get_dev());
+ ulwip->loop = loop;
+}
+
+void ulwip_exit(int err)
+{
+ struct ulwip *ulwip;
+
+ ulwip = eth_lwip_priv(eth_get_dev());
+ ulwip->loop = 0;
+ ulwip->err = err;
+}
+
+int ulwip_app_get_err(void)
+{
+ struct ulwip *ulwip;
+
+ ulwip = eth_lwip_priv(eth_get_dev());
+ return ulwip->err;
+}
+
+struct ulwip_if {
+};
+
+#define LWIP_PORT_INIT_NETMASK(addr) IP4_ADDR((addr), 255, 255, 255, 0)
+
+void ulwip_poll(void)
+{
+ struct pbuf *p;
+ int err;
+ struct netif *netif = netif_get_by_index(1);
+
+ p = low_level_input(netif);
+ if (!p) {
+ log_err("error p = low_level_input = NULL\n");
+ return;
+ }
+
+ /* ethernet_input always returns ERR_OK */
+ err = ethernet_input(p, netif);
+ if (err)
+ log_err("ip4_input err %d\n", err);
+
+ return;
+}
+
+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;
+
+ /* We allocate a pbuf chain of pbufs from the pool. */
+ p = pbuf_alloc(PBUF_RAW, len, PBUF_POOL);
+ if (p) {
+ /* 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
+* 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();
+
+ 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 */
+
