[U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index

Joe Hershberger joe.hershberger at ni.com
Wed Jun 27 06:01:31 UTC 2018


On Wed, Jun 27, 2018 at 12:55 AM, Heinrich Schuchardt
<xypron.glpk at 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 at 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 at 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


More information about the U-Boot mailing list