[PATCH v3 00/10] Add support for ubi environment to create volumes (v3)

[PATCH v3 00/10] Add support for ubi environment to create volumes (v3)

[Weijie Gao]

This patch series expose more ubi helpers to allow the ubi environment to
create ubi env volumes if not exist.

Changes:
v2: Add const qualifier for char */void * in function parameters
    Adjust normal command message

v3: Fix command messages
    Fix some incorrect code flow and error handling
    Correct dynamic parameter type
    Add comments for exported functions

Weijie Gao (10):
  ubi: remove unnecessary extern directive from function prototypes
  cmd: ubi: mark read-only function parameters with const
  cmd: ubi: use void * for buf parameter in ubi_volume_read
  cmd: ubifs: mark string parameters with const
  cmd: ubi: change the type of parameter dynamic to bool
  cmd: ubi: reorganize command messages
  cmd: ubi: export more APIs to public
  cmd: ubi: allow creating volume with all free spaces in ubi_create_vol
  ubi: add comments for exported ubi API functions
  env: ubi: add support to create environment volume if it does not
    exist

 cmd/ubi.c             | 162 ++++++++++++++++++++++++++++--------------
 cmd/ubifs.c           |   2 +-
 env/Kconfig           |  11 +++
 env/ubi.c             |  39 +++++++++-
 fs/ubifs/super.c      |   2 +-
 fs/ubifs/ubifs.c      |   2 +-
 include/ubi_uboot.h   | 106 +++++++++++++++++++++++++--
 include/ubifs_uboot.h |   4 +-
 8 files changed, 260 insertions(+), 68 deletions(-)

-- 
2.45.2

[PATCH v3 01/10] ubi: remove unnecessary extern directive from function prototypes

[Weijie Gao]

The extern directive is unnecessary for function declaration and should be
removed.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2: new
v3: not changed
---
 include/ubi_uboot.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index ea0db69c72a..05fb9634d81 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -44,12 +44,12 @@
 #endif
 
 /* functions */
-extern int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
-extern int ubi_init(void);
-extern void ubi_exit(void);
-extern int ubi_part(char *part_name, const char *vid_header_offset);
-extern int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size);
-extern int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size);
+int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
+int ubi_init(void);
+void ubi_exit(void);
+int ubi_part(char *part_name, const char *vid_header_offset);
+int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size);
+int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_mount(char *vol_name);
-- 
2.45.2

[PATCH v3 02/10] cmd: ubi: mark read-only function parameters with const

[Weijie Gao]

Parameters like part/volume name and buffer for writing are not being
modified by the callee functions and should be marked const.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2: new
v3: not changed
---
 cmd/ubi.c           | 41 ++++++++++++++++++++++-------------------
 include/ubi_uboot.h |  7 ++++---
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 93de6f3aea2..f75ceff11ee 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -39,7 +39,7 @@ static struct ubi_device *ubi;
 #include <ubifs_uboot.h>
 #endif
 
-static void display_volume_info(struct ubi_device *ubi)
+static void display_volume_info(const struct ubi_device *ubi)
 {
 	int i;
 
@@ -50,7 +50,7 @@ static void display_volume_info(struct ubi_device *ubi)
 	}
 }
 
-static void display_ubi_info(struct ubi_device *ubi)
+static void display_ubi_info(const struct ubi_device *ubi)
 {
 	ubi_msg("MTD device name:            \"%s\"", ubi->mtd->name);
 	ubi_msg("MTD device size:            %llu MiB", ubi->flash_size >> 20);
@@ -149,12 +149,12 @@ static int ubi_list(const char *var, int numeric)
 	return 0;
 }
 
-static int ubi_check_volumename(const struct ubi_volume *vol, char *name)
+static int ubi_check_volumename(const struct ubi_volume *vol, const char *name)
 {
 	return strcmp(vol->name, name);
 }
 
-static int ubi_check(char *name)
+static int ubi_check(const char *name)
 {
 	int i;
 
@@ -213,8 +213,8 @@ bad:
 	return err;
 }
 
-static int ubi_create_vol(char *volume, int64_t size, int dynamic, int vol_id,
-			  bool skipcheck)
+static int ubi_create_vol(const char *volume, int64_t size, int dynamic,
+			  int vol_id, bool skipcheck)
 {
 	struct ubi_mkvol_req req;
 	int err;
@@ -247,7 +247,7 @@ static int ubi_create_vol(char *volume, int64_t size, int dynamic, int vol_id,
 	return ubi_create_volume(ubi, &req);
 }
 
-static struct ubi_volume *ubi_find_volume(char *volume)
+static struct ubi_volume *ubi_find_volume(const char *volume)
 {
 	struct ubi_volume *vol;
 	int i;
@@ -262,7 +262,7 @@ static struct ubi_volume *ubi_find_volume(char *volume)
 	return NULL;
 }
 
-static int ubi_remove_vol(char *volume)
+static int ubi_remove_vol(const char *volume)
 {
 	int err, reserved_pebs, i;
 	struct ubi_volume *vol;
@@ -316,7 +316,7 @@ out_err:
 	return err;
 }
 
-static int ubi_rename_vol(char *oldname, char *newname)
+static int ubi_rename_vol(const char *oldname, const char *newname)
 {
 	struct ubi_volume *vol;
 	struct ubi_rename_entry rename;
@@ -354,7 +354,8 @@ static int ubi_rename_vol(char *oldname, char *newname)
 	return ubi_rename_volumes(ubi, &list);
 }
 
-static int ubi_volume_continue_write(char *volume, void *buf, size_t size)
+static int ubi_volume_continue_write(const char *volume, const void *buf,
+				     size_t size)
 {
 	int err;
 	struct ubi_volume *vol;
@@ -394,8 +395,8 @@ static int ubi_volume_continue_write(char *volume, void *buf, size_t size)
 	return 0;
 }
 
-int ubi_volume_begin_write(char *volume, void *buf, size_t size,
-	size_t full_size)
+int ubi_volume_begin_write(const char *volume, const void *buf, size_t size,
+			   size_t full_size)
 {
 	int err;
 	int rsvd_bytes;
@@ -424,8 +425,8 @@ int ubi_volume_begin_write(char *volume, void *buf, size_t size,
 	return ubi_volume_continue_write(volume, buf, size);
 }
 
-static int ubi_volume_offset_write(char *volume, void *buf, loff_t offset,
-				   size_t size)
+static int ubi_volume_offset_write(const char *volume, const void *buf,
+				   loff_t offset, size_t size)
 {
 	int len, tbuf_size, ret;
 	u64 lnum;
@@ -487,7 +488,8 @@ exit:
 	return ret;
 }
 
-int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size)
+int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
+		     size_t size)
 {
 	int ret;
 
@@ -503,7 +505,7 @@ int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size)
 	return ret;
 }
 
-int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
+int ubi_volume_read(const char *volume, char *buf, loff_t offset, size_t size)
 {
 	int err, lnum, off, len, tbuf_size;
 	void *tbuf;
@@ -587,7 +589,8 @@ int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size)
 	return err;
 }
 
-static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
+static int ubi_dev_scan(const struct mtd_info *info,
+			const char *vid_header_offset)
 {
 	char ubi_mtd_param_buffer[80];
 	int err;
@@ -611,7 +614,7 @@ static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
 	return 0;
 }
 
