| Message ID | 1346255203-1225-1-git-send-email-morpheus.ibis@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
More CC On Wednesday 29 August 2012 17:46:43 Pavel Herrmann wrote: > This driver uses files as block devices, can be used for testing disk > operations on sandbox. Port count and filenames are set in board config. > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com> > CC: Marek Vasut <marex@denx.de> > --- > drivers/block/Makefile | 1 + > drivers/block/loop.c | 107 > ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h | > 9 ++++ > 3 files changed, 117 insertions(+) > create mode 100644 drivers/block/loop.c > > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index f1ebdcc..5eecf37 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} += loop.o > > COBJS := $(COBJS-y) > SRCS := $(COBJS:.o=.c) > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > new file mode 100644 > index 0000000..c9edfc3 > --- /dev/null > +++ b/drivers/block/loop.c > @@ -0,0 +1,107 @@ > +/* > + * (C) Copyright 2012 > + * Pavel Herrmann <morpheus.ibis@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> > + > +static const char revision[] = "0.0"; > +static const char vendor[] = "loopback"; > + > +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS; > +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE; > + > +extern block_dev_desc_t sata_dev_desc[]; > + > +int init_sata(int dev) > +{ > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > + int fd; > + > + if ((dev < 0) || (dev >= max_devs)) { > + 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 */ > + pdev->priv = (void *) (long) 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 retval; > + > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > + > + 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 retval; > + > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > + retval = os_write(fd, buffer, ATA_SECT_SIZE * blkcnt); > + > + return retval/ATA_SECT_SIZE; > +} > + > +int scan_sata(int dev) > +{ > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > + int fd = (long) pdev->priv; > + int namelen; > + lbaint_t bytes = 0; > + memcpy(pdev->vendor, vendor, sizeof(vendor)); > + memcpy(pdev->revision, revision, sizeof(revision)); > + namelen = sizeof(filenames[dev]); > + if (namelen > 20) > + namelen = 20; > + memcpy(pdev->product, filenames[dev], namelen); > + pdev->product[20] = 0; > + > + if (fd != -1) { > + pdev->type = DEV_TYPE_HARDDISK; > + pdev->blksz = ATA_SECT_SIZE; > + pdev->lun = 0; > + > + bytes = os_lseek(fd, 0, OS_SEEK_END); > + pdev->lba = bytes/ATA_SECT_SIZE; > + } > + > + printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n", > + filenames[dev], bytes, pdev->lba); > + > + return 0; > +} > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h > index 0220386..412341f 100644 > --- a/include/configs/sandbox.h > +++ b/include/configs/sandbox.h > @@ -93,4 +93,13 @@ > "stdout=serial\0" \ > "stderr=serial\0" > > +/* SATA loopback device */ > +#define CONFIG_CMD_SATA > +#define CONFIG_SATA_LOOP > +#define CONFIG_SATA_LOOP_DISKS {"disk1", "disk2"} > +#define CONFIG_SYS_SATA_MAX_DEVICE 2 > +#define CONFIG_DOS_PARTITION > +#define CONFIG_CMD_FAT > +#define CONFIG_CMD_EXT2 > + > #endif
Dear Pavel Herrmann, > This driver uses files as block devices, can be used for testing disk > operations on sandbox. Port count and filenames are set in board config. > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com> > CC: Marek Vasut <marex@denx.de> > --- > drivers/block/Makefile | 1 + > drivers/block/loop.c | 107 > ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h | > 9 ++++ > 3 files changed, 117 insertions(+) > create mode 100644 drivers/block/loop.c > > diff --git a/drivers/block/Makefile b/drivers/block/Makefile > index f1ebdcc..5eecf37 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} += loop.o Move this to a more descriptive filename, maybe sata_loopback.c ? > > COBJS := $(COBJS-y) > SRCS := $(COBJS:.o=.c) > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > new file mode 100644 > index 0000000..c9edfc3 > --- /dev/null > +++ b/drivers/block/loop.c > @@ -0,0 +1,107 @@ > +/* > + * (C) Copyright 2012 > + * Pavel Herrmann <morpheus.ibis@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> > + > +static const char revision[] = "0.0"; > +static const char vendor[] = "loopback"; > + > +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS; > +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE; > + > +extern block_dev_desc_t sata_dev_desc[]; > + > +int init_sata(int dev) > +{ > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); Superfluous braces ... Actually, I think sata_dev_desc as it would work very well too. > + int fd; > + > + if ((dev < 0) || (dev >= max_devs)) { > + printf("file index %d is out of range\n", dev); Capital "F" and period at the end of a sentence. > + return -EINVAL; > + } > + > + fd = os_open(filenames[dev], OS_O_RDWR); > + /* this is ugly, but saves allocation for 1 int */ Same here. > + pdev->priv = (void *) (long) 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; If pdev is NULL, this will crash > + lbaint_t retval; > + > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > + > + 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 retval; > + > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); Please do the multiplication in a consistent manner, same for division etc. ... like you do on the following line (x1 [space] oper [space] x2). Besides, lseek can fail, can it not? > + retval = os_write(fd, buffer, ATA_SECT_SIZE * blkcnt); > + > + return retval/ATA_SECT_SIZE; > +} > + > +int scan_sata(int dev) > +{ > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > + int fd = (long) pdev->priv; > + int namelen; > + lbaint_t bytes = 0; newline here > + memcpy(pdev->vendor, vendor, sizeof(vendor)); > + memcpy(pdev->revision, revision, sizeof(revision)); > + namelen = sizeof(filenames[dev]); newline here > + if (namelen > 20) > + namelen = 20; Why do you trim down the string, won't simple strdup() work? newline here > + memcpy(pdev->product, filenames[dev], namelen); > + pdev->product[20] = 0; > + > + if (fd != -1) { And if "fd" is -1 ? > + pdev->type = DEV_TYPE_HARDDISK; > + pdev->blksz = ATA_SECT_SIZE; > + pdev->lun = 0; > + > + bytes = os_lseek(fd, 0, OS_SEEK_END); > + pdev->lba = bytes/ATA_SECT_SIZE; > + } > + > + printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n", > + filenames[dev], bytes, pdev->lba); > + > + return 0; > +} > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h > index 0220386..412341f 100644 > --- a/include/configs/sandbox.h > +++ b/include/configs/sandbox.h > @@ -93,4 +93,13 @@ > "stdout=serial\0" \ > "stderr=serial\0" > > +/* SATA loopback device */ > +#define CONFIG_CMD_SATA > +#define CONFIG_SATA_LOOP > +#define CONFIG_SATA_LOOP_DISKS {"disk1", "disk2"} > +#define CONFIG_SYS_SATA_MAX_DEVICE 2 > +#define CONFIG_DOS_PARTITION > +#define CONFIG_CMD_FAT > +#define CONFIG_CMD_EXT2 Move this to a separate patch. > #endif Best regards, Marek Vasut
On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: ...snip... > > +extern block_dev_desc_t sata_dev_desc[]; > > + > > +int init_sata(int dev) > > +{ > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > Superfluous braces ... Actually, I think sata_dev_desc as it would work very > well too. Straight copy from dwc_ahsata.c, makes it more readable thought, as the order of operation is not very intuitive IMHO. > > +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; > > If pdev is NULL, this will crash well, it isn't, at least not from the command - thats why you define the number of ports in advance, you get "dev" already range-checked > > + lbaint_t retval; > > + > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > > + > > + 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 retval; > > + > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > Besides, lseek can fail, can it not? If you open a pipe (or nothing), yes in the first case, you shouldn't, in the second, the I/O op will harmlessly fail as well > > + if (namelen > 20) > > + namelen = 20; > > Why do you trim down the string, won't simple strdup() work? nah, the destination is char[21], as it is the exact length of corresponding field in ATA identify response (one more for a 0 at the end) > > + memcpy(pdev->product, filenames[dev], namelen); > > + pdev->product[20] = 0; > > + > > + if (fd != -1) { > > And if "fd" is -1 ? then all defaults to an invalid device, because you failed to open the file, for whatever the reason. "agreed" to the other comments Best Regards Pavel Herrmann
Dear Pavel Herrmann, > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: > ...snip... > > > > +extern block_dev_desc_t sata_dev_desc[]; > > > + > > > +int init_sata(int dev) > > > +{ > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > > > Superfluous braces ... Actually, I think sata_dev_desc as it would work > > very well too. > > Straight copy from dwc_ahsata.c, makes it more readable thought, as the > order of operation is not very intuitive IMHO. sata_dev_desc + dev ? > > > +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; > > > > If pdev is NULL, this will crash > > well, it isn't, at least not from the command - thats why you define the > number of ports in advance, you get "dev" already range-checked Range check is fine, but will pdev be inited? It's a pointer from some array. > > > + lbaint_t retval; > > > + > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > > > + > > > + 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 retval; > > > + > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > > Besides, lseek can fail, can it not? > > If you open a pipe (or nothing), yes > in the first case, you shouldn't Shouldn't ... what? Sorry, I cannot parse this. > in the second, the I/O op will harmlessly > fail as well How so? > > > + if (namelen > 20) > > > + namelen = 20; > > > > Why do you trim down the string, won't simple strdup() work? > > nah, the destination is char[21], as it is the exact length of > corresponding field in ATA identify response (one more for a 0 at the end) I see, is it a full path ? If so, it might be a better idea to use the filename itself instead of the whole path. So you'd prevent names like "~/../foo/../.././bar.img" . > > > + memcpy(pdev->product, filenames[dev], namelen); > > > + pdev->product[20] = 0; > > > + > > > + if (fd != -1) { > > > > And if "fd" is -1 ? > > then all defaults to an invalid device, because you failed to open the > file, for whatever the reason. At least the printf below will choke, since pdev->lba is uninited > "agreed" to the other comments > > Best Regards > Pavel Herrmann > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Best regards, Marek Vasut
On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote: > Dear Pavel Herrmann, > > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: > > ...snip... > > > > > > +extern block_dev_desc_t sata_dev_desc[]; > > > > + > > > > +int init_sata(int dev) > > > > +{ > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > > > > > Superfluous braces ... Actually, I think sata_dev_desc as it would work > > > very well too. > > > > Straight copy from dwc_ahsata.c, makes it more readable thought, as the > > order of operation is not very intuitive IMHO. > > sata_dev_desc + dev ? even less intuitive > > > > +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; > > > > > > If pdev is NULL, this will crash > > > > well, it isn't, at least not from the command - thats why you define the > > number of ports in advance, you get "dev" already range-checked > > Range check is fine, but will pdev be inited? It's a pointer from some > array. init_sata is called first, so pdev is inited (see cmd_sata.c) > > > > + lbaint_t retval; > > > > + > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > > > > + > > > > + 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 retval; > > > > + > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > > > > Besides, lseek can fail, can it not? > > > > If you open a pipe (or nothing), yes > > in the first case, you shouldn't > > Shouldn't ... what? Sorry, I cannot parse this. shouldn't do that - means i agree there should be a check in case you are actively trying to break things, and use pipes/sockets as loop blocks > > in the second, the I/O op will harmlessly > > fail as well > > How so? because then the fd is -1, and read/write will do the right thing there (nothing, return -1 and set errno to EBADF) > > > > + if (namelen > 20) > > > > + namelen = 20; > > > > > > Why do you trim down the string, won't simple strdup() work? > > > > nah, the destination is char[21], as it is the exact length of > > corresponding field in ATA identify response (one more for a 0 at the end) > > I see, is it a full path ? If so, it might be a better idea to use the > filename itself instead of the whole path. So you'd prevent names like > "~/../foo/../.././bar.img" . yes, i was thinking about "...${last 17 bytes of the name}" if the name was longer, but this proved significantly simpler for demonstrating the general idea. > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > + pdev->product[20] = 0; > > > > + > > > > + if (fd != -1) { > > > > > > And if "fd" is -1 ? > > > > then all defaults to an invalid device, because you failed to open the > > file, for whatever the reason. > > At least the printf below will choke, since pdev->lba is uninited not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not doing anything we get an empty device Best Regards Pavel Herrmann
Dear Pavel Herrmann, > On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote: > > Dear Pavel Herrmann, > > > > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: > > > ...snip... > > > > > > > > +extern block_dev_desc_t sata_dev_desc[]; > > > > > + > > > > > +int init_sata(int dev) > > > > > +{ > > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > > > > > > > Superfluous braces ... Actually, I think sata_dev_desc as it would > > > > work very well too. > > > > > > Straight copy from dwc_ahsata.c, makes it more readable thought, as the > > > order of operation is not very intuitive IMHO. > > > > sata_dev_desc + dev ? > > even less intuitive Why so? > > > > > +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; > > > > > > > > If pdev is NULL, this will crash > > > > > > well, it isn't, at least not from the command - thats why you define > > > the number of ports in advance, you get "dev" already range-checked > > > > Range check is fine, but will pdev be inited? It's a pointer from some > > array. > > init_sata is called first, so pdev is inited (see cmd_sata.c) Unless it fails. Then what ? > > > > > + lbaint_t retval; > > > > > + > > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); > > > > > + > > > > > + 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 retval; > > > > > + > > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); > > > > > > > > Besides, lseek can fail, can it not? > > > > > > If you open a pipe (or nothing), yes > > > in the first case, you shouldn't > > > > Shouldn't ... what? Sorry, I cannot parse this. > > shouldn't do that - means i agree there should be a check in case you are > actively trying to break things, and use pipes/sockets as loop blocks Good > > > in the second, the I/O op will harmlessly > > > fail as well > > > > How so? > > because then the fd is -1, and read/write will do the right thing there > (nothing, return -1 and set errno to EBADF) From write(2) -->8-- RETURN VALUE On success, the number of bytes written is returned (zero indicates nothing was written). On error, -1 is returned, and errno is set appropriately. If count is zero and fd refers to a regular file, then write() may return a failure status if one of the errors below is detected. If no errors are detected, 0 will be returned without causing any other effect. If count is zero and fd refers to a file other than a regular file, the results are not specified. --8<-- I don't see the case where fd = -1 handled there at all. The last sentence resembles it, but in that case, the behavior is undefined. Can you elaborate please? > > > > > + if (namelen > 20) > > > > > + namelen = 20; > > > > > > > > Why do you trim down the string, won't simple strdup() work? > > > > > > nah, the destination is char[21], as it is the exact length of > > > corresponding field in ATA identify response (one more for a 0 at the > > > end) > > > > I see, is it a full path ? If so, it might be a better idea to use the > > filename itself instead of the whole path. So you'd prevent names like > > "~/../foo/../.././bar.img" . > > yes, i was thinking about "...${last 17 bytes of the name}" if the name was > longer, but this proved significantly simpler for demonstrating the general > idea. I think the FS code might contain some function to fixup the path and get filename from path. > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > + pdev->product[20] = 0; > > > > > + > > > > > + if (fd != -1) { > > > > > > > > And if "fd" is -1 ? > > > > > > then all defaults to an invalid device, because you failed to open the > > > file, for whatever the reason. > > > > At least the printf below will choke, since pdev->lba is uninited > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not > doing anything we get an empty device I see ... shall we also move all these memcpy() calls in to if (fd != -1) then? > Best Regards > Pavel Herrmann Best regards, Marek Vasut
On Thursday 30 August 2012 23:53:58 Marek Vasut wrote: > Dear Pavel Herrmann, > > > On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote: > > > Dear Pavel Herrmann, > > > > > > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: > > > > ...snip... > > > > > > > > > > +extern block_dev_desc_t sata_dev_desc[]; > > > > > > + > > > > > > +int init_sata(int dev) > > > > > > +{ > > > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > > > > > > > > > Superfluous braces ... Actually, I think sata_dev_desc as it would > > > > > work very well too. > > > > > > > > Straight copy from dwc_ahsata.c, makes it more readable thought, as > > > > the > > > > order of operation is not very intuitive IMHO. > > > > > > sata_dev_desc + dev ? > > > > even less intuitive > > Why so? because of the silent "*sizeof(sata_dev_desc)". I know this is standardized in C (so is the order of operands), but doing "+" on non-numbers is a little too C++ for me. I know that generated code will be eactly the same in all cases. > > > > > > +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; > > > > > > > > > > If pdev is NULL, this will crash > > > > > > > > well, it isn't, at least not from the command - thats why you define > > > > the number of ports in advance, you get "dev" already range-checked > > > > > > Range check is fine, but will pdev be inited? It's a pointer from some > > > array. > > > > init_sata is called first, so pdev is inited (see cmd_sata.c) > > Unless it fails. Then what ? the only way init can "fail" is if it gets a wrong device number (which should not happen), or if it cannot open the file, in which case it still sets pdev as -1. > > > > in the second, the I/O op will harmlessly > > > > fail as well > > > > > > How so? > > > > because then the fd is -1, and read/write will do the right thing there > > (nothing, return -1 and set errno to EBADF) > > From write(2) > > -->8-- > > RETURN VALUE > On success, the number of bytes written is returned (zero indicates > nothing was written). On error, -1 is returned, > and errno is set appropriately. > > If count is zero and fd refers to a regular file, then write() may > return a failure status if one of the errors below > is detected. If no errors are detected, 0 will be returned without > causing any other effect. If count is zero and fd > refers to a file other than a regular file, the results are not > specified. > > --8<-- > > I don't see the case where fd = -1 handled there at all. The last sentence > resembles it, but in that case, the behavior is undefined. Can you elaborate > please? RETURN VALUE ... On error, -1 is returned, and errno is set appropriately. ... ERRORS ... EBADF fd is not a valid file descriptor or is not open for writing. ... -1 is definitely not a valid file descriptor. this point is moot, as checking success of lseek (because of pipes/sockets) will filter out invalid fd as well > > > > > > + if (namelen > 20) > > > > > > + namelen = 20; > > > > > > > > > > Why do you trim down the string, won't simple strdup() work? > > > > > > > > nah, the destination is char[21], as it is the exact length of > > > > corresponding field in ATA identify response (one more for a 0 at the > > > > end) > > > > > > I see, is it a full path ? If so, it might be a better idea to use the > > > filename itself instead of the whole path. So you'd prevent names like > > > "~/../foo/../.././bar.img" . > > > > yes, i was thinking about "...${last 17 bytes of the name}" if the name > > was > > longer, but this proved significantly simpler for demonstrating the > > general > > idea. > > I think the FS code might contain some function to fixup the path and get > filename from path. that still wouldn't solve the problem, flename can still be over 20 bytes long > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > + pdev->product[20] = 0; > > > > > > + > > > > > > + if (fd != -1) { > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > then all defaults to an invalid device, because you failed to open the > > > > file, for whatever the reason. > > > > > > At least the printf below will choke, since pdev->lba is uninited > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not > > doing anything we get an empty device > > I see ... shall we also move all these memcpy() calls in to if (fd != -1) > then? I'd like to know that the device is a loopback, and what filename, not just that it failed to init Pavel Herrmann
Dear Pavel Herrmann, > On Thursday 30 August 2012 23:53:58 Marek Vasut wrote: > > Dear Pavel Herrmann, > > > > > On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote: > > > > Dear Pavel Herrmann, > > > > > > > > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote: > > > > > ...snip... > > > > > > > > > > > > +extern block_dev_desc_t sata_dev_desc[]; > > > > > > > + > > > > > > > +int init_sata(int dev) > > > > > > > +{ > > > > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); > > > > > > > > > > > > Superfluous braces ... Actually, I think sata_dev_desc as it > > > > > > would work very well too. > > > > > > > > > > Straight copy from dwc_ahsata.c, makes it more readable thought, as > > > > > the > > > > > order of operation is not very intuitive IMHO. > > > > > > > > sata_dev_desc + dev ? > > > > > > even less intuitive > > > > Why so? > > because of the silent "*sizeof(sata_dev_desc)". > I know this is standardized in C (so is the order of operands), but doing > "+" on non-numbers is a little too C++ for me. I know that generated code > will be eactly the same in all cases. It's simple increment of a pointer, normal thing. > > > > > > > +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; > > > > > > > > > > > > If pdev is NULL, this will crash > > > > > > > > > > well, it isn't, at least not from the command - thats why you > > > > > define the number of ports in advance, you get "dev" already > > > > > range-checked > > > > > > > > Range check is fine, but will pdev be inited? It's a pointer from > > > > some array. > > > > > > init_sata is called first, so pdev is inited (see cmd_sata.c) > > > > Unless it fails. Then what ? > > the only way init can "fail" is if it gets a wrong device number (which > should not happen), or if it cannot open the file, in which case it still > sets pdev as -1. If pdev is -1, then this explodes for certain, on unaligned access and on wrong ptr deref. > > > > > in the second, the I/O op will harmlessly > > > > > fail as well > > > > > > > > How so? > > > > > > because then the fd is -1, and read/write will do the right thing there > > > (nothing, return -1 and set errno to EBADF) > > > > From write(2) > > > > -->8-- > > > > RETURN VALUE > > > > On success, the number of bytes written is returned (zero > > indicates > > > > nothing was written). On error, -1 is returned, > > > > and errno is set appropriately. > > > > If count is zero and fd refers to a regular file, then write() may > > > > return a failure status if one of the errors below > > > > is detected. If no errors are detected, 0 will be returned > > without > > > > causing any other effect. If count is zero and fd > > > > refers to a file other than a regular file, the results are not > > > > specified. > > > > --8<-- > > > > I don't see the case where fd = -1 handled there at all. The last > > sentence resembles it, but in that case, the behavior is undefined. Can > > you elaborate please? > > RETURN VALUE > ... > On error, -1 is returned, and errno is set appropriately. > ... > ERRORS > ... > EBADF fd is not a valid file descriptor or is not open for writing. > ... > -1 is definitely not a valid file descriptor. I see, good catch. > this point is moot, as checking success of lseek (because of pipes/sockets) > will filter out invalid fd as well > > > > > > > > + if (namelen > 20) > > > > > > > + namelen = 20; > > > > > > > > > > > > Why do you trim down the string, won't simple strdup() work? > > > > > > > > > > nah, the destination is char[21], as it is the exact length of > > > > > corresponding field in ATA identify response (one more for a 0 at > > > > > the end) > > > > > > > > I see, is it a full path ? If so, it might be a better idea to use > > > > the filename itself instead of the whole path. So you'd prevent > > > > names like "~/../foo/../.././bar.img" . > > > > > > yes, i was thinking about "...${last 17 bytes of the name}" if the name > > > was > > > longer, but this proved significantly simpler for demonstrating the > > > general > > > idea. > > > > I think the FS code might contain some function to fixup the path and get > > filename from path. > > that still wouldn't solve the problem, flename can still be over 20 bytes > long Then pick the first 20 ... but this is really discutable > > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > > + pdev->product[20] = 0; > > > > > > > + > > > > > > > + if (fd != -1) { > > > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > > > then all defaults to an invalid device, because you failed to open > > > > > the file, for whatever the reason. > > > > > > > > At least the printf below will choke, since pdev->lba is uninited > > > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by > > > not doing anything we get an empty device > > > > I see ... shall we also move all these memcpy() calls in to if (fd != -1) > > then? > > I'd like to know that the device is a loopback, and what filename, not just > that it failed to init But are such data used somewhere further down the road? > Pavel Herrmann Best regards, Marek Vasut
On Friday 31 of August 2012 14:57:41 Marek Vasut wrote: > > > > > > > > +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; > > > > > > > > > > > > > > If pdev is NULL, this will crash > > > > > > > > > > > > well, it isn't, at least not from the command - thats why you > > > > > > define the number of ports in advance, you get "dev" already > > > > > > range-checked > > > > > > > > > > Range check is fine, but will pdev be inited? It's a pointer from > > > > > some array. > > > > > > > > init_sata is called first, so pdev is inited (see cmd_sata.c) > > > > > > Unless it fails. Then what ? > > > > the only way init can "fail" is if it gets a wrong device number (which > > should not happen), or if it cannot open the file, in which case it still > > sets pdev as -1. > > If pdev is -1, then this explodes for certain, on unaligned access and on > wrong ptr deref. again, no there is no pointer in this, pdev->priv is actually an int stored as void*, pdev itself is pointer to a global array. which one does explode in your case? > > > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > > > + pdev->product[20] = 0; > > > > > > > > + > > > > > > > > + if (fd != -1) { > > > > > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > > > > > then all defaults to an invalid device, because you failed to open > > > > > > the file, for whatever the reason. > > > > > > > > > > At least the printf below will choke, since pdev->lba is uninited > > > > > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by > > > > not doing anything we get an empty device > > > > > > I see ... shall we also move all these memcpy() calls in to if (fd != > > > -1) > > > then? > > > > I'd like to know that the device is a loopback, and what filename, not > > just > > that it failed to init > > But are such data used somewhere further down the road? yes, "sata info" for example Pavel Herrmann
Dear Pavel Herrmann, > On Friday 31 of August 2012 14:57:41 Marek Vasut wrote: > > > > > > > > > +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; > > > > > > > > > > > > > > > > If pdev is NULL, this will crash > > > > > > > > > > > > > > well, it isn't, at least not from the command - thats why you > > > > > > > define the number of ports in advance, you get "dev" already > > > > > > > range-checked > > > > > > > > > > > > Range check is fine, but will pdev be inited? It's a pointer from > > > > > > some array. > > > > > > > > > > init_sata is called first, so pdev is inited (see cmd_sata.c) > > > > > > > > Unless it fails. Then what ? > > > > > > the only way init can "fail" is if it gets a wrong device number (which > > > should not happen), or if it cannot open the file, in which case it > > > still sets pdev as -1. > > > > If pdev is -1, then this explodes for certain, on unaligned access and on > > wrong ptr deref. > > again, no > > there is no pointer in this, pdev->priv is actually an int stored as void*, > pdev itself is pointer to a global array. > which one does explode in your case? pdev, but given that the pointer is inited, I see the issue at hand now. > > > > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > > > > + pdev->product[20] = 0; > > > > > > > > > + > > > > > > > > > + if (fd != -1) { > > > > > > > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > > > > > > > then all defaults to an invalid device, because you failed to > > > > > > > open the file, for whatever the reason. > > > > > > > > > > > > At least the printf below will choke, since pdev->lba is uninited > > > > > > > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore > > > > > by not doing anything we get an empty device > > > > > > > > I see ... shall we also move all these memcpy() calls in to if (fd != > > > > -1) > > > > then? > > > > > > I'd like to know that the device is a loopback, and what filename, not > > > just > > > that it failed to init > > > > But are such data used somewhere further down the road? > > yes, "sata info" for example And how does it determine that the init failed? I think we're almost clear on these things. > Pavel Herrmann Best regards, Marek Vasut
On Friday 31 of August 2012 18:02:22 Marek Vasut wrote: > > > > > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > > > > > + pdev->product[20] = 0; > > > > > > > > > > + > > > > > > > > > > + if (fd != -1) { > > > > > > > > > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > > > > > > > > > then all defaults to an invalid device, because you failed to > > > > > > > > open the file, for whatever the reason. > > > > > > > > > > > > > > At least the printf below will choke, since pdev->lba is > > > > > > > uninited > > > > > > > > > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and therefore > > > > > > by not doing anything we get an empty device > > > > > > > > > > I see ... shall we also move all these memcpy() calls in to if (fd > > > > > != > > > > > -1) > > > > > then? > > > > > > > > I'd like to know that the device is a loopback, and what filename, not > > > > just > > > > that it failed to init > > > > > > But are such data used somewhere further down the road? > > > > yes, "sata info" for example > > And how does it determine that the init failed? given that the only thing the init does is open a file and put the decriptor to ->priv, you can tell whether it failed by looking at the descriptor (-1 means failed). the other fail-path in init is if it is given a wrong index, but that should never happen (at least not when called from cmd_sata) on a disk with 0 size you cant have a partition table, so any FS code will refuse to work, any direct operation will fail because fd is -1 or did you mean a different "it"? > I think we're almost clear on these things. thats a relief Pavel Herrmann
Dear Pavel Herrmann, > On Friday 31 of August 2012 18:02:22 Marek Vasut wrote: > > > > > > > > > > > + memcpy(pdev->product, filenames[dev], namelen); > > > > > > > > > > > + pdev->product[20] = 0; > > > > > > > > > > > + > > > > > > > > > > > + if (fd != -1) { > > > > > > > > > > > > > > > > > > > > And if "fd" is -1 ? > > > > > > > > > > > > > > > > > > then all defaults to an invalid device, because you failed > > > > > > > > > to open the file, for whatever the reason. > > > > > > > > > > > > > > > > At least the printf below will choke, since pdev->lba is > > > > > > > > uninited > > > > > > > > > > > > > > not the case. sata_dev_desc is inited in cmd_sata.c, and > > > > > > > therefore by not doing anything we get an empty device > > > > > > > > > > > > I see ... shall we also move all these memcpy() calls in to if > > > > > > (fd != > > > > > > -1) > > > > > > then? > > > > > > > > > > I'd like to know that the device is a loopback, and what filename, > > > > > not just > > > > > that it failed to init > > > > > > > > But are such data used somewhere further down the road? > > > > > > yes, "sata info" for example > > > > And how does it determine that the init failed? > > given that the only thing the init does is open a file and put the > decriptor to ->priv, you can tell whether it failed by looking at the > descriptor (-1 means failed). the other fail-path in init is if it is > given a wrong index, but that should never happen (at least not when > called from cmd_sata) > > on a disk with 0 size you cant have a partition table, so any FS code will > refuse to work, any direct operation will fail because fd is -1 > > or did you mean a different "it"? Ok, I'd like to get it reviewed by someone else and then I think it can be applied. Mike? > > I think we're almost clear on these things. > > thats a relief > > Pavel Herrmann Best regards, Marek Vasut
diff --git a/drivers/block/Makefile b/drivers/block/Makefile index f1ebdcc..5eecf37 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} += loop.o COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/block/loop.c b/drivers/block/loop.c new file mode 100644 index 0000000..c9edfc3 --- /dev/null +++ b/drivers/block/loop.c @@ -0,0 +1,107 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@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> + +static const char revision[] = "0.0"; +static const char vendor[] = "loopback"; + +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS; +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE; + +extern block_dev_desc_t sata_dev_desc[]; + +int init_sata(int dev) +{ + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); + int fd; + + if ((dev < 0) || (dev >= max_devs)) { + 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 */ + pdev->priv = (void *) (long) 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 retval; + + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt); + + 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 retval; + + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET); + retval = os_write(fd, buffer, ATA_SECT_SIZE * blkcnt); + + return retval/ATA_SECT_SIZE; +} + +int scan_sata(int dev) +{ + block_dev_desc_t *pdev = &(sata_dev_desc[dev]); + int fd = (long) pdev->priv; + int namelen; + lbaint_t bytes = 0; + memcpy(pdev->vendor, vendor, sizeof(vendor)); + memcpy(pdev->revision, revision, sizeof(revision)); + namelen = sizeof(filenames[dev]); + if (namelen > 20) + namelen = 20; + memcpy(pdev->product, filenames[dev], namelen); + pdev->product[20] = 0; + + if (fd != -1) { + pdev->type = DEV_TYPE_HARDDISK; + pdev->blksz = ATA_SECT_SIZE; + pdev->lun = 0; + + bytes = os_lseek(fd, 0, OS_SEEK_END); + pdev->lba = bytes/ATA_SECT_SIZE; + } + + printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n", + filenames[dev], bytes, pdev->lba); + + return 0; +} diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 0220386..412341f 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -93,4 +93,13 @@ "stdout=serial\0" \ "stderr=serial\0" +/* SATA loopback device */ +#define CONFIG_CMD_SATA +#define CONFIG_SATA_LOOP +#define CONFIG_SATA_LOOP_DISKS {"disk1", "disk2"} +#define CONFIG_SYS_SATA_MAX_DEVICE 2 +#define CONFIG_DOS_PARTITION +#define CONFIG_CMD_FAT +#define CONFIG_CMD_EXT2 + #endif
This driver uses files as block devices, can be used for testing disk operations on sandbox. Port count and filenames are set in board config. Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com> CC: Marek Vasut <marex@denx.de> --- drivers/block/Makefile | 1 + drivers/block/loop.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h | 9 ++++ 3 files changed, 117 insertions(+) create mode 100644 drivers/block/loop.c