[PATCH 0/7] U-Boot environment data as a filesystem

[PATCH 0/7] U-Boot environment data as a filesystem

[Andrey Smirnov]

Everyone:

This series adds code needed to expose U-Boot environemnt variable
data partition as a regular filesystem in Barebox. It currently only
supports the use-case where environment is stored on SD/MMC device,
since that is the only use-case I have access to for testing, however
adding support for other cases should be relatively
straightforward. The series is currently lacking documentation for
Barebox specific DT bindings it introduces, however an example of
usage can be seen in

ARM: rdu2: Add U-Boot environment partitions
ARM: rdu1: Add U-Boot environment partition

I'll add the appropriate documentation in v2 once all of the details
are hashed out.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (6):
  filetype: Add "U-Boot environmemnt variable data" filetype
  filetype: Allow specifying cdev's filetype explicitly
  drivers: Introduce late_platform_driver()
  misc: Add a driver to expose U-Boot environment variable data
  fs: Add a driver to access U-Boot environment variables
  ARM: rdu2: Add U-Boot environment partitions

Cory Tusar (1):
  ARM: rdu1: Add U-Boot environment partition

 arch/arm/dts/imx51-zii-rdu1.dts    |  21 ++
 arch/arm/dts/imx6qdl-zii-rdu2.dtsi |  27 ++
 common/filetype.c                  |   8 +
 drivers/misc/Kconfig               |  12 +
 drivers/misc/Makefile              |   1 +
 drivers/misc/ubootvar.c            | 322 +++++++++++++++++++
 fs/Kconfig                         |   8 +
 fs/Makefile                        |   1 +
 fs/ubootvarfs.c                    | 499 +++++++++++++++++++++++++++++
 include/driver.h                   |   4 +
 include/filetype.h                 |   1 +
 11 files changed, 904 insertions(+)
 create mode 100644 drivers/misc/ubootvar.c
 create mode 100644 fs/ubootvarfs.c

-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/7] filetype: Add "U-Boot environmemnt variable data" filetype

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/filetype.c  | 2 ++
 include/filetype.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/common/filetype.c b/common/filetype.c
index e2d707b15..429911533 100644
--- a/common/filetype.c
+++ b/common/filetype.c
@@ -77,6 +77,8 @@ static const struct filetype_str filetype_str[] = {
 	[filetype_imx_image_v2] = { "i.MX image (v2)", "imx-image-v2" },
 	[filetype_layerscape_image] = { "Layerscape image", "layerscape-PBL" },
 	[filetype_layerscape_qspi_image] = { "Layerscape QSPI image", "layerscape-qspi-PBL" },
+	[filetype_ubootvar] = { "U-Boot environmemnt variable data",
+				"ubootvar" },
 };
 
 const char *file_type_to_string(enum filetype f)
diff --git a/include/filetype.h b/include/filetype.h
index dcb331a6c..f1be04e81 100644
--- a/include/filetype.h
+++ b/include/filetype.h
@@ -47,6 +47,7 @@ enum filetype {
 	filetype_imx_image_v2,
 	filetype_layerscape_image,
 	filetype_layerscape_qspi_image,
+	filetype_ubootvar,
 	filetype_max,
 };
 
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/7] filetype: Allow specifying cdev's filetype explicitly

[Andrey Smirnov]

Allow specifying cdev's filetype explicitly to support the cases where
the type of a cdev is known apriori, yet cannot be determined by
reading the cdev's content.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 common/filetype.c | 6 ++++++
 include/driver.h  | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/common/filetype.c b/common/filetype.c
index 429911533..e4c8005b5 100644
--- a/common/filetype.c
+++ b/common/filetype.c
@@ -425,6 +425,11 @@ enum filetype cdev_detect_type(const char *name)
 	if (!cdev)
 		return type;
 
+	if (cdev->filetype != filetype_unknown) {
+		type = cdev->filetype;
+		goto cdev_close;
+	}
+
 	buf = xzalloc(FILE_TYPE_SAFE_BUFSIZE);
 	ret = cdev_read(cdev, buf, FILE_TYPE_SAFE_BUFSIZE, 0, 0);
 	if (ret < 0)
@@ -434,6 +439,7 @@ enum filetype cdev_detect_type(const char *name)
 
 err_out:
 	free(buf);
+cdev_close:
 	cdev_close(cdev);
 	return type;
 }
diff --git a/include/driver.h b/include/driver.h
index 26ec413bd..fe2d30ab5 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -22,6 +22,7 @@
 #include <linux/list.h>
 #include <linux/ioport.h>
 #include <of.h>
+#include <filetype.h>
 
 #define FORMAT_DRIVER_NAME_ID	"%s%d"
 
@@ -464,6 +465,7 @@ struct cdev {
 	struct list_head link_entry, links;
 	struct list_head partition_entry, partitions;
 	struct cdev *master;
+	enum filetype filetype;
 };
 
 int devfs_create(struct cdev *);
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/7] drivers: Introduce late_platform_driver()

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 include/driver.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/driver.h b/include/driver.h
index fe2d30ab5..300603fa3 100644
--- a/include/driver.h
+++ b/include/driver.h
@@ -420,6 +420,8 @@ int platform_driver_register(struct driver_d *drv);
 	register_driver_macro(device,platform,drv)
 #define console_platform_driver(drv)	\
 	register_driver_macro(console,platform,drv)
+#define late_platform_driver(drv)	\
+	register_driver_macro(late,platform,drv)
 
 int platform_device_register(struct device_d *new_device);
 
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/7] misc: Add a driver to expose U-Boot environment variable data

[Andrey Smirnov]

Add a driver to expose U-Boot environment variable data as a single
mmap-able device. Not very useful on its own, it is a crucial
low-level plumbing needed by filesystem driver introduced in the
following commit.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
---
 drivers/misc/Kconfig    |  12 ++
 drivers/misc/Makefile   |   1 +
 drivers/misc/ubootvar.c | 322 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/misc/ubootvar.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4c8a769c4..0f736f8bd 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -23,4 +23,16 @@ config STATE_DRV
 config DEV_MEM
         bool "Generic memory I/O device (/dev/mem)"
 
+config UBOOTVAR
+	bool "U-Boot environment storage"
+	help
+	  This driver exposes U-Boot environment variable storage as a
+	  single mmap-able device, hiding various low-level details
+	  such as:
+	      - Preamble format differences
+	      - Read/write logic in presence of redundant partition
+
+	  While it can be used standalone, it is best when coupled
+	  with corresponding filesystem driver.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d4e616d51..bc1c01ea4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_JTAG)		+= jtag.o
 obj-$(CONFIG_SRAM)		+= sram.o
 obj-$(CONFIG_STATE_DRV)		+= state.o
 obj-$(CONFIG_DEV_MEM)		+= mem.o
