All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols
@ 2023-08-12  0:06 Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 1/4] mtd: ubi: register UBI attachments as DM devices Sam Edwards
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Sam Edwards @ 2023-08-12  0:06 UTC (permalink / raw)
  To: Heiko Schocher, Kyungmin Park; +Cc: u-boot, Sam Edwards

Hi UBI maintainers,

My target's rootfs is a read-only filesystem stored in a static UBI volume,
mounted via a "ubiblock" device after boot. I'd also like to keep the boot
files in the same filesystem, so that it's all coupled together. To that end,
I'm working on a patchset so that U-Boot can read static UBI volumes as
read-only block devices (paralleling Linux's ubiblock mechanism).

I'm very happy with how the first 3 patches in this series turned out (so I'm
not asking about them per se, though feedback is certainly welcome). The fourth
is where I got stuck: while the code definitely works, it requires bringing DM
headers into disk/part.c, which certainly will not fly in mainline. I need to
plumb this through drivers/block/blk-uclass.c to do it "properly."

Part of the problem here is that these are *volumes,* but they are (optionally)
*named.* In U-Boot's current view of block devices, it is only a partition, and
not a volume, that may have a name. These aren't "partitions" in U-Boot's view,
since a partition is a contiguous slice of a bigger block device, while these
are (logically) separate block devices.

So, I would need to invent a new function that can look up a named (sub)volume.
I also probably need to invent new syntax, so that I'm not overloading the 0:1
syntax for partitions. I'm not especially beholden to the ':', but I do want to
use the same syntax for volume numbers and volume names, to mirror Linux's
`ubi.block=x,y` syntax.

I'm also trying to reclaim the name "ubi" to refer to a UBI volume, while
U-Boot currently thinks it should refer to the presently-mounted UBIFS. In my
current series, the meaning of "ubi" depends on whether ubifs is mounted, for
backwards-compatibility. If this isn't palpable, I could consider other options
like "ubivol"/"ubiblock"/"ubiblk"/"ubistatic"/...

So, the feedback I'm hoping for would be:
1) What is a good syntax for referring to a logical volume by name or ID?
   Keeping in mind this may affect more than just UBI, if e.g. U-Boot learns to
   peer inside LVM2s in the future.
2) What should I call the block functions for looking up a block device's
   subvolume by type+parentidx+{name,ID}?
3) Is my strategy of reclaiming "ubi" sound, or should I be conceding that to
   UBIFS and using a new type name for static UBI volumes?
4) Does my choose_blksz_for_volume() function make sense, or should I always be
   using a preferred block size (like 512) if possible?

Cheers,
Sam

Sam Edwards (4):
  mtd: ubi: register UBI attachments as DM devices
  mtd: ubi: bind block device driver for static volumes
  disk: part: fall-through if "ubi" requested but ubifs not mounted
  HACK: enable access to `ubi 0:volname` block devices

 cmd/ubi.c                    |  11 +++
 disk/part.c                  |  70 +++++++++++--
 drivers/mtd/ubi/Makefile     |   1 +
 drivers/mtd/ubi/ubi-uclass.c | 184 +++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h       |   1 +
 include/ubi_uboot.h          |   5 +
 6 files changed, 264 insertions(+), 8 deletions(-)
 create mode 100644 drivers/mtd/ubi/ubi-uclass.c

-- 
2.41.0


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

* [RFC PATCH 1/4] mtd: ubi: register UBI attachments as DM devices
  2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
