Hi Joe, On 16 February 2015 at 22:16, Joe Hershberger <joe.hershber...@gmail.com> wrote: > Hi Simon, > > On Sun, Feb 15, 2015 at 9:50 AM, Simon Glass <s...@chromium.org> wrote: >> >> Hi Joe, >> >> On 10 February 2015 at 18:30, Joe Hershberger <joe.hershber...@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.hershber...@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@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.hershber...@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.hershber...@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.hershber...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