+obj-$(CONFIG_UBOOTVAR)		+= ubootvar.o
diff --git a/drivers/misc/ubootvar.c b/drivers/misc/ubootvar.c
new file mode 100644
index 000000000..d9def3e1a
--- /dev/null
+++ b/drivers/misc/ubootvar.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * U-Boot environment vriable blob driver
+ *
+ * Copyright (C) 2019 Zodiac Inflight Innovations
+ */
+
+#include <common.h>
+#include <init.h>
+#include <io.h>
+#include <of.h>
+#include <malloc.h>
+#include <partition.h>
+#include <envfs.h>
+#include <fs.h>
+#include <libfile.h>
+#include <command.h>
+#include <crc.h>
+
+enum ubootvar_flag_scheme {
+	FLAG_NONE,
+	FLAG_BOOLEAN,
+	FLAG_INCREMENTAL,
+};
+
+struct ubootvar_data {
+	struct cdev cdev;
+	char *path[2];
+	bool current;
+	uint8_t flag;
+	int count;
+};
+
+static int ubootvar_flush(struct cdev *cdev)
+{
+	struct device_d *dev = cdev->dev;
+	struct ubootvar_data *ubdata = dev->priv;
+	const char *path = ubdata->path[!ubdata->current];
+	uint32_t crc = 0xffffffff;
+	struct resource *mem;
+	resource_size_t size;
+	const void *data;
+	int fd, ret = 0;
+
+	mem = dev_get_resource(dev, IORESOURCE_MEM, 0);
+	if (IS_ERR(mem)) {
+		dev_err(dev, "Failed to get associated memory resource\n");
+		return PTR_ERR(mem);
+	}
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0) {
+		dev_err(dev, "Failed to open %s\n", path);
+		return -errno;
+	}
+	/*
+	 * FIXME: This code needs to do a proper protect/unprotect and
+	 * erase calls to work on MTD devices
+	 */
+
+	/*
+	 * Write a dummy CRC first as a way of invalidating the
+	 * environment in case we fail mid-flushing
+	 */
+	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
+		dev_err(dev, "Failed to write dummy CRC\n");
+		ret = -errno;
+		goto close_fd;
+	}
+
+	if (ubdata->count > 1) {
+		/*
+		 * FIXME: This assumes FLAG_INCREMENTAL
+		 */
+		const uint8_t flag = ++ubdata->flag;
+
+		if (write_full(fd, &flag, sizeof(flag)) != sizeof(flag)) {
+			dev_dbg(dev, "Failed to write flag\n");
+			ret = -errno;
+			goto close_fd;
+		}
+	}
+
+	data = (const void *)mem->start;
+	size = resource_size(mem);
+
+	/*
+	 * Write out and flush all of the new environement data
+	 */
+	if (write_full(fd, data, size) != size) {
+		dev_dbg(dev, "Failed to write data\n");
+		ret = -errno;
+		goto close_fd;
+	}
+
+	if (flush(fd)) {
+		dev_dbg(dev, "Failed to flush written data\n");
+		ret = -errno;
+		goto close_fd;
+	}
+	/*
+	 * Now that all of the environment data is out, we can go back
+	 * to the start of the block and write correct CRC, to finish
+	 * the processs.
+	 */
+	if (lseek(fd, 0, SEEK_SET) != 0) {
+		dev_dbg(dev, "lseek() failed\n");
+		ret = -errno;
+		goto close_fd;
+	}
+
+	crc = crc32(0, data, size);
+	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
+		dev_dbg(dev, "Failed to write valid CRC\n");
+		ret = -errno;
+		goto close_fd;
+	}
+	/*
+	 * Now that we've successfuly written new enviroment blob out,
+	 * swtich current partition.
+	 */
+	ubdata->current = !ubdata->current;
+
+close_fd:
+	close(fd);
+	return ret;
+}
+
+static struct cdev_operations ubootvar_ops = {
+	.read = mem_read,
+	.write = mem_write,
+	.memmap = generic_memmap_rw,
+	.flush = ubootvar_flush,
+};
+
+static void ubootenv_info(struct device_d *dev)
+{
+	struct ubootvar_data *ubdata = dev->priv;
+
+	printf("Current environment copy: device-path-%d\n", ubdata->current);
+}
+
+static int ubootenv_probe(struct device_d *dev)
+{
+	struct ubootvar_data *ubdata;
+	unsigned int crc_ok = 0;
+	int ret, i, count = 0;
+	uint32_t crc[2];
+	uint8_t flag[2];
+	size_t size[2];
+	void *blob[2] = { NULL, NULL };
+	uint8_t *data[2];
+
+	/*
+	 * FIXME: Flag scheme is determined by the type of underlined
+	 * non-volatible device, so it should probably come from
+	 * Device Tree binding. Currently we just assume incremental
+	 * scheme since that is what is used on SD/eMMC devices.
+	 */
+	enum ubootvar_flag_scheme flag_scheme = FLAG_INCREMENTAL;
+
+	ubdata = xzalloc(sizeof(*ubdata));
+
+	ret = of_find_path(dev->device_node, "device-path-0",
+			   &ubdata->path[0],
+			   OF_FIND_PATH_FLAGS_BB);
+	if (ret) {
+		dev_err(dev, "Failed to find first device\n");
+		goto out;
+	}
+
+	count++;
+
+	if (!of_find_path(dev->device_node, "device-path-1",
+			  &ubdata->path[1],
+			  OF_FIND_PATH_FLAGS_BB)) {
+		count++;
+	} else {
+		/*
+		 * If there's no redundant environment partition we
+		 * configure both paths to point to the same device,
+		 * so that writing logic could stay the same for both
+		 * redundant and non-redundant cases
+		 */
+		ubdata->path[1] = ubdata->path[0];
+	}
+
+	for (i = 0; i < count; i++) {
+		data[i] = blob[i] = read_file(ubdata->path[i], &size[i]);
+		if (!blob[i]) {
+			dev_err(dev, "Failed to read U-Boot environment\n");
+			ret = -EIO;
+			goto out;
+		}
+
+		crc[i] = *(uint32_t *)data[i];
+
+		size[i] -= sizeof(uint32_t);
+		data[i] += sizeof(uint32_t);
+
+		if (count > 1) {
+			/*
+			 * When used in primary/redundant
+			 * configuration, environment header has an
+			 * additional "flag" byte
+			 */
+			flag[i] = *data[i];
+			size[i] -= sizeof(uint8_t);
+			data[i] += sizeof(uint8_t);
+		}
+
+		crc_ok |= (crc32(0, data[i], size[i]) == crc[i]) << i;
+	}
+
+	switch (crc_ok) {
+	case 0b00:
+		ret = -EINVAL;
+		dev_err(dev, "Bad CRC, can't continue further\n");
+		goto out;
+	case 0b11:
+		/*
+		 * Both partition are valid, so we need to examine
+		 * flags to determine which one to use as current
+		 */
+		switch (flag_scheme) {
+		case FLAG_INCREMENTAL:
+			if ((flag[0] == 0xff && flag[1] == 0) ||
+			    (flag[1] == 0xff && flag[0] == 0)) {
+				/*
+				 * When flag overflow happens current
+				 * partition is the one whose counter
+				 * reached zero first. That is if
+				 * flag[1] == 0 is true (1), then i
+				 * would be 1 as well
+				 */
+				i = flag[1] == 0;
+			} else {
+				/*
+				 * In no-overflow case the partition
+				 * with higher flag value is
+				 * considered current
+				 */
+				i = flag[1] > flag[0];
+			}
+			break;
+		default:
+			ret = -EINVAL;
+			dev_err(dev, "Unknown flag scheme %u\n", flag_scheme);
+			goto out;
+		}
+		break;
+	default:
+		/*
+		 * Only one partition is valid, so the choice of the
+		 * current one is obvious
+		 */
+		i = __ffs(crc_ok);
+		break;
+	};
+
+	ret = device_add_resource(dev, "data", (resource_size_t)data[i],
+				  size[i], IORESOURCE_MEM);
+	if (ret) {
+		dev_err(dev, "Failed to add resource\n");
+		goto out;
+	}
+
+	ubdata->cdev.name = basprintf("ubootvar%d",
+				      cdev_find_free_index("ubootvar"));
+	ubdata->cdev.size = size[i];
+	ubdata->cdev.ops = &ubootvar_ops;
+	ubdata->cdev.dev = dev;
+	ubdata->cdev.filetype = filetype_ubootvar;
+	ubdata->current = i;
+	ubdata->count = count;
+	ubdata->flag = flag[i];
+
+	dev->priv = ubdata;
+
+	ret = devfs_create(&ubdata->cdev);
+	if (ret) {
+		dev_err(dev, "Failed to create corresponding cdev\n");
+		goto out;
+	}
+
+	cdev_create_default_automount(&ubdata->cdev);
+
+	if (count > 1) {
+		/*
+		 * We won't be using read data from redundant
+		 * parttion, so we may as well free at this point
+		 */
+		free(blob[!i]);
+	}
+
+	dev->info = ubootenv_info;
+
+	return 0;
+out:
+	for (i = 0; i < count; i++)
+		free(blob[i]);
+
+	free(ubdata);
+
+	return ret;
+}
+
+static struct of_device_id ubootenv_dt_ids[] = {
+	{
+		.compatible = "barebox,uboot-environment",
+	}, {
+		/* sentinel */
+	}
+};
+
+static struct driver_d ubootenv_driver = {
+	.name		= "uboot-environment",
+	.probe		= ubootenv_probe,
+	.of_compatible	= ubootenv_dt_ids,
+};
+late_platform_driver(ubootenv_driver);
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/7] fs: Add a driver to access U-Boot environment variables

[Andrey Smirnov]

Add a driver working on top of ubootvar device and exposing U-Boot
environment variable data as files.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
---
 fs/Kconfig      |   8 +
 fs/Makefile     |   1 +
 fs/ubootvarfs.c | 499 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 508 insertions(+)
 create mode 100644 fs/ubootvarfs.c

diff --git a/fs/Kconfig b/fs/Kconfig
index e3a95321c..adf281a5b 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -118,4 +118,12 @@ config FS_RATP
 	  This enables support for transferring files over RATP. A host can
 	  export a directory which can then be mounted under barebox.
 
+config FS_UBOOTVARFS
+	bool
+	depends on UBOOTVAR
+	prompt "U-Boot environment variable filesystem support"
+	help
+	  This filesystem driver provides access to U-Boot environment
+	  variables.
+
 endmenu
diff --git a/fs/Makefile b/fs/Makefile
index ac3e6a03a..9889a6507 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_FS_SMHFS) += smhfs.o
 obj-$(CONFIG_FS_PSTORE) += pstore/
 obj-$(CONFIG_FS_SQUASHFS) += squashfs/
 obj-$(CONFIG_FS_RATP)	+= ratpfs.o
