Loop block device for sandbox

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 <[email protected]>

CC Marek Vasut <[email protected]>

---
 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

drivers/block/Makefile +1 -0

index f1ebdcc..5eecf37 100644
@@ -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)

drivers/block/loop.c new file +107 -0

new file mode 100644
index 0000000..c9edfc3
@@ -0,0 +1,107 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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;
+}

include/configs/sandbox.h +9 -0

index 0220386..412341f 100644
@@ -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 <[email protected]>

CC Marek Vasut <[email protected]>

---
 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

drivers/block/Makefile +0 -0

index f1ebdcc..5eecf37 100644

drivers/block/Makefile +0 -0

 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)

drivers/block/loop.c new file +31 -0

new file mode 100644
index 0000000..c9edfc3
@@ -0,0 +1,107 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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";

drivers/block/loop.c new file +9 -0

+	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) +{

drivers/block/loop.c new file +13 -0

+		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;
+}

include/configs/sandbox.h +0 -0

index 0220386..412341f 100644
@@ -93,4 +93,13 @@
 					"stdout=serial\0" \

drivers/block/loop.c line 41

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+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.

drivers/block/loop.c line 59

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

drivers/block/loop.c new file +3 -0

+{
+	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+	int fd = (long) pdev->priv;

If pdev is NULL, this will crash

Quoted 9 lines

drivers/block/loop.c new file +9 -0

+	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

drivers/block/loop.c line 62

Quoted 6 lines

*buffer) +{

drivers/block/loop.c new file +5 -0

+	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?

drivers/block/loop.c line 90

Quoted 2 lines

drivers/block/loop.c new file +2 -0

+	if (namelen > 20)
+		namelen = 20;

Why do you trim down the string, won't simple strdup() work?

drivers/block/loop.c line 94

Quoted 4 lines

drivers/block/loop.c new file +4 -0

+	memcpy(pdev->product, filenames[dev], namelen);
+	pdev->product[20] = 0;
+
+	if (fd != -1) {

And if "fd" is -1 ?

...snip...

Quoted 8 lines

drivers/block/loop.c line 41

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+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.

Quoted 7 lines

drivers/block/loop.c line 59

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

drivers/block/loop.c new file +3 -0

+{
+	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

Quoted 9 lines

Quoted 9 lines

drivers/block/loop.c new file +9 -0

+	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

Quoted 8 lines

drivers/block/loop.c line 62

Quoted 6 lines

*buffer) +{

drivers/block/loop.c new file +5 -0

+	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

Quoted 4 lines

drivers/block/loop.c line 90

Quoted 2 lines

drivers/block/loop.c new file +2 -0

+	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)

Quoted 6 lines

drivers/block/loop.c line 94

Quoted 4 lines

drivers/block/loop.c new file +4 -0

+	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
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Dear Pavel Herrmann,

Quoted 12 lines · On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:

...snip...

Quoted 8 lines

drivers/block/loop.c line 41

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+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.

Quoted 2 lines

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 ?

Quoted 8 lines

Quoted 7 lines

drivers/block/loop.c line 59

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

drivers/block/loop.c new file +3 -0

+{
+	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+	int fd = (long) pdev->priv;

If pdev is NULL, this will crash

Quoted 2 lines

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.

Quoted 18 lines

Quoted 17 lines

Quoted 10 lines

drivers/block/loop.c new file +9 -0

+	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) +{

drivers/block/loop.c line 62

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+	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?

Quoted 2 lines

If you open a pipe (or nothing), yes
in the first case, you shouldn't

Shouldn't ... what? Sorry, I cannot parse this.

Quoted 2 lines

in the second, the I/O op will harmlessly
fail as well

How so?

Quoted 7 lines

Quoted 4 lines

drivers/block/loop.c line 90

Quoted 2 lines

drivers/block/loop.c new file +2 -0

+	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" .

Quoted 9 lines

Quoted 6 lines

drivers/block/loop.c line 94

Quoted 4 lines

drivers/block/loop.c new file +4 -0

+	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

Quoted 14 lines

Dear Pavel Herrmann,

Quoted 12 lines · On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:

...snip...

Quoted 8 lines

drivers/block/loop.c line 41

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

drivers/block/loop.c line 59

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

drivers/block/loop.c new file +3 -0

+{
+	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+	int fd = (long) pdev->priv;

If pdev is NULL, this will crash

Quoted 5 lines

Quoted 2 lines

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)

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

Quoted 10 lines

drivers/block/loop.c new file +9 -0

+	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) +{

drivers/block/loop.c line 62

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+	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?

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 2 lines

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)

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

drivers/block/loop.c line 90

Quoted 2 lines

drivers/block/loop.c new file +2 -0

+	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)

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/block/loop.c line 94

Quoted 4 lines

drivers/block/loop.c new file +4 -0

+	memcpy(pdev->product, filenames[dev], namelen);
+	pdev->product[20] = 0;
+
+	if (fd != -1) {

And if "fd" is -1 ?

Quoted 4 lines

Quoted 2 lines

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,

Quoted 19 lines · On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote:

Quoted 18 lines

Dear Pavel Herrmann,

Quoted 15 lines · On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:

...snip...

Quoted 8 lines

drivers/block/loop.c line 41

Quoted 5 lines

drivers/block/loop.c new file +5 -0

+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.

Quoted 3 lines

Quoted 1 line

sata_dev_desc + dev ?

even less intuitive

Why so?

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

drivers/block/loop.c line 59

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

drivers/block/loop.c new file +3 -0

+{
+	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

Quoted 4 lines

Quoted 2 lines

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 ?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

Quoted 4 lines

drivers/block/loop.c line 90

Quoted 2 lines

drivers/block/loop.c new file +2 -0

+	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" .

Quoted 5 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

drivers/block/loop.c line 94

Quoted 4 lines

drivers/block/loop.c new file +4 -0

+	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.

Quoted 4 lines

Quoted 1 line

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?

Quoted 23 lines

Dear Pavel Herrmann,

Quoted 21 lines · On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote:

Quoted 19 lines

Dear Pavel Herrmann,

Quoted 12 lines · On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:

...snip...

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+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.

Quoted 3 lines

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 ?

Quoted 3 lines

Quoted 1 line

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.

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

Quoted 11 lines

Quoted 7 lines

Review

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

(quoted diff) +3 -0

+{
+	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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 32 lines

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 19 lines

Quoted 18 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (namelen > 20)
+		namelen = 20;

Why do you trim down the string, won't simple strdup() work?

Quoted 7 lines

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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

Quoted 5 lines

Quoted 2 lines

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

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Review

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

(quoted diff) +3 -0

+{
+	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+	int fd = (long) pdev->priv;

If pdev is NULL, this will crash

Quoted 6 lines

Quoted 3 lines

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)

Quoted 5 lines

Quoted 1 line

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.

Quoted 19 lines

Quoted 18 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	memcpy(pdev->product, filenames[dev], namelen);
+	pdev->product[20] = 0;
+
+	if (fd != -1) {

And if "fd" is -1 ?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 3 lines

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?

Quoted 23 lines

Quoted 22 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Review

Quoted 5 lines

+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)

(quoted diff) +3 -0

+{
+	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

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	memcpy(pdev->product, filenames[dev], namelen);
+	pdev->product[20] = 0;
+
+	if (fd != -1) {

And if "fd" is -1 ?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 5 lines

Quoted 3 lines

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

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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

Quoted 7 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	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

Quoted 7 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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"?

Quoted 1 line

I think we're almost clear on these things.

thats a relief

Pavel Herrmann

Signed-off-by Pavel Herrmann <[email protected]>

---
 include/configs/sandbox.h | 9 +++++++++
 1 file changed, 9 insertions(+)

include/configs/sandbox.h +0 -0

index 0220386..3126542 100644
@@ -93,4 +93,13 @@
 					"stdout=serial\0" \
 					"stderr=serial\0"
 

Dear Pavel Herrmann,

I don't understand what this patch does from the lacking description. Please add proper description to the patch. ALWAYS!

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 <[email protected]>

CC Marek Vasut <[email protected]>

---
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 | 122
++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123
insertions(+)
 create mode 100644 drivers/block/sata_loopback.c

drivers/block/Makefile +1 -0

index f1ebdcc..c95651a 100644
@@ -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)

drivers/block/sata_loopback.c new file +25 -0

new file mode 100644
index 0000000..7f25a4f
@@ -0,0 +1,122 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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>

drivers/block/sata_loopback.c new file +36 -0

+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 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) +{

drivers/block/sata_loopback.c new file +39 -0

+	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;
+}
+
+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;
+
+	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;

drivers/block/sata_loopback.c line 35

Quoted 6 lines

drivers/block/sata_loopback.c new file +6 -0

+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
+static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;

One more thing is missing -- documentation for these (add to doc/ ).

Alternatively (which would be much better), let sandbox uboot accept params and supply these as params.

Quoted 8 lines

drivers/block/sata_loopback.c line 35

Quoted 6 lines

drivers/block/sata_loopback.c new file +6 -0

+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
+static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;

One more thing is missing -- documentation for these (add to doc/ ).

which file shoud that be exactly? README.sata has nothing about configs, i cannot find any README.sandbox or README.configs

Quoted 2 lines

Alternatively (which would be much better), let sandbox uboot accept params and supply these as params.

or even make a command that would allow you to specify a filename in runtime, possibly with a dynamic number of ports.

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 <[email protected]>

CC Marek Vasut <[email protected]>

CC Mike Frysinger <[email protected]>

---
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

drivers/block/Makefile +1 -0

index f1ebdcc..c95651a 100644
@@ -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)

drivers/block/sata_loopback.c new file +32 -0

new file mode 100644
index 0000000..0e6923b
@@ -0,0 +1,200 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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>
+

drivers/block/sata_loopback.c new file +9 -0

+	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);

drivers/block/sata_loopback.c new file +24 -0

+	 */
+	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) +{

drivers/block/sata_loopback.c new file +2 -0

+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;

drivers/block/sata_loopback.c new file +44 -0

+}
+
+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]);

drivers/block/sata_loopback.c new file +7 -0

+				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);

+);

Dear Pavel Herrmann,

Quoted 7 lines

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 <[email protected]>

CC Marek Vasut <[email protected]>

CC Mike Frysinger <[email protected]>

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 ;-)

Quoted 77 lines

---

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

drivers/block/Makefile +1 -0

index f1ebdcc..c95651a 100644
@@ -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)

drivers/block/sata_loopback.c new file +36 -0

new file mode 100644
index 0000000..0e6923b
@@ -0,0 +1,200 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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];

drivers/block/sata_loopback.c line 42

Quoted 6 lines

drivers/block/sata_loopback.c new file +6 -0

+
+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.

drivers/block/sata_loopback.c line 46

Quoted 4 lines

drivers/block/sata_loopback.c new file +4 -0

+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd, old_fd;
+
+

Redundant newline

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+	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;

drivers/block/sata_loopback.c

Quoted 8 lines

drivers/block/sata_loopback.c new file +8 -0

+	}
+
+	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 

drivers/block/sata_loopback.c line 65

Quoted 1 line

drivers/block/sata_loopback.c new file +1 -0

+	 * this value before calling sata_init.

Sorry, I can't parse this.

Quoted 28 lines

drivers/block/sata_loopback.c new file +8 -0

+	 */
+	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)

drivers/block/sata_loopback.c new file +16 -0

+{
+	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) +{

drivers/block/sata_loopback.c new file +2 -0

+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;

drivers/block/sata_loopback.c line 102

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+	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?

Quoted 44 lines

drivers/block/sata_loopback.c new file +44 -0

+}
+
+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]);

drivers/block/sata_loopback.c line 156

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+	}
+
+	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.

drivers/block/sata_loopback.c line 169

Quoted 12 lines

drivers/block/sata_loopback.c new file +12 -0

+	/* 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.

drivers/block/sata_loopback.c line 174

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+		return scan_sata(dev);
+
+	case 3:
+		if (!strncmp(argv[1], "inf", 3)) {
+			dev = simple_strtoul(argv[2], NULL, 10);

Same here

drivers/block/sata_loopback.c line 181

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+			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.

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+				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);

drivers/block/sata_loopback.c line 198

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+		}
+		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 .

drivers/block/sata_loopback.c line 199

Quoted 1 line

drivers/block/sata_loopback.c new file +1 -0

+	"sata_loop load devnum file - load file from host FS into loop devnum"

sata_loop is redundant above.

Quoted 1 line

+);

Quoted 2 lines

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

Quoted 65 lines

drivers/block/sata_loopback.c line 169

Quoted 12 lines

drivers/block/sata_loopback.c new file +12 -0

+	/* 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.

drivers/block/sata_loopback.c line 174

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+		return scan_sata(dev);
+
+	case 3:
+		if (!strncmp(argv[1], "inf", 3)) {
+			dev = simple_strtoul(argv[2], NULL, 10);

Same here

drivers/block/sata_loopback.c line 181

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+			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.

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+				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);

drivers/block/sata_loopback.c line 198

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+		}
+		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 .

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into loop

devnum"

sata_loop is redundant above.

Quoted 1 line

+);

Dear Pavel Herrmann,

[...]

Quoted 5 lines

Quoted 2 lines

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

Sorry, I can't parse this ... but ...

Quoted 2 lines

, so i can safely call free() later, otherwise
this segfaults when called before sata scan

The free() function frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc(). Otherwise, or if free(ptr) has already been called before, undefined behavior occurs. If ptr is NULL, no operation is performed.

So if you call free() on null pointer, nothing happens. Where's the real problem?

Quoted 6 lines

Quoted 4 lines

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 .

wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into loop

devnum"

sata_loop is redundant above.

really? how do you figure?

Run "help sata_loop" and see the result ...

Quoted 20 lines

Dear Pavel Herrmann,

[...]

Quoted 5 lines

Quoted 2 lines

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

Sorry, I can't parse this ... but ...

Quoted 2 lines

, so i can safely call free() later, otherwise
this segfaults when called before sata scan

The free() function frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc(). Otherwise, or if free(ptr) has already been called before, undefined behavior occurs. If ptr is NULL, no operation is performed.

Quoted 2 lines

So if you call free() on null pointer, nothing happens. Where's the real problem?

if you called "sata init" before setting all the loops you would get to open(NULL) and a strlen(NULL). i think its easier to supply valid empty string then check for NULL at every access.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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 .

wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

still, i should have a "sata_loop info" work on all files, so theres a valid command with argc 2

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into loop

devnum"

sata_loop is redundant above.

really? how do you figure?

Run "help sata_loop" and see the result ...

=>help sata_loop
sata_loop - SATA loopback

Usage:
sata_loop [info] devnum - show info about loop devnum sata_loop load devnum file - load file from host FS into loop devnum

i dont see your problem

Pavel Herrmann

Dear Pavel Herrmann,

Quoted 25 lines · On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:

Quoted 23 lines

Dear Pavel Herrmann,

[...]

Quoted 5 lines

Quoted 2 lines

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

Sorry, I can't parse this ... but ...

Quoted 2 lines

, so i can safely call free() later, otherwise
this segfaults when called before sata scan

The free() function frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc().
Otherwise, or if free(ptr) has already been called before, undefined behavior occurs. If ptr is NULL, no operation is performed.

So if you call free() on null pointer, nothing happens. Where's the real problem?

Quoted 3 lines

if you called "sata init" before setting all the loops you would get to open(NULL) and a strlen(NULL). i think its easier to supply valid empty string then check for NULL at every access.

I'd say check for null on every access ... you'd lower memory requirements and the increase in code requirement should really be very minor. Also, it'd remove the "zeroed" static variable altogether, correct?

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

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 .

wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

still, i should have a "sata_loop info" work on all files, so theres a valid command with argc 2

Understood.

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into loop

devnum"

sata_loop is redundant above.

really? how do you figure?

Run "help sata_loop" and see the result ...

=>help sata_loop
sata_loop - SATA loopback

Usage:
sata_loop [info] devnum - show info about loop devnum sata_loop load devnum file - load file from host FS into loop devnum

Quoted 1 line

i dont see your problem

Ewww ... nevermind ... I just detected yet another stupid issue in the codebase. I'd say this "sata_loop" should be either omitted and filled by the macro or something. But that's for other time to solve ... so I'm fine with this now.

Quoted 27 lines

Dear Pavel Herrmann,

Quoted 25 lines · On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:

Quoted 23 lines

Dear Pavel Herrmann,

[...]

Quoted 5 lines

Quoted 2 lines

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

Sorry, I can't parse this ... but ...

Quoted 2 lines

, so i can safely call free() later, otherwise
this segfaults when called before sata scan

The free() function frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc().
Otherwise, or if free(ptr) has already been called before, undefined behavior occurs. If ptr is NULL, no operation is performed.

So if you call free() on null pointer, nothing happens. Where's the real problem?

Quoted 8 lines

Quoted 3 lines

if you called "sata init" before setting all the loops you would get to open(NULL) and a strlen(NULL). i think its easier to supply valid empty string then check for NULL at every access.

I'd say check for null on every access ... you'd lower memory requirements and the increase in code requirement should really be very minor. Also, it'd remove the "zeroed" static variable altogether, correct?

OK, fine by me

Quoted 32 lines

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

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 .

wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

still, i should have a "sata_loop info" work on all files, so theres a valid command with argc 2

Understood.

Quoted 17 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into

loop devnum"

sata_loop is redundant above.

really? how do you figure?

Run "help sata_loop" and see the result ...

=>help sata_loop
sata_loop - SATA loopback

Usage:
sata_loop [info] devnum - show info about loop devnum sata_loop load devnum file - load file from host FS into loop devnum

i dont see your problem

Quoted 4 lines

Ewww ... nevermind ... I just detected yet another stupid issue in the codebase. I'd say this "sata_loop" should be either omitted and filled by the macro or something. But that's for other time to solve ... so I'm fine with this now.

it actually is, but only to the first line (as both lines are in one string). In my opinion it should either add this for each line (with some magic string modification) or none at all, to keep it consistent

Pavel Herrmann

Dear Pavel Herrmann,

Quoted 7 lines

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 <[email protected]>

CC Marek Vasut <[email protected]>

CC Mike Frysinger <[email protected]>

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 ;-)

Quoted 77 lines

---

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

drivers/block/Makefile +1 -0

index f1ebdcc..c95651a 100644
@@ -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)

drivers/block/sata_loopback.c new file +36 -0

new file mode 100644
index 0000000..0e6923b
@@ -0,0 +1,200 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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];

drivers/block/sata_loopback.c line 42

Quoted 6 lines

drivers/block/sata_loopback.c new file +6 -0

+
+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.

Quoted 42 lines

drivers/block/sata_loopback.c new file +42 -0

+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]);

drivers/block/sata_loopback.c line 156

Quoted 10 lines

drivers/block/sata_loopback.c new file +10 -0

+	}
+
+	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.

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+	/* make sure we have valid filenames */
+	if (!zeroed) {
+		init_sata(0);
+		zeroed = 1;
+	}
+
+	switch (argc) {

drivers/block/sata_loopback.c line 169

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+	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.

drivers/block/sata_loopback.c line 174

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+		return scan_sata(dev);
+
+	case 3:
+		if (!strncmp(argv[1], "inf", 3)) {
+			dev = simple_strtoul(argv[2], NULL, 10);

Same here

drivers/block/sata_loopback.c line 181

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+			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.

Quoted 13 lines

drivers/block/sata_loopback.c new file +13 -0

+				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;
+}
+

drivers/block/sata_loopback.c line 198

Quoted 4 lines

drivers/block/sata_loopback.c new file +4 -0

+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 .

drivers/block/sata_loopback.c line 199

Quoted 1 line

drivers/block/sata_loopback.c new file +1 -0

+	"sata_loop load devnum file - load file from host FS into loop devnum"

sata_loop is redundant above.

Quoted 5 lines

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

Quoted 7 lines

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+	/* make sure we have valid filenames */
+	if (!zeroed) {
+		init_sata(0);
+		zeroed = 1;
+	}
+
+	switch (argc) {

Quoted 8 lines

drivers/block/sata_loopback.c line 169

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+	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.

hint - scan_sata does the range check in this codepath

Quoted 7 lines

drivers/block/sata_loopback.c line 174

Quoted 5 lines

drivers/block/sata_loopback.c new file +5 -0

+		return scan_sata(dev);
+
+	case 3:
+		if (!strncmp(argv[1], "inf", 3)) {
+			dev = simple_strtoul(argv[2], NULL, 10);

Same here

see above

Quoted 26 lines

drivers/block/sata_loopback.c line 181

Quoted 7 lines

drivers/block/sata_loopback.c new file +7 -0

+			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.

Quoted 13 lines

drivers/block/sata_loopback.c new file +13 -0

+				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;
+}
+

Quoted 8 lines

drivers/block/sata_loopback.c line 198

Quoted 4 lines

drivers/block/sata_loopback.c new file +4 -0

+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 .

wont help, still need argc 3 or 4

Quoted 4 lines

Quoted 2 lines

(quoted diff) +1 -0

+	"sata_loop load devnum file - load file from host FS into loop

devnum"

sata_loop is redundant above.

really? how do you figure?

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 <[email protected]>

CC Marek Vasut <[email protected]>

CC Mike Frysinger <[email protected]>

---
Changes for v4:
checkpatch fixes
use NULLs instead of ""
extend sata_loop command

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 | 212
++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h     |
  8 ++
 3 files changed, 221 insertions(+)
 create mode 100644 drivers/block/sata_loopback.c

drivers/block/Makefile +1 -0

index f1ebdcc..c95651a 100644
@@ -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)

drivers/block/sata_loopback.c new file +87 -0

new file mode 100644
index 0000000..600fbdc
@@ -0,0 +1,212 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <[email protected]>
+ *
+ * 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";
+
+/* this will be auto-initialized to NULLs */
+static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+extern block_dev_desc_t sata_dev_desc[];
+
+static inline int range_check(int dev)
+{
+	return  (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
+}
+
+int init_sata(int dev)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd, old_fd;
+
+	if (range_check(dev)) {
+		printf("File index %d is out of range.\n", dev);
+		return -EINVAL;
+	}
+
+	if (filenames[dev])
+		fd = os_open(filenames[dev], OS_O_RDWR);
+	else
+		fd = -1;
+
+	old_fd = (long) pdev->priv;
+	/* This is ugly, but saves allocation for 1 int. */
+	pdev->priv = (void *) (long) fd;
+
+	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) +{

drivers/block/sata_loopback.c new file +124 -0

+	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;
+}
+
+int scan_sata(int dev)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	int namelen;
+	char *filename;
+	lbaint_t bytes = 0;
+
+	if (range_check(dev)) {
+		printf("File index %d is out of range.\n", dev);
+		return -EINVAL;
+	}
+
+	if (filenames[dev])
+		filename = filenames[dev];
+	else
+		filename = "";
+
+	memcpy(pdev->vendor, vendor, sizeof(vendor));
+	memcpy(pdev->revision, revision, sizeof(revision));
+	namelen = strlen(filename);
+
+	if (namelen > 20) {
+		/* take the last 17 chars, prepend them with "..." */
+		memcpy(pdev->product, "...", 3);
+		memcpy(pdev->product+3, filename + (namelen - 17), 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, filename, 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, filename);
+	}
+
+	return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int dev = 0;
+
+	switch (argc) {
+	case 0:
+	case 1:
+		return CMD_RET_USAGE;
+	case 2:
+		if (!strncmp(argv[1], "inf", 3)) {
+			for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
+				scan_sata(dev);
+			return 0;
+		}
+		return CMD_RET_USAGE;
+	case 3:
+		if (!strncmp(argv[1], "inf", 3)) {
+			dev = simple_strtoul(argv[2], NULL, 10);
+			return scan_sata(dev);
+		}
+		return CMD_RET_USAGE;
+	case 4:
+		if (!strncmp(argv[1], "load", 4)) {
+			dev = simple_strtoul(argv[2], NULL, 10);
+			/*
+			 * init_sata() and scan_sata() do their own range
+			 * check, however we need to explicitly do it here
+			 * as well.
+			 */
+			if (range_check(dev)) {
+				printf("File index %d is out of range.\n", dev);
+				return -EINVAL;
+			}
+			free(filenames[dev]);
+			filenames[dev] = strdup(argv[3]);
+			init_sata(dev);
+			scan_sata(dev);
+			/*
+			 * Scan the partition table if we succeeded in loading
+			 * the new loop file.
+			 */
+			if (sata_dev_desc[dev].lba > 0)
+				init_part(&sata_dev_desc[dev]);
+
+			return 0;
+		}
+		return CMD_RET_USAGE;
+	}
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	sata_loop, 4, 1, do_loop,
+	"SATA loopback",
+	"info - show info about all loop devices\n"
+	"sata_loop info devnum - show info about loop devnum\n"
+	"sata_loop load devnum file - load file from host FS into loop devnum"
+);

include/configs/sandbox.h +8 -0

index 0220386..a713430 100644
@@ -93,4 +93,12 @@
 					"stdout=serial\0" \
 					"stderr=serial\0"
 
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SYS_SATA_MAX_DEVICE 2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
 #endif