-static int ubi_set_skip_check(char *volume, bool skip_check)
+static int ubi_set_skip_check(const char *volume, bool skip_check)
 {
 	struct ubi_vtbl_record vtbl_rec;
 	struct ubi_volume *vol;
@@ -658,7 +661,7 @@ static int ubi_detach(void)
 	return 0;
 }
 
-int ubi_part(char *part_name, const char *vid_header_offset)
+int ubi_part(const char *part_name, const char *vid_header_offset)
 {
 	struct mtd_info *mtd;
 	int err;
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 05fb9634d81..a949d1b80dd 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -47,9 +47,10 @@
 int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
 int ubi_init(void);
 void ubi_exit(void);
-int ubi_part(char *part_name, const char *vid_header_offset);
-int ubi_volume_write(char *volume, void *buf, loff_t offset, size_t size);
-int ubi_volume_read(char *volume, char *buf, loff_t offset, size_t size);
+int ubi_part(const char *part_name, const char *vid_header_offset);
+int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
+		     size_t size);
+int ubi_volume_read(const char *volume, char *buf, loff_t offset, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_mount(char *vol_name);
-- 
2.45.2

[PATCH v3 03/10] cmd: ubi: use void * for buf parameter in ubi_volume_read

[Weijie Gao]

Use void * to avoid explicit type casting as what ubi_volume_write has done
already.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2: new
v3: not changed
---
 cmd/ubi.c           | 4 ++--
 env/ubi.c           | 4 ++--
 include/ubi_uboot.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index f75ceff11ee..8e3cfaaddbb 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -505,7 +505,7 @@ int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
 	return ret;
 }
 
-int ubi_volume_read(const char *volume, char *buf, loff_t offset, size_t size)
+int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size)
 {
 	int err, lnum, off, len, tbuf_size;
 	void *tbuf;
@@ -877,7 +877,7 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		}
 
 		if (argc == 3) {
-			return ubi_volume_read(argv[3], (char *)addr, 0, size);
+			return ubi_volume_read(argv[3], (void *)addr, 0, size);
 		}
 	}
 
diff --git a/env/ubi.c b/env/ubi.c
index e9865b45ebc..46970ba754f 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -132,14 +132,14 @@ static int env_ubi_load(void)
 		return -EIO;
 	}
 
-	read1_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME, (void *)tmp_env1, 0,
+	read1_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME, tmp_env1, 0,
 				     CONFIG_ENV_SIZE);
 	if (read1_fail)
 		printf("\n** Unable to read env from %s:%s **\n",
 		       CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME);
 
 	read2_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME_REDUND,
-				     (void *)tmp_env2, 0, CONFIG_ENV_SIZE);
+				     tmp_env2, 0, CONFIG_ENV_SIZE);
 	if (read2_fail)
 		printf("\n** Unable to read redundant env from %s:%s **\n",
 		       CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME_REDUND);
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index a949d1b80dd..dd22ec7537a 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -50,7 +50,7 @@ void ubi_exit(void);
 int ubi_part(const char *part_name, const char *vid_header_offset);
 int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
 		     size_t size);
-int ubi_volume_read(const char *volume, char *buf, loff_t offset, size_t size);
+int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_mount(char *vol_name);
-- 
2.45.2

[PATCH v3 04/10] cmd: ubifs: mark string parameters with const

[Weijie Gao]

File name and volume name should be const as they will not be modified in
these functions.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2: new
v3: not changed
---
 cmd/ubifs.c           | 2 +-
 fs/ubifs/super.c      | 2 +-
 fs/ubifs/ubifs.c      | 2 +-
 include/ubi_uboot.h   | 2 +-
 include/ubifs_uboot.h | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cmd/ubifs.c b/cmd/ubifs.c
index 22e95db8ca5..81f2d37fc7b 100644
--- a/cmd/ubifs.c
+++ b/cmd/ubifs.c
@@ -19,7 +19,7 @@
 static int ubifs_initialized;
 static int ubifs_mounted;
 
-int cmd_ubifs_mount(char *vol_name)
+int cmd_ubifs_mount(const char *vol_name)
 {
 	int ret;
 
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index b6004b88f4e..9c8974a97a5 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2694,7 +2694,7 @@ MODULE_VERSION(__stringify(UBIFS_VERSION));
 MODULE_AUTHOR("Artem Bityutskiy, Adrian Hunter");
 MODULE_DESCRIPTION("UBIFS - UBI File System");
 #else
-int uboot_ubifs_mount(char *vol_name)
+int uboot_ubifs_mount(const char *vol_name)
 {
 	struct dentry *ret;
 	int flags;
diff --git a/fs/ubifs/ubifs.c b/fs/ubifs/ubifs.c
index b0cc0d2e1b2..f6f21e6af22 100644
--- a/fs/ubifs/ubifs.c
+++ b/fs/ubifs/ubifs.c
@@ -974,7 +974,7 @@ void ubifs_close(void)
 }
 
 /* Compat wrappers for common/cmd_ubifs.c */
-int ubifs_load(char *filename, unsigned long addr, u32 size)
+int ubifs_load(const char *filename, unsigned long addr, u32 size)
 {
 	loff_t actread;
 	int err;
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index dd22ec7537a..6ebd8a3b613 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -53,7 +53,7 @@ int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
 int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);
 
 extern struct ubi_device *ubi_devices[];
-int cmd_ubifs_mount(char *vol_name);
+int cmd_ubifs_mount(const char *vol_name);
 int cmd_ubifs_umount(void);
 
 #if IS_ENABLED(CONFIG_UBI_BLOCK)
diff --git a/include/ubifs_uboot.h b/include/ubifs_uboot.h
index db8a29e9bbd..0877dd84f99 100644
--- a/include/ubifs_uboot.h
+++ b/include/ubifs_uboot.h
@@ -18,10 +18,10 @@ struct blk_desc;
 struct disk_partition;
 
 int ubifs_init(void);
-int uboot_ubifs_mount(char *vol_name);
+int uboot_ubifs_mount(const char *vol_name);
 void uboot_ubifs_umount(void);
 int ubifs_is_mounted(void);
-int ubifs_load(char *filename, unsigned long addr, u32 size);
+int ubifs_load(const char *filename, unsigned long addr, u32 size);
 
 int ubifs_set_blk_dev(struct blk_desc *rbdd, struct disk_partition *info);
 int ubifs_ls(const char *dir_name);
-- 
2.45.2

[PATCH v3 05/10] cmd: ubi: change the type of parameter dynamic to bool

[Weijie Gao]

This patch changes the type of the 'dynamic' parameter of ubi_create_vol()
to bool as it's used as a boolean.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
v3: new
---
 cmd/ubi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 8e3cfaaddbb..8d04ff61fb6 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -213,7 +213,7 @@ bad:
 	return err;
 }
 
-static int ubi_create_vol(const char *volume, int64_t size, int dynamic,
+static int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
 			  int vol_id, bool skipcheck)
 {
 	struct ubi_mkvol_req req;
@@ -765,7 +765,7 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 
 	if (strncmp(argv[1], "create", 6) == 0) {
-		int dynamic = 1;	/* default: dynamic volume */
+		bool dynamic = true;	/* default: dynamic volume */
 		int id = UBI_VOL_NUM_AUTO;
 
 		/* Use maximum available size */
@@ -786,7 +786,7 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		/* E.g., create volume size type */
 		if (argc == 5) {
 			if (strncmp(argv[4], "s", 1) == 0)
-				dynamic = 0;
+				dynamic = false;
 			else if (strncmp(argv[4], "d", 1) != 0) {
 				printf("Incorrect type\n");
 				return 1;
-- 
2.45.2

[PATCH v3 06/10] cmd: ubi: reorganize command messages

[Weijie Gao]

This patch moves normal subcommand messages into the main command function.
This will allow current and potential api functions being called with clean
output.

A new function ubi_require_volume() is added for finding and printing error
message if volume not found. The original ubi_find_volume() will be silent
for being an api function.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
v2: new
v3: Fix use-after-free and "ubi read" command message displaying error
---
 cmd/ubi.c | 109 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 79 insertions(+), 30 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 8d04ff61fb6..9f121ea2931 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -241,8 +241,7 @@ static int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
 		printf("verify_mkvol_req failed %d\n", err);
 		return err;
 	}
-	printf("Creating %s volume %s of size %lld\n",
-		dynamic ? "dynamic" : "static", volume, size);
+
 	/* Call real ubi create volume */
 	return ubi_create_volume(ubi, &req);
 }
@@ -258,10 +257,19 @@ static struct ubi_volume *ubi_find_volume(const char *volume)
 			return vol;
 	}
 
-	printf("Volume %s not found!\n", volume);
 	return NULL;
 }
 
+static struct ubi_volume *ubi_require_volume(const char *volume)
+{
+	struct ubi_volume *vol = ubi_find_volume(volume);
+
+	if (!vol)
+		printf("Volume %s not found!\n", volume);
+
+	return vol;
+}
+
 static int ubi_remove_vol(const char *volume)
 {
 	int err, reserved_pebs, i;
@@ -271,8 +279,6 @@ static int ubi_remove_vol(const char *volume)
 	if (vol == NULL)
 		return ENODEV;
 
-	printf("Remove UBI volume %s (id %d)\n", vol->name, vol->vol_id);
-
 	if (ubi->ro_mode) {
 		printf("It's read-only mode\n");
 		err = EROFS;
@@ -334,8 +340,6 @@ static int ubi_rename_vol(const char *oldname, const char *newname)
 		return EINVAL;
 	}
 
-	printf("Rename UBI volume %s to %s\n", oldname, newname);
-
 	if (ubi->ro_mode) {
 		printf("%s: ubi device is in read-only mode\n", __func__);
 		return EROFS;
@@ -360,7 +364,7 @@ static int ubi_volume_continue_write(const char *volume, const void *buf,
 	int err;
 	struct ubi_volume *vol;
 
-	vol = ubi_find_volume(volume);
+	vol = ubi_require_volume(volume);
 	if (vol == NULL)
 		return ENODEV;
 
@@ -402,7 +406,7 @@ int ubi_volume_begin_write(const char *volume, const void *buf, size_t size,
 	int rsvd_bytes;
 	struct ubi_volume *vol;
 
-	vol = ubi_find_volume(volume);
+	vol = ubi_require_volume(volume);
 	if (vol == NULL)
 		return ENODEV;
 
@@ -434,7 +438,7 @@ static int ubi_volume_offset_write(const char *volume, const void *buf,
 	loff_t off = offset;
 	void *tbuf;
 
-	vol = ubi_find_volume(volume);
+	vol = ubi_require_volume(volume);
 	if (!vol)
 		return -ENODEV;
 
@@ -514,7 +518,7 @@ int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size)
 	loff_t offp = offset;
 	size_t len_read;
 
-	vol = ubi_find_volume(volume);
+	vol = ubi_require_volume(volume);
 	if (vol == NULL)
 		return ENODEV;
 
@@ -529,12 +533,8 @@ int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size)
 	if (offp == vol->used_bytes)
 		return 0;
 
-	if (size == 0) {
-		printf("No size specified -> Using max size (%lld)\n", vol->used_bytes);
+	if (size == 0)
 		size = vol->used_bytes;
-	}
-
-	printf("Read %zu bytes from volume %s to %p\n", size, volume, buf);
 
 	if (vol->corrupted)
 		printf("read from corrupted volume %d", vol->vol_id);
@@ -619,13 +619,10 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)
 	struct ubi_vtbl_record vtbl_rec;
 	struct ubi_volume *vol;
 
-	vol = ubi_find_volume(volume);
+	vol = ubi_require_volume(volume);
 	if (!vol)
 		return ENODEV;
 
-	printf("%sing skip_check on volume %s\n",
-	       skip_check ? "Sett" : "Clear", volume);
-
 	vtbl_rec = ubi->vtbl[vol->vol_id];
 	if (skip_check) {
 		vtbl_rec.flags |= UBI_VTBL_SKIP_CRC_CHECK_FLG;
@@ -698,6 +695,8 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	int64_t size;
 	ulong addr = 0;
 	bool skipcheck = false;
+	struct ubi_volume *vol;
+	int ret;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		}
 		/* E.g., create volume */
 		if (argc == 3) {
-			return ubi_create_vol(argv[2], size, dynamic, id,
-					      skipcheck);
+			ret = ubi_create_vol(argv[2], size, dynamic, id,
+					     skipcheck);
+			if (!ret)
+				printf("Created %s volume %s of size %lld\n",
+				       dynamic ? "dynamic" : "static", argv[2],
+				       size);
+			else if (ret == -EEXIST)
+				printf("Volume %s already exists!\n", argv[2]);
+
+			return ret;
 		}
 	}
 
 	if (strncmp(argv[1], "remove", 6) == 0) {
 		/* E.g., remove volume */
-		if (argc == 3)
-			return ubi_remove_vol(argv[2]);
+		if (argc == 3) {
+			int vol_id;
+
+			vol = ubi_find_volume(argv[2]);
+			if (!vol)
+				return 0;
+
+			vol_id = vol->vol_id;
+
+			ret = ubi_remove_vol(argv[2]);
+			if (!ret) {
+				printf("Removed UBI volume %s (id %d)\n",
+				       argv[2], vol_id);
+			}
+
+			return ret;
+		}
 	}
 
-	if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1], "rename", 6))
-		return ubi_rename_vol(argv[2], argv[3]);
+	if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1], "rename", 6)) {
+		ret = ubi_rename_vol(argv[2], argv[3]);
+		if (!ret) {
+			printf("UBI volume %s renamed to %s\n", argv[2],
+			       argv[3]);
+		}
+
+		return ret;
+	}
 
 	if (strncmp(argv[1], "skipcheck", 9) == 0) {
 		/* E.g., change skip_check flag */
 		if (argc == 4) {
 			skipcheck = strncmp(argv[3], "on", 2) == 0;
-			return ubi_set_skip_check(argv[2], skipcheck);
+			ret = ubi_set_skip_check(argv[2], skipcheck);
+			if (!ret) {
+				printf("%s skip_check on volume %s\n",
+				       skipcheck ? "Set" : "Cleared", argv[2]);
+			}
+
+			return ret;
 		}
 	}
 
 	if (strncmp(argv[1], "write", 5) == 0) {
-		int ret;
-
 		if (argc < 5) {
 			printf("Please see usage\n");
 			return 1;
@@ -877,7 +910,23 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		}
 
 		if (argc == 3) {
-			return ubi_volume_read(argv[3], (void *)addr, 0, size);
+			if (!size) {
+				vol = ubi_require_volume(argv[3]);
+				if (!vol)
+					return 1;
+
+				printf("No size specified -> Using max size (%lld)\n",
+				       vol->used_bytes);
+				size = vol->used_bytes;
+			}
+
+			ret = ubi_volume_read(argv[3], (void *)addr, 0, size);
+			if (!ret) {
+				printf("%lld bytes read from volume %s to 0x%lx\n",
+				       size, argv[3], addr);
+			}
+
+			return ret;
 		}
 	}
 
-- 
2.45.2

[PATCH v3 07/10] cmd: ubi: export more APIs to public

[Weijie Gao]

Export the following functions to public:

- ubi_detach(): this is paired with ubi_part(). One may call this function
  to completely clean up the ubi subsystem after using ubi_part().

- ubi_{create,find,remove}_vol: this is a set of functions for volume
  management.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
v2: not changed
v3: updated commit message
---
 cmd/ubi.c           | 10 +++++-----
 include/ubi_uboot.h |  5 +++++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index 9f121ea2931..c8b31f92662 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -213,8 +213,8 @@ bad:
 	return err;
 }
 
-static int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
-			  int vol_id, bool skipcheck)
+int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,
+		   bool skipcheck)
 {
 	struct ubi_mkvol_req req;
 	int err;
@@ -246,7 +246,7 @@ static int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
 	return ubi_create_volume(ubi, &req);
 }
 
-static struct ubi_volume *ubi_find_volume(const char *volume)
+struct ubi_volume *ubi_find_volume(const char *volume)
 {
 	struct ubi_volume *vol;
 	int i;
@@ -270,7 +270,7 @@ static struct ubi_volume *ubi_require_volume(const char *volume)
 	return vol;
 }
 
-static int ubi_remove_vol(const char *volume)
+int ubi_remove_vol(const char *volume)
 {
 	int err, reserved_pebs, i;
 	struct ubi_volume *vol;
@@ -635,7 +635,7 @@ static int ubi_set_skip_check(const char *volume, bool skip_check)
 	return ubi_change_vtbl_record(ubi, vol->vol_id, &vtbl_rec);
 }
 
-static int ubi_detach(void)
+int ubi_detach(void)
 {
 #ifdef CONFIG_CMD_UBIFS
 	/*
diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index 6ebd8a3b613..ee78f02ae94 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -47,10 +47,15 @@
 int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
 int ubi_init(void);
 void ubi_exit(void);
+int ubi_detach(void);
 int ubi_part(const char *part_name, const char *vid_header_offset);
 int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
 		     size_t size);
 int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);
+int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,
+		   bool skipcheck);
+struct ubi_volume *ubi_find_volume(const char *volume);
+int ubi_remove_vol(const char *volume);
 
 extern struct ubi_device *ubi_devices[];
 int cmd_ubifs_mount(const char *vol_name);
-- 
2.45.2

[PATCH v3 08/10] cmd: ubi: allow creating volume with all free spaces in ubi_create_vol

[Weijie Gao]

Although the ubi command itself supports creating volume with all
free spaces, the api ubi_create_vol() does not.

Since negative size is invalid, this patch replaces negative size
with all free space size in ubi_create_vol().

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2: not changed
v3: not changed
---
 cmd/ubi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cmd/ubi.c b/cmd/ubi.c
index c8b31f92662..9c008e78ab1 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -226,7 +226,11 @@ int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,
 
 	req.vol_id = vol_id;
 	req.alignment = 1;
-	req.bytes = size;
+
+	if (size < 0)
+		req.bytes = ubi->avail_pebs * ubi->leb_size;
+	else
+		req.bytes = size;
 
 	strcpy(req.name, volume);
 	req.name_len = strlen(volume);
-- 
2.45.2

[PATCH v3 09/10] ubi: add comments for exported ubi API functions

[Weijie Gao]

This patch adds comments for exported ubi command API functions.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
v3: new
---
 include/ubi_uboot.h | 86 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
index ee78f02ae94..0769f089e6b 100644
--- a/include/ubi_uboot.h
+++ b/include/ubi_uboot.h
@@ -47,14 +47,100 @@
 int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
 int ubi_init(void);
 void ubi_exit(void);
+
+/**
+ * ubi_detach() - detach UBI from MTD partition
+ *
+ * This function performs the cleanup of the UBI subsystem to make sure the
+ * MTD partition can be safely used by other purpose, or be attached again
+ * with ubi_part().
+ *
+ * Any mounted UBIFS will be unmounted automatically.
+ *
+ * Return: 0 on success.
+ */
 int ubi_detach(void);
+
+/**
+ * ubi_part() - attach UBI to MTD partition
+ * @part_name: name of the MTD partition to attach
+ * @vid_header_offset: VID header offset (string)
+ *
+ * This function detaches any existing UBI device, then probes for the
+ * specified MTD partition, and then scans it to initialize UBI.
+ *
+ * @vid_header_offset is optional and is usually set to NULL.
+ *
+ * Return: 0 on success, 1 if partition not found, or -ve on error.
+ */
 int ubi_part(const char *part_name, const char *vid_header_offset);
+
+/**
+ * ubi_volume_write() - write data to UBI volume
+ * @volume: name of the volume to write to
+ * @buf: data buffer to be written
+ * @offset: start offset for writing
+ * @size: number of bytes to write
+ *
+ * This function writes data to the specific UBI volume. If the offset is zero,
+ * it initiates a full volume update. Otherwise, it performs an offset-based
+ * write using LEB changes.
+ *
+ * Return: 0 on success, or -ve on error.
+ */
 int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
 		     size_t size);
+
+/**
+ * ubi_volume_read() - read data from UBI volume
+ * @volume: name of the volume to read from
+ * @buf: buffer to hold the read data
+ * @offset: start offset for reading
+ * @size: number of bytes to read
+ *
+ * This function reads data from the specified UBI volume. If @size is zero,
+ * the function reads the entire volume content starting from @offset.
+ *
+ * Return: 0 on success, or -ve on error.
+ */
 int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);
+
+/**
+ * ubi_create_vol() - create UBI volume
+ * @volume: name of the volume to create
+ * @size: size of the volume in bytes
+ * @dynamic: create dynamic volume if set to true
+ * @vol_id: volume ID
+ * @skipcheck: skip CRC check on this volume if set to true
+ *
+ * This function creates a new UBI volume with the specified parameters.
+ * If @size is negative, all available spaces will be used.
+ * if @vol_id is negative, volume ID will be selected and assigned by UBI.
+ *
+ * Return: 0 on success, or -ve on error.
+ */
 int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,
 		   bool skipcheck);
