[U-Boot] [PATCH RFC] sandbox: Add tap based networking

[Matthias Weisser]

Am 29.11.2011 16:24, schrieb Mike Frysinger:
> On Tuesday 29 November 2011 09:09:03 Matthias Weisser wrote:
>> This patch adds support for networking to sandbox architecture using tap. A
>> tap device "tap0" has to be created e.g. using openvpn
>> ....
> 
> this info should be in the changelog as it's useful

Will do

>> As sandbox is build using the native compiler, which is in my case x86_64,
>> ulong is 64 bit in size. This caused non-working IP stuff as IPaddr_t is
>> typedefed to ulong. I changed this typedef to u32 which helped but is
>> quite invasive to all network related stuff. This is the main reason why
>> this patched is marked as RFC.
> 
> could you elaborate on "non-working IP stuff" ?

Nothing worked. e.g. ARP wasn't working due to the memcpy in function
NetReadIP in net.h which uses sizeof(IPaddr_t) to copy the IP address
out of the network packet. sizeof(IPaddr_t) yields 8 on x86_64. After
changing the typedef to a type having 32 bits everything worked. So this
is one of the zillion problems fixed which we will see if u-boot is
ported to real 64 bit hardware :-)

>> --- a/arch/sandbox/cpu/cpu.c
>> +++ b/arch/sandbox/cpu/cpu.c
>>
>> +#if defined(CONFIG_DRIVER_TAP)
> 
> i think Wolfgang NAK-ed the idea of using "DRIVER" in the config name.  so 
> let's use something like CONFIG_NET_TAP.

Done.

>> --- a/arch/sandbox/cpu/os.c
>> +++ b/arch/sandbox/cpu/os.c
>>
>> +#include <sys/ioctl.h>
>> +#include <sys/socket.h>
>> +#include <linux/if.h>
>> +#include <linux/if_tun.h>
> 
> these should be behind the new CONFIG name, as should the func itself

Done

>> +int os_tap_open(char *dev)
> 
> const char *dev
> 
>> +	if (*dev)
>> +		strncpy(ifr.ifr_name, dev, IFNAMSIZ);
> 
> let's just make it required

Done

>> --- /dev/null
>> +++ b/drivers/net/tap.c
>>
>> +static int tap_send(struct eth_device *dev, void *packet, int length)
>> ...
>> +	os_write(priv->fd, (void *)packet, length);
> 
> packet is already void*, so no need to cast

packet of tap_send need to be volatile. So added volatile there and let
the cast there.

>> +static int tap_recv(struct eth_device *dev)
>> +{
>> +	struct tap_priv *priv = dev->priv;
>> +	uchar buf[PKTSIZE];
> 
> there are already common network buffers for you to use: NetRxPackets[0]

Will use them

>> +	int length;
>> +
>> +	length = os_read(priv->fd, buf, PKTSIZE);
> 
> os_read returns ssize_t, not int

NetReceive needs an int later on. I added an explicit cast.

>> +static int tap_set_hwaddr(struct eth_device *dev)
>> +{
>> +	/* Nothing to be done here */
>> +	return 0;
>> +}
> 
> isn't there an ioctl that lets you control this ?

Sure. But if I read the the docs correct it is an privileged operation
and I don't think we wan't to run u-boot as super user all the time. How
is the situation handled on real hardware when the MAC is programmed to
an EEPROM on the NIC. Can the MAC be read from the NIC and set to
u-boot? This would be the best solution as linux should take care about
MAC address assignment.

>> +int tap_initialize(bd_t *bd)
>> ...
>> +	struct eth_device *edev;
> 
> this should be named "dev" to stay consistent

Done.

>> +	edev = (struct eth_device *)malloc(sizeof(struct eth_device));
>> +	tap = (struct tap_priv *)malloc(sizeof(struct tap_priv));
> 
> no need for the casts

Right

>> +	if (!edev) {
>> +		puts("tap: not enough malloc memory for eth_device\n");
>> +		res = -ENOMEM;
>> +		goto err1;
>> +	}
>> +
>> +	if (!tap) {
>> +		puts("tap: not enough malloc memory for tap_priv\n");
>> +		res = -ENOMEM;
>> +		goto err2;
>> +	}
> 
> drop the puts(), and return 0, not -ENOMEM

I don't understand why, but done. We are in a PC environment where the
puts shouldn't hurt in code size and may help in debugging.

>> +	strncpy(tap->dev, "tap0", sizeof(tap->dev));
> 
> no reason we couldn't support more than one tap, is there ?  make the device 
> name an argument to tap_initialize(), and then the board caller can do:
> 	tap_initialize("tap0");
> 	tap_initialize("tap1");
> 	tap_initialize("fooiefoofoo");

Done.

>> +	tap->fd = os_tap_open(tap->dev);
>> +	if (tap->fd < 0) {
>> +		res = -EIO;
>> +		goto err3;
>> +	}
> 
> this should return -1

Done.

>> +	sprintf(edev->name, "TAP");
> 
> use the same name as tap->dev.  in fact, i'd just chuck tap->dev and only use 
> dev->name.

Done

Thanks for the review. Will wait for some additional comments and send a
new version then.

-- 
Matthias