[PATCH 0/7] Detect partition changes at runtime

[PATCH 0/7] Detect partition changes at runtime

[Sascha Hauer]

With this series barebox can detect partition table changes at runtime
and reparse the partition table in that case. With this it's possible
for example to write an image to a SD card and mount partitions directly
afterwards without restarting barebox.

The fastboot code is also changed to check for existing partitions when
they are actually used and no longer during initialization time of the
gadget. With this it becomes possible to expose a full device via
fastboot and also some not yet existing partitions. When the full device
is written to one can then write to a (newly created) partition directly
afterwards.

The reparsing of the partition table only works when none of the
partitions are mounted or otherwise opened. When a partition is open
when the partition table changes, then barebox will continue with
the old partition table just like Linux does.

Sascha

Sascha Hauer (7):
  fs: move cdev open count to cdev_open()/cdev_close()
  common: partitions: efi: fix memory leak
  partition: allow to reparse a partition table
  block: reparse partition table when necessary
  fastboot: pass list to fb_addvar()
  fastboot: add function to free a list of fastboot variables
  fastboot: evaluate fastboot partitions when used

 common/block.c          | 30 +++++++++++++++++++--
 common/fastboot.c       | 59 ++++++++++++++++++++++++++---------------
 common/partitions.c     | 20 ++++++++++++++
 common/partitions/efi.c | 10 +++----
 fs/devfs-core.c         | 13 +++++++--
 fs/devfs.c              |  4 ---
 include/block.h         |  2 ++
 include/disks.h         |  1 +
 8 files changed, 104 insertions(+), 35 deletions(-)

-- 
2.39.2

[PATCH 1/7] fs: move cdev open count to cdev_open()/cdev_close()

[Sascha Hauer]

All code opening a cdev goes through cdev_open(), so open counting
must be done there and not in devfs_open() which itself only calls
cdev_open().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/devfs-core.c | 13 +++++++++++--
 fs/devfs.c      |  4 ----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/devfs-core.c b/fs/devfs-core.c
index c7187e3c01..87d4591d99 100644
--- a/fs/devfs-core.c
+++ b/fs/devfs-core.c
@@ -175,8 +175,15 @@ int cdev_find_free_index(const char *basename)
 
 int cdev_open(struct cdev *cdev, unsigned long flags)
 {
-	if (cdev->ops->open)
-		return cdev->ops->open(cdev, flags);
+	int ret;
+
+	if (cdev->ops->open) {
+		ret = cdev->ops->open(cdev, flags);
+		if (ret)
+			return ret;
+	}
+
+	cdev->open++;
 
 	return 0;
 }
@@ -221,6 +228,8 @@ void cdev_close(struct cdev *cdev)
 {
 	if (cdev->ops->close)
 		cdev->ops->close(cdev);
+
+	cdev->open--;
 }
 
 ssize_t cdev_read(struct cdev *cdev, void *buf, size_t count, loff_t offset, ulong flags)
diff --git a/fs/devfs.c b/fs/devfs.c
index c30ae4f384..c8ddbbdab0 100644
--- a/fs/devfs.c
+++ b/fs/devfs.c
@@ -114,8 +114,6 @@ static int devfs_open(struct device *_dev, FILE *f, const char *filename)
 			return ret;
 	}
 
-	cdev->open++;
-
 	return 0;
 }
 
@@ -130,8 +128,6 @@ static int devfs_close(struct device *_dev, FILE *f)
 			return ret;
 	}
 
-	cdev->open--;
-
 	return 0;
 }
 
-- 
2.39.2

[PATCH 2/7] common: partitions: efi: fix memory leak

[Sascha Hauer]