+
+/**
+ * ubi_find_volume() - find UBI volume by name
+ * @volume: name of the volume to find
+ *
+ * This function searches for a UBI volume with the specified name in the
+ * current UBI device.
+ *
+ * Return: pointer to the UBI volume structure, or %NULL if not found.
+ */
 struct ubi_volume *ubi_find_volume(const char *volume);
+
+/**
+ * ubi_remove_vol() - remove UBI volume
+ * @volume: name of the volume to remove
+ *
+ * This function removes an existing UBI volume from the current UBI device.
+ *
+ * Return: 0 on success, or a positive error code on failure.
+ */
 int ubi_remove_vol(const char *volume);
 
 extern struct ubi_device *ubi_devices[];
-- 
2.45.2

[PATCH v3 10/10] env: ubi: add support to create environment volume if it does not exist

[Weijie Gao]

Add an option to allow environment volume being auto created if not
exist.

Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
v2: updated kconfig help text
    added error output when failed to create volume
v3: fix error handling and default env setting
---
 env/Kconfig | 11 +++++++++++
 env/ubi.c   | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/env/Kconfig b/env/Kconfig
index 7abd82ab6f3..8404e0d44eb 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -716,6 +716,17 @@ config ENV_UBI_VOLUME_REDUND
 	help
 	  Name of the redundant volume that you want to store the environment in.
 
+config ENV_UBI_VOLUME_CREATE
+	bool "Create UBI volume if not exist"
+	depends on ENV_IS_IN_UBI
+	help
+	  This option is useful if u-boot will be booted from a fresh device
+	  where environment volume hasn't been created in the UBI partition.
+	  This is a common case where factory UBI image contains only volumes
+	  with valid data.
+	  By enabling this option, environment volume(s) will be created before
+	  loading if not exist.
+
 config ENV_UBI_VID_OFFSET
 	int "ubi environment VID offset"
 	depends on ENV_IS_IN_UBI
diff --git a/env/ubi.c b/env/ubi.c
index 46970ba754f..6b995ce40ac 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -103,12 +103,29 @@ static int env_ubi_save(void)
 }
 #endif /* CONFIG_ENV_REDUNDANT */
 
+static int env_ubi_volume_create(const char *volume)
+{
+	struct ubi_volume *vol;
+	int ret;
+
+	vol = ubi_find_volume(volume);
+	if (vol)
+		return 0;
+
+	ret = ubi_create_vol(volume, CONFIG_ENV_SIZE, true, UBI_VOL_NUM_AUTO,
+			     false);
+	if (ret)
+		printf("Failed to create environment volume '%s'\n", volume);
+
+	return ret;
+}
+
 #ifdef CONFIG_ENV_REDUNDANT
 static int env_ubi_load(void)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(char, env1_buf, CONFIG_ENV_SIZE);
 	ALLOC_CACHE_ALIGN_BUFFER(char, env2_buf, CONFIG_ENV_SIZE);
-	int read1_fail, read2_fail;
+	int read1_fail, read2_fail, create1_fail, create2_fail;
 	env_t *tmp_env1, *tmp_env2;
 
 	/*
@@ -132,6 +149,15 @@ static int env_ubi_load(void)
 		return -EIO;
 	}
 
+	if (IS_ENABLED(CONFIG_ENV_UBI_VOLUME_CREATE)) {
+		create1_fail = env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME);
+		create2_fail = env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME_REDUND);
+		if (create1_fail && create2_fail) {
+			env_set_default(NULL, 0);
+			return -ENODEV;
+		}
+	}
+
 	read1_fail = ubi_volume_read(CONFIG_ENV_UBI_VOLUME, tmp_env1, 0,
 				     CONFIG_ENV_SIZE);
 	if (read1_fail)
@@ -169,6 +195,13 @@ static int env_ubi_load(void)
 		return -EIO;
 	}
 
+	if (IS_ENABLED(CONFIG_ENV_UBI_VOLUME_CREATE)) {
+		if (env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME)) {
+			env_set_default(NULL, 0);
+			return -ENODEV;
+		}
+	}
+
 	if (ubi_volume_read(CONFIG_ENV_UBI_VOLUME, buf, 0, CONFIG_ENV_SIZE)) {
 		printf("\n** Unable to read env from %s:%s **\n",
 		       CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME);
-- 
2.45.2

Re: [PATCH v3 05/10] cmd: ubi: change the type of parameter dynamic to bool

[Simon Glass]

On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> cmd: ubi: change the type of parameter dynamic to bool
>
> This patch changes the type of the 'dynamic' parameter of ubi_create_vol()
> to bool as it's used as a boolean.
>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
>
> cmd/ubi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 09/10] ubi: add comments for exported ubi API functions

[Simon Glass]

Hi Weijie,

On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> ubi: add comments for exported ubi API functions
>
> This patch adds comments for exported ubi command API functions.
>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
>
> include/ubi_uboot.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> @@ -47,14 +47,100 @@
> + * ubi_part() - attach UBI to MTD partition
> + * @part_name: name of the MTD partition to attach
> + * @vid_header_offset: VID header offset (string)
> + *
> + * This function detaches any existing UBI device, then probes for the
> + * specified MTD partition, and then scans it to initialize UBI.
> + *
> + * @vid_header_offset is optional and is usually set to NULL.
> + *
> + * Return: 0 on success, 1 if partition not found, or -ve on error.
> + */
> int ubi_part(const char *part_name, const char *vid_header_offset);

Most error paths in this file return positive errno values, so '-ve on
error' is not quite right. Please double-check and document the actual
convention.

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> @@ -47,14 +47,100 @@
> + * ubi_volume_read() - read data from UBI volume
> + * @volume: name of the volume to read from
> + * @buf: buffer to hold the read data
> + * @offset: start offset for reading
> + * @size: number of bytes to read
> + *
> + * This function reads data from the specified UBI volume. If @size is zero,
> + * the function reads the entire volume content starting from @offset.
> + *
> + * Return: 0 on success, or -ve on error.
> + */
> int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);

Same here early-exit paths return positive errno (ENODEV, EBUSY,
EBADF, ENOMEM) while the read loop negates ubi_eba_read_leb()'s error.
Better to normalise to one convention, and please be consistent across
all of these.

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> @@ -47,14 +47,100 @@
> + * This function creates a new UBI volume with the specified parameters.
> + * If @size is negative, all available spaces will be used.
> + * if @vol_id is negative, volume ID will be selected and assigned by UBI.
> + *
> + * Return: 0 on success, or -ve on error.
> + */
> int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,

>                  bool skipcheck);

Capitlal I on 'If'. Also 'all available spaces' reads better as 'all
available space'. Also worth mentioning that @vol_id should be
UBI_VOL_NUM_AUTO for auto-assignment.

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> @@ -47,14 +47,100 @@
> + * ubi_detach() - detach UBI from MTD partition
> + *
> + * This function performs the cleanup of the UBI subsystem to make sure the
> + * MTD partition can be safely used by other purpose, or be attached again
> + * with ubi_part().
> + *
> + * Any mounted UBIFS will be unmounted automatically.
> + *
> + * Return: 0 on success.
> + */
> int ubi_detach(void);

'used by other purpose' should be 'used for another purpose'. Since
this only ever returns 0, you could say 'Return: 0' and drop 'on
success'.

Regards,
Simon

Re: [PATCH v3 06/10] cmd: ubi: reorganize command messages

[Simon Glass]

Hi Weijie,

On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> cmd: ubi: reorganize command messages
>
> This patch moves normal subcommand messages into the main command function.
> This will allow current and potential api functions being called with clean
> output.
>
> A new function ubi_require_volume() is added for finding and printing error
> message if volume not found. The original ubi_find_volume() will be silent
> for being an api function.
>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
>
> cmd/ubi.c | 109 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 79 insertions(+), 30 deletions(-)

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>       if (strncmp(argv[1], 'remove', 6) == 0) {
>               /* E.g., remove volume */
> -             if (argc == 3)
> -                     return ubi_remove_vol(argv[2]);
> +             if (argc == 3) {
> +                     int vol_id;
> +
> +                     vol = ubi_find_volume(argv[2]);
> +                     if (!vol)
> +                             return 0;
> +
> +                     vol_id = vol->vol_id;
> +
> +                     ret = ubi_remove_vol(argv[2]);

This seems wrong - previously 'ubi remove nonexistent' printed an
error via ubi_find_volume() and returned ENODEV; now it silently
returns 0. I think you should use ubi_require_volume() here.

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>               if (argc == 3) {
> +                     if (!size) {
> +                             vol = ubi_require_volume(argv[3]);
> +                             if (!vol)
> +                                     return 1;
> +
> +                             printf("No size specified -> Using max size (%lld)\n",
> +                                    vol->used_bytes);
> +                             size = vol->used_bytes;
> +                     }
> +
> +                     ret = ubi_volume_read(argv[3], (void *)addr, 0, size);

When size is 0 the volume is now looked up twice,. i.e. here and again
inside ubi_volume_read() - can you drop the fallback in
ubi_volume_read() or move the 'No size specified' print back there
guarded by a verbose flag.

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> -     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1], 'rename', 6))
> -             return ubi_rename_vol(argv[2], argv[3]);
> +     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1], 'rename', 6)) {
> +             ret = ubi_rename_vol(argv[2], argv[3]);
> +             if (!ret) {
> +                     printf("UBI volume %s renamed to %s\n", argv[2],
> +                            argv[3]);
> +             }
> +
> +             return ret;
> +     }

While you are here, please add an 'argc == 4' check to fix a
pre-existing bug - i.e. argv[2]/argv[3] are dereferenced
unconditionally.

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +                     else if (ret == -EEXIST)
> +                             printf("Volume %s already exists!\n", argv[2]);
> +
> +                     return ret;

Just to check: ubi_create_volume() returns -EEXIST, but
verify_mkvol_req() returns positive errnos, so name-too-long or
bad-alignment failures get no friendly message here. What do you think
about handling the verify_mkvol_req() errors here too, or making that
helper silent like ubi_find_volume()?

Regards,
Simon

Re: [PATCH v3 10/10] env: ubi: add support to create environment volume if it does not exist

[Simon Glass]

Hi Weijie,

On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> env: ubi: add support to create environment volume if it does not exist
>
> Add an option to allow environment volume being auto created if not
> exist.
>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
>
> env/Kconfig | 11 +++++++++++
>  env/ubi.c   | 35 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 1 deletion(-)

> diff --git a/env/Kconfig b/env/Kconfig
> @@ -699,6 +699,17 @@ config ENV_UBI_VOLUME_REDUND
> +config ENV_UBI_VOLUME_CREATE
> +     bool "Create UBI volume if not exist"
> +     depends on ENV_IS_IN_UBI
> +     help
> +       This option is useful if u-boot will be booted from a fresh device
> +       where environment volume hasn't been created in the UBI partition.
> +       This is a common case where factory UBI image contains only volumes
> +       with valid data.
> +       By enabling this option, environment volume(s) will be created before
> +       loading if not exist.

Please use 'U-Boot' rather than 'u-boot' and tidy the grammar (e.g.
'if it does not exist', 'where the environment volume has not been
created', 'any missing environment volumes will be created before
loading'). Also mention that with CONFIG_ENV_REDUNDANT both volumes
are created.

> diff --git a/env/ubi.c b/env/ubi.c
> @@ -134,6 +151,15 @@ static int env_ubi_load(void)
> +     if (IS_ENABLED(CONFIG_ENV_UBI_VOLUME_CREATE)) {
> +             create1_fail = env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME);
> +             create2_fail = env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME_REDUND);
> +             if (create1_fail && create2_fail) {
> +                     env_set_default(NULL, 0);
> +                     return -ENODEV;
> +             }
> +     }

Just to clarify the intent: if only one creation fails, this falls
through and the subsequent ubi_volume_read() prints 'Unable to read
redundant env from ...' which is confusing on a fresh device. Clearer
to fail (or print a dedicated message) as soon as either creation
fails - what do you think?

> diff --git a/env/ubi.c b/env/ubi.c
> @@ -105,12 +105,29 @@ static int env_ubi_save(void)
> +     ret = ubi_create_vol(volume, CONFIG_ENV_SIZE, true, UBI_VOL_NUM_AUTO,
> +                          false);

The dynamic flag is hard-coded to true. Sensible default, but please
mention the choice in the commit message or Kconfig help.

The commit message is quite terse - normally we like to see a
motivation (fresh device with a factory UBI image lacking the env
volume) - also note that for CONFIG_ENV_REDUNDANT both volumes are
created.

Regards,
Simon

Re: [PATCH v3 07/10] cmd: ubi: export more APIs to public

[Simon Glass]

Hi Weijie,

On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> cmd: ubi: export more APIs to public
>
> Export the following functions to public:
>
> - ubi_detach(): this is paired with ubi_part(). One may call this function
>   to completely clean up the ubi subsystem after using ubi_part().
>
> - ubi_{create,find,remove}_vol: this is a set of functions for volume
>   management.
>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
>
> cmd/ubi.c           | 10 +++++-----
>  include/ubi_uboot.h |  5 +++++
>  2 files changed, 10 insertions(+), 5 deletions(-)

> diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> @@ -47,10 +47,15 @@
>  int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
>  int ubi_init(void);
>  void ubi_exit(void);
> +int ubi_detach(void);
>  int ubi_part(const char *part_name, const char *vid_header_offset);
>  int ubi_volume_write(const char *volume, const void *buf, loff_t offset,
>                    size_t size);
>  int ubi_volume_read(const char *volume, void *buf, loff_t offset, size_t size);
> +int ubi_create_vol(const char *volume, int64_t size, bool dynamic, int vol_id,
> +                bool skipcheck);
> +struct ubi_volume *ubi_find_volume(const char *volume);
> +int ubi_remove_vol(const char *volume);

Please squash patch 9 (the kernel-doc comments) into this one.
Exporting an API and documenting it belong in the same change -
otherwise reviewers cannot judge the function contract from this patch
alone.

> diff --git a/cmd/ubi.c b/cmd/ubi.c
> @@ -270,7 +270,7 @@ static struct ubi_volume *ubi_require_volume(const char *volume)
>       return vol;
>  }
>
> -static int ubi_remove_vol(const char *volume)
> +int ubi_remove_vol(const char *volume)
>  {
>       int err, reserved_pebs, i;
>       struct ubi_volume *vol;

Hmm, now that this is public, the mixed return convention is a bit
awkward: ubi_remove_vol() returns positive ENODEV/EROFS in some paths
and negative values from ubi_change_vtbl_record()/ubi_eba_unmap_leb()
in others. ubi_create_vol() has the same issue. Worth a follow-up so
external callers get a consistent convention - ideally negative errno

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 07/10] cmd: ubi: export more APIs to public

[Weijie Gao]

On Tue, 2026-04-28 at 08:11 -0600, Simon Glass wrote:
> Hi Weijie,
> 
> On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> > cmd: ubi: export more APIs to public
> > 
> > Export the following functions to public:
> > 
> > - ubi_detach(): this is paired with ubi_part(). One may call this
> > function
> >   to completely clean up the ubi subsystem after using ubi_part().
> > 
> > - ubi_{create,find,remove}_vol: this is a set of functions for
> > volume
> >   management.
> > 
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > 
> > cmd/ubi.c           | 10 +++++-----
> >  include/ubi_uboot.h |  5 +++++
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > @@ -47,10 +47,15 @@
> >  int ubi_mtd_param_parse(const char *val, struct kernel_param *kp);
> >  int ubi_init(void);
> >  void ubi_exit(void);
> > +int ubi_detach(void);
> >  int ubi_part(const char *part_name, const char
> > *vid_header_offset);
> >  int ubi_volume_write(const char *volume, const void *buf, loff_t
> > offset,
> >                    size_t size);
> >  int ubi_volume_read(const char *volume, void *buf, loff_t offset,
> > size_t size);
> > +int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
> > int vol_id,
> > +                bool skipcheck);
> > +struct ubi_volume *ubi_find_volume(const char *volume);
> > +int ubi_remove_vol(const char *volume);
> 
> Please squash patch 9 (the kernel-doc comments) into this one.
> Exporting an API and documenting it belong in the same change -
> otherwise reviewers cannot judge the function contract from this
> patch
> alone.
> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -270,7 +270,7 @@ static struct ubi_volume
> > *ubi_require_volume(const char *volume)
> >       return vol;
> >  }
> > 
> > -static int ubi_remove_vol(const char *volume)
> > +int ubi_remove_vol(const char *volume)
> >  {
> >       int err, reserved_pebs, i;
> >       struct ubi_volume *vol;
> 
> Hmm, now that this is public, the mixed return convention is a bit
> awkward: ubi_remove_vol() returns positive ENODEV/EROFS in some paths
> and negative values from ubi_change_vtbl_record()/ubi_eba_unmap_leb()
> in others. ubi_create_vol() has the same issue. Worth a follow-up so
> external callers get a consistent convention - ideally negative errno

Using uniformed negative return value is apparently better.
Patch 9 will also benefit from this: we can safely say returning -ve on
error.

The ubi layer does return negative value in all paths already. I plan
to change all return value in ubi.c to negative in all functions and
return 1 (CMD_RET_FAILURE) for the command handler. 

> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v3 09/10] ubi: add comments for exported ubi API functions

[Weijie Gao]

On Tue, 2026-04-28 at 08:08 -0600, Simon Glass wrote:
> Hi Weijie,
> 
> On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> > ubi: add comments for exported ubi API functions
> > 
> > This patch adds comments for exported ubi command API functions.
> > 
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > 
> > include/ubi_uboot.h | 86
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > @@ -47,14 +47,100 @@
> > + * ubi_part() - attach UBI to MTD partition
> > + * @part_name: name of the MTD partition to attach
> > + * @vid_header_offset: VID header offset (string)
> > + *
> > + * This function detaches any existing UBI device, then probes for
> > the
> > + * specified MTD partition, and then scans it to initialize UBI.
> > + *
> > + * @vid_header_offset is optional and is usually set to NULL.
> > + *
> > + * Return: 0 on success, 1 if partition not found, or -ve on
> > error.
> > + */
> > int ubi_part(const char *part_name, const char *vid_header_offset);
> 
> Most error paths in this file return positive errno values, so '-ve
> on
> error' is not quite right. Please double-check and document the
> actual
> convention.
> 
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > @@ -47,14 +47,100 @@
> > + * ubi_volume_read() - read data from UBI volume
> > + * @volume: name of the volume to read from
> > + * @buf: buffer to hold the read data
> > + * @offset: start offset for reading
> > + * @size: number of bytes to read
> > + *
> > + * This function reads data from the specified UBI volume. If
> > @size is zero,
> > + * the function reads the entire volume content starting from
> > @offset.
> > + *
> > + * Return: 0 on success, or -ve on error.
> > + */
> > int ubi_volume_read(const char *volume, void *buf, loff_t offset,
> > size_t size);
> 
> Same here early-exit paths return positive errno (ENODEV, EBUSY,
> EBADF, ENOMEM) while the read loop negates ubi_eba_read_leb()'s
> error.
> Better to normalise to one convention, and please be consistent
> across
> all of these.

For the above two comments for returning -ve, I'll send another patch
to change all return value to negative.

> 
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > @@ -47,14 +47,100 @@
> > + * This function creates a new UBI volume with the specified
> > parameters.
> > + * If @size is negative, all available spaces will be used.
> > + * if @vol_id is negative, volume ID will be selected and assigned
> > by UBI.
> > + *
> > + * Return: 0 on success, or -ve on error.
> > + */
> > int ubi_create_vol(const char *volume, int64_t size, bool dynamic,
> > int vol_id,
> >                  bool skipcheck);
> 
> Capitlal I on 'If'. Also 'all available spaces' reads better as 'all
> available space'. Also worth mentioning that @vol_id should be
> UBI_VOL_NUM_AUTO for auto-assignment.
> 
> > diff --git a/include/ubi_uboot.h b/include/ubi_uboot.h
> > @@ -47,14 +47,100 @@
> > + * ubi_detach() - detach UBI from MTD partition
> > + *
> > + * This function performs the cleanup of the UBI subsystem to make
> > sure the
> > + * MTD partition can be safely used by other purpose, or be
> > attached again
> > + * with ubi_part().
> > + *
> > + * Any mounted UBIFS will be unmounted automatically.
> > + *
> > + * Return: 0 on success.
> > + */
> > int ubi_detach(void);
> 
> 'used by other purpose' should be 'used for another purpose'. Since
> this only ever returns 0, you could say 'Return: 0' and drop 'on
> success'.

I'll fix the rest two comments.

> 
> Regards,
> Simon

Re: [PATCH v3 06/10] cmd: ubi: reorganize command messages

[Weijie Gao]

