[U-Boot] [PATCH] Loop block device for sandbox

[U-Boot] [PATCH] Loop block device for sandbox

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

[U-Boot] [PATCH] Loop block device for sandbox

[Pavel Herrmann]

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

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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

[U-Boot] [PATCH] Loop block device for sandbox

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

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

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut

[U-Boot] [PATCH] Loop block device for sandbox

[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

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

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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

[U-Boot] [PATCH] Loop block device for sandbox

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


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

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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

[U-Boot] [PATCH] Loop block device for sandbox

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

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

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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

[U-Boot] [PATCH] Loop block device for sandbox

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

> I think we're almost clear on these things.

thats a relief
 
Pavel Herrmann

[U-Boot] [PATCH] Loop block device for sandbox

[Marek Vasut]

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

[U-Boot] [PATCH v2 1/2] Loop block device for sandbox

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

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
 COBJS-$(CONFIG_IDE_SIL680) += sil680.o
 COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
 COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..7f25a4f
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,122 @@
+/*
+ * (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[] = "SATA 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 start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_read(fd, buffer, length_byte);
+
+	return retval/ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	lbaint_t start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_write(fd, buffer, length_byte);
+
+	return retval/ATA_SECT_SIZE;
+}
+
+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;
+		pdev->blksz = ATA_SECT_SIZE;
+		pdev->lun = 0;
+		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;
+}
-- 
1.7.12

[U-Boot] [PATCH 2/2] Use loop block device in sandbox board

[Pavel Herrmann]

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
 include/configs/sandbox.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..3126542 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", "obscenelylongfilename", "disk3"}
+#define CONFIG_SYS_SATA_MAX_DEVICE 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
 #endif
-- 
1.7.12

[U-Boot] [PATCH 2/2] Use loop block device in sandbox board

[Marek Vasut]

Dear Pavel Herrmann,

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

> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> ---
>  include/configs/sandbox.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 0220386..3126542 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", "obscenelylongfilename", "disk3"}
> +#define CONFIG_SYS_SATA_MAX_DEVICE 3
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_CMD_FAT
> +#define CONFIG_CMD_EXT2
> +
>  #endif

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 1/2] Loop block device for sandbox

[Marek Vasut]

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>
> ---
> 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
> 
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
>  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> 
>  COBJS	:= $(COBJS-y)
>  SRCS	:= $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..7f25a4f
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,122 @@
> +/*
> + * (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[] = "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.

> +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) +{
> +	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;
> +		pdev->blksz = ATA_SECT_SIZE;
> +		pdev->lun = 0;
> +		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;
> +}

Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/2] Use loop block device in sandbox board

[Pavel Herrmann]

Enable SATA_LOOP and a few disk-related commands for sandbox

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
changes for v2:
  add a few words of description

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

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..3126542 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", "obscenelylongfilename", "disk3"}
+#define CONFIG_SYS_SATA_MAX_DEVICE 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
 #endif
-- 
1.7.12

[U-Boot] [PATCH 2/2] Use loop block device in sandbox board

[Pavel Herrmann]

Hi

On Saturday 01 of September 2012 16:20:12 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> I don't understand what this patch does from the lacking description. Please
> add proper description to the patch. ALWAYS!

see $title perhaps? there is not much more to say.

v2 on the way

Pavel Herrmann

[U-Boot] [PATCH v2 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Monday 03 of September 2012 18:49:00 Marek Vasut wrote:
> > +
> > +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

> 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

[U-Boot] [PATCH v2 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Monday 03 of September 2012 18:49:00 Marek Vasut wrote:
> > > +
> > > +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

Create one ... especially for the newly added feature -- eg. 
README.sata_loopback or something,

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

Good idea, indeed.

> Pavel Herrmann

Best regards,
Marek Vasut

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Pavel Herrmann]

This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
CC: Mike Frysinger <vapier@gentoo.org>
---
Changes for v3:
  introduce sata_loop command

Changes for v2:
  split sandbox config off into separate patch (2/2)
  rename file to signify exported API
  style fixes
  show end of long filenames rather than beginning
  check for lseek errors to indicate non-regular file

 drivers/block/Makefile        |   1 +
 drivers/block/sata_loopback.c | 200 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+)
 create mode 100644 drivers/block/sata_loopback.c

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
 COBJS-$(CONFIG_IDE_SIL680) += sil680.o
 COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
 COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..0e6923b
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,200 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.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>
+#include <command.h>
+#include <malloc.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+extern block_dev_desc_t sata_dev_desc[];
+
+int init_sata(int dev)
+{
+	static int zeroed = 0;
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd, old_fd;
+
+
+	if (!zeroed) {
+		int i;
+		for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
+			filenames[i]=strdup("");
+		zeroed = 1;
+	}
+
+	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+		printf("File index %d is out of range.\n", dev);
+		return -EINVAL;
+	}
+
+	fd = os_open(filenames[dev], OS_O_RDWR);
+	/* This is ugly, but saves allocation for 1 int. */
+	old_fd = (long) pdev->priv;
+	pdev->priv = (void *) (long) fd;
+	/*
+	 * sadly we cannot set -1 to all as above, because "sata init" will zero
+	 * this value before calling sata_init.
+	 */
+	if ((old_fd > 2) || (old_fd < 0))
+		os_close(old_fd);
+
+	return 0;
+}
+
+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	lbaint_t start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_read(fd, buffer, length_byte);
+
+	return retval/ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	lbaint_t start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_write(fd, buffer, length_byte);
+
+	return retval/ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	int namelen;
+	const char *filename = filenames[dev];
+	lbaint_t bytes = 0;
+
+	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+		printf("File index %d is out of range.\n", dev);
+		return -EINVAL;
+	}
+
+	memcpy(pdev->vendor, vendor, sizeof(vendor));
+	memcpy(pdev->revision, revision, sizeof(revision));
+	namelen = strlen(filenames[dev]);
+
+	if (namelen > 20) {
+		/* take the last 17 chars, prepend them with "..." */
+		filename += (namelen - 17);
+		memcpy(pdev->product, "...", 3);
+		memcpy(pdev->product+3, filename, 17);
+	} else
+		memcpy(pdev->product, filename, namelen);
+
+	pdev->product[20] = 0;
+
+	bytes = os_lseek(fd, 0, OS_SEEK_END);
+	if (bytes != -1) {
+		pdev->type = DEV_TYPE_HARDDISK;
+		pdev->blksz = ATA_SECT_SIZE;
+		pdev->lun = 0;
+		pdev->lba = bytes/ATA_SECT_SIZE;
+		printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
+			" %lu\n", dev, filenames[dev], bytes, pdev->lba);
+	} else {
+		pdev->type = DEV_TYPE_HARDDISK;
+		pdev->blksz = ATA_SECT_SIZE;
+		pdev->lun = 0;
+		pdev->lba = 0;
+		printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
+			 dev, filenames[dev]);
+	}
+
+	return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int dev = 0;
+	static int zeroed = 0;
+
+	/* 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);
+		return scan_sata(dev);
+
+	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);
+			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+				printf("File index %d is out of range.\n", dev);
+				return -EINVAL;
+			}
+			free(filenames[dev]);
+			filenames[dev] = strdup(argv[3]);
+			init_sata(dev);
+			return scan_sata(dev);
+		}
+		return CMD_RET_USAGE;
+	}
+	return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+	sata_loop, 4, 1, do_loop,
+	"SATA loopback",
+	"[info] devnum - show info about loop devnum\n"
+	"sata_loop load devnum file - load file from host FS into loop devnum"
+);
-- 
1.7.12

[U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board

[Pavel Herrmann]

Enable SATA_LOOP and a few disk-related commands for sandbox

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v3:
  drop static names for loop devices

Changes for v2:
  add a few words of description

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

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..c238feb 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -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 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
 #endif
-- 
1.7.12

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.
> 
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> CC: Mike Frysinger <vapier@gentoo.org>

ERROR: "foo * bar" should be "foo *bar"
#136: FILE: drivers/block/sata_loopback.c:36:
+static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];

WARNING: externs should be avoided in .c files
#138: FILE: drivers/block/sata_loopback.c:38:
+extern block_dev_desc_t sata_dev_desc[];

ERROR: do not initialise statics to 0 or NULL
#142: FILE: drivers/block/sata_loopback.c:42:
+       static int zeroed = 0;

ERROR: spaces required around that '=' (ctx:VxV)
#150: FILE: drivers/block/sata_loopback.c:50:
+                       filenames[i]=strdup("");
                                    ^

ERROR: space prohibited after that open parenthesis '('
#181: FILE: drivers/block/sata_loopback.c:81:
+       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)

ERROR: space prohibited after that open parenthesis '('
#197: FILE: drivers/block/sata_loopback.c:97:
+       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)

ERROR: do not initialise statics to 0 or NULL
#256: FILE: drivers/block/sata_loopback.c:156:
+       static int zeroed = 0;

total: 6 errors, 1 warnings, 207 lines checked

Checkpatch and I never sleep, don't even try ;-)

> ---
> Changes for v3:
>   introduce sata_loop command
> 
> Changes for v2:
>   split sandbox config off into separate patch (2/2)
>   rename file to signify exported API
>   style fixes
>   show end of long filenames rather than beginning
>   check for lseek errors to indicate non-regular file
> 
>  drivers/block/Makefile        |   1 +
>  drivers/block/sata_loopback.c | 200
> ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> insertions(+)
>  create mode 100644 drivers/block/sata_loopback.c
> 
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
>  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> 
>  COBJS	:= $(COBJS-y)
>  SRCS	:= $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..0e6923b
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,200 @@
> +/*
> + * (C) Copyright 2012
> + * Pavel Herrmann <morpheus.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>
> +#include <command.h>
> +#include <malloc.h>
> +
> +static const char revision[] = "0.0";
> +static const char vendor[] = "SATA loopback";
> +
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> +
> +extern block_dev_desc_t sata_dev_desc[];
> +
> +int init_sata(int dev)
> +{
> +	static int zeroed = 0;

Is this really needed here and below? Pull this crap out.

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

Redundant newline

> +	if (!zeroed) {
> +		int i;
> +		for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
> +			filenames[i]=strdup("");
> +		zeroed = 1;
> +	}
> +
> +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> +		printf("File index %d is out of range.\n", dev);
> +		return -EINVAL;
> +	}
> +
> +	fd = os_open(filenames[dev], OS_O_RDWR);
> +	/* This is ugly, but saves allocation for 1 int. */
> +	old_fd = (long) pdev->priv;
> +	pdev->priv = (void *) (long) fd;
> +	/*
> +	 * sadly we cannot set -1 to all as above, because "sata init" will zero
> +	 * this value before calling sata_init.

Sorry, I can't parse this.

> +	 */
> +	if ((old_fd > 2) || (old_fd < 0))
> +		os_close(old_fd);
> +
> +	return 0;
> +}
> +
> +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
> +{
> +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> +	int fd = (long) pdev->priv;
> +	lbaint_t start_byte = ATA_SECT_SIZE * start;
> +	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> +	lbaint_t retval;
> +
> +	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> +		return -1;
> +
> +	retval = os_read(fd, buffer, length_byte);
> +
> +	return retval/ATA_SECT_SIZE;
> +}
> +
> +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> *buffer) +{
> +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> +	int fd = (long) pdev->priv;
> +	lbaint_t start_byte = ATA_SECT_SIZE * start;
> +	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> +	lbaint_t retval;
> +
> +	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> +		return -1;
> +
> +	retval = os_write(fd, buffer, length_byte);
> +
> +	return retval/ATA_SECT_SIZE;

a[space]operator[space]b ... did you ignore all my previous comments?

> +}
> +
> +int scan_sata(int dev)
> +{
> +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> +	int fd = (long) pdev->priv;
> +	int namelen;
> +	const char *filename = filenames[dev];
> +	lbaint_t bytes = 0;
> +
> +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> +		printf("File index %d is out of range.\n", dev);
> +		return -EINVAL;
> +	}
> +
> +	memcpy(pdev->vendor, vendor, sizeof(vendor));
> +	memcpy(pdev->revision, revision, sizeof(revision));
> +	namelen = strlen(filenames[dev]);
> +
> +	if (namelen > 20) {
> +		/* take the last 17 chars, prepend them with "..." */
> +		filename += (namelen - 17);
> +		memcpy(pdev->product, "...", 3);
> +		memcpy(pdev->product+3, filename, 17);
> +	} else
> +		memcpy(pdev->product, filename, namelen);
> +
> +	pdev->product[20] = 0;
> +
> +	bytes = os_lseek(fd, 0, OS_SEEK_END);
> +	if (bytes != -1) {
> +		pdev->type = DEV_TYPE_HARDDISK;
> +		pdev->blksz = ATA_SECT_SIZE;
> +		pdev->lun = 0;
> +		pdev->lba = bytes/ATA_SECT_SIZE;
> +		printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> +			" %lu\n", dev, filenames[dev], bytes, pdev->lba);
> +	} else {
> +		pdev->type = DEV_TYPE_HARDDISK;
> +		pdev->blksz = ATA_SECT_SIZE;
> +		pdev->lun = 0;
> +		pdev->lba = 0;
> +		printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> +			 dev, filenames[dev]);
> +	}
> +
> +	return 0;
> +}
> +
> +
> +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int dev = 0;
> +	static int zeroed = 0;

