On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote: > Dear Pavel Herrmann, > > > This driver uses files as block devices, can be used for testing disk > > operations on sandbox. > > A new command "sata_loop" is introduced to load files in runtime. > > > > Signed-off-by: Pavel Herrmann <morpheus.i...@gmail.com> > > CC: Marek Vasut <ma...@denx.de> > > CC: Mike Frysinger <vap...@gentoo.org> > > ERROR: "foo * bar" should be "foo *bar" > #136: FILE: drivers/block/sata_loopback.c:36: > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE]; > > WARNING: externs should be avoided in .c files > #138: FILE: drivers/block/sata_loopback.c:38: > +extern block_dev_desc_t sata_dev_desc[]; > > ERROR: do not initialise statics to 0 or NULL > #142: FILE: drivers/block/sata_loopback.c:42: > + static int zeroed = 0; > > ERROR: spaces required around that '=' (ctx:VxV) > #150: FILE: drivers/block/sata_loopback.c:50: > + filenames[i]=strdup(""); > ^ > > ERROR: space prohibited after that open parenthesis '(' > #181: FILE: drivers/block/sata_loopback.c:81: > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte) > > ERROR: space prohibited after that open parenthesis '(' > #197: FILE: drivers/block/sata_loopback.c:97: > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte) > > ERROR: do not initialise statics to 0 or NULL > #256: FILE: drivers/block/sata_loopback.c:156: > + static int zeroed = 0; > > total: 6 errors, 1 warnings, 207 lines checked > > Checkpatch and I never sleep, don't even try ;-) >
sorry, my bad, i thought i had it on pre-commit > > --- > > > > Changes for v3: > > introduce sata_loop command > > > > Changes for v2: > > split sandbox config off into separate patch (2/2) > > rename file to signify exported API > > style fixes > > show end of long filenames rather than beginning > > check for lseek errors to indicate non-regular file > > > > drivers/block/Makefile | 1 + > > drivers/block/sata_loopback.c | 200 > > > > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 > > insertions(+) > > > > create mode 100644 drivers/block/sata_loopback.c > > > > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > > index f1ebdcc..c95651a 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_SATA_LOOP} += sata_loopback.o > > > > COBJS := $(COBJS-y) > > SRCS := $(COBJS:.o=.c) > > > > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c > > new file mode 100644 > > index 0000000..0e6923b > > --- /dev/null > > +++ b/drivers/block/sata_loopback.c > > @@ -0,0 +1,200 @@ > > +/* > > + * (C) Copyright 2012 > > + * Pavel Herrmann <morpheus.i...@gmail.com> > > + * > > + * 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 <common.h> > > +#include <part.h> > > +#include <ata.h> > > +#include <libata.h> > > +#include <errno.h> > > +#include <os.h> > > +#include <command.h> > > +#include <malloc.h> > > + > > +static const char revision[] = "0.0"; > > +static const char vendor[] = "SATA loopback"; > > + > > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE]; > > + > > +extern block_dev_desc_t sata_dev_desc[]; > > + > > +int init_sata(int dev) > > +{ > > + static int zeroed = 0; > > Is this really needed here and below? Pull this crap out. yes, because you dont know whetehr this gets called first from sata_loop or from "sata init" > > + block_dev_desc_t *pdev = &sata_dev_desc[dev]; > > + int fd, old_fd; > > + > > + > > Redundant newline > > > + if (!zeroed) { > > + int i; > > + for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++) > > + filenames[i]=strdup(""); > > + zeroed = 1; > > + } > > + > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) { > > + printf("File index %d is out of range.\n", dev); > > + return -EINVAL; > > + } > > + > > + fd = os_open(filenames[dev], OS_O_RDWR); > > + /* This is ugly, but saves allocation for 1 int. */ > > + old_fd = (long) pdev->priv; > > + pdev->priv = (void *) (long) fd; > > + /* > > + * sadly we cannot set -1 to all as above, because "sata init" will zero > > + * this value before calling sata_init. > > Sorry, I can't parse this. > > > + */ > > + if ((old_fd > 2) || (old_fd < 0)) > > + os_close(old_fd); > > + > > + return 0; > > +} > > + > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void > > *buffer) > > +{ > > + block_dev_desc_t *pdev = &sata_dev_desc[dev]; > > + int fd = (long) pdev->priv; > > + lbaint_t start_byte = ATA_SECT_SIZE * start; > > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt; > > + lbaint_t retval; > > + > > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte) > > + return -1; > > + > > + retval = os_read(fd, buffer, length_byte); > > + > > + return retval/ATA_SECT_SIZE; > > +} > > + > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void > > *buffer) +{ > > + block_dev_desc_t *pdev = &sata_dev_desc[dev]; > > + int fd = (long) pdev->priv; > > + lbaint_t start_byte = ATA_SECT_SIZE * start; > > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt; > > + lbaint_t retval; > > + > > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte) > > + return -1; > > + > > + retval = os_write(fd, buffer, length_byte); > > + > > + return retval/ATA_SECT_SIZE; > > a[space]operator[space]b ... did you ignore all my previous comments? > > > +} > > + > > +int scan_sata(int dev) > > +{ > > + block_dev_desc_t *pdev = &sata_dev_desc[dev]; > > + int fd = (long) pdev->priv; > > + int namelen; > > + const char *filename = filenames[dev]; > > + lbaint_t bytes = 0; > > + > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) { > > + printf("File index %d is out of range.\n", dev); > > + return -EINVAL; > > + } > > + > > + memcpy(pdev->vendor, vendor, sizeof(vendor)); > > + memcpy(pdev->revision, revision, sizeof(revision)); > > + namelen = strlen(filenames[dev]); > > + > > + if (namelen > 20) { > > + /* take the last 17 chars, prepend them with "..." */ > > + filename += (namelen - 17); > > + memcpy(pdev->product, "...", 3); > > + memcpy(pdev->product+3, filename, 17); > > + } else > > + memcpy(pdev->product, filename, namelen); > > + > > + pdev->product[20] = 0; > > + > > + bytes = os_lseek(fd, 0, OS_SEEK_END); > > + if (bytes != -1) { > > + pdev->type = DEV_TYPE_HARDDISK; > > + pdev->blksz = ATA_SECT_SIZE; > > + pdev->lun = 0; > > + pdev->lba = bytes/ATA_SECT_SIZE; > > + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:" > > + " %lu\n", dev, filenames[dev], bytes, pdev->lba); > > + } else { > > + pdev->type = DEV_TYPE_HARDDISK; > > + pdev->blksz = ATA_SECT_SIZE; > > + pdev->lun = 0; > > + pdev->lba = 0; > > + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n", > > + dev, filenames[dev]); > > + } > > + > > + return 0; > > +} > > + > > + > > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > +{ > > + int dev = 0; > > + static int zeroed = 0; > > move this out. > > besides, I think it'd be much systematic to just scream at user to call > "sata rescan" and bail out instead of doing it for him. i dont actually need a sata rescan, i just need to make sure i have dynamically allocated names, so i can safely call free() later, otherwise this segfaults when called before sata scan > > + /* make sure we have valid filenames */ > > + if (!zeroed) { > > + init_sata(0); > > + zeroed = 1; > > + } > > + > > + switch (argc) { > > + case 0: > > + case 1: > > + return CMD_RET_USAGE; > > + case 2: > > + dev = simple_strtoul(argv[1], NULL, 10); > > Ok, so if I run this command and ask for device 0xb00bf33d ... will this > survive? Hint: it won't, rangecheck missing. > > > + return scan_sata(dev); > > + > > + case 3: > > + if (!strncmp(argv[1], "inf", 3)) { > > + dev = simple_strtoul(argv[2], NULL, 10); > > Same here > > > + return scan_sata(dev); > > + } > > + return CMD_RET_USAGE; > > + case 4: > > + if (!strncmp(argv[1], "load", 4)) { > > + dev = simple_strtoul(argv[2], NULL, 10); > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) { > > And here you have it ? > > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into > lightweight static inline function. > > > + printf("File index %d is out of range.\n", dev); > > + return -EINVAL; > > + } > > + free(filenames[dev]); > > + filenames[dev] = strdup(argv[3]); > > + init_sata(dev); > > + return scan_sata(dev); > > + } > > + return CMD_RET_USAGE; > > + } > > + return CMD_RET_USAGE; > > +} > > + > > +U_BOOT_CMD( > > + sata_loop, 4, 1, do_loop, > > + "SATA loopback", > > + "[info] devnum - show info about loop devnum\n" > > Make this "info" part mandatory. Than you can cut the whole argc loop into > simple "if argc != 2 ; then fail" . And do simple checking for the first > letter of the argument being either i or d . > > > + "sata_loop load devnum file - load file from host FS into loop devnum" > > sata_loop is redundant above. > > > +); _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot