All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs
@ 2015-08-22 18:04 Hans de Goede
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use Hans de Goede
                   ` (4 more replies)
  0 siblings, 5 replies; 25+ messages in thread
From: Hans de Goede @ 2015-08-22 18:04 UTC (permalink / raw)
  To: u-boot

Hi Stephen & Scott,

As requested by Stephen here is a new version of my patch-set to add
mtd with ubi on top with ubifs on top support to distro_bootcmd, this
time using the generic filesystem related commands / code.

Scott, can you review the first three patches, and perhaps these should
also be merged through your tree ?

Stephen, can you review the last patch ? This can go in more or less
independetly (unless Scott nacks the entire generic fs approach), since
it does not do anything unless enabled.

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
@ 2015-08-22 18:04 ` Hans de Goede
  2015-08-25 11:00   ` Heiko Schocher
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 2/4] ubifs: Add functions " Hans de Goede
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-08-22 18:04 UTC (permalink / raw)
  To: u-boot

Modify the ubifs u-boot wrapper function prototypes for generic fs use,
and give them their own header file.

This is a preparation patch for adding ubifs support to the generic fs
code from fs/fs.c.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 common/cmd_ubifs.c    | 14 +++--------
 fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
 fs/ubifs/ubifs.h      |  6 +----
 include/ubifs_uboot.h | 29 +++++++++++++++++++++
 4 files changed, 89 insertions(+), 30 deletions(-)
 create mode 100644 include/ubifs_uboot.h

diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
index 8e9a4e5..0a3dd24 100644
--- a/common/cmd_ubifs.c
+++ b/common/cmd_ubifs.c
@@ -15,11 +15,10 @@
 #include <common.h>
 #include <config.h>
 #include <command.h>
-
-#include "../fs/ubifs/ubifs.h"
+#include <ubifs_uboot.h>
 
 static int ubifs_initialized;
-static int ubifs_mounted;
+int ubifs_mounted;
 
 static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
 				char * const argv[])
@@ -54,14 +53,7 @@ int ubifs_is_mounted(void)
 
 void cmd_ubifs_umount(void)
 {
-
-	if (ubifs_sb) {
-		printf("Unmounting UBIFS volume %s!\n",
-		       ((struct ubifs_info *)(ubifs_sb->s_fs_info))->vi.name);
-		ubifs_umount(ubifs_sb->s_fs_info);
-	}
-
-	ubifs_sb = NULL;
+	uboot_ubifs_umount();
 	ubifs_mounted = 0;
 	ubifs_initialized = 0;
 }
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 6dd6174..5af861c 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -568,7 +568,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
 	return 0;
 }
 
-int ubifs_ls(char *filename)
+int ubifs_ls(const char *filename)
 {
 	struct ubifs_info *c = ubifs_sb->s_fs_info;
 	struct file *file;
@@ -579,7 +579,7 @@ int ubifs_ls(char *filename)
 	int ret = 0;
 
 	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
-	inum = ubifs_findfile(ubifs_sb, filename);
+	inum = ubifs_findfile(ubifs_sb, (char *)filename);
 	if (!inum) {
 		ret = -1;
 		goto out;
@@ -785,7 +785,8 @@ error:
 	return err;
 }
 
-int ubifs_load(char *filename, u32 addr, u32 size)
+int ubifs_read(const char *filename, void *buf, loff_t offset,
+	       loff_t size, loff_t *actread)
 {
 	struct ubifs_info *c = ubifs_sb->s_fs_info;
 	unsigned long inum;
@@ -796,10 +797,18 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 	int count;
 	int last_block_size = 0;
 
+	*actread = 0;
+
+	if (offset & (PAGE_SIZE - 1)) {
+		printf("ubifs: Error offset must be a multple of %d\n",
+		       PAGE_SIZE);
+		return -1;
+	}
+
 	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
 	/* ubifs_findfile will resolve symlinks, so we know that we get
 	 * the real file here */
-	inum = ubifs_findfile(ubifs_sb, filename);
+	inum = ubifs_findfile(ubifs_sb, (char *)filename);
 	if (!inum) {
 		err = -1;
 		goto out;
@@ -815,19 +824,24 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 		goto out;
 	}
 
+	if (offset > inode->i_size) {
+		printf("ubifs: Error offset (%lld) > file-size (%lld)\n",
+		       offset, size);
+		err = -1;
+		goto put_inode;
+	}
+
 	/*
 	 * If no size was specified or if size bigger than filesize
 	 * set size to filesize
 	 */
-	if ((size == 0) || (size > inode->i_size))
-		size = inode->i_size;
+	if ((size == 0) || (size > (inode->i_size - offset)))
+		size = inode->i_size - offset;
 
 	count = (size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT;
-	printf("Loading file '%s' to addr 0x%08x with size %d (0x%08x)...\n",
-	       filename, addr, size, size);
 
-	page.addr = (void *)addr;
-	page.index = 0;
+	page.addr = buf;
+	page.index = offset / PAGE_SIZE;
 	page.inode = inode;
 	for (i = 0; i < count; i++) {
 		/*
@@ -844,16 +858,44 @@ int ubifs_load(char *filename, u32 addr, u32 size)
 		page.index++;
 	}
 
-	if (err)
+	if (err) {
 		printf("Error reading file '%s'\n", filename);
-	else {
-		setenv_hex("filesize", size);
-		printf("Done\n");
+		*actread = i * PAGE_SIZE;
+	} else {
+		*actread = size;
 	}
 
+put_inode:
 	ubifs_iput(inode);
 
 out:
 	ubi_close_volume(c->ubi);
 	return err;
 }
+
+/* Compat wrappers for common/cmd_ubifs.c */
+int ubifs_load(char *filename, u32 addr, u32 size)
+{
+	loff_t actread;
+	int err;
+
+	printf("Loading file '%s' to addr 0x%08x...\n", filename, addr);
+
+	err = ubifs_read(filename, (void *)addr, 0, size, &actread);
+	if (err == 0) {
+		setenv_hex("filesize", actread);
+		printf("Done\n");
+	}
+
+	return err;
+}
+
+void uboot_ubifs_umount(void)
+{
+	if (ubifs_sb) {
+		printf("Unmounting UBIFS volume %s!\n",
+		       ((struct ubifs_info *)(ubifs_sb->s_fs_info))->vi.name);
+		ubifs_umount(ubifs_sb->s_fs_info);
+		ubifs_sb = NULL;
+	}
+}
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index a51b237..225965c 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -34,6 +34,7 @@
 #include <asm/atomic.h>
 #include <asm-generic/atomic-long.h>
 #include <ubi_uboot.h>
+#include <ubifs_uboot.h>
 
 #include <linux/ctype.h>
 #include <linux/time.h>
@@ -2379,11 +2380,6 @@ int ubifs_decompress(const void *buf, int len, void *out, int *out_len,
 #include "key.h"
 
 #ifdef __UBOOT__
-/* these are used in cmd_ubifs.c */
-int ubifs_init(void);
-int uboot_ubifs_mount(char *vol_name);
 void ubifs_umount(struct ubifs_info *c);
-int ubifs_ls(char *dir_name);
-int ubifs_load(char *filename, u32 addr, u32 size);
 #endif
 #endif /* !__UBIFS_H__ */
diff --git a/include/ubifs_uboot.h b/include/ubifs_uboot.h
new file mode 100644
index 0000000..d10a909
--- /dev/null
+++ b/include/ubifs_uboot.h
@@ -0,0 +1,29 @@
+/*
+ * UBIFS u-boot wrapper functions header
+ *
+ * Copyright (C) 2006-2008 Nokia Corporation
+ *
+ * (C) Copyright 2008-2009
+ * Stefan Roese, DENX Software Engineering, sr at denx.de.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ *
+ * Authors: Artem Bityutskiy (???????? ?????)
+ *          Adrian Hunter
+ */
+
+#ifndef __UBIFS_UBOOT_H__
+#define __UBIFS_UBOOT_H__
+
+extern int ubifs_mounted;
+
+int ubifs_init(void);
+int uboot_ubifs_mount(char *vol_name);
+void uboot_ubifs_umount(void);
+int ubifs_load(char *filename, u32 addr, u32 size);
+
+int ubifs_ls(const char *dir_name);
+int ubifs_read(const char *filename, void *buf, loff_t offset,
+	       loff_t size, loff_t *actread);
+
+#endif /* __UBIFS_UBOOT_H__ */
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/4] ubifs: Add functions for generic fs use
  2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use Hans de Goede
@ 2015-08-22 18:04 ` Hans de Goede
  2015-08-25 11:02   ` Heiko Schocher
  2015-09-01 19:57   ` Stephen Warren
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support Hans de Goede
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 25+ messages in thread
From: Hans de Goede @ 2015-08-22 18:04 UTC (permalink / raw)
  To: u-boot

