[U-Boot] [RFC PATCH v3 14/14] dm: eth: Add a bridge to a real network for sandbox
Simon Glass
sjg at chromium.org
Wed Feb 18 06:02:24 CET 2015
Hi Joe,
On 16 February 2015 at 22:16, Joe Hershberger <joe.hershberger at gmail.com> wrote:
> Hi Simon,
>
> On Sun, Feb 15, 2015 at 9:50 AM, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Joe,
>>
>> On 10 February 2015 at 18:30, Joe Hershberger <joe.hershberger at 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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list