@ 2023-08-12  0:06 ` Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 2/4] mtd: ubi: bind block device driver for static volumes Sam Edwards
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Sam Edwards @ 2023-08-12  0:06 UTC (permalink / raw)
  To: Heiko Schocher, Kyungmin Park; +Cc: u-boot, Sam Edwards

This is in preparation for exposing static UBI volumes as block devices.

A UBI uclass and driver are introduced, and a "ubi0" virtual device
with the proper driver is created below whichever MTD device is
attached as the active UBI partition. This virtual device will soon
be the parent for the BLK devices that represent the static volumes.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 cmd/ubi.c                    | 11 ++++++
 drivers/mtd/ubi/Makefile     |  1 +
 drivers/mtd/ubi/ubi-uclass.c | 74 ++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h       |  1 +
 include/ubi_uboot.h          |  5 +++
 5 files changed, 92 insertions(+)
 create mode 100644 drivers/mtd/ubi/ubi-uclass.c

diff --git a/cmd/ubi.c b/cmd/ubi.c
index b61ae1efea..ab336d30f1 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -560,6 +560,13 @@ static int ubi_detach(void)
 		cmd_ubifs_umount();
 #endif
 
+#ifdef CONFIG_DM_MTD
+	/*
+	 * Clean up any UBI devices in DM
+	 */
+	ubi_dm_unbind_all();
+#endif
+
 	/*
 	 * Call ubi_exit() before re-initializing the UBI subsystem
 	 */
@@ -598,6 +605,10 @@ int ubi_part(char *part_name, const char *vid_header_offset)
 		return err;
 	}
 
+#ifdef CONFIG_DM_MTD
+	ubi_dm_bind(0);
+#endif
+
 	ubi = ubi_devices[0];
 
 	return 0;
diff --git a/drivers/mtd/ubi/Makefile b/drivers/mtd/ubi/Makefile
index 30d00fbdfe..375075f75e 100644
--- a/drivers/mtd/ubi/Makefile
+++ b/drivers/mtd/ubi/Makefile
@@ -7,3 +7,4 @@ obj-y += attach.o build.o vtbl.o vmt.o upd.o kapi.o eba.o io.o wl.o crc32.o
 obj-$(CONFIG_MTD_UBI_FASTMAP) += fastmap.o
 obj-y += misc.o
 obj-y += debug.o
+obj-$(CONFIG_DM_MTD) += ubi-uclass.o
diff --git a/drivers/mtd/ubi/ubi-uclass.c b/drivers/mtd/ubi/ubi-uclass.c
new file mode 100644
index 0000000000..f8971e793e
--- /dev/null
+++ b/drivers/mtd/ubi/ubi-uclass.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * ubi-uclass.c - UBI partition and volume block device uclass driver
+ *
+ * Copyright (C) 2023 Sam Edwards <CFSworks@gmail.com>
+ */
+
+#define LOG_CATEGORY UCLASS_UBI
+
+#include <common.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <ubi_uboot.h>
+
+int ubi_dm_bind(unsigned int index)
+{
+	struct udevice *dev;
+	int ret;
+	char name[16];
+	const char *name_dup;
+	struct ubi_device *ubi = ubi_devices[index];
+	const struct mtd_info *mtd = ubi->mtd;
+
+	/* MTD partitions are not in DM; navigate to the real MTD device */
+	if (mtd->parent)
+		mtd = mtd->parent;
+
+	snprintf(name, sizeof(name), "ubi%u", index);
+	name_dup = strdup(name);
+	ret = device_bind(mtd->dev, DM_DRIVER_GET(ubi), name_dup, ubi,
+			  ofnode_null(), &dev);
+	if (ret) {
+		free((void *)name_dup);
+		return ret;
+	}
+
+	device_set_name_alloced(dev);
+
+	return 0;
+}
+
+int ubi_dm_unbind_all(void)
+{
+	int ret;
+	struct uclass *uc;
+	struct udevice *dev;
+	struct udevice *next;
+
+	ret = uclass_get(UCLASS_UBI, &uc);
+	if (ret)
+		return ret;
+
+	uclass_foreach_dev_safe(dev, next, uc) {
+		ret = device_remove(dev, DM_REMOVE_NORMAL);
+		if (ret)
+			return ret;
+
+		ret = device_unbind(dev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+U_BOOT_DRIVER(ubi) = {
+	.id		= UCLASS_UBI,
+	.name		= "ubi_dev",
+};
+
+UCLASS_DRIVER(ubi) = {
+	.id		= UCLASS_UBI,
+	.name		= "ubi",
+};
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 307ad6931c..407166d080 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -133,6 +133,7 @@ enum uclass_id {
 	UCLASS_THERMAL,		/* Thermal sensor */
 	UCLASS_TIMER,		/* Timer device */
 	UCLASS_TPM,		/* Trusted Platform Module TIS interface */
+	UCLASS_UBI,		/* Unsorted Block Images MTD partition */
 	UCLASS_UFS,		/* Universal Flash Storage */
 	UCLASS_USB,		/* USB bus */
 	UCLASS_USB_DEV_GENERIC,	/* USB generic device */
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 6da348eb62..9d37848f03 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -52,6 +52,11 @@ extern int ubi_part(char *part_name, const char *vid_header_offset);
 extern int ubi_volume_write(char *volume, void *buf, size_t size);
 extern int ubi_volume_read(char *volume, char *buf, size_t size);
 
+#ifdef CONFIG_DM_MTD
+extern int ubi_dm_bind(unsigned int);
+extern int ubi_dm_unbind_all(void);
+#endif
+
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_mount(char *vol_name);
 int cmd_ubifs_umount(void);
-- 
2.41.0


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

* [RFC PATCH 2/4] mtd: ubi: bind block device driver for static volumes
  2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 1/4] mtd: ubi: register UBI attachments as DM devices Sam Edwards
@ 2023-08-12  0:06 ` Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 3/4] disk: part: fall-through if "ubi" requested but ubifs not mounted Sam Edwards
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Sam Edwards @ 2023-08-12  0:06 UTC (permalink / raw)
  To: Heiko Schocher, Kyungmin Park; +Cc: u-boot, Sam Edwards