In efi_partition() gpt and ptes is allocated but never freed. It's no
longer used when leaving this function, so free the memory before
leaving it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/partitions/efi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/partitions/efi.c b/common/partitions/efi.c
index 0add66e6e4..d0e14d5abb 100644
--- a/common/partitions/efi.c
+++ b/common/partitions/efi.c
@@ -440,11 +440,8 @@ static void efi_partition(void *buf, struct block_device *blk,
 	int nb_part;
 	struct partition *pentry;
 
-	if (!find_valid_gpt(buf, blk, &gpt, &ptes) || !gpt || !ptes) {
-		kfree(gpt);
-		kfree(ptes);
-		return;
-	}
+	if (!find_valid_gpt(buf, blk, &gpt, &ptes) || !gpt || !ptes)
+		goto out;
 
 	snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
 	dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
@@ -474,6 +471,9 @@ static void efi_partition(void *buf, struct block_device *blk,
 		pentry->typeuuid = ptes[i].partition_type_guid;
 		pd->used_entries++;
 	}
+out:
+	kfree(gpt);
+	kfree(ptes);
 }
 
 static struct partition_parser efi_partition_parser = {
-- 
2.39.2

[PATCH 3/7] partition: allow to reparse a partition table

[Sascha Hauer]

The partition table of a block device may change when it is written to.
Allow to reparse the partition table when this happens.
This is easy when none of the partitions is opened, in that case just
remove the old partitions and call parse_partition_table() again.
When a partition is opened (including mounted) we can't remove it. In
that case continue with the old partition table and print a warning.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/partitions.c | 20 ++++++++++++++++++++
 include/disks.h     |  1 +
 2 files changed, 21 insertions(+)

diff --git a/common/partitions.c b/common/partitions.c
index 78b68276c3..cfcd0e080b 100644
--- a/common/partitions.c
+++ b/common/partitions.c
@@ -154,6 +154,26 @@ int parse_partition_table(struct block_device *blk)
 	return rc;
 }
 
+int reparse_partition_table(struct block_device *blk)
+{
+	struct cdev *cdev = &blk->cdev;
+	struct cdev *c, *tmp;
+
+	list_for_each_entry(c, &cdev->partitions, partition_entry) {
+		if (c->open) {
+			pr_warn("%s is busy, will continue to use old partition table\n", c->name);
+			return -EBUSY;
+		}
+	}
+
+	list_for_each_entry_safe(c, tmp, &cdev->partitions, partition_entry) {
+		if (c->flags & DEVFS_PARTITION_FROM_TABLE)
+			cdevfs_del_partition(c);
+	}
+
+	return parse_partition_table(blk);
+}
+
 int partition_parser_register(struct partition_parser *p)
 {
 	list_add_tail(&p->list, &partition_parser_list);
diff --git a/include/disks.h b/include/disks.h
index 1ca7063c54..ccb50d3ce9 100644
--- a/include/disks.h
+++ b/include/disks.h
@@ -24,5 +24,6 @@ struct partition_entry {
 } __attribute__ ((packed));
 
 extern int parse_partition_table(struct block_device*);
+int reparse_partition_table(struct block_device *blk);
 
 #endif /* DISKS_H */
-- 
2.39.2

[PATCH 4/7] block: reparse partition table when necessary

[Sascha Hauer]

Call reparse_partition_table() when the partition table may have
changed. We detect this by recording if the block device has been
written to in the area where the partition table is.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/block.c  | 30 ++++++++++++++++++++++++++++--
 include/block.h |  2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/common/block.c b/common/block.c
index 3a4a9fb731..007c8bed12 100644
--- a/common/block.c
+++ b/common/block.c
@@ -284,6 +284,17 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
 	blkcnt_t blocks;
 	int ret;
 
+	/*
+	 * When the offset that is written to is within the first two
+	 * LBAs then the partition table has changed, reparse the partition
+	 * table at close time in this case. A GPT covers more space than
+	 * only the first two LBAs, but a CRC of the remaining pieces is
+	 * written to LBA1, so LBA1 must change as well when the partioning
+	 * is changed.
+	 */
+	if (offset < 2 * SECTOR_SIZE)
+		blk->need_reparse = true;
+
 	if (offset & mask) {
 		size_t now = BLOCKSIZE(blk) - (offset & mask);
 		void *iobuf = block_get(blk, block);
@@ -335,13 +346,28 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
 static int block_op_flush(struct cdev *cdev)
 {
 	struct block_device *blk = cdev->priv;
+	int ret;
 
 	blk->discard_start = blk->discard_size = 0;
 
-	return writebuffer_flush(blk);
+	ret = writebuffer_flush(blk);
+
+	return ret;
 }
 
-static int block_op_close(struct cdev *cdev) __alias(block_op_flush);
+static int block_op_close(struct cdev *cdev)
+{
+	struct block_device *blk = cdev->priv;
+
+	block_op_flush(cdev);
+
+	if (blk->need_reparse) {
+		reparse_partition_table(blk);
+		blk->need_reparse = false;
+	}
+
+	return 0;
+}
 
 static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset)
 {
diff --git a/include/block.h b/include/block.h
index 44037bd74c..aaaa97400f 100644
--- a/include/block.h
+++ b/include/block.h
@@ -33,6 +33,8 @@ struct block_device {
 	struct list_head idle_blocks;
 
 	struct cdev cdev;
+
+	bool need_reparse;
 };
 
 extern struct list_head block_device_list;
-- 
2.39.2

[PATCH 5/7] fastboot: pass list to fb_addvar()

[Sascha Hauer]

With followup patches we'll maintain two lists of fastboot variables. As
a preparation pass the list to which the variable shall be added to
fb_addvar(). No functional change intended.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/fastboot.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index 261283d50a..eaf99b08f4 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -66,7 +66,7 @@ static void fb_setvar(struct fb_variable *var, const char *fmt, ...)
 	va_end(ap);
 }
 
-static struct fb_variable *fb_addvar(struct fastboot *fb, const char *fmt, ...)
+static struct fb_variable *fb_addvar(struct fastboot *fb, struct list_head *list, const char *fmt, ...)
 {
 	struct fb_variable *var = xzalloc(sizeof(*var));
 	va_list ap;
@@ -75,7 +75,7 @@ static struct fb_variable *fb_addvar(struct fastboot *fb, const char *fmt, ...)
 	var->name = bvasprintf(fmt, ap);
 	va_end(ap);
 
-	list_add_tail(&var->list, &fb->variables);
+	list_add_tail(&var->list, list);
 
 	return var;
 }
@@ -152,9 +152,9 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
 	if (ret)
 		return ret;
 
-	var = fb_addvar(fb, "partition-size:%s", fentry->name);
+	var = fb_addvar(fb, &fb->variables, "partition-size:%s", fentry->name);
 	fb_setvar(var, "%08zx", size);
-	var = fb_addvar(fb, "partition-type:%s", fentry->name);
+	var = fb_addvar(fb, &fb->variables, "partition-type:%s", fentry->name);
 	fb_setvar(var, "%s", type);
 
 	return ret;
@@ -168,12 +168,12 @@ int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
 
 	INIT_LIST_HEAD(&fb->variables);
 
-	var = fb_addvar(fb, "version");
+	var = fb_addvar(fb, &fb->variables, "version");
 	fb_setvar(var, "0.4");
-	var = fb_addvar(fb, "bootloader-version");
+	var = fb_addvar(fb, &fb->variables, "bootloader-version");
 	fb_setvar(var, release_string);
 	if (IS_ENABLED(CONFIG_FASTBOOT_SPARSE)) {
-		var = fb_addvar(fb, "max-download-size");
+		var = fb_addvar(fb, &fb->variables, "max-download-size");
 		fb_setvar(var, "%u", fastboot_max_download_size);
 	}
 
-- 
2.39.2

[PATCH 6/7] fastboot: add function to free a list of fastboot variables

[Sascha Hauer]

With followup patches we'll maintain two lists of fastboot variables. As
a preparation create a function which frees fastboot variables on a
list.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/fastboot.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index eaf99b08f4..f41c02a576 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -195,16 +195,21 @@ int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
 	return 0;
 }
 