Implement the necessary functions for implementing generic fs support
for ubifs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 fs/ubifs/ubifs.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/ubifs_uboot.h |  4 ++++
 2 files changed, 66 insertions(+)

diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index 5af861c..89f1f2a 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -568,6 +568,22 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
 	return 0;
 }
 
+int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
+{
+	/* Check that ubifs is mounted and that we are not being a blkdev */
+	if (!ubifs_mounted) {
+		printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
+		return -1;
+	}
+
+	if (rbdd) {
+		printf("UBIFS cannot be used with normal block devices\n");
+		return -1;
+	}
+
+	return 0;
+}
+
 int ubifs_ls(const char *filename)
 {
 	struct ubifs_info *c = ubifs_sb->s_fs_info;
@@ -616,6 +632,48 @@ out:
 	return ret;
 }
 
+int ubifs_exists(const char *filename)
+{
+	struct ubifs_info *c = ubifs_sb->s_fs_info;
+	unsigned long inum;
+
+	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
+	inum = ubifs_findfile(ubifs_sb, (char *)filename);
+	ubi_close_volume(c->ubi);
+
+	return inum != 0;
+}
+
+int ubifs_size(const char *filename, loff_t *size)
+{
+	struct ubifs_info *c = ubifs_sb->s_fs_info;
+	unsigned long inum;
+	struct inode *inode;
+	int err = 0;
+
+	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
+
+	inum = ubifs_findfile(ubifs_sb, (char *)filename);
+	if (!inum) {
+		err = -1;
+		goto out;
+	}
+
+	inode = ubifs_iget(ubifs_sb, inum);
+	if (IS_ERR(inode)) {
+		printf("%s: Error reading inode %ld!\n", __func__, inum);
+		err = PTR_ERR(inode);
+		goto out;
+	}
+
+	*size = inode->i_size;
+
+	ubifs_iput(inode);
+out:
+	ubi_close_volume(c->ubi);
+	return err;
+}
+
 /*
  * ubifsload...
  */
@@ -873,6 +931,10 @@ out:
 	return err;
 }
 
+void ubifs_close(void)
+{
+}
+
 /* Compat wrappers for common/cmd_ubifs.c */
 int ubifs_load(char *filename, u32 addr, u32 size)
 {
diff --git a/include/ubifs_uboot.h b/include/ubifs_uboot.h
index d10a909..c600e38 100644
--- a/include/ubifs_uboot.h
+++ b/include/ubifs_uboot.h
@@ -22,8 +22,12 @@ int uboot_ubifs_mount(char *vol_name);
 void uboot_ubifs_umount(void);
 int ubifs_load(char *filename, u32 addr, u32 size);
 
+int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info);
 int ubifs_ls(const char *dir_name);
+int ubifs_exists(const char *filename);
+int ubifs_size(const char *filename, loff_t *size);
 int ubifs_read(const char *filename, void *buf, loff_t offset,
 	       loff_t size, loff_t *actread);
+void ubifs_close(void);
 
 #endif /* __UBIFS_UBOOT_H__ */
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support
  2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use Hans de Goede
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 2/4] ubifs: Add functions " Hans de Goede
@ 2015-08-22 18:04 ` Hans de Goede
  2015-08-25 11:20   ` Heiko Schocher
  2015-09-01 20:03   ` Stephen Warren
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
  2015-08-24 16:57 ` [U-Boot] [PATCH v2 0/4] " Scott Wood
  4 siblings, 2 replies; 25+ messages in thread
From: Hans de Goede @ 2015-08-22 18:04 UTC (permalink / raw)
  To: u-boot

Add generic fs support, so that commands like ls, load and test -e can be
used on ubifs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 disk/part.c  | 23 +++++++++++++++++++++++
 fs/fs.c      | 16 ++++++++++++++++
 include/fs.h |  1 +
 3 files changed, 40 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 43485c9..e1a8bde 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -10,6 +10,7 @@
 #include <ide.h>
 #include <malloc.h>
 #include <part.h>
+#include <ubifs_uboot.h>
 
 #undef	PART_DEBUG
 
@@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str,
 		return 0;
 	}
 
+#ifdef CONFIG_CMD_UBIFS
+	/*
+	 * Special-case ubi, ubi goes through a mtd, rathen then through
+	 * a regular block device.
+	 */
+	if (0 == strcmp(ifname, "ubi")) {
+		if (!ubifs_mounted) {
+			printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
+			return -1;
+		}
+
+		*dev_desc = NULL;
+		memset(info, 0, sizeof(*info));
+		strcpy((char *)info->type, BOOT_PART_TYPE);
+		strcpy((char *)info->name, "UBI");
+#ifdef CONFIG_PARTITION_UUIDS
+		info->uuid[0] = 0;
+#endif
+		return 0;
+	}
+#endif
+
 	/* If no dev_part_str, use bootdevice environment variable */
 	if (!dev_part_str || !strlen(dev_part_str) ||
 	    !strcmp(dev_part_str, "-"))
diff --git a/fs/fs.c b/fs/fs.c
index 827b143..b2d6a53 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -23,6 +23,7 @@
 #include <fat.h>
 #include <fs.h>
 #include <sandboxfs.h>
+#include <ubifs_uboot.h>
 #include <asm/io.h>
 #include <div64.h>
 #include <linux/math64.h>
@@ -157,6 +158,21 @@ static struct fstype_info fstypes[] = {
 		.uuid = fs_uuid_unsupported,
 	},
 #endif
