[U-Boot] [PATCH 5/5] net: sandbox-raw: Allow interface to be specified by index
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Jun 27 09:47:09 UTC 2018
On 06/27/2018 08:01 AM, Joe Hershberger wrote:
> 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.
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
>
More information about the U-Boot
mailing list