-void fastboot_generic_free(struct fastboot *fb)
+static void fastboot_free_variables(struct list_head *list)
 {
 	struct fb_variable *var, *tmp;
 
-	list_for_each_entry_safe(var, tmp, &fb->variables, list) {
+	list_for_each_entry_safe(var, tmp, list, list) {
 		free(var->name);
 		free(var->value);
 		list_del(&var->list);
 		free(var);
 	}
+}
+
+void fastboot_generic_free(struct fastboot *fb)
+{
+	fastboot_free_variables(&fb->variables);
 
 	free(fb->tempname);
 
-- 
2.39.2

[PATCH 7/7] fastboot: evaluate fastboot partitions when used

[Sascha Hauer]

So far we have evaluated the fastboot partition description during
initialization of the fastboot gadget. Let's make this more flexible
and parse it when actually needed.
This gives us the possibility to first write an image including a
partition table to a disk and afterwards write an image to a single
partition created by the first whole disk write.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/fastboot.c | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/common/fastboot.c b/common/fastboot.c
index f41c02a576..acf138af8a 100644
--- a/common/fastboot.c
+++ b/common/fastboot.c
@@ -80,7 +80,7 @@ static struct fb_variable *fb_addvar(struct fastboot *fb, struct list_head *list
 	return var;
 }
 
-static int fastboot_add_partition_variables(struct fastboot *fb,
+static int fastboot_add_partition_variables(struct fastboot *fb, struct list_head *list,
 		struct file_list_entry *fentry)
 {
 	struct stat s;
@@ -152,9 +152,9 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
 	if (ret)
 		return ret;
 
-	var = fb_addvar(fb, &fb->variables, "partition-size:%s", fentry->name);
+	var = fb_addvar(fb, list, "partition-size:%s", fentry->name);
 	fb_setvar(var, "%08zx", size);
-	var = fb_addvar(fb, &fb->variables, "partition-type:%s", fentry->name);
+	var = fb_addvar(fb, list, "partition-type:%s", fentry->name);
 	fb_setvar(var, "%s", type);
 
 	return ret;
@@ -162,8 +162,6 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
 
 int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
 {
-	int ret;
-	struct file_list_entry *fentry;
 	struct fb_variable *var;
 
 	INIT_LIST_HEAD(&fb->variables);
@@ -186,12 +184,6 @@ int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
 	if (export_bbu)
 		bbu_append_handlers_to_file_list(fb->files);
 
-	file_list_for_each_entry(fb->files, fentry) {
-		ret = fastboot_add_partition_variables(fb, fentry);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
@@ -305,26 +297,44 @@ static int strcmp_l1(const char *s1, const char *s2)
 static void cb_getvar(struct fastboot *fb, const char *cmd)
 {
 	struct fb_variable *var;
+	LIST_HEAD(partition_list);
+	struct file_list_entry *fentry;
+
+	file_list_for_each_entry(fb->files, fentry)
+		fastboot_add_partition_variables(fb, &partition_list, fentry);
 
 	pr_debug("getvar: \"%s\"\n", cmd);
 
 	if (!strcmp_l1(cmd, "all")) {
-		list_for_each_entry(var, &fb->variables, list) {
+		list_for_each_entry(var, &fb->variables, list)
 			fastboot_tx_print(fb, FASTBOOT_MSG_INFO, "%s: %s",
 					  var->name, var->value);
-		}
+
+		list_for_each_entry(var, &partition_list, list)
+			fastboot_tx_print(fb, FASTBOOT_MSG_INFO, "%s: %s",
+					  var->name, var->value);
+
 		fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, "");
-		return;
+		goto out;
 	}
 
 	list_for_each_entry(var, &fb->variables, list) {
 		if (!strcmp(cmd, var->name)) {
 			fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, var->value);
-			return;
+			goto out;
+		}
+	}
+
+	list_for_each_entry(var, &partition_list, list) {
+		if (!strcmp(cmd, var->name)) {
+			fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, var->value);
+			goto out;
 		}
 	}
 
 	fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, "");
+out:
+	fastboot_free_variables(&partition_list);
 }
 
 int fastboot_handle_download_data(struct fastboot *fb, const void *buffer,
-- 
2.39.2

Re: [PATCH 0/7] Detect partition changes at runtime

[Ahmad Fatoum]

Hello Sascha,

On 15.02.24 08:47, Sascha Hauer wrote:
> With this series barebox can detect partition table changes at runtime
> and reparse the partition table in that case. With this it's possible
> for example to write an image to a SD card and mount partitions directly
> afterwards without restarting barebox.

That's cool!

How much extra effort do you think would it be to support plug-and-play
of SD-Cards? CONFIG_MCI_POLLER or something would poll card detect and
if it disappears: deregister all partitions and if it appears and probing
is enabled for the device (or it was previously probed), reinit the card
and re-enable the partition table.

That would be a nice stress test for the new functionality :-)

Cheers,
Ahmad

> 
> The fastboot code is also changed to check for existing partitions when
> they are actually used and no longer during initialization time of the
> gadget. With this it becomes possible to expose a full device via
> fastboot and also some not yet existing partitions. When the full device
> is written to one can then write to a (newly created) partition directly
> afterwards.
> 
> The reparsing of the partition table only works when none of the
> partitions are mounted or otherwise opened. When a partition is open
> when the partition table changes, then barebox will continue with
> the old partition table just like Linux does.
> 
> Sascha
> 
> Sascha Hauer (7):
>   fs: move cdev open count to cdev_open()/cdev_close()
>   common: partitions: efi: fix memory leak
>   partition: allow to reparse a partition table
>   block: reparse partition table when necessary
>   fastboot: pass list to fb_addvar()
>   fastboot: add function to free a list of fastboot variables
>   fastboot: evaluate fastboot partitions when used
> 
>  common/block.c          | 30 +++++++++++++++++++--
>  common/fastboot.c       | 59 ++++++++++++++++++++++++++---------------
>  common/partitions.c     | 20 ++++++++++++++
>  common/partitions/efi.c | 10 +++----
>  fs/devfs-core.c         | 13 +++++++--
>  fs/devfs.c              |  4 ---
>  include/block.h         |  2 ++
>  include/disks.h         |  1 +
>  8 files changed, 104 insertions(+), 35 deletions(-)
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/7] partition: allow to reparse a partition table

[Marco Felsch]

On 24-02-15, Sascha Hauer wrote:
> The partition table of a block device may change when it is written to.
> Allow to reparse the partition table when this happens.
> This is easy when none of the partitions is opened, in that case just
> remove the old partitions and call parse_partition_table() again.
> When a partition is opened (including mounted) we can't remove it. In
> that case continue with the old partition table and print a warning.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Just two nits, to make cdev access more opaque.

> ---
>  common/partitions.c | 20 ++++++++++++++++++++
>  include/disks.h     |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/common/partitions.c b/common/partitions.c
> index 78b68276c3..cfcd0e080b 100644
> --- a/common/partitions.c
> +++ b/common/partitions.c
> @@ -154,6 +154,26 @@ int parse_partition_table(struct block_device *blk)
>  	return rc;
>  }
>  
> +int reparse_partition_table(struct block_device *blk)
> +{
> +	struct cdev *cdev = &blk->cdev;
> +	struct cdev *c, *tmp;
> +
> +	list_for_each_entry(c, &cdev->partitions, partition_entry) {
> +		if (c->open) {

		if (cdev_is_open(c))

> +			pr_warn("%s is busy, will continue to use old partition table\n", c->name);
> +			return -EBUSY;
> +		}
> +	}
> +
> +	list_for_each_entry_safe(c, tmp, &cdev->partitions, partition_entry) {
> +		if (c->flags & DEVFS_PARTITION_FROM_TABLE)

		if (cdev_flag_set(c, DEVFS_PARTITION_FROM_TABLE))

> +			cdevfs_del_partition(c);
> +	}
> +
> +	return parse_partition_table(blk);
> +}
> +
>  int partition_parser_register(struct partition_parser *p)
>  {
>  	list_add_tail(&p->list, &partition_parser_list);
> diff --git a/include/disks.h b/include/disks.h
> index 1ca7063c54..ccb50d3ce9 100644
> --- a/include/disks.h
> +++ b/include/disks.h
> @@ -24,5 +24,6 @@ struct partition_entry {
>  } __attribute__ ((packed));
>  
>  extern int parse_partition_table(struct block_device*);
> +int reparse_partition_table(struct block_device *blk);
>  
>  #endif /* DISKS_H */
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 7/7] fastboot: evaluate fastboot partitions when used