+obj-$(CONFIG_FS_UBOOTVARFS) += ubootvarfs.o
diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
new file mode 100644
index 000000000..8de97b2de
--- /dev/null
+++ b/fs/ubootvarfs.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Zodiac Inflight Innovations
+ */
+
+#define pr_fmt(fmt) "ubootvarfs: " fmt
+
+#include <common.h>
+#include <driver.h>
+#include <init.h>
+#include <malloc.h>
+#include <fs.h>
+#include <string.h>
+#include <errno.h>
+#include <linux/stat.h>
+#include <xfuncs.h>
+#include <fcntl.h>
+#include <efi.h>
+#include <wchar.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+/**
+ * Some theory of operation:
+ *
+ * U-Boot environment variable data is expected to be presented as a
+ * single blob containing an arbitrary number "<key>=<value>\0" paris
+ * without any other auxiliary information (accomplished by ubootvar
+ * driver)
+ *
+ * Filesystem driver code in this file parses above data an creates a
+ * linked list of all of the "variables" found (see @ubootvarfs_var to
+ * what information is recorded).
+ *
+ * With that in place reading or writing file data becomes as trivial
+ * as looking up a variable in the linked list by name and then
+ * memcpy()-ing bytes from its value region.
+ *
+ * The only moderately tricky part is resising a given file/variable
+ * since, given the underlying data format, it requires us to move all
+ * of the key/value data that comes after the given file/variable as
+ * well as to adjust all of the cached offsets stored in varaible
+ * linked list. See ubootvarfs_adjust() for the implementation
+ * details.
+ */
+
+/**
+ * struct ubootvarfs_var - U-Boot environment key-value pair
+ *
+ * @list:	Linked list head
+ * @name:	Pointer to memory containing key string (variable name)
+ * @name_len:	Variable name's (above) length
+ * @start:	Start of value in memory
+ * @end:	End of value in memory
+ */
+struct ubootvarfs_var {
+	struct list_head list;
+	char *name;
+	size_t name_len;
+	char *start;
+	char *end;
+};
+
+/**
+ * struct ubootvarfs_data - U-Boot environment data
+ *
+ * @var_list:	Linked list of all of the parsed variables
+ * @fd:		File descriptor of underlying ubootvar device
+ * @end:	End of U-boot environment
+ * @limit:	U-boot environment limit (can't grow to go past the limit)
+ */
+struct ubootvarfs_data {
+	struct list_head var_list;
+	int fd;
+	char *end;
+	const char *limit;
+};
+
+struct ubootvarfs_inode {
+	struct inode inode;
+	struct ubootvarfs_var *var;
+	struct ubootvarfs_data *data;
+};
+
+static struct ubootvarfs_inode *inode_to_node(struct inode *inode)
+{
+	return container_of(inode, struct ubootvarfs_inode, inode);
+}
+
+static const struct inode_operations ubootvarfs_file_inode_operations;
+static const struct file_operations ubootvarfs_dir_operations;
+static const struct inode_operations ubootvarfs_dir_inode_operations;
+static const struct file_operations ubootvarfs_file_operations;
+
+static struct inode *ubootvarfs_get_inode(struct super_block *sb,
+					  const struct inode *dir,
+					  umode_t mode,
+					  struct ubootvarfs_var *var)
+{
+	struct inode *inode = new_inode(sb);
+	struct ubootvarfs_inode *node;
+
+	if (!inode)
+		return NULL;
+
+	inode->i_ino = get_next_ino();
+	inode->i_mode = mode;
+	if (var)
+		inode->i_size = var->end - var->start;
+
+	node = inode_to_node(inode);
+	node->var = var;
+
+	switch (mode & S_IFMT) {
+	default:
+		return NULL;
+	case S_IFREG:
+		inode->i_op = &ubootvarfs_file_inode_operations;
+		inode->i_fop = &ubootvarfs_file_operations;
+		break;
+	case S_IFDIR:
+		inode->i_op = &ubootvarfs_dir_inode_operations;
+		inode->i_fop = &ubootvarfs_dir_operations;
+		inc_nlink(inode);
+		break;
+	}
+
+	return inode;
+}
+
+static struct ubootvarfs_var *
+ubootvarfs_var_by_name(struct ubootvarfs_data *data, const char *name)
+{
+	struct ubootvarfs_var *var;
+	const size_t len = strlen(name);
+
+	list_for_each_entry(var, &data->var_list, list) {
+		if (len == var->name_len &&
+		    !memcmp(name, var->name, var->name_len))
+			return var;
+	}
+
+	return NULL;
+}
+
+static struct dentry *ubootvarfs_lookup(struct inode *dir,
+					struct dentry *dentry,
+					unsigned int flags)
+{
+	struct super_block *sb = dir->i_sb;
+	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
+	struct ubootvarfs_data *data = fsdev->dev.priv;
+	struct ubootvarfs_var *var;
+	struct inode *inode;
+
+	var = ubootvarfs_var_by_name(data, dentry->name);
+	if (!var)
+		return NULL;
+
+	inode = ubootvarfs_get_inode(dir->i_sb, dir, S_IFREG | 0777, var);
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	d_add(dentry, inode);
+
+	return NULL;
+}
+
+static int ubootvarfs_iterate(struct file *file, struct dir_context *ctx)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = d_inode(dentry);
+	struct ubootvarfs_inode *node = inode_to_node(inode);
+	struct ubootvarfs_data *data = node->data;
+	struct ubootvarfs_var *var;
+
+	dir_emit_dots(file, ctx);
+
+	list_for_each_entry(var, &data->var_list, list)
+		dir_emit(ctx, var->name, var->name_len, 0, DT_REG);
+
+	return 0;
+}
+
+static const struct file_operations ubootvarfs_dir_operations = {
+	.iterate = ubootvarfs_iterate,
+};
+
+/**
+ * ubootvarfs_relocate_tail() - Move all of the data after given inode by delta
+ *
+ * @node:	Inode marking the start of the data
+ * @delta:	Offset to move the data by
+ *
+ * This function move all of the environment data that starts after
+ * the given @node by @delta bytes. In case the data is moved towards
+ * the start of the environment data blob trailing leftover data is
+ * zeroed out
+ */
+static void ubootvarfs_relocate_tail(struct ubootvarfs_inode *node,
+				     int delta)
+{
+	struct ubootvarfs_var *var = node->var;
+	struct ubootvarfs_data *data = node->data;
+	const size_t n = data->end - var->start;
+	void *src = var->end + 1;
+
+	memmove(src + delta, src, n);
+
+	data->end += delta;
+
+	if (delta < 0) {
+		/*
+		 * Remove all of the trailing leftovers
+		 */
+		memset(data->end, '\0', -delta);
+	}
+}
+
+/**
+ * ubootvarfs_adjust() - Adjust the size of a variable blob
+ *
+ * @node:	Inode marking where to start adjustement from
+ * @delta:	Offset to adjust by
+ *
+ * This function move all of the environment data that starts after
+ * the given @node by @delta bytes and updates all of the affected
+ * ubootvarfs_var's in varaible linked list
+ */
+static void ubootvarfs_adjust(struct ubootvarfs_inode *node,
+			      int delta)
+{
+	struct ubootvarfs_var *var = node->var;
+	struct ubootvarfs_data *data = node->data;
+
+	ubootvarfs_relocate_tail(node, delta);
+
+	list_for_each_entry_continue(var, &data->var_list, list) {
+		var->name += delta;
+		var->start += delta;
+		var->end += delta;
+	}
+}
+
+static int ubootvarfs_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct inode *inode = d_inode(dentry);
+
+	if (inode) {
+		struct ubootvarfs_inode *node = inode_to_node(inode);
+		struct ubootvarfs_var *var = node->var;
+		/*
+		 * -1 at the end is to account for '\0' at the end
+		 * that needs to be removed as well
+		 */
+		const int delta = var->name - var->end - 1;
+
+		ubootvarfs_adjust(node, delta);
+
+		list_del(&var->list);
+		free(var);
+	}
+
+	return simple_unlink(dir, dentry);
+}
+
+static int ubootvarfs_create(struct inode *dir, struct dentry *dentry,
+			     umode_t mode)
+{
+	struct super_block *sb = dir->i_sb;
+	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
+	struct ubootvarfs_data *data = fsdev->dev.priv;
+	struct inode *inode;
+	struct ubootvarfs_var *var;
+	size_t len = strlen(dentry->name);
+	/*
+	 * We'll be adding <varname>=\0\0 to the end of our data, so
+	 * we need to make sure there's enough room for it. Note that
+	 * + 3 is to accoutn for '=', and two '\0' from above
+	 */
+	if (data->end + len + 3 > data->limit)
+		return -ENOSPC;
+
+	var = xmalloc(sizeof(*var));
+
+	var->name = data->end;
+	memcpy(var->name, dentry->name, len);
+	var->name_len = len;
+	var->start = var->name + len;
+	*var->start++ = '=';
+	*var->start = '\0';
+	var->end = var->start;
+	data->end = var->end + 1;
+	*data->end = '\0';
+
+	list_add_tail(&var->list, &data->var_list);
+
+	inode = ubootvarfs_get_inode(sb, dir, mode, var);
+	d_instantiate(dentry, inode);
+
+	return 0;
+}
+
+static const struct inode_operations ubootvarfs_dir_inode_operations = {
+	.lookup = ubootvarfs_lookup,
+	.unlink = ubootvarfs_unlink,
+	.create = ubootvarfs_create,
+};
+
+static struct inode *ubootvarfs_alloc_inode(struct super_block *sb)
+{
+	struct ubootvarfs_inode *node;
+	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
+	struct ubootvarfs_data *data = fsdev->dev.priv;
+
+	node = xzalloc(sizeof(*node));
+	node->data = data;
+
+	return &node->inode;
+}
+
+static void ubootvarfs_destroy_inode(struct inode *inode)
+{
+	struct ubootvarfs_inode *node = inode_to_node(inode);
+
+	free(node->var);
+	free(node);
+}
+
+static const struct super_operations ubootvarfs_ops = {
+	.alloc_inode = ubootvarfs_alloc_inode,
+	.destroy_inode = ubootvarfs_destroy_inode,
+};
+
+static int ubootvarfs_io(struct device_d *dev, FILE *f, void *buf,
+			 size_t insize, bool read)
+{
+	struct inode *inode = f->f_inode;
+	struct ubootvarfs_inode *node = inode_to_node(inode);
+	void *ptr = node->var->start + f->pos;
+
+	if (read)
+		memcpy(buf, ptr, insize);
+	else
+		memcpy(ptr, buf, insize);
+
+	return insize;
+}
+
+static int ubootvarfs_read(struct device_d *dev, FILE *f, void *buf,
+			   size_t insize)
+{
+	return ubootvarfs_io(dev, f, buf, insize, true);
+}
+
+static int ubootvarfs_write(struct device_d *dev, FILE *f, const void *buf,
+			    size_t insize)
+{
+	return ubootvarfs_io(dev, f, (void *)buf, insize, false);
+}
+
+static int ubootvarfs_truncate(struct device_d *dev, FILE *f, loff_t size)
+{
+	struct inode *inode = f->f_inode;
+	struct ubootvarfs_inode *node = inode_to_node(inode);
+	struct ubootvarfs_data *data = node->data;
+	struct ubootvarfs_var *var = node->var;
+	const int delta = size - inode->i_size;
+
+	if (size == inode->i_size)
+		return 0;
+
+	if (data->end + delta >= data->limit)
+		return -ENOSPC;
+
+	ubootvarfs_adjust(node, delta);
+
+	if (delta > 0)
+		memset(var->end, '\0', delta);
+
+	var->end += delta;
+	*var->end = '\0';
+
+	return 0;
+}
+
+static void ubootvarfs_parse(struct ubootvarfs_data *data, char *blob,
+			     size_t size)
+{
+	struct ubootvarfs_var *var;
+	const char *start = blob;
+	size_t len;
+	char *sep;
+
+	data->limit = blob + size;
+	INIT_LIST_HEAD(&data->var_list);
+
+	while (*blob) {
+		var = xmalloc(sizeof(*var));
+		len = strnlen(blob, size);
+
+		var->name = blob;
+		var->end  = blob + len;
+
+		sep = strchr(blob, '=');
+		if (sep) {
+			var->start = sep + 1;
+			var->name_len = sep - blob;
+
+			list_add_tail(&var->list, &data->var_list);
+		} else {
+			pr_err("No separator in data @ 0x%08x. Skipped.",
+			       blob - start);
+			free(var);
+		}
+
+		len++; /* account for '\0' */
+		size -= len;
+		blob += len;
+	};
+
+	data->end = blob;
+}
+
+static int ubootvarfs_probe(struct device_d *dev)
+{
+	struct inode *inode;
+	struct ubootvarfs_data *data = xzalloc(sizeof(*data));
+	struct fs_device_d *fsdev = dev_to_fs_device(dev);
+	struct super_block *sb = &fsdev->sb;
+	struct stat s;
+	void *map;
+	int ret;
+
+	dev->priv = data;
+
+	data->fd = open(fsdev->backingstore, O_RDWR);
+	if (data->fd < 0) {
+		ret = -errno;
+		goto free_data;
+	}
+
+	if (fstat(data->fd, &s) < 0) {
+		ret = -errno;
+		goto exit;
+	}
+
+	map = memmap(data->fd, PROT_READ | PROT_WRITE);
+	if (map == MAP_FAILED) {
+		ret = -errno;
+		goto exit;
+	}
+
+	ubootvarfs_parse(data, map, s.st_size);
+
+	sb->s_op = &ubootvarfs_ops;
+	inode = ubootvarfs_get_inode(sb, NULL, S_IFDIR, NULL);
+	sb->s_root = d_make_root(inode);
+
+	/*
+	 * We don't use cdev * directly, but this is needed for
+	 * cdev_get_mount_path() to work right
+	 */
+	fsdev->cdev = cdev_by_name(devpath_to_name(fsdev->backingstore));
+
+	return 0;
+exit:
+	close(data->fd);
+free_data:
+	free(data);
+	return ret;
+}
+
+static void ubootvarfs_remove(struct device_d *dev)
+{
+	struct ubootvarfs_data *data = dev->priv;
+
+	flush(data->fd);
+	close(data->fd);
+	free(data);
+}
+
+static struct fs_driver_d ubootvarfs_driver = {
+	.truncate = ubootvarfs_truncate,
+	.read = ubootvarfs_read,
+	.write = ubootvarfs_write,
+	.type = filetype_ubootvar,
+	.drv = {
+		.probe = ubootvarfs_probe,
+		.remove = ubootvarfs_remove,
+		.name = "ubootvarfs",
+	}
+};
+
+static int ubootvarfs_init(void)
+{
+	return register_fs_driver(&ubootvarfs_driver);
+}
+coredevice_initcall(ubootvarfs_init);
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/7] ARM: rdu2: Add U-Boot environment partitions

[Andrey Smirnov]

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 arch/arm/dts/imx6qdl-zii-rdu2.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
index bfc75ba60..937fc5d67 100644
--- a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
+++ b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
@@ -47,6 +47,12 @@
 			compatible = "barebox,environment";
 			device-path = &nor_flash, "partname:barebox-environment";
 		};
+
+		ubootenv {
+			compatible = "barebox,uboot-environment";
+			device-path-0 = &usdhc4, "partname:uboot-environment-0";
+			device-path-1 = &usdhc4, "partname:uboot-environment-1";
+		};
 	};
 
 	device-info {
@@ -249,6 +255,27 @@
 	dr_mode = "otg";
 };
 
+&usdhc4 {
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		/*
+		 * Layout info is taken from /etc/fw_env.config
+		 */
+		partition@c0000 {
+			label = "uboot-environment-0";
+			reg = <0xc0000 0x4000>;
+		};
+
+		partition@cc800 {
+			label = "uboot-environment-1";
+			reg = <0xcc800 0x4000>;
+		};
+	};
+};
+
 &gpio3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_gpio3_hog>;
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/7] ARM: rdu1: Add U-Boot environment partition

[Andrey Smirnov]

From: Cory Tusar <cory.tusar@zii.aero>

Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
---
 arch/arm/dts/imx51-zii-rdu1.dts | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm/dts/imx51-zii-rdu1.dts b/arch/arm/dts/imx51-zii-rdu1.dts
index 01e46baf2..7400c8d77 100644
--- a/arch/arm/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/dts/imx51-zii-rdu1.dts
@@ -23,6 +23,11 @@
 			compatible = "barebox,environment";
 			device-path = &spinor, "partname:barebox-environment";
 		};
+
+		ubootenv {
+			compatible = "barebox,uboot-environment";
+			device-path-0 = &esdhc1, "partname:uboot-environment-0";
+		};
 	};
 
 	aliases {
@@ -61,6 +66,22 @@
 	};
 };
 
+&esdhc1 {
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		/*
+		 * Layout info is taken from /etc/fw_env.config
+		 */
+		partition@c0000 {
+			label = "uboot-environment-0";
+			reg = <0xc0000 0x20000>;
+		};
+	};
+};
+
 &mdio_gpio {
 	switch: switch@0 {};
 };
-- 
2.21.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] misc: Add a driver to expose U-Boot environment variable data

[Sascha Hauer]

On Mon, May 27, 2019 at 01:18:50PM -0700, Andrey Smirnov wrote:
> Add a driver to expose U-Boot environment variable data as a single
> mmap-able device. Not very useful on its own, it is a crucial
> low-level plumbing needed by filesystem driver introduced in the
> following commit.

It wasn't clear to me why we need this driver at all, so it might be
worth adding a comment that this is for handling redundant env.

> +static int ubootvar_flush(struct cdev *cdev)
> +{
> +	struct device_d *dev = cdev->dev;
> +	struct ubootvar_data *ubdata = dev->priv;
> +	const char *path = ubdata->path[!ubdata->current];
> +	uint32_t crc = 0xffffffff;
> +	struct resource *mem;
> +	resource_size_t size;
> +	const void *data;
> +	int fd, ret = 0;
> +
> +	mem = dev_get_resource(dev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(mem)) {
> +		dev_err(dev, "Failed to get associated memory resource\n");
> +		return PTR_ERR(mem);
> +	}

Resources of IORESOURCE_MEM are meant to describe an area in the
physical memory space. You shouldn't abuse it to describe some
arbitrarily allocated memory area. Why not simply put the pointers into
struct ubootvar_data?

> +
> +	fd = open(path, O_WRONLY);
> +	if (fd < 0) {
> +		dev_err(dev, "Failed to open %s\n", path);
> +		return -errno;
> +	}
> +	/*
> +	 * FIXME: This code needs to do a proper protect/unprotect and
> +	 * erase calls to work on MTD devices
> +	 */
> +
> +	/*
> +	 * Write a dummy CRC first as a way of invalidating the
> +	 * environment in case we fail mid-flushing
> +	 */
> +	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> +		dev_err(dev, "Failed to write dummy CRC\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	if (ubdata->count > 1) {
> +		/*
> +		 * FIXME: This assumes FLAG_INCREMENTAL
> +		 */
> +		const uint8_t flag = ++ubdata->flag;
> +
> +		if (write_full(fd, &flag, sizeof(flag)) != sizeof(flag)) {
> +			dev_dbg(dev, "Failed to write flag\n");
> +			ret = -errno;
> +			goto close_fd;
> +		}
> +	}
> +
> +	data = (const void *)mem->start;
> +	size = resource_size(mem);
> +
> +	/*
> +	 * Write out and flush all of the new environement data
> +	 */

s/environement/environment/

> +	if (write_full(fd, data, size) != size) {
> +		dev_dbg(dev, "Failed to write data\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	if (flush(fd)) {
> +		dev_dbg(dev, "Failed to flush written data\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +	/*
> +	 * Now that all of the environment data is out, we can go back
> +	 * to the start of the block and write correct CRC, to finish
> +	 * the processs.
> +	 */
> +	if (lseek(fd, 0, SEEK_SET) != 0) {
> +		dev_dbg(dev, "lseek() failed\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +
> +	crc = crc32(0, data, size);
> +	if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> +		dev_dbg(dev, "Failed to write valid CRC\n");
> +		ret = -errno;
> +		goto close_fd;
> +	}
> +	/*
> +	 * Now that we've successfuly written new enviroment blob out,
> +	 * swtich current partition.

s/swtich/switch/

> +	 */
> +	ubdata->current = !ubdata->current;
> +
> +close_fd:
> +	close(fd);
> +	return ret;
> +}
> +
> +static struct cdev_operations ubootvar_ops = {
> +	.read = mem_read,
> +	.write = mem_write,
> +	.memmap = generic_memmap_rw,

Ok, now I understand this struct resource thingy, you want to reuse
mem_read and friends. Given that these functions are oneliners to
implement I still suggest implementing them.

> +	.flush = ubootvar_flush,
> +};
> +
> +static void ubootenv_info(struct device_d *dev)
> +{
> +	struct ubootvar_data *ubdata = dev->priv;
> +
> +	printf("Current environment copy: device-path-%d\n", ubdata->current);
> +}
> +
> +static int ubootenv_probe(struct device_d *dev)
> +{
> +	struct ubootvar_data *ubdata;
> +	unsigned int crc_ok = 0;
> +	int ret, i, count = 0;
> +	uint32_t crc[2];
> +	uint8_t flag[2];
> +	size_t size[2];
> +	void *blob[2] = { NULL, NULL };
> +	uint8_t *data[2];
> +
> +	/*
> +	 * FIXME: Flag scheme is determined by the type of underlined
> +	 * non-volatible device, so it should probably come from
> +	 * Device Tree binding. Currently we just assume incremental
> +	 * scheme since that is what is used on SD/eMMC devices.
> +	 */
> +	enum ubootvar_flag_scheme flag_scheme = FLAG_INCREMENTAL;
> +
> +	ubdata = xzalloc(sizeof(*ubdata));
> +
> +	ret = of_find_path(dev->device_node, "device-path-0",
> +			   &ubdata->path[0],
> +			   OF_FIND_PATH_FLAGS_BB);

Maybe allow "device-path" without -x suffix when the U-Boot environment
is not redundant?

> +	if (ret) {
> +		dev_err(dev, "Failed to find first device\n");
> +		goto out;
> +	}
> +
> +	count++;
> +
> +	if (!of_find_path(dev->device_node, "device-path-1",
> +			  &ubdata->path[1],
> +			  OF_FIND_PATH_FLAGS_BB)) {
> +		count++;
> +	} else {
> +		/*
> +		 * If there's no redundant environment partition we
> +		 * configure both paths to point to the same device,
> +		 * so that writing logic could stay the same for both
> +		 * redundant and non-redundant cases
> +		 */
> +		ubdata->path[1] = ubdata->path[0];

ubdata->path[0] contains an allocated string, so we must be careful when
freeing this. You don't free it at all, so there's no problem currently.
I think you should either free it correctly in the error path or make
sure that both ubdata->path[0] and ubdata->path[1] contain allocated
strings.

> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		data[i] = blob[i] = read_file(ubdata->path[i], &size[i]);
> +		if (!blob[i]) {
> +			dev_err(dev, "Failed to read U-Boot environment\n");
> +			ret = -EIO;
> +			goto out;
> +		}
> +
> +		crc[i] = *(uint32_t *)data[i];
> +
> +		size[i] -= sizeof(uint32_t);
> +		data[i] += sizeof(uint32_t);
> +
> +		if (count > 1) {
> +			/*
> +			 * When used in primary/redundant
> +			 * configuration, environment header has an
> +			 * additional "flag" byte
> +			 */
> +			flag[i] = *data[i];
> +			size[i] -= sizeof(uint8_t);
> +			data[i] += sizeof(uint8_t);
> +		}
> +
> +		crc_ok |= (crc32(0, data[i], size[i]) == crc[i]) << i;
> +	}
> +
> +	switch (crc_ok) {
> +	case 0b00:
> +		ret = -EINVAL;
> +		dev_err(dev, "Bad CRC, can't continue further\n");
> +		goto out;

So when there's no valid U-Boot env is found (erased for example) then
this driver errors out and we won't have a chance to ever write
something there. Is this intended?

> +	case 0b11:
> +		/*
> +		 * Both partition are valid, so we need to examine
> +		 * flags to determine which one to use as current
> +		 */
> +		switch (flag_scheme) {
> +		case FLAG_INCREMENTAL:
> +			if ((flag[0] == 0xff && flag[1] == 0) ||
> +			    (flag[1] == 0xff && flag[0] == 0)) {
> +				/*
> +				 * When flag overflow happens current
> +				 * partition is the one whose counter
> +				 * reached zero first. That is if
> +				 * flag[1] == 0 is true (1), then i
> +				 * would be 1 as well
> +				 */
> +				i = flag[1] == 0;
> +			} else {
> +				/*
> +				 * In no-overflow case the partition
> +				 * with higher flag value is
> +				 * considered current
> +				 */
> +				i = flag[1] > flag[0];
> +			}
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			dev_err(dev, "Unknown flag scheme %u\n", flag_scheme);
> +			goto out;
> +		}
> +		break;
> +	default:
> +		/*
> +		 * Only one partition is valid, so the choice of the
> +		 * current one is obvious
> +		 */
> +		i = __ffs(crc_ok);

'i' changes its meaning from a loop counter to the partition currently
used. This makes this code harder to read, could you use a separate
variable for the currently used partition?

> +		break;
> +	};
> +
> +	ret = device_add_resource(dev, "data", (resource_size_t)data[i],
> +				  size[i], IORESOURCE_MEM);
> +	if (ret) {
> +		dev_err(dev, "Failed to add resource\n");
> +		goto out;
> +	}
> +
> +	ubdata->cdev.name = basprintf("ubootvar%d",
> +				      cdev_find_free_index("ubootvar"));
> +	ubdata->cdev.size = size[i];
> +	ubdata->cdev.ops = &ubootvar_ops;
> +	ubdata->cdev.dev = dev;
> +	ubdata->cdev.filetype = filetype_ubootvar;
> +	ubdata->current = i;
> +	ubdata->count = count;
> +	ubdata->flag = flag[i];
> +
> +	dev->priv = ubdata;
> +
> +	ret = devfs_create(&ubdata->cdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to create corresponding cdev\n");
> +		goto out;
> +	}
> +
> +	cdev_create_default_automount(&ubdata->cdev);
> +
> +	if (count > 1) {
> +		/*
> +		 * We won't be using read data from redundant
> +		 * parttion, so we may as well free at this point
> +		 */
> +		free(blob[!i]);
> +	}
> +
> +	dev->info = ubootenv_info;
> +
> +	return 0;
> +out:
> +	for (i = 0; i < count; i++)
> +		free(blob[i]);
> +
> +	free(ubdata);
> +
> +	return ret;
> +}
> +
> +static struct of_device_id ubootenv_dt_ids[] = {
> +	{
> +		.compatible = "barebox,uboot-environment",

Please add
Documentation/devicetree/bindings/barebox/barebox,uboot-environment.rst

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/7] fs: Add a driver to access U-Boot environment variables

[Sascha Hauer]

On Mon, May 27, 2019 at 01:18:51PM -0700, Andrey Smirnov wrote:
> Add a driver working on top of ubootvar device and exposing U-Boot
> environment variable data as files.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
> ---
>  fs/Kconfig      |   8 +
>  fs/Makefile     |   1 +
>  fs/ubootvarfs.c | 499 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 508 insertions(+)
>  create mode 100644 fs/ubootvarfs.c
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index e3a95321c..adf281a5b 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -118,4 +118,12 @@ config FS_RATP
>  	  This enables support for transferring files over RATP. A host can
>  	  export a directory which can then be mounted under barebox.
>  
> +config FS_UBOOTVARFS
> +	bool
> +	depends on UBOOTVAR
> +	prompt "U-Boot environment variable filesystem support"
> +	help
> +	  This filesystem driver provides access to U-Boot environment
> +	  variables.
> +
>  endmenu
> diff --git a/fs/Makefile b/fs/Makefile
> index ac3e6a03a..9889a6507 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_FS_SMHFS) += smhfs.o
>  obj-$(CONFIG_FS_PSTORE) += pstore/
>  obj-$(CONFIG_FS_SQUASHFS) += squashfs/
>  obj-$(CONFIG_FS_RATP)	+= ratpfs.o
> +obj-$(CONFIG_FS_UBOOTVARFS) += ubootvarfs.o
> diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
> new file mode 100644
> index 000000000..8de97b2de
> --- /dev/null
> +++ b/fs/ubootvarfs.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Zodiac Inflight Innovations
> + */
> +
> +#define pr_fmt(fmt) "ubootvarfs: " fmt
> +
> +#include <common.h>
> +#include <driver.h>
> +#include <init.h>
> +#include <malloc.h>
> +#include <fs.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <linux/stat.h>
> +#include <xfuncs.h>
> +#include <fcntl.h>
> +#include <efi.h>
> +#include <wchar.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +/**
> + * Some theory of operation:
> + *
> + * U-Boot environment variable data is expected to be presented as a
> + * single blob containing an arbitrary number "<key>=<value>\0" paris

s/paris/pairs/

> + * without any other auxiliary information (accomplished by ubootvar
> + * driver)
> + *
> + * Filesystem driver code in this file parses above data an creates a
> + * linked list of all of the "variables" found (see @ubootvarfs_var to
> + * what information is recorded).
> + *
> + * With that in place reading or writing file data becomes as trivial
> + * as looking up a variable in the linked list by name and then
> + * memcpy()-ing bytes from its value region.
> + *
> + * The only moderately tricky part is resising a given file/variable

s/resising/resizing/

> + * since, given the underlying data format, it requires us to move all
> + * of the key/value data that comes after the given file/variable as
> + * well as to adjust all of the cached offsets stored in varaible

s/varaible/variable/

> + * linked list. See ubootvarfs_adjust() for the implementation
> + * details.

Wouldn't it be easier and more straight forward to serialize and
deserialize the U-Boot environment as a whole rather than trying to
adjust an in-memory representation each time the env is changed?

> +static int ubootvarfs_create(struct inode *dir, struct dentry *dentry,
> +			     umode_t mode)
> +{
> +	struct super_block *sb = dir->i_sb;
> +	struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
> +	struct ubootvarfs_data *data = fsdev->dev.priv;
> +	struct inode *inode;
> +	struct ubootvarfs_var *var;
> +	size_t len = strlen(dentry->name);
> +	/*
> +	 * We'll be adding <varname>=\0\0 to the end of our data, so
> +	 * we need to make sure there's enough room for it. Note that
> +	 * + 3 is to accoutn for '=', and two '\0' from above

s/accoutn/account/

> +static void ubootvarfs_remove(struct device_d *dev)
> +{
> +	struct ubootvarfs_data *data = dev->priv;
> +
> +	flush(data->fd);
> +	close(data->fd);
> +	free(data);
> +}

So the environment is written only when the FS is unmounted. We might
want to have some other method to trigger writing. I don't know how this
method may look yet though.

Sasch

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 6/7] ARM: rdu2: Add U-Boot environment partitions

[Sascha Hauer]

On Mon, May 27, 2019 at 01:18:52PM -0700, Andrey Smirnov wrote:
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  arch/arm/dts/imx6qdl-zii-rdu2.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> index bfc75ba60..937fc5d67 100644
> --- a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> +++ b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> @@ -47,6 +47,12 @@
>  			compatible = "barebox,environment";
>  			device-path = &nor_flash, "partname:barebox-environment";
>  		};
> +
> +		ubootenv {
> +			compatible = "barebox,uboot-environment";
> +			device-path-0 = &usdhc4, "partname:uboot-environment-0";
> +			device-path-1 = &usdhc4, "partname:uboot-environment-1";
> +		};

Please add a label to the partitions and point to them here directly
rather than using the partname: string.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 6/7] ARM: rdu2: Add U-Boot environment partitions

[Andrey Smirnov]

On Tue, May 28, 2019 at 2:57 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Mon, May 27, 2019 at 01:18:52PM -0700, Andrey Smirnov wrote:
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  arch/arm/dts/imx6qdl-zii-rdu2.dtsi | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> > index bfc75ba60..937fc5d67 100644
> > --- a/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> > +++ b/arch/arm/dts/imx6qdl-zii-rdu2.dtsi
> > @@ -47,6 +47,12 @@
> >                       compatible = "barebox,environment";
> >                       device-path = &nor_flash, "partname:barebox-environment";
> >               };
> > +
> > +             ubootenv {
> > +                     compatible = "barebox,uboot-environment";
> > +                     device-path-0 = &usdhc4, "partname:uboot-environment-0";
> > +                     device-path-1 = &usdhc4, "partname:uboot-environment-1";
> > +             };
>
> Please add a label to the partitions and point to them here directly
> rather than using the partname: string.
>

OK, will do in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] misc: Add a driver to expose U-Boot environment variable data

[Andrey Smirnov]

On Tue, May 28, 2019 at 2:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Mon, May 27, 2019 at 01:18:50PM -0700, Andrey Smirnov wrote:
> > Add a driver to expose U-Boot environment variable data as a single
> > mmap-able device. Not very useful on its own, it is a crucial
> > low-level plumbing needed by filesystem driver introduced in the
> > following commit.
>
> It wasn't clear to me why we need this driver at all, so it might be
> worth adding a comment that this is for handling redundant env.
>

Was it before of after you read the Kconfig help that talks about it?
Asking to see if copying that text into commit would be enough, or if
should write a more detailed explanation.

> > +static int ubootvar_flush(struct cdev *cdev)
> > +{
> > +     struct device_d *dev = cdev->dev;
> > +     struct ubootvar_data *ubdata = dev->priv;
> > +     const char *path = ubdata->path[!ubdata->current];
> > +     uint32_t crc = 0xffffffff;
> > +     struct resource *mem;
> > +     resource_size_t size;
> > +     const void *data;
> > +     int fd, ret = 0;
> > +
> > +     mem = dev_get_resource(dev, IORESOURCE_MEM, 0);
> > +     if (IS_ERR(mem)) {
> > +             dev_err(dev, "Failed to get associated memory resource\n");
> > +             return PTR_ERR(mem);
> > +     }
>
> Resources of IORESOURCE_MEM are meant to describe an area in the
> physical memory space. You shouldn't abuse it to describe some
> arbitrarily allocated memory area. Why not simply put the pointers into
> struct ubootvar_data?
>
> > +
> > +     fd = open(path, O_WRONLY);
> > +     if (fd < 0) {
> > +             dev_err(dev, "Failed to open %s\n", path);
> > +             return -errno;
> > +     }
> > +     /*
> > +      * FIXME: This code needs to do a proper protect/unprotect and
> > +      * erase calls to work on MTD devices
> > +      */
> > +
> > +     /*
> > +      * Write a dummy CRC first as a way of invalidating the
> > +      * environment in case we fail mid-flushing
> > +      */
> > +     if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> > +             dev_err(dev, "Failed to write dummy CRC\n");
> > +             ret = -errno;
> > +             goto close_fd;
> > +     }
> > +
> > +     if (ubdata->count > 1) {
> > +             /*
> > +              * FIXME: This assumes FLAG_INCREMENTAL
> > +              */
> > +             const uint8_t flag = ++ubdata->flag;
> > +
> > +             if (write_full(fd, &flag, sizeof(flag)) != sizeof(flag)) {
> > +                     dev_dbg(dev, "Failed to write flag\n");
> > +                     ret = -errno;
> > +                     goto close_fd;
> > +             }
> > +     }
> > +
> > +     data = (const void *)mem->start;
> > +     size = resource_size(mem);
> > +
> > +     /*
> > +      * Write out and flush all of the new environement data
> > +      */
>
> s/environement/environment/
>
> > +     if (write_full(fd, data, size) != size) {
> > +             dev_dbg(dev, "Failed to write data\n");
> > +             ret = -errno;
> > +             goto close_fd;
> > +     }
> > +
> > +     if (flush(fd)) {
> > +             dev_dbg(dev, "Failed to flush written data\n");
> > +             ret = -errno;
> > +             goto close_fd;
> > +     }
> > +     /*
> > +      * Now that all of the environment data is out, we can go back
> > +      * to the start of the block and write correct CRC, to finish
> > +      * the processs.
> > +      */
> > +     if (lseek(fd, 0, SEEK_SET) != 0) {
> > +             dev_dbg(dev, "lseek() failed\n");
> > +             ret = -errno;
> > +             goto close_fd;
> > +     }
> > +
> > +     crc = crc32(0, data, size);
> > +     if (write_full(fd, &crc, sizeof(crc)) != sizeof(crc)) {
> > +             dev_dbg(dev, "Failed to write valid CRC\n");
> > +             ret = -errno;
> > +             goto close_fd;
> > +     }
> > +     /*
> > +      * Now that we've successfuly written new enviroment blob out,
> > +      * swtich current partition.
>
> s/swtich/switch/
>
> > +      */
> > +     ubdata->current = !ubdata->current;
> > +
> > +close_fd:
> > +     close(fd);
> > +     return ret;
> > +}
> > +
> > +static struct cdev_operations ubootvar_ops = {
> > +     .read = mem_read,
> > +     .write = mem_write,
> > +     .memmap = generic_memmap_rw,
>
> Ok, now I understand this struct resource thingy, you want to reuse
> mem_read and friends. Given that these functions are oneliners to
> implement I still suggest implementing them.
>

OK, I'll have to make mem_copy() visible globally for them to remain
one-liners, but that should be doable.

> > +     .flush = ubootvar_flush,
> > +};
> > +
> > +static void ubootenv_info(struct device_d *dev)
> > +{
> > +     struct ubootvar_data *ubdata = dev->priv;
> > +
> > +     printf("Current environment copy: device-path-%d\n", ubdata->current);
> > +}
> > +
> > +static int ubootenv_probe(struct device_d *dev)
> > +{
> > +     struct ubootvar_data *ubdata;
> > +     unsigned int crc_ok = 0;
> > +     int ret, i, count = 0;
> > +     uint32_t crc[2];
> > +     uint8_t flag[2];
> > +     size_t size[2];
> > +     void *blob[2] = { NULL, NULL };
> > +     uint8_t *data[2];
> > +
> > +     /*
> > +      * FIXME: Flag scheme is determined by the type of underlined
> > +      * non-volatible device, so it should probably come from
> > +      * Device Tree binding. Currently we just assume incremental
> > +      * scheme since that is what is used on SD/eMMC devices.
> > +      */
> > +     enum ubootvar_flag_scheme flag_scheme = FLAG_INCREMENTAL;
> > +
> > +     ubdata = xzalloc(sizeof(*ubdata));
> > +
> > +     ret = of_find_path(dev->device_node, "device-path-0",
> > +                        &ubdata->path[0],
> > +                        OF_FIND_PATH_FLAGS_BB);
>
> Maybe allow "device-path" without -x suffix when the U-Boot environment
> is not redundant?
>

I was modelling this after "pinctrl" property in which always has
trailing "-0". I can add the support for "device-path" in v2, though.

> > +     if (ret) {
> > +             dev_err(dev, "Failed to find first device\n");
> > +             goto out;
> > +     }
> > +
> > +     count++;
> > +
> > +     if (!of_find_path(dev->device_node, "device-path-1",
> > +                       &ubdata->path[1],
> > +                       OF_FIND_PATH_FLAGS_BB)) {
> > +             count++;
> > +     } else {
> > +             /*
> > +              * If there's no redundant environment partition we
> > +              * configure both paths to point to the same device,
> > +              * so that writing logic could stay the same for both
> > +              * redundant and non-redundant cases
> > +              */
> > +             ubdata->path[1] = ubdata->path[0];
>
> ubdata->path[0] contains an allocated string, so we must be careful when
> freeing this. You don't free it at all, so there's no problem currently.
> I think you should either free it correctly in the error path or make
> sure that both ubdata->path[0] and ubdata->path[1] contain allocated
> strings.
>

Oh, missed the fact that those strings are allocated. Will fix in v2.

> > +     }
> > +
> > +     for (i = 0; i < count; i++) {
> > +             data[i] = blob[i] = read_file(ubdata->path[i], &size[i]);
> > +             if (!blob[i]) {
> > +                     dev_err(dev, "Failed to read U-Boot environment\n");
> > +                     ret = -EIO;
> > +                     goto out;
> > +             }
> > +
> > +             crc[i] = *(uint32_t *)data[i];
> > +
> > +             size[i] -= sizeof(uint32_t);
> > +             data[i] += sizeof(uint32_t);
> > +
> > +             if (count > 1) {
> > +                     /*
> > +                      * When used in primary/redundant
> > +                      * configuration, environment header has an
> > +                      * additional "flag" byte
> > +                      */
> > +                     flag[i] = *data[i];
> > +                     size[i] -= sizeof(uint8_t);
> > +                     data[i] += sizeof(uint8_t);
> > +             }
> > +
> > +             crc_ok |= (crc32(0, data[i], size[i]) == crc[i]) << i;
> > +     }
> > +
> > +     switch (crc_ok) {
> > +     case 0b00:
> > +             ret = -EINVAL;
> > +             dev_err(dev, "Bad CRC, can't continue further\n");
> > +             goto out;
>
> So when there's no valid U-Boot env is found (erased for example) then
> this driver errors out and we won't have a chance to ever write
> something there. Is this intended?
>

The only use-case we have for this is to communicate data to already
existing U-Boot installation with a valid environment data, so the
scenario you are talking about never came up. I guess I can change the
code to zero the buffered data out and proceed, so that FS layer would
perceive this as an empty environment.

> > +     case 0b11:
> > +             /*
> > +              * Both partition are valid, so we need to examine
> > +              * flags to determine which one to use as current
> > +              */
> > +             switch (flag_scheme) {
> > +             case FLAG_INCREMENTAL:
> > +                     if ((flag[0] == 0xff && flag[1] == 0) ||
> > +                         (flag[1] == 0xff && flag[0] == 0)) {
> > +                             /*
> > +                              * When flag overflow happens current
> > +                              * partition is the one whose counter
> > +                              * reached zero first. That is if
> > +                              * flag[1] == 0 is true (1), then i
> > +                              * would be 1 as well
> > +                              */
> > +                             i = flag[1] == 0;
> > +                     } else {
> > +                             /*
> > +                              * In no-overflow case the partition
> > +                              * with higher flag value is
> > +                              * considered current
> > +                              */
> > +                             i = flag[1] > flag[0];
> > +                     }
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     dev_err(dev, "Unknown flag scheme %u\n", flag_scheme);
> > +                     goto out;
> > +             }
> > +             break;
> > +     default:
> > +             /*
> > +              * Only one partition is valid, so the choice of the
> > +              * current one is obvious
> > +              */
> > +             i = __ffs(crc_ok);
>
> 'i' changes its meaning from a loop counter to the partition currently
> used. This makes this code harder to read, could you use a separate
> variable for the currently used partition?
>

Sure, will do in v2.

> > +             break;
> > +     };
> > +
> > +     ret = device_add_resource(dev, "data", (resource_size_t)data[i],
> > +                               size[i], IORESOURCE_MEM);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to add resource\n");
> > +             goto out;
> > +     }
> > +
> > +     ubdata->cdev.name = basprintf("ubootvar%d",
> > +                                   cdev_find_free_index("ubootvar"));
> > +     ubdata->cdev.size = size[i];
> > +     ubdata->cdev.ops = &ubootvar_ops;
> > +     ubdata->cdev.dev = dev;
> > +     ubdata->cdev.filetype = filetype_ubootvar;
> > +     ubdata->current = i;
> > +     ubdata->count = count;
> > +     ubdata->flag = flag[i];
> > +
> > +     dev->priv = ubdata;
> > +
> > +     ret = devfs_create(&ubdata->cdev);
> > +     if (ret) {
> > +             dev_err(dev, "Failed to create corresponding cdev\n");
> > +             goto out;
> > +     }
> > +
> > +     cdev_create_default_automount(&ubdata->cdev);
> > +
> > +     if (count > 1) {
> > +             /*
> > +              * We won't be using read data from redundant
> > +              * parttion, so we may as well free at this point
> > +              */
> > +             free(blob[!i]);
> > +     }
> > +
> > +     dev->info = ubootenv_info;
> > +
> > +     return 0;
> > +out:
> > +     for (i = 0; i < count; i++)
> > +             free(blob[i]);
> > +
> > +     free(ubdata);
> > +
> > +     return ret;
> > +}
> > +
> > +static struct of_device_id ubootenv_dt_ids[] = {
> > +     {
> > +             .compatible = "barebox,uboot-environment",
>
> Please add
> Documentation/devicetree/bindings/barebox/barebox,uboot-environment.rst
>

Will do.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/7] fs: Add a driver to access U-Boot environment variables

[Andrey Smirnov]

On Tue, May 28, 2019 at 2:56 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Mon, May 27, 2019 at 01:18:51PM -0700, Andrey Smirnov wrote:
> > Add a driver working on top of ubootvar device and exposing U-Boot
> > environment variable data as files.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
> > ---
> >  fs/Kconfig      |   8 +
> >  fs/Makefile     |   1 +
> >  fs/ubootvarfs.c | 499 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 508 insertions(+)
> >  create mode 100644 fs/ubootvarfs.c
> >
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index e3a95321c..adf281a5b 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -118,4 +118,12 @@ config FS_RATP
> >         This enables support for transferring files over RATP. A host can
> >         export a directory which can then be mounted under barebox.
> >
> > +config FS_UBOOTVARFS
> > +     bool
> > +     depends on UBOOTVAR
> > +     prompt "U-Boot environment variable filesystem support"
> > +     help
> > +       This filesystem driver provides access to U-Boot environment
> > +       variables.
> > +
> >  endmenu
> > diff --git a/fs/Makefile b/fs/Makefile
> > index ac3e6a03a..9889a6507 100644
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -18,3 +18,4 @@ obj-$(CONFIG_FS_SMHFS) += smhfs.o
> >  obj-$(CONFIG_FS_PSTORE) += pstore/
> >  obj-$(CONFIG_FS_SQUASHFS) += squashfs/
> >  obj-$(CONFIG_FS_RATP)        += ratpfs.o
> > +obj-$(CONFIG_FS_UBOOTVARFS) += ubootvarfs.o
> > diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
> > new file mode 100644
> > index 000000000..8de97b2de
> > --- /dev/null
> > +++ b/fs/ubootvarfs.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2019 Zodiac Inflight Innovations
> > + */
> > +
> > +#define pr_fmt(fmt) "ubootvarfs: " fmt
> > +
> > +#include <common.h>
> > +#include <driver.h>
> > +#include <init.h>
> > +#include <malloc.h>
> > +#include <fs.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <linux/stat.h>
> > +#include <xfuncs.h>
> > +#include <fcntl.h>
> > +#include <efi.h>
> > +#include <wchar.h>
> > +#include <linux/err.h>
> > +#include <linux/ctype.h>
> > +
> > +/**
> > + * Some theory of operation:
> > + *
> > + * U-Boot environment variable data is expected to be presented as a
> > + * single blob containing an arbitrary number "<key>=<value>\0" paris
>
> s/paris/pairs/
>
> > + * without any other auxiliary information (accomplished by ubootvar
> > + * driver)
> > + *
> > + * Filesystem driver code in this file parses above data an creates a
> > + * linked list of all of the "variables" found (see @ubootvarfs_var to
> > + * what information is recorded).
> > + *
> > + * With that in place reading or writing file data becomes as trivial
> > + * as looking up a variable in the linked list by name and then
> > + * memcpy()-ing bytes from its value region.
> > + *
> > + * The only moderately tricky part is resising a given file/variable
>
> s/resising/resizing/
>
> > + * since, given the underlying data format, it requires us to move all
> > + * of the key/value data that comes after the given file/variable as
> > + * well as to adjust all of the cached offsets stored in varaible
>
> s/varaible/variable/
>
> > + * linked list. See ubootvarfs_adjust() for the implementation
> > + * details.
>
> Wouldn't it be easier and more straight forward to serialize and
> deserialize the U-Boot environment as a whole rather than trying to
> adjust an in-memory representation each time the env is changed?
>

Eh, I don't think it'll save much if at all. Ubootvarfs_create() might
loose a couple of lines of code, ubootvarfs_adjust() and
ubootvarfs_relocate_tail() will go away, but new code to do
serialization would partially negate gains there. Ubootvarfs_unlink()
will probably loose a line or two ubootvarfs_truncate() would probably
remain equally weird, just in a different way.

OTOH, going that way would mean that all of the data would have to be
cached twice (once in ubootvar blob and then seprartely in
ubootvarfs_var elements of the list) as well as having to do a bunch
of unnecessary data processing. Given typical data size those problems
are probably negligible, but combined with the fact that all of the
code would have to be re-tested it makes me less than eager to go that
way.

I can make this change if you insist, but if it is up to me, I'd rather not.

> > +static int ubootvarfs_create(struct inode *dir, struct dentry *dentry,
> > +                          umode_t mode)
> > +{
> > +     struct super_block *sb = dir->i_sb;
> > +     struct fs_device_d *fsdev = container_of(sb, struct fs_device_d, sb);
> > +     struct ubootvarfs_data *data = fsdev->dev.priv;
> > +     struct inode *inode;
> > +     struct ubootvarfs_var *var;
> > +     size_t len = strlen(dentry->name);
> > +     /*
> > +      * We'll be adding <varname>=\0\0 to the end of our data, so
> > +      * we need to make sure there's enough room for it. Note that
> > +      * + 3 is to accoutn for '=', and two '\0' from above
>
> s/accoutn/account/
>
> > +static void ubootvarfs_remove(struct device_d *dev)
> > +{
> > +     struct ubootvarfs_data *data = dev->priv;
> > +
> > +     flush(data->fd);
> > +     close(data->fd);
> > +     free(data);
> > +}
>
> So the environment is written only when the FS is unmounted. We might
> want to have some other method to trigger writing. I don't know how this
> method may look yet though.
>

One idea would be to add .fsync() callback to FS drivers struct as
well as implement "fsync" command that will go through all of the
filesystems and call their ->fsync() methods.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/7] misc: Add a driver to expose U-Boot environment variable data

[Sascha Hauer]

On Tue, May 28, 2019 at 06:19:51PM -0700, Andrey Smirnov wrote:
> On Tue, May 28, 2019 at 2:34 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Mon, May 27, 2019 at 01:18:50PM -0700, Andrey Smirnov wrote:
> > > Add a driver to expose U-Boot environment variable data as a single
> > > mmap-able device. Not very useful on its own, it is a crucial
> > > low-level plumbing needed by filesystem driver introduced in the
> > > following commit.
> >
> > It wasn't clear to me why we need this driver at all, so it might be
> > worth adding a comment that this is for handling redundant env.
> >
> 
> Was it before of after you read the Kconfig help that talks about it?
> Asking to see if copying that text into commit would be enough, or if
> should write a more detailed explanation.

It was before I had read the Kconfig text. Indeed the Kconfig text makes
it clear.

> > > +      */
> > > +     ubdata->current = !ubdata->current;
> > > +
> > > +close_fd:
> > > +     close(fd);
> > > +     return ret;
> > > +}
> > > +
> > > +static struct cdev_operations ubootvar_ops = {
> > > +     .read = mem_read,
> > > +     .write = mem_write,
> > > +     .memmap = generic_memmap_rw,
> >
> > Ok, now I understand this struct resource thingy, you want to reuse
> > mem_read and friends. Given that these functions are oneliners to
> > implement I still suggest implementing them.
> >
> 
> OK, I'll have to make mem_copy() visible globally for them to remain
> one-liners, but that should be doable.

You won't need mem_copy(), plain memcpy() is enough. mem_copy() is only for
accessing registers in with the correct width, but that doesn't matter
here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 5/7] fs: Add a driver to access U-Boot environment variables

[Sascha Hauer]

On Tue, May 28, 2019 at 07:05:35PM -0700, Andrey Smirnov wrote:
> On Tue, May 28, 2019 at 2:56 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Mon, May 27, 2019 at 01:18:51PM -0700, Andrey Smirnov wrote:
> > > Add a driver working on top of ubootvar device and exposing U-Boot
> > > environment variable data as files.
> > >
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > Signed-off-by: Cory Tusar <cory.tusar@zii.aero>
> > > ---
> > >  fs/Kconfig      |   8 +
> > >  fs/Makefile     |   1 +
> > >  fs/ubootvarfs.c | 499 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 508 insertions(+)
> > >  create mode 100644 fs/ubootvarfs.c
> > >
> > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > index e3a95321c..adf281a5b 100644
> > > --- a/fs/Kconfig
> > > +++ b/fs/Kconfig
> > > @@ -118,4 +118,12 @@ config FS_RATP
> > >         This enables support for transferring files over RATP. A host can
> > >         export a directory which can then be mounted under barebox.
> > >
> > > +config FS_UBOOTVARFS
> > > +     bool
> > > +     depends on UBOOTVAR
> > > +     prompt "U-Boot environment variable filesystem support"
> > > +     help
> > > +       This filesystem driver provides access to U-Boot environment
> > > +       variables.
> > > +
> > >  endmenu
> > > diff --git a/fs/Makefile b/fs/Makefile
> > > index ac3e6a03a..9889a6507 100644
> > > --- a/fs/Makefile
> > > +++ b/fs/Makefile
> > > @@ -18,3 +18,4 @@ obj-$(CONFIG_FS_SMHFS) += smhfs.o
> > >  obj-$(CONFIG_FS_PSTORE) += pstore/
> > >  obj-$(CONFIG_FS_SQUASHFS) += squashfs/
> > >  obj-$(CONFIG_FS_RATP)        += ratpfs.o
> > > +obj-$(CONFIG_FS_UBOOTVARFS) += ubootvarfs.o
> > > diff --git a/fs/ubootvarfs.c b/fs/ubootvarfs.c
> > > new file mode 100644
> > > index 000000000..8de97b2de
> > > --- /dev/null
> > > +++ b/fs/ubootvarfs.c
> > > @@ -0,0 +1,499 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2019 Zodiac Inflight Innovations
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "ubootvarfs: " fmt
> > > +
> > > +#include <common.h>
> > > +#include <driver.h>
> > > +#include <init.h>
> > > +#include <malloc.h>
> > > +#include <fs.h>
> > > +#include <string.h>
> > > +#include <errno.h>
> > > +#include <linux/stat.h>
> > > +#include <xfuncs.h>
> > > +#include <fcntl.h>
> > > +#include <efi.h>
> > > +#include <wchar.h>
> > > +#include <linux/err.h>
> > > +#include <linux/ctype.h>
> > > +
> > > +/**
> > > + * Some theory of operation:
> > > + *
> > > + * U-Boot environment variable data is expected to be presented as a
> > > + * single blob containing an arbitrary number "<key>=<value>\0" paris
> >
> > s/paris/pairs/
> >
> > > + * without any other auxiliary information (accomplished by ubootvar
> > > + * driver)
> > > + *
> > > + * Filesystem driver code in this file parses above data an creates a
> > > + * linked list of all of the "variables" found (see @ubootvarfs_var to
> > > + * what information is recorded).
> > > + *
> > > + * With that in place reading or writing file data becomes as trivial
> > > + * as looking up a variable in the linked list by name and then
> > > + * memcpy()-ing bytes from its value region.
> > > + *
> > > + * The only moderately tricky part is resising a given file/variable
> >
> > s/resising/resizing/
> >
> > > + * since, given the underlying data format, it requires us to move all
> > > + * of the key/value data that comes after the given file/variable as
> > > + * well as to adjust all of the cached offsets stored in varaible
> >
> > s/varaible/variable/
> >
> > > + * linked list. See ubootvarfs_adjust() for the implementation
> > > + * details.
> >
> > Wouldn't it be easier and more straight forward to serialize and
> > deserialize the U-Boot environment as a whole rather than trying to
> > adjust an in-memory representation each time the env is changed?
> >
> 
> Eh, I don't think it'll save much if at all. Ubootvarfs_create() might
> loose a couple of lines of code, ubootvarfs_adjust() and
> ubootvarfs_relocate_tail() will go away, but new code to do
> serialization would partially negate gains there. Ubootvarfs_unlink()
> will probably loose a line or two ubootvarfs_truncate() would probably
> remain equally weird, just in a different way.
> 
> OTOH, going that way would mean that all of the data would have to be
> cached twice (once in ubootvar blob and then seprartely in
> ubootvarfs_var elements of the list) as well as having to do a bunch
> of unnecessary data processing. Given typical data size those problems
> are probably negligible, but combined with the fact that all of the
> code would have to be re-tested it makes me less than eager to go that
> way.
> 
> I can make this change if you insist, but if it is up to me, I'd rather not.

I don't care that much, hopefully I'll never have to look at this code
again ;)

> >
> > So the environment is written only when the FS is unmounted. We might
> > want to have some other method to trigger writing. I don't know how this
> > method may look yet though.
> >
> 
> One idea would be to add .fsync() callback to FS drivers struct as
> well as implement "fsync" command that will go through all of the
> filesystems and call their ->fsync() methods.

This sounds good at first. One thing I see is that both the U-Boot and
barebox environment is normally actively saved. You normally don't
expect that a fsync has effects that the user doesn't want to have (at
least not at the moment).
Anyway, we can leave this topic for later.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox