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

> 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" ?

> --- 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.

> --- 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

> +int os_tap_open(char *dev)

const char *dev

> +     if (*dev)
> +             strncpy(ifr.ifr_name, dev, IFNAMSIZ);

let's just make it required

> --- /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

> +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]

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

os_read returns ssize_t, not int

> +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 ?

> +int tap_initialize(bd_t *bd)
> ...
> +     struct eth_device *edev;

this should be named "dev" to stay consistent

> +     edev = (struct eth_device *)malloc(sizeof(struct eth_device));
> +     tap = (struct tap_priv *)malloc(sizeof(struct tap_priv));

no need for the casts

> +     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

> +     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");

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

this should return -1

> +     sprintf(edev->name, "TAP");

use the same name as tap->dev.  in fact, i'd just chuck tap->dev and only use 
dev->name.
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to