[U-Boot] Loop block device for sandbox

Submitted by Pavel Herrmann on Aug. 29, 2012, 3:46 p.m.

Details

Message ID 1346255203-1225-1-git-send-email-morpheus.ibis@gmail.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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