[Marco Felsch]

On 24-02-15, Sascha Hauer wrote:
> So far we have evaluated the fastboot partition description during
> initialization of the fastboot gadget. Let's make this more flexible
> and parse it when actually needed.
> This gives us the possibility to first write an image including a
> partition table to a disk and afterwards write an image to a single
> partition created by the first whole disk write.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/fastboot.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f41c02a576..acf138af8a 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -80,7 +80,7 @@ static struct fb_variable *fb_addvar(struct fastboot *fb, struct list_head *list
>  	return var;
>  }
>  
> -static int fastboot_add_partition_variables(struct fastboot *fb,
> +static int fastboot_add_partition_variables(struct fastboot *fb, struct list_head *list,
>  		struct file_list_entry *fentry)
>  {
>  	struct stat s;
> @@ -152,9 +152,9 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
>  	if (ret)
>  		return ret;
>  
> -	var = fb_addvar(fb, &fb->variables, "partition-size:%s", fentry->name);
> +	var = fb_addvar(fb, list, "partition-size:%s", fentry->name);
>  	fb_setvar(var, "%08zx", size);
> -	var = fb_addvar(fb, &fb->variables, "partition-type:%s", fentry->name);
> +	var = fb_addvar(fb, list, "partition-type:%s", fentry->name);
>  	fb_setvar(var, "%s", type);
>  
>  	return ret;
> @@ -162,8 +162,6 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
>  
>  int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
>  {
> -	int ret;
> -	struct file_list_entry *fentry;
>  	struct fb_variable *var;
>  
>  	INIT_LIST_HEAD(&fb->variables);
> @@ -186,12 +184,6 @@ int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
>  	if (export_bbu)
>  		bbu_append_handlers_to_file_list(fb->files);
>  
> -	file_list_for_each_entry(fb->files, fentry) {
> -		ret = fastboot_add_partition_variables(fb, fentry);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -305,26 +297,44 @@ static int strcmp_l1(const char *s1, const char *s2)
>  static void cb_getvar(struct fastboot *fb, const char *cmd)
>  {
>  	struct fb_variable *var;
> +	LIST_HEAD(partition_list);
> +	struct file_list_entry *fentry;
> +
> +	file_list_for_each_entry(fb->files, fentry)
> +		fastboot_add_partition_variables(fb, &partition_list, fentry);

Albeit we can ignore it, I would check the return code and print a
warning like:

	file_list_for_each_entry(fb->files, fentry) {
		int ret;

		ret = fastboot_add_partition_variables(fb, &partition_list, fentry);
		if (ret) {
			pr_warn("Failed to add partition variables");
			return;
		}
	}

Regards,
  Marco

>  
>  	pr_debug("getvar: \"%s\"\n", cmd);
>  
>  	if (!strcmp_l1(cmd, "all")) {
> -		list_for_each_entry(var, &fb->variables, list) {
> +		list_for_each_entry(var, &fb->variables, list)
>  			fastboot_tx_print(fb, FASTBOOT_MSG_INFO, "%s: %s",
>  					  var->name, var->value);
> -		}
> +
> +		list_for_each_entry(var, &partition_list, list)
> +			fastboot_tx_print(fb, FASTBOOT_MSG_INFO, "%s: %s",
> +					  var->name, var->value);
> +
>  		fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, "");
> -		return;
> +		goto out;
>  	}
>  
>  	list_for_each_entry(var, &fb->variables, list) {
>  		if (!strcmp(cmd, var->name)) {
>  			fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, var->value);
> -			return;
> +			goto out;
> +		}
> +	}
> +
> +	list_for_each_entry(var, &partition_list, list) {
> +		if (!strcmp(cmd, var->name)) {
> +			fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, var->value);
> +			goto out;
>  		}
>  	}
>  
>  	fastboot_tx_print(fb, FASTBOOT_MSG_OKAY, "");
> +out:
> +	fastboot_free_variables(&partition_list);
>  }
>  
>  int fastboot_handle_download_data(struct fastboot *fb, const void *buffer,
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 4/7] block: reparse partition table when necessary

[Marco Felsch]

On 24-02-15, Sascha Hauer wrote:
> Call reparse_partition_table() when the partition table may have
> changed. We detect this by recording if the block device has been
> written to in the area where the partition table is.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/block.c  | 30 ++++++++++++++++++++++++++++--
>  include/block.h |  2 ++
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/common/block.c b/common/block.c
> index 3a4a9fb731..007c8bed12 100644
> --- a/common/block.c
> +++ b/common/block.c
> @@ -284,6 +284,17 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
>  	blkcnt_t blocks;
>  	int ret;
>  
> +	/*
> +	 * When the offset that is written to is within the first two
> +	 * LBAs then the partition table has changed, reparse the partition
> +	 * table at close time in this case. A GPT covers more space than
> +	 * only the first two LBAs, but a CRC of the remaining pieces is
> +	 * written to LBA1, so LBA1 must change as well when the partioning
> +	 * is changed.
> +	 */
> +	if (offset < 2 * SECTOR_SIZE)
> +		blk->need_reparse = true;
> +
>  	if (offset & mask) {
>  		size_t now = BLOCKSIZE(blk) - (offset & mask);
>  		void *iobuf = block_get(blk, block);
> @@ -335,13 +346,28 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
>  static int block_op_flush(struct cdev *cdev)
>  {
>  	struct block_device *blk = cdev->priv;
> +	int ret;
>  
>  	blk->discard_start = blk->discard_size = 0;
>  
> -	return writebuffer_flush(blk);
> +	ret = writebuffer_flush(blk);
> +
> +	return ret;

This change is not required or do I miss something?

Regards,
  Marco

>  }
>  
> -static int block_op_close(struct cdev *cdev) __alias(block_op_flush);
> +static int block_op_close(struct cdev *cdev)
> +{
> +	struct block_device *blk = cdev->priv;
> +
> +	block_op_flush(cdev);
> +
> +	if (blk->need_reparse) {
> +		reparse_partition_table(blk);
> +		blk->need_reparse = false;
> +	}
> +
> +	return 0;
> +}
>  
>  static int block_op_discard_range(struct cdev *cdev, loff_t count, loff_t offset)
>  {
> diff --git a/include/block.h b/include/block.h
> index 44037bd74c..aaaa97400f 100644
> --- a/include/block.h
> +++ b/include/block.h
> @@ -33,6 +33,8 @@ struct block_device {
>  	struct list_head idle_blocks;
>  
>  	struct cdev cdev;
> +
> +	bool need_reparse;
>  };
>  
>  extern struct list_head block_device_list;
> -- 
> 2.39.2
> 
> 
> 

Re: [PATCH 0/7] Detect partition changes at runtime

[Marco Felsch]

Hi Sascha,

On 24-02-15, Sascha Hauer wrote:
> With this series barebox can detect partition table changes at runtime
> and reparse the partition table in that case. With this it's possible
> for example to write an image to a SD card and mount partitions directly
> afterwards without restarting barebox.
> 
> The fastboot code is also changed to check for existing partitions when
> they are actually used and no longer during initialization time of the
> gadget. With this it becomes possible to expose a full device via
> fastboot and also some not yet existing partitions. When the full device
> is written to one can then write to a (newly created) partition directly
> afterwards.
> 
> The reparsing of the partition table only works when none of the
> partitions are mounted or otherwise opened. When a partition is open
> when the partition table changes, then barebox will continue with
> the old partition table just like Linux does.

Finally :) except my three comments fell free to add my:

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Re: [PATCH 0/7] Detect partition changes at runtime

[Sascha Hauer]

On Thu, 15 Feb 2024 08:47:50 +0100, Sascha Hauer wrote:
> With this series barebox can detect partition table changes at runtime
> and reparse the partition table in that case. With this it's possible
> for example to write an image to a SD card and mount partitions directly
> afterwards without restarting barebox.
> 
> The fastboot code is also changed to check for existing partitions when
> they are actually used and no longer during initialization time of the
> gadget. With this it becomes possible to expose a full device via
> fastboot and also some not yet existing partitions. When the full device
> is written to one can then write to a (newly created) partition directly
> afterwards.
> 
> [...]

Applied, thanks!

[1/7] fs: move cdev open count to cdev_open()/cdev_close()
      https://git.pengutronix.de/cgit/barebox/commit/?id=d84c3daf1314 (link may not be stable)
[2/7] common: partitions: efi: fix memory leak
      https://git.pengutronix.de/cgit/barebox/commit/?id=729370043ab6 (link may not be stable)
[3/7] partition: allow to reparse a partition table
      https://git.pengutronix.de/cgit/barebox/commit/?id=c641d9ed9d35 (link may not be stable)
[4/7] block: reparse partition table when necessary
      https://git.pengutronix.de/cgit/barebox/commit/?id=713a23cb2aea (link may not be stable)
[5/7] fastboot: pass list to fb_addvar()
      https://git.pengutronix.de/cgit/barebox/commit/?id=8e9895959d3f (link may not be stable)
[6/7] fastboot: add function to free a list of fastboot variables
      https://git.pengutronix.de/cgit/barebox/commit/?id=a8614972b161 (link may not be stable)
[7/7] fastboot: evaluate fastboot partitions when used
      https://git.pengutronix.de/cgit/barebox/commit/?id=6a191155be4e (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH 3/7] partition: allow to reparse a partition table

[Sascha Hauer]

On Thu, Feb 15, 2024 at 12:07:52PM +0100, Marco Felsch wrote:
> On 24-02-15, Sascha Hauer wrote:
> > The partition table of a block device may change when it is written to.
> > Allow to reparse the partition table when this happens.
> > This is easy when none of the partitions is opened, in that case just
> > remove the old partitions and call parse_partition_table() again.
> > When a partition is opened (including mounted) we can't remove it. In
> > that case continue with the old partition table and print a warning.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Just two nits, to make cdev access more opaque.
> 
> > ---
> >  common/partitions.c | 20 ++++++++++++++++++++
> >  include/disks.h     |  1 +
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/common/partitions.c b/common/partitions.c
> > index 78b68276c3..cfcd0e080b 100644
> > --- a/common/partitions.c
> > +++ b/common/partitions.c
> > @@ -154,6 +154,26 @@ int parse_partition_table(struct block_device *blk)
> >  	return rc;
> >  }
> >  
> > +int reparse_partition_table(struct block_device *blk)
> > +{
> > +	struct cdev *cdev = &blk->cdev;
> > +	struct cdev *c, *tmp;
> > +
> > +	list_for_each_entry(c, &cdev->partitions, partition_entry) {
> > +		if (c->open) {
> 
> 		if (cdev_is_open(c))

I was assuming this function already exists, but it doesn't. I'll leave
that for a later exercise.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 7/7] fastboot: evaluate fastboot partitions when used

[Sascha Hauer]

On Thu, Feb 15, 2024 at 01:51:18PM +0100, Marco Felsch wrote:
> On 24-02-15, Sascha Hauer wrote:
> > So far we have evaluated the fastboot partition description during
> > initialization of the fastboot gadget. Let's make this more flexible
> > and parse it when actually needed.
> > This gives us the possibility to first write an image including a
> > partition table to a disk and afterwards write an image to a single
> > partition created by the first whole disk write.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  common/fastboot.c | 40 +++++++++++++++++++++++++---------------
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> > 
> > diff --git a/common/fastboot.c b/common/fastboot.c
> > index f41c02a576..acf138af8a 100644
> > --- a/common/fastboot.c
> > +++ b/common/fastboot.c
> > @@ -80,7 +80,7 @@ static struct fb_variable *fb_addvar(struct fastboot *fb, struct list_head *list
> >  	return var;
> >  }
> >  
> > -static int fastboot_add_partition_variables(struct fastboot *fb,
> > +static int fastboot_add_partition_variables(struct fastboot *fb, struct list_head *list,
> >  		struct file_list_entry *fentry)
> >  {
> >  	struct stat s;
> > @@ -152,9 +152,9 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
> >  	if (ret)
> >  		return ret;
> >  
> > -	var = fb_addvar(fb, &fb->variables, "partition-size:%s", fentry->name);
> > +	var = fb_addvar(fb, list, "partition-size:%s", fentry->name);
> >  	fb_setvar(var, "%08zx", size);
> > -	var = fb_addvar(fb, &fb->variables, "partition-type:%s", fentry->name);
> > +	var = fb_addvar(fb, list, "partition-type:%s", fentry->name);
> >  	fb_setvar(var, "%s", type);
> >  
> >  	return ret;
> > @@ -162,8 +162,6 @@ static int fastboot_add_partition_variables(struct fastboot *fb,
> >  
> >  int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
> >  {
> > -	int ret;
> > -	struct file_list_entry *fentry;
> >  	struct fb_variable *var;
> >  
> >  	INIT_LIST_HEAD(&fb->variables);
> > @@ -186,12 +184,6 @@ int fastboot_generic_init(struct fastboot *fb, bool export_bbu)
> >  	if (export_bbu)
> >  		bbu_append_handlers_to_file_list(fb->files);
> >  
> > -	file_list_for_each_entry(fb->files, fentry) {
> > -		ret = fastboot_add_partition_variables(fb, fentry);
> > -		if (ret)
> > -			return ret;
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > @@ -305,26 +297,44 @@ static int strcmp_l1(const char *s1, const char *s2)
> >  static void cb_getvar(struct fastboot *fb, const char *cmd)
> >  {
> >  	struct fb_variable *var;
> > +	LIST_HEAD(partition_list);
> > +	struct file_list_entry *fentry;
> > +
> > +	file_list_for_each_entry(fb->files, fentry)
> > +		fastboot_add_partition_variables(fb, &partition_list, fentry);
> 
> Albeit we can ignore it, I would check the return code and print a
> warning like:
> 
> 	file_list_for_each_entry(fb->files, fentry) {
> 		int ret;
> 
> 		ret = fastboot_add_partition_variables(fb, &partition_list, fentry);
> 		if (ret) {
> 			pr_warn("Failed to add partition variables");
> 			return;
> 		}
> 	}

Yes, did it like this.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/7] block: reparse partition table when necessary

[Sascha Hauer]

On Thu, Feb 15, 2024 at 01:52:28PM +0100, Marco Felsch wrote:
> On 24-02-15, Sascha Hauer wrote:
> > Call reparse_partition_table() when the partition table may have
> > changed. We detect this by recording if the block device has been
> > written to in the area where the partition table is.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  common/block.c  | 30 ++++++++++++++++++++++++++++--
> >  include/block.h |  2 ++
> >  2 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/block.c b/common/block.c
> > index 3a4a9fb731..007c8bed12 100644
> > --- a/common/block.c
> > +++ b/common/block.c
> > @@ -284,6 +284,17 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
> >  	blkcnt_t blocks;
> >  	int ret;
> >  
> > +	/*
> > +	 * When the offset that is written to is within the first two
> > +	 * LBAs then the partition table has changed, reparse the partition
> > +	 * table at close time in this case. A GPT covers more space than
> > +	 * only the first two LBAs, but a CRC of the remaining pieces is
> > +	 * written to LBA1, so LBA1 must change as well when the partioning
> > +	 * is changed.
> > +	 */
> > +	if (offset < 2 * SECTOR_SIZE)
> > +		blk->need_reparse = true;
> > +
> >  	if (offset & mask) {
> >  		size_t now = BLOCKSIZE(blk) - (offset & mask);
> >  		void *iobuf = block_get(blk, block);
> > @@ -335,13 +346,28 @@ static ssize_t block_op_write(struct cdev *cdev, const void *buf, size_t count,
> >  static int block_op_flush(struct cdev *cdev)
> >  {
> >  	struct block_device *blk = cdev->priv;
> > +	int ret;
> >  
> >  	blk->discard_start = blk->discard_size = 0;
> >  
> > -	return writebuffer_flush(blk);
> > +	ret = writebuffer_flush(blk);
> > +
> > +	return ret;
> 
> This change is not required or do I miss something?

Fixed, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |