Patchwork [U-Boot] Loop block device for sandbox

login
register
mail settings
Submitter Pavel Herrmann
Date Aug. 29, 2012, 3:46 p.m.
Message ID <1346255203-1225-1-git-send-email-morpheus.ibis@gmail.com>
Download mbox | patch
Permalink /patch/180717/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Pavel Herrmann - Aug. 29, 2012, 3:46 p.m.
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
Pavel Herrmann - Aug. 29, 2012, 3:48 p.m.
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
Marek Vasut - Aug. 29, 2012, 10:18 p.m.
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
Pavel Herrmann - Aug. 30, 2012, 5:14 p.m.
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
Marek Vasut - Aug. 30, 2012, 6:45 p.m.
Dear Pavel Herrmann,

> On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:
> ...snip...
> 
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > +
> > > +int init_sata(int dev)
> > > +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > 
> > Superfluous braces ... Actually, I think sata_dev_desc as it would work
> > very well too.
> 
> Straight copy from dwc_ahsata.c, makes it more readable thought, as the
> order of operation is not very intuitive IMHO.

sata_dev_desc + dev ?

> > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer)
> > > +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > +	int fd = (long) pdev->priv;
> > 
> > If pdev is NULL, this will crash
> 
> well, it isn't, at least not from the command - thats why you define the
> number of ports in advance, you get "dev" already range-checked

Range check is fine, but will pdev be inited? It's a pointer from some array.

> > > +	lbaint_t retval;
> > > +
> > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > +	retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
> > > +
> > > +	return retval/ATA_SECT_SIZE;
> > > +}
> > > +
> > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer) +{
> > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > +	int fd = (long) pdev->priv;
> > > +	lbaint_t retval;
> > > +
> > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > 
> > Besides, lseek can fail, can it not?
> 
> If you open a pipe (or nothing), yes
> in the first case, you shouldn't

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

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

How so?

> > > +	if (namelen > 20)
> > > +		namelen = 20;
> > 
> > Why do you trim down the string, won't simple strdup() work?
> 
> nah, the destination is char[21], as it is the exact length of
> corresponding field in ATA identify response (one more for a 0 at the end)

I see, is it a full path ? If so, it might be a better idea to use the filename 
itself instead of the whole path. So you'd prevent names like 
"~/../foo/../.././bar.img" .

> > > +	memcpy(pdev->product, filenames[dev], namelen);
> > > +	pdev->product[20] = 0;
> > > +
> > > +	if (fd != -1) {
> > 
> > And if "fd" is -1 ?
> 
> then all defaults to an invalid device, because you failed to open the
> file, for whatever the reason.

At least the printf below will choke, since pdev->lba is uninited

> "agreed" to the other comments
> 
> Best Regards
> Pavel Herrmann
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut
Pavel Herrmann - Aug. 30, 2012, 7:07 p.m.
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
Marek Vasut - Aug. 30, 2012, 9:53 p.m.
Dear Pavel Herrmann,

> On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> > 
> > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:
> > > ...snip...
> > > 
> > > > > +extern block_dev_desc_t sata_dev_desc[];
> > > > > +
> > > > > +int init_sata(int dev)
> > > > > +{
> > > > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > 
> > > > Superfluous braces ... Actually, I think sata_dev_desc as it would
> > > > work very well too.
> > > 
> > > Straight copy from dwc_ahsata.c, makes it more readable thought, as the
> > > order of operation is not very intuitive IMHO.
> > 
> > sata_dev_desc + dev ?
> 
> even less intuitive

Why so?

> > > > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > > > *buffer)
> > > > > +{
> > > > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > > +	int fd = (long) pdev->priv;
> > > > 
> > > > If pdev is NULL, this will crash
> > > 
> > > well, it isn't, at least not from the command - thats why you define
> > > the number of ports in advance, you get "dev" already range-checked
> > 
> > Range check is fine, but will pdev be inited? It's a pointer from some
> > array.
> 
> init_sata is called first, so pdev is inited (see cmd_sata.c)

Unless it fails. Then what ?

> > > > > +	lbaint_t retval;
> > > > > +
> > > > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > > > +	retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
> > > > > +
> > > > > +	return retval/ATA_SECT_SIZE;
> > > > > +}
> > > > > +
> > > > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > > > *buffer) +{
> > > > > +	block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > > +	int fd = (long) pdev->priv;
> > > > > +	lbaint_t retval;
> > > > > +
> > > > > +	os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > > 
> > > > Besides, lseek can fail, can it not?
> > > 
> > > If you open a pipe (or nothing), yes
> > > in the first case, you shouldn't
> > 
> > Shouldn't ... what? Sorry, I cannot parse this.
> 
> shouldn't do that - means i agree there should be a check in case you are
> actively trying to break things, and use pipes/sockets as loop blocks

Good

> > > in the second, the I/O op will harmlessly
> > > fail as well
> > 
> > How so?
> 
> because then the fd is -1, and read/write will do the right thing there
> (nothing, return -1 and set errno to EBADF)

From write(2)

-->8--

RETURN VALUE
       On  success,  the number of bytes written is returned (zero indicates 
nothing was written).  On error, -1 is returned,
       and errno is set appropriately.

       If count is zero and fd refers to a regular file, then write() may return 
a failure status if one of the errors  below
       is detected.  If no errors are detected, 0 will be returned without 
causing any other effect.  If count is zero and fd
       refers to a file other than a regular file, the results are not 
specified.

--8<--

I don't see the case where fd = -1 handled there at all. The last sentence 
resembles it, but in that case, the behavior is undefined. Can you elaborate 
please?

> > > > > +	if (namelen > 20)
> > > > > +		namelen = 20;
> > > > 
> > > > Why do you trim down the string, won't simple strdup() work?
> > > 
> > > nah, the destination is char[21], as it is the exact length of
> > > corresponding field in ATA identify response (one more for a 0 at the
> > > end)
> > 
> > I see, is it a full path ? If so, it might be a better idea to use the
> > filename itself instead of the whole path. So you'd prevent names like
> > "~/../foo/../.././bar.img" .
> 
> yes, i was thinking about "...${last 17 bytes of the name}" if the name was
> longer, but this proved significantly simpler for demonstrating the general
> idea.

I think the FS code might contain some function to fixup the path and get 
filename from path.

> > > > > +	memcpy(pdev->product, filenames[dev], namelen);
> > > > > +	pdev->product[20] = 0;
> > > > > +
> > > > > +	if (fd != -1) {
> > > > 
> > > > And if "fd" is -1 ?
> > > 
> > > then all defaults to an invalid device, because you failed to open the
> > > file, for whatever the reason.
> > 
> > At least the printf below will choke, since pdev->lba is uninited
> 
> not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not
> doing anything we get an empty device

I see ... shall we also move all these memcpy() calls in to if (fd != -1) then?

> Best Regards
> Pavel Herrmann

Best regards,
Marek Vasut
Pavel Herrmann - Aug. 31, 2012, 9:09 a.m.
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
Marek Vasut - Aug. 31, 2012, 12:57 p.m.
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
Pavel Herrmann - Aug. 31, 2012, 2:25 p.m.
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
Marek Vasut - Aug. 31, 2012, 4:02 p.m.
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
Pavel Herrmann - Aug. 31, 2012, 5:56 p.m.
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
Marek Vasut - Aug. 31, 2012, 7:02 p.m.
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

Patch

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