All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers
@ 2018-06-26 21:19 Joe Hershberger
  2018-06-26 21:19 ` [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv Joe Hershberger
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

It seems as sandbox moved to livetree these drivers were not updated.
Heinrich suggested correctly that at this point we should be enumerating
network interface names, not hard-coding eth0.
Also, there were a few bugs that needed fixing.


Joe Hershberger (5):
  sandbox: eth-raw: Correct valid socket test in send/recv
  net: Correct comment in Kconfig
  net: sandbox: Convert sandbox mock eth driver to livetree
  net: sandbox-raw: Convert raw eth driver to livetree
  net: sandbox-raw: Allow interface to be specified by index

 arch/sandbox/cpu/eth-raw-os.c         | 32 ++++++++++++++++++++++++++++++--
 arch/sandbox/dts/sandbox.dts          |  2 +-
 arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
 drivers/net/Kconfig                   |  2 +-
 drivers/net/sandbox-raw.c             | 28 ++++++++++++++++++----------
 drivers/net/sandbox.c                 | 15 +++++++++++----
 6 files changed, 64 insertions(+), 19 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv
  2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
@ 2018-06-26 21:19 ` Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
  2018-06-26 21:19 ` [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig Joe Hershberger
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

In open, the socket is correctly checked to be -1 in the error case.
In send and recv, we checked for 0, but that is a valid socket number.

Correct this by checking for -1 as a bad socket everywhere.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/cpu/eth-raw-os.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
index e054a0702a..61f23ed210 100644
--- a/arch/sandbox/cpu/eth-raw-os.c
+++ b/arch/sandbox/cpu/eth-raw-os.c
@@ -156,7 +156,7 @@ int sandbox_eth_raw_os_send(void *packet, int length,
 	int retval;
 	struct udphdr *udph = packet + sizeof(struct iphdr);
 
-	if (!priv->sd || !priv->device)
+	if (priv->sd < 0 || !priv->device)
 		return -EINVAL;
 
 	/*
@@ -221,7 +221,7 @@ int sandbox_eth_raw_os_recv(void *packet, int *length,
 	int retval;
 	int saddr_size;
 
-	if (!priv->sd || !priv->device)
+	if (priv->sd < 0 || !priv->device)
 		return -EINVAL;
 	saddr_size = sizeof(struct sockaddr);
 	retval = recvfrom(priv->sd, packet, 1536, 0,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig
  2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
  2018-06-26 21:19 ` [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv Joe Hershberger
@ 2018-06-26 21:19 ` Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
  2018-06-26 21:19 ` [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree Joe Hershberger
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 drivers/net/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index e88f056d84..5f26a0004a 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -8,7 +8,7 @@ config DM_ETH
 	  Enable driver model for Ethernet.
 
 	  The eth_*() interface will be implemented by the UC_ETH class
-	  This is currently implemented in net/eth.c
+	  This is currently implemented in net/eth-uclass.c
 	  Look in include/net.h for details.
 
 config DRIVER_TI_CPSW
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree
  2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
  2018-06-26 21:19 ` [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv Joe Hershberger
  2018-06-26 21:19 ` [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig Joe Hershberger
@ 2018-06-26 21:19 ` Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
  2018-06-26 21:19 ` [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw " Joe Hershberger
  2018-06-26 21:19 ` [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index Joe Hershberger
  4 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

Use the dev_ functions to access DT properties.

Also correct the reading of the fake MAC address. The format from the DT
is u32s, so to accurately read the MAC from the DT, we need to cast each
value to a u8.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 drivers/net/sandbox.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index b34712bd06..d5a30c05b8 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -56,13 +56,20 @@ void sandbox_eth_skip_timeout(void)
 static int sb_eth_start(struct udevice *dev)
 {
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
+	/* The DT integers are 32-bits */
+	const u32 mac[ARP_HLEN];
 
 	debug("eth_sandbox: Start\n");
 
-	fdtdec_get_byte_array(gd->fdt_blob, dev_of_offset(dev),
-			      "fake-host-hwaddr", priv->fake_host_hwaddr,
-			      ARP_HLEN);
+	if (dev_read_u32_array(dev, "fake-host-hwaddr", mac, ARP_HLEN)) {
+		printf("'fake-host-hwaddr' is missing from the DT\n");
+		return -EINVAL;
+	}
+	for (int i = 0; i < ARP_HLEN; i++)
+		priv->fake_host_hwaddr[i] = (uint8_t)mac[i];
+
 	priv->recv_packet_buffer = net_rx_packets[0];
+
 	return 0;
 }
 
@@ -204,7 +211,7 @@ static int sb_eth_ofdata_to_platdata(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 
-	pdata->iobase = devfdt_get_addr(dev);
+	pdata->iobase = dev_read_addr(dev);
 	return 0;
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw eth driver to livetree
  2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
                   ` (2 preceding siblings ...)
  2018-06-26 21:19 ` [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree Joe Hershberger
@ 2018-06-26 21:19 ` Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
  2018-06-26 21:19 ` [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index Joe Hershberger
  4 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 drivers/net/sandbox-raw.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
index 3f8020f629..f835a6a7f3 100644
--- a/drivers/net/sandbox-raw.c
+++ b/drivers/net/sandbox-raw.c
@@ -25,17 +25,19 @@ static int sb_eth_raw_start(struct udevice *dev)
 
 	debug("eth_sandbox_raw: Start\n");
 
-	interface = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
-					    "host-raw-interface", NULL);
-	if (interface == NULL)
-		return -EINVAL;
-
-	if (strcmp(interface, "lo") == 0) {
-		priv->local = 1;
-		env_set("ipaddr", "127.0.0.1");
-		env_set("serverip", "127.0.0.1");
+	interface = dev_read_prop(dev, "host-raw-interface", NULL);
+	if (interface) {
+		printf("eth_sandbox_raw: Using %s from DT\n", interface);
+		if (strcmp(interface, "lo") == 0) {
+			priv->local = 1;
+			env_set("ipaddr", "127.0.0.1");
+			env_set("serverip", "127.0.0.1");
+		}
+		return sandbox_eth_raw_os_start(interface, pdata->enetaddr,
+						priv);
 	}
-	return sandbox_eth_raw_os_start(interface, pdata->enetaddr, priv);
+
+	return -EINVAL;
 }
 
 static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
@@ -144,7 +146,7 @@ static int sb_eth_raw_ofdata_to_platdata(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 
-	pdata->iobase = devfdt_get_addr(dev);
+	pdata->iobase = dev_read_addr(dev);
 	return 0;
 }
 
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
                   ` (3 preceding siblings ...)
  2018-06-26 21:19 ` [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw " Joe Hershberger
@ 2018-06-26 21:19 ` Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
  2018-06-27  1:15   ` Heinrich Schuchardt
  4 siblings, 2 replies; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 21:19 UTC (permalink / raw)
  To: u-boot

With systemd stable interface names, eth0 will almost never exist.
Instead of using that name in the sandbox.dts, use index 2 (the first
interface after "lo"). Enumerate the interfaces on the system to choose
a valid interace name.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
---

 arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
 arch/sandbox/dts/sandbox.dts          |  2 +-
 arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
 drivers/net/sandbox-raw.c             |  8 +++++++-
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
index 61f23ed210..11b5271f31 100644
--- a/arch/sandbox/cpu/eth-raw-os.c
+++ b/arch/sandbox/cpu/eth-raw-os.c
@@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
 		return _raw_packet_start(ifname, ethmac, priv);
 }
 
+int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
+				 struct eth_sandbox_raw_priv *priv)
+{
+	int ret;
+	struct if_nameindex *ni, *i;
+	const char *ifname = NULL;
+
+	ni = if_nameindex();
+	if (!ni)
+		return -EINVAL;
+
+	for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
+		if (ifindex == i->if_index) {
+			ifname = i->if_name;
+			printf("(%s)\n", ifname);
+			break;
+		}
+	}
+	/* Don't bother supporting 'lo' by index. Just use the name in DTS */
+	if (ifname)
+		ret = _raw_packet_start(ifname, ethmac, priv);
+	else
+		ret = -EINVAL;
+
+	if_freenameindex(ni);
+	return ret;
+}
+
 int sandbox_eth_raw_os_send(void *packet, int length,
 			    struct eth_sandbox_raw_priv *priv)
 {
diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
index 0ea2452742..e7f6c194d0 100644
--- a/arch/sandbox/dts/sandbox.dts
+++ b/arch/sandbox/dts/sandbox.dts
@@ -56,7 +56,7 @@
 	eth at 80000000 {
 		compatible = "sandbox,eth-raw";
 		reg = <0x80000000 0x1000>;
-		host-raw-interface = "eth0";
+		host-raw-interface-idx = <2>;
 	};
 
 	eth at 90000000 {
diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
index f986d3142d..70b29b4508 100644
--- a/arch/sandbox/include/asm/eth-raw-os.h
+++ b/arch/sandbox/include/asm/eth-raw-os.h
@@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
 };
 
 int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
-			    struct eth_sandbox_raw_priv *priv);
+			     struct eth_sandbox_raw_priv *priv);
+int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
+				 struct eth_sandbox_raw_priv *priv);
 int sandbox_eth_raw_os_send(void *packet, int length,
 			    struct eth_sandbox_raw_priv *priv);
 int sandbox_eth_raw_os_recv(void *packet, int *length,
diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
index f835a6a7f3..3bde8d84ce 100644
--- a/drivers/net/sandbox-raw.c
+++ b/drivers/net/sandbox-raw.c
@@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
 	struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	const char *interface;
+	u32 index;
 
 	debug("eth_sandbox_raw: Start\n");
 
@@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
 						priv);
 	}
 
-	return -EINVAL;
+	if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
+		return -EINVAL;
+
+	printf("eth_sandbox_raw: Using interface index %d from DT ", index);
+
+	return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
 }
 
 static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv
  2018-06-26 21:19 ` [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv Joe Hershberger
@ 2018-06-26 23:18   ` Simon Glass
  0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:18 UTC (permalink / raw)
  To: u-boot

On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
> In open, the socket is correctly checked to be -1 in the error case.
> In send and recv, we checked for 0, but that is a valid socket number.
>
> Correct this by checking for -1 as a bad socket everywhere.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/cpu/eth-raw-os.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig
  2018-06-26 21:19 ` [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig Joe Hershberger
@ 2018-06-26 23:18   ` Simon Glass
  0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:18 UTC (permalink / raw)
  To: u-boot

On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  drivers/net/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree
  2018-06-26 21:19 ` [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree Joe Hershberger
@ 2018-06-26 23:18   ` Simon Glass
  2018-06-26 23:25     ` Joe Hershberger
  0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:18 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Use the dev_ functions to access DT properties.
>
> Also correct the reading of the fake MAC address. The format from the DT
> is u32s, so to accurately read the MAC from the DT, we need to cast each
> value to a u8.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  drivers/net/sandbox.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
> index b34712bd06..d5a30c05b8 100644
> --- a/drivers/net/sandbox.c
> +++ b/drivers/net/sandbox.c
> @@ -56,13 +56,20 @@ void sandbox_eth_skip_timeout(void)
>  static int sb_eth_start(struct udevice *dev)
>  {
>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
> +       /* The DT integers are 32-bits */
> +       const u32 mac[ARP_HLEN];
>
>         debug("eth_sandbox: Start\n");
>
> -       fdtdec_get_byte_array(gd->fdt_blob, dev_of_offset(dev),
> -                             "fake-host-hwaddr", priv->fake_host_hwaddr,
> -                             ARP_HLEN);
> +       if (dev_read_u32_array(dev, "fake-host-hwaddr", mac, ARP_HLEN)) {
> +               printf("'fake-host-hwaddr' is missing from the DT\n");
> +               return -EINVAL;
> +       }

This is not equivalent  - I think you need a dev_read_u8_array() or similar.

> +       for (int i = 0; i < ARP_HLEN; i++)
> +               priv->fake_host_hwaddr[i] = (uint8_t)mac[i];
> +
>         priv->recv_packet_buffer = net_rx_packets[0];
> +
>         return 0;
>  }
>
> @@ -204,7 +211,7 @@ static int sb_eth_ofdata_to_platdata(struct udevice *dev)
>  {
>         struct eth_pdata *pdata = dev_get_platdata(dev);
>
> -       pdata->iobase = devfdt_get_addr(dev);
> +       pdata->iobase = dev_read_addr(dev);
>         return 0;
>  }
>
> --
> 2.11.0
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw eth driver to livetree
  2018-06-26 21:19 ` [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw " Joe Hershberger
@ 2018-06-26 23:18   ` Simon Glass
  2018-06-26 23:28     ` Joe Hershberger
  0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:18 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  drivers/net/sandbox-raw.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
> index 3f8020f629..f835a6a7f3 100644
> --- a/drivers/net/sandbox-raw.c
> +++ b/drivers/net/sandbox-raw.c
> @@ -25,17 +25,19 @@ static int sb_eth_raw_start(struct udevice *dev)
>
>         debug("eth_sandbox_raw: Start\n");
>
> -       interface = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
> -                                           "host-raw-interface", NULL);
> -       if (interface == NULL)
> -               return -EINVAL;
> -
> -       if (strcmp(interface, "lo") == 0) {
> -               priv->local = 1;
> -               env_set("ipaddr", "127.0.0.1");
> -               env_set("serverip", "127.0.0.1");
> +       interface = dev_read_prop(dev, "host-raw-interface", NULL);

dev_read_string() ?

> +       if (interface) {

Can you do:

if (!interface)
   return -EINVAL;

> +               printf("eth_sandbox_raw: Using %s from DT\n", interface);
> +               if (strcmp(interface, "lo") == 0) {
> +                       priv->local = 1;
> +                       env_set("ipaddr", "127.0.0.1");
> +                       env_set("serverip", "127.0.0.1");
> +               }
> +               return sandbox_eth_raw_os_start(interface, pdata->enetaddr,
> +                                               priv);
>         }
> -       return sandbox_eth_raw_os_start(interface, pdata->enetaddr, priv);
> +
> +       return -EINVAL;
>  }
>
>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
> @@ -144,7 +146,7 @@ static int sb_eth_raw_ofdata_to_platdata(struct udevice *dev)
>  {
>         struct eth_pdata *pdata = dev_get_platdata(dev);
>
> -       pdata->iobase = devfdt_get_addr(dev);
> +       pdata->iobase = dev_read_addr(dev);
>         return 0;
>  }
>
> --
> 2.11.0
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-26 21:19 ` [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index Joe Hershberger
@ 2018-06-26 23:18   ` Simon Glass
  2018-06-26 23:31     ` Joe Hershberger
  2018-06-27  1:15   ` Heinrich Schuchardt
  1 sibling, 1 reply; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:18 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
> With systemd stable interface names, eth0 will almost never exist.
> Instead of using that name in the sandbox.dts, use index 2 (the first
> interface after "lo"). Enumerate the interfaces on the system to choose
> a valid interace name.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>
>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>  arch/sandbox/dts/sandbox.dts          |  2 +-
>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>  drivers/net/sandbox-raw.c             |  8 +++++++-
>  4 files changed, 39 insertions(+), 3 deletions(-)
>

This seems brittle too. A name seems better to me. I might be missing
something though. Can we ask the OS for a name, etc?

Regards,
Simon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree
  2018-06-26 23:18   ` Simon Glass
@ 2018-06-26 23:25     ` Joe Hershberger
  2018-06-26 23:28       ` Simon Glass
  0 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 23:25 UTC (permalink / raw)
  To: u-boot

Hi Simon,

On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> Use the dev_ functions to access DT properties.
>>
>> Also correct the reading of the fake MAC address. The format from the DT
>> is u32s, so to accurately read the MAC from the DT, we need to cast each
>> value to a u8.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>
>>  drivers/net/sandbox.c | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
>> index b34712bd06..d5a30c05b8 100644
>> --- a/drivers/net/sandbox.c
>> +++ b/drivers/net/sandbox.c
>> @@ -56,13 +56,20 @@ void sandbox_eth_skip_timeout(void)
>>  static int sb_eth_start(struct udevice *dev)
>>  {
>>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
>> +       /* The DT integers are 32-bits */
>> +       const u32 mac[ARP_HLEN];
>>
>>         debug("eth_sandbox: Start\n");
>>
>> -       fdtdec_get_byte_array(gd->fdt_blob, dev_of_offset(dev),
>> -                             "fake-host-hwaddr", priv->fake_host_hwaddr,
>> -                             ARP_HLEN);
>> +       if (dev_read_u32_array(dev, "fake-host-hwaddr", mac, ARP_HLEN)) {
>> +               printf("'fake-host-hwaddr' is missing from the DT\n");
>> +               return -EINVAL;
>> +       }
>
> This is not equivalent  - I think you need a dev_read_u8_array() or similar.

Yes, I know it's not equivalent... I noted that in the commit log.
This is fixing a bug too.

>
>> +       for (int i = 0; i < ARP_HLEN; i++)
>> +               priv->fake_host_hwaddr[i] = (uint8_t)mac[i];
>> +
>>         priv->recv_packet_buffer = net_rx_packets[0];
>> +
>>         return 0;
>>  }
>>
>> @@ -204,7 +211,7 @@ static int sb_eth_ofdata_to_platdata(struct udevice *dev)
>>  {
>>         struct eth_pdata *pdata = dev_get_platdata(dev);
>>
>> -       pdata->iobase = devfdt_get_addr(dev);
>> +       pdata->iobase = dev_read_addr(dev);
>>         return 0;
>>  }
>>
>> --
>> 2.11.0
>>
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree
  2018-06-26 23:25     ` Joe Hershberger
@ 2018-06-26 23:28       ` Simon Glass
  2018-06-27  0:29         ` Joe Hershberger
  0 siblings, 1 reply; 22+ messages in thread
From: Simon Glass @ 2018-06-26 23:28 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On 26 June 2018 at 16:25, Joe Hershberger <joe.hershberger@ni.com> wrote:
> Hi Simon,
>
> On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Joe,
>>
>> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>> Use the dev_ functions to access DT properties.
>>>
>>> Also correct the reading of the fake MAC address. The format from the DT
>>> is u32s, so to accurately read the MAC from the DT, we need to cast each
>>> value to a u8.
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>
>>>  drivers/net/sandbox.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
>>> index b34712bd06..d5a30c05b8 100644
>>> --- a/drivers/net/sandbox.c
>>> +++ b/drivers/net/sandbox.c
>>> @@ -56,13 +56,20 @@ void sandbox_eth_skip_timeout(void)
>>>  static int sb_eth_start(struct udevice *dev)
>>>  {
>>>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
>>> +       /* The DT integers are 32-bits */
>>> +       const u32 mac[ARP_HLEN];
>>>
>>>         debug("eth_sandbox: Start\n");
>>>
>>> -       fdtdec_get_byte_array(gd->fdt_blob, dev_of_offset(dev),
>>> -                             "fake-host-hwaddr", priv->fake_host_hwaddr,
>>> -                             ARP_HLEN);
>>> +       if (dev_read_u32_array(dev, "fake-host-hwaddr", mac, ARP_HLEN)) {
>>> +               printf("'fake-host-hwaddr' is missing from the DT\n");
>>> +               return -EINVAL;
>>> +       }
>>
>> This is not equivalent  - I think you need a dev_read_u8_array() or similar.
>
> Yes, I know it's not equivalent... I noted that in the commit log.
> This is fixing a bug too.

Yes I see that, but I didn't realise there was a bug in the code.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw eth driver to livetree
  2018-06-26 23:18   ` Simon Glass
@ 2018-06-26 23:28     ` Joe Hershberger
  0 siblings, 0 replies; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 23:28 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>
>>  drivers/net/sandbox-raw.c | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
>>
>> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>> index 3f8020f629..f835a6a7f3 100644
>> --- a/drivers/net/sandbox-raw.c
>> +++ b/drivers/net/sandbox-raw.c
>> @@ -25,17 +25,19 @@ static int sb_eth_raw_start(struct udevice *dev)
>>
>>         debug("eth_sandbox_raw: Start\n");
>>
>> -       interface = fdt_getprop(gd->fdt_blob, dev_of_offset(dev),
>> -                                           "host-raw-interface", NULL);
>> -       if (interface == NULL)
>> -               return -EINVAL;
>> -
>> -       if (strcmp(interface, "lo") == 0) {
>> -               priv->local = 1;
>> -               env_set("ipaddr", "127.0.0.1");
>> -               env_set("serverip", "127.0.0.1");
>> +       interface = dev_read_prop(dev, "host-raw-interface", NULL);
>
> dev_read_string() ?

OK.

>
>> +       if (interface) {
>
> Can you do:
>
> if (!interface)
>    return -EINVAL;

I realize it looks a bit awkward in this patch, but it's laid out this
way to accommodate patch 5/5.

>> +               printf("eth_sandbox_raw: Using %s from DT\n", interface);
>> +               if (strcmp(interface, "lo") == 0) {
>> +                       priv->local = 1;
>> +                       env_set("ipaddr", "127.0.0.1");
>> +                       env_set("serverip", "127.0.0.1");
>> +               }
>> +               return sandbox_eth_raw_os_start(interface, pdata->enetaddr,
>> +                                               priv);
>>         }
>> -       return sandbox_eth_raw_os_start(interface, pdata->enetaddr, priv);
>> +
>> +       return -EINVAL;
>>  }
>>
>>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
>> @@ -144,7 +146,7 @@ static int sb_eth_raw_ofdata_to_platdata(struct udevice *dev)
>>  {
>>         struct eth_pdata *pdata = dev_get_platdata(dev);
>>
>> -       pdata->iobase = devfdt_get_addr(dev);
>> +       pdata->iobase = dev_read_addr(dev);
>>         return 0;
>>  }
>>
>> --
>> 2.11.0
>>
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-26 23:18   ` Simon Glass
@ 2018-06-26 23:31     ` Joe Hershberger
  2018-06-27  4:41       ` Simon Glass
  0 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-26 23:31 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> With systemd stable interface names, eth0 will almost never exist.
>> Instead of using that name in the sandbox.dts, use index 2 (the first
>> interface after "lo"). Enumerate the interfaces on the system to choose
>> a valid interace name.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>
>>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>  arch/sandbox/dts/sandbox.dts          |  2 +-
>>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>  drivers/net/sandbox-raw.c             |  8 +++++++-
>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>
>
> This seems brittle too. A name seems better to me. I might be missing
> something though. Can we ask the OS for a name, etc?

This asks for the equivalent of "eth0", but asks the OS to tell us the
actual name. I don't know that we can possibly choose the correct
network interface to use across any machine configuration. Choosing
the first one, whatever it's name is, seems like a pretty reliable way
to get a good interface in most cases.

>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree
  2018-06-26 23:28       ` Simon Glass
@ 2018-06-27  0:29         ` Joe Hershberger
  0 siblings, 0 replies; 22+ messages in thread
From: Joe Hershberger @ 2018-06-27  0:29 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 26, 2018 at 6:28 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Joe,
>
> On 26 June 2018 at 16:25, Joe Hershberger <joe.hershberger@ni.com> wrote:
>> Hi Simon,
>>
>> On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Joe,
>>>
>>> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>>> Use the dev_ functions to access DT properties.
>>>>
>>>> Also correct the reading of the fake MAC address. The format from the DT
>>>> is u32s, so to accurately read the MAC from the DT, we need to cast each
>>>> value to a u8.
>>>>
>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>>> ---
>>>>
>>>>  drivers/net/sandbox.c | 15 +++++++++++----
>>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
>>>> index b34712bd06..d5a30c05b8 100644
>>>> --- a/drivers/net/sandbox.c
>>>> +++ b/drivers/net/sandbox.c
>>>> @@ -56,13 +56,20 @@ void sandbox_eth_skip_timeout(void)
>>>>  static int sb_eth_start(struct udevice *dev)
>>>>  {
>>>>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
>>>> +       /* The DT integers are 32-bits */
>>>> +       const u32 mac[ARP_HLEN];
>>>>
>>>>         debug("eth_sandbox: Start\n");
>>>>
>>>> -       fdtdec_get_byte_array(gd->fdt_blob, dev_of_offset(dev),
>>>> -                             "fake-host-hwaddr", priv->fake_host_hwaddr,
>>>> -                             ARP_HLEN);
>>>> +       if (dev_read_u32_array(dev, "fake-host-hwaddr", mac, ARP_HLEN)) {
>>>> +               printf("'fake-host-hwaddr' is missing from the DT\n");
>>>> +               return -EINVAL;
>>>> +       }
>>>
>>> This is not equivalent  - I think you need a dev_read_u8_array() or similar.
>>
>> Yes, I know it's not equivalent... I noted that in the commit log.
>> This is fixing a bug too.
>
> Yes I see that, but I didn't realise there was a bug in the code.

OK, turns out this is the result of an inconsistency in the dts files,
and should actually be able to be u8s.

I'll correct it for v2.

> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-26 21:19 ` [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index Joe Hershberger
  2018-06-26 23:18   ` Simon Glass
@ 2018-06-27  1:15   ` Heinrich Schuchardt
  2018-06-27  4:58     ` Joe Hershberger
  1 sibling, 1 reply; 22+ messages in thread
From: Heinrich Schuchardt @ 2018-06-27  1:15 UTC (permalink / raw)
  To: u-boot

On 06/26/2018 11:19 PM, Joe Hershberger wrote:
> With systemd stable interface names, eth0 will almost never exist.
> Instead of using that name in the sandbox.dts, use index 2 (the first
> interface after "lo"). Enumerate the interfaces on the system to choose
> a valid interace name.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>

Hello Joe,

thanks for addressing this issue.

On my laptop the raw interfaces are currently called:
1 - lo - loopback
2 - enp0s25 - wired ethernet
3 - wlp12s0 - WLAN

But this list is not static:

It is allowable to define multiple loopback interfaces. Additional
bridges, tap and tun interfaces may be defined. A computer may also have
multiple wired and wireless interfaces. And of cause I may choose to
plug in an additional ethernet adapter while my computer is running.

The interface index does not tell which interface is actually connected
to the network. My laptop uses wpl12s0 (index = 3) for sending out this
mail. So, please, do not replace one invalid assumption (eth0) by
another (index = 2).

I doubt there is a guarantee that index 1 is a loopback interface on
POSIX systems. If you need a special treatment of loopback devices you
could analyze the properties of each interface to detect which one is a
loopback interface.

There is no need for using the device tree for the specification of
interfaces. Instead a raw ethernet device can be set up for each
interface. Let the user choose at runtime which device he wants to use
for testing.

Best regards

Heinrich

> ---
> 
>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>  arch/sandbox/dts/sandbox.dts          |  2 +-
>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>  drivers/net/sandbox-raw.c             |  8 +++++++-
>  4 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
> index 61f23ed210..11b5271f31 100644
> --- a/arch/sandbox/cpu/eth-raw-os.c
> +++ b/arch/sandbox/cpu/eth-raw-os.c
> @@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>  		return _raw_packet_start(ifname, ethmac, priv);
>  }
>  
> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
> +				 struct eth_sandbox_raw_priv *priv)
> +{
> +	int ret;
> +	struct if_nameindex *ni, *i;
> +	const char *ifname = NULL;
> +
> +	ni = if_nameindex();
> +	if (!ni)
> +		return -EINVAL;
> +
> +	for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
> +		if (ifindex == i->if_index) {
> +			ifname = i->if_name;
> +			printf("(%s)\n", ifname);
> +			break;
> +		}
> +	}
> +	/* Don't bother supporting 'lo' by index. Just use the name in DTS */
> +	if (ifname)
> +		ret = _raw_packet_start(ifname, ethmac, priv);
> +	else
> +		ret = -EINVAL;
> +
> +	if_freenameindex(ni);
> +	return ret;
> +}
> +
>  int sandbox_eth_raw_os_send(void *packet, int length,
>  			    struct eth_sandbox_raw_priv *priv)
>  {
> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
> index 0ea2452742..e7f6c194d0 100644
> --- a/arch/sandbox/dts/sandbox.dts
> +++ b/arch/sandbox/dts/sandbox.dts
> @@ -56,7 +56,7 @@
>  	eth at 80000000 {
>  		compatible = "sandbox,eth-raw";
>  		reg = <0x80000000 0x1000>;
> -		host-raw-interface = "eth0";
> +		host-raw-interface-idx = <2>;
>  	};
>  
>  	eth at 90000000 {
> diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
> index f986d3142d..70b29b4508 100644
> --- a/arch/sandbox/include/asm/eth-raw-os.h
> +++ b/arch/sandbox/include/asm/eth-raw-os.h
> @@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
>  };
>  
>  int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
> -			    struct eth_sandbox_raw_priv *priv);
> +			     struct eth_sandbox_raw_priv *priv);
> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
> +				 struct eth_sandbox_raw_priv *priv);
>  int sandbox_eth_raw_os_send(void *packet, int length,
>  			    struct eth_sandbox_raw_priv *priv);
>  int sandbox_eth_raw_os_recv(void *packet, int *length,
> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
> index f835a6a7f3..3bde8d84ce 100644
> --- a/drivers/net/sandbox-raw.c
> +++ b/drivers/net/sandbox-raw.c
> @@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
>  	struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
>  	struct eth_pdata *pdata = dev_get_platdata(dev);
>  	const char *interface;
> +	u32 index;
>  
>  	debug("eth_sandbox_raw: Start\n");
>  
> @@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
>  						priv);
>  	}
>  
> -	return -EINVAL;
> +	if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
> +		return -EINVAL;
> +
> +	printf("eth_sandbox_raw: Using interface index %d from DT ", index);
> +
> +	return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
>  }
>  
>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-26 23:31     ` Joe Hershberger
@ 2018-06-27  4:41       ` Simon Glass
  0 siblings, 0 replies; 22+ messages in thread
From: Simon Glass @ 2018-06-27  4:41 UTC (permalink / raw)
  To: u-boot

Hi Joe,

On 26 June 2018 at 16:31, Joe Hershberger <joe.hershberger@ni.com> wrote:
> On Tue, Jun 26, 2018 at 6:18 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Joe,
>>
>> On 26 June 2018 at 14:19, Joe Hershberger <joe.hershberger@ni.com> wrote:
>>> With systemd stable interface names, eth0 will almost never exist.
>>> Instead of using that name in the sandbox.dts, use index 2 (the first
>>> interface after "lo"). Enumerate the interfaces on the system to choose
>>> a valid interace name.
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>> ---
>>>
>>>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>>  arch/sandbox/dts/sandbox.dts          |  2 +-
>>>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>>  drivers/net/sandbox-raw.c             |  8 +++++++-
>>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>
>> This seems brittle too. A name seems better to me. I might be missing
>> something though. Can we ask the OS for a name, etc?
>
> This asks for the equivalent of "eth0", but asks the OS to tell us the
> actual name. I don't know that we can possibly choose the correct
> network interface to use across any machine configuration. Choosing
> the first one, whatever it's name is, seems like a pretty reliable way
> to get a good interface in most cases.

OK well let's see how it works out.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-27  1:15   ` Heinrich Schuchardt
@ 2018-06-27  4:58     ` Joe Hershberger
  2018-06-27  5:55       ` Heinrich Schuchardt
  0 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-27  4:58 UTC (permalink / raw)
  To: u-boot

Hi Heinrich,

On Tue, Jun 26, 2018 at 8:15 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 06/26/2018 11:19 PM, Joe Hershberger wrote:
>> With systemd stable interface names, eth0 will almost never exist.
>> Instead of using that name in the sandbox.dts, use index 2 (the first
>> interface after "lo"). Enumerate the interfaces on the system to choose
>> a valid interace name.
>>
>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>
> Hello Joe,
>
> thanks for addressing this issue.
>
> On my laptop the raw interfaces are currently called:
> 1 - lo - loopback
> 2 - enp0s25 - wired ethernet
> 3 - wlp12s0 - WLAN
>
> But this list is not static:
>
> It is allowable to define multiple loopback interfaces. Additional
> bridges, tap and tun interfaces may be defined. A computer may also have
> multiple wired and wireless interfaces. And of cause I may choose to
> plug in an additional ethernet adapter while my computer is running.
>
> The interface index does not tell which interface is actually connected
> to the network. My laptop uses wpl12s0 (index = 3) for sending out this
> mail. So, please, do not replace one invalid assumption (eth0) by
> another (index = 2).

While index=2 is not an ideal assumption, I don't think invalid is a
fair description. It is far better than eth0, which on modern systems
is very unlikely. The driver is probed based on DTS entries. This is
simply for manual testing, so if the common situation doesn't apply to
a person, they can easily modify the DTS to specify the name of their
interface, or they can specify a different index. I don't think the
fact that it doesn't ideally fit every possible situation warrants
changing how these are probed to somehow be dynamically created based
on the OS.

> I doubt there is a guarantee that index 1 is a loopback interface on
> POSIX systems. If you need a special treatment of loopback devices you
> could analyze the properties of each interface to detect which one is a
> loopback interface.

The only special treatment for local interfaces is that they don't
work with raw sockets. You can see that in the code. At the same time,
the local interface is pretty crippled for testing the network stack
anyway (no ICMP support) so if there is more than one (why would you?)
then that one not named "lo" will not work.

> There is no need for using the device tree for the specification of
> interfaces. Instead a raw ethernet device can be set up for each
> interface. Let the user choose at runtime which device he wants to use
> for testing.

You say there is no need, but that's the way it works in the sandbox.
There is no "OS" subsystem that is there to enumerate new interfaces
dynamically, the way something like PCI on a real board would.  I
don't think we would want it completely automatic anyway, because
things like the test.dts have only fake sandbox eth interfaces, not
any raw, since we want the unit test to run consistently regardless of
the arrangement of network interfaces on the test system it runs on.

Cheers,
-Joe

> Best regards
>
> Heinrich
>
>> ---
>>
>>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>  arch/sandbox/dts/sandbox.dts          |  2 +-
>>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>  drivers/net/sandbox-raw.c             |  8 +++++++-
>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
>> index 61f23ed210..11b5271f31 100644
>> --- a/arch/sandbox/cpu/eth-raw-os.c
>> +++ b/arch/sandbox/cpu/eth-raw-os.c
>> @@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>               return _raw_packet_start(ifname, ethmac, priv);
>>  }
>>
>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>> +                              struct eth_sandbox_raw_priv *priv)
>> +{
>> +     int ret;
>> +     struct if_nameindex *ni, *i;
>> +     const char *ifname = NULL;
>> +
>> +     ni = if_nameindex();
>> +     if (!ni)
>> +             return -EINVAL;
>> +
>> +     for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
>> +             if (ifindex == i->if_index) {
>> +                     ifname = i->if_name;
>> +                     printf("(%s)\n", ifname);
>> +                     break;
>> +             }
>> +     }
>> +     /* Don't bother supporting 'lo' by index. Just use the name in DTS */
>> +     if (ifname)
>> +             ret = _raw_packet_start(ifname, ethmac, priv);
>> +     else
>> +             ret = -EINVAL;
>> +
>> +     if_freenameindex(ni);
>> +     return ret;
>> +}
>> +
>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>                           struct eth_sandbox_raw_priv *priv)
>>  {
>> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
>> index 0ea2452742..e7f6c194d0 100644
>> --- a/arch/sandbox/dts/sandbox.dts
>> +++ b/arch/sandbox/dts/sandbox.dts
>> @@ -56,7 +56,7 @@
>>       eth at 80000000 {
>>               compatible = "sandbox,eth-raw";
>>               reg = <0x80000000 0x1000>;
>> -             host-raw-interface = "eth0";
>> +             host-raw-interface-idx = <2>;
>>       };
>>
>>       eth at 90000000 {
>> diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
>> index f986d3142d..70b29b4508 100644
>> --- a/arch/sandbox/include/asm/eth-raw-os.h
>> +++ b/arch/sandbox/include/asm/eth-raw-os.h
>> @@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
>>  };
>>
>>  int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>> -                         struct eth_sandbox_raw_priv *priv);
>> +                          struct eth_sandbox_raw_priv *priv);
>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>> +                              struct eth_sandbox_raw_priv *priv);
>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>                           struct eth_sandbox_raw_priv *priv);
>>  int sandbox_eth_raw_os_recv(void *packet, int *length,
>> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>> index f835a6a7f3..3bde8d84ce 100644
>> --- a/drivers/net/sandbox-raw.c
>> +++ b/drivers/net/sandbox-raw.c
>> @@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
>>       struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
>>       struct eth_pdata *pdata = dev_get_platdata(dev);
>>       const char *interface;
>> +     u32 index;
>>
>>       debug("eth_sandbox_raw: Start\n");
>>
>> @@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
>>                                               priv);
>>       }
>>
>> -     return -EINVAL;
>> +     if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
>> +             return -EINVAL;
>> +
>> +     printf("eth_sandbox_raw: Using interface index %d from DT ", index);
>> +
>> +     return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
>>  }
>>
>>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-27  4:58     ` Joe Hershberger
@ 2018-06-27  5:55       ` Heinrich Schuchardt
  2018-06-27  6:01         ` Joe Hershberger
  0 siblings, 1 reply; 22+ messages in thread
From: Heinrich Schuchardt @ 2018-06-27  5:55 UTC (permalink / raw)
  To: u-boot

On 06/27/2018 06:58 AM, Joe Hershberger wrote:
> Hi Heinrich,
> 
> On Tue, Jun 26, 2018 at 8:15 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 06/26/2018 11:19 PM, Joe Hershberger wrote:
>>> With systemd stable interface names, eth0 will almost never exist.
>>> Instead of using that name in the sandbox.dts, use index 2 (the first
>>> interface after "lo"). Enumerate the interfaces on the system to choose
>>> a valid interace name.
>>>
>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>
>> Hello Joe,
>>
>> thanks for addressing this issue.
>>
>> On my laptop the raw interfaces are currently called:
>> 1 - lo - loopback
>> 2 - enp0s25 - wired ethernet
>> 3 - wlp12s0 - WLAN
>>
>> But this list is not static:
>>
>> It is allowable to define multiple loopback interfaces. Additional
>> bridges, tap and tun interfaces may be defined. A computer may also have
>> multiple wired and wireless interfaces. And of cause I may choose to
>> plug in an additional ethernet adapter while my computer is running.
>>
>> The interface index does not tell which interface is actually connected
>> to the network. My laptop uses wpl12s0 (index = 3) for sending out this
>> mail. So, please, do not replace one invalid assumption (eth0) by
>> another (index = 2).
> 
> While index=2 is not an ideal assumption, I don't think invalid is a
> fair description. It is far better than eth0, which on modern systems

Using a laptop over WLAN is not special. So many users will be using an
interface with index=3.

So your assumption that all computers are created equal (using index=2)
is invalid.

> is very unlikely. The driver is probed based on DTS entries. This is
> simply for manual testing, so if the common situation doesn't apply to
> a person, they can easily modify the DTS to specify the name of their
> interface, or they can specify a different index. I don't think the
> fact that it doesn't ideally fit every possible situation warrants
> changing how these are probed to somehow be dynamically created based
> on the OS.
> 
>> I doubt there is a guarantee that index 1 is a loopback interface on
>> POSIX systems. If you need a special treatment of loopback devices you
>> could analyze the properties of each interface to detect which one is a
>> loopback interface.
> 
> The only special treatment for local interfaces is that they don't
> work with raw sockets. You can see that in the code. At the same time,
> the local interface is pretty crippled for testing the network stack
> anyway (no ICMP support) so if there is more than one (why would you?)
> then that one not named "lo" will not work.
> 
>> There is no need for using the device tree for the specification of
>> interfaces. Instead a raw ethernet device can be set up for each
>> interface. Let the user choose at runtime which device he wants to use
>> for testing.
> 
> You say there is no need, but that's the way it works in the sandbox.
> There is no "OS" subsystem that is there to enumerate new interfaces
> dynamically, the way something like PCI on a real board would.  I
> don't think we would want it completely automatic anyway, because
> things like the test.dts have only fake sandbox eth interfaces, not
> any raw, since we want the unit test to run consistently regardless of
> the arrangement of network interfaces on the test system it runs on.

NAK until the Sandbox works in the same way both on my laptop (WLAN,
index=3) and on my workstation (wired, index=2) without manually
patching source code.

Best regards

Heinrich

> 
> Cheers,
> -Joe
> 
>> Best regards
>>
>> Heinrich
>>
>>> ---
>>>
>>>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>>  arch/sandbox/dts/sandbox.dts          |  2 +-
>>>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>>  drivers/net/sandbox-raw.c             |  8 +++++++-
>>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
>>> index 61f23ed210..11b5271f31 100644
>>> --- a/arch/sandbox/cpu/eth-raw-os.c
>>> +++ b/arch/sandbox/cpu/eth-raw-os.c
>>> @@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>>               return _raw_packet_start(ifname, ethmac, priv);
>>>  }
>>>
>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>> +                              struct eth_sandbox_raw_priv *priv)
>>> +{
>>> +     int ret;
>>> +     struct if_nameindex *ni, *i;
>>> +     const char *ifname = NULL;
>>> +
>>> +     ni = if_nameindex();
>>> +     if (!ni)
>>> +             return -EINVAL;
>>> +
>>> +     for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
>>> +             if (ifindex == i->if_index) {
>>> +                     ifname = i->if_name;
>>> +                     printf("(%s)\n", ifname);
>>> +                     break;
>>> +             }
>>> +     }
>>> +     /* Don't bother supporting 'lo' by index. Just use the name in DTS */
>>> +     if (ifname)
>>> +             ret = _raw_packet_start(ifname, ethmac, priv);
>>> +     else
>>> +             ret = -EINVAL;
>>> +
>>> +     if_freenameindex(ni);
>>> +     return ret;
>>> +}
>>> +
>>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>>                           struct eth_sandbox_raw_priv *priv)
>>>  {
>>> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
>>> index 0ea2452742..e7f6c194d0 100644
>>> --- a/arch/sandbox/dts/sandbox.dts
>>> +++ b/arch/sandbox/dts/sandbox.dts
>>> @@ -56,7 +56,7 @@
>>>       eth at 80000000 {
>>>               compatible = "sandbox,eth-raw";
>>>               reg = <0x80000000 0x1000>;
>>> -             host-raw-interface = "eth0";
>>> +             host-raw-interface-idx = <2>;
>>>       };
>>>
>>>       eth at 90000000 {
>>> diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
>>> index f986d3142d..70b29b4508 100644
>>> --- a/arch/sandbox/include/asm/eth-raw-os.h
>>> +++ b/arch/sandbox/include/asm/eth-raw-os.h
>>> @@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
>>>  };
>>>
>>>  int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>> -                         struct eth_sandbox_raw_priv *priv);
>>> +                          struct eth_sandbox_raw_priv *priv);
>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>> +                              struct eth_sandbox_raw_priv *priv);
>>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>>                           struct eth_sandbox_raw_priv *priv);
>>>  int sandbox_eth_raw_os_recv(void *packet, int *length,
>>> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>>> index f835a6a7f3..3bde8d84ce 100644
>>> --- a/drivers/net/sandbox-raw.c
>>> +++ b/drivers/net/sandbox-raw.c
>>> @@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>       struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
>>>       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>       const char *interface;
>>> +     u32 index;
>>>
>>>       debug("eth_sandbox_raw: Start\n");
>>>
>>> @@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>                                               priv);
>>>       }
>>>
>>> -     return -EINVAL;
>>> +     if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
>>> +             return -EINVAL;
>>> +
>>> +     printf("eth_sandbox_raw: Using interface index %d from DT ", index);
>>> +
>>> +     return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
>>>  }
>>>
>>>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-27  5:55       ` Heinrich Schuchardt
@ 2018-06-27  6:01         ` Joe Hershberger
  2018-06-27  9:47           ` Heinrich Schuchardt
  0 siblings, 1 reply; 22+ messages in thread
From: Joe Hershberger @ 2018-06-27  6:01 UTC (permalink / raw)
  To: u-boot

On Wed, Jun 27, 2018 at 12:55 AM, Heinrich Schuchardt
<xypron.glpk@gmx.de> wrote:
> On 06/27/2018 06:58 AM, Joe Hershberger wrote:
>> Hi Heinrich,
>>
>> On Tue, Jun 26, 2018 at 8:15 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 06/26/2018 11:19 PM, Joe Hershberger wrote:
>>>> With systemd stable interface names, eth0 will almost never exist.
>>>> Instead of using that name in the sandbox.dts, use index 2 (the first
>>>> interface after "lo"). Enumerate the interfaces on the system to choose
>>>> a valid interace name.
>>>>
>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>>
>>> Hello Joe,
>>>
>>> thanks for addressing this issue.
>>>
>>> On my laptop the raw interfaces are currently called:
>>> 1 - lo - loopback
>>> 2 - enp0s25 - wired ethernet
>>> 3 - wlp12s0 - WLAN
>>>
>>> But this list is not static:
>>>
>>> It is allowable to define multiple loopback interfaces. Additional
>>> bridges, tap and tun interfaces may be defined. A computer may also have
>>> multiple wired and wireless interfaces. And of cause I may choose to
>>> plug in an additional ethernet adapter while my computer is running.
>>>
>>> The interface index does not tell which interface is actually connected
>>> to the network. My laptop uses wpl12s0 (index = 3) for sending out this
>>> mail. So, please, do not replace one invalid assumption (eth0) by
>>> another (index = 2).
>>
>> While index=2 is not an ideal assumption, I don't think invalid is a
>> fair description. It is far better than eth0, which on modern systems
>
> Using a laptop over WLAN is not special. So many users will be using an
> interface with index=3.
>
> So your assumption that all computers are created equal (using index=2)
> is invalid.
>
>> is very unlikely. The driver is probed based on DTS entries. This is
>> simply for manual testing, so if the common situation doesn't apply to
>> a person, they can easily modify the DTS to specify the name of their
>> interface, or they can specify a different index. I don't think the
>> fact that it doesn't ideally fit every possible situation warrants
>> changing how these are probed to somehow be dynamically created based
>> on the OS.
>>
>>> I doubt there is a guarantee that index 1 is a loopback interface on
>>> POSIX systems. If you need a special treatment of loopback devices you
>>> could analyze the properties of each interface to detect which one is a
>>> loopback interface.
>>
>> The only special treatment for local interfaces is that they don't
>> work with raw sockets. You can see that in the code. At the same time,
>> the local interface is pretty crippled for testing the network stack
>> anyway (no ICMP support) so if there is more than one (why would you?)
>> then that one not named "lo" will not work.
>>
>>> There is no need for using the device tree for the specification of
>>> interfaces. Instead a raw ethernet device can be set up for each
>>> interface. Let the user choose at runtime which device he wants to use
>>> for testing.
>>
>> You say there is no need, but that's the way it works in the sandbox.
>> There is no "OS" subsystem that is there to enumerate new interfaces
>> dynamically, the way something like PCI on a real board would.  I
>> don't think we would want it completely automatic anyway, because
>> things like the test.dts have only fake sandbox eth interfaces, not
>> any raw, since we want the unit test to run consistently regardless of
>> the arrangement of network interfaces on the test system it runs on.
>
> NAK until the Sandbox works in the same way both on my laptop (WLAN,
> index=3) and on my workstation (wired, index=2) without manually
> patching source code.