On Tue, 2026-04-28 at 08:10 -0600, Simon Glass wrote:
> Hi Weijie,
> 
> On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> > cmd: ubi: reorganize command messages
> > 
> > This patch moves normal subcommand messages into the main command
> > function.
> > This will allow current and potential api functions being called
> > with clean
> > output.
> > 
> > A new function ubi_require_volume() is added for finding and
> > printing error
> > message if volume not found. The original ubi_find_volume() will be
> > silent
> > for being an api function.
> > 
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > 
> > cmd/ubi.c | 109 +++++++++++++++++++++++++++++++++++++++++++++----
> > -------------
> >  1 file changed, 79 insertions(+), 30 deletions(-)
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> >       if (strncmp(argv[1], 'remove', 6) == 0) {
> >               /* E.g., remove volume */
> > -             if (argc == 3)
> > -                     return ubi_remove_vol(argv[2]);
> > +             if (argc == 3) {
> > +                     int vol_id;
> > +
> > +                     vol = ubi_find_volume(argv[2]);
> > +                     if (!vol)
> > +                             return 0;
> > +
> > +                     vol_id = vol->vol_id;
> > +
> > +                     ret = ubi_remove_vol(argv[2]);
> 
> This seems wrong - previously 'ubi remove nonexistent' printed an
> error via ubi_find_volume() and returned ENODEV; now it silently
> returns 0. I think you should use ubi_require_volume() here.

I'll fix this.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> >               if (argc == 3) {
> > +                     if (!size) {
> > +                             vol = ubi_require_volume(argv[3]);
> > +                             if (!vol)
> > +                                     return 1;
> > +
> > +                             printf("No size specified -> Using
> > max size (%lld)\n",
> > +                                    vol->used_bytes);
> > +                             size = vol->used_bytes;
> > +                     }
> > +
> > +                     ret = ubi_volume_read(argv[3], (void *)addr,
> > 0, size);
> 
> When size is 0 the volume is now looked up twice,. i.e. here and
> again
> inside ubi_volume_read() - can you drop the fallback in
> ubi_volume_read() or move the 'No size specified' print back there
> guarded by a verbose flag.

I think we can do some modifications to ubi_volume_read:

static int __ubi_volume_read(struct ubi_volume *vol, ...) { ... }

int ubi_volume_read(const char *volume, ...)
{
	struct ubi_volume *vol = ubi_require_volume(volume);
	...
	return __ubi_volume_read(vol, ...);
}

And in do_ubi() we always call ubi_require_volume() to get vol and call
__ubi_volume_read with vol.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > -     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1],
> > 'rename', 6))
> > -             return ubi_rename_vol(argv[2], argv[3]);
> > +     if (IS_ENABLED(CONFIG_CMD_UBI_RENAME) && !strncmp(argv[1],
> > 'rename', 6)) {
> > +             ret = ubi_rename_vol(argv[2], argv[3]);
> > +             if (!ret) {
> > +                     printf("UBI volume %s renamed to %s\n",
> > argv[2],
> > +                            argv[3]);
> > +             }
> > +
> > +             return ret;
> > +     }
> 
> While you are here, please add an 'argc == 4' check to fix a
> pre-existing bug - i.e. argv[2]/argv[3] are dereferenced
> unconditionally.

OK.

> 
> > diff --git a/cmd/ubi.c b/cmd/ubi.c
> > @@ -806,31 +805,65 @@ static int do_ubi(struct cmd_tbl *cmdtp, int
> > flag, int argc, char *const argv[])
> > +                     else if (ret == -EEXIST)
> > +                             printf("Volume %s already exists!\n",
> > argv[2]);
> > +
> > +                     return ret;
> 
> Just to check: ubi_create_volume() returns -EEXIST, but
> verify_mkvol_req() returns positive errnos, so name-too-long or
> bad-alignment failures get no friendly message here. What do you
> think
> about handling the verify_mkvol_req() errors here too, or making that
> helper silent like ubi_find_volume()?

I found that the ubi layer will print error message in this case:
ubi0 error: ubi_create_volume: volume "test" exists (ID 2)
ubi0 error: ubi_create_volume: cannot create volume 11, error -17

So this modification is unnecessary and I'll remove this in next patch.

All error messages will be kept in verify_mkvol_req().

> 
> Regards,
> Simon

Re: [PATCH v3 10/10] env: ubi: add support to create environment volume if it does not exist

[Weijie Gao]

On Tue, 2026-04-28 at 08:11 -0600, Simon Glass wrote:
> Hi Weijie,
> 
> On 2026-04-27T07:09:11, Weijie Gao <weijie.gao@mediatek.com> wrote:
> > env: ubi: add support to create environment volume if it does not
> > exist
> > 
> > Add an option to allow environment volume being auto created if not
> > exist.
> > 
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > 
> > env/Kconfig | 11 +++++++++++
> >  env/ubi.c   | 35 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 45 insertions(+), 1 deletion(-)
> > diff --git a/env/Kconfig b/env/Kconfig
> > @@ -699,6 +699,17 @@ config ENV_UBI_VOLUME_REDUND
> > +config ENV_UBI_VOLUME_CREATE
> > +     bool "Create UBI volume if not exist"
> > +     depends on ENV_IS_IN_UBI
> > +     help
> > +       This option is useful if u-boot will be booted from a fresh
> > device
> > +       where environment volume hasn't been created in the UBI
> > partition.
> > +       This is a common case where factory UBI image contains only
> > volumes
> > +       with valid data.
> > +       By enabling this option, environment volume(s) will be
> > created before
> > +       loading if not exist.
> 
> Please use 'U-Boot' rather than 'u-boot' and tidy the grammar (e.g.
> 'if it does not exist', 'where the environment volume has not been
> created', 'any missing environment volumes will be created before
> loading'). Also mention that with CONFIG_ENV_REDUNDANT both volumes
> are created.

OK.

> 
> > diff --git a/env/ubi.c b/env/ubi.c
> > @@ -134,6 +151,15 @@ static int env_ubi_load(void)
> > +     if (IS_ENABLED(CONFIG_ENV_UBI_VOLUME_CREATE)) {
> > +             create1_fail =
> > env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME);
> > +             create2_fail =
> > env_ubi_volume_create(CONFIG_ENV_UBI_VOLUME_REDUND);
> > +             if (create1_fail && create2_fail) {
> > +                     env_set_default(NULL, 0);
> > +                     return -ENODEV;
> > +             }
> > +     }
> 
> Just to clarify the intent: if only one creation fails, this falls
> through and the subsequent ubi_volume_read() prints 'Unable to read
> redundant env from ...' which is confusing on a fresh device. Clearer
> to fail (or print a dedicated message) as soon as either creation
> fails - what do you think?

There's one case:
One volume exists and contains valid env data, and another one is
missing.
If we still failed to create the missing one (perhaps due to no space
left in UBI), we can continue to read the valid volume.

So I think it's better to add a check before reading:
if (!create1_fail) {
	read1_fail = ubi_volume_read(...);
} else {
	read1_fail = create1_fail;
}

> 
> > diff --git a/env/ubi.c b/env/ubi.c
> > @@ -105,12 +105,29 @@ static int env_ubi_save(void)
> > +     ret = ubi_create_vol(volume, CONFIG_ENV_SIZE, true,
> > UBI_VOL_NUM_AUTO,
> > +                          false);
> 
> The dynamic flag is hard-coded to true. Sensible default, but please
> mention the choice in the commit message or Kconfig help.

Ah. I remind that this is the reason I added __weak in the first patch.
One can override this function to create static volume.
I'll add another Kconfig to allow user to choose the volume type.

> 
> The commit message is quite terse - normally we like to see a
> motivation (fresh device with a factory UBI image lacking the env
> volume) - also note that for CONFIG_ENV_REDUNDANT both volumes are
> created.

OK. I'll modify it.

> 
> Regards,
> Simon