+#ifdef CONFIG_CMD_UBIFS
+	{
+		.fstype = FS_TYPE_UBIFS,
+		.name = "ubifs",
+		.null_dev_desc_ok = true,
+		.probe = ubifs_set_blk_dev,
+		.close = ubifs_close,
+		.ls = ubifs_ls,
+		.exists = ubifs_exists,
+		.size = ubifs_size,
+		.read = ubifs_read,
+		.write = fs_write_unsupported,
+		.uuid = fs_uuid_unsupported,
+	},
+#endif
 	{
 		.fstype = FS_TYPE_ANY,
 		.name = "unsupported",
diff --git a/include/fs.h b/include/fs.h
index fd1e4ab..059a395 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -22,6 +22,7 @@
 #define FS_TYPE_FAT	1
 #define FS_TYPE_EXT	2
 #define FS_TYPE_SANDBOX	3
+#define FS_TYPE_UBIFS	4
 
 /*
  * Tell the fs layer which block device an partition to use for future
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs
  2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
                   ` (2 preceding siblings ...)
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support Hans de Goede
@ 2015-08-22 18:04 ` Hans de Goede
  2015-09-01 20:13   ` Stephen Warren
  2015-08-24 16:57 ` [U-Boot] [PATCH v2 0/4] " Scott Wood
  4 siblings, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-08-22 18:04 UTC (permalink / raw)
  To: u-boot

From: Roy Spliet <r.spliet@ultimaker.com>

Under the assumptions of having a UBI volume called boot, containing
a ubifs filesystem.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/config_distro_bootcmd.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 3a360ca4..2b36d80 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -72,6 +72,24 @@
 	BOOT_TARGET_DEVICES_references_MMC_without_CONFIG_CMD_MMC
 #endif
 
+#ifdef CONFIG_CMD_UBIFS
+#define BOOTENV_SHARED_UBIFS \
+	"ubifs_boot=" \
+		"if ubi part UBI && ubifsmount ubi${devnum}:boot; then "  \
+			"setenv devtype ubi; "                            \
+			"setenv bootpart 0; "                             \
+			"run scan_dev_for_boot; "                         \
+		"fi\0"
+#define BOOTENV_DEV_UBIFS	BOOTENV_DEV_BLKDEV
+#define BOOTENV_DEV_NAME_UBIFS	BOOTENV_DEV_NAME_BLKDEV
+#else
+#define BOOTENV_SHARED_UBIFS
+#define BOOTENV_DEV_UBIFS \
+	BOOT_TARGET_DEVICES_references_UBIFS_without_CONFIG_CMD_UBIFS
+#define BOOTENV_DEV_NAME_UBIFS \
+	BOOT_TARGET_DEVICES_references_UBIFS_without_CONFIG_CMD_UBIFS
+#endif
+
 #ifdef CONFIG_CMD_SATA
 #define BOOTENV_SHARED_SATA	BOOTENV_SHARED_BLKDEV(sata)
 #define BOOTENV_DEV_SATA	BOOTENV_DEV_BLKDEV
@@ -185,6 +203,7 @@
 	BOOTENV_SHARED_SATA \
 	BOOTENV_SHARED_SCSI \
 	BOOTENV_SHARED_IDE \
+	BOOTENV_SHARED_UBIFS \
 	"boot_prefixes=/ /boot/\0" \
 	"boot_scripts=boot.scr.uimg boot.scr\0" \
 	"boot_script_dhcp=boot.scr.uimg\0" \
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs
  2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
                   ` (3 preceding siblings ...)
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
@ 2015-08-24 16:57 ` Scott Wood
  2015-08-25  7:15   ` Hans de Goede
  4 siblings, 1 reply; 25+ messages in thread
From: Scott Wood @ 2015-08-24 16:57 UTC (permalink / raw)
  To: u-boot

On Sat, 2015-08-22 at 20:04 +0200, Hans de Goede wrote:
> Hi Stephen & Scott,
> 
> As requested by Stephen here is a new version of my patch-set to add
> mtd with ubi on top with ubifs on top support to distro_bootcmd, this
> time using the generic filesystem related commands / code.
> 
> Scott, can you review the first three patches, and perhaps these should
> also be merged through your tree ?

I don't see anything NAND-specific about them, nor am I particularly familiar 
with ubifs internals or U-Boot filesystem support...

Kyungmin Park and Heiko Schocher are listed as the UBI custodians.

-Scott

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs
  2015-08-24 16:57 ` [U-Boot] [PATCH v2 0/4] " Scott Wood
@ 2015-08-25  7:15   ` Hans de Goede
  2015-08-25  7:31     ` Heiko Schocher
  0 siblings, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-08-25  7:15 UTC (permalink / raw)
  To: u-boot

Hi,

On 24-08-15 18:57, Scott Wood wrote:
> On Sat, 2015-08-22 at 20:04 +0200, Hans de Goede wrote:
>> Hi Stephen & Scott,
>>
>> As requested by Stephen here is a new version of my patch-set to add
>> mtd with ubi on top with ubifs on top support to distro_bootcmd, this
>> time using the generic filesystem related commands / code.
>>
>> Scott, can you review the first three patches, and perhaps these should
>> also be merged through your tree ?
>
> I don't see anything NAND-specific about them, nor am I particularly familiar
> with ubifs internals or U-Boot filesystem support...
>
> Kyungmin Park and Heiko Schocher are listed as the UBI custodians.

My bad, since you are the mtd custodian, I assumed that you would be
maintaining ubi[fs] too, my mistake.

Heiko, Kyungmin (added to the Cc), can one of you please review the first
3 patches of this set. I think this is post v2015.10 material, once reviewed
I can merge it through the sunxi tree, or you can pick it up.

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs
  2015-08-25  7:15   ` Hans de Goede
@ 2015-08-25  7:31     ` Heiko Schocher
  0 siblings, 0 replies; 25+ messages in thread
From: Heiko Schocher @ 2015-08-25  7:31 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 25.08.2015 um 09:15 schrieb Hans de Goede:
> Hi,
>
> On 24-08-15 18:57, Scott Wood wrote:
>> On Sat, 2015-08-22 at 20:04 +0200, Hans de Goede wrote:
>>> Hi Stephen & Scott,
>>>
>>> As requested by Stephen here is a new version of my patch-set to add
>>> mtd with ubi on top with ubifs on top support to distro_bootcmd, this
>>> time using the generic filesystem related commands / code.
>>>
>>> Scott, can you review the first three patches, and perhaps these should
>>> also be merged through your tree ?
>>
>> I don't see anything NAND-specific about them, nor am I particularly familiar
>> with ubifs internals or U-Boot filesystem support...
>>
>> Kyungmin Park and Heiko Schocher are listed as the UBI custodians.
>
> My bad, since you are the mtd custodian, I assumed that you would be
> maintaining ubi[fs] too, my mistake.
>
> Heiko, Kyungmin (added to the Cc), can one of you please review the first
> 3 patches of this set. I think this is post v2015.10 material, once reviewed
> I can merge it through the sunxi tree, or you can pick it up.

I had it on my list, but as I thought (as you ;-) it is 2015.10
material I looked not deeper into it ... Sorry, I look into it ASAP.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use Hans de Goede
@ 2015-08-25 11:00   ` Heiko Schocher
  2015-08-25 11:32     ` Hans de Goede
  0 siblings, 1 reply; 25+ messages in thread
From: Heiko Schocher @ 2015-08-25 11:00 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 22.08.2015 um 20:04 schrieb Hans de Goede:
> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
> and give them their own header file.
>
> This is a preparation patch for adding ubifs support to the generic fs
> code from fs/fs.c.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   common/cmd_ubifs.c    | 14 +++--------
>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>   fs/ubifs/ubifs.h      |  6 +----
>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>   4 files changed, 89 insertions(+), 30 deletions(-)
>   create mode 100644 include/ubifs_uboot.h

Only one nitpick, beside of this, you can add my:

Reviewed-by: Heiko Schocher <hs@denx.de>

> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
> index 8e9a4e5..0a3dd24 100644
> --- a/common/cmd_ubifs.c
> +++ b/common/cmd_ubifs.c
> @@ -15,11 +15,10 @@
>   #include <common.h>
>   #include <config.h>
>   #include <command.h>
> -
> -#include "../fs/ubifs/ubifs.h"
> +#include <ubifs_uboot.h>
>
>   static int ubifs_initialized;
> -static int ubifs_mounted;
> +int ubifs_mounted;

later you add "extern int ubifs_mounted" in include/ubifs_uboot.h

Maybe you want to add a function, which returns the state
of this var and let the var static?

bye,
Heiko
>   static int do_ubifs_mount(cmd_tbl_t *cmdtp, int flag, int argc,
>   				char * const argv[])
> @@ -54,14 +53,7 @@ int ubifs_is_mounted(void)
>
>   void cmd_ubifs_umount(void)
>   {
> -
> -	if (ubifs_sb) {
> -		printf("Unmounting UBIFS volume %s!\n",
> -		       ((struct ubifs_info *)(ubifs_sb->s_fs_info))->vi.name);
> -		ubifs_umount(ubifs_sb->s_fs_info);
> -	}
> -
> -	ubifs_sb = NULL;
> +	uboot_ubifs_umount();
>   	ubifs_mounted = 0;
>   	ubifs_initialized = 0;
>   }
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 6dd6174..5af861c 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -568,7 +568,7 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
>   	return 0;
>   }
>
> -int ubifs_ls(char *filename)
> +int ubifs_ls(const char *filename)
>   {
>   	struct ubifs_info *c = ubifs_sb->s_fs_info;
>   	struct file *file;
> @@ -579,7 +579,7 @@ int ubifs_ls(char *filename)
>   	int ret = 0;
>
>   	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
> -	inum = ubifs_findfile(ubifs_sb, filename);
> +	inum = ubifs_findfile(ubifs_sb, (char *)filename);
>   	if (!inum) {
>   		ret = -1;
>   		goto out;
> @@ -785,7 +785,8 @@ error:
>   	return err;
>   }
>
> -int ubifs_load(char *filename, u32 addr, u32 size)
> +int ubifs_read(const char *filename, void *buf, loff_t offset,
> +	       loff_t size, loff_t *actread)
>   {
>   	struct ubifs_info *c = ubifs_sb->s_fs_info;
>   	unsigned long inum;
> @@ -796,10 +797,18 @@ int ubifs_load(char *filename, u32 addr, u32 size)
>   	int count;
>   	int last_block_size = 0;
>
> +	*actread = 0;
> +
> +	if (offset & (PAGE_SIZE - 1)) {
> +		printf("ubifs: Error offset must be a multple of %d\n",
> +		       PAGE_SIZE);
> +		return -1;
> +	}
> +
>   	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
>   	/* ubifs_findfile will resolve symlinks, so we know that we get
>   	 * the real file here */
> -	inum = ubifs_findfile(ubifs_sb, filename);
> +	inum = ubifs_findfile(ubifs_sb, (char *)filename);
>   	if (!inum) {
>   		err = -1;
>   		goto out;
> @@ -815,19 +824,24 @@ int ubifs_load(char *filename, u32 addr, u32 size)
>   		goto out;
>   	}
>
> +	if (offset > inode->i_size) {
> +		printf("ubifs: Error offset (%lld) > file-size (%lld)\n",
> +		       offset, size);
> +		err = -1;
> +		goto put_inode;
> +	}
> +
>   	/*
>   	 * If no size was specified or if size bigger than filesize
>   	 * set size to filesize
>   	 */
> -	if ((size == 0) || (size > inode->i_size))
> -		size = inode->i_size;
> +	if ((size == 0) || (size > (inode->i_size - offset)))
> +		size = inode->i_size - offset;
>
>   	count = (size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT;
> -	printf("Loading file '%s' to addr 0x%08x with size %d (0x%08x)...\n",
> -	       filename, addr, size, size);
>
> -	page.addr = (void *)addr;
> -	page.index = 0;
> +	page.addr = buf;
> +	page.index = offset / PAGE_SIZE;
>   	page.inode = inode;
>   	for (i = 0; i < count; i++) {
>   		/*
> @@ -844,16 +858,44 @@ int ubifs_load(char *filename, u32 addr, u32 size)
>   		page.index++;
>   	}
>
> -	if (err)
> +	if (err) {
>   		printf("Error reading file '%s'\n", filename);
> -	else {
> -		setenv_hex("filesize", size);
> -		printf("Done\n");
> +		*actread = i * PAGE_SIZE;
> +	} else {
> +		*actread = size;
>   	}
>
> +put_inode:
>   	ubifs_iput(inode);
>
>   out:
>   	ubi_close_volume(c->ubi);
>   	return err;
>   }
> +
> +/* Compat wrappers for common/cmd_ubifs.c */
> +int ubifs_load(char *filename, u32 addr, u32 size)
> +{
> +	loff_t actread;
> +	int err;
> +
> +	printf("Loading file '%s' to addr 0x%08x...\n", filename, addr);
> +
> +	err = ubifs_read(filename, (void *)addr, 0, size, &actread);
> +	if (err == 0) {
> +		setenv_hex("filesize", actread);
> +		printf("Done\n");
> +	}
> +
> +	return err;
> +}
> +
> +void uboot_ubifs_umount(void)
> +{
> +	if (ubifs_sb) {
> +		printf("Unmounting UBIFS volume %s!\n",
> +		       ((struct ubifs_info *)(ubifs_sb->s_fs_info))->vi.name);
> +		ubifs_umount(ubifs_sb->s_fs_info);
> +		ubifs_sb = NULL;
> +	}
> +}
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index a51b237..225965c 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -34,6 +34,7 @@
>   #include <asm/atomic.h>
>   #include <asm-generic/atomic-long.h>
>   #include <ubi_uboot.h>
> +#include <ubifs_uboot.h>
>
>   #include <linux/ctype.h>
>   #include <linux/time.h>
> @@ -2379,11 +2380,6 @@ int ubifs_decompress(const void *buf, int len, void *out, int *out_len,
>   #include "key.h"
>
>   #ifdef __UBOOT__
> -/* these are used in cmd_ubifs.c */
> -int ubifs_init(void);
> -int uboot_ubifs_mount(char *vol_name);
>   void ubifs_umount(struct ubifs_info *c);
> -int ubifs_ls(char *dir_name);
> -int ubifs_load(char *filename, u32 addr, u32 size);
>   #endif
>   #endif /* !__UBIFS_H__ */
> diff --git a/include/ubifs_uboot.h b/include/ubifs_uboot.h
> new file mode 100644
> index 0000000..d10a909
> --- /dev/null
> +++ b/include/ubifs_uboot.h
> @@ -0,0 +1,29 @@
> +/*
> + * UBIFS u-boot wrapper functions header
> + *
> + * Copyright (C) 2006-2008 Nokia Corporation
> + *
> + * (C) Copyright 2008-2009
> + * Stefan Roese, DENX Software Engineering, sr at denx.de.
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + *
> + * Authors: Artem Bityutskiy (???????? ?????)
> + *          Adrian Hunter
> + */
> +
> +#ifndef __UBIFS_UBOOT_H__
> +#define __UBIFS_UBOOT_H__
> +
> +extern int ubifs_mounted;
> +
> +int ubifs_init(void);
> +int uboot_ubifs_mount(char *vol_name);
> +void uboot_ubifs_umount(void);
> +int ubifs_load(char *filename, u32 addr, u32 size);
> +
> +int ubifs_ls(const char *dir_name);
> +int ubifs_read(const char *filename, void *buf, loff_t offset,
> +	       loff_t size, loff_t *actread);
> +
> +#endif /* __UBIFS_UBOOT_H__ */
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/4] ubifs: Add functions for generic fs use
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 2/4] ubifs: Add functions " Hans de Goede
@ 2015-08-25 11:02   ` Heiko Schocher
  2015-09-01 19:57   ` Stephen Warren
  1 sibling, 0 replies; 25+ messages in thread
From: Heiko Schocher @ 2015-08-25 11:02 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 22.08.2015 um 20:04 schrieb Hans de Goede:
> Implement the necessary functions for implementing generic fs support
> for ubifs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   fs/ubifs/ubifs.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/ubifs_uboot.h |  4 ++++
>   2 files changed, 66 insertions(+)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
>
> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
> index 5af861c..89f1f2a 100644
> --- a/fs/ubifs/ubifs.c
> +++ b/fs/ubifs/ubifs.c
> @@ -568,6 +568,22 @@ static unsigned long ubifs_findfile(struct super_block *sb, char *filename)
>   	return 0;
>   }
>
> +int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
> +{
> +	/* Check that ubifs is mounted and that we are not being a blkdev */
> +	if (!ubifs_mounted) {
> +		printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
> +		return -1;
> +	}
> +
> +	if (rbdd) {
> +		printf("UBIFS cannot be used with normal block devices\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   int ubifs_ls(const char *filename)
>   {
>   	struct ubifs_info *c = ubifs_sb->s_fs_info;
> @@ -616,6 +632,48 @@ out:
>   	return ret;
>   }
>
> +int ubifs_exists(const char *filename)
> +{
> +	struct ubifs_info *c = ubifs_sb->s_fs_info;
> +	unsigned long inum;
> +
> +	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
> +	inum = ubifs_findfile(ubifs_sb, (char *)filename);
> +	ubi_close_volume(c->ubi);
> +
> +	return inum != 0;
> +}
> +
> +int ubifs_size(const char *filename, loff_t *size)
> +{
> +	struct ubifs_info *c = ubifs_sb->s_fs_info;
> +	unsigned long inum;
> +	struct inode *inode;
> +	int err = 0;
> +
> +	c->ubi = ubi_open_volume(c->vi.ubi_num, c->vi.vol_id, UBI_READONLY);
> +
> +	inum = ubifs_findfile(ubifs_sb, (char *)filename);
> +	if (!inum) {
> +		err = -1;
> +		goto out;
> +	}
> +
> +	inode = ubifs_iget(ubifs_sb, inum);
> +	if (IS_ERR(inode)) {
> +		printf("%s: Error reading inode %ld!\n", __func__, inum);
> +		err = PTR_ERR(inode);
> +		goto out;
> +	}
> +
> +	*size = inode->i_size;
> +
> +	ubifs_iput(inode);
> +out:
> +	ubi_close_volume(c->ubi);
> +	return err;
> +}
> +
>   /*
>    * ubifsload...
>    */
> @@ -873,6 +931,10 @@ out:
>   	return err;
>   }
>
> +void ubifs_close(void)
> +{
> +}
> +
>   /* Compat wrappers for common/cmd_ubifs.c */
>   int ubifs_load(char *filename, u32 addr, u32 size)
>   {
> diff --git a/include/ubifs_uboot.h b/include/ubifs_uboot.h
> index d10a909..c600e38 100644
> --- a/include/ubifs_uboot.h
> +++ b/include/ubifs_uboot.h
> @@ -22,8 +22,12 @@ int uboot_ubifs_mount(char *vol_name);
>   void uboot_ubifs_umount(void);
>   int ubifs_load(char *filename, u32 addr, u32 size);
>
> +int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info);
>   int ubifs_ls(const char *dir_name);
> +int ubifs_exists(const char *filename);
> +int ubifs_size(const char *filename, loff_t *size);
>   int ubifs_read(const char *filename, void *buf, loff_t offset,
>   	       loff_t size, loff_t *actread);
> +void ubifs_close(void);
>
>   #endif /* __UBIFS_UBOOT_H__ */
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support Hans de Goede
@ 2015-08-25 11:20   ` Heiko Schocher
  2015-09-01 20:03   ` Stephen Warren
  1 sibling, 0 replies; 25+ messages in thread
From: Heiko Schocher @ 2015-08-25 11:20 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 22.08.2015 um 20:04 schrieb Hans de Goede:
> Add generic fs support, so that commands like ls, load and test -e can be
> used on ubifs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   disk/part.c  | 23 +++++++++++++++++++++++
>   fs/fs.c      | 16 ++++++++++++++++
>   include/fs.h |  1 +
>   3 files changed, 40 insertions(+)

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
>
> diff --git a/disk/part.c b/disk/part.c
> index 43485c9..e1a8bde 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -10,6 +10,7 @@
>   #include <ide.h>
>   #include <malloc.h>
>   #include <part.h>
> +#include <ubifs_uboot.h>
>
>   #undef	PART_DEBUG
>
> @@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str,
>   		return 0;
>   	}
>
> +#ifdef CONFIG_CMD_UBIFS
> +	/*
> +	 * Special-case ubi, ubi goes through a mtd, rathen then through
> +	 * a regular block device.
> +	 */
> +	if (0 == strcmp(ifname, "ubi")) {
> +		if (!ubifs_mounted) {
> +			printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
> +			return -1;
> +		}
> +
> +		*dev_desc = NULL;
> +		memset(info, 0, sizeof(*info));
> +		strcpy((char *)info->type, BOOT_PART_TYPE);
> +		strcpy((char *)info->name, "UBI");
> +#ifdef CONFIG_PARTITION_UUIDS
> +		info->uuid[0] = 0;
> +#endif
> +		return 0;
> +	}
> +#endif
> +
>   	/* If no dev_part_str, use bootdevice environment variable */
>   	if (!dev_part_str || !strlen(dev_part_str) ||
>   	    !strcmp(dev_part_str, "-"))
> diff --git a/fs/fs.c b/fs/fs.c
> index 827b143..b2d6a53 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -23,6 +23,7 @@
>   #include <fat.h>
>   #include <fs.h>
>   #include <sandboxfs.h>
> +#include <ubifs_uboot.h>
>   #include <asm/io.h>
>   #include <div64.h>
>   #include <linux/math64.h>
> @@ -157,6 +158,21 @@ static struct fstype_info fstypes[] = {
>   		.uuid = fs_uuid_unsupported,
>   	},
>   #endif
> +#ifdef CONFIG_CMD_UBIFS
> +	{
> +		.fstype = FS_TYPE_UBIFS,
> +		.name = "ubifs",
> +		.null_dev_desc_ok = true,
> +		.probe = ubifs_set_blk_dev,
> +		.close = ubifs_close,
> +		.ls = ubifs_ls,
> +		.exists = ubifs_exists,
> +		.size = ubifs_size,
> +		.read = ubifs_read,
> +		.write = fs_write_unsupported,
> +		.uuid = fs_uuid_unsupported,
> +	},
> +#endif
>   	{
>   		.fstype = FS_TYPE_ANY,
>   		.name = "unsupported",
> diff --git a/include/fs.h b/include/fs.h
> index fd1e4ab..059a395 100644
> --- a/include/fs.h
> +++ b/include/fs.h
> @@ -22,6 +22,7 @@
>   #define FS_TYPE_FAT	1
>   #define FS_TYPE_EXT	2
>   #define FS_TYPE_SANDBOX	3
> +#define FS_TYPE_UBIFS	4
>
>   /*
>    * Tell the fs layer which block device an partition to use for future
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-25 11:00   ` Heiko Schocher
@ 2015-08-25 11:32     ` Hans de Goede
  2015-08-28 14:52       ` Tom Rini
  0 siblings, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-08-25 11:32 UTC (permalink / raw)
  To: u-boot

Hi,

On 25-08-15 13:00, Heiko Schocher wrote:
> Hello Hans,
>
> Am 22.08.2015 um 20:04 schrieb Hans de Goede:
>> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
>> and give them their own header file.
>>
>> This is a preparation patch for adding ubifs support to the generic fs
>> code from fs/fs.c.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   common/cmd_ubifs.c    | 14 +++--------
>>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>>   fs/ubifs/ubifs.h      |  6 +----
>>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>>   4 files changed, 89 insertions(+), 30 deletions(-)
>>   create mode 100644 include/ubifs_uboot.h
>
> Only one nitpick, beside of this, you can add my:
>
> Reviewed-by: Heiko Schocher <hs@denx.de>
>
>> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
>> index 8e9a4e5..0a3dd24 100644
>> --- a/common/cmd_ubifs.c
>> +++ b/common/cmd_ubifs.c
>> @@ -15,11 +15,10 @@
>>   #include <common.h>
>>   #include <config.h>
>>   #include <command.h>
>> -
>> -#include "../fs/ubifs/ubifs.h"
>> +#include <ubifs_uboot.h>
>>
>>   static int ubifs_initialized;
>> -static int ubifs_mounted;
>> +int ubifs_mounted;
>
> later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
>
> Maybe you want to add a function, which returns the state
> of this var and let the var static?

Yes that would be cleaner, I'll fix the patch-set to do
things that way.

Thanks for the reviews.

So when the time come comes (when v2015.10 is out), how do
we merge these 3, shall I take them upstream through the
linux-sunxi tree, or do you want me to send a v2 with this fixed,
and then you take them upstream ?

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-25 11:32     ` Hans de Goede
@ 2015-08-28 14:52       ` Tom Rini
  2015-08-28 15:33         ` Heiko Schocher
  2015-08-31 16:08         ` Hans de Goede
  0 siblings, 2 replies; 25+ messages in thread
From: Tom Rini @ 2015-08-28 14:52 UTC (permalink / raw)
  To: u-boot

On Tue, Aug 25, 2015 at 01:32:05PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 25-08-15 13:00, Heiko Schocher wrote:
> >Hello Hans,
> >
> >Am 22.08.2015 um 20:04 schrieb Hans de Goede:
> >>Modify the ubifs u-boot wrapper function prototypes for generic fs use,
> >>and give them their own header file.
> >>
> >>This is a preparation patch for adding ubifs support to the generic fs
> >>code from fs/fs.c.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >>  common/cmd_ubifs.c    | 14 +++--------
> >>  fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
> >>  fs/ubifs/ubifs.h      |  6 +----
> >>  include/ubifs_uboot.h | 29 +++++++++++++++++++++
> >>  4 files changed, 89 insertions(+), 30 deletions(-)
> >>  create mode 100644 include/ubifs_uboot.h
> >
> >Only one nitpick, beside of this, you can add my:
> >
> >Reviewed-by: Heiko Schocher <hs@denx.de>
> >
> >>diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
> >>index 8e9a4e5..0a3dd24 100644
> >>--- a/common/cmd_ubifs.c
> >>+++ b/common/cmd_ubifs.c
> >>@@ -15,11 +15,10 @@
> >>  #include <common.h>
> >>  #include <config.h>
> >>  #include <command.h>
> >>-
> >>-#include "../fs/ubifs/ubifs.h"
> >>+#include <ubifs_uboot.h>
> >>
> >>  static int ubifs_initialized;
> >>-static int ubifs_mounted;
> >>+int ubifs_mounted;
> >
> >later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
> >
> >Maybe you want to add a function, which returns the state
> >of this var and let the var static?
> 
> Yes that would be cleaner, I'll fix the patch-set to do
> things that way.
> 
> Thanks for the reviews.
> 
> So when the time come comes (when v2015.10 is out), how do
> we merge these 3, shall I take them upstream through the
> linux-sunxi tree, or do you want me to send a v2 with this fixed,
> and then you take them upstream ?

I can just take 'em all into master :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150828/dabc5046/attachment.sig>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-28 14:52       ` Tom Rini
@ 2015-08-28 15:33         ` Heiko Schocher
  2015-08-31 16:08         ` Hans de Goede
  1 sibling, 0 replies; 25+ messages in thread
From: Heiko Schocher @ 2015-08-28 15:33 UTC (permalink / raw)
  To: u-boot

Hello tom,

Am 28.08.2015 um 16:52 schrieb Tom Rini:
> On Tue, Aug 25, 2015 at 01:32:05PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 25-08-15 13:00, Heiko Schocher wrote:
>>> Hello Hans,
>>>
>>> Am 22.08.2015 um 20:04 schrieb Hans de Goede:
>>>> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
>>>> and give them their own header file.
>>>>
>>>> This is a preparation patch for adding ubifs support to the generic fs
>>>> code from fs/fs.c.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   common/cmd_ubifs.c    | 14 +++--------
>>>>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>>>>   fs/ubifs/ubifs.h      |  6 +----
>>>>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>>>>   4 files changed, 89 insertions(+), 30 deletions(-)
>>>>   create mode 100644 include/ubifs_uboot.h
>>>
>>> Only one nitpick, beside of this, you can add my:
>>>
>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>>
>>>> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
>>>> index 8e9a4e5..0a3dd24 100644
>>>> --- a/common/cmd_ubifs.c
>>>> +++ b/common/cmd_ubifs.c
>>>> @@ -15,11 +15,10 @@
>>>>   #include <common.h>
>>>>   #include <config.h>
>>>>   #include <command.h>
>>>> -
>>>> -#include "../fs/ubifs/ubifs.h"
>>>> +#include <ubifs_uboot.h>
>>>>
>>>>   static int ubifs_initialized;
>>>> -static int ubifs_mounted;
>>>> +int ubifs_mounted;
>>>
>>> later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
>>>
>>> Maybe you want to add a function, which returns the state
>>> of this var and let the var static?
>>
>> Yes that would be cleaner, I'll fix the patch-set to do
>> things that way.
>>
>> Thanks for the reviews.
>>
>> So when the time come comes (when v2015.10 is out), how do
>> we merge these 3, shall I take them upstream through the
>> linux-sunxi tree, or do you want me to send a v2 with this fixed,
>> and then you take them upstream ?
>
> I can just take 'em all into master :)

Ok, from my side ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-28 14:52       ` Tom Rini
  2015-08-28 15:33         ` Heiko Schocher
@ 2015-08-31 16:08         ` Hans de Goede
  2015-09-01  3:46           ` Heiko Schocher
  1 sibling, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-08-31 16:08 UTC (permalink / raw)
  To: u-boot

Hi,

On 28-08-15 16:52, Tom Rini wrote:
> On Tue, Aug 25, 2015 at 01:32:05PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 25-08-15 13:00, Heiko Schocher wrote:
>>> Hello Hans,
>>>
>>> Am 22.08.2015 um 20:04 schrieb Hans de Goede:
>>>> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
>>>> and give them their own header file.
>>>>
>>>> This is a preparation patch for adding ubifs support to the generic fs
>>>> code from fs/fs.c.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>   common/cmd_ubifs.c    | 14 +++--------
>>>>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>>>>   fs/ubifs/ubifs.h      |  6 +----
>>>>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>>>>   4 files changed, 89 insertions(+), 30 deletions(-)
>>>>   create mode 100644 include/ubifs_uboot.h
>>>
>>> Only one nitpick, beside of this, you can add my:
>>>
>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>>
>>>> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
>>>> index 8e9a4e5..0a3dd24 100644
>>>> --- a/common/cmd_ubifs.c
>>>> +++ b/common/cmd_ubifs.c
>>>> @@ -15,11 +15,10 @@
>>>>   #include <common.h>
>>>>   #include <config.h>
>>>>   #include <command.h>
>>>> -
>>>> -#include "../fs/ubifs/ubifs.h"
>>>> +#include <ubifs_uboot.h>
>>>>
>>>>   static int ubifs_initialized;
>>>> -static int ubifs_mounted;
>>>> +int ubifs_mounted;
>>>
>>> later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
>>>
>>> Maybe you want to add a function, which returns the state
>>> of this var and let the var static?
>>
>> Yes that would be cleaner, I'll fix the patch-set to do
>> things that way.
>>
>> Thanks for the reviews.
>>
>> So when the time come comes (when v2015.10 is out), how do
>> we merge these 3, shall I take them upstream through the
>> linux-sunxi tree, or do you want me to send a v2 with this fixed,
>> and then you take them upstream ?
>
> I can just take 'em all into master :)

Heiko and I both thought we were too far in the cycle for that,
but I must admit I do not see that much chance of these changes
causing regressions and they are a nice improvement.

So I'll send a v2 for you to merge, unless Heiko objects.

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-08-31 16:08         ` Hans de Goede
@ 2015-09-01  3:46           ` Heiko Schocher
  2015-09-01 10:57             ` Heiko Schocher
  0 siblings, 1 reply; 25+ messages in thread
From: Heiko Schocher @ 2015-09-01  3:46 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 31.08.2015 um 18:08 schrieb Hans de Goede:
> Hi,
>
> On 28-08-15 16:52, Tom Rini wrote:
>> On Tue, Aug 25, 2015 at 01:32:05PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 25-08-15 13:00, Heiko Schocher wrote:
>>>> Hello Hans,
>>>>
>>>> Am 22.08.2015 um 20:04 schrieb Hans de Goede:
>>>>> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
>>>>> and give them their own header file.
>>>>>
>>>>> This is a preparation patch for adding ubifs support to the generic fs
>>>>> code from fs/fs.c.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>   common/cmd_ubifs.c    | 14 +++--------
>>>>>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>>>>>   fs/ubifs/ubifs.h      |  6 +----
>>>>>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>>>>>   4 files changed, 89 insertions(+), 30 deletions(-)
>>>>>   create mode 100644 include/ubifs_uboot.h
>>>>
>>>> Only one nitpick, beside of this, you can add my:
>>>>
>>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>>>
>>>>> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
>>>>> index 8e9a4e5..0a3dd24 100644
>>>>> --- a/common/cmd_ubifs.c
>>>>> +++ b/common/cmd_ubifs.c
>>>>> @@ -15,11 +15,10 @@
>>>>>   #include <common.h>
>>>>>   #include <config.h>
>>>>>   #include <command.h>
>>>>> -
>>>>> -#include "../fs/ubifs/ubifs.h"
>>>>> +#include <ubifs_uboot.h>
>>>>>
>>>>>   static int ubifs_initialized;
>>>>> -static int ubifs_mounted;
>>>>> +int ubifs_mounted;
>>>>
>>>> later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
>>>>
>>>> Maybe you want to add a function, which returns the state
>>>> of this var and let the var static?
>>>
>>> Yes that would be cleaner, I'll fix the patch-set to do
>>> things that way.
>>>
>>> Thanks for the reviews.
>>>
>>> So when the time come comes (when v2015.10 is out), how do
>>> we merge these 3, shall I take them upstream through the
>>> linux-sunxi tree, or do you want me to send a v2 with this fixed,
>>> and then you take them upstream ?
>>
>> I can just take 'em all into master :)
>
> Heiko and I both thought we were too far in the cycle for that,
> but I must admit I do not see that much chance of these changes
> causing regressions and they are a nice improvement.
>
> So I'll send a v2 for you to merge, unless Heiko objects.

Let me do with your v2 some tests today, if they are ok its Toms
decision.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
  2015-09-01  3:46           ` Heiko Schocher
@ 2015-09-01 10:57             ` Heiko Schocher
  0 siblings, 0 replies; 25+ messages in thread
From: Heiko Schocher @ 2015-09-01 10:57 UTC (permalink / raw)
  To: u-boot

Hello Hans,

Am 01.09.2015 um 05:46 schrieb Heiko Schocher:
> Hello Hans,
>
> Am 31.08.2015 um 18:08 schrieb Hans de Goede:
>> Hi,
>>
>> On 28-08-15 16:52, Tom Rini wrote:
>>> On Tue, Aug 25, 2015 at 01:32:05PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 25-08-15 13:00, Heiko Schocher wrote:
>>>>> Hello Hans,
>>>>>
>>>>> Am 22.08.2015 um 20:04 schrieb Hans de Goede:
>>>>>> Modify the ubifs u-boot wrapper function prototypes for generic fs use,
>>>>>> and give them their own header file.
>>>>>>
>>>>>> This is a preparation patch for adding ubifs support to the generic fs
>>>>>> code from fs/fs.c.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>   common/cmd_ubifs.c    | 14 +++--------
>>>>>>   fs/ubifs/ubifs.c      | 70 ++++++++++++++++++++++++++++++++++++++++-----------
>>>>>>   fs/ubifs/ubifs.h      |  6 +----
>>>>>>   include/ubifs_uboot.h | 29 +++++++++++++++++++++
>>>>>>   4 files changed, 89 insertions(+), 30 deletions(-)
>>>>>>   create mode 100644 include/ubifs_uboot.h
>>>>>
>>>>> Only one nitpick, beside of this, you can add my:
>>>>>
>>>>> Reviewed-by: Heiko Schocher <hs@denx.de>
>>>>>
>>>>>> diff --git a/common/cmd_ubifs.c b/common/cmd_ubifs.c
>>>>>> index 8e9a4e5..0a3dd24 100644
>>>>>> --- a/common/cmd_ubifs.c
>>>>>> +++ b/common/cmd_ubifs.c
>>>>>> @@ -15,11 +15,10 @@
>>>>>>   #include <common.h>
>>>>>>   #include <config.h>
>>>>>>   #include <command.h>
>>>>>> -
>>>>>> -#include "../fs/ubifs/ubifs.h"
>>>>>> +#include <ubifs_uboot.h>
>>>>>>
>>>>>>   static int ubifs_initialized;
>>>>>> -static int ubifs_mounted;
>>>>>> +int ubifs_mounted;
>>>>>
>>>>> later you add "extern int ubifs_mounted" in include/ubifs_uboot.h
>>>>>
>>>>> Maybe you want to add a function, which returns the state
>>>>> of this var and let the var static?
>>>>
>>>> Yes that would be cleaner, I'll fix the patch-set to do
>>>> things that way.
>>>>
>>>> Thanks for the reviews.
>>>>
>>>> So when the time come comes (when v2015.10 is out), how do
>>>> we merge these 3, shall I take them upstream through the
>>>> linux-sunxi tree, or do you want me to send a v2 with this fixed,
>>>> and then you take them upstream ?
>>>
>>> I can just take 'em all into master :)
>>
>> Heiko and I both thought we were too far in the cycle for that,
>> but I must admit I do not see that much chance of these changes
>> causing regressions and they are a nice improvement.
>>
>> So I'll send a v2 for you to merge, unless Heiko objects.
>
> Let me do with your v2 some tests today, if they are ok its Toms
> decision.

Tested the patches:

  Patchwork [U-Boot,v2,1/3] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use
http://patchwork.ozlabs.org/patch/512543/

  Patchwork [U-Boot,v2,2/3] ubifs: Add functions for generic fs use
http://patchwork.ozlabs.org/patch/512545/

  Patchwork [U-Boot,v2,3/3] ubifs: Add generic fs support
http://patchwork.ozlabs.org/patch/512544/

on the aristainetos2 board (with an ubifs on SPI NOR and NAND) without
seeing errors ... Are the above patches your current set? You have
in the subject "[PATCH v2 1/4]" ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/4] ubifs: Add functions for generic fs use
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 2/4] ubifs: Add functions " Hans de Goede
  2015-08-25 11:02   ` Heiko Schocher
@ 2015-09-01 19:57   ` Stephen Warren
  2015-09-01 20:01     ` Michael Trimarchi
  2015-09-14 17:29     ` Hans de Goede
  1 sibling, 2 replies; 25+ messages in thread
From: Stephen Warren @ 2015-09-01 19:57 UTC (permalink / raw)
  To: u-boot

On 08/22/2015 11:04 AM, Hans de Goede wrote:
> Implement the necessary functions for implementing generic fs support
> for ubifs.

> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c

> +int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
> +{
> +	/* Check that ubifs is mounted and that we are not being a blkdev */
> +	if (!ubifs_mounted) {
> +		printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
> +		return -1;
> +	}
> +
> +	if (rbdd) {
> +		printf("UBIFS cannot be used with normal block devices\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}

I think those printf() should be debug(). Otherwise, if (a) someone
attempts to run generic filesystem commands on a device with no
filesystem or (b) we add new filesystems into fstypes[] after ubifs,
those prints are going to happen even though a user didn't do something
to explicitly cause a ubifs-related issue.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/4] ubifs: Add functions for generic fs use
  2015-09-01 19:57   ` Stephen Warren
@ 2015-09-01 20:01     ` Michael Trimarchi
  2015-09-14 17:29     ` Hans de Goede
  1 sibling, 0 replies; 25+ messages in thread
From: Michael Trimarchi @ 2015-09-01 20:01 UTC (permalink / raw)
  To: u-boot

Hi

On Sep 1, 2015 9:57 PM, "Stephen Warren" <swarren@wwwdotorg.org> wrote:
>
> On 08/22/2015 11:04 AM, Hans de Goede wrote:
> > Implement the necessary functions for implementing generic fs support
> > for ubifs.
>
> > diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>
> > +int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
> > +{
> > +     /* Check that ubifs is mounted and that we are not being a blkdev
*/
> > +     if (!ubifs_mounted) {
> > +             printf("UBIFS not mounted, use ubifsmount to mount volume
first!\n");
> > +             return -1;
> > +     }
> > +
> > +     if (rbdd) {
> > +             printf("UBIFS cannot be used with normal block
devices\n");
> > +             return -1;
> > +     }
> > +
> > +     return 0;
> > +}
>
> I think those printf() should be debug(). Otherwise, if (a) someone
> attempts to run generic filesystem commands on a device with no
> filesystem or (b) we add new filesystems into fstypes[] after ubifs,
> those prints are going to happen even though a user didn't do something
> to explicitly cause a ubifs-related issue.
>

Personally I don't like return code like -1

Michael
_______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support Hans de Goede
  2015-08-25 11:20   ` Heiko Schocher
@ 2015-09-01 20:03   ` Stephen Warren
  2015-09-14 17:35     ` Hans de Goede
  1 sibling, 1 reply; 25+ messages in thread
From: Stephen Warren @ 2015-09-01 20:03 UTC (permalink / raw)
  To: u-boot

On 08/22/2015 11:04 AM, Hans de Goede wrote:
> Add generic fs support, so that commands like ls, load and test -e can be
> used on ubifs.

> @@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str,
>  		return 0;
>  	}
>  
> +#ifdef CONFIG_CMD_UBIFS
> +	/*
> +	 * Special-case ubi, ubi goes through a mtd, rathen then through
> +	 * a regular block device.
> +	 */
> +	if (0 == strcmp(ifname, "ubi")) {
> +		if (!ubifs_mounted) {
> +			printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
> +			return -1;
> +		}
> +
> +		*dev_desc = NULL;
> +		memset(info, 0, sizeof(*info));
> +		strcpy((char *)info->type, BOOT_PART_TYPE);
> +		strcpy((char *)info->name, "UBI");
> +#ifdef CONFIG_PARTITION_UUIDS
> +		info->uuid[0] = 0;
> +#endif
> +		return 0;
> +	}
> +#endif

We now have two paths through this function that can "Return" a NULL
dev_desc. This makes it impossible for sandbox and ubifs to successfully
co-exist in the same U-Boot binary, since the sandbox and ubifs fs probe
functions won't be able to tell if "hostfs" or "ubifs" was passed to
get_device_and_partition(). Perhaps there's no ubifs support in sandbox
right now, so there's no issue?

If this is an issue that needs to be solved now, I think the best
solution would be for the two special cases in
get_device_and_partition() to "return" a real dev_desc rather than NULL.
Since there's nothing meaningful to put there, how about returning a
hard-coded value that can then be checked in the fs probe functions to
make sure it matches:

get_device_and_partition():

	if (hostfs) {
		...
		*dev_desc = &hostfs_fake_dev_desc;
		...
		return 0;
	}
	if (ubi) {
		...
		*dev_desc = &ubifs_fake_dev_desc;
		...
		return 0;
	}

ubifs_set_blk_dev():

	if (rbdd != &ubifs_fake_dev_desc)
		return -1;
	...
	return 0;

... that said, I wonder if the ubifs special case in
get_device_and_partition() shouldn't actually perform the ubifs_mount()
call itself, based on the user-supplied parameters?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs
  2015-08-22 18:04 ` [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
@ 2015-09-01 20:13   ` Stephen Warren
  2015-09-14 17:48     ` Hans de Goede
  0 siblings, 1 reply; 25+ messages in thread
From: Stephen Warren @ 2015-09-01 20:13 UTC (permalink / raw)
  To: u-boot

On 08/22/2015 11:04 AM, Hans de Goede wrote:
> From: Roy Spliet <r.spliet@ultimaker.com>
> 
> Under the assumptions of having a UBI volume called boot, containing
> a ubifs filesystem.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I'd expect the person in the "From:" line above to have an s-o-b line
too. If the patch has changed so much that isn't appropriate, perhaps
change the patch's git author to you and just mentioned "Based on work
by: Roy ..."?

> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h

> +#ifdef CONFIG_CMD_UBIFS
> +#define BOOTENV_SHARED_UBIFS \
> +	"ubifs_boot=" \
> +		"if ubi part UBI && ubifsmount ubi${devnum}:boot; then "  \

I don't know ubifs well enough to know the implications of that
particular set of ubi commands, but as far as the
impact-on/integration-into the bootcmd.h file, this patch looks fine.

Acked-by: Stephen Warren <swarren@nvidia.com>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 2/4] ubifs: Add functions for generic fs use
  2015-09-01 19:57   ` Stephen Warren
  2015-09-01 20:01     ` Michael Trimarchi
@ 2015-09-14 17:29     ` Hans de Goede
  1 sibling, 0 replies; 25+ messages in thread
From: Hans de Goede @ 2015-09-14 17:29 UTC (permalink / raw)
  To: u-boot

Hi,

On 01-09-15 21:57, Stephen Warren wrote:
> On 08/22/2015 11:04 AM, Hans de Goede wrote:
>> Implement the necessary functions for implementing generic fs support
>> for ubifs.
>
>> diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
>
>> +int ubifs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
>> +{
>> +	/* Check that ubifs is mounted and that we are not being a blkdev */
>> +	if (!ubifs_mounted) {
>> +		printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
>> +		return -1;
>> +	}
>> +
>> +	if (rbdd) {
>> +		printf("UBIFS cannot be used with normal block devices\n");
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>
> I think those printf() should be debug(). Otherwise, if (a) someone
> attempts to run generic filesystem commands on a device with no
> filesystem or (b) we add new filesystems into fstypes[] after ubifs,
> those prints are going to happen even though a user didn't do something
> to explicitly cause a ubifs-related issue.

Ack, will fix.

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support
  2015-09-01 20:03   ` Stephen Warren
@ 2015-09-14 17:35     ` Hans de Goede
  2015-09-21 18:11       ` Stephen Warren
  0 siblings, 1 reply; 25+ messages in thread
From: Hans de Goede @ 2015-09-14 17:35 UTC (permalink / raw)
  To: u-boot

Hi,

On 01-09-15 22:03, Stephen Warren wrote:
> On 08/22/2015 11:04 AM, Hans de Goede wrote:
>> Add generic fs support, so that commands like ls, load and test -e can be
>> used on ubifs.
>
>> @@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str,
>>   		return 0;
>>   	}
>>
>> +#ifdef CONFIG_CMD_UBIFS
>> +	/*
>> +	 * Special-case ubi, ubi goes through a mtd, rathen then through
>> +	 * a regular block device.
>> +	 */
>> +	if (0 == strcmp(ifname, "ubi")) {
>> +		if (!ubifs_mounted) {
>> +			printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
>> +			return -1;
>> +		}
>> +
>> +		*dev_desc = NULL;
>> +		memset(info, 0, sizeof(*info));
>> +		strcpy((char *)info->type, BOOT_PART_TYPE);
>> +		strcpy((char *)info->name, "UBI");
>> +#ifdef CONFIG_PARTITION_UUIDS
>> +		info->uuid[0] = 0;
>> +#endif
>> +		return 0;
>> +	}
>> +#endif
>
> We now have two paths through this function that can "Return" a NULL
> dev_desc. This makes it impossible for sandbox and ubifs to successfully
> co-exist in the same U-Boot binary, since the sandbox and ubifs fs probe
> functions won't be able to tell if "hostfs" or "ubifs" was passed to
> get_device_and_partition(). Perhaps there's no ubifs support in sandbox
> right now, so there's no issue?

Right, ubifs is for raw nand, sandbox is for access to a host filesystem
in a sandbox build. I basically never expect both CONFIG_CMD_UBIFS and
CONFIG_SANDBOX to be set at the same time. I'll add a pre-processor
check + #error to enforce this in the next version.

> If this is an issue that needs to be solved now, I think the best
> solution would be for the two special cases in
> get_device_and_partition() to "return" a real dev_desc rather than NULL.
> Since there's nothing meaningful to put there, how about returning a
> hard-coded value that can then be checked in the fs probe functions to
> make sure it matches:
>
> get_device_and_partition():
>
> 	if (hostfs) {
> 		...
> 		*dev_desc = &hostfs_fake_dev_desc;
> 		...
> 		return 0;
> 	}
> 	if (ubi) {
> 		...
> 		*dev_desc = &ubifs_fake_dev_desc;
> 		...
> 		return 0;
> 	}
>
> ubifs_set_blk_dev():
>
> 	if (rbdd != &ubifs_fake_dev_desc)
> 		return -1;
> 	...
> 	return 0;
>
> ... that said, I wonder if the ubifs special case in
> get_device_and_partition() shouldn't actually perform the ubifs_mount()
> call itself, based on the user-supplied parameters?

That is not possible as the supplied parameter for a generic fs call
is a device index + partition number, where as ubi volumes use
names (strings).

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs
  2015-09-01 20:13   ` Stephen Warren
@ 2015-09-14 17:48     ` Hans de Goede
  0 siblings, 0 replies; 25+ messages in thread
From: Hans de Goede @ 2015-09-14 17:48 UTC (permalink / raw)
  To: u-boot

Hi,

On 01-09-15 22:13, Stephen Warren wrote:
> On 08/22/2015 11:04 AM, Hans de Goede wrote:
>> From: Roy Spliet <r.spliet@ultimaker.com>
>>
>> Under the assumptions of having a UBI volume called boot, containing
>> a ubifs filesystem.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> I'd expect the person in the "From:" line above to have an s-o-b line
> too. If the patch has changed so much that isn't appropriate, perhaps
> change the patch's git author to you and just mentioned "Based on work
> by: Roy ..."?

The code has changed significantly, I've send Roy a mail asking him which
solution he prefers, and I'll fix things up in my tree when I've an answer.

Regards,

Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support
  2015-09-14 17:35     ` Hans de Goede
@ 2015-09-21 18:11       ` Stephen Warren
  0 siblings, 0 replies; 25+ messages in thread
From: Stephen Warren @ 2015-09-21 18:11 UTC (permalink / raw)
  To: u-boot

On 09/14/2015 11:35 AM, Hans de Goede wrote:
> Hi,
>
> On 01-09-15 22:03, Stephen Warren wrote:
>> On 08/22/2015 11:04 AM, Hans de Goede wrote:
>>> Add generic fs support, so that commands like ls, load and test -e
>>> can be
>>> used on ubifs.
>>
>>> @@ -530,6 +531,28 @@ int get_device_and_partition(const char *ifname,
>>> const char *dev_part_str,
>>>           return 0;
>>>       }
>>>
>>> +#ifdef CONFIG_CMD_UBIFS
>>> +    /*
>>> +     * Special-case ubi, ubi goes through a mtd, rathen then through
>>> +     * a regular block device.
>>> +     */
>>> +    if (0 == strcmp(ifname, "ubi")) {
>>> +        if (!ubifs_mounted) {
>>> +            printf("UBIFS not mounted, use ubifsmount to mount
>>> volume first!\n");
>>> +            return -1;
>>> +        }
>>> +
>>> +        *dev_desc = NULL;
>>> +        memset(info, 0, sizeof(*info));
>>> +        strcpy((char *)info->type, BOOT_PART_TYPE);
>>> +        strcpy((char *)info->name, "UBI");
>>> +#ifdef CONFIG_PARTITION_UUIDS
>>> +        info->uuid[0] = 0;
>>> +#endif
>>> +        return 0;
>>> +    }
>>> +#endif
>>
>> We now have two paths through this function that can "Return" a NULL
>> dev_desc. This makes it impossible for sandbox and ubifs to successfully
>> co-exist in the same U-Boot binary, since the sandbox and ubifs fs probe
>> functions won't be able to tell if "hostfs" or "ubifs" was passed to
>> get_device_and_partition(). Perhaps there's no ubifs support in sandbox
>> right now, so there's no issue?
>
> Right, ubifs is for raw nand, sandbox is for access to a host filesystem
> in a sandbox build. I basically never expect both CONFIG_CMD_UBIFS and
> CONFIG_SANDBOX to be set at the same time. I'll add a pre-processor
> check + #error to enforce this in the next version.
>
>> If this is an issue that needs to be solved now, I think the best
>> solution would be for the two special cases in
>> get_device_and_partition() to "return" a real dev_desc rather than NULL.
>> Since there's nothing meaningful to put there, how about returning a
>> hard-coded value that can then be checked in the fs probe functions to
>> make sure it matches:
>>
>> get_device_and_partition():
>>
>>     if (hostfs) {
>>         ...
>>         *dev_desc = &hostfs_fake_dev_desc;
>>         ...
>>         return 0;
>>     }
>>     if (ubi) {
>>         ...
>>         *dev_desc = &ubifs_fake_dev_desc;
>>         ...
>>         return 0;
>>     }
>>
>> ubifs_set_blk_dev():
>>
>>     if (rbdd != &ubifs_fake_dev_desc)
>>         return -1;
>>     ...
>>     return 0;
>>
>> ... that said, I wonder if the ubifs special case in
>> get_device_and_partition() shouldn't actually perform the ubifs_mount()
>> call itself, based on the user-supplied parameters?
>
> That is not possible as the supplied parameter for a generic fs call
> is a device index + partition number, where as ubi volumes use
> names (strings).

I don't think that matters.

The "generic fs" plumbing added in this series doesn't require a volume 
name to be passed from the "generic fs" layer to the ubifs layer; it 
assumes that a ubifs volume is already mounted and so no volume identity 
is required. I wasn't implying that should be changed. Given that, the 
disparity between parameters doesn't matter, since there's no need to 
translate the ubifs_fake_dev_dec to a ubifs volume name at all; the only 
thing it'd be used for is as an identity check to dispatch from the 
generic fs layer to the right underlying filesystem.

Still, the current solution you have should be fine for now. We only 
need to fix this if someone implements a raw NAND emulator for sandbox.

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2015-09-21 18:11 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-22 18:04 [U-Boot] [PATCH v2 0/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
2015-08-22 18:04 ` [U-Boot] [PATCH v2 1/4] ubifs: Modify ubifs u-boot wrapper function prototypes for generic fs use Hans de Goede
2015-08-25 11:00   ` Heiko Schocher
2015-08-25 11:32     ` Hans de Goede
2015-08-28 14:52       ` Tom Rini
2015-08-28 15:33         ` Heiko Schocher
2015-08-31 16:08         ` Hans de Goede
2015-09-01  3:46           ` Heiko Schocher
2015-09-01 10:57             ` Heiko Schocher
2015-08-22 18:04 ` [U-Boot] [PATCH v2 2/4] ubifs: Add functions " Hans de Goede
2015-08-25 11:02   ` Heiko Schocher
2015-09-01 19:57   ` Stephen Warren
2015-09-01 20:01     ` Michael Trimarchi
2015-09-14 17:29     ` Hans de Goede
2015-08-22 18:04 ` [U-Boot] [PATCH v2 3/4] ubifs: Add generic fs support Hans de Goede
2015-08-25 11:20   ` Heiko Schocher
2015-09-01 20:03   ` Stephen Warren
2015-09-14 17:35     ` Hans de Goede
2015-09-21 18:11       ` Stephen Warren
2015-08-22 18:04 ` [U-Boot] [PATCH v2 4/4] distro_bootcmd: Add support for booting from ubifs Hans de Goede
2015-09-01 20:13   ` Stephen Warren
2015-09-14 17:48     ` Hans de Goede
2015-08-24 16:57 ` [U-Boot] [PATCH v2 0/4] " Scott Wood
2015-08-25  7:15   ` Hans de Goede
2015-08-25  7:31     ` Heiko Schocher

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.