So it sounds like you'd be fine if there were simply 2 entries in the
DTS... one that lists index=2 and another that lists index=3.

> Best regards
>
> Heinrich
>
>>
>> Cheers,
>> -Joe
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> ---
>>>>
>>>>  arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>>>  arch/sandbox/dts/sandbox.dts          |  2 +-
>>>>  arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>>>  drivers/net/sandbox-raw.c             |  8 +++++++-
>>>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
>>>> index 61f23ed210..11b5271f31 100644
>>>> --- a/arch/sandbox/cpu/eth-raw-os.c
>>>> +++ b/arch/sandbox/cpu/eth-raw-os.c
>>>> @@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>>>               return _raw_packet_start(ifname, ethmac, priv);
>>>>  }
>>>>
>>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>>> +                              struct eth_sandbox_raw_priv *priv)
>>>> +{
>>>> +     int ret;
>>>> +     struct if_nameindex *ni, *i;
>>>> +     const char *ifname = NULL;
>>>> +
>>>> +     ni = if_nameindex();
>>>> +     if (!ni)
>>>> +             return -EINVAL;
>>>> +
>>>> +     for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
>>>> +             if (ifindex == i->if_index) {
>>>> +                     ifname = i->if_name;
>>>> +                     printf("(%s)\n", ifname);
>>>> +                     break;
>>>> +             }
>>>> +     }
>>>> +     /* Don't bother supporting 'lo' by index. Just use the name in DTS */
>>>> +     if (ifname)
>>>> +             ret = _raw_packet_start(ifname, ethmac, priv);
>>>> +     else
>>>> +             ret = -EINVAL;
>>>> +
>>>> +     if_freenameindex(ni);
>>>> +     return ret;
>>>> +}
>>>> +
>>>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>>>                           struct eth_sandbox_raw_priv *priv)
>>>>  {
>>>> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
>>>> index 0ea2452742..e7f6c194d0 100644
>>>> --- a/arch/sandbox/dts/sandbox.dts
>>>> +++ b/arch/sandbox/dts/sandbox.dts
>>>> @@ -56,7 +56,7 @@
>>>>       eth at 80000000 {
>>>>               compatible = "sandbox,eth-raw";
>>>>               reg = <0x80000000 0x1000>;
>>>> -             host-raw-interface = "eth0";
>>>> +             host-raw-interface-idx = <2>;
>>>>       };
>>>>
>>>>       eth at 90000000 {
>>>> diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
>>>> index f986d3142d..70b29b4508 100644
>>>> --- a/arch/sandbox/include/asm/eth-raw-os.h
>>>> +++ b/arch/sandbox/include/asm/eth-raw-os.h
>>>> @@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
>>>>  };
>>>>
>>>>  int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>>> -                         struct eth_sandbox_raw_priv *priv);
>>>> +                          struct eth_sandbox_raw_priv *priv);
>>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>>> +                              struct eth_sandbox_raw_priv *priv);
>>>>  int sandbox_eth_raw_os_send(void *packet, int length,
>>>>                           struct eth_sandbox_raw_priv *priv);
>>>>  int sandbox_eth_raw_os_recv(void *packet, int *length,
>>>> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>>>> index f835a6a7f3..3bde8d84ce 100644
>>>> --- a/drivers/net/sandbox-raw.c
>>>> +++ b/drivers/net/sandbox-raw.c
>>>> @@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>>       struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
>>>>       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>       const char *interface;
>>>> +     u32 index;
>>>>
>>>>       debug("eth_sandbox_raw: Start\n");
>>>>
>>>> @@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>>                                               priv);
>>>>       }
>>>>
>>>> -     return -EINVAL;
>>>> +     if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>> +     printf("eth_sandbox_raw: Using interface index %d from DT ", index);
>>>> +
>>>> +     return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
>>>>  }
>>>>
>>>>  static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
>>>>
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> https://lists.denx.de/listinfo/u-boot
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
  2018-06-27  6:01         ` Joe Hershberger
@ 2018-06-27  9:47           ` Heinrich Schuchardt
  0 siblings, 0 replies; 22+ messages in thread
From: Heinrich Schuchardt @ 2018-06-27  9:47 UTC (permalink / raw)
  To: u-boot


On 06/27/2018 08:01 AM, Joe Hershberger wrote:
> On Wed, Jun 27, 2018 at 12:55 AM, Heinrich Schuchardt
> <xypron.glpk@gmx.de> wrote:
>> On 06/27/2018 06:58 AM, Joe Hershberger wrote:
>>> Hi Heinrich,
>>>
>>> On Tue, Jun 26, 2018 at 8:15 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 06/26/2018 11:19 PM, Joe Hershberger wrote:
>>>>> With systemd stable interface names, eth0 will almost never exist.
>>>>> Instead of using that name in the sandbox.dts, use index 2 (the first
>>>>> interface after "lo"). Enumerate the interfaces on the system to choose
>>>>> a valid interace name.
>>>>>
>>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>>>>
>>>> Hello Joe,
>>>>
>>>> thanks for addressing this issue.
>>>>
>>>> On my laptop the raw interfaces are currently called:
>>>> 1 - lo - loopback
>>>> 2 - enp0s25 - wired ethernet
>>>> 3 - wlp12s0 - WLAN
>>>>
>>>> But this list is not static:
>>>>
>>>> It is allowable to define multiple loopback interfaces. Additional
>>>> bridges, tap and tun interfaces may be defined. A computer may also have
>>>> multiple wired and wireless interfaces. And of cause I may choose to
>>>> plug in an additional ethernet adapter while my computer is running.
>>>>
>>>> The interface index does not tell which interface is actually connected
>>>> to the network. My laptop uses wpl12s0 (index = 3) for sending out this
>>>> mail. So, please, do not replace one invalid assumption (eth0) by
>>>> another (index = 2).
>>>
>>> While index=2 is not an ideal assumption, I don't think invalid is a
>>> fair description. It is far better than eth0, which on modern systems
>>
>> Using a laptop over WLAN is not special. So many users will be using an
>> interface with index=3.
>>
>> So your assumption that all computers are created equal (using index=2)
>> is invalid.
>>
>>> is very unlikely. The driver is probed based on DTS entries. This is
>>> simply for manual testing, so if the common situation doesn't apply to
>>> a person, they can easily modify the DTS to specify the name of their
>>> interface, or they can specify a different index. I don't think the
>>> fact that it doesn't ideally fit every possible situation warrants
>>> changing how these are probed to somehow be dynamically created based
>>> on the OS.
>>>
>>>> I doubt there is a guarantee that index 1 is a loopback interface on
>>>> POSIX systems. If you need a special treatment of loopback devices you
>>>> could analyze the properties of each interface to detect which one is a
>>>> loopback interface.
>>>
>>> The only special treatment for local interfaces is that they don't
>>> work with raw sockets. You can see that in the code. At the same time,
>>> the local interface is pretty crippled for testing the network stack
>>> anyway (no ICMP support) so if there is more than one (why would you?)
>>> then that one not named "lo" will not work.
>>>
>>>> There is no need for using the device tree for the specification of
>>>> interfaces. Instead a raw ethernet device can be set up for each
>>>> interface. Let the user choose at runtime which device he wants to use
>>>> for testing.
>>>
>>> You say there is no need, but that's the way it works in the sandbox.
>>> There is no "OS" subsystem that is there to enumerate new interfaces
>>> dynamically, the way something like PCI on a real board would.  I
>>> don't think we would want it completely automatic anyway, because
>>> things like the test.dts have only fake sandbox eth interfaces, not
>>> any raw, since we want the unit test to run consistently regardless of
>>> the arrangement of network interfaces on the test system it runs on.
>>
>> NAK until the Sandbox works in the same way both on my laptop (WLAN,
>> index=3) and on my workstation (wired, index=2) without manually
>> patching source code.
> 
> So it sounds like you'd be fine if there were simply 2 entries in the
> DTS... one that lists index=2 and another that lists index=3.

No, you didn't get the point. First there is no guarantee that the 
loopback is index 1. And there is no guarantee that index 2 or 3 
represent the connection to the internet.

Would you accept a patch that will only work if the power connector is 
for 110V/60 Hz and volume is measured in imperial gallons?

Best regards

Heinrich

> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Cheers,
>>> -Joe
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> ---
>>>>>
>>>>>   arch/sandbox/cpu/eth-raw-os.c         | 28 ++++++++++++++++++++++++++++
>>>>>   arch/sandbox/dts/sandbox.dts          |  2 +-
>>>>>   arch/sandbox/include/asm/eth-raw-os.h |  4 +++-
>>>>>   drivers/net/sandbox-raw.c             |  8 +++++++-
>>>>>   4 files changed, 39 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c
>>>>> index 61f23ed210..11b5271f31 100644
>>>>> --- a/arch/sandbox/cpu/eth-raw-os.c
>>>>> +++ b/arch/sandbox/cpu/eth-raw-os.c
>>>>> @@ -150,6 +150,34 @@ int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>>>>                return _raw_packet_start(ifname, ethmac, priv);
>>>>>   }
>>>>>
>>>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>>>> +                              struct eth_sandbox_raw_priv *priv)
>>>>> +{
>>>>> +     int ret;
>>>>> +     struct if_nameindex *ni, *i;
>>>>> +     const char *ifname = NULL;
>>>>> +
>>>>> +     ni = if_nameindex();
>>>>> +     if (!ni)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     for (i = ni; !(i->if_index == 0 && !i->if_name); i++) {
>>>>> +             if (ifindex == i->if_index) {
>>>>> +                     ifname = i->if_name;
>>>>> +                     printf("(%s)\n", ifname);
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +     /* Don't bother supporting 'lo' by index. Just use the name in DTS */
>>>>> +     if (ifname)
>>>>> +             ret = _raw_packet_start(ifname, ethmac, priv);
>>>>> +     else
>>>>> +             ret = -EINVAL;
>>>>> +
>>>>> +     if_freenameindex(ni);
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>>   int sandbox_eth_raw_os_send(void *packet, int length,
>>>>>                            struct eth_sandbox_raw_priv *priv)
>>>>>   {
>>>>> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
>>>>> index 0ea2452742..e7f6c194d0 100644
>>>>> --- a/arch/sandbox/dts/sandbox.dts
>>>>> +++ b/arch/sandbox/dts/sandbox.dts
>>>>> @@ -56,7 +56,7 @@
>>>>>        eth at 80000000 {
>>>>>                compatible = "sandbox,eth-raw";
>>>>>                reg = <0x80000000 0x1000>;
>>>>> -             host-raw-interface = "eth0";
>>>>> +             host-raw-interface-idx = <2>;
>>>>>        };
>>>>>
>>>>>        eth at 90000000 {
>>>>> diff --git a/arch/sandbox/include/asm/eth-raw-os.h b/arch/sandbox/include/asm/eth-raw-os.h
>>>>> index f986d3142d..70b29b4508 100644
>>>>> --- a/arch/sandbox/include/asm/eth-raw-os.h
>>>>> +++ b/arch/sandbox/include/asm/eth-raw-os.h
>>>>> @@ -29,7 +29,9 @@ struct eth_sandbox_raw_priv {
>>>>>   };
>>>>>
>>>>>   int sandbox_eth_raw_os_start(const char *ifname, unsigned char *ethmac,
>>>>> -                         struct eth_sandbox_raw_priv *priv);
>>>>> +                          struct eth_sandbox_raw_priv *priv);
>>>>> +int sandbox_eth_raw_os_start_idx(unsigned int ifindex, unsigned char *ethmac,
>>>>> +                              struct eth_sandbox_raw_priv *priv);
>>>>>   int sandbox_eth_raw_os_send(void *packet, int length,
>>>>>                            struct eth_sandbox_raw_priv *priv);
>>>>>   int sandbox_eth_raw_os_recv(void *packet, int *length,
>>>>> diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>>>>> index f835a6a7f3..3bde8d84ce 100644
>>>>> --- a/drivers/net/sandbox-raw.c
>>>>> +++ b/drivers/net/sandbox-raw.c
>>>>> @@ -22,6 +22,7 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>>>        struct eth_sandbox_raw_priv *priv = dev_get_priv(dev);
>>>>>        struct eth_pdata *pdata = dev_get_platdata(dev);
>>>>>        const char *interface;
>>>>> +     u32 index;
>>>>>
>>>>>        debug("eth_sandbox_raw: Start\n");
>>>>>
>>>>> @@ -37,7 +38,12 @@ static int sb_eth_raw_start(struct udevice *dev)
>>>>>                                                priv);
>>>>>        }
>>>>>
>>>>> -     return -EINVAL;
>>>>> +     if (dev_read_u32(dev, "host-raw-interface-idx", &index) < 0)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     printf("eth_sandbox_raw: Using interface index %d from DT ", index);
>>>>> +
>>>>> +     return sandbox_eth_raw_os_start_idx(index, pdata->enetaddr, priv);
>>>>>   }
>>>>>
>>>>>   static int sb_eth_raw_send(struct udevice *dev, void *packet, int length)
>>>>>
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> https://lists.denx.de/listinfo/u-boot
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2018-06-27  9:47 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-26 21:19 [U-Boot] [PATCH 0/5] sandbox: net: Fix sandbox eth drivers Joe Hershberger
2018-06-26 21:19 ` [U-Boot] [PATCH 1/5] sandbox: eth-raw: Correct valid socket test in send/recv Joe Hershberger
2018-06-26 23:18   ` Simon Glass
2018-06-26 21:19 ` [U-Boot] [PATCH 2/5] net: Correct comment in Kconfig Joe Hershberger
2018-06-26 23:18   ` Simon Glass
2018-06-26 21:19 ` [U-Boot] [PATCH 3/5] net: sandbox: Convert sandbox mock eth driver to livetree Joe Hershberger
2018-06-26 23:18   ` Simon Glass
2018-06-26 23:25     ` Joe Hershberger
2018-06-26 23:28       ` Simon Glass
2018-06-27  0:29         ` Joe Hershberger
2018-06-26 21:19 ` [U-Boot] [PATCH 4/5] net: sandbox-raw: Convert raw " Joe Hershberger
2018-06-26 23:18   ` Simon Glass
2018-06-26 23:28     ` Joe Hershberger
2018-06-26 21:19 ` [U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index Joe Hershberger
2018-06-26 23:18   ` Simon Glass
2018-06-26 23:31     ` Joe Hershberger
2018-06-27  4:41       ` Simon Glass
2018-06-27  1:15   ` Heinrich Schuchardt
2018-06-27  4:58     ` Joe Hershberger
2018-06-27  5:55       ` Heinrich Schuchardt
2018-06-27  6:01         ` Joe Hershberger
2018-06-27  9:47           ` Heinrich Schuchardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.