[U-Boot] [RFC PATCH v3 14/14] dm: eth: Add a bridge to a real network for sandbox

[Simon Glass <sjg@chromium.org>]

Hi Joe,

On 16 February 2015 at 22:16, Joe Hershberger <joe.hershberger@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Feb 15, 2015 at 9:50 AM, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 10 February 2015 at 18:30, Joe Hershberger <joe.hershberger@ni.com>
>> wrote:
>> > Implement a bridge between u-boot's network stack and Linux's raw packet
>> > API allowing the sandbox to send and receive packets using the host
>> > machine's network interface.
>> >
>> > This raw Ethernet API requires elevated privileges.  You can either run
>> > as root, or you can add the capability needed like so:
>> >
>> > sudo /sbin/setcap "CAP_NET_RAW+ep" u-boot
>>
>> Can you add a note about thsi in README.sandbox? This seems like a
>> major new feature. It was talked about a few years ago when sandbox
>> was first created.
>
> OK.  Can you maybe point me to that conversation so I can understand what
> was anticipated potentially covering more of what was expected.

All I could find was this:

http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/108687

>
>> >
>> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
>> >
>> > ---
>> >
>> > Changes in v3:
>> > -Made the os raw packet support for sandbox eth build and work.
>> >
>> > Changes in v2:
>> > -Added the raw packet proof-of-concept patch.
>> >
>> >  arch/sandbox/dts/sandbox.dts              |   6 ++
>> >  arch/sandbox/include/asm/sandbox-raw-os.h |  16 ++++
>> >  drivers/net/Makefile                      |  11 +++
>> >  drivers/net/sandbox-raw-os.c              | 105
>> > ++++++++++++++++++++++++
>> >  drivers/net/sandbox-raw.c                 | 128
>> > ++++++++++++++++++++++++++++++
>> >  include/configs/sandbox.h                 |   1 +
>> >  6 files changed, 267 insertions(+)
>> >  create mode 100644 arch/sandbox/include/asm/sandbox-raw-os.h
>> >  create mode 100644 drivers/net/sandbox-raw-os.c
>> >  create mode 100644 drivers/net/sandbox-raw.c
>> >
>> > diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
>> > index ba635e8..13bd6c2 100644
>> > --- a/arch/sandbox/dts/sandbox.dts
>> > +++ b/arch/sandbox/dts/sandbox.dts
>> > @@ -188,4 +188,10 @@
>> >                 reg = <0x10002000 0x1000>;
>> >                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x00>;
>> >         };
>> > +
>> > +       eth at 80000000 {
>> > +               compatible = "sandbox,eth,raw";
>>
>> We normally have "vendor,device", so maybe "sandbox,raw-eth"
>
> OK
>
>> > +               reg = <0x80000000 0x1000>;
>> > +               host-raw-interface = "eth0";
>> > +       };
>> >  };
>> > diff --git a/arch/sandbox/include/asm/sandbox-raw-os.h
>> > b/arch/sandbox/include/asm/sandbox-raw-os.h
>> > new file mode 100644
>> > index 0000000..4e5d418
>> > --- /dev/null
>> > +++ b/arch/sandbox/include/asm/sandbox-raw-os.h
>> > @@ -0,0 +1,16 @@
>> > +/*
>> > + * Copyright (c) 2015 National Instruments
>> > + *
>> > + * (C) Copyright 2015
>> > + * Joe Hershberger <joe.hershberger@ni.com>
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +
>> > +#pragma once
>>
>> We use #ifdef for this in U-Boot at present. I'm not sure why -
>> perhaps compatibility?
>
> OK
>
>> > +
>> > +int sandbox_raw_init(int *sd, void **devp, const char *ifname,
>> > +                    unsigned char *ethmac);
>> > +int sandbox_raw_send(void *packet, int length, int sd, void *device);
>> > +int sandbox_raw_recv(void *packet, int *length, int sd, void *device);
>> > +void sandbox_raw_halt(int *sd, void **devp);
>>
>> Function comments, also what is 'device'?
>
> Comments about what?  These are just the functions that implement the
> Ethernet driver ops.  Is it not sufficient to document the driver ops
> themselves (as you requested in the other patch)?

Well they seem to have different parameters:

- what is 'device'?
- what is'sd''?

I think these warrant comments.

>
>> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> > index 15dc431..39975b3 100644
>> > --- a/drivers/net/Makefile
>> > +++ b/drivers/net/Makefile
>> > @@ -51,6 +51,8 @@ obj-$(CONFIG_PCNET) += pcnet.o
>> >  obj-$(CONFIG_RTL8139) += rtl8139.o
>> >  obj-$(CONFIG_RTL8169) += rtl8169.o
>> >  obj-$(CONFIG_ETH_SANDBOX) += sandbox.o
>> > +obj-$(CONFIG_ETH_SANDBOX_RAW) += sandbox-raw.o
>> > +obj-$(CONFIG_ETH_SANDBOX_RAW) += sandbox-raw-os.o
>> >  obj-$(CONFIG_SH_ETHER) += sh_eth.o
>> >  obj-$(CONFIG_SMC91111) += smc91111.o
>> >  obj-$(CONFIG_SMC911X) += smc911x.o
>> > @@ -68,3 +70,12 @@ obj-$(CONFIG_XILINX_LL_TEMAC) += xilinx_ll_temac.o
>> > xilinx_ll_temac_mdio.o \
>> >  obj-$(CONFIG_ZYNQ_GEM) += zynq_gem.o
>> >  obj-$(CONFIG_FSL_MC_ENET) += fsl_mc/
>> >  obj-$(CONFIG_VSC9953) += vsc9953.o
>> > +
>> > +# sandbox-raw-os.c is built in the system env, so needs standard
>> > includes
>> > +# CFLAGS_REMOVE_sandbox-raw-os.o cannot be used to drop header include
>> > path
>> > +quiet_cmd_cc_sandbox-raw-os.o = CC $(quiet_modtag)  $@
>> > +cmd_cc_sandbox-raw-os.o = $(CC) $(filter-out -nostdinc, \
>> > +       $(patsubst -I%,-idirafter%,$(c_flags))) -c -o $@ lt;
>> > +
>> > +$(obj)/sandbox-raw-os.o: $(src)/sandbox-raw-os.c FORCE
>> > +       $(call if_changed_dep,cc_sandbox-raw-os.o)
>>
>> Can we please move this to the same directory as os.c, so that all the
>> OS-specific stuff is in one place.
>
> OK
>
>
>> > diff --git a/drivers/net/sandbox-raw-os.c b/drivers/net/sandbox-raw-os.c
>> > new file mode 100644
>> > index 0000000..43fae60
>> > --- /dev/null
>> > +++ b/drivers/net/sandbox-raw-os.c
>> > @@ -0,0 +1,105 @@
>> > +/*
>> > + * Copyright (c) 2015 National Instruments
>> > + *
>> > + * (C) Copyright 2015
>> > + * Joe Hershberger <joe.hershberger@ni.com>
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +
>> > +#include <errno.h>
>> > +#include <net/if.h>
>> > +#include <netinet/in.h>
>> > +#include <stdio.h>
>> > +#include <stdlib.h>
>> > +#include <string.h>
>> > +#include <sys/types.h>
>> > +#include <sys/ioctl.h>
>> > +#include <sys/socket.h>
>> > +#include <unistd.h>
>> > +
>> > +#include <linux/if_ether.h>
>> > +#include <linux/if_packet.h>
>> > +
>> > +int sandbox_raw_init(int *sd, void **devp, const char *ifname,
>> > +                    unsigned char *ethmac)
>> > +{
>> > +       int tempsd = 0;
>> > +       struct ifreq ifr;
>> > +
>> > +       strcpy(ifr.ifr_name, ifname);
>> > +       ifr.ifr_addr.sa_family = AF_INET;
>> > +       memset(ethmac, 0, 6);
>> > +       tempsd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW);
>> > +       if (tempsd < 0) {
>> > +               printf("Failed to open socket: %d %s\n", errno,
>> > +                      strerror(errno));
>> > +               return 1;
>>
>> How about:
>>
>>    return -errno
>>
>> here? That is what U-Boot tends to use now.
>
> OK
>
>
>> > +       }
>> > +       if (ioctl(tempsd, SIOCGIFHWADDR, &ifr) < 0) {
>> > +               printf("Failed to call ioctl: %s\n", strerror(errno));
>> > +               close(tempsd);
>> > +               return 1;
>> > +       }
>> > +       /*
>> > +        * This only works if the MAC address is overridden with the
>> > actual MAC
>> > +        * address of the interface being used.
>> > +        */
>> > +       memcpy(ethmac, ifr.ifr_hwaddr.sa_data, 6 * sizeof(uint8_t));
>> > +       close(tempsd);
>> > +
>> > +       *devp = malloc(sizeof(struct sockaddr_ll));
>> > +       struct sockaddr_ll *device = *devp;
>> > +       memset(device, 0, sizeof(struct sockaddr_ll));
>> > +       device->sll_ifindex = if_nametoindex(ifname);
>> > +       device->sll_family = AF_PACKET;
>> > +       memcpy(device->sll_addr, ethmac, 6);
>> > +       device->sll_halen = htons(6);
>> > +
>> > +       *sd = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
>> > +       setsockopt(*sd, SOL_SOCKET, SO_BINDTODEVICE, ifname,
>> > +                  strlen(ifname) + 1);
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +int sandbox_raw_send(void *packet, int length, int sd, void *device)
>> > +{
>> > +       int retval;
>> > +
>> > +       if (!sd || !device)
>> > +               return -EINVAL;
>> > +       retval = sendto(sd, packet, length, 0,
>> > +                  (struct sockaddr *)device, sizeof(struct
>> > sockaddr_ll));
>> > +       if (retval < 0)
>> > +               printf("Failed to send packet: %d %s\n", errno,
>> > +                      strerror(errno));
>> > +       return retval;
>> > +}
>> > +
>> > +int sandbox_raw_recv(void *packet, int *length, int sd, void *device)
>> > +{
>> > +       int retval;
>> > +       int saddr_size;
>> > +
>> > +       if (!sd || !device)
>> > +               return -EINVAL;
>> > +       saddr_size = sizeof(struct sockaddr);
>> > +       retval = recvfrom(sd, packet, 1536, 0, (struct sockaddr
>> > *)device,
>> > +                         (socklen_t *)&saddr_size);
>> > +       *length = 0;
>> > +       if (retval > 0) {
>> > +               *length = retval;
>> > +               return 0;
>> > +       }
>> > +
>> > +       return retval;
>> > +}
>> > +
>> > +void sandbox_raw_halt(int *sd, void **devp)
>> > +{
>> > +       free((struct sockaddr_ll *)*devp);
>> > +       *devp = NULL;
>> > +       close(*sd);
>> > +       *sd = -1;
>> > +}
>> > diff --git a/drivers/net/sandbox-raw.c b/drivers/net/sandbox-raw.c
>> > new file mode 100644
>> > index 0000000..90e462a
>> > --- /dev/null
>> > +++ b/drivers/net/sandbox-raw.c
>> > @@ -0,0 +1,128 @@
>> > +/*
>> > + * Copyright (c) 2015 National Instruments
>> > + *
>> > + * (C) Copyright 2015
>> > + * Joe Hershberger <joe.hershberger@ni.com>
>> > + *
>> > + * SPDX-License-Identifier:    GPL-2.0+
>> > + */
>> > +
>> > +#include <asm/sandbox-raw-os.h>
>> > +#include <common.h>
>> > +#include <dm.h>
>> > +#include <fdtdec.h>
>> > +#include <malloc.h>
>> > +#include <net.h>
>> > +
>> > +DECLARE_GLOBAL_DATA_PTR;
>> > +
>> > +struct eth_sandbox_raw_priv {
>> > +       int sd;
>> > +       void *device;
>>
>> comments
>>
>> > +};
>> > +
>> > +static int sb_eth_raw_init(struct udevice *dev, bd_t *bis)
>> > +{
>> > +       debug("eth_sandbox_raw: Init\n");
>>
>> Put after declarations (perhaps removing code from decls if you like)
>> in each case.
>>
>> > +
>> > +       struct eth_sandbox_raw_priv *priv = dev->priv;
>>
>> add blank line
>>
>> > +       if (!priv)
>> > +               return -EINVAL;
>>
>> As elsewhere I don't understand why this is needed
>>
>> > +
>> > +       struct eth_pdata *pdata = dev->platdata;
>> > +       int retval;
>> > +       const char *interface = fdt_getprop(gd->fdt_blob,
>> > dev->of_offset,
>> > +                                           "host-raw-interface", NULL);
>> > +
>> > +       retval = sandbox_raw_init(&priv->sd, &priv->device, interface,
>> > +                                 pdata->enetaddr);
>> > +
>> > +       return retval;
>> > +}
>> > +
>> > +static int sb_eth_raw_send(struct udevice *dev, void *packet, int
>> > length)
>> > +{
>> > +       debug("eth_sandbox_raw: Send packet %d\n", length);
>> > +
>> > +       struct eth_sandbox_raw_priv *priv = dev->priv;
>>
>> dev_get_priv(dev) in each case
>>
>> > +
>> > +       return sandbox_raw_send(packet, length, priv->sd, priv->device);
>> > +}
>> > +
>> > +static int sb_eth_raw_recv(struct udevice *dev)
>> > +{
>> > +       struct eth_sandbox_raw_priv *priv = dev->priv;
>> > +
>>
>> remove blank line
>>
>> > +       int retval;
>> > +       uchar buffer[PKTSIZE];
>> > +       int length;
>> > +
>> > +       if (!priv)
>> > +               return 0;
>> > +       retval = sandbox_raw_recv(buffer, &length, priv->sd,
>> > priv->device);
>> > +       if (!retval && length) {
>> > +               debug("eth_sandbox_raw: received packet %d\n",
>> > +                     length);
>> > +               NetReceive(buffer, length);
>> > +       }
>> > +       return 0;
>> > +}
>> > +
>> > +static void sb_eth_raw_halt(struct udevice *dev)
>> > +{
>> > +       debug("eth_sandbox_raw: Halt\n");
>> > +
>> > +       struct eth_sandbox_raw_priv *priv = dev->priv;
>> > +
>> > +       if (!priv)
>> > +               return;
>> > +       sandbox_raw_halt(&priv->sd, &priv->device);
>> > +}
>> > +
>> > +static int sb_eth_raw_write_hwaddr(struct udevice *dev)
>> > +{
>> > +       struct eth_pdata *pdata = dev->platdata;
>> > +
>> > +       debug("eth_sandbox_raw %s: Write HW ADDR - %pM\n", dev->name,
>> > +             pdata->enetaddr);
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct eth_ops sb_eth_raw_ops = {
>> > +       .init                   = sb_eth_raw_init,
>> > +       .send                   = sb_eth_raw_send,
>> > +       .recv                   = sb_eth_raw_recv,
>> > +       .halt                   = sb_eth_raw_halt,
>> > +       .write_hwaddr           = sb_eth_raw_write_hwaddr,
>> > +};
>> > +
>> > +static int sb_eth_raw_remove(struct udevice *dev)
>> > +{
>> > +       return 0;
>> > +}
>> > +
>> > +#ifdef CONFIG_OF_CONTROL
>>
>> This is always defined for sandbox.
>
> I figured it was a good pattern to use in general, but if you want me to
> prune it out of sandbox-only code I can.

I think so, it is pointless really and just clutters the code. I can't
imagine sandbox being built without device tree (famous last words!)

>
>> > +static int sb_eth_raw_ofdata_to_platdata(struct udevice *dev)
>> > +{
>> > +       struct eth_pdata *pdata = dev->platdata;
>> > +
>> > +       pdata->iobase = fdtdec_get_addr(gd->fdt_blob, dev->of_offset,
>> > "reg");
>>
>> dev_get_addr() is now available in dm/master if you prefer to use that.
>
> OK
>
>
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct udevice_id sb_eth_raw_ids[] = {
>> > +       { .compatible = "sandbox,eth,raw" },
>> > +       { }
>> > +};
>> > +#endif
>> > +
>> > +U_BOOT_DRIVER(eth_sandbox_raw) = {
>> > +       .name   = "eth_sandbox_raw",
>> > +       .id     = UCLASS_ETH,
>> > +       .of_match = of_match_ptr(sb_eth_raw_ids),
>> > +       .ofdata_to_platdata =
>> > of_match_ptr(sb_eth_raw_ofdata_to_platdata),
>> > +       .remove = sb_eth_raw_remove,
>> > +       .ops    = &sb_eth_raw_ops,
>> > +       .priv_auto_alloc_size = sizeof(struct eth_sandbox_raw_priv),
>> > +       .platdata_auto_alloc_size = sizeof(struct eth_pdata),
>> > +};
>> > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
>> > index 9df5f74..7afa3d2 100644
>> > --- a/include/configs/sandbox.h
>> > +++ b/include/configs/sandbox.h
>> > @@ -141,6 +141,7 @@
>> >
>> >  #define CONFIG_DM_ETH
>> >  #define CONFIG_ETH_SANDBOX
>> > +#define CONFIG_ETH_SANDBOX_RAW
>>
>> These should go in Kconfig. Could be a follow-on patch if that is easier.
>
> OK
>
>> >  #define CONFIG_CMD_PING
>> >
>> >  #define CONFIG_CMD_HASH
>> > --
>> > 1.7.11.5
>> >

Regards,
Simon