| Message ID | 1368654883.11014.29.camel@localhost |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Henrik, On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström <henrik@henriknordstrom.net> wrote: > Version 2 of this patch addressing the code comments by Simon and adding > some small missing pieces. Looks good. For change log, you should follow the standard approach - also instead of 'comments by Simon' it is good to list the things you changed. You might find patman useful for preparing and sending patches. > > left on the to-do is documentation and adding test suite tests. > > --- > A simple "host" block driver using any host file/device > as backing store. > > Adds two sb subcommands for managing host device bindings: > sb bind <dev> [<filename>] - bind "host" device to file > sb info [<dev>] - show device binding & info > > Signed-off-by: Henrik Nordstrom <henrik@henriknordstrom.net> > --- > common/cmd_sandbox.c | 54 +++++++++++++++++++ > disk/part.c | 23 +++----- > drivers/block/Makefile | 1 + > drivers/block/sandbox.c | 127 +++++++++++++++++++++++++++++++++++++++++++++ > include/config_fallbacks.h | 3 +- > include/configs/sandbox.h | 6 +++ > include/part.h | 3 ++ > include/sandboxblockdev.h | 29 +++++++++++ > 8 files changed, 228 insertions(+), 18 deletions(-) > create mode 100644 drivers/block/sandbox.c > create mode 100644 include/sandboxblockdev.h > > diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c > index a28a844..59fbc97 100644 > --- a/common/cmd_sandbox.c > +++ b/common/cmd_sandbox.c > @@ -19,6 +19,8 @@ > > #include <common.h> > #include <fs.h> > +#include <part.h> > +#include <sandboxblockdev.h> > > static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc, > char * const argv[]) > @@ -38,10 +40,60 @@ static int do_sandbox_save(cmd_tbl_t *cmdtp, int flag, int argc, > return do_save(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX, 16); > } > > +static int do_sandbox_bind(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + if (argc < 2 || argc > 3) > + return CMD_RET_USAGE; > + char *ep; > + char *dev_str = argv[1]; > + char *file = argc >= 3 ? argv[2] : NULL; > + int dev = simple_strtoul(dev_str, &ep, 16); blank line here, please fix globally (a blank line between declarations and code). > + if (*ep) { > + printf("** Bad device specification %s **\n", dev_str); > + return CMD_RET_USAGE; > + } > + return host_dev_bind(dev, file); > +} > + > +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc, > + char * const argv[]) > +{ > + if (argc < 1 || argc > 2) > + return CMD_RET_USAGE; Probably best to put this after the declarations > + int min_dev = 0; > + int max_dev = CONFIG_HOST_MAX_DEVICES - 1; blank line here > + if (argc >= 2) { > + char *ep; > + char *dev_str = argv[1]; > + int dev = simple_strtoul(dev_str, &ep, 16); and here > + if (*ep) { > + printf("** Bad device specification %s **\n", dev_str); > + return CMD_RET_USAGE; > + } > + min_dev = dev; > + max_dev = dev; > + } > + int dev; Move to top of function. Try to collect your declarations within a block when it's easy to do so. > + printf("%3s %12s %s\n", "dev", "blocks", "path"); > + for (dev = min_dev; dev <= max_dev; dev++) { > + printf("%3d ", dev); > + block_dev_desc_t *blk_dev = host_get_dev(dev); > + if (!blk_dev) > + continue; Does this case ever happen? If so you don't print \n. > + struct host_block_dev *host_dev = blk_dev->priv; Maybe leave the assignment here but put the declaration at the start of the block. > + printf("%12lu %s\n", (unsigned long)blk_dev->lba, > + host_dev->filename); > + } > + return 0; > +} > + > static cmd_tbl_t cmd_sandbox_sub[] = { > U_BOOT_CMD_MKENT(load, 7, 0, do_sandbox_load, "", ""), > U_BOOT_CMD_MKENT(ls, 3, 0, do_sandbox_ls, "", ""), > U_BOOT_CMD_MKENT(save, 6, 0, do_sandbox_save, "", ""), > + U_BOOT_CMD_MKENT(bind, 3, 0, do_sandbox_bind, "", ""), > + U_BOOT_CMD_MKENT(info, 3, 0, do_sandbox_info, "", ""), > }; > > static int do_sandbox(cmd_tbl_t *cmdtp, int flag, int argc, > @@ -70,4 +122,6 @@ U_BOOT_CMD( > "sb ls host <filename> - list files on host\n" > "sb save host <dev> <filename> <addr> <bytes> [<offset>] - " > "save a file to host\n" > + "sb bind <dev> [<filename>] - bind \"host\" device to file\n" > + "sb info [<dev>] - show device binding & info" > ); > diff --git a/disk/part.c b/disk/part.c > index d73625c..5906aef 100644 > --- a/disk/part.c > +++ b/disk/part.c > @@ -59,6 +59,9 @@ static const struct block_drvr block_drvr[] = { > #if defined(CONFIG_SYSTEMACE) > { .name = "ace", .get_dev = systemace_get_dev, }, > #endif > +#if defined(CONFIG_SANDBOX) > + { .name = "host", .get_dev = host_get_dev, }, > +#endif > { }, > }; > > @@ -302,6 +305,9 @@ static void print_part_header (const char *type, block_dev_desc_t * dev_desc) > case IF_TYPE_MMC: > puts ("MMC"); > break; > + case IF_TYPE_HOST: > + puts("HOST"); > + break; > default: > puts ("UNKNOWN"); > break; > @@ -462,23 +468,6 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str, > int part; > disk_partition_t tmpinfo; > > - /* > - * For now, we have a special case for sandbox, since there is no > - * real block device support. > - */ > - if (0 == strcmp(ifname, "host")) { > - *dev_desc = NULL; > - info->start = info->size = info->blksz = 0; > - info->bootable = 0; > - strcpy((char *)info->type, BOOT_PART_TYPE); > - strcpy((char *)info->name, "Sandbox host"); > -#ifdef CONFIG_PARTITION_UUIDS > - info->uuid[0] = 0; > -#endif > - > - return 0; > - } > - > /* If no dev_part_str, use bootdevice environment variable */ > if (!dev_part_str || !strlen(dev_part_str) || > !strcmp(dev_part_str, "-")) > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index f1ebdcc..2d2fb55 100644 > --- a/drivers/block/Makefile > +++ b/drivers/block/Makefile > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o > COBJS-$(CONFIG_IDE_SIL680) += sil680.o > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o > COBJS-$(CONFIG_SYSTEMACE) += systemace.o > +COBJS-$(CONFIG_SANDBOX) += sandbox.o Alphabetic order so this should go up two. > > COBJS := $(COBJS-y) > SRCS := $(COBJS:.o=.c) > diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c > new file mode 100644 > index 0000000..90f7dca > --- /dev/null > +++ b/drivers/block/sandbox.c > @@ -0,0 +1,127 @@ > +/* > + * Copyright (C) 2013 Henrik Nordstrom <henrik@henriknordstrom.net> > + * > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + */ > + > +#include <config.h> > +#include <common.h> > +#include <part.h> > +#include <os.h> > +#include <malloc.h> > +#include <sandboxblockdev.h> How about sandbox-blockdev.h > + > +static struct host_block_dev host_devices[CONFIG_HOST_MAX_DEVICES]; > + > +static struct host_block_dev * > +find_host_device(int dev) > +{ > + if (dev >= 0 && dev < CONFIG_HOST_MAX_DEVICES) > + return &host_devices[dev]; > + > + printf("Invalid host device number\n"); puts() when you don't have format args. Please fix globally. > + return NULL; > +} > + > +static unsigned long host_block_read(int dev, unsigned long start, > + lbaint_t blkcnt, void *buffer) > +{ > + struct host_block_dev *host_dev = find_host_device(dev); > + if (os_lseek(host_dev->fd, > + start * host_dev->blk_dev.blksz, > + OS_SEEK_SET) == -1) { > + printf("ERROR: Invalid position\n"); > + return -1; > + } > + ssize_t len = os_read(host_dev->fd, buffer, > + blkcnt * host_dev->blk_dev.blksz); > + if (len >= 0) > + return len / host_dev->blk_dev.blksz; > + return -1; > +} > + > +static unsigned long host_block_write(int dev, unsigned long start, > + lbaint_t blkcnt, const void *buffer) > +{ > + struct host_block_dev *host_dev = find_host_device(dev); > + if (os_lseek(host_dev->fd, > + start * host_dev->blk_dev.blksz, > + OS_SEEK_SET) == -1) { > + printf("ERROR: Invalid position\n"); > + return -1; > + } > + ssize_t len = os_write(host_dev->fd, buffer, blkcnt * > + host_dev->blk_dev.blksz); > + if (len >= 0) > + return len / host_dev->blk_dev.blksz; > + return -1; > +} > + > +int host_dev_bind(int dev, char *filename) > +{ > + struct host_block_dev *host_dev = find_host_device(dev); > + if (host_dev->blk_dev.priv) { > + os_close(host_dev->fd); > + host_dev->blk_dev.priv = NULL; > + } > + if (host_dev->filename) > + free(host_dev->filename); > + if (filename && *filename) { > + host_dev->filename = strdup(filename); > + } else { > + host_dev->filename = NULL; > + return 0; > + } > + > + host_dev->fd = os_open(host_dev->filename, OS_O_RDWR); > + if (host_dev->fd == -1) { > + printf("Failed to access host backing file '%s'\n", > + host_dev->filename); > + return 1; -ENOENT might be better (include errno.h) > + } > + > + block_dev_desc_t *blk_dev = &host_dev->blk_dev; > + blk_dev->if_type = IF_TYPE_HOST; > + blk_dev->priv = host_dev; > + blk_dev->blksz = 512; > + blk_dev->lba = os_lseek(host_dev->fd, 0, OS_SEEK_END) / blk_dev->blksz; > + blk_dev->block_read = host_block_read; > + blk_dev->block_write = host_block_write; > + blk_dev->dev = dev; > + blk_dev->part_type = PART_TYPE_UNKNOWN; > + init_part(blk_dev); > + > + return 0; > +} > + > +block_dev_desc_t *host_get_dev(int dev) > +{ > + struct host_block_dev *host_dev = find_host_device(dev); > + > + if (!host_dev) > + return NULL; > + > + if (!host_dev->blk_dev.priv) { > + printf("Not bound to a backing file\n"); > + return NULL; > + } > + > + block_dev_desc_t *blk_dev = &host_dev->blk_dev; > + return blk_dev; > +} > diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h > index e59ee96..9ea8c09 100644 > --- a/include/config_fallbacks.h > +++ b/include/config_fallbacks.h > @@ -49,7 +49,8 @@ > defined(CONFIG_CMD_USB) || \ > defined(CONFIG_CMD_PART) || \ > defined(CONFIG_MMC) || \ > - defined(CONFIG_SYSTEMACE) > + defined(CONFIG_SYSTEMACE) || \ > + defined(CONFIG_SANDBOX) > #define HAVE_BLOCK_DEVICE > #endif > > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h > index 788207d..844a49f 100644 > --- a/include/configs/sandbox.h > +++ b/include/configs/sandbox.h > @@ -38,6 +38,11 @@ > #define CONFIG_CMD_FAT > #define CONFIG_CMD_EXT4 > #define CONFIG_CMD_EXT4_WRITE > +#define CONFIG_CMD_PART > +#define CONFIG_DOS_PARTITION > +#define CONFIG_PARTITION_UUIDS > +#define CONFIG_EFI_PARTITION > +#define CONFIG_HOST_MAX_DEVICES 4 > > #define CONFIG_SYS_VSNPRINTF > > @@ -45,6 +50,7 @@ > #define CONFIG_SANDBOX_GPIO > #define CONFIG_SANDBOX_GPIO_COUNT 20 > > + > /* > * Size of malloc() pool, although we don't actually use this yet. > */ > diff --git a/include/part.h b/include/part.h > index f7c7cc5..a29d615 100644 > --- a/include/part.h > +++ b/include/part.h > @@ -74,6 +74,7 @@ typedef struct block_dev_desc { > #define IF_TYPE_MMC 6 > #define IF_TYPE_SD 7 > #define IF_TYPE_SATA 8 > +#define IF_TYPE_HOST 9 > > /* Part types */ > #define PART_TYPE_UNKNOWN 0x00 > @@ -118,6 +119,7 @@ block_dev_desc_t* usb_stor_get_dev(int dev); > block_dev_desc_t* mmc_get_dev(int dev); > block_dev_desc_t* systemace_get_dev(int dev); > block_dev_desc_t* mg_disk_get_dev(int dev); > +block_dev_desc_t *host_get_dev(int dev); > > /* disk/part.c */ > int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t *info); > @@ -139,6 +141,7 @@ static inline block_dev_desc_t* usb_stor_get_dev(int dev) { return NULL; } > static inline block_dev_desc_t* mmc_get_dev(int dev) { return NULL; } > static inline block_dev_desc_t* systemace_get_dev(int dev) { return NULL; } > static inline block_dev_desc_t* mg_disk_get_dev(int dev) { return NULL; } > +static inline block_dev_desc_t *host_get_dev(int dev) { return NULL; } > > static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, > disk_partition_t *info) { return -1; } > diff --git a/include/sandboxblockdev.h b/include/sandboxblockdev.h > new file mode 100644 > index 0000000..8723062 > --- /dev/null > +++ b/include/sandboxblockdev.h > @@ -0,0 +1,29 @@ > +/* > + * Copyright (c) 2013, Henrik Nordstrom <henrik@henriknordstrom.net> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ > + > +#ifndef __SANDBOX_BLOCK_DEV__ > +#define __SANDBOX_BLOCK_DEV__ > + > +struct host_block_dev { > + block_dev_desc_t blk_dev; > + char *filename; > + int fd; > +}; > + > +int host_dev_bind(int dev, char *filename); Please add a comment as to what this function does, describing the meaning and valid values for each argument. > + > +#endif > -- > 1.8.1.4 > > > For testing, I tried: =>sb bind 0 chromiumos_test_image.bin =>sb info dev blocks path 0 4956096 chromiumos_test_image.bin 1 Not bound to a backing file 2 Not bound to a backing file 3 Not bound to a backing file =>part list host 0 (works fine) =>ext4load host 0:3 Segmentation fault (core dumped) (not sure why that is...maybe a bug?) FAT works though: =>fatls host 0:c u-boot/ 3451512 vmlinuz.uimg.a 3250192 vmlinuz.a 2 file(s), 1 dir(s) => Regards, Simon
ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass: > Hi Henrik, > > On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström > <henrik@henriknordstrom.net> wrote: > > Version 2 of this patch addressing the code comments by Simon and adding > > some small missing pieces. > > Looks good. > > For change log, you should follow the standard approach - also instead > of 'comments by Simon' it is good to list the things you changed. You > might find patman useful for preparing and sending patches. Just looked into it and looks nice. Giving it a try for next version. Took a little while to get started, mostly because it crashes & burns when not finding an matching alias for sandbox. > blank line here, please fix globally (a blank line between > declarations and code). Ok. > > +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc, > > + char * const argv[]) > > +{ > > + if (argc < 1 || argc > 2) > > + return CMD_RET_USAGE; > > Probably best to put this after the declarations Ok. Also restrucured do_sandbox_bind a little to match this. There one of the declarations depends on this being checked first. > Move to top of function. Try to collect your declarations within a > block when it's easy to do so. Ok. > > + printf("%3s %12s %s\n", "dev", "blocks", "path"); > > + for (dev = min_dev; dev <= max_dev; dev++) { > > + printf("%3d ", dev); > > + block_dev_desc_t *blk_dev = host_get_dev(dev); > > + if (!blk_dev) > > + continue; > > Does this case ever happen? If so you don't print \n. Yes. And it then relies on the driver to print an error. > > + struct host_block_dev *host_dev = blk_dev->priv; > > Maybe leave the assignment here but put the declaration at the start > of the block. Yes. > > COBJS-$(CONFIG_IDE_SIL680) += sil680.o > > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o > > COBJS-$(CONFIG_SYSTEMACE) += systemace.o > > +COBJS-$(CONFIG_SANDBOX) += sandbox.o > > Alphabetic order so this should go up two. up seven. sorted on filename here not CONFIG_.. > > +#include <sandboxblockdev.h> > > How about sandbox-blockdev.h Yee. > puts() when you don't have format args. Please fix globally. Ok. > > + if (host_dev->fd == -1) { > > + printf("Failed to access host backing file '%s'\n", > > + host_dev->filename); > > + return 1; > > -ENOENT might be better (include errno.h) or maybe just -errno? and handle the error in do_sandbox_bind(). > > +int host_dev_bind(int dev, char *filename); > > Please add a comment as to what this function does, describing the > meaning and valid values for each argument. Ok. > =>ext4load host 0:3 > Segmentation fault (core dumped) Doesn't ext2load expect a address & filename to load? Regads Henrik
Hi Henrik, On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström <henrik@henriknordstrom.net> wrote: > ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass: >> Hi Henrik, >> >> On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström >> <henrik@henriknordstrom.net> wrote: >> > Version 2 of this patch addressing the code comments by Simon and adding >> > some small missing pieces. >> >> Looks good. >> >> For change log, you should follow the standard approach - also instead >> of 'comments by Simon' it is good to list the things you changed. You >> might find patman useful for preparing and sending patches. > > Just looked into it and looks nice. Giving it a try for next version. > > Took a little while to get started, mostly because it crashes & burns > when not finding an matching alias for sandbox. > >> blank line here, please fix globally (a blank line between >> declarations and code). > > Ok. > >> > +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc, >> > + char * const argv[]) >> > +{ >> > + if (argc < 1 || argc > 2) >> > + return CMD_RET_USAGE; >> >> Probably best to put this after the declarations > > Ok. Also restrucured do_sandbox_bind a little to match this. There one > of the declarations depends on this being checked first. > > >> Move to top of function. Try to collect your declarations within a >> block when it's easy to do so. > > Ok. > > >> > + printf("%3s %12s %s\n", "dev", "blocks", "path"); >> > + for (dev = min_dev; dev <= max_dev; dev++) { >> > + printf("%3d ", dev); >> > + block_dev_desc_t *blk_dev = host_get_dev(dev); >> > + if (!blk_dev) >> > + continue; >> >> Does this case ever happen? If so you don't print \n. > > Yes. And it then relies on the driver to print an error. > >> > + struct host_block_dev *host_dev = blk_dev->priv; >> >> Maybe leave the assignment here but put the declaration at the start >> of the block. > > Yes. > >> > COBJS-$(CONFIG_IDE_SIL680) += sil680.o >> > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o >> > COBJS-$(CONFIG_SYSTEMACE) += systemace.o >> > +COBJS-$(CONFIG_SANDBOX) += sandbox.o >> >> Alphabetic order so this should go up two. > > up seven. sorted on filename here not CONFIG_.. > >> > +#include <sandboxblockdev.h> >> >> How about sandbox-blockdev.h > > Yee. > >> puts() when you don't have format args. Please fix globally. > > Ok. > >> > + if (host_dev->fd == -1) { >> > + printf("Failed to access host backing file '%s'\n", >> > + host_dev->filename); >> > + return 1; >> >> -ENOENT might be better (include errno.h) > > or maybe just -errno? > > and handle the error in do_sandbox_bind(). > >> > +int host_dev_bind(int dev, char *filename); >> >> Please add a comment as to what this function does, describing the >> meaning and valid values for each argument. > > Ok. > >> =>ext4load host 0:3 >> Segmentation fault (core dumped) > > Doesn't ext2load expect a address & filename to load? Sorry I meant: =>ext4ls host 0:3 Segmentation fault (core dumped) It may not be your code, but I think the segfault is there. > > Regads > Henrik > Regards, Simon
tor 2013-05-16 klockan 21:53 -0700 skrev Simon Glass: > Sorry I meant: > > =>ext4ls host 0:3 > Segmentation fault (core dumped) > > It may not be your code, but I think the segfault is there. It's crashing in ext4 for me. #0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56 #0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56 56 get_fs()->total_sect = (info->size * info->blksz) >> 57 get_fs()->dev_desc->log2blksz; 58 get_fs()->dev_desc = rbdd; p(gdb) p get_fs() $2 = (struct ext_filesystem *) 0x644410 <ext_fs> (gdb) p $2->dev_desc $5 = (block_dev_desc_t *) 0x0 Looks like a generic ext4 bug in the new block size handling code. I think it just needs to be shuffled around a bit so the dev_desc is assigned before, not after. Regards Henrik
Hi Henrik, On Sat, May 18, 2013 at 7:24 AM, Henrik Nordström <henrik@henriknordstrom.net> wrote: > tor 2013-05-16 klockan 21:53 -0700 skrev Simon Glass: > >> Sorry I meant: >> >> =>ext4ls host 0:3 >> Segmentation fault (core dumped) >> >> It may not be your code, but I think the segfault is there. > > It's crashing in ext4 for me. > > #0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, > info=info@entry=0x643980 <fs_partition>) at dev.c:56 > > #0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, > info=info@entry=0x643980 <fs_partition>) at dev.c:56 > 56 get_fs()->total_sect = (info->size * info->blksz) >> > 57 get_fs()->dev_desc->log2blksz; > 58 get_fs()->dev_desc = rbdd; > > p(gdb) p get_fs() > $2 = (struct ext_filesystem *) 0x644410 <ext_fs> > (gdb) p $2->dev_desc > $5 = (block_dev_desc_t *) 0x0 > > Looks like a generic ext4 bug in the new block size handling code. > > I think it just needs to be shuffled around a bit so the dev_desc is > assigned before, not after. Well that's a great bug to find. It isn't related to your patch though - if you have time it would be nice to get a separate patch to fix that bug. > > Regards > Henrik > Regards, Simon
HI Henrik, On Thu, May 16, 2013 at 9:53 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Henrik, > > On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström > <henrik@henriknordstrom.net> wrote: > > ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass: > >> Hi Henrik, > >> > >> On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström > >> <henrik@henriknordstrom.net> wrote: > >> > Version 2 of this patch addressing the code comments by Simon and > adding > >> > some small missing pieces. > >> > >> Looks good. > >> > >> For change log, you should follow the standard approach - also instead > >> of 'comments by Simon' it is good to list the things you changed. You > >> might find patman useful for preparing and sending patches. > > > > Just looked into it and looks nice. Giving it a try for next version. > > > Did you send a new version? I might have missed it. Regards, Simon
sön 2013-06-16 klockan 07:50 -0700 skrev Simon Glass:
> Did you send a new version? I might have missed it.
Not yet. Work got in a bit of a panic mode and then been ill for a
while. Not forgotten.
Regards
Henrik
Hi Henrik, On Sun, Jun 16, 2013 at 7:39 PM, Henrik Nordström < henrik@henriknordstrom.net> wrote: > sön 2013-06-16 klockan 07:50 -0700 skrev Simon Glass: > > > Did you send a new version? I might have missed it. > > Not yet. Work got in a bit of a panic mode and then been ill for a > while. Not forgotten. > FYI I think there the blksz field needs to be set also, in drivers/block/sandbox.c. I am using: ... blk_dev->priv = host_dev; blk_dev->log2blksz = 9; blk_dev->blksz = 1 << blk_dev->log2blksz; blk_dev->lba = os_lseek(host_dev->fd, 0, OS_SEEK_END) / blk_dev->blksz; ... Regards, Simon
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index a28a844..59fbc97 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -19,6 +19,8 @@ #include <common.h> #include <fs.h> +#include <part.h> +#include <sandboxblockdev.h> static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -38,10 +40,60 @@ static int do_sandbox_save(cmd_tbl_t *cmdtp, int flag, int argc, return do_save(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX, 16); } +static int do_sandbox_bind(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + if (argc < 2 || argc > 3) + return CMD_RET_USAGE; + char *ep; + char *dev_str = argv[1]; + char *file = argc >= 3 ? argv[2] : NULL; + int dev = simple_strtoul(dev_str, &ep, 16); + if (*ep) { + printf("** Bad device specification %s **\n", dev_str); + return CMD_RET_USAGE; + } + return host_dev_bind(dev, file); +} + +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + if (argc < 1 || argc > 2) + return CMD_RET_USAGE; + int min_dev = 0; + int max_dev = CONFIG_HOST_MAX_DEVICES - 1; + if (argc >= 2) { + char *ep; + char *dev_str = argv[1]; + int dev = simple_strtoul(dev_str, &ep, 16); + if (*ep) { + printf("** Bad device specification %s **\n", dev_str); + return CMD_RET_USAGE; + } + min_dev = dev; + max_dev = dev; + } + int dev; + printf("%3s %12s %s\n", "dev", "blocks", "path"); + for (dev = min_dev; dev <= max_dev; dev++) { + printf("%3d ", dev); + block_dev_desc_t *blk_dev = host_get_dev(dev); + if (!blk_dev) + continue; + struct host_block_dev *host_dev = blk_dev->priv; + printf("%12lu %s\n", (unsigned long)blk_dev->lba, + host_dev->filename); + } + return 0; +} + static cmd_tbl_t cmd_sandbox_sub[] = { U_BOOT_CMD_MKENT(load, 7, 0, do_sandbox_load, "", ""), U_BOOT_CMD_MKENT(ls, 3, 0, do_sandbox_ls, "", ""), U_BOOT_CMD_MKENT(save, 6, 0, do_sandbox_save, "", ""), + U_BOOT_CMD_MKENT(bind, 3, 0, do_sandbox_bind, "", ""), + U_BOOT_CMD_MKENT(info, 3, 0, do_sandbox_info, "", ""), }; static int do_sandbox(cmd_tbl_t *cmdtp, int flag, int argc, @@ -70,4 +122,6 @@ U_BOOT_CMD( "sb ls host <filename> - list files on host\n" "sb save host <dev> <filename> <addr> <bytes> [<offset>] - " "save a file to host\n" + "sb bind <dev> [<filename>] - bind \"host\" device to file\n" + "sb info [<dev>] - show device binding & info" ); diff --git a/disk/part.c b/disk/part.c index d73625c..5906aef 100644 --- a/disk/part.c +++ b/disk/part.c @@ -59,6 +59,9 @@ static const struct block_drvr block_drvr[] = { #if defined(CONFIG_SYSTEMACE) { .name = "ace", .get_dev = systemace_get_dev, }, #endif +#if defined(CONFIG_SANDBOX) + { .name = "host", .get_dev = host_get_dev, }, +#endif { }, }; @@ -302,6 +305,9 @@ static void print_part_header (const char *type, block_dev_desc_t * dev_desc) case IF_TYPE_MMC: puts ("MMC"); break; + case IF_TYPE_HOST: + puts("HOST"); + break; default: puts ("UNKNOWN"); break; @@ -462,23 +468,6 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str, int part; disk_partition_t tmpinfo; - /* - * For now, we have a special case for sandbox, since there is no - * real block device support. - */ - if (0 == strcmp(ifname, "host")) { - *dev_desc = NULL; - info->start = info->size = info->blksz = 0; - info->bootable = 0; - strcpy((char *)info->type, BOOT_PART_TYPE); - strcpy((char *)info->name, "Sandbox host"); -#ifdef CONFIG_PARTITION_UUIDS - info->uuid[0] = 0; -#endif - - return 0; - } - /* If no dev_part_str, use bootdevice environment variable */ if (!dev_part_str || !strlen(dev_part_str) || !strcmp(dev_part_str, "-")) diff --git a/drivers/block/Makefile b/drivers/block/Makefile index f1ebdcc..2d2fb55 100644 --- a/drivers/block/Makefile +++ b/drivers/block/Makefile @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o COBJS-$(CONFIG_IDE_SIL680) += sil680.o COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o COBJS-$(CONFIG_SYSTEMACE) += systemace.o +COBJS-$(CONFIG_SANDBOX) += sandbox.o COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c new file mode 100644 index 0000000..90f7dca --- /dev/null +++ b/drivers/block/sandbox.c @@ -0,0 +1,127 @@ +/* + * Copyright (C) 2013 Henrik Nordstrom <henrik@henriknordstrom.net> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <config.h> +#include <common.h> +#include <part.h> +#include <os.h> +#include <malloc.h> +#include <sandboxblockdev.h> + +static struct host_block_dev host_devices[CONFIG_HOST_MAX_DEVICES]; + +static struct host_block_dev * +find_host_device(int dev) +{ + if (dev >= 0 && dev < CONFIG_HOST_MAX_DEVICES) + return &host_devices[dev]; + + printf("Invalid host device number\n"); + return NULL; +} + +static unsigned long host_block_read(int dev, unsigned long start, + lbaint_t blkcnt, void *buffer) +{ + struct host_block_dev *host_dev = find_host_device(dev); + if (os_lseek(host_dev->fd, + start * host_dev->blk_dev.blksz, + OS_SEEK_SET) == -1) { + printf("ERROR: Invalid position\n"); + return -1; + } + ssize_t len = os_read(host_dev->fd, buffer, + blkcnt * host_dev->blk_dev.blksz); + if (len >= 0) + return len / host_dev->blk_dev.blksz; + return -1; +} + +static unsigned long host_block_write(int dev, unsigned long start, + lbaint_t blkcnt, const void *buffer) +{ + struct host_block_dev *host_dev = find_host_device(dev); + if (os_lseek(host_dev->fd, + start * host_dev->blk_dev.blksz, + OS_SEEK_SET) == -1) { + printf("ERROR: Invalid position\n"); + return -1; + } + ssize_t len = os_write(host_dev->fd, buffer, blkcnt * + host_dev->blk_dev.blksz); + if (len >= 0) + return len / host_dev->blk_dev.blksz; + return -1; +} + +int host_dev_bind(int dev, char *filename) +{ + struct host_block_dev *host_dev = find_host_device(dev); + if (host_dev->blk_dev.priv) { + os_close(host_dev->fd); + host_dev->blk_dev.priv = NULL; + } + if (host_dev->filename) + free(host_dev->filename); + if (filename && *filename) { + host_dev->filename = strdup(filename); + } else { + host_dev->filename = NULL; + return 0; + } + + host_dev->fd = os_open(host_dev->filename, OS_O_RDWR); + if (host_dev->fd == -1) { + printf("Failed to access host backing file '%s'\n", + host_dev->filename); + return 1; + } + + block_dev_desc_t *blk_dev = &host_dev->blk_dev; + blk_dev->if_type = IF_TYPE_HOST; + blk_dev->priv = host_dev; + blk_dev->blksz = 512; + blk_dev->lba = os_lseek(host_dev->fd, 0, OS_SEEK_END) / blk_dev->blksz; + blk_dev->block_read = host_block_read; + blk_dev->block_write = host_block_write; + blk_dev->dev = dev; + blk_dev->part_type = PART_TYPE_UNKNOWN; + init_part(blk_dev); + + return 0; +} + +block_dev_desc_t *host_get_dev(int dev) +{ + struct host_block_dev *host_dev = find_host_device(dev); + + if (!host_dev) + return NULL; + + if (!host_dev->blk_dev.priv) { + printf("Not bound to a backing file\n"); + return NULL; + } + + block_dev_desc_t *blk_dev = &host_dev->blk_dev; + return blk_dev; +} diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index e59ee96..9ea8c09 100644 --- a/include/config_fallbacks.h +++ b/include/config_fallbacks.h @@ -49,7 +49,8 @@ defined(CONFIG_CMD_USB) || \ defined(CONFIG_CMD_PART) || \ defined(CONFIG_MMC) || \ - defined(CONFIG_SYSTEMACE) + defined(CONFIG_SYSTEMACE) || \ + defined(CONFIG_SANDBOX) #define HAVE_BLOCK_DEVICE #endif diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 788207d..844a49f 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -38,6 +38,11 @@ #define CONFIG_CMD_FAT #define CONFIG_CMD_EXT4 #define CONFIG_CMD_EXT4_WRITE +#define CONFIG_CMD_PART +#define CONFIG_DOS_PARTITION +#define CONFIG_PARTITION_UUIDS +#define CONFIG_EFI_PARTITION +#define CONFIG_HOST_MAX_DEVICES 4 #define CONFIG_SYS_VSNPRINTF @@ -45,6 +50,7 @@ #define CONFIG_SANDBOX_GPIO #define CONFIG_SANDBOX_GPIO_COUNT 20 + /* * Size of malloc() pool, although we don't actually use this yet. */ diff --git a/include/part.h b/include/part.h index f7c7cc5..a29d615 100644 --- a/include/part.h +++ b/include/part.h @@ -74,6 +74,7 @@ typedef struct block_dev_desc { #define IF_TYPE_MMC 6 #define IF_TYPE_SD 7 #define IF_TYPE_SATA 8 +#define IF_TYPE_HOST 9 /* Part types */ #define PART_TYPE_UNKNOWN 0x00 @@ -118,6 +119,7 @@ block_dev_desc_t* usb_stor_get_dev(int dev); block_dev_desc_t* mmc_get_dev(int dev); block_dev_desc_t* systemace_get_dev(int dev); block_dev_desc_t* mg_disk_get_dev(int dev); +block_dev_desc_t *host_get_dev(int dev); /* disk/part.c */ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t *info); @@ -139,6 +141,7 @@ static inline block_dev_desc_t* usb_stor_get_dev(int dev) { return NULL; } static inline block_dev_desc_t* mmc_get_dev(int dev) { return NULL; } static inline block_dev_desc_t* systemace_get_dev(int dev) { return NULL; } static inline block_dev_desc_t* mg_disk_get_dev(int dev) { return NULL; } +static inline block_dev_desc_t *host_get_dev(int dev) { return NULL; } static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t *info) { return -1; } diff --git a/include/sandboxblockdev.h b/include/sandboxblockdev.h new file mode 100644 index 0000000..8723062 --- /dev/null +++ b/include/sandboxblockdev.h @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2013, Henrik Nordstrom <henrik@henriknordstrom.net> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#ifndef __SANDBOX_BLOCK_DEV__ +#define __SANDBOX_BLOCK_DEV__ + +struct host_block_dev { + block_dev_desc_t blk_dev; + char *filename; + int fd; +}; + +int host_dev_bind(int dev, char *filename); + +#endif
Version 2 of this patch addressing the code comments by Simon and adding some small missing pieces. left on the to-do is documentation and adding test suite tests. --- A simple "host" block driver using any host file/device as backing store. Adds two sb subcommands for managing host device bindings: sb bind <dev> [<filename>] - bind "host" device to file sb info [<dev>] - show device binding & info Signed-off-by: Henrik Nordstrom <henrik@henriknordstrom.net> --- common/cmd_sandbox.c | 54 +++++++++++++++++++ disk/part.c | 23 +++----- drivers/block/Makefile | 1 + drivers/block/sandbox.c | 127 +++++++++++++++++++++++++++++++++++++++++++++ include/config_fallbacks.h | 3 +- include/configs/sandbox.h | 6 +++ include/part.h | 3 ++ include/sandboxblockdev.h | 29 +++++++++++ 8 files changed, 228 insertions(+), 18 deletions(-) create mode 100644 drivers/block/sandbox.c create mode 100644 include/sandboxblockdev.h