This makes static UBI volumes readable as block devices, however
no mechanism for selecting these volume devices yet exists.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/mtd/ubi/ubi-uclass.c | 110 +++++++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/drivers/mtd/ubi/ubi-uclass.c b/drivers/mtd/ubi/ubi-uclass.c
index f8971e793e..231d6d90c7 100644
--- a/drivers/mtd/ubi/ubi-uclass.c
+++ b/drivers/mtd/ubi/ubi-uclass.c
@@ -8,10 +8,119 @@
 #define LOG_CATEGORY UCLASS_UBI
 
 #include <common.h>
+#include <blk.h>
 #include <dm.h>
 #include <dm/device-internal.h>
 #include <ubi_uboot.h>
 
+static ulong ubi_bread(struct udevice *dev, lbaint_t lba, lbaint_t blkcnt,
+		       void *dst)
+{
+	int err, lnum;
+	struct blk_desc *blk = dev_get_uclass_plat(dev);
+	struct ubi_device *ubi = dev_get_plat(dev->parent);
+	struct ubi_volume *vol = ubi->volumes[blk->devnum];
+	lbaint_t lba_per_peb = vol->usable_leb_size / blk->blksz;
+	lbaint_t lba_off, lba_len, total = 0;
+
+	while (blkcnt) {
+		lnum = lba / lba_per_peb;
+		lba_off = lba % lba_per_peb;
+		lba_len = lba_per_peb - lba_off;
+		if (lba_len > blkcnt)
+			lba_len = blkcnt;
+
+		err = ubi_eba_read_leb(ubi, vol, lnum, dst,
+				       lba_off << blk->log2blksz,
+				       lba_len << blk->log2blksz, 0);
+		if (err) {
+			pr_err("UBI read error %x\n", err);
+			break;
+		}
+
+		lba += lba_len;
+		blkcnt -= lba_len;
+		dst += lba_len << blk->log2blksz;
+		total += lba_len;
+	}
+
+	return total;
+}
+
+static const struct blk_ops ubi_block_ops = {
+	.read 	= ubi_bread,
+};
+
+U_BOOT_DRIVER(ubi_block) = {
+	.name	= "ubi_block",
+	.id	= UCLASS_BLK,
+	.ops	= &ubi_block_ops,
+};
+
+static bool is_power_of_two(unsigned int x)
+{
+	return (x & -x) == x;
+}
+
+static unsigned int choose_blksz_for_volume(const struct ubi_volume *vol)
+{
+	/*
+	 * U-Boot assumes a power-of-two blksz; however, UBI LEBs are
+	 * very often not suitably sized. To solve this, we divide the
+	 * LEBs into a whole number of LBAs per LEB, such that each LBA
+	 * addresses a power-of-two-sized block. To choose the blksz,
+	 * we either:
+	 * 1) Use the volume alignment, if it's a non-unity power of
+	 *    two. The LEB size is a multiple of this alignment, and it
+	 *    allows the user to force a particular blksz if needed for
+	 *    their use case.
+	 * 2) Otherwise, find the greatest power-of-two factor of the
+	 *    LEB size.
+	 */
+	if (vol->alignment > 1 && is_power_of_two(vol->alignment))
+		return vol->alignment;
+
+	unsigned int blksz = 1;
+	while ((vol->usable_leb_size & blksz) == 0)
+		blksz <<= 1;
+	return blksz;
+}
+
+static int ubi_post_bind(struct udevice *dev)
+{
+	int i;
+	int ret;
+	unsigned int blksz;
+	lbaint_t lba;
+	struct ubi_device *ubi = dev_get_plat(dev);
+
+	for (i = 0; i < ubi->vtbl_slots; i++) {
+		struct ubi_volume *vol = ubi->volumes[i];
+		if (!vol || vol->vol_id >= UBI_INTERNAL_VOL_START ||
+		    vol->vol_type != UBI_STATIC_VOLUME)
+			continue;
+
+		if (vol->updating || vol->upd_marker) {
+			pr_err("** UBI volume %d (\"%s\") midupdate; ignored\n",
+			       vol->vol_id, vol->name);
+			continue;
+		}
+
+		blksz = choose_blksz_for_volume(vol);
+		lba = DIV_ROUND_UP((unsigned long long)vol->used_bytes, blksz);
+
+		pr_debug("UBI volume %d (\"%s\"): %lu blocks, %d bytes each\n",
+			 vol->vol_id, vol->name, lba, blksz);
+
+		ret = blk_create_device(dev, "ubi_block", vol->name, UCLASS_UBI,
+					vol->vol_id, blksz, lba, NULL);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int ubi_dm_bind(unsigned int index)
 {
 	struct udevice *dev;
@@ -71,4 +180,5 @@ U_BOOT_DRIVER(ubi) = {
 UCLASS_DRIVER(ubi) = {
 	.id		= UCLASS_UBI,
 	.name		= "ubi",
+	.post_bind	= ubi_post_bind,
 };
-- 
2.41.0


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

* [RFC PATCH 3/4] disk: part: fall-through if "ubi" requested but ubifs not mounted
  2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 1/4] mtd: ubi: register UBI attachments as DM devices Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 2/4] mtd: ubi: bind block device driver for static volumes Sam Edwards
@ 2023-08-12  0:06 ` Sam Edwards
  2023-08-12  0:06 ` [RFC PATCH 4/4] HACK: enable access to `ubi 0:volname` block devices Sam Edwards
  2023-08-14  6:45 ` [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Heiko Schocher
  4 siblings, 0 replies; 8+ messages in thread
From: Sam Edwards @ 2023-08-12  0:06 UTC (permalink / raw)
  To: Heiko Schocher, Kyungmin Park; +Cc: u-boot, Sam Edwards

Since we're adding the ability to access static UBI volumes as block
devices, it is no longer an error to use the "ubi" ifname with UBIFS
unmounted.

Ideally, the access to UBIFS should instead be called "ubifs" but it
would break backwards compatibility to change this. Instead, use the
UBIFS mount status to disambiguate what the user means by "ubi"

There is no change in functionality in this patch: UBIFS access works
the same and an error still occurs when using "ubi" without UBIFS
mounted. The only difference is that now, the error message is a plain
"Bad device specification" and does not suggest using ubifsmount.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 disk/part.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/disk/part.c b/disk/part.c
index 0a03b8213d..1ad8277b65 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -492,15 +492,13 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
 
 #if IS_ENABLED(CONFIG_CMD_UBIFS) && !IS_ENABLED(CONFIG_SPL_BUILD)
 	/*
-	 * Special-case ubi, ubi goes through a mtd, rather than through
-	 * a regular block device.
+	 * Special-case ubifs, which does not go through the block device layer
+	 * and also must be mounted ahead of time. U-Boot has historically
+	 * called this "ubi" too, even though *static* UBI volumes are
+	 * accessible as block devices. For backward compatibility, assume that
+	 * when UBIFS is mounted, the user intends "ubi" to mean "ubifs."
 	 */
-	if (!strcmp(ifname, "ubi")) {
-		if (!ubifs_is_mounted()) {
-			printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
-			return -EINVAL;
-		}
-
+	if (ubifs_is_mounted() && !strcmp(ifname, "ubi")) {
 		strcpy((char *)info->type, BOOT_PART_TYPE);
 		strcpy((char *)info->name, "UBI");
 		return 0;
-- 
2.41.0


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

* [RFC PATCH 4/4] HACK: enable access to `ubi 0:volname` block devices
  2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
                   ` (2 preceding siblings ...)
  2023-08-12  0:06 ` [RFC PATCH 3/4] disk: part: fall-through if "ubi" requested but ubifs not mounted Sam Edwards
@ 2023-08-12  0:06 ` Sam Edwards
  2023-08-14  6:45 ` [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Heiko Schocher
  4 siblings, 0 replies; 8+ messages in thread
From: Sam Edwards @ 2023-08-12  0:06 UTC (permalink / raw)
  To: Heiko Schocher, Kyungmin Park; +Cc: u-boot, Sam Edwards

---
 disk/part.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 1ad8277b65..85eb51429a 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -14,6 +14,9 @@
 #include <malloc.h>
 #include <part.h>
 #include <ubifs_uboot.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
 
 #undef	PART_DEBUG
 
@@ -505,6 +508,59 @@ int blk_get_device_part_str(const char *ifname, const char *dev_part_str,
 	}
 #endif
 
+#if IS_ENABLED(CONFIG_CMD_UBI) && !IS_ENABLED(CONFIG_SPL_BUILD)
+	/*
+	 * Also special-case UBI, which may use names to find the specific
+	 * volumes, so this deviates a bit from the typical devnum:partnum
+	 * syntax.
+	 */
+	if (!strcmp(ifname, "ubi")) {
+		dev = hextoul(dev_part_str, &ep);
+		if (*ep == ':') {
+			struct udevice *ubi_dev = NULL;
+			struct udevice *vol_dev = NULL;
+			part_str = &ep[1];
+
+			ret = uclass_find_device(UCLASS_UBI, dev, &ubi_dev);
+			if (!ubi_dev || ret) {
+				printf("** Cannot find UBI %x\n", dev);
+				return -EINVAL;
+			}
+
+			part = hextoul(part_str, &ep);
+			if (!*ep) {
+				struct udevice *tmp_dev;
+				device_foreach_child(tmp_dev, ubi_dev) {
+					struct blk_desc *desc = dev_get_uclass_plat(tmp_dev);
+					if (desc->devnum == part) {
+						vol_dev = tmp_dev;
+						break;
+					}
+				}
+			}
+
+			if (!vol_dev)
+				ret = device_find_child_by_name(ubi_dev, part_str, &vol_dev);
+
+			if (!vol_dev || ret) {
+				printf("** UBI volume %s not found\n", part_str);
+				return -EINVAL;
+			}
+
+			ret = device_probe(vol_dev);
+			if (ret)
+				return ret;
+
+			*dev_desc = dev_get_uclass_plat(vol_dev);
+			part_get_info_whole_disk(*dev_desc, info);
+			return 0;
+		}
+
+		printf("UBIFS not mounted, use ubifsmount to mount volume first!\n");
+		return -EINVAL;
+	}
+#endif
+
 	/* If no dev_part_str, use bootdevice environment variable */
 	if (!dev_part_str || !strlen(dev_part_str) ||
 	    !strcmp(dev_part_str, "-"))
-- 
2.41.0


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

* Re: [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols
  2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
                   ` (3 preceding siblings ...)
  2023-08-12  0:06 ` [RFC PATCH 4/4] HACK: enable access to `ubi 0:volname` block devices Sam Edwards
@ 2023-08-14  6:45 ` Heiko Schocher
  2023-09-07 21:46   ` Sam Edwards
  4 siblings, 1 reply; 8+ messages in thread
From: Heiko Schocher @ 2023-08-14  6:45 UTC (permalink / raw)
  To: Sam Edwards, Simon Glass; +Cc: u-boot, Kyungmin Park

Hello Sam,

many thnkas for your patchset!

On 12.08.23 02:06, Sam Edwards wrote:
> Hi UBI maintainers,
> 
> My target's rootfs is a read-only filesystem stored in a static UBI volume,
> mounted via a "ubiblock" device after boot. I'd also like to keep the boot
> files in the same filesystem, so that it's all coupled together. To that end,
> I'm working on a patchset so that U-Boot can read static UBI volumes as
> read-only block devices (paralleling Linux's ubiblock mechanism).
> 
> I'm very happy with how the first 3 patches in this series turned out (so I'm
> not asking about them per se, though feedback is certainly welcome). The fourth
> is where I got stuck: while the code definitely works, it requires bringing DM
> headers into disk/part.c, which certainly will not fly in mainline. I need to
> plumb this through drivers/block/blk-uclass.c to do it "properly."
> 
> Part of the problem here is that these are *volumes,* but they are (optionally)
> *named.* In U-Boot's current view of block devices, it is only a partition, and
> not a volume, that may have a name. These aren't "partitions" in U-Boot's view,
> since a partition is a contiguous slice of a bigger block device, while these
> are (logically) separate block devices.
> 
> So, I would need to invent a new function that can look up a named (sub)volume.
> I also probably need to invent new syntax, so that I'm not overloading the 0:1
> syntax for partitions. I'm not especially beholden to the ':', but I do want to
> use the same syntax for volume numbers and volume names, to mirror Linux's
> `ubi.block=x,y` syntax.
> 
> I'm also trying to reclaim the name "ubi" to refer to a UBI volume, while
> U-Boot currently thinks it should refer to the presently-mounted UBIFS. In my
> current series, the meaning of "ubi" depends on whether ubifs is mounted, for
> backwards-compatibility. If this isn't palpable, I could consider other options
> like "ubivol"/"ubiblock"/"ubiblk"/"ubistatic"/...
> 
> So, the feedback I'm hoping for would be:
> 1) What is a good syntax for referring to a logical volume by name or ID?
>    Keeping in mind this may affect more than just UBI, if e.g. U-Boot learns to
>    peer inside LVM2s in the future.

Yes, we should have here some generic part...

> 2) What should I call the block functions for looking up a block device's
>    subvolume by type+parentidx+{name,ID}?
> 3) Is my strategy of reclaiming "ubi" sound, or should I be conceding that to
>    UBIFS and using a new type name for static UBI volumes?
> 4) Does my choose_blksz_for_volume() function make sense, or should I always be
>    using a preferred block size (like 512) if possible?

Your patches look good to me, and yes, we have to discuss changes in disk/part.c

I added Simon here as he has much more knowledge here, hope he can comment
this part.

bye,
Heiko
> 
> Cheers,
> Sam
> 
> Sam Edwards (4):
>   mtd: ubi: register UBI attachments as DM devices
>   mtd: ubi: bind block device driver for static volumes
>   disk: part: fall-through if "ubi" requested but ubifs not mounted
>   HACK: enable access to `ubi 0:volname` block devices
> 
>  cmd/ubi.c                    |  11 +++
>  disk/part.c                  |  70 +++++++++++--
>  drivers/mtd/ubi/Makefile     |   1 +
>  drivers/mtd/ubi/ubi-uclass.c | 184 +++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h       |   1 +
>  include/ubi_uboot.h          |   5 +
>  6 files changed, 264 insertions(+), 8 deletions(-)
>  create mode 100644 drivers/mtd/ubi/ubi-uclass.c
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

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

* Re: [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols
  2023-08-14  6:45 ` [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Heiko Schocher
@ 2023-09-07 21:46   ` Sam Edwards
  2023-09-21  6:44     ` Heiko Schocher
  0 siblings, 1 reply; 8+ messages in thread
From: Sam Edwards @ 2023-09-07 21:46 UTC (permalink / raw)
  To: hs, Simon Glass; +Cc: u-boot, Kyungmin Park

Hi Heiko and Simon,

Thought I'd follow-up to keep this discussion going. The main thing I 
would like to decide first (as it lets me start relying on it in boot 
scripts) would be the UBI access syntax:

=> ls ubi 0:rootfs /boot
=> ls ubi 0:2 /boot

Do those look good? Should I be trying to mimic the accepted syntax of 
fs/ubifs/super.c:open_ubi()? Perhaps "ubi 0!rootfs" and/or "ubi 0_2"? 
Not using ':' leaves open the possibility for logical volumes (LVM2/UBI) 
to contain partitions - not that I expect anyone will want that. :)

Cheers,
Sam

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

* Re: [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols
  2023-09-07 21:46   ` Sam Edwards
@ 2023-09-21  6:44     ` Heiko Schocher
  0 siblings, 0 replies; 8+ messages in thread
From: Heiko Schocher @ 2023-09-21  6:44 UTC (permalink / raw)
  To: Sam Edwards, Simon Glass; +Cc: u-boot, Kyungmin Park

Hello Sam,

sorry for the late reply...

On 07.09.23 23:46, Sam Edwards wrote:
> Hi Heiko and Simon,
> 
> Thought I'd follow-up to keep this discussion going. The main thing I would like to decide first (as
> it lets me start relying on it in boot scripts) would be the UBI access syntax:
> 
> => ls ubi 0:rootfs /boot
> => ls ubi 0:2 /boot

Looks perfect for me.

> Do those look good? Should I be trying to mimic the accepted syntax of fs/ubifs/super.c:open_ubi()?
> Perhaps "ubi 0!rootfs" and/or "ubi 0_2"? Not using ':' leaves open the possibility for logical
> volumes (LVM2/UBI) to contain partitions - not that I expect anyone will want that. :)

Good question... You never know... so from my side, yes it would
be good to allow both options ... so "ubi 0:2" and "ubi 0_2",
something like if ":" is in use "ubi 0:2" else try "ubi 0_2"

Please rebase your patchset when 2023.10 is out, thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

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

end of thread, other threads:[~2023-09-21  6:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-12  0:06 [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Sam Edwards
2023-08-12  0:06 ` [RFC PATCH 1/4] mtd: ubi: register UBI attachments as DM devices Sam Edwards
2023-08-12  0:06 ` [RFC PATCH 2/4] mtd: ubi: bind block device driver for static volumes Sam Edwards
2023-08-12  0:06 ` [RFC PATCH 3/4] disk: part: fall-through if "ubi" requested but ubifs not mounted Sam Edwards
2023-08-12  0:06 ` [RFC PATCH 4/4] HACK: enable access to `ubi 0:volname` block devices Sam Edwards
2023-08-14  6:45 ` [RFC PATCH 0/4] mtd: ubi: Enable accessing RO filesystems in UBI vols Heiko Schocher
2023-09-07 21:46   ` Sam Edwards
2023-09-21  6:44     ` 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.