move this out.

besides, I think it'd be much systematic to just scream at user to call "sata 
rescan" and bail out instead of doing it for him.

> +	/* make sure we have valid filenames */
> +	if (!zeroed) {
> +		init_sata(0);
> +		zeroed = 1;
> +	}
> +
> +	switch (argc) {
> +	case 0:
> +	case 1:
> +		return CMD_RET_USAGE;
> +	case 2:
> +		dev = simple_strtoul(argv[1], NULL, 10);

Ok, so if I run this command and ask for device 0xb00bf33d ... will this 
survive? Hint: it won't, rangecheck missing.

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

Same here

> +			return scan_sata(dev);
> +		}
> +		return CMD_RET_USAGE;
> +	case 4:
> +		if (!strncmp(argv[1], "load", 4)) {
> +			dev = simple_strtoul(argv[2], NULL, 10);
> +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {

And here you have it ?

Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into 
lightweight static inline function.

> +				printf("File index %d is out of range.\n", dev);
> +				return -EINVAL;
> +			}
> +			free(filenames[dev]);
> +			filenames[dev] = strdup(argv[3]);
> +			init_sata(dev);
> +			return scan_sata(dev);
> +		}
> +		return CMD_RET_USAGE;
> +	}
> +	return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> +	sata_loop, 4, 1, do_loop,
> +	"SATA loopback",
> +	"[info] devnum - show info about loop devnum\n"

Make this "info" part mandatory. Than you can cut the whole argc loop into 
simple "if argc != 2 ; then fail" . And do simple checking for the first letter 
of the argument being either i or d .

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

sata_loop is redundant above.

> +);

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> > 
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
> 
> ERROR: "foo * bar" should be "foo *bar"
> #136: FILE: drivers/block/sata_loopback.c:36:
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> 
> WARNING: externs should be avoided in .c files
> #138: FILE: drivers/block/sata_loopback.c:38:
> +extern block_dev_desc_t sata_dev_desc[];
> 
> ERROR: do not initialise statics to 0 or NULL
> #142: FILE: drivers/block/sata_loopback.c:42:
> +       static int zeroed = 0;
> 
> ERROR: spaces required around that '=' (ctx:VxV)
> #150: FILE: drivers/block/sata_loopback.c:50:
> +                       filenames[i]=strdup("");
>                                     ^
> 
> ERROR: space prohibited after that open parenthesis '('
> #181: FILE: drivers/block/sata_loopback.c:81:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: space prohibited after that open parenthesis '('
> #197: FILE: drivers/block/sata_loopback.c:97:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: do not initialise statics to 0 or NULL
> #256: FILE: drivers/block/sata_loopback.c:156:
> +       static int zeroed = 0;
> 
> total: 6 errors, 1 warnings, 207 lines checked
> 
> Checkpatch and I never sleep, don't even try ;-)
> 

sorry, my bad, i thought i had it on pre-commit

> > ---
> > 
> > Changes for v3:
> >   introduce sata_loop command
> > 
> > Changes for v2:
> >   split sandbox config off into separate patch (2/2)
> >   rename file to signify exported API
> >   style fixes
> >   show end of long filenames rather than beginning
> >   check for lseek errors to indicate non-regular file
> >  
> >  drivers/block/Makefile        |   1 +
> >  drivers/block/sata_loopback.c | 200
> > 
> > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> > insertions(+)
> > 
> >  create mode 100644 drivers/block/sata_loopback.c
> > 
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > 
> >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > 
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > 
> >  COBJS	:= $(COBJS-y)
> >  SRCS	:= $(COBJS:.o=.c)
> > 
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..0e6923b
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.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>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +int init_sata(int dev)
> > +{
> > +	static int zeroed = 0;
> 
> Is this really needed here and below? Pull this crap out.

yes, because you dont know whetehr this gets called first from sata_loop or 
from "sata init"

> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd, old_fd;
> > +
> > +
> 
> Redundant newline
> 
> > +	if (!zeroed) {
> > +		int i;
> > +		for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
> > +			filenames[i]=strdup("");
> > +		zeroed = 1;
> > +	}
> > +
> > +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > +		printf("File index %d is out of range.\n", dev);
> > +		return -EINVAL;
> > +	}
> > +
> > +	fd = os_open(filenames[dev], OS_O_RDWR);
> > +	/* This is ugly, but saves allocation for 1 int. */
> > +	old_fd = (long) pdev->priv;
> > +	pdev->priv = (void *) (long) fd;
> > +	/*
> > +	 * sadly we cannot set -1 to all as above, because "sata init" will 
zero
> > +	 * this value before calling sata_init.
> 
> Sorry, I can't parse this.
> 
> > +	 */
> > +	if ((old_fd > 2) || (old_fd < 0))
> > +		os_close(old_fd);
> > +
> > +	return 0;
> > +}
> > +
> > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer)
> > +{
> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd = (long) pdev->priv;
> > +	lbaint_t start_byte = ATA_SECT_SIZE * start;
> > +	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > +	lbaint_t retval;
> > +
> > +	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > +		return -1;
> > +
> > +	retval = os_read(fd, buffer, length_byte);
> > +
> > +	return retval/ATA_SECT_SIZE;
> > +}
> > +
> > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer) +{
> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd = (long) pdev->priv;
> > +	lbaint_t start_byte = ATA_SECT_SIZE * start;
> > +	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > +	lbaint_t retval;
> > +
> > +	if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > +		return -1;
> > +
> > +	retval = os_write(fd, buffer, length_byte);
> > +
> > +	return retval/ATA_SECT_SIZE;
> 
> a[space]operator[space]b ... did you ignore all my previous comments?
> 
> > +}
> > +
> > +int scan_sata(int dev)
> > +{
> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd = (long) pdev->priv;
> > +	int namelen;
> > +	const char *filename = filenames[dev];
> > +	lbaint_t bytes = 0;
> > +
> > +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > +		printf("File index %d is out of range.\n", dev);
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(pdev->vendor, vendor, sizeof(vendor));
> > +	memcpy(pdev->revision, revision, sizeof(revision));
> > +	namelen = strlen(filenames[dev]);
> > +
> > +	if (namelen > 20) {
> > +		/* take the last 17 chars, prepend them with "..." */
> > +		filename += (namelen - 17);
> > +		memcpy(pdev->product, "...", 3);
> > +		memcpy(pdev->product+3, filename, 17);
> > +	} else
> > +		memcpy(pdev->product, filename, namelen);
> > +
> > +	pdev->product[20] = 0;
> > +
> > +	bytes = os_lseek(fd, 0, OS_SEEK_END);
> > +	if (bytes != -1) {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = bytes/ATA_SECT_SIZE;
> > +		printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > +			" %lu\n", dev, filenames[dev], bytes, pdev->lba);
> > +	} else {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = 0;
> > +		printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > +			 dev, filenames[dev]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	int dev = 0;
> > +	static int zeroed = 0;
> 
> move this out.
> 
> besides, I think it'd be much systematic to just scream at user to call
> "sata rescan" and bail out instead of doing it for him.

i dont actually need a sata rescan, i just need to make sure i have 
dynamically allocated names, so i can safely call free() later, otherwise this 
segfaults when called before sata scan

> > +	/* make sure we have valid filenames */
> > +	if (!zeroed) {
> > +		init_sata(0);
> > +		zeroed = 1;
> > +	}
> > +
> > +	switch (argc) {
> > +	case 0:
> > +	case 1:
> > +		return CMD_RET_USAGE;
> > +	case 2:
> > +		dev = simple_strtoul(argv[1], NULL, 10);
> 
> Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> survive? Hint: it won't, rangecheck missing.
> 
> > +		return scan_sata(dev);
> > +
> > +	case 3:
> > +		if (!strncmp(argv[1], "inf", 3)) {
> > +			dev = simple_strtoul(argv[2], NULL, 10);
> 
> Same here
> 
> > +			return scan_sata(dev);
> > +		}
> > +		return CMD_RET_USAGE;
> > +	case 4:
> > +		if (!strncmp(argv[1], "load", 4)) {
> > +			dev = simple_strtoul(argv[2], NULL, 10);
> > +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> 
> And here you have it ?
> 
> Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into
> lightweight static inline function.
> 
> > +				printf("File index %d is out of range.\n", dev);
> > +				return -EINVAL;
> > +			}
> > +			free(filenames[dev]);
> > +			filenames[dev] = strdup(argv[3]);
> > +			init_sata(dev);
> > +			return scan_sata(dev);
> > +		}
> > +		return CMD_RET_USAGE;
> > +	}
> > +	return CMD_RET_USAGE;
> > +}
> > +
> > +U_BOOT_CMD(
> > +	sata_loop, 4, 1, do_loop,
> > +	"SATA loopback",
> > +	"[info] devnum - show info about loop devnum\n"
> 
> Make this "info" part mandatory. Than you can cut the whole argc loop into
> simple "if argc != 2 ; then fail" . And do simple checking for the first
> letter of the argument being either i or d .
> 
> > +	"sata_loop load devnum file - load file from host FS into loop devnum"
> 
> sata_loop is redundant above.
> 
> > +);

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Pavel Herrmann]

