* [U-Boot] [PATCH] Loop block device for sandbox
@ 2012-08-29 15:46 Pavel Herrmann
2012-08-29 15:48 ` Pavel Herrmann
` (2 more replies)
0 siblings, 3 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-29 15:46 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox. Port count and filenames are set in board config.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
---
drivers/block/Makefile | 1 +
drivers/block/loop.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++
include/configs/sandbox.h | 9 ++++
3 files changed, 117 insertions(+)
create mode 100644 drivers/block/loop.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..5eecf37 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
COBJS-$(CONFIG_IDE_SIL680) += sil680.o
COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += loop.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
new file mode 100644
index 0000000..c9edfc3
--- /dev/null
+++ b/drivers/block/loop.c
@@ -0,0 +1,107 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <libata.h>
+#include <errno.h>
+#include <os.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "loopback";
+
+static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
+static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;
+
+extern block_dev_desc_t sata_dev_desc[];
+
+int init_sata(int dev)
+{
+ block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+ int fd;
+
+ if ((dev < 0) || (dev >= max_devs)) {
+ printf("file index %d is out of range\n", dev);
+ return -EINVAL;
+ }
+
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ /* this is ugly, but saves allocation for 1 int */
+ pdev->priv = (void *) (long) fd;
+
+ return 0;
+}
+
+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+ int fd = (long) pdev->priv;
+ lbaint_t retval;
+
+ os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
+ retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+ int fd = (long) pdev->priv;
+ lbaint_t retval;
+
+ os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
+ retval = os_write(fd, buffer, ATA_SECT_SIZE * blkcnt);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
+ int fd = (long) pdev->priv;
+ int namelen;
+ lbaint_t bytes = 0;
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = sizeof(filenames[dev]);
+ if (namelen > 20)
+ namelen = 20;
+ memcpy(pdev->product, filenames[dev], namelen);
+ pdev->product[20] = 0;
+
+ if (fd != -1) {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ }
+
+ printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n",
+ filenames[dev], bytes, pdev->lba);
+
+ return 0;
+}
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..412341f 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -93,4 +93,13 @@
"stdout=serial\0" \
"stderr=serial\0"
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SATA_LOOP_DISKS {"disk1", "disk2"}
+#define CONFIG_SYS_SATA_MAX_DEVICE 2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
#endif
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-29 15:46 [U-Boot] [PATCH] Loop block device for sandbox Pavel Herrmann
@ 2012-08-29 15:48 ` Pavel Herrmann
2012-08-29 22:18 ` Marek Vasut
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
2 siblings, 0 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-29 15:48 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-29 15:46 [U-Boot] [PATCH] Loop block device for sandbox Pavel Herrmann
2012-08-29 15:48 ` Pavel Herrmann
@ 2012-08-29 22:18 ` Marek Vasut
2012-08-30 17:14 ` Pavel Herrmann
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
2 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-08-29 22:18 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-29 22:18 ` Marek Vasut
@ 2012-08-30 17:14 ` Pavel Herrmann
2012-08-30 18:45 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-30 17:14 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-30 17:14 ` Pavel Herrmann
@ 2012-08-30 18:45 ` Marek Vasut
2012-08-30 19:07 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-08-30 18:45 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:
> ...snip...
>
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > +
> > > +int init_sata(int dev)
> > > +{
> > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> >
> > Superfluous braces ... Actually, I think sata_dev_desc as it would work
> > very well too.
>
> Straight copy from dwc_ahsata.c, makes it more readable thought, as the
> order of operation is not very intuitive IMHO.
sata_dev_desc + dev ?
> > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer)
> > > +{
> > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > + int fd = (long) pdev->priv;
> >
> > If pdev is NULL, this will crash
>
> well, it isn't, at least not from the command - thats why you define the
> number of ports in advance, you get "dev" already range-checked
Range check is fine, but will pdev be inited? It's a pointer from some array.
> > > + lbaint_t retval;
> > > +
> > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
> > > +
> > > + return retval/ATA_SECT_SIZE;
> > > +}
> > > +
> > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer) +{
> > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > + int fd = (long) pdev->priv;
> > > + lbaint_t retval;
> > > +
> > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> >
> > Besides, lseek can fail, can it not?
>
> If you open a pipe (or nothing), yes
> in the first case, you shouldn't
Shouldn't ... what? Sorry, I cannot parse this.
> in the second, the I/O op will harmlessly
> fail as well
How so?
> > > + if (namelen > 20)
> > > + namelen = 20;
> >
> > Why do you trim down the string, won't simple strdup() work?
>
> nah, the destination is char[21], as it is the exact length of
> corresponding field in ATA identify response (one more for a 0 at the end)
I see, is it a full path ? If so, it might be a better idea to use the filename
itself instead of the whole path. So you'd prevent names like
"~/../foo/../.././bar.img" .
> > > + memcpy(pdev->product, filenames[dev], namelen);
> > > + pdev->product[20] = 0;
> > > +
> > > + if (fd != -1) {
> >
> > And if "fd" is -1 ?
>
> then all defaults to an invalid device, because you failed to open the
> file, for whatever the reason.
At least the printf below will choke, since pdev->lba is uninited
> "agreed" to the other comments
>
> Best Regards
> Pavel Herrmann
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-30 18:45 ` Marek Vasut
@ 2012-08-30 19:07 ` Pavel Herrmann
2012-08-30 21:53 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-30 19:07 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-30 19:07 ` Pavel Herrmann
@ 2012-08-30 21:53 ` Marek Vasut
2012-08-31 9:09 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-08-30 21:53 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Thursday 30 of August 2012 20:45:13 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > > On Thursday 30 of August 2012 00:18:18 Marek Vasut wrote:
> > > ...snip...
> > >
> > > > > +extern block_dev_desc_t sata_dev_desc[];
> > > > > +
> > > > > +int init_sata(int dev)
> > > > > +{
> > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > >
> > > > Superfluous braces ... Actually, I think sata_dev_desc as it would
> > > > work very well too.
> > >
> > > Straight copy from dwc_ahsata.c, makes it more readable thought, as the
> > > order of operation is not very intuitive IMHO.
> >
> > sata_dev_desc + dev ?
>
> even less intuitive
Why so?
> > > > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > > > *buffer)
> > > > > +{
> > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > > + int fd = (long) pdev->priv;
> > > >
> > > > If pdev is NULL, this will crash
> > >
> > > well, it isn't, at least not from the command - thats why you define
> > > the number of ports in advance, you get "dev" already range-checked
> >
> > Range check is fine, but will pdev be inited? It's a pointer from some
> > array.
>
> init_sata is called first, so pdev is inited (see cmd_sata.c)
Unless it fails. Then what ?
> > > > > + lbaint_t retval;
> > > > > +
> > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > > > + retval = os_read(fd, buffer, ATA_SECT_SIZE * blkcnt);
> > > > > +
> > > > > + return retval/ATA_SECT_SIZE;
> > > > > +}
> > > > > +
> > > > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > > > *buffer) +{
> > > > > + block_dev_desc_t *pdev = &(sata_dev_desc[dev]);
> > > > > + int fd = (long) pdev->priv;
> > > > > + lbaint_t retval;
> > > > > +
> > > > > + os_lseek(fd, start*ATA_SECT_SIZE, OS_SEEK_SET);
> > > >
> > > > Besides, lseek can fail, can it not?
> > >
> > > If you open a pipe (or nothing), yes
> > > in the first case, you shouldn't
> >
> > Shouldn't ... what? Sorry, I cannot parse this.
>
> shouldn't do that - means i agree there should be a check in case you are
> actively trying to break things, and use pipes/sockets as loop blocks
Good
> > > in the second, the I/O op will harmlessly
> > > fail as well
> >
> > How so?
>
> because then the fd is -1, and read/write will do the right thing there
> (nothing, return -1 and set errno to EBADF)
From write(2)
-->8--
RETURN VALUE
On success, the number of bytes written is returned (zero indicates
nothing was written). On error, -1 is returned,
and errno is set appropriately.
If count is zero and fd refers to a regular file, then write() may return
a failure status if one of the errors below
is detected. If no errors are detected, 0 will be returned without
causing any other effect. If count is zero and fd
refers to a file other than a regular file, the results are not
specified.
--8<--
I don't see the case where fd = -1 handled there@all. The last sentence
resembles it, but in that case, the behavior is undefined. Can you elaborate
please?
> > > > > + if (namelen > 20)
> > > > > + namelen = 20;
> > > >
> > > > Why do you trim down the string, won't simple strdup() work?
> > >
> > > nah, the destination is char[21], as it is the exact length of
> > > corresponding field in ATA identify response (one more for a 0 at the
> > > end)
> >
> > I see, is it a full path ? If so, it might be a better idea to use the
> > filename itself instead of the whole path. So you'd prevent names like
> > "~/../foo/../.././bar.img" .
>
> yes, i was thinking about "...${last 17 bytes of the name}" if the name was
> longer, but this proved significantly simpler for demonstrating the general
> idea.
I think the FS code might contain some function to fixup the path and get
filename from path.
> > > > > + memcpy(pdev->product, filenames[dev], namelen);
> > > > > + pdev->product[20] = 0;
> > > > > +
> > > > > + if (fd != -1) {
> > > >
> > > > And if "fd" is -1 ?
> > >
> > > then all defaults to an invalid device, because you failed to open the
> > > file, for whatever the reason.
> >
> > At least the printf below will choke, since pdev->lba is uninited
>
> not the case. sata_dev_desc is inited in cmd_sata.c, and therefore by not
> doing anything we get an empty device
I see ... shall we also move all these memcpy() calls in to if (fd != -1) then?
> Best Regards
> Pavel Herrmann
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-30 21:53 ` Marek Vasut
@ 2012-08-31 9:09 ` Pavel Herrmann
2012-08-31 12:57 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-31 9:09 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-31 9:09 ` Pavel Herrmann
@ 2012-08-31 12:57 ` Marek Vasut
2012-08-31 14:25 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-08-31 12:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-31 12:57 ` Marek Vasut
@ 2012-08-31 14:25 ` Pavel Herrmann
2012-08-31 16:02 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-31 14:25 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-31 14:25 ` Pavel Herrmann
@ 2012-08-31 16:02 ` Marek Vasut
2012-08-31 17:56 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-08-31 16:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-31 16:02 ` Marek Vasut
@ 2012-08-31 17:56 ` Pavel Herrmann
2012-08-31 19:02 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-08-31 17:56 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH] Loop block device for sandbox
2012-08-31 17:56 ` Pavel Herrmann
@ 2012-08-31 19:02 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-08-31 19:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v2 1/2] Loop block device for sandbox
2012-08-29 15:46 [U-Boot] [PATCH] Loop block device for sandbox Pavel Herrmann
2012-08-29 15:48 ` Pavel Herrmann
2012-08-29 22:18 ` Marek Vasut
@ 2012-09-01 13:19 ` Pavel Herrmann
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
` (2 more replies)
2 siblings, 3 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-01 13:19 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox. Port count and filenames are set in board config.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
---
Changes for v2:
split sandbox config off into separate patch (2/2)
rename file to signify exported API
style fixes
show end of long filenames rather than beginning
check for lseek errors to indicate non-regular file
drivers/block/Makefile | 1 +
drivers/block/sata_loopback.c | 122 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 123 insertions(+)
create mode 100644 drivers/block/sata_loopback.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
COBJS-$(CONFIG_IDE_SIL680) += sil680.o
COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..7f25a4f
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,122 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <libata.h>
+#include <errno.h>
+#include <os.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
+static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;
+
+extern block_dev_desc_t sata_dev_desc[];
+
+int init_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd;
+
+ if ((dev < 0) || (dev >= max_devs)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ /* This is ugly, but saves allocation for 1 int. */
+ pdev->priv = (void *) (long) fd;
+
+ return 0;
+}
+
+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_read(fd, buffer, length_byte);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_write(fd, buffer, length_byte);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ int namelen;
+ const char *filename = filenames[dev];
+ lbaint_t bytes = 0;
+
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = strlen(filenames[dev]);
+
+ if (namelen > 20) {
+ /* take the last 17 chars, prepend them with "..." */
+ filename += (namelen - 17);
+ memcpy(pdev->product, "...", 3);
+ memcpy(pdev->product+3, filename, 17);
+ } else
+ memcpy(pdev->product, filename, namelen);
+
+ pdev->product[20] = 0;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ if (bytes != -1) {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ }
+
+ printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n",
+ filenames[dev], bytes, pdev->lba);
+
+ return 0;
+}
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH 2/2] Use loop block device in sandbox board
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
@ 2012-09-01 13:19 ` Pavel Herrmann
2012-09-01 14:20 ` Marek Vasut
2012-09-03 17:23 ` [U-Boot] [PATCH v2 " Pavel Herrmann
2012-09-03 16:49 ` [U-Boot] [PATCH v2 1/2] Loop block device for sandbox Marek Vasut
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
2 siblings, 2 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-01 13:19 UTC (permalink / raw)
To: u-boot
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
include/configs/sandbox.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..3126542 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -93,4 +93,13 @@
"stdout=serial\0" \
"stderr=serial\0"
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SATA_LOOP_DISKS {"disk1", "obscenelylongfilename", "disk3"}
+#define CONFIG_SYS_SATA_MAX_DEVICE 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
#endif
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH 2/2] Use loop block device in sandbox board
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
@ 2012-09-01 14:20 ` Marek Vasut
2012-09-03 17:24 ` Pavel Herrmann
2012-09-03 17:23 ` [U-Boot] [PATCH v2 " Pavel Herrmann
1 sibling, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-01 14:20 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
I don't understand what this patch does from the lacking description. Please add
proper description to the patch. ALWAYS!
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> ---
> include/configs/sandbox.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 0220386..3126542 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -93,4 +93,13 @@
> "stdout=serial\0" \
> "stderr=serial\0"
>
> +/* SATA loopback device */
> +#define CONFIG_CMD_SATA
> +#define CONFIG_SATA_LOOP
> +#define CONFIG_SATA_LOOP_DISKS {"disk1", "obscenelylongfilename", "disk3"}
> +#define CONFIG_SYS_SATA_MAX_DEVICE 3
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_CMD_FAT
> +#define CONFIG_CMD_EXT2
> +
> #endif
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v2 1/2] Loop block device for sandbox
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
@ 2012-09-03 16:49 ` Marek Vasut
2012-09-03 17:31 ` Pavel Herrmann
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
2 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-03 16:49 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> This driver uses files as block devices, can be used for testing disk
> operations on sandbox. Port count and filenames are set in board config.
>
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> ---
> Changes for v2:
> split sandbox config off into separate patch (2/2)
> rename file to signify exported API
> style fixes
> show end of long filenames rather than beginning
> check for lseek errors to indicate non-regular file
>
> drivers/block/Makefile | 1 +
> drivers/block/sata_loopback.c | 122
> ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123
> insertions(+)
> create mode 100644 drivers/block/sata_loopback.c
>
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
>
> COBJS := $(COBJS-y)
> SRCS := $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..7f25a4f
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,122 @@
> +/*
> + * (C) Copyright 2012
> + * Pavel Herrmann <morpheus.ibis@gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <part.h>
> +#include <ata.h>
> +#include <libata.h>
> +#include <errno.h>
> +#include <os.h>
> +
> +static const char revision[] = "0.0";
> +static const char vendor[] = "SATA loopback";
> +
> +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
> +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;
One more thing is missing -- documentation for these (add to doc/ ).
Alternatively (which would be much better), let sandbox uboot accept params and
supply these as params.
> +extern block_dev_desc_t sata_dev_desc[];
> +
> +int init_sata(int dev)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd;
> +
> + if ((dev < 0) || (dev >= max_devs)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + fd = os_open(filenames[dev], OS_O_RDWR);
> + /* This is ugly, but saves allocation for 1 int. */
> + pdev->priv = (void *) (long) fd;
> +
> + return 0;
> +}
> +
> +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_read(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;
> +}
> +
> +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> *buffer) +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_write(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;
> +}
> +
> +int scan_sata(int dev)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + int namelen;
> + const char *filename = filenames[dev];
> + lbaint_t bytes = 0;
> +
> + memcpy(pdev->vendor, vendor, sizeof(vendor));
> + memcpy(pdev->revision, revision, sizeof(revision));
> + namelen = strlen(filenames[dev]);
> +
> + if (namelen > 20) {
> + /* take the last 17 chars, prepend them with "..." */
> + filename += (namelen - 17);
> + memcpy(pdev->product, "...", 3);
> + memcpy(pdev->product+3, filename, 17);
> + } else
> + memcpy(pdev->product, filename, namelen);
> +
> + pdev->product[20] = 0;
> +
> + bytes = os_lseek(fd, 0, OS_SEEK_END);
> + if (bytes != -1) {
> + pdev->type = DEV_TYPE_HARDDISK;
> + pdev->blksz = ATA_SECT_SIZE;
> + pdev->lun = 0;
> + pdev->lba = bytes/ATA_SECT_SIZE;
> + }
> +
> + printf("SATA loop info:\nfilename: %s\nsize: %lu\nblock count: %lu\n",
> + filenames[dev], bytes, pdev->lba);
> +
> + return 0;
> +}
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v2 2/2] Use loop block device in sandbox board
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
2012-09-01 14:20 ` Marek Vasut
@ 2012-09-03 17:23 ` Pavel Herrmann
1 sibling, 0 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-03 17:23 UTC (permalink / raw)
To: u-boot
Enable SATA_LOOP and a few disk-related commands for sandbox
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
changes for v2:
add a few words of description
include/configs/sandbox.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..3126542 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -93,4 +93,13 @@
"stdout=serial\0" \
"stderr=serial\0"
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SATA_LOOP_DISKS {"disk1", "obscenelylongfilename", "disk3"}
+#define CONFIG_SYS_SATA_MAX_DEVICE 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
#endif
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH 2/2] Use loop block device in sandbox board
2012-09-01 14:20 ` Marek Vasut
@ 2012-09-03 17:24 ` Pavel Herrmann
0 siblings, 0 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-03 17:24 UTC (permalink / raw)
To: u-boot
Hi
On Saturday 01 of September 2012 16:20:12 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> I don't understand what this patch does from the lacking description. Please
> add proper description to the patch. ALWAYS!
see $title perhaps? there is not much more to say.
v2 on the way
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v2 1/2] Loop block device for sandbox
2012-09-03 16:49 ` [U-Boot] [PATCH v2 1/2] Loop block device for sandbox Marek Vasut
@ 2012-09-03 17:31 ` Pavel Herrmann
2012-09-03 20:20 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-03 17:31 UTC (permalink / raw)
To: u-boot
On Monday 03 of September 2012 18:49:00 Marek Vasut wrote:
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
> > +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;
>
> One more thing is missing -- documentation for these (add to doc/ ).
which file shoud that be exactly? README.sata has nothing about configs, i
cannot find any README.sandbox or README.configs
> Alternatively (which would be much better), let sandbox uboot accept params
> and supply these as params.
or even make a command that would allow you to specify a filename in runtime,
possibly with a dynamic number of ports.
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v2 1/2] Loop block device for sandbox
2012-09-03 17:31 ` Pavel Herrmann
@ 2012-09-03 20:20 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-03 20:20 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Monday 03 of September 2012 18:49:00 Marek Vasut wrote:
> > > +
> > > +static const char revision[] = "0.0";
> > > +static const char vendor[] = "SATA loopback";
> > > +
> > > +static const char * const filenames[] = CONFIG_SATA_LOOP_DISKS;
> > > +static int max_devs = CONFIG_SYS_SATA_MAX_DEVICE;
> >
> > One more thing is missing -- documentation for these (add to doc/ ).
>
> which file shoud that be exactly? README.sata has nothing about configs, i
> cannot find any README.sandbox or README.configs
Create one ... especially for the newly added feature -- eg.
README.sata_loopback or something,
> > Alternatively (which would be much better), let sandbox uboot accept
> > params and supply these as params.
>
> or even make a command that would allow you to specify a filename in
> runtime, possibly with a dynamic number of ports.
Good idea, indeed.
> Pavel Herrmann
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
2012-09-03 16:49 ` [U-Boot] [PATCH v2 1/2] Loop block device for sandbox Marek Vasut
@ 2012-09-05 11:16 ` Pavel Herrmann
2012-09-05 11:16 ` [U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board Pavel Herrmann
` (2 more replies)
2 siblings, 3 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-05 11:16 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
CC: Mike Frysinger <vapier@gentoo.org>
---
Changes for v3:
introduce sata_loop command
Changes for v2:
split sandbox config off into separate patch (2/2)
rename file to signify exported API
style fixes
show end of long filenames rather than beginning
check for lseek errors to indicate non-regular file
drivers/block/Makefile | 1 +
drivers/block/sata_loopback.c | 200 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 201 insertions(+)
create mode 100644 drivers/block/sata_loopback.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
COBJS-$(CONFIG_IDE_SIL680) += sil680.o
COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..0e6923b
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,200 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <libata.h>
+#include <errno.h>
+#include <os.h>
+#include <command.h>
+#include <malloc.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+extern block_dev_desc_t sata_dev_desc[];
+
+int init_sata(int dev)
+{
+ static int zeroed = 0;
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd, old_fd;
+
+
+ if (!zeroed) {
+ int i;
+ for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
+ filenames[i]=strdup("");
+ zeroed = 1;
+ }
+
+ if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ /* This is ugly, but saves allocation for 1 int. */
+ old_fd = (long) pdev->priv;
+ pdev->priv = (void *) (long) fd;
+ /*
+ * sadly we cannot set -1 to all as above, because "sata init" will zero
+ * this value before calling sata_init.
+ */
+ if ((old_fd > 2) || (old_fd < 0))
+ os_close(old_fd);
+
+ return 0;
+}
+
+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_read(fd, buffer, length_byte);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_write(fd, buffer, length_byte);
+
+ return retval/ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ int namelen;
+ const char *filename = filenames[dev];
+ lbaint_t bytes = 0;
+
+ if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = strlen(filenames[dev]);
+
+ if (namelen > 20) {
+ /* take the last 17 chars, prepend them with "..." */
+ filename += (namelen - 17);
+ memcpy(pdev->product, "...", 3);
+ memcpy(pdev->product+3, filename, 17);
+ } else
+ memcpy(pdev->product, filename, namelen);
+
+ pdev->product[20] = 0;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ if (bytes != -1) {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
+ " %lu\n", dev, filenames[dev], bytes, pdev->lba);
+ } else {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = 0;
+ printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
+ dev, filenames[dev]);
+ }
+
+ return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int dev = 0;
+ static int zeroed = 0;
+
+ /* make sure we have valid filenames */
+ if (!zeroed) {
+ init_sata(0);
+ zeroed = 1;
+ }
+
+ switch (argc) {
+ case 0:
+ case 1:
+ return CMD_RET_USAGE;
+ case 2:
+ dev = simple_strtoul(argv[1], NULL, 10);
+ return scan_sata(dev);
+
+ case 3:
+ if (!strncmp(argv[1], "inf", 3)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ return scan_sata(dev);
+ }
+ return CMD_RET_USAGE;
+ case 4:
+ if (!strncmp(argv[1], "load", 4)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+ free(filenames[dev]);
+ filenames[dev] = strdup(argv[3]);
+ init_sata(dev);
+ return scan_sata(dev);
+ }
+ return CMD_RET_USAGE;
+ }
+ return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+ sata_loop, 4, 1, do_loop,
+ "SATA loopback",
+ "[info] devnum - show info about loop devnum\n"
+ "sata_loop load devnum file - load file from host FS into loop devnum"
+);
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
@ 2012-09-05 11:16 ` Pavel Herrmann
2012-09-05 11:33 ` [U-Boot] [PATCH v3 1/2] Loop block device for sandbox Marek Vasut
2012-09-06 12:31 ` [U-Boot] [PATCH v4 " Pavel Herrmann
2 siblings, 0 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-05 11:16 UTC (permalink / raw)
To: u-boot
Enable SATA_LOOP and a few disk-related commands for sandbox
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v3:
drop static names for loop devices
Changes for v2:
add a few words of description
include/configs/sandbox.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..c238feb 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -93,4 +93,12 @@
"stdout=serial\0" \
"stderr=serial\0"
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SYS_SATA_MAX_DEVICE 3
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
#endif
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
2012-09-05 11:16 ` [U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board Pavel Herrmann
@ 2012-09-05 11:33 ` Marek Vasut
2012-09-05 12:38 ` Pavel Herrmann
2012-09-05 12:42 ` Pavel Herrmann
2012-09-06 12:31 ` [U-Boot] [PATCH v4 " Pavel Herrmann
2 siblings, 2 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-05 11:33 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.
>
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> CC: Mike Frysinger <vapier@gentoo.org>
ERROR: "foo * bar" should be "foo *bar"
#136: FILE: drivers/block/sata_loopback.c:36:
+static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
WARNING: externs should be avoided in .c files
#138: FILE: drivers/block/sata_loopback.c:38:
+extern block_dev_desc_t sata_dev_desc[];
ERROR: do not initialise statics to 0 or NULL
#142: FILE: drivers/block/sata_loopback.c:42:
+ static int zeroed = 0;
ERROR: spaces required around that '=' (ctx:VxV)
#150: FILE: drivers/block/sata_loopback.c:50:
+ filenames[i]=strdup("");
^
ERROR: space prohibited after that open parenthesis '('
#181: FILE: drivers/block/sata_loopback.c:81:
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
ERROR: space prohibited after that open parenthesis '('
#197: FILE: drivers/block/sata_loopback.c:97:
+ if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
ERROR: do not initialise statics to 0 or NULL
#256: FILE: drivers/block/sata_loopback.c:156:
+ static int zeroed = 0;
total: 6 errors, 1 warnings, 207 lines checked
Checkpatch and I never sleep, don't even try ;-)
> ---
> Changes for v3:
> introduce sata_loop command
>
> Changes for v2:
> split sandbox config off into separate patch (2/2)
> rename file to signify exported API
> style fixes
> show end of long filenames rather than beginning
> check for lseek errors to indicate non-regular file
>
> drivers/block/Makefile | 1 +
> drivers/block/sata_loopback.c | 200
> ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> insertions(+)
> create mode 100644 drivers/block/sata_loopback.c
>
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
>
> COBJS := $(COBJS-y)
> SRCS := $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..0e6923b
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,200 @@
> +/*
> + * (C) Copyright 2012
> + * Pavel Herrmann <morpheus.ibis@gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <part.h>
> +#include <ata.h>
> +#include <libata.h>
> +#include <errno.h>
> +#include <os.h>
> +#include <command.h>
> +#include <malloc.h>
> +
> +static const char revision[] = "0.0";
> +static const char vendor[] = "SATA loopback";
> +
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> +
> +extern block_dev_desc_t sata_dev_desc[];
> +
> +int init_sata(int dev)
> +{
> + static int zeroed = 0;
Is this really needed here and below? Pull this crap out.
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd, old_fd;
> +
> +
Redundant newline
> + if (!zeroed) {
> + int i;
> + for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
> + filenames[i]=strdup("");
> + zeroed = 1;
> + }
> +
> + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + fd = os_open(filenames[dev], OS_O_RDWR);
> + /* This is ugly, but saves allocation for 1 int. */
> + old_fd = (long) pdev->priv;
> + pdev->priv = (void *) (long) fd;
> + /*
> + * sadly we cannot set -1 to all as above, because "sata init" will zero
> + * this value before calling sata_init.
Sorry, I can't parse this.
> + */
> + if ((old_fd > 2) || (old_fd < 0))
> + os_close(old_fd);
> +
> + return 0;
> +}
> +
> +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_read(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;
> +}
> +
> +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> *buffer) +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_write(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;
a[space]operator[space]b ... did you ignore all my previous comments?
> +}
> +
> +int scan_sata(int dev)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + int namelen;
> + const char *filename = filenames[dev];
> + lbaint_t bytes = 0;
> +
> + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + memcpy(pdev->vendor, vendor, sizeof(vendor));
> + memcpy(pdev->revision, revision, sizeof(revision));
> + namelen = strlen(filenames[dev]);
> +
> + if (namelen > 20) {
> + /* take the last 17 chars, prepend them with "..." */
> + filename += (namelen - 17);
> + memcpy(pdev->product, "...", 3);
> + memcpy(pdev->product+3, filename, 17);
> + } else
> + memcpy(pdev->product, filename, namelen);
> +
> + pdev->product[20] = 0;
> +
> + bytes = os_lseek(fd, 0, OS_SEEK_END);
> + if (bytes != -1) {
> + pdev->type = DEV_TYPE_HARDDISK;
> + pdev->blksz = ATA_SECT_SIZE;
> + pdev->lun = 0;
> + pdev->lba = bytes/ATA_SECT_SIZE;
> + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> + " %lu\n", dev, filenames[dev], bytes, pdev->lba);
> + } else {
> + pdev->type = DEV_TYPE_HARDDISK;
> + pdev->blksz = ATA_SECT_SIZE;
> + pdev->lun = 0;
> + pdev->lba = 0;
> + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> + dev, filenames[dev]);
> + }
> +
> + return 0;
> +}
> +
> +
> +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> + int dev = 0;
> + static int zeroed = 0;
move this out.
besides, I think it'd be much systematic to just scream at user to call "sata
rescan" and bail out instead of doing it for him.
> + /* make sure we have valid filenames */
> + if (!zeroed) {
> + init_sata(0);
> + zeroed = 1;
> + }
> +
> + switch (argc) {
> + case 0:
> + case 1:
> + return CMD_RET_USAGE;
> + case 2:
> + dev = simple_strtoul(argv[1], NULL, 10);
Ok, so if I run this command and ask for device 0xb00bf33d ... will this
survive? Hint: it won't, rangecheck missing.
> + return scan_sata(dev);
> +
> + case 3:
> + if (!strncmp(argv[1], "inf", 3)) {
> + dev = simple_strtoul(argv[2], NULL, 10);
Same here
> + return scan_sata(dev);
> + }
> + return CMD_RET_USAGE;
> + case 4:
> + if (!strncmp(argv[1], "load", 4)) {
> + dev = simple_strtoul(argv[2], NULL, 10);
> + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
And here you have it ?
Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into
lightweight static inline function.
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> + free(filenames[dev]);
> + filenames[dev] = strdup(argv[3]);
> + init_sata(dev);
> + return scan_sata(dev);
> + }
> + return CMD_RET_USAGE;
> + }
> + return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> + sata_loop, 4, 1, do_loop,
> + "SATA loopback",
> + "[info] devnum - show info about loop devnum\n"
Make this "info" part mandatory. Than you can cut the whole argc loop into
simple "if argc != 2 ; then fail" . And do simple checking for the first letter
of the argument being either i or d .
> + "sata_loop load devnum file - load file from host FS into loop devnum"
sata_loop is redundant above.
> +);
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 11:33 ` [U-Boot] [PATCH v3 1/2] Loop block device for sandbox Marek Vasut
@ 2012-09-05 12:38 ` Pavel Herrmann
2012-09-05 12:48 ` Marek Vasut
2012-09-05 12:42 ` Pavel Herrmann
1 sibling, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-05 12:38 UTC (permalink / raw)
To: u-boot
On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> >
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
>
> ERROR: "foo * bar" should be "foo *bar"
> #136: FILE: drivers/block/sata_loopback.c:36:
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
>
> WARNING: externs should be avoided in .c files
> #138: FILE: drivers/block/sata_loopback.c:38:
> +extern block_dev_desc_t sata_dev_desc[];
>
> ERROR: do not initialise statics to 0 or NULL
> #142: FILE: drivers/block/sata_loopback.c:42:
> + static int zeroed = 0;
>
> ERROR: spaces required around that '=' (ctx:VxV)
> #150: FILE: drivers/block/sata_loopback.c:50:
> + filenames[i]=strdup("");
> ^
>
> ERROR: space prohibited after that open parenthesis '('
> #181: FILE: drivers/block/sata_loopback.c:81:
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
>
> ERROR: space prohibited after that open parenthesis '('
> #197: FILE: drivers/block/sata_loopback.c:97:
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
>
> ERROR: do not initialise statics to 0 or NULL
> #256: FILE: drivers/block/sata_loopback.c:156:
> + static int zeroed = 0;
>
> total: 6 errors, 1 warnings, 207 lines checked
>
> Checkpatch and I never sleep, don't even try ;-)
>
sorry, my bad, i thought i had it on pre-commit
> > ---
> >
> > Changes for v3:
> > introduce sata_loop command
> >
> > Changes for v2:
> > split sandbox config off into separate patch (2/2)
> > rename file to signify exported API
> > style fixes
> > show end of long filenames rather than beginning
> > check for lseek errors to indicate non-regular file
> >
> > drivers/block/Makefile | 1 +
> > drivers/block/sata_loopback.c | 200
> >
> > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> > insertions(+)
> >
> > create mode 100644 drivers/block/sata_loopback.c
> >
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> >
> > COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> >
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> >
> > COBJS := $(COBJS-y)
> > SRCS := $(COBJS:.o=.c)
> >
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..0e6923b
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.ibis@gmail.com>
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <part.h>
> > +#include <ata.h>
> > +#include <libata.h>
> > +#include <errno.h>
> > +#include <os.h>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +int init_sata(int dev)
> > +{
> > + static int zeroed = 0;
>
> Is this really needed here and below? Pull this crap out.
yes, because you dont know whetehr this gets called first from sata_loop or
from "sata init"
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd, old_fd;
> > +
> > +
>
> Redundant newline
>
> > + if (!zeroed) {
> > + int i;
> > + for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
> > + filenames[i]=strdup("");
> > + zeroed = 1;
> > + }
> > +
> > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > +
> > + fd = os_open(filenames[dev], OS_O_RDWR);
> > + /* This is ugly, but saves allocation for 1 int. */
> > + old_fd = (long) pdev->priv;
> > + pdev->priv = (void *) (long) fd;
> > + /*
> > + * sadly we cannot set -1 to all as above, because "sata init" will
zero
> > + * this value before calling sata_init.
>
> Sorry, I can't parse this.
>
> > + */
> > + if ((old_fd > 2) || (old_fd < 0))
> > + os_close(old_fd);
> > +
> > + return 0;
> > +}
> > +
> > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > + lbaint_t retval;
> > +
> > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > + return -1;
> > +
> > + retval = os_read(fd, buffer, length_byte);
> > +
> > + return retval/ATA_SECT_SIZE;
> > +}
> > +
> > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer) +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > + lbaint_t retval;
> > +
> > + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > + return -1;
> > +
> > + retval = os_write(fd, buffer, length_byte);
> > +
> > + return retval/ATA_SECT_SIZE;
>
> a[space]operator[space]b ... did you ignore all my previous comments?
>
> > +}
> > +
> > +int scan_sata(int dev)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + int namelen;
> > + const char *filename = filenames[dev];
> > + lbaint_t bytes = 0;
> > +
> > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > +
> > + memcpy(pdev->vendor, vendor, sizeof(vendor));
> > + memcpy(pdev->revision, revision, sizeof(revision));
> > + namelen = strlen(filenames[dev]);
> > +
> > + if (namelen > 20) {
> > + /* take the last 17 chars, prepend them with "..." */
> > + filename += (namelen - 17);
> > + memcpy(pdev->product, "...", 3);
> > + memcpy(pdev->product+3, filename, 17);
> > + } else
> > + memcpy(pdev->product, filename, namelen);
> > +
> > + pdev->product[20] = 0;
> > +
> > + bytes = os_lseek(fd, 0, OS_SEEK_END);
> > + if (bytes != -1) {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = bytes/ATA_SECT_SIZE;
> > + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > + " %lu\n", dev, filenames[dev], bytes, pdev->lba);
> > + } else {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = 0;
> > + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > + dev, filenames[dev]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > + int dev = 0;
> > + static int zeroed = 0;
>
> move this out.
>
> besides, I think it'd be much systematic to just scream at user to call
> "sata rescan" and bail out instead of doing it for him.
i dont actually need a sata rescan, i just need to make sure i have
dynamically allocated names, so i can safely call free() later, otherwise this
segfaults when called before sata scan
> > + /* make sure we have valid filenames */
> > + if (!zeroed) {
> > + init_sata(0);
> > + zeroed = 1;
> > + }
> > +
> > + switch (argc) {
> > + case 0:
> > + case 1:
> > + return CMD_RET_USAGE;
> > + case 2:
> > + dev = simple_strtoul(argv[1], NULL, 10);
>
> Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> survive? Hint: it won't, rangecheck missing.
>
> > + return scan_sata(dev);
> > +
> > + case 3:
> > + if (!strncmp(argv[1], "inf", 3)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
>
> Same here
>
> > + return scan_sata(dev);
> > + }
> > + return CMD_RET_USAGE;
> > + case 4:
> > + if (!strncmp(argv[1], "load", 4)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
> > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
>
> And here you have it ?
>
> Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into
> lightweight static inline function.
>
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > + free(filenames[dev]);
> > + filenames[dev] = strdup(argv[3]);
> > + init_sata(dev);
> > + return scan_sata(dev);
> > + }
> > + return CMD_RET_USAGE;
> > + }
> > + return CMD_RET_USAGE;
> > +}
> > +
> > +U_BOOT_CMD(
> > + sata_loop, 4, 1, do_loop,
> > + "SATA loopback",
> > + "[info] devnum - show info about loop devnum\n"
>
> Make this "info" part mandatory. Than you can cut the whole argc loop into
> simple "if argc != 2 ; then fail" . And do simple checking for the first
> letter of the argument being either i or d .
>
> > + "sata_loop load devnum file - load file from host FS into loop devnum"
>
> sata_loop is redundant above.
>
> > +);
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 11:33 ` [U-Boot] [PATCH v3 1/2] Loop block device for sandbox Marek Vasut
2012-09-05 12:38 ` Pavel Herrmann
@ 2012-09-05 12:42 ` Pavel Herrmann
2012-09-05 12:52 ` Marek Vasut
1 sibling, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-05 12:42 UTC (permalink / raw)
To: u-boot
second try...
On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> >
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
>
> ERROR: "foo * bar" should be "foo *bar"
> #136: FILE: drivers/block/sata_loopback.c:36:
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
>
> WARNING: externs should be avoided in .c files
> #138: FILE: drivers/block/sata_loopback.c:38:
> +extern block_dev_desc_t sata_dev_desc[];
>
> ERROR: do not initialise statics to 0 or NULL
> #142: FILE: drivers/block/sata_loopback.c:42:
> + static int zeroed = 0;
>
> ERROR: spaces required around that '=' (ctx:VxV)
> #150: FILE: drivers/block/sata_loopback.c:50:
> + filenames[i]=strdup("");
> ^
>
> ERROR: space prohibited after that open parenthesis '('
> #181: FILE: drivers/block/sata_loopback.c:81:
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
>
> ERROR: space prohibited after that open parenthesis '('
> #197: FILE: drivers/block/sata_loopback.c:97:
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
>
> ERROR: do not initialise statics to 0 or NULL
> #256: FILE: drivers/block/sata_loopback.c:156:
> + static int zeroed = 0;
>
> total: 6 errors, 1 warnings, 207 lines checked
>
> Checkpatch and I never sleep, don't even try ;-)
>
sorry, my bad, i thought i had it on pre-commit
> > ---
> >
> > Changes for v3:
> > introduce sata_loop command
> >
> > Changes for v2:
> > split sandbox config off into separate patch (2/2)
> > rename file to signify exported API
> > style fixes
> > show end of long filenames rather than beginning
> > check for lseek errors to indicate non-regular file
> >
> > drivers/block/Makefile | 1 +
> > drivers/block/sata_loopback.c | 200
> >
> > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> > insertions(+)
> >
> > create mode 100644 drivers/block/sata_loopback.c
> >
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> >
> > COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> >
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> >
> > COBJS := $(COBJS-y)
> > SRCS := $(COBJS:.o=.c)
> >
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..0e6923b
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.ibis@gmail.com>
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <part.h>
> > +#include <ata.h>
> > +#include <libata.h>
> > +#include <errno.h>
> > +#include <os.h>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +int init_sata(int dev)
> > +{
> > + static int zeroed = 0;
>
> Is this really needed here and below? Pull this crap out.
yes, because you dont know whether this gets called first from sata_loop or
from "sata init"
> > +int scan_sata(int dev)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + int namelen;
> > + const char *filename = filenames[dev];
> > + lbaint_t bytes = 0;
> > +
> > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > +
> > + memcpy(pdev->vendor, vendor, sizeof(vendor));
> > + memcpy(pdev->revision, revision, sizeof(revision));
> > + namelen = strlen(filenames[dev]);
> > +
> > + if (namelen > 20) {
> > + /* take the last 17 chars, prepend them with "..." */
> > + filename += (namelen - 17);
> > + memcpy(pdev->product, "...", 3);
> > + memcpy(pdev->product+3, filename, 17);
> > + } else
> > + memcpy(pdev->product, filename, namelen);
> > +
> > + pdev->product[20] = 0;
> > +
> > + bytes = os_lseek(fd, 0, OS_SEEK_END);
> > + if (bytes != -1) {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = bytes/ATA_SECT_SIZE;
> > + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > + " %lu\n", dev, filenames[dev], bytes, pdev->lba);
> > + } else {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = 0;
> > + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > + dev, filenames[dev]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > + int dev = 0;
> > + static int zeroed = 0;
>
> move this out.
>
> besides, I think it'd be much systematic to just scream at user to call
> "sata rescan" and bail out instead of doing it for him.
i dont actually need a sata rescan, i just need to make sure i have
dynamically allocated names, so i can safely call free() later, otherwise this
segfaults when called before sata scan
> > + /* make sure we have valid filenames */
> > + if (!zeroed) {
> > + init_sata(0);
> > + zeroed = 1;
> > + }
> > +
> > + switch (argc) {
> > + case 0:
> > + case 1:
> > + return CMD_RET_USAGE;
> > + case 2:
> > + dev = simple_strtoul(argv[1], NULL, 10);
>
> Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> survive? Hint: it won't, rangecheck missing.
hint - scan_sata does the range check in this codepath
> > + return scan_sata(dev);
> > +
> > + case 3:
> > + if (!strncmp(argv[1], "inf", 3)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
>
> Same here
see above
> > + return scan_sata(dev);
> > + }
> > + return CMD_RET_USAGE;
> > + case 4:
> > + if (!strncmp(argv[1], "load", 4)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
> > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
>
> And here you have it ?
>
> Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out into
> lightweight static inline function.
>
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > + free(filenames[dev]);
> > + filenames[dev] = strdup(argv[3]);
> > + init_sata(dev);
> > + return scan_sata(dev);
> > + }
> > + return CMD_RET_USAGE;
> > + }
> > + return CMD_RET_USAGE;
> > +}
> > +
> > +U_BOOT_CMD(
> > + sata_loop, 4, 1, do_loop,
> > + "SATA loopback",
> > + "[info] devnum - show info about loop devnum\n"
>
> Make this "info" part mandatory. Than you can cut the whole argc loop into
> simple "if argc != 2 ; then fail" . And do simple checking for the first
> letter of the argument being either i or d .
wont help, still need argc 3 or 4
> > + "sata_loop load devnum file - load file from host FS into loop devnum"
>
> sata_loop is redundant above.
really? how do you figure?
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 12:38 ` Pavel Herrmann
@ 2012-09-05 12:48 ` Marek Vasut
2012-09-05 20:25 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-05 12:48 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
[...]
> > besides, I think it'd be much systematic to just scream at user to call
> > "sata rescan" and bail out instead of doing it for him.
>
> i dont actually need a sata rescan, i just need to make sure i have
> dynamically allocated names
Sorry, I can't parse this ... but ...
> , so i can safely call free() later, otherwise
> this segfaults when called before sata scan
The free() function frees the memory space pointed to by ptr, which must have
been returned by a previous call to malloc(), calloc() or realloc(). Otherwise,
or if free(ptr) has already been called before, undefined behavior occurs. If
ptr is NULL, no operation is performed.
So if you call free() on null pointer, nothing happens. Where's the real
problem?
> > > + /* make sure we have valid filenames */
> > > + if (!zeroed) {
> > > + init_sata(0);
> > > + zeroed = 1;
> > > + }
> > > +
> > > + switch (argc) {
> > > + case 0:
> > > + case 1:
> > > + return CMD_RET_USAGE;
> > > + case 2:
> > > + dev = simple_strtoul(argv[1], NULL, 10);
> >
> > Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> > survive? Hint: it won't, rangecheck missing.
> >
> > > + return scan_sata(dev);
> > > +
> > > + case 3:
> > > + if (!strncmp(argv[1], "inf", 3)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> >
> > Same here
> >
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + case 4:
> > > + if (!strncmp(argv[1], "load", 4)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> >
> > And here you have it ?
> >
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> >
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > + free(filenames[dev]);
> > > + filenames[dev] = strdup(argv[3]);
> > > + init_sata(dev);
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + }
> > > + return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > + sata_loop, 4, 1, do_loop,
> > > + "SATA loopback",
> > > + "[info] devnum - show info about loop devnum\n"
> >
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
> >
> > > + "sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> >
> > sata_loop is redundant above.
> >
> > > +);
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 12:42 ` Pavel Herrmann
@ 2012-09-05 12:52 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-05 12:52 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
[...]
> >
> > move this out.
> >
> > besides, I think it'd be much systematic to just scream at user to call
> > "sata rescan" and bail out instead of doing it for him.
>
> i dont actually need a sata rescan, i just need to make sure i have
> dynamically allocated names, so i can safely call free() later, otherwise
> this segfaults when called before sata scan
See my previous email
> > > + /* make sure we have valid filenames */
> > > + if (!zeroed) {
> > > + init_sata(0);
> > > + zeroed = 1;
> > > + }
> > > +
> > > + switch (argc) {
> > > + case 0:
> > > + case 1:
> > > + return CMD_RET_USAGE;
> > > + case 2:
> > > + dev = simple_strtoul(argv[1], NULL, 10);
> >
> > Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> > survive? Hint: it won't, rangecheck missing.
>
> hint - scan_sata does the range check in this codepath
saw it, see below. Duplication of code is not good and is error prone.
> > > + return scan_sata(dev);
> > > +
> > > + case 3:
> > > + if (!strncmp(argv[1], "inf", 3)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> >
> > Same here
>
> see above
>
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + case 4:
> > > + if (!strncmp(argv[1], "load", 4)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> > > + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> >
> > And here you have it ?
> >
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> >
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > + free(filenames[dev]);
> > > + filenames[dev] = strdup(argv[3]);
> > > + init_sata(dev);
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + }
> > > + return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > + sata_loop, 4, 1, do_loop,
> > > + "SATA loopback",
> > > + "[info] devnum - show info about loop devnum\n"
> >
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
>
> wont help, still need argc 3 or 4
Makes is simpler, you can bail out if it's not 3 or 4
> > > + "sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> >
> > sata_loop is redundant above.
>
> really? how do you figure?
Run "help sata_loop" and see the result ...
>
> Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 12:48 ` Marek Vasut
@ 2012-09-05 20:25 ` Pavel Herrmann
2012-09-06 1:08 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-05 20:25 UTC (permalink / raw)
To: u-boot
On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> [...]
>
> > > besides, I think it'd be much systematic to just scream at user to call
> > > "sata rescan" and bail out instead of doing it for him.
> >
> > i dont actually need a sata rescan, i just need to make sure i have
> > dynamically allocated names
>
> Sorry, I can't parse this ... but ...
>
> > , so i can safely call free() later, otherwise
> > this segfaults when called before sata scan
>
> The free() function frees the memory space pointed to by ptr, which must
> have been returned by a previous call to malloc(), calloc() or realloc().
> Otherwise, or if free(ptr) has already been called before, undefined
> behavior occurs. If ptr is NULL, no operation is performed.
>
> So if you call free() on null pointer, nothing happens. Where's the real
> problem?
if you called "sata init" before setting all the loops you would get to
open(NULL) and a strlen(NULL). i think its easier to supply valid empty string
then check for NULL at every access.
> > >
> > > Make this "info" part mandatory. Than you can cut the whole argc loop
> > > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > > first letter of the argument being either i or d .
> >
> > wont help, still need argc 3 or 4
>
> Makes is simpler, you can bail out if it's not 3 or 4
still, i should have a "sata_loop info" work on all files, so theres a valid
command with argc 2
> > > > + "sata_loop load devnum file - load file from host FS into loop
> > > > devnum"
> > >
> > > sata_loop is redundant above.
> >
> > really? how do you figure?
>
> Run "help sata_loop" and see the result ...
=>help sata_loop
sata_loop - SATA loopback
Usage:
sata_loop [info] devnum - show info about loop devnum
sata_loop load devnum file - load file from host FS into loop devnum
i dont see your problem
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-05 20:25 ` Pavel Herrmann
@ 2012-09-06 1:08 ` Marek Vasut
2012-09-06 8:45 ` Pavel Herrmann
0 siblings, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-06 1:08 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > [...]
> >
> > > > besides, I think it'd be much systematic to just scream at user to
> > > > call "sata rescan" and bail out instead of doing it for him.
> > >
> > > i dont actually need a sata rescan, i just need to make sure i have
> > > dynamically allocated names
> >
> > Sorry, I can't parse this ... but ...
> >
> > > , so i can safely call free() later, otherwise
> > > this segfaults when called before sata scan
> >
> > The free() function frees the memory space pointed to by ptr, which must
> > have been returned by a previous call to malloc(), calloc() or realloc().
> > Otherwise, or if free(ptr) has already been called before, undefined
> > behavior occurs. If ptr is NULL, no operation is performed.
> >
> > So if you call free() on null pointer, nothing happens. Where's the real
> > problem?
>
> if you called "sata init" before setting all the loops you would get to
> open(NULL) and a strlen(NULL). i think its easier to supply valid empty
> string then check for NULL at every access.
I'd say check for null on every access ... you'd lower memory requirements and
the increase in code requirement should really be very minor. Also, it'd remove
the "zeroed" static variable altogether, correct?
> > > > Make this "info" part mandatory. Than you can cut the whole argc loop
> > > > into simple "if argc != 2 ; then fail" . And do simple checking for
> > > > the first letter of the argument being either i or d .
> > >
> > > wont help, still need argc 3 or 4
> >
> > Makes is simpler, you can bail out if it's not 3 or 4
>
> still, i should have a "sata_loop info" work on all files, so theres a
> valid command with argc 2
Understood.
> > > > > + "sata_loop load devnum file - load file from host FS into loop
> > > > > devnum"
> > > >
> > > > sata_loop is redundant above.
> > >
> > > really? how do you figure?
> >
> > Run "help sata_loop" and see the result ...
>
> =>help sata_loop
> sata_loop - SATA loopback
>
> Usage:
> sata_loop [info] devnum - show info about loop devnum
> sata_loop load devnum file - load file from host FS into loop devnum
>
> i dont see your problem
Ewww ... nevermind ... I just detected yet another stupid issue in the codebase.
I'd say this "sata_loop" should be either omitted and filled by the macro or
something. But that's for other time to solve ... so I'm fine with this now.
> Pavel Herrmann
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-06 1:08 ` Marek Vasut
@ 2012-09-06 8:45 ` Pavel Herrmann
2012-09-06 8:48 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-06 8:45 UTC (permalink / raw)
To: u-boot
On Thursday 06 of September 2012 03:08:42 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> > On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > >
> > > [...]
> > >
> > > > > besides, I think it'd be much systematic to just scream at user to
> > > > > call "sata rescan" and bail out instead of doing it for him.
> > > >
> > > > i dont actually need a sata rescan, i just need to make sure i have
> > > > dynamically allocated names
> > >
> > > Sorry, I can't parse this ... but ...
> > >
> > > > , so i can safely call free() later, otherwise
> > > > this segfaults when called before sata scan
> > >
> > > The free() function frees the memory space pointed to by ptr, which must
> > > have been returned by a previous call to malloc(), calloc() or
> > > realloc().
> > > Otherwise, or if free(ptr) has already been called before, undefined
> > > behavior occurs. If ptr is NULL, no operation is performed.
> > >
> > > So if you call free() on null pointer, nothing happens. Where's the real
> > > problem?
> >
> > if you called "sata init" before setting all the loops you would get to
> > open(NULL) and a strlen(NULL). i think its easier to supply valid empty
> > string then check for NULL at every access.
>
> I'd say check for null on every access ... you'd lower memory requirements
> and the increase in code requirement should really be very minor. Also,
> it'd remove the "zeroed" static variable altogether, correct?
OK, fine by me
> > > > > Make this "info" part mandatory. Than you can cut the whole argc
> > > > > loop
> > > > > into simple "if argc != 2 ; then fail" . And do simple checking for
> > > > > the first letter of the argument being either i or d .
> > > >
> > > > wont help, still need argc 3 or 4
> > >
> > > Makes is simpler, you can bail out if it's not 3 or 4
> >
> > still, i should have a "sata_loop info" work on all files, so theres a
> > valid command with argc 2
>
> Understood.
>
> > > > > > + "sata_loop load devnum file - load file from host FS into loop
> > > > > > devnum"
> > > > >
> > > > > sata_loop is redundant above.
> > > >
> > > > really? how do you figure?
> > >
> > > Run "help sata_loop" and see the result ...
> >
> > =>help sata_loop
> > sata_loop - SATA loopback
> >
> > Usage:
> > sata_loop [info] devnum - show info about loop devnum
> > sata_loop load devnum file - load file from host FS into loop devnum
> >
> > i dont see your problem
>
> Ewww ... nevermind ... I just detected yet another stupid issue in the
> codebase. I'd say this "sata_loop" should be either omitted and filled by
> the macro or something. But that's for other time to solve ... so I'm fine
> with this now.
it actually is, but only to the first line (as both lines are in one string).
In my opinion it should either add this for each line (with some magic string
modification) or none at all, to keep it consistent
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
2012-09-06 8:45 ` Pavel Herrmann
@ 2012-09-06 8:48 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-06 8:48 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Thursday 06 of September 2012 03:08:42 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > > On Wednesday 05 of September 2012 14:48:40 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > >
> > > > [...]
> > > >
> > > > > > besides, I think it'd be much systematic to just scream at user
> > > > > > to call "sata rescan" and bail out instead of doing it for him.
> > > > >
> > > > > i dont actually need a sata rescan, i just need to make sure i have
> > > > > dynamically allocated names
> > > >
> > > > Sorry, I can't parse this ... but ...
> > > >
> > > > > , so i can safely call free() later, otherwise
> > > > > this segfaults when called before sata scan
> > > >
> > > > The free() function frees the memory space pointed to by ptr, which
> > > > must have been returned by a previous call to malloc(), calloc() or
> > > > realloc().
> > > > Otherwise, or if free(ptr) has already been called before, undefined
> > > > behavior occurs. If ptr is NULL, no operation is performed.
> > > >
> > > > So if you call free() on null pointer, nothing happens. Where's the
> > > > real problem?
> > >
> > > if you called "sata init" before setting all the loops you would get to
> > > open(NULL) and a strlen(NULL). i think its easier to supply valid empty
> > > string then check for NULL at every access.
> >
> > I'd say check for null on every access ... you'd lower memory
> > requirements and the increase in code requirement should really be very
> > minor. Also, it'd remove the "zeroed" static variable altogether,
> > correct?
>
> OK, fine by me
>
> > > > > > Make this "info" part mandatory. Than you can cut the whole argc
> > > > > > loop
> > > > > > into simple "if argc != 2 ; then fail" . And do simple checking
> > > > > > for the first letter of the argument being either i or d .
> > > > >
> > > > > wont help, still need argc 3 or 4
> > > >
> > > > Makes is simpler, you can bail out if it's not 3 or 4
> > >
> > > still, i should have a "sata_loop info" work on all files, so theres a
> > > valid command with argc 2
> >
> > Understood.
> >
> > > > > > > + "sata_loop load devnum file - load file from host FS into
> > > > > > > loop devnum"
> > > > > >
> > > > > > sata_loop is redundant above.
> > > > >
> > > > > really? how do you figure?
> > > >
> > > > Run "help sata_loop" and see the result ...
> > >
> > > =>help sata_loop
> > > sata_loop - SATA loopback
> > >
> > > Usage:
> > > sata_loop [info] devnum - show info about loop devnum
> > > sata_loop load devnum file - load file from host FS into loop devnum
> > >
> > > i dont see your problem
> >
> > Ewww ... nevermind ... I just detected yet another stupid issue in the
> > codebase. I'd say this "sata_loop" should be either omitted and filled by
> > the macro or something. But that's for other time to solve ... so I'm
> > fine with this now.
>
> it actually is, but only to the first line (as both lines are in one
> string). In my opinion it should either add this for each line (with some
> magic string modification) or none at all, to keep it consistent
Keep it as is now, but it's something that should eventually be resolved.
> Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
2012-09-05 11:16 ` [U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board Pavel Herrmann
2012-09-05 11:33 ` [U-Boot] [PATCH v3 1/2] Loop block device for sandbox Marek Vasut
@ 2012-09-06 12:31 ` Pavel Herrmann
2012-09-06 23:29 ` Marek Vasut
2012-09-16 11:58 ` [U-Boot] [PATCH v5 " Pavel Herrmann
2 siblings, 2 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-06 12:31 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
CC: Marek Vasut <marex@denx.de>
CC: Mike Frysinger <vapier@gentoo.org>
---
Changes for v4:
checkpatch fixes
use NULLs instead of ""
extend sata_loop command
Changes for v3:
introduce sata_loop command
Changes for v2:
split sandbox config off into separate patch (2/2)
rename file to signify exported API
style fixes
show end of long filenames rather than beginning
check for lseek errors to indicate non-regular file
drivers/block/Makefile | 1 +
drivers/block/sata_loopback.c | 212 ++++++++++++++++++++++++++++++++++++++++++
include/configs/sandbox.h | 8 ++
3 files changed, 221 insertions(+)
create mode 100644 drivers/block/sata_loopback.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
COBJS-$(CONFIG_IDE_SIL680) += sil680.o
COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..600fbdc
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,212 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <libata.h>
+#include <errno.h>
+#include <os.h>
+#include <command.h>
+#include <malloc.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+/* this will be auto-initialized to NULLs */
+static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+extern block_dev_desc_t sata_dev_desc[];
+
+static inline int range_check(int dev)
+{
+ return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
+}
+
+int init_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd, old_fd;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ else
+ fd = -1;
+
+ old_fd = (long) pdev->priv;
+ /* This is ugly, but saves allocation for 1 int. */
+ pdev->priv = (void *) (long) fd;
+
+ if ((old_fd > 2) || (old_fd < 0))
+ os_close(old_fd);
+
+ return 0;
+}
+
+lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_read(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_write(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ int namelen;
+ char *filename;
+ lbaint_t bytes = 0;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ filename = filenames[dev];
+ else
+ filename = "";
+
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = strlen(filename);
+
+ if (namelen > 20) {
+ /* take the last 17 chars, prepend them with "..." */
+ memcpy(pdev->product, "...", 3);
+ memcpy(pdev->product+3, filename + (namelen - 17), 17);
+ } else
+ memcpy(pdev->product, filename, namelen);
+
+ pdev->product[20] = 0;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ if (bytes != -1) {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
+ " %lu\n", dev, filename, bytes, pdev->lba);
+ } else {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = 0;
+ printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
+ dev, filename);
+ }
+
+ return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int dev = 0;
+
+ switch (argc) {
+ case 0:
+ case 1:
+ return CMD_RET_USAGE;
+ case 2:
+ if (!strncmp(argv[1], "inf", 3)) {
+ for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
+ scan_sata(dev);
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ case 3:
+ if (!strncmp(argv[1], "inf", 3)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ return scan_sata(dev);
+ }
+ return CMD_RET_USAGE;
+ case 4:
+ if (!strncmp(argv[1], "load", 4)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ /*
+ * init_sata() and scan_sata() do their own range
+ * check, however we need to explicitly do it here
+ * as well.
+ */
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+ free(filenames[dev]);
+ filenames[dev] = strdup(argv[3]);
+ init_sata(dev);
+ scan_sata(dev);
+ /*
+ * Scan the partition table if we succeeded in loading
+ * the new loop file.
+ */
+ if (sata_dev_desc[dev].lba > 0)
+ init_part(&sata_dev_desc[dev]);
+
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ }
+ return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+ sata_loop, 4, 1, do_loop,
+ "SATA loopback",
+ "info - show info about all loop devices\n"
+ "sata_loop info devnum - show info about loop devnum\n"
+ "sata_loop load devnum file - load file from host FS into loop devnum"
+);
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 0220386..a713430 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -93,4 +93,12 @@
"stdout=serial\0" \
"stderr=serial\0"
+/* SATA loopback device */
+#define CONFIG_CMD_SATA
+#define CONFIG_SATA_LOOP
+#define CONFIG_SYS_SATA_MAX_DEVICE 2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+
#endif
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-06 12:31 ` [U-Boot] [PATCH v4 " Pavel Herrmann
@ 2012-09-06 23:29 ` Marek Vasut
2012-09-07 9:19 ` Pavel Herrmann
2012-09-16 11:58 ` [U-Boot] [PATCH v5 " Pavel Herrmann
1 sibling, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-06 23:29 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.
WARNING: externs should be avoided in .c files
#141: FILE: drivers/block/sata_loopback.c:39:
+extern block_dev_desc_t sata_dev_desc[];
total: 0 errors, 1 warnings, 231 lines checked
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
MULTISTATEMENT_MACRO_USE_DO_WHILE
> Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> CC: Marek Vasut <marex@denx.de>
> CC: Mike Frysinger <vapier@gentoo.org>
> ---
> Changes for v4:
> checkpatch fixes
> use NULLs instead of ""
> extend sata_loop command
>
> Changes for v3:
> introduce sata_loop command
>
> Changes for v2:
> split sandbox config off into separate patch (2/2)
> rename file to signify exported API
> style fixes
> show end of long filenames rather than beginning
> check for lseek errors to indicate non-regular file
>
> drivers/block/Makefile | 1 +
> drivers/block/sata_loopback.c | 212
> ++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h |
> 8 ++
> 3 files changed, 221 insertions(+)
> create mode 100644 drivers/block/sata_loopback.c
>
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..c95651a 100644
> --- a/drivers/block/Makefile
> +++ b/drivers/block/Makefile
> @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
>
> COBJS := $(COBJS-y)
> SRCS := $(COBJS:.o=.c)
> diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> new file mode 100644
> index 0000000..600fbdc
> --- /dev/null
> +++ b/drivers/block/sata_loopback.c
> @@ -0,0 +1,212 @@
> +/*
> + * (C) Copyright 2012
> + * Pavel Herrmann <morpheus.ibis@gmail.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <part.h>
> +#include <ata.h>
> +#include <libata.h>
> +#include <errno.h>
> +#include <os.h>
> +#include <command.h>
> +#include <malloc.h>
> +
> +static const char revision[] = "0.0";
> +static const char vendor[] = "SATA loopback";
> +
> +/* this will be auto-initialized to NULLs */
> +static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> +
> +extern block_dev_desc_t sata_dev_desc[];
> +
> +static inline int range_check(int dev)
> +{
> + return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
> +}
> +
> +int init_sata(int dev)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd, old_fd;
> +
> + if (range_check(dev)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + if (filenames[dev])
> + fd = os_open(filenames[dev], OS_O_RDWR);
> + else
> + fd = -1;
> +
> + old_fd = (long) pdev->priv;
> + /* This is ugly, but saves allocation for 1 int. */
> + pdev->priv = (void *) (long) fd;
> +
> + if ((old_fd > 2) || (old_fd < 0))
> + os_close(old_fd);
> +
> + return 0;
> +}
> +
> +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_read(fd, buffer, length_byte);
> +
> + return retval / ATA_SECT_SIZE;
> +}
> +
> +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> *buffer) +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_write(fd, buffer, length_byte);
> +
> + return retval / ATA_SECT_SIZE;
> +}
> +
> +int scan_sata(int dev)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + int namelen;
> + char *filename;
> + lbaint_t bytes = 0;
> +
> + if (range_check(dev)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + if (filenames[dev])
> + filename = filenames[dev];
> + else
> + filename = "";
> +
> + memcpy(pdev->vendor, vendor, sizeof(vendor));
> + memcpy(pdev->revision, revision, sizeof(revision));
> + namelen = strlen(filename);
> +
> + if (namelen > 20) {
> + /* take the last 17 chars, prepend them with "..." */
> + memcpy(pdev->product, "...", 3);
> + memcpy(pdev->product+3, filename + (namelen - 17), 17);
> + } else
> + memcpy(pdev->product, filename, namelen);
> +
> + pdev->product[20] = 0;
> +
> + bytes = os_lseek(fd, 0, OS_SEEK_END);
> + if (bytes != -1) {
> + pdev->type = DEV_TYPE_HARDDISK;
> + pdev->blksz = ATA_SECT_SIZE;
> + pdev->lun = 0;
> + pdev->lba = bytes/ATA_SECT_SIZE;
> + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> + " %lu\n", dev, filename, bytes, pdev->lba);
> + } else {
> + pdev->type = DEV_TYPE_HARDDISK;
> + pdev->blksz = ATA_SECT_SIZE;
> + pdev->lun = 0;
> + pdev->lba = 0;
> + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> + dev, filename);
> + }
> +
> + return 0;
> +}
> +
> +
> +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> + int dev = 0;
> +
> + switch (argc) {
> + case 0:
> + case 1:
> + return CMD_RET_USAGE;
> + case 2:
> + if (!strncmp(argv[1], "inf", 3)) {
> + for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
> + scan_sata(dev);
> + return 0;
> + }
> + return CMD_RET_USAGE;
> + case 3:
> + if (!strncmp(argv[1], "inf", 3)) {
> + dev = simple_strtoul(argv[2], NULL, 10);
> + return scan_sata(dev);
> + }
> + return CMD_RET_USAGE;
> + case 4:
> + if (!strncmp(argv[1], "load", 4)) {
> + dev = simple_strtoul(argv[2], NULL, 10);
> + /*
> + * init_sata() and scan_sata() do their own range
> + * check, however we need to explicitly do it here
> + * as well.
> + */
> + if (range_check(dev)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> + free(filenames[dev]);
> + filenames[dev] = strdup(argv[3]);
> + init_sata(dev);
> + scan_sata(dev);
> + /*
> + * Scan the partition table if we succeeded in loading
> + * the new loop file.
> + */
> + if (sata_dev_desc[dev].lba > 0)
> + init_part(&sata_dev_desc[dev]);
> +
> + return 0;
> + }
> + return CMD_RET_USAGE;
> + }
> + return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> + sata_loop, 4, 1, do_loop,
> + "SATA loopback",
> + "info - show info about all loop devices\n"
> + "sata_loop info devnum - show info about loop devnum\n"
> + "sata_loop load devnum file - load file from host FS into loop devnum"
> +);
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 0220386..a713430 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -93,4 +93,12 @@
> "stdout=serial\0" \
> "stderr=serial\0"
>
> +/* SATA loopback device */
> +#define CONFIG_CMD_SATA
> +#define CONFIG_SATA_LOOP
> +#define CONFIG_SYS_SATA_MAX_DEVICE 2
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_CMD_FAT
> +#define CONFIG_CMD_EXT2
> +
> #endif
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-06 23:29 ` Marek Vasut
@ 2012-09-07 9:19 ` Pavel Herrmann
2012-09-07 9:26 ` Marek Vasut
2012-09-13 22:31 ` Tom Rini
0 siblings, 2 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-07 9:19 UTC (permalink / raw)
To: u-boot
On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
>
> WARNING: externs should be avoided in .c files
> #141: FILE: drivers/block/sata_loopback.c:39:
> +extern block_dev_desc_t sata_dev_desc[];
>
> total: 0 errors, 1 warnings, 231 lines checked
>
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> MULTISTATEMENT_MACRO_USE_DO_WHILE
Yes, i know about that, and chose to ignore it.
i will not create a header file for a single line that checkpatch doesnt like
(with just a warning), especially when other drivers have such a line in them
as well.
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
> > ---
> >
> > Changes for v4:
> > checkpatch fixes
> > use NULLs instead of ""
> > extend sata_loop command
> >
> > Changes for v3:
> > introduce sata_loop command
> >
> > Changes for v2:
> > split sandbox config off into separate patch (2/2)
> > rename file to signify exported API
> > style fixes
> > show end of long filenames rather than beginning
> > check for lseek errors to indicate non-regular file
> >
> > drivers/block/Makefile | 1 +
> > drivers/block/sata_loopback.c | 212
> >
> > ++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h |
> >
> > 8 ++
> >
> > 3 files changed, 221 insertions(+)
> > create mode 100644 drivers/block/sata_loopback.c
> >
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> >
> > COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> >
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> >
> > COBJS := $(COBJS-y)
> > SRCS := $(COBJS:.o=.c)
> >
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..600fbdc
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,212 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.ibis@gmail.com>
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <part.h>
> > +#include <ata.h>
> > +#include <libata.h>
> > +#include <errno.h>
> > +#include <os.h>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +/* this will be auto-initialized to NULLs */
> > +static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +static inline int range_check(int dev)
> > +{
> > + return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
> > +}
> > +
> > +int init_sata(int dev)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd, old_fd;
> > +
> > + if (range_check(dev)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > +
> > + if (filenames[dev])
> > + fd = os_open(filenames[dev], OS_O_RDWR);
> > + else
> > + fd = -1;
> > +
> > + old_fd = (long) pdev->priv;
> > + /* This is ugly, but saves allocation for 1 int. */
> > + pdev->priv = (void *) (long) fd;
> > +
> > + if ((old_fd > 2) || (old_fd < 0))
> > + os_close(old_fd);
> > +
> > + return 0;
> > +}
> > +
> > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > + lbaint_t retval;
> > +
> > + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > + return -1;
> > +
> > + retval = os_read(fd, buffer, length_byte);
> > +
> > + return retval / ATA_SECT_SIZE;
> > +}
> > +
> > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > *buffer) +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > + lbaint_t retval;
> > +
> > + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > + return -1;
> > +
> > + retval = os_write(fd, buffer, length_byte);
> > +
> > + return retval / ATA_SECT_SIZE;
> > +}
> > +
> > +int scan_sata(int dev)
> > +{
> > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > + int fd = (long) pdev->priv;
> > + int namelen;
> > + char *filename;
> > + lbaint_t bytes = 0;
> > +
> > + if (range_check(dev)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > +
> > + if (filenames[dev])
> > + filename = filenames[dev];
> > + else
> > + filename = "";
> > +
> > + memcpy(pdev->vendor, vendor, sizeof(vendor));
> > + memcpy(pdev->revision, revision, sizeof(revision));
> > + namelen = strlen(filename);
> > +
> > + if (namelen > 20) {
> > + /* take the last 17 chars, prepend them with "..." */
> > + memcpy(pdev->product, "...", 3);
> > + memcpy(pdev->product+3, filename + (namelen - 17), 17);
> > + } else
> > + memcpy(pdev->product, filename, namelen);
> > +
> > + pdev->product[20] = 0;
> > +
> > + bytes = os_lseek(fd, 0, OS_SEEK_END);
> > + if (bytes != -1) {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = bytes/ATA_SECT_SIZE;
> > + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > + " %lu\n", dev, filename, bytes, pdev->lba);
> > + } else {
> > + pdev->type = DEV_TYPE_HARDDISK;
> > + pdev->blksz = ATA_SECT_SIZE;
> > + pdev->lun = 0;
> > + pdev->lba = 0;
> > + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > + dev, filename);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > + int dev = 0;
> > +
> > + switch (argc) {
> > + case 0:
> > + case 1:
> > + return CMD_RET_USAGE;
> > + case 2:
> > + if (!strncmp(argv[1], "inf", 3)) {
> > + for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
> > + scan_sata(dev);
> > + return 0;
> > + }
> > + return CMD_RET_USAGE;
> > + case 3:
> > + if (!strncmp(argv[1], "inf", 3)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
> > + return scan_sata(dev);
> > + }
> > + return CMD_RET_USAGE;
> > + case 4:
> > + if (!strncmp(argv[1], "load", 4)) {
> > + dev = simple_strtoul(argv[2], NULL, 10);
> > + /*
> > + * init_sata() and scan_sata() do their own range
> > + * check, however we need to explicitly do it here
> > + * as well.
> > + */
> > + if (range_check(dev)) {
> > + printf("File index %d is out of range.\n", dev);
> > + return -EINVAL;
> > + }
> > + free(filenames[dev]);
> > + filenames[dev] = strdup(argv[3]);
> > + init_sata(dev);
> > + scan_sata(dev);
> > + /*
> > + * Scan the partition table if we succeeded in loading
> > + * the new loop file.
> > + */
> > + if (sata_dev_desc[dev].lba > 0)
> > + init_part(&sata_dev_desc[dev]);
> > +
> > + return 0;
> > + }
> > + return CMD_RET_USAGE;
> > + }
> > + return CMD_RET_USAGE;
> > +}
> > +
> > +U_BOOT_CMD(
> > + sata_loop, 4, 1, do_loop,
> > + "SATA loopback",
> > + "info - show info about all loop devices\n"
> > + "sata_loop info devnum - show info about loop devnum\n"
> > + "sata_loop load devnum file - load file from host FS into loop devnum"
> > +);
> > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> > index 0220386..a713430 100644
> > --- a/include/configs/sandbox.h
> > +++ b/include/configs/sandbox.h
> > @@ -93,4 +93,12 @@
> >
> > "stdout=serial\0" \
> > "stderr=serial\0"
> >
> > +/* SATA loopback device */
> > +#define CONFIG_CMD_SATA
> > +#define CONFIG_SATA_LOOP
> > +#define CONFIG_SYS_SATA_MAX_DEVICE 2
> > +#define CONFIG_DOS_PARTITION
> > +#define CONFIG_CMD_FAT
> > +#define CONFIG_CMD_EXT2
> > +
> >
> > #endif
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-07 9:19 ` Pavel Herrmann
@ 2012-09-07 9:26 ` Marek Vasut
2012-09-07 9:38 ` Pavel Herrmann
2012-09-13 22:31 ` Tom Rini
1 sibling, 1 reply; 43+ messages in thread
From: Marek Vasut @ 2012-09-07 9:26 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > > This driver uses files as block devices, can be used for testing disk
> > > operations on sandbox.
> > > A new command "sata_loop" is introduced to load files in runtime.
> >
> > WARNING: externs should be avoided in .c files
> > #141: FILE: drivers/block/sata_loopback.c:39:
> > +extern block_dev_desc_t sata_dev_desc[];
> >
> > total: 0 errors, 1 warnings, 231 lines checked
> >
> > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > MULTISTATEMENT_MACRO_USE_DO_WHILE
>
> Yes, i know about that, and chose to ignore it.
Sorry, this is not how it works.
> i will not create a header file for a single line that checkpatch doesnt
> like (with just a warning), especially when other drivers have such a line
> in them as well.
So, implement proper accessor for this list? Or export the structure through
some sata-related header? How does other do it?
> > > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > > CC: Marek Vasut <marex@denx.de>
> > > CC: Mike Frysinger <vapier@gentoo.org>
> > > ---
> > >
> > > Changes for v4:
> > > checkpatch fixes
> > > use NULLs instead of ""
> > > extend sata_loop command
> > >
> > > Changes for v3:
> > > introduce sata_loop command
> > >
> > > Changes for v2:
> > > split sandbox config off into separate patch (2/2)
> > > rename file to signify exported API
> > > style fixes
> > > show end of long filenames rather than beginning
> > > check for lseek errors to indicate non-regular file
> > >
> > > drivers/block/Makefile | 1 +
> > > drivers/block/sata_loopback.c | 212
> > >
> > > ++++++++++++++++++++++++++++++++++++++++++ include/configs/sandbox.h
> > > |
> > >
> > > 8 ++
> > >
> > > 3 files changed, 221 insertions(+)
> > > create mode 100644 drivers/block/sata_loopback.c
> > >
> > > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > > index f1ebdcc..c95651a 100644
> > > --- a/drivers/block/Makefile
> > > +++ b/drivers/block/Makefile
> > > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > >
> > > COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > > COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > >
> > > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > >
> > > COBJS := $(COBJS-y)
> > > SRCS := $(COBJS:.o=.c)
> > >
> > > diff --git a/drivers/block/sata_loopback.c
> > > b/drivers/block/sata_loopback.c new file mode 100644
> > > index 0000000..600fbdc
> > > --- /dev/null
> > > +++ b/drivers/block/sata_loopback.c
> > > @@ -0,0 +1,212 @@
> > > +/*
> > > + * (C) Copyright 2012
> > > + * Pavel Herrmann <morpheus.ibis@gmail.com>
> > > + *
> > > + * See file CREDITS for list of people who contributed to this
> > > + * project.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License as
> > > + * published by the Free Software Foundation; either version 2 of
> > > + * the License, or (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program; if not, write to the Free Software
> > > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > > + * MA 02111-1307 USA
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <part.h>
> > > +#include <ata.h>
> > > +#include <libata.h>
> > > +#include <errno.h>
> > > +#include <os.h>
> > > +#include <command.h>
> > > +#include <malloc.h>
> > > +
> > > +static const char revision[] = "0.0";
> > > +static const char vendor[] = "SATA loopback";
> > > +
> > > +/* this will be auto-initialized to NULLs */
> > > +static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > > +
> > > +extern block_dev_desc_t sata_dev_desc[];
> > > +
> > > +static inline int range_check(int dev)
> > > +{
> > > + return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
> > > +}
> > > +
> > > +int init_sata(int dev)
> > > +{
> > > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > > + int fd, old_fd;
> > > +
> > > + if (range_check(dev)) {
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (filenames[dev])
> > > + fd = os_open(filenames[dev], OS_O_RDWR);
> > > + else
> > > + fd = -1;
> > > +
> > > + old_fd = (long) pdev->priv;
> > > + /* This is ugly, but saves allocation for 1 int. */
> > > + pdev->priv = (void *) (long) fd;
> > > +
> > > + if ((old_fd > 2) || (old_fd < 0))
> > > + os_close(old_fd);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer)
> > > +{
> > > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > > + int fd = (long) pdev->priv;
> > > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > > + lbaint_t retval;
> > > +
> > > + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > > + return -1;
> > > +
> > > + retval = os_read(fd, buffer, length_byte);
> > > +
> > > + return retval / ATA_SECT_SIZE;
> > > +}
> > > +
> > > +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> > > *buffer) +{
> > > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > > + int fd = (long) pdev->priv;
> > > + lbaint_t start_byte = ATA_SECT_SIZE * start;
> > > + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> > > + lbaint_t retval;
> > > +
> > > + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> > > + return -1;
> > > +
> > > + retval = os_write(fd, buffer, length_byte);
> > > +
> > > + return retval / ATA_SECT_SIZE;
> > > +}
> > > +
> > > +int scan_sata(int dev)
> > > +{
> > > + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > > + int fd = (long) pdev->priv;
> > > + int namelen;
> > > + char *filename;
> > > + lbaint_t bytes = 0;
> > > +
> > > + if (range_check(dev)) {
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (filenames[dev])
> > > + filename = filenames[dev];
> > > + else
> > > + filename = "";
> > > +
> > > + memcpy(pdev->vendor, vendor, sizeof(vendor));
> > > + memcpy(pdev->revision, revision, sizeof(revision));
> > > + namelen = strlen(filename);
> > > +
> > > + if (namelen > 20) {
> > > + /* take the last 17 chars, prepend them with "..." */
> > > + memcpy(pdev->product, "...", 3);
> > > + memcpy(pdev->product+3, filename + (namelen - 17), 17);
> > > + } else
> > > + memcpy(pdev->product, filename, namelen);
> > > +
> > > + pdev->product[20] = 0;
> > > +
> > > + bytes = os_lseek(fd, 0, OS_SEEK_END);
> > > + if (bytes != -1) {
> > > + pdev->type = DEV_TYPE_HARDDISK;
> > > + pdev->blksz = ATA_SECT_SIZE;
> > > + pdev->lun = 0;
> > > + pdev->lba = bytes/ATA_SECT_SIZE;
> > > + printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > > + " %lu\n", dev, filename, bytes, pdev->lba);
> > > + } else {
> > > + pdev->type = DEV_TYPE_HARDDISK;
> > > + pdev->blksz = ATA_SECT_SIZE;
> > > + pdev->lun = 0;
> > > + pdev->lba = 0;
> > > + printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > > + dev, filename);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +
> > > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > > +{
> > > + int dev = 0;
> > > +
> > > + switch (argc) {
> > > + case 0:
> > > + case 1:
> > > + return CMD_RET_USAGE;
> > > + case 2:
> > > + if (!strncmp(argv[1], "inf", 3)) {
> > > + for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
> > > + scan_sata(dev);
> > > + return 0;
> > > + }
> > > + return CMD_RET_USAGE;
> > > + case 3:
> > > + if (!strncmp(argv[1], "inf", 3)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> > > + return scan_sata(dev);
> > > + }
> > > + return CMD_RET_USAGE;
> > > + case 4:
> > > + if (!strncmp(argv[1], "load", 4)) {
> > > + dev = simple_strtoul(argv[2], NULL, 10);
> > > + /*
> > > + * init_sata() and scan_sata() do their own range
> > > + * check, however we need to explicitly do it here
> > > + * as well.
> > > + */
> > > + if (range_check(dev)) {
> > > + printf("File index %d is out of range.\n", dev);
> > > + return -EINVAL;
> > > + }
> > > + free(filenames[dev]);
> > > + filenames[dev] = strdup(argv[3]);
> > > + init_sata(dev);
> > > + scan_sata(dev);
> > > + /*
> > > + * Scan the partition table if we succeeded in loading
> > > + * the new loop file.
> > > + */
> > > + if (sata_dev_desc[dev].lba > 0)
> > > + init_part(&sata_dev_desc[dev]);
> > > +
> > > + return 0;
> > > + }
> > > + return CMD_RET_USAGE;
> > > + }
> > > + return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > + sata_loop, 4, 1, do_loop,
> > > + "SATA loopback",
> > > + "info - show info about all loop devices\n"
> > > + "sata_loop info devnum - show info about loop devnum\n"
> > > + "sata_loop load devnum file - load file from host FS into loop
> > > devnum" +);
> > > diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> > > index 0220386..a713430 100644
> > > --- a/include/configs/sandbox.h
> > > +++ b/include/configs/sandbox.h
> > > @@ -93,4 +93,12 @@
> > >
> > > "stdout=serial\0" \
> > > "stderr=serial\0"
> > >
> > > +/* SATA loopback device */
> > > +#define CONFIG_CMD_SATA
> > > +#define CONFIG_SATA_LOOP
> > > +#define CONFIG_SYS_SATA_MAX_DEVICE 2
> > > +#define CONFIG_DOS_PARTITION
> > > +#define CONFIG_CMD_FAT
> > > +#define CONFIG_CMD_EXT2
> > > +
> > >
> > > #endif
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-07 9:26 ` Marek Vasut
@ 2012-09-07 9:38 ` Pavel Herrmann
2012-09-07 9:42 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-07 9:38 UTC (permalink / raw)
To: u-boot
On Friday 07 of September 2012 11:26:48 Marek Vasut wrote:
> Dear Pavel Herrmann,
>
> > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > >
> > > > This driver uses files as block devices, can be used for testing disk
> > > > operations on sandbox.
> > > > A new command "sata_loop" is introduced to load files in runtime.
> > >
> > > WARNING: externs should be avoided in .c files
> > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > +extern block_dev_desc_t sata_dev_desc[];
> > >
> > > total: 0 errors, 1 warnings, 231 lines checked
> > >
> > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> >
> > Yes, i know about that, and chose to ignore it.
>
> Sorry, this is not how it works.
>
> > i will not create a header file for a single line that checkpatch doesnt
> > like (with just a warning), especially when other drivers have such a line
> > in them as well.
>
> So, implement proper accessor for this list? Or export the structure through
> some sata-related header? How does other do it?
other drivers have the same line that i do, therefore ignore the checkpatch
warning, same as i do
see sata_dwc.c, ata_piix.c, fsl_sata.c, sata_sil3114.c
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-07 9:38 ` Pavel Herrmann
@ 2012-09-07 9:42 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-07 9:42 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> On Friday 07 of September 2012 11:26:48 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > > Dear Pavel Herrmann,
> > > >
> > > > > This driver uses files as block devices, can be used for testing
> > > > > disk operations on sandbox.
> > > > > A new command "sata_loop" is introduced to load files in runtime.
> > > >
> > > > WARNING: externs should be avoided in .c files
> > > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > > +extern block_dev_desc_t sata_dev_desc[];
> > > >
> > > > total: 0 errors, 1 warnings, 231 lines checked
> > > >
> > > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> > >
> > > Yes, i know about that, and chose to ignore it.
> >
> > Sorry, this is not how it works.
> >
> > > i will not create a header file for a single line that checkpatch
> > > doesnt like (with just a warning), especially when other drivers have
> > > such a line in them as well.
> >
> > So, implement proper accessor for this list? Or export the structure
> > through some sata-related header? How does other do it?
>
> other drivers have the same line that i do, therefore ignore the checkpatch
> warning, same as i do
Oh god damned! This is definitelly not the way to go!
NAK! NAK! NAK!! NAK!!! NAK!!!!! NAK!!!!!!!! ... you get the idea ;-)
> see sata_dwc.c, ata_piix.c, fsl_sata.c, sata_sil3114.c
Sad state of codebase, thanks for pointing out the obvious :-( You know, now
that you pointed it out, you also know what to do :-)
Besides, you'd have to do it anyway, so let's do it now.
> Pavel Herrmann
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-07 9:19 ` Pavel Herrmann
2012-09-07 9:26 ` Marek Vasut
@ 2012-09-13 22:31 ` Tom Rini
2012-09-16 11:49 ` Pavel Herrmann
1 sibling, 1 reply; 43+ messages in thread
From: Tom Rini @ 2012-09-13 22:31 UTC (permalink / raw)
To: u-boot
On Fri, Sep 07, 2012 at 11:19:03AM +0200, Pavel Herrmann wrote:
> On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > Dear Pavel Herrmann,
> >
> > > This driver uses files as block devices, can be used for testing disk
> > > operations on sandbox.
> > > A new command "sata_loop" is introduced to load files in runtime.
> >
> > WARNING: externs should be avoided in .c files
> > #141: FILE: drivers/block/sata_loopback.c:39:
> > +extern block_dev_desc_t sata_dev_desc[];
> >
> > total: 0 errors, 1 warnings, 231 lines checked
> >
> > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > MULTISTATEMENT_MACRO_USE_DO_WHILE
>
> Yes, i know about that, and chose to ignore it.
> i will not create a header file for a single line that checkpatch doesnt like
> (with just a warning), especially when other drivers have such a line in them
> as well.
Please move your extern into <sata.h>, as the starting point before
merge. If you could also go and fix:
$ git grep -l sata_dev_desc
common/cmd_sata.c
drivers/block/ata_piix.c
drivers/block/dwc_ahsata.c
drivers/block/dwc_ahsata.h
drivers/block/fsl_sata.c
drivers/block/pata_bfin.c
drivers/block/pata_bfin.h
drivers/block/sata_dwc.c
drivers/block/sata_sil.c
drivers/block/sata_sil.h
drivers/block/sata_sil3114.c
To use <sata.h> (some already do) instead of their own private extern
I'd greatly appreciate it.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120913/62ef6192/attachment.pgp>
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v4 1/2] Loop block device for sandbox
2012-09-13 22:31 ` Tom Rini
@ 2012-09-16 11:49 ` Pavel Herrmann
0 siblings, 0 replies; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-16 11:49 UTC (permalink / raw)
To: u-boot
Hi
On Thursday 13 September 2012 15:31:39 Tom Rini wrote:
> On Fri, Sep 07, 2012 at 11:19:03AM +0200, Pavel Herrmann wrote:
> > On Friday 07 of September 2012 01:29:55 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > >
> > > > This driver uses files as block devices, can be used for testing disk
> > > > operations on sandbox.
> > > > A new command "sata_loop" is introduced to load files in runtime.
> > >
> > > WARNING: externs should be avoided in .c files
> > > #141: FILE: drivers/block/sata_loopback.c:39:
> > > +extern block_dev_desc_t sata_dev_desc[];
> > >
> > > total: 0 errors, 1 warnings, 231 lines checked
> > >
> > > NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX
> > > MULTISTATEMENT_MACRO_USE_DO_WHILE
> >
> > Yes, i know about that, and chose to ignore it.
> > i will not create a header file for a single line that checkpatch doesnt
> > like (with just a warning), especially when other drivers have such a
> > line in them as well.
>
> Please move your extern into <sata.h>, as the starting point before
> merge. If you could also go and fix:
> $ git grep -l sata_dev_desc
> common/cmd_sata.c
> drivers/block/ata_piix.c
> drivers/block/dwc_ahsata.c
> drivers/block/dwc_ahsata.h
> drivers/block/fsl_sata.c
> drivers/block/pata_bfin.c
> drivers/block/pata_bfin.h
> drivers/block/sata_dwc.c
> drivers/block/sata_sil.c
> drivers/block/sata_sil.h
> drivers/block/sata_sil3114.c
>
> To use <sata.h> (some already do) instead of their own private extern
> I'd greatly appreciate it.
I dont think <sata.h> is the place to put this, as it is a "consumer-facing"
header. rather create some common sata header in drivers/block
Pavel Herrmann
^ permalink raw reply [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v5 1/2] Loop block device for sandbox
2012-09-06 12:31 ` [U-Boot] [PATCH v4 " Pavel Herrmann
2012-09-06 23:29 ` Marek Vasut
@ 2012-09-16 11:58 ` Pavel Herrmann
2012-09-28 9:21 ` [U-Boot] [PATCH v6 " Pavel Herrmann
1 sibling, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-16 11:58 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v5:
use common sata extern header - this requires "[PATCH] Fix checkpatch warnings
about externs in *.c" to be merged first
make sure we have valid callbacks before scanning partitions
Changes for v4:
checkpatch fixes
use NULLs instead of ""
extend sata_loop command
Changes for v3:
introduce sata_loop command
Changes for v2:
split sandbox config off into separate patch (2/2)
rename file to signify exported API
style fixes
show end of long filenames rather than beginning
check for lseek errors to indicate non-regular file
drivers/block/Makefile | 1 +
drivers/block/sata_loopback.c | 213 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 214 insertions(+)
create mode 100644 drivers/block/sata_loopback.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index f1ebdcc..c95651a 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
COBJS-$(CONFIG_IDE_SIL680) += sil680.o
COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
COBJS-$(CONFIG_SYSTEMACE) += systemace.o
+COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..eb3103b
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,213 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <libata.h>
+#include <errno.h>
+#include <os.h>
+#include <command.h>
+#include <malloc.h>
+#include "sata_extern.h"
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+/* this will be auto-initialized to NULLs */
+static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+static inline int range_check(int dev)
+{
+ return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
+}
+
+int init_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd, old_fd;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ else
+ fd = -1;
+
+ old_fd = (long) pdev->priv;
+ /* This is ugly, but saves allocation for 1 int. */
+ pdev->priv = (void *) (long) fd;
+
+ if ((old_fd > 2) || (old_fd < 0))
+ os_close(old_fd);
+
+ return 0;
+}
+
+ulong sata_read(int dev, ulong start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_read(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+ulong sata_write(int dev, ulong start, lbaint_t blkcnt, const void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_write(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ int namelen;
+ char *filename;
+ lbaint_t bytes = 0;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ filename = filenames[dev];
+ else
+ filename = "";
+
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = strlen(filename);
+
+ if (namelen > 20) {
+ /* take the last 17 chars, prepend them with "..." */
+ memcpy(pdev->product, "...", 3);
+ memcpy(pdev->product+3, filename + (namelen - 17), 17);
+ } else
+ memcpy(pdev->product, filename, namelen);
+
+ pdev->product[20] = 0;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ if (bytes != -1) {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ pdev->block_read = sata_read;
+ pdev->block_write = sata_write;
+ printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
+ " %lu\n", dev, filename, bytes, pdev->lba);
+ } else {
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->lba = 0;
+ printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
+ dev, filename);
+ }
+
+ return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int dev = 0;
+
+ switch (argc) {
+ case 0:
+ case 1:
+ return CMD_RET_USAGE;
+ case 2:
+ if (!strncmp(argv[1], "inf", 3)) {
+ for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
+ scan_sata(dev);
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ case 3:
+ if (!strncmp(argv[1], "inf", 3)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ return scan_sata(dev);
+ }
+ return CMD_RET_USAGE;
+ case 4:
+ if (!strncmp(argv[1], "load", 4)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ /*
+ * init_sata() and scan_sata() do their own range
+ * check, however we need to explicitly do it here
+ * as well.
+ */
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+ free(filenames[dev]);
+ filenames[dev] = strdup(argv[3]);
+ init_sata(dev);
+ scan_sata(dev);
+ /*
+ * Scan the partition table if we succeeded in loading
+ * the new loop file.
+ */
+ if (sata_dev_desc[dev].lba > 0)
+ init_part(&sata_dev_desc[dev]);
+
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ }
+ return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+ sata_loop, 4, 1, do_loop,
+ "SATA loopback",
+ "info - show info about all loop devices\n"
+ "sata_loop info devnum - show info about loop devnum\n"
+ "sata_loop load devnum file - load file from host FS into loop devnum"
+);
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v6 1/2] Loop block device for sandbox
2012-09-16 11:58 ` [U-Boot] [PATCH v5 " Pavel Herrmann
@ 2012-09-28 9:21 ` Pavel Herrmann
2012-09-28 18:22 ` Marek Vasut
0 siblings, 1 reply; 43+ messages in thread
From: Pavel Herrmann @ 2012-09-28 9:21 UTC (permalink / raw)
To: u-boot
This driver uses files as block devices, can be used for testing disk
operations on sandbox.
A new command "sata_loop" is introduced to load files in runtime.
Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
---
Changes for v6:
sync with new version of "Fix checkpatch warnings about externs in *.c"
set all constant disk properties even on not connected drivers
Changes for v5:
use common sata extern header - this requires "[PATCH] Fix checkpatch warnings
about externs in *.c" to be merged first
make sure we have valid callbacks before scanning partitions
Changes for v4:
checkpatch fixes
use NULLs instead of ""
extend sata_loop command
Changes for v3:
introduce sata_loop command
Changes for v2:
split sandbox config off into separate patch (2/2)
rename file to signify exported API
style fixes
show end of long filenames rather than beginning
check for lseek errors to indicate non-regular file
drivers/block/sata_loopback.c | 209 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 209 insertions(+)
create mode 100644 drivers/block/sata_loopback.c
diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
new file mode 100644
index 0000000..b55b270
--- /dev/null
+++ b/drivers/block/sata_loopback.c
@@ -0,0 +1,209 @@
+/*
+ * (C) Copyright 2012
+ * Pavel Herrmann <morpheus.ibis@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <part.h>
+#include <ata.h>
+#include <sata.h>
+#include <errno.h>
+#include <os.h>
+#include <command.h>
+#include <malloc.h>
+
+static const char revision[] = "0.0";
+static const char vendor[] = "SATA loopback";
+
+/* this will be auto-initialized to NULLs */
+static char *filenames[CONFIG_SYS_SATA_MAX_DEVICE];
+
+static inline int range_check(int dev)
+{
+ return (dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE);
+}
+
+int init_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd, old_fd;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ fd = os_open(filenames[dev], OS_O_RDWR);
+ else
+ fd = -1;
+
+ old_fd = (long) pdev->priv;
+ /* This is ugly, but saves allocation for 1 int. */
+ pdev->priv = (void *) (long) fd;
+
+ if ((old_fd > 2) || (old_fd < 0))
+ os_close(old_fd);
+
+ return 0;
+}
+
+ulong sata_read(int dev, ulong start, lbaint_t blkcnt, void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_read(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+ulong sata_write(int dev, ulong start, lbaint_t blkcnt, const void *buffer)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ lbaint_t start_byte = ATA_SECT_SIZE * start;
+ lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
+ lbaint_t retval;
+
+ if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
+ return -1;
+
+ retval = os_write(fd, buffer, length_byte);
+
+ return retval / ATA_SECT_SIZE;
+}
+
+int scan_sata(int dev)
+{
+ block_dev_desc_t *pdev = &sata_dev_desc[dev];
+ int fd = (long) pdev->priv;
+ int namelen;
+ char *filename;
+ lbaint_t bytes = 0;
+
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+
+ if (filenames[dev])
+ filename = filenames[dev];
+ else
+ filename = "";
+
+ memcpy(pdev->vendor, vendor, sizeof(vendor));
+ memcpy(pdev->revision, revision, sizeof(revision));
+ namelen = strlen(filename);
+
+ if (namelen > 20) {
+ /* take the last 17 chars, prepend them with "..." */
+ memcpy(pdev->product, "...", 3);
+ memcpy(pdev->product+3, filename + (namelen - 17), 17);
+ } else
+ memcpy(pdev->product, filename, namelen);
+
+ pdev->product[20] = 0;
+ pdev->type = DEV_TYPE_HARDDISK;
+ pdev->blksz = ATA_SECT_SIZE;
+ pdev->lun = 0;
+ pdev->block_read = sata_read;
+ pdev->block_write = sata_write;
+
+ bytes = os_lseek(fd, 0, OS_SEEK_END);
+ if (bytes != -1) {
+ pdev->lba = bytes/ATA_SECT_SIZE;
+ printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
+ " %lu\n", dev, filename, bytes, pdev->lba);
+ } else {
+ pdev->lba = 0;
+ printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
+ dev, filename);
+ }
+
+ return 0;
+}
+
+
+int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+ int dev = 0;
+
+ switch (argc) {
+ case 0:
+ case 1:
+ return CMD_RET_USAGE;
+ case 2:
+ if (!strncmp(argv[1], "inf", 3)) {
+ for (dev = 0; dev < CONFIG_SYS_SATA_MAX_DEVICE; dev++)
+ scan_sata(dev);
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ case 3:
+ if (!strncmp(argv[1], "inf", 3)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ return scan_sata(dev);
+ }
+ return CMD_RET_USAGE;
+ case 4:
+ if (!strncmp(argv[1], "load", 4)) {
+ dev = simple_strtoul(argv[2], NULL, 10);
+ /*
+ * init_sata() and scan_sata() do their own range
+ * check, however we need to explicitly do it here
+ * as well.
+ */
+ if (range_check(dev)) {
+ printf("File index %d is out of range.\n", dev);
+ return -EINVAL;
+ }
+ free(filenames[dev]);
+ filenames[dev] = strdup(argv[3]);
+ init_sata(dev);
+ scan_sata(dev);
+ /*
+ * Scan the partition table if we succeeded in loading
+ * the new loop file.
+ */
+ if (sata_dev_desc[dev].lba > 0)
+ init_part(&sata_dev_desc[dev]);
+
+ return 0;
+ }
+ return CMD_RET_USAGE;
+ }
+ return CMD_RET_USAGE;
+}
+
+U_BOOT_CMD(
+ sata_loop, 4, 1, do_loop,
+ "SATA loopback",
+ "info - show info about all loop devices\n"
+ "sata_loop info devnum - show info about loop devnum\n"
+ "sata_loop load devnum file - load file from host FS into loop devnum"
+);
--
1.7.12
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [U-Boot] [PATCH v6 1/2] Loop block device for sandbox
2012-09-28 9:21 ` [U-Boot] [PATCH v6 " Pavel Herrmann
@ 2012-09-28 18:22 ` Marek Vasut
0 siblings, 0 replies; 43+ messages in thread
From: Marek Vasut @ 2012-09-28 18:22 UTC (permalink / raw)
To: u-boot
Dear Pavel Herrmann,
> This driver uses files as block devices, can be used for testing disk
> operations on sandbox.
> A new command "sata_loop" is introduced to load files in runtime.
The description might use a little bit of caressing here :)
[...]
> + if (os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
Use errno consistently ... so -EINVAL ? Or something else?
> + retval = os_read(fd, buffer, length_byte);
> +
> + return retval / ATA_SECT_SIZE;
> +}
[...]
> + if (!strncmp(argv[1], "load", 4)) {
if (strncmp())
return CMD_USAGE;
one indent less here ;-)
> + dev = simple_strtoul(argv[2], NULL, 10);
> + /*
> + * init_sata() and scan_sata() do their own range
> + * check, however we need to explicitly do it here
> + * as well.
> + */
> + if (range_check(dev)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> + free(filenames[dev]);
> + filenames[dev] = strdup(argv[3]);
> + init_sata(dev);
> + scan_sata(dev);
> + /*
> + * Scan the partition table if we succeeded in loading
> + * the new loop file.
> + */
> + if (sata_dev_desc[dev].lba > 0)
> + init_part(&sata_dev_desc[dev]);
> +
> + return 0;
> + }
> + return CMD_RET_USAGE;
> + }
> + return CMD_RET_USAGE;
> +}
> +
> +U_BOOT_CMD(
> + sata_loop, 4, 1, do_loop,
> + "SATA loopback",
> + "info - show info about all loop devices\n"
> + "sata_loop info devnum - show info about loop devnum\n"
> + "sata_loop load devnum file - load file from host FS into loop devnum"
> +);
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2012-09-28 18:22 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-29 15:46 [U-Boot] [PATCH] Loop block device for sandbox Pavel Herrmann
2012-08-29 15:48 ` Pavel Herrmann
2012-08-29 22:18 ` Marek Vasut
2012-08-30 17:14 ` Pavel Herrmann
2012-08-30 18:45 ` Marek Vasut
2012-08-30 19:07 ` Pavel Herrmann
2012-08-30 21:53 ` Marek Vasut
2012-08-31 9:09 ` Pavel Herrmann
2012-08-31 12:57 ` Marek Vasut
2012-08-31 14:25 ` Pavel Herrmann
2012-08-31 16:02 ` Marek Vasut
2012-08-31 17:56 ` Pavel Herrmann
2012-08-31 19:02 ` Marek Vasut
2012-09-01 13:19 ` [U-Boot] [PATCH v2 1/2] " Pavel Herrmann
2012-09-01 13:19 ` [U-Boot] [PATCH 2/2] Use loop block device in sandbox board Pavel Herrmann
2012-09-01 14:20 ` Marek Vasut
2012-09-03 17:24 ` Pavel Herrmann
2012-09-03 17:23 ` [U-Boot] [PATCH v2 " Pavel Herrmann
2012-09-03 16:49 ` [U-Boot] [PATCH v2 1/2] Loop block device for sandbox Marek Vasut
2012-09-03 17:31 ` Pavel Herrmann
2012-09-03 20:20 ` Marek Vasut
2012-09-05 11:16 ` [U-Boot] [PATCH v3 " Pavel Herrmann
2012-09-05 11:16 ` [U-Boot] [PATCH v3 2/2] Use loop block device in sandbox board Pavel Herrmann
2012-09-05 11:33 ` [U-Boot] [PATCH v3 1/2] Loop block device for sandbox Marek Vasut
2012-09-05 12:38 ` Pavel Herrmann
2012-09-05 12:48 ` Marek Vasut
2012-09-05 20:25 ` Pavel Herrmann
2012-09-06 1:08 ` Marek Vasut
2012-09-06 8:45 ` Pavel Herrmann
2012-09-06 8:48 ` Marek Vasut
2012-09-05 12:42 ` Pavel Herrmann
2012-09-05 12:52 ` Marek Vasut
2012-09-06 12:31 ` [U-Boot] [PATCH v4 " Pavel Herrmann
2012-09-06 23:29 ` Marek Vasut
2012-09-07 9:19 ` Pavel Herrmann
2012-09-07 9:26 ` Marek Vasut
2012-09-07 9:38 ` Pavel Herrmann
2012-09-07 9:42 ` Marek Vasut
2012-09-13 22:31 ` Tom Rini
2012-09-16 11:49 ` Pavel Herrmann
2012-09-16 11:58 ` [U-Boot] [PATCH v5 " Pavel Herrmann
2012-09-28 9:21 ` [U-Boot] [PATCH v6 " Pavel Herrmann
2012-09-28 18:22 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.