second try...

On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> > 
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
> 
> ERROR: "foo * bar" should be "foo *bar"
> #136: FILE: drivers/block/sata_loopback.c:36:
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> 
> WARNING: externs should be avoided in .c files
> #138: FILE: drivers/block/sata_loopback.c:38:
> +extern block_dev_desc_t sata_dev_desc[];
> 
> ERROR: do not initialise statics to 0 or NULL
> #142: FILE: drivers/block/sata_loopback.c:42:
> +       static int zeroed = 0;
> 
> ERROR: spaces required around that '=' (ctx:VxV)
> #150: FILE: drivers/block/sata_loopback.c:50:
> +                       filenames[i]=strdup("");
>                                     ^
> 
> ERROR: space prohibited after that open parenthesis '('
> #181: FILE: drivers/block/sata_loopback.c:81:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: space prohibited after that open parenthesis '('
> #197: FILE: drivers/block/sata_loopback.c:97:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: do not initialise statics to 0 or NULL
> #256: FILE: drivers/block/sata_loopback.c:156:
> +       static int zeroed = 0;
> 
> total: 6 errors, 1 warnings, 207 lines checked
> 
> Checkpatch and I never sleep, don't even try ;-)
> 

sorry, my bad, i thought i had it on pre-commit

> > ---
> > 
> > Changes for v3:
> >   introduce sata_loop command
> > 
> > Changes for v2:
> >   split sandbox config off into separate patch (2/2)
> >   rename file to signify exported API
> >   style fixes
> >   show end of long filenames rather than beginning
> >   check for lseek errors to indicate non-regular file
> >  
> >  drivers/block/Makefile        |   1 +
> >  drivers/block/sata_loopback.c | 200
> > 
> > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> > insertions(+)
> > 
> >  create mode 100644 drivers/block/sata_loopback.c
> > 
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > 
> >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > 
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > 
> >  COBJS	:= $(COBJS-y)
> >  SRCS	:= $(COBJS:.o=.c)
> > 
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..0e6923b
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.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>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +int init_sata(int dev)
> > +{
> > +	static int zeroed = 0;
> 
> Is this really needed here and below? Pull this crap out.

yes, because you dont know whether this gets called first from sata_loop or 
from "sata init"

> > +int scan_sata(int dev)
> > +{
> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd = (long) pdev->priv;
> > +	int namelen;
> > +	const char *filename = filenames[dev];
> > +	lbaint_t bytes = 0;
> > +
> > +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > +		printf("File index %d is out of range.\n", dev);
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(pdev->vendor, vendor, sizeof(vendor));
> > +	memcpy(pdev->revision, revision, sizeof(revision));
> > +	namelen = strlen(filenames[dev]);
> > +
> > +	if (namelen > 20) {
> > +		/* take the last 17 chars, prepend them with "..." */
> > +		filename += (namelen - 17);
> > +		memcpy(pdev->product, "...", 3);
> > +		memcpy(pdev->product+3, filename, 17);
> > +	} else
> > +		memcpy(pdev->product, filename, namelen);
> > +
> > +	pdev->product[20] = 0;
> > +
> > +	bytes = os_lseek(fd, 0, OS_SEEK_END);
> > +	if (bytes != -1) {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = bytes/ATA_SECT_SIZE;
> > +		printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > +			" %lu\n", dev, filenames[dev], bytes, pdev->lba);
> > +	} else {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = 0;
> > +		printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > +			 dev, filenames[dev]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	int dev = 0;
> > +	static int zeroed = 0;
> 
> move this out.
> 
> besides, I think it'd be much systematic to just scream at user to call
> "sata rescan" and bail out instead of doing it for him.

i dont actually need a sata rescan, i just need to make sure i have 
dynamically allocated names, so i can safely call free() later, otherwise this 
segfaults when called before sata scan

> > +	/* make sure we have valid filenames */
> > +	if (!zeroed) {
> > +		init_sata(0);
> > +		zeroed = 1;
> > +	}
> > +
> > +	switch (argc) {
> > +	case 0:
> > +	case 1:
> > +		return CMD_RET_USAGE;
> > +	case 2:
> > +		dev = simple_strtoul(argv[1], NULL, 10);
> 
> Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> survive? Hint: it won't, rangecheck missing.

hint - scan_sata does the range check in this codepath

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

see above

> > +			return scan_sata(dev);
> > +		}
> > +		return CMD_RET_USAGE;
> > +	case 4:
> > +		if (!strncmp(argv[1], "load", 4)) {
> > +			dev = simple_strtoul(argv[2], NULL, 10);
> > +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> 
> And here you have it ?
> 
> Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into
> lightweight static inline function.
> 
> > +				printf("File index %d is out of range.\n", dev);
> > +				return -EINVAL;
> > +			}
> > +			free(filenames[dev]);
> > +			filenames[dev] = strdup(argv[3]);
> > +			init_sata(dev);
> > +			return scan_sata(dev);
> > +		}
> > +		return CMD_RET_USAGE;
> > +	}
> > +	return CMD_RET_USAGE;
> > +}
> > +
> > +U_BOOT_CMD(
> > +	sata_loop, 4, 1, do_loop,
> > +	"SATA loopback",
> > +	"[info] devnum - show info about loop devnum\n"
> 
> Make this "info" part mandatory. Than you can cut the whole argc loop into
> simple "if argc != 2 ; then fail" . And do simple checking for the first
> letter of the argument being either i or d .

wont help, still need argc 3 or 4

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

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

[...]

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

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

> > > +	/* make sure we have valid filenames */
> > > +	if (!zeroed) {
> > > +		init_sata(0);
> > > +		zeroed = 1;
> > > +	}
> > > +
> > > +	switch (argc) {
> > > +	case 0:
> > > +	case 1:
> > > +		return CMD_RET_USAGE;
> > > +	case 2:
> > > +		dev = simple_strtoul(argv[1], NULL, 10);
> > 
> > Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> > survive? Hint: it won't, rangecheck missing.
> > 
> > > +		return scan_sata(dev);
> > > +
> > > +	case 3:
> > > +		if (!strncmp(argv[1], "inf", 3)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > 
> > Same here
> > 
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	case 4:
> > > +		if (!strncmp(argv[1], "load", 4)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > > +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > 
> > And here you have it ?
> > 
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> > 
> > > +				printf("File index %d is out of range.\n", dev);
> > > +				return -EINVAL;
> > > +			}
> > > +			free(filenames[dev]);
> > > +			filenames[dev] = strdup(argv[3]);
> > > +			init_sata(dev);
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	}
> > > +	return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > +	sata_loop, 4, 1, do_loop,
> > > +	"SATA loopback",
> > > +	"[info] devnum - show info about loop devnum\n"
> > 
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
> > 
> > > +	"sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> > 
> > sata_loop is redundant above.
> > 
> > > +);

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

[...]

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

See my previous email

> > > +	/* 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.
> 
> hint - scan_sata does the range check in this codepath

saw it, see below. Duplication of code is not good and is error prone.

> > > +		return scan_sata(dev);
> > > +
> > > +	case 3:
> > > +		if (!strncmp(argv[1], "inf", 3)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > 
> > Same here
> 
> see above
> 
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	case 4:
> > > +		if (!strncmp(argv[1], "load", 4)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > > +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > 
> > And here you have it ?
> > 
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> > 
> > > +				printf("File index %d is out of range.\n", dev);
> > > +				return -EINVAL;
> > > +			}
> > > +			free(filenames[dev]);
> > > +			filenames[dev] = strdup(argv[3]);
> > > +			init_sata(dev);
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	}
> > > +	return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > +	sata_loop, 4, 1, do_loop,
> > > +	"SATA loopback",
> > > +	"[info] devnum - show info about loop devnum\n"
> > 
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
> 
> wont help, still need argc 3 or 4

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

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

> 
> Pavel Herrmann

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> [...]
> 
> > > 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 ...
> 
> > , 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?

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.

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

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

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > [...]
> > 
> > > > 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 ...
> > 
> > > , 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?
> 
> 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?

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

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

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.

> Pavel Herrmann

Best regards,
Marek Vasut

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Thursday 06 of September 2012 03:08:42 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > [...]
> > > 
> > > > > 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 ...
> > > 
> > > > , 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?
> > 
> > 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

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

[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Thursday 06 of September 2012 03:08:42 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > > 
> > > > [...]
> > > > 
> > > > > > 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 ...
> > > > 
> > > > > , 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?
> > > 
> > > 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
> 
> > > > > > 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.
> > 
> > > > > > > +	"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
> > 
> > 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

Keep it as is now, but it's something that should eventually be resolved.

> Pavel Herrmann

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Pavel Herrmann]

This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
CC: Mike Frysinger <vapier@gentoo.org>
---
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

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
 COBJS-$(CONFIG_IDE_SIL680) += sil680.o
 COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
 COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..600fbdc
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,212 @@
+/*
+ * (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>
+#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)
+{
+	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"
+);
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..a713430 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -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
-- 
1.7.12

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.

WARNING: externs should be avoided in .c files
#141: FILE: drivers/block/sata_loopback.c:39:
+extern block_dev_desc_t sata_dev_desc[];

total: 0 errors, 1 warnings, 231 lines checked

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX 
MULTISTATEMENT_MACRO_USE_DO_WHILE

> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> CC: Mike Frysinger <vapier@gentoo.org>
> ---
> 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
> 
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
>  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> 
>  COBJS	:= $(COBJS-y)
>  SRCS	:= $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..600fbdc
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,212 @@
> +/*
> + * (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>
> +#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) +{
> +	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"
> +);
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 0220386..a713430 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -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

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> 
> WARNING: externs should be avoided in .c files
> #141: FILE: drivers/block/sata_loopback.c:39:
> +extern block_dev_desc_t sata_dev_desc[];
> 
> total: 0 errors, 1 warnings, 231 lines checked
> 
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE

Yes, i know about that, and chose to ignore it.
i will not create a header file for a single line that checkpatch doesnt like 
(with just a warning), especially when other drivers have such a line in them 
as well.

> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
> > ---
> > 
> > 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
> > 
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > 
> >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > 
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > 
> >  COBJS	:= $(COBJS-y)
> >  SRCS	:= $(COBJS:.o=.c)
> > 
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..600fbdc
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * (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>
> > +#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) +{
> > +	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"
> > +);
> > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> > index 0220386..a713430 100644
> > --- a/include/configs/sandbox.h
> > +++ b/include/configs/sandbox.h
> > @@ -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

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > This driver uses files as block devices, can be used for testing disk
> > > operations on sandbox.
> > > A new command "sata_loop" is introduced to load files in runtime.
> > 
> > WARNING: externs should be avoided in .c files
> > #141: FILE: drivers/block/sata_loopback.c:39:
> > +extern block_dev_desc_t sata_dev_desc[];
> > 
> > total: 0 errors, 1 warnings, 231 lines checked
> > 
> > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > MULTISTATEMENT_MACRO_USE_DO_WHILE
> 
> Yes, i know about that, and chose to ignore it.

Sorry, this is not how it works.

> i will not create a header file for a single line that checkpatch doesnt
> like (with just a warning), especially when other drivers have such a line
> in them as well.

So, implement proper accessor for this list? Or export the structure through 
some sata-related header? How does other do it?

> > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > > CC: Marek Vasut <marex@denx.de>
> > > CC: Mike Frysinger <vapier@gentoo.org>
> > > ---
> > > 
> > > 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
> > > 
> > > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > > index f1ebdcc..c95651a 100644
> > > --- a/drivers/block/Makefile
> > > +++ b/drivers/block/Makefile
> > > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > > 
> > >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > > 
> > > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > > 
> > >  COBJS	:= $(COBJS-y)
> > >  SRCS	:= $(COBJS:.o=.c)
> > > 
> > > diff --git a/drivers/block/sata_loopback.c
> > > b/drivers/block/sata_loopback.c new file mode 100644
> > > index 0000000..600fbdc
> > > --- /dev/null
> > > +++ b/drivers/block/sata_loopback.c
> > > @@ -0,0 +1,212 @@
> > > +/*
> > > + * (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>
> > > +#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) +{
> > > +	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" +);
> > > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> > > index 0220386..a713430 100644
> > > --- a/include/configs/sandbox.h
> > > +++ b/include/configs/sandbox.h
> > > @@ -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

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Pavel Herrmann]

On Friday 07 of September 2012 11:26:48 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > > This driver uses files as block devices, can be used for testing disk
> > > > operations on sandbox.
> > > > A new command "sata_loop" is introduced to load files in runtime.
> > > 
> > > WARNING: externs should be avoided in .c files
> > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > 
> > > total: 0 errors, 1 warnings, 231 lines checked
> > > 
> > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> > 
> > Yes, i know about that, and chose to ignore it.
> 
> Sorry, this is not how it works.
> 
> > i will not create a header file for a single line that checkpatch doesnt
> > like (with just a warning), especially when other drivers have such a line
> > in them as well.
> 
> So, implement proper accessor for this list? Or export the structure through
> some sata-related header? How does other do it?

other drivers have the same line that i do, therefore ignore the checkpatch 
warning, same as i do

see sata_dwc.c, ata_piix.c, fsl_sata.c, sata_sil3114.c

Pavel Herrmann

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> On Friday 07 of September 2012 11:26:48 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > > 
> > > > > This driver uses files as block devices, can be used for testing
> > > > > disk operations on sandbox.
> > > > > A new command "sata_loop" is introduced to load files in runtime.
> > > > 
> > > > WARNING: externs should be avoided in .c files
> > > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > > +extern block_dev_desc_t sata_dev_desc[];
> > > > 
> > > > total: 0 errors, 1 warnings, 231 lines checked
> > > > 
> > > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> > > 
> > > Yes, i know about that, and chose to ignore it.
> > 
> > Sorry, this is not how it works.
> > 
> > > i will not create a header file for a single line that checkpatch
> > > doesnt like (with just a warning), especially when other drivers have
> > > such a line in them as well.
> > 
> > So, implement proper accessor for this list? Or export the structure
> > through some sata-related header? How does other do it?
> 
> other drivers have the same line that i do, therefore ignore the checkpatch
> warning, same as i do

Oh god damned! This is definitelly not the way to go!

NAK! NAK! NAK!! NAK!!! NAK!!!!! NAK!!!!!!!! ... you get the idea ;-)

> see sata_dwc.c, ata_piix.c, fsl_sata.c, sata_sil3114.c

Sad state of codebase, thanks for pointing out the obvious :-( You know, now 
that you pointed it out, you also know what to do :-)

Besides, you'd have to do it anyway, so let's do it now.

> Pavel Herrmann

Best regards,
Marek Vasut

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Tom Rini]

On Fri, Sep 07, 2012 at 11:19:03AM +0200, Pavel Herrmann wrote:
> On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > This driver uses files as block devices, can be used for testing disk
> > > operations on sandbox.
> > > A new command "sata_loop" is introduced to load files in runtime.
> > 
> > WARNING: externs should be avoided in .c files
> > #141: FILE: drivers/block/sata_loopback.c:39:
> > +extern block_dev_desc_t sata_dev_desc[];
> > 
> > total: 0 errors, 1 warnings, 231 lines checked
> > 
> > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > MULTISTATEMENT_MACRO_USE_DO_WHILE
> 
> Yes, i know about that, and chose to ignore it.
> i will not create a header file for a single line that checkpatch doesnt like 
> (with just a warning), especially when other drivers have such a line in them 
> as well.

Please move your extern into <sata.h>, as the starting point before
merge.  If you could also go and fix:
$ git grep -l sata_dev_desc
common/cmd_sata.c
drivers/block/ata_piix.c
drivers/block/dwc_ahsata.c
drivers/block/dwc_ahsata.h
drivers/block/fsl_sata.c
drivers/block/pata_bfin.c
drivers/block/pata_bfin.h
drivers/block/sata_dwc.c
drivers/block/sata_sil.c
drivers/block/sata_sil.h
drivers/block/sata_sil3114.c

To use <sata.h> (some already do) instead of their own private extern
I'd greatly appreciate it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120913/62ef6192/attachment.pgp>

[U-Boot] [PATCH v4 1/2] Loop block device for sandbox

[Pavel Herrmann]

Hi

On Thursday 13 September 2012 15:31:39 Tom Rini wrote:
> On Fri, Sep 07, 2012 at 11:19:03AM +0200, Pavel Herrmann wrote:
> > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > > This driver uses files as block devices, can be used for testing disk
> > > > operations on sandbox.
> > > > A new command "sata_loop" is introduced to load files in runtime.
> > > 
> > > WARNING: externs should be avoided in .c files
> > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > 
> > > total: 0 errors, 1 warnings, 231 lines checked
> > > 
> > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> > 
> > Yes, i know about that, and chose to ignore it.
> > i will not create a header file for a single line that checkpatch doesnt
> > like (with just a warning), especially when other drivers have such a
> > line in them as well.
> 
> Please move your extern into <sata.h>, as the starting point before
> merge.  If you could also go and fix:
> $ git grep -l sata_dev_desc
> common/cmd_sata.c
> drivers/block/ata_piix.c
> drivers/block/dwc_ahsata.c
> drivers/block/dwc_ahsata.h
> drivers/block/fsl_sata.c
> drivers/block/pata_bfin.c
> drivers/block/pata_bfin.h
> drivers/block/sata_dwc.c
> drivers/block/sata_sil.c
> drivers/block/sata_sil.h
> drivers/block/sata_sil3114.c
> 
> To use <sata.h> (some already do) instead of their own private extern
> I'd greatly appreciate it.

I dont think <sata.h> is the place to put this, as it is a "consumer-facing" 
header. rather create some common sata header in drivers/block

Pavel Herrmann

[U-Boot] [PATCH v5 1/2] Loop block device for sandbox

[Pavel Herrmann]

This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v5:
  use common sata extern header - this requires "[PATCH] Fix checkpatch warnings
  about externs in *.c" to be merged first
  make sure we have valid callbacks before scanning partitions

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

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
 COBJS-$(CONFIG_IDE_SIL680) += sil680.o
 COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
 COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..eb3103b
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,213 @@
+/*
+ * (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>
+#include <command.h>
+#include <malloc.h>
+#include "sata_extern.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];
+
+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;
+}
+
+ulong sata_read(int dev, ulong 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;
+}
+
+ulong sata_write(int dev, ulong start, lbaint_t blkcnt, const void *buffer)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	lbaint_t start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_write(fd, buffer, length_byte);
+
+	return retval / ATA_SECT_SIZE;
+}
+
+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;
+		pdev->block_read = sata_read;
+		pdev->block_write = sata_write;
+		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"
+);
-- 
1.7.12

[U-Boot] [PATCH v6 1/2] Loop block device for sandbox

[Pavel Herrmann]

This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v6:
  sync with new version of "Fix checkpatch warnings about externs in *.c"
  set all constant disk properties even on not connected drivers

Changes for v5:
  use common sata extern header - this requires "[PATCH] Fix checkpatch warnings
  about externs in *.c" to be merged first
  make sure we have valid callbacks before scanning partitions

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/sata_loopback.c | 209 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 209 insertions(+)
 create mode 100644 drivers/block/sata_loopback.c

diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..b55b270
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,209 @@
+/*
+ * (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 <sata.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];
+
+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;
+}
+
+ulong sata_read(int dev, ulong 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;
+}
+
+ulong sata_write(int dev, ulong start, lbaint_t blkcnt, const void *buffer)
+{
+	block_dev_desc_t *pdev = &sata_dev_desc[dev];
+	int fd = (long) pdev->priv;
+	lbaint_t start_byte = ATA_SECT_SIZE * start;
+	lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+	lbaint_t retval;
+
+	if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+		return -1;
+
+	retval = os_write(fd, buffer, length_byte);
+
+	return retval / ATA_SECT_SIZE;
+}
+
+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;
+	pdev->type = DEV_TYPE_HARDDISK;
+	pdev->blksz = ATA_SECT_SIZE;
+	pdev->lun = 0;
+	pdev->block_read = sata_read;
+	pdev->block_write = sata_write;
+
+	bytes = os_lseek(fd, 0, OS_SEEK_END);
+	if (bytes != -1) {
+		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->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"
+);
-- 
1.7.12

[U-Boot] [PATCH v6 1/2] Loop block device for sandbox

[Marek Vasut]

Dear Pavel Herrmann,

> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.

The description might use a little bit of caressing here :)

[...]

> +	if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> +		return -1;

Use errno consistently ... so -EINVAL ? Or something else?

> +	retval = os_read(fd, buffer, length_byte);
> +
> +	return retval / ATA_SECT_SIZE;
> +}

[...]
> +		if (!strncmp(argv[1], "load", 4)) {

if (strncmp())
 return CMD_USAGE;

one indent less here ;-)

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