[PATCH RFT v2 0/3] fastboot: add support for generic block flashing

[PATCH RFT v2 0/3] fastboot: add support for generic block flashing

[neil.armstrong]

This serie permits using any block device as target
for fastboot by moving the generic block logic into
a common set of helpers and also use them as generic
backend.

The erase logic has been extended to support software
erase since only 2 block drivers exposes the erase
operation.

Tests are welcome to make sure this series doesn't
introduce any regressions on the emmc backend.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v2:
- Dropped applied virtio erase patch
- Reorganize patches, introducing helpers first, using them in mmc afterwards
- Added soft-erase logic
- Added move helpers to handle the partitions erase & flash from emmc
- Fixed const var on last patch
- Link to v1: https://lore.kernel.org/all/20240306185921.1854109-1-dimorinny@google.com/

---
Dmitrii Merkurev (3):
      fastboot: blk: introduce fastboot block flashing support
      fastboot: blk: switch emmc to use the block helpers
      fastboot: integrate block flashing back-end

 drivers/fastboot/Kconfig      |  20 ++-
 drivers/fastboot/Makefile     |   4 +-
 drivers/fastboot/fb_block.c   | 313 ++++++++++++++++++++++++++++++++++++++++++
 drivers/fastboot/fb_command.c |   8 ++
 drivers/fastboot/fb_common.c  |  16 ++-
 drivers/fastboot/fb_getvar.c  |   8 +-
 drivers/fastboot/fb_mmc.c     | 210 ++--------------------------
 include/fb_block.h            | 104 ++++++++++++++
 8 files changed, 477 insertions(+), 206 deletions(-)
---
base-commit: f892a7f397a66d8d09f418d1e0e06dfb48bac27d
change-id: 20250408-topic-fastboot-blk-c5e14cd59224

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[neil.armstrong]

From: Dmitrii Merkurev <dimorinny@google.com>

Introduce fastboot block flashing functions and helpers
to be shared with the MMC implementation.

The write logic comes from the mmc implementation, while
the partition lookup is much simpler and could be extended.

For the erase logic, allmost no block drivers exposes the
erase operation, except mmc & virtio, so in order to allow
erasiong any partition a soft-erase logic has been added
to write zero-ed buffers in a loop.

Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/fastboot/Kconfig    |  20 ++-
 drivers/fastboot/Makefile   |   4 +-
 drivers/fastboot/fb_block.c | 313 ++++++++++++++++++++++++++++++++++++++++++++
 include/fb_block.h          | 104 +++++++++++++++
 4 files changed, 438 insertions(+), 3 deletions(-)

diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
index 1eb460f5a02a87089d85b252375202d9acaec9fa..76b35d8419e02a7f1d36988fbdac190b10bff35e 100644
--- a/drivers/fastboot/Kconfig
+++ b/drivers/fastboot/Kconfig
@@ -92,7 +92,7 @@ config FASTBOOT_USB_DEV
 config FASTBOOT_FLASH
 	bool "Enable FASTBOOT FLASH command"
 	default y if ARCH_SUNXI || ARCH_ROCKCHIP
-	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
+	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS) || BLK
 	select IMAGE_SPARSE
 	help
 	  The fastboot protocol includes a "flash" command for writing
@@ -114,12 +114,16 @@ choice
 
 config FASTBOOT_FLASH_MMC
 	bool "FASTBOOT on MMC"
-	depends on MMC
+	depends on MMC && BLK
 
 config FASTBOOT_FLASH_NAND
 	bool "FASTBOOT on NAND"
 	depends on MTD_RAW_NAND && CMD_MTDPARTS
 
+config FASTBOOT_FLASH_BLOCK
+	bool "FASTBOOT on block device"
+	depends on BLK
+
 endchoice
 
 config FASTBOOT_FLASH_MMC_DEV
@@ -194,6 +198,18 @@ config FASTBOOT_MMC_USER_NAME
 	  defined here.
 	  The default target name for erasing EMMC_USER is "mmc0".
 
+config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
+	string "Define FASTBOOT block interface name"
+	depends on FASTBOOT_FLASH_BLOCK
+	help
+	  "Fastboot block interface name (mmc, virtio, etc)"
+
+config FASTBOOT_FLASH_BLOCK_DEVICE_ID
+	int "Define FASTBOOT block device id"
+	depends on FASTBOOT_FLASH_BLOCK
+	help
+	  "Fastboot block device id"
+
 config FASTBOOT_GPT_NAME
 	string "Target name for updating GPT"
 	depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile
index 048af5aa823436956142a536c5f7dcf1a8948726..91e98763e8eab84ccd9b8e5354ff1419f61ef372 100644
--- a/drivers/fastboot/Makefile
+++ b/drivers/fastboot/Makefile
@@ -3,5 +3,7 @@
 obj-y += fb_common.o
 obj-y += fb_getvar.o
 obj-y += fb_command.o
-obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o
+obj-$(CONFIG_FASTBOOT_FLASH_BLOCK) += fb_block.o
+# MMC reuses block implementation
+obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_block.o fb_mmc.o
 obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o
diff --git a/drivers/fastboot/fb_block.c b/drivers/fastboot/fb_block.c
new file mode 100644
index 0000000000000000000000000000000000000000..e79429a9732c1da1ce88358747cf1cb9419cd12b
--- /dev/null
+++ b/drivers/fastboot/fb_block.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: BSD-2-Clause
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ */
+
+#include <blk.h>
+#include <div64.h>
+#include <fastboot-internal.h>
+#include <fastboot.h>
+#include <fb_block.h>
+#include <image-sparse.h>
+#include <part.h>
+#include <malloc.h>
+
+/**
+ * FASTBOOT_MAX_BLOCKS_ERASE - maximum blocks to erase per derase call
+ *
+ * in the ERASE case we can have much larger buffer size since
+ * we're not transferring an actual buffer
+ */
+#define FASTBOOT_MAX_BLOCKS_ERASE 1048576
+/**
+ * FASTBOOT_MAX_BLOCKS_SOFT_ERASE - maximum blocks to software erase at once
+ */
+#define FASTBOOT_MAX_BLOCKS_SOFT_ERASE 4096
+/**
+ * FASTBOOT_MAX_BLOCKS_WRITE - maximum blocks to write per dwrite call
+ */
+#define FASTBOOT_MAX_BLOCKS_WRITE 65536
+
+struct fb_block_sparse {
+	struct blk_desc	*dev_desc;
+};
+
+static lbaint_t fb_block_write(struct blk_desc *block_dev, lbaint_t start,
+			       lbaint_t blkcnt, const void *buffer)
+{
+	lbaint_t blk = start;
+	lbaint_t blks_written = 0;
+	lbaint_t cur_blkcnt = 0;
+	lbaint_t blks = 0;
+	void *erase_buf = NULL;
+	int erase_buf_blks = 0;
+	int step = buffer ? FASTBOOT_MAX_BLOCKS_WRITE : FASTBOOT_MAX_BLOCKS_ERASE;
+	int i;
+
+	for (i = 0; i < blkcnt; i += step) {
+		cur_blkcnt = min((int)blkcnt - i, step);
+		if (buffer) {
+			if (fastboot_progress_callback)
+				fastboot_progress_callback("writing");
+			blks_written = blk_dwrite(block_dev, blk, cur_blkcnt,
+						  buffer + (i * block_dev->blksz));
+		} else {
+			if (fastboot_progress_callback)
+				fastboot_progress_callback("erasing");
+
+			if (!erase_buf) {
+				blks_written = blk_derase(block_dev, blk, cur_blkcnt);
+
+				/* Allocate erase buffer if erase is not implemented */
+				if ((long)blks_written == -ENOSYS) {
+					erase_buf_blks = min_t(long, blkcnt,
+							       FASTBOOT_MAX_BLOCKS_SOFT_ERASE);
+					erase_buf = malloc(erase_buf_blks * block_dev->blksz);
+					memset(erase_buf, 0, erase_buf_blks * block_dev->blksz);
+
+					printf("Slowly writing empty buffers due to missing erase operation\n");
+				}
+			}
+
+			/* Write 0s instead of using erase operation, inefficient but functional */
+			if (erase_buf) {
+				int j;
+
+				blks_written = 0;
+				for (j = 0; j < cur_blkcnt; j += erase_buf_blks) {
+					lbaint_t remain = min_t(lbaint_t, cur_blkcnt - j,
+								erase_buf_blks);
+
+					blks_written += blk_dwrite(block_dev, blk + j,
+								   remain, erase_buf);
+					printf(".");
+				}
+			}
+		}
+		blk += blks_written;
+		blks += blks_written;
+	}
+
+	if (erase_buf)
+		free(erase_buf);
+
+	return blks;
+}
+
+static lbaint_t fb_block_sparse_write(struct sparse_storage *info,
+				      lbaint_t blk, lbaint_t blkcnt,
+				      const void *buffer)
+{
+	struct fb_block_sparse *sparse = info->priv;
+	struct blk_desc *dev_desc = sparse->dev_desc;
+
+	return fb_block_write(dev_desc, blk, blkcnt, buffer);
+}
+
+static lbaint_t fb_block_sparse_reserve(struct sparse_storage *info,
+					lbaint_t blk, lbaint_t blkcnt)
+{
+	return blkcnt;
+}
+
+int fastboot_block_get_part_info(const char *part_name,
+				 struct blk_desc **dev_desc,
+				 struct disk_partition *part_info,
+				 char *response)
+{
+	int ret;
+	const char *interface = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
+						   CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME,
+						   NULL);
+	const int device = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
+					      CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID, -1);
+
+	if (!part_name || !strcmp(part_name, "")) {
+		fastboot_fail("partition not given", response);
+		return -ENOENT;
+	}
+	if (!interface || !strcmp(interface, "")) {
+		fastboot_fail("block interface isn't provided", response);
+		return -EINVAL;
+	}
+
+	*dev_desc = blk_get_dev(interface, device);
+	if (!dev_desc) {
+		fastboot_fail("no such device", response);
+		return -ENODEV;
+	}
+
+	ret = part_get_info_by_name(*dev_desc, part_name, part_info);
+	if (ret < 0)
+		fastboot_fail("failed to get partition info", response);
+
+	return ret;
+}
+
+void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
+				   char *response)
+{
+	lbaint_t written;
+
+	debug("Start Erasing %s...\n", disk_name);
+
+	written = fb_block_write(dev_desc, 0, dev_desc->lba, NULL);
+	if (written != dev_desc->lba) {
+		pr_err("Failed to erase %s\n", disk_name);
+		fastboot_response("FAIL", response, "Failed to erase %s", disk_name);
+		return;
+	}
+
+	printf("........ erased " LBAFU " bytes from '%s'\n",
+	       dev_desc->lba * dev_desc->blksz, disk_name);
+	fastboot_okay(NULL, response);
+}
+
+void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
+			      const char *part_name, uint alignment, char *response)
+{
+	lbaint_t written, blks_start, blks_size;
+
+	if (alignment) {
+		blks_start = (info->start + alignment - 1) & ~(alignment - 1);
+		if (info->size >= alignment)
+			blks_size = (info->size - (blks_start - info->start)) &
+				(~(alignment - 1));
+		else
+			blks_size = 0;
+
+		printf("Erasing blocks " LBAFU " to " LBAFU " due to alignment\n",
+		       blks_start, blks_start + blks_size);
+	} else {
+		blks_start = info->start;
+		blks_size = info->size;
+	}
+
+	written = fb_block_write(dev_desc, blks_start, blks_size, NULL);
+	if (written != blks_size) {
+		fastboot_fail("failed to erase partition", response);
+		return;
+	}
+
+	printf("........ erased " LBAFU " bytes from '%s'\n",
+	       blks_size * info->blksz, part_name);
+	fastboot_okay(NULL, response);
+}
+
+void fastboot_block_erase(const char *part_name, char *response)
+{
+	struct blk_desc *dev_desc;
+	struct disk_partition part_info;
+
+	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
+		return;
+
+	fastboot_block_raw_erase(dev_desc, &part_info, part_name, 0, response);
+}
+
+void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
+				   void *buffer, u32 download_bytes, char *response)
+{
+	lbaint_t blkcnt;
+	lbaint_t blks;
+
+	/* determine number of blocks to write */
+	blkcnt = ((download_bytes + (dev_desc->blksz - 1)) & ~(dev_desc->blksz - 1));
+	blkcnt = lldiv(blkcnt, dev_desc->blksz);
+
+	if (blkcnt > dev_desc->lba) {
+		pr_err("too large for disk: '%s'\n", disk_name);
+		fastboot_fail("too large for disk", response);
+		return;
+	}
+
+	puts("Flashing Raw Image\n");
+
+	blks = fb_block_write(dev_desc, 0, blkcnt, buffer);
+
+	if (blks != blkcnt) {
+		pr_err("failed writing to %s\n", disk_name);
+		fastboot_fail("failed writing to device", response);
+		return;
+	}
+
+	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * dev_desc->blksz,
+	       disk_name);
+	fastboot_okay(NULL, response);
+}
+
+void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
+				    struct disk_partition *info, const char *part_name,
+				    void *buffer, u32 download_bytes, char *response)
+{
+	lbaint_t blkcnt;
+	lbaint_t blks;
+
+	/* determine number of blocks to write */
+	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
+	blkcnt = lldiv(blkcnt, info->blksz);
+
+	if (blkcnt > info->size) {
+		pr_err("too large for partition: '%s'\n", part_name);
+		fastboot_fail("too large for partition", response);
+		return;
+	}
+
+	puts("Flashing Raw Image\n");
+
+	blks = fb_block_write(dev_desc, info->start, blkcnt, buffer);
+
+	if (blks != blkcnt) {
+		pr_err("failed writing to device %d\n", dev_desc->devnum);
+		fastboot_fail("failed writing to device", response);
+		return;
+	}
+
+	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
+	       part_name);
+	fastboot_okay(NULL, response);
+}
+
+void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
+				       const char *part_name, void *buffer, char *response)
+{
+	struct fb_block_sparse sparse_priv;
+	struct sparse_storage sparse;
+	int err;
+
+	sparse_priv.dev_desc = dev_desc;
+
+	sparse.blksz = info->blksz;
+	sparse.start = info->start;
+	sparse.size = info->size;
+	sparse.write = fb_block_sparse_write;
+	sparse.reserve = fb_block_sparse_reserve;
+	sparse.mssg = fastboot_fail;
+
+	printf("Flashing sparse image at offset " LBAFU "\n",
+	       sparse.start);
+
+	sparse.priv = &sparse_priv;
+	err = write_sparse_image(&sparse, part_name, buffer,
+				 response);
+	if (!err)
+		fastboot_okay(NULL, response);
+}
+
+void fastboot_block_flash_write(const char *part_name, void *download_buffer,
+				u32 download_bytes, char *response)
+{
+	struct blk_desc *dev_desc;
+	struct disk_partition part_info;
+
+	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
+		return;
+
+	if (is_sparse_image(download_buffer)) {
+		fastboot_block_write_sparse_image(dev_desc, &part_info, part_name,
+						  download_buffer, response);
+	} else {
+		fastboot_block_write_raw_image(dev_desc, &part_info, part_name,
+					       download_buffer, download_bytes, response);
+	}
+}
diff --git a/include/fb_block.h b/include/fb_block.h
new file mode 100644
index 0000000000000000000000000000000000000000..cfc824214acb515a0cc2f13fd786c606c6141489
--- /dev/null
+++ b/include/fb_block.h
@@ -0,0 +1,104 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (C) 2024 The Android Open Source Project
+ */
+
+#ifndef _FB_BLOCK_H_
+#define _FB_BLOCK_H_
+
+struct blk_desc;
+struct disk_partition;
+
+/**
+ * fastboot_block_get_part_info() - Lookup block partition by name
+ *
+ * @part_name: Named partition to lookup
+ * @dev_desc: Pointer to returned blk_desc pointer
+ * @part_info: Pointer to returned struct disk_partition
+ * @response: Pointer to fastboot response buffer
+ */
+int fastboot_block_get_part_info(const char *part_name,
+				 struct blk_desc **dev_desc,
+				 struct disk_partition *part_info,
+				 char *response);
+
+/**
+ * fastboot_block_raw_erase() - Erase raw block device partition
+ *
+ * @dev_desc: Block device we're going write to
+ * @info: Partition we're going write to
+ * @part_name: Name of partition we're going write to
+ * @alignment: erase start and size alignment, specify 0 to ignore
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
+			      const char *part_name, uint alignment, char *response);
+
+/**
+ * fastboot_block_raw_erase_disk() - Erase raw block device
+ *
+ * @dev_desc: Block device we're going write to
+ * @disk_name: Name of disk we're going write to
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
+				   char *response);
+
+/**
+ * fastboot_block_erase() - Erase partition on block device for fastboot
+ *
+ * @part_name: Named partition to erase
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_erase(const char *part_name, char *response);
+
+/**
+ * fastboot_block_write_raw_disk() - Write raw image to block device
+ *
+ * @dev_desc: Block device we're going write to
+ * @disk_name: Name of disk we're going write to
+ * @buffer: Downloaded buffer pointer
+ * @download_bytes: Size of content on downloaded buffer pointer
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
+				   void *buffer, u32 download_bytes, char *response);
+
+/**
+ * fastboot_block_write_raw_image() - Write raw image to block device partition
+ *
+ * @dev_desc: Block device we're going write to
+ * @info: Partition we're going write to
+ * @part_name: Name of partition we're going write to
+ * @buffer: Downloaded buffer pointer
+ * @download_bytes: Size of content on downloaded buffer pointer
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
+				    struct disk_partition *info, const char *part_name,
+				    void *buffer, u32 download_bytes, char *response);
+
+/**
+ * fastboot_block_write_sparse_image() - Write sparse image to block device partition
+ *
+ * @dev_desc: Block device we're going write to
+ * @info: Partition we're going write to
+ * @part_name: Name of partition we're going write to
+ * @buffer: Downloaded buffer pointer
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
+				       const char *part_name, void *buffer, char *response);
+
+/**
+ * fastboot_block_flash_write() - Write image to block device for fastboot
+ *
+ * @part_name: Named partition to write image to
+ * @download_buffer: Pointer to image data
+ * @download_bytes: Size of image data
+ * @response: Pointer to fastboot response buffer
+ */
+void fastboot_block_flash_write(const char *part_name, void *download_buffer,
+				u32 download_bytes, char *response);
+
+#endif // _FB_BLOCK_H_

-- 
2.34.1

[PATCH RFT v2 2/3] fastboot: blk: switch emmc to use the block helpers

[neil.armstrong]

From: Dmitrii Merkurev <dimorinny@google.com>

Switch the mmc backend to this new shared block helpers,
reducing block logic and only leaving MMC specific logic.

Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/fastboot/fb_mmc.c | 210 +++-------------------------------------------
 1 file changed, 12 insertions(+), 198 deletions(-)

diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c
index dca7c222f35659b22d327541b245760a6a6d7b35..11d9c8e84602c7434733c060b84c91c38772ac9f 100644
--- a/drivers/fastboot/fb_mmc.c
+++ b/drivers/fastboot/fb_mmc.c
@@ -8,6 +8,7 @@
 #include <env.h>
 #include <fastboot.h>
 #include <fastboot-internal.h>
+#include <fb_block.h>
 #include <fb_mmc.h>
 #include <image-sparse.h>
 #include <image.h>
@@ -20,10 +21,6 @@
 
 #define BOOT_PARTITION_NAME "boot"
 
-struct fb_mmc_sparse {
-	struct blk_desc	*dev_desc;
-};
-
 static int raw_part_get_info_by_name(struct blk_desc *dev_desc,
 				     const char *name,
 				     struct disk_partition *info)
@@ -114,118 +111,10 @@ static int part_get_info_by_name_or_alias(struct blk_desc **dev_desc,
 	return do_get_part_info(dev_desc, name, info);
 }
 
-/**
- * fb_mmc_blk_write() - Write/erase MMC in chunks of FASTBOOT_MAX_BLK_WRITE
- *
- * @block_dev: Pointer to block device
- * @start: First block to write/erase
- * @blkcnt: Count of blocks
- * @buffer: Pointer to data buffer for write or NULL for erase
- */
-static lbaint_t fb_mmc_blk_write(struct blk_desc *block_dev, lbaint_t start,
-				 lbaint_t blkcnt, const void *buffer)
-{
-	lbaint_t blk = start;
-	lbaint_t blks_written;
-	lbaint_t cur_blkcnt;
-	lbaint_t blks = 0;
-	int i;
-
-	for (i = 0; i < blkcnt; i += FASTBOOT_MAX_BLK_WRITE) {
-		cur_blkcnt = min((int)blkcnt - i, FASTBOOT_MAX_BLK_WRITE);
-		if (buffer) {
-			if (fastboot_progress_callback)
-				fastboot_progress_callback("writing");
-			blks_written = blk_dwrite(block_dev, blk, cur_blkcnt,
-						  buffer + (i * block_dev->blksz));
-		} else {
-			if (fastboot_progress_callback)
-				fastboot_progress_callback("erasing");
-			blks_written = blk_derase(block_dev, blk, cur_blkcnt);
-		}
-		blk += blks_written;
-		blks += blks_written;
-	}
-	return blks;
-}
-
-static lbaint_t fb_mmc_sparse_write(struct sparse_storage *info,
-		lbaint_t blk, lbaint_t blkcnt, const void *buffer)
-{
-	struct fb_mmc_sparse *sparse = info->priv;
-	struct blk_desc *dev_desc = sparse->dev_desc;
-
-	return fb_mmc_blk_write(dev_desc, blk, blkcnt, buffer);
-}
-
-static lbaint_t fb_mmc_sparse_reserve(struct sparse_storage *info,
-		lbaint_t blk, lbaint_t blkcnt)
-{
-	return blkcnt;
-}
-
-static void write_raw_image(struct blk_desc *dev_desc,
-			    struct disk_partition *info, const char *part_name,
-			    void *buffer, u32 download_bytes, char *response)
-{
-	lbaint_t blkcnt;
-	lbaint_t blks;
-
-	/* determine number of blocks to write */
-	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
-	blkcnt = lldiv(blkcnt, info->blksz);
-
-	if (blkcnt > info->size) {
-		pr_err("too large for partition: '%s'\n", part_name);
-		fastboot_fail("too large for partition", response);
-		return;
-	}
-
-	puts("Flashing Raw Image\n");
-
-	blks = fb_mmc_blk_write(dev_desc, info->start, blkcnt, buffer);
-
-	if (blks != blkcnt) {
-		pr_err("failed writing to device %d\n", dev_desc->devnum);
-		fastboot_fail("failed writing to device", response);
-		return;
-	}
-
-	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
-	       part_name);
-	fastboot_okay(NULL, response);
-}
-
-#if defined(CONFIG_FASTBOOT_MMC_BOOT_SUPPORT) || \
-	defined(CONFIG_FASTBOOT_MMC_USER_SUPPORT)
-static int fb_mmc_erase_mmc_hwpart(struct blk_desc *dev_desc)
-{
-	lbaint_t blks;
-
-	debug("Start Erasing mmc hwpart[%u]...\n", dev_desc->hwpart);
-
-	blks = fb_mmc_blk_write(dev_desc, 0, dev_desc->lba, NULL);
-
-	if (blks != dev_desc->lba) {
-		pr_err("Failed to erase mmc hwpart[%u]\n", dev_desc->hwpart);
-		return 1;
-	}
-
-	printf("........ erased %llu bytes from mmc hwpart[%u]\n",
-	       (u64)(dev_desc->lba * dev_desc->blksz), dev_desc->hwpart);
-
-	return 0;
-}
-#endif
-
 #ifdef CONFIG_FASTBOOT_MMC_BOOT_SUPPORT
 static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer,
 			    int hwpart, u32 buff_sz, char *response)
 {
-	lbaint_t blkcnt;
-	lbaint_t blks;
-	unsigned long blksz;
-
 	// To operate on EMMC_BOOT1/2 (mmc0boot0/1) we first change the hwpart
 	if (blk_dselect_hwpart(dev_desc, hwpart)) {
 		pr_err("Failed to select hwpart\n");
@@ -233,42 +122,11 @@ static void fb_mmc_boot_ops(struct blk_desc *dev_desc, void *buffer,
 		return;
 	}
 
-	if (buffer) { /* flash */
-
-		/* determine number of blocks to write */
-		blksz = dev_desc->blksz;
-		blkcnt = ((buff_sz + (blksz - 1)) & ~(blksz - 1));
-		blkcnt = lldiv(blkcnt, blksz);
-
-		if (blkcnt > dev_desc->lba) {
-			pr_err("Image size too large\n");
-			fastboot_fail("Image size too large", response);
-			return;
-		}
-
-		debug("Start Flashing Image to EMMC_BOOT%d...\n", hwpart);
-
-		blks = fb_mmc_blk_write(dev_desc, 0, blkcnt, buffer);
-
-		if (blks != blkcnt) {
-			pr_err("Failed to write EMMC_BOOT%d\n", hwpart);
-			fastboot_fail("Failed to write EMMC_BOOT part",
-				      response);
-			return;
-		}
-
-		printf("........ wrote %llu bytes to EMMC_BOOT%d\n",
-		       (u64)(blkcnt * blksz), hwpart);
-	} else { /* erase */
-		if (fb_mmc_erase_mmc_hwpart(dev_desc)) {
-			pr_err("Failed to erase EMMC_BOOT%d\n", hwpart);
-			fastboot_fail("Failed to erase EMMC_BOOT part",
-				      response);
-			return;
-		}
-	}
-
-	fastboot_okay(NULL, response);
+	if (buffer) /* flash */
+		fastboot_block_write_raw_disk(dev_desc, "EMMC_BOOT",
+					      buffer, buff_sz, response);
+	else /* erase */
+		fastboot_block_raw_erase_disk(dev_desc, "EMMC_BOOT", response);
 }
 #endif
 
@@ -609,30 +467,11 @@ void fastboot_mmc_flash_write(const char *cmd, void *download_buffer,
 		return;
 
 	if (is_sparse_image(download_buffer)) {
-		struct fb_mmc_sparse sparse_priv;
-		struct sparse_storage sparse;
-		int err;
-
-		sparse_priv.dev_desc = dev_desc;
-
-		sparse.blksz = info.blksz;
-		sparse.start = info.start;
-		sparse.size = info.size;
-		sparse.write = fb_mmc_sparse_write;
-		sparse.reserve = fb_mmc_sparse_reserve;
-		sparse.mssg = fastboot_fail;
-
-		printf("Flashing sparse image at offset " LBAFU "\n",
-		       sparse.start);
-
-		sparse.priv = &sparse_priv;
-		err = write_sparse_image(&sparse, cmd, download_buffer,
-					 response);
-		if (!err)
-			fastboot_okay(NULL, response);
+		fastboot_block_write_sparse_image(dev_desc, &info, cmd,
+						  download_buffer, response);
 	} else {
-		write_raw_image(dev_desc, &info, cmd, download_buffer,
-				download_bytes, response);
+		fastboot_block_write_raw_image(dev_desc, &info, cmd, download_buffer,
+					       download_bytes, response);
 	}
 }
 
@@ -646,7 +485,6 @@ void fastboot_mmc_erase(const char *cmd, char *response)
 {
 	struct blk_desc *dev_desc;
 	struct disk_partition info;
-	lbaint_t blks, blks_start, blks_size, grp_size;
 	struct mmc *mmc = find_mmc_device(CONFIG_FASTBOOT_FLASH_MMC_DEV);
 
 #ifdef CONFIG_FASTBOOT_MMC_BOOT_SUPPORT
@@ -673,10 +511,7 @@ void fastboot_mmc_erase(const char *cmd, char *response)
 		if (!dev_desc)
 			return;
 
-		if (fb_mmc_erase_mmc_hwpart(dev_desc))
-			fastboot_fail("Failed to erase EMMC_USER", response);
-		else
-			fastboot_okay(NULL, response);
+		fastboot_block_raw_erase_disk(dev_desc, "EMMC_USER", response);
 		return;
 	}
 #endif
@@ -685,26 +520,5 @@ void fastboot_mmc_erase(const char *cmd, char *response)
 		return;
 
 	/* Align blocks to erase group size to avoid erasing other partitions */
-	grp_size = mmc->erase_grp_size;
-	blks_start = (info.start + grp_size - 1) & ~(grp_size - 1);
-	if (info.size >= grp_size)
-		blks_size = (info.size - (blks_start - info.start)) &
-				(~(grp_size - 1));
-	else
-		blks_size = 0;
-
-	printf("Erasing blocks " LBAFU " to " LBAFU " due to alignment\n",
-	       blks_start, blks_start + blks_size);
-
-	blks = fb_mmc_blk_write(dev_desc, blks_start, blks_size, NULL);
-
-	if (blks != blks_size) {
-		pr_err("failed erasing from device %d\n", dev_desc->devnum);
-		fastboot_fail("failed erasing from device", response);
-		return;
-	}
-
-	printf("........ erased " LBAFU " bytes from '%s'\n",
-	       blks_size * info.blksz, cmd);
-	fastboot_okay(NULL, response);
+	fastboot_block_raw_erase(dev_desc, &info, cmd, mmc->erase_grp_size, response);
 }

-- 
2.34.1

[PATCH RFT v2 3/3] fastboot: integrate block flashing back-end

[neil.armstrong]

From: Dmitrii Merkurev <dimorinny@google.com>

1. Get partition info/size
2. Erase partition
3. Flash partition
4. BCB

Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/fastboot/fb_command.c |  8 ++++++++
 drivers/fastboot/fb_common.c  | 16 ++++++++++++----
 drivers/fastboot/fb_getvar.c  |  8 +++++++-
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index e4484d65aca5994cbaeb1a9276e09fbe732fb46f..f1ab824ad619d04b4430b0391d13d256581ccea1 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -8,6 +8,7 @@
 #include <env.h>
 #include <fastboot.h>
 #include <fastboot-internal.h>
+#include <fb_block.h>
 #include <fb_mmc.h>
 #include <fb_nand.h>
 #include <part.h>
@@ -336,6 +337,10 @@ void fastboot_data_complete(char *response)
  */
 static void __maybe_unused flash(char *cmd_parameter, char *response)
 {
+	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK))
+		fastboot_block_flash_write(cmd_parameter, fastboot_buf_addr,
+					   image_size, response);
+
 	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
 		fastboot_mmc_flash_write(cmd_parameter, fastboot_buf_addr,
 					 image_size, response);
@@ -356,6 +361,9 @@ static void __maybe_unused flash(char *cmd_parameter, char *response)
  */
 static void __maybe_unused erase(char *cmd_parameter, char *response)
 {
+	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK))
+		fastboot_block_erase(cmd_parameter, response);
+
 	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
 		fastboot_mmc_erase(cmd_parameter, response);
 
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
index 12ffb463deb98229b3486cfd54b4785fcc041224..58d99f1472e2ffbf9460f0e9a8d4cb14b3f1bacb 100644
--- a/drivers/fastboot/fb_common.c
+++ b/drivers/fastboot/fb_common.c
@@ -97,16 +97,24 @@ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason)
 		[FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot",
 		[FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery"
 	};
-	const int mmc_dev = config_opt_enabled(CONFIG_FASTBOOT_FLASH_MMC,
-					       CONFIG_FASTBOOT_FLASH_MMC_DEV, -1);
 
-	if (!IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC))
+	int device = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
+					CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID, -1);
+	if (device == -1) {
+		device = config_opt_enabled(CONFIG_FASTBOOT_FLASH_MMC,
+					    CONFIG_FASTBOOT_FLASH_MMC_DEV, -1);
+	}
+	const char *bcb_iface = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
+						   CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME,
+						   "mmc");
+
+	if (device == -1)
 		return -EINVAL;
 
 	if (reason >= FASTBOOT_REBOOT_REASONS_COUNT)
 		return -EINVAL;
 
-	ret = bcb_find_partition_and_load("mmc", mmc_dev, "misc");
+	ret = bcb_find_partition_and_load(bcb_iface, device, "misc");
 	if (ret)
 		goto out;
 
diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
index 9c2ce65a4e5bce0da6b18aa1b2818f7db556c528..f083b21c797dc7e55315f2cba017a4372483fa92 100644
--- a/drivers/fastboot/fb_getvar.c
+++ b/drivers/fastboot/fb_getvar.c
@@ -7,6 +7,7 @@
 #include <fastboot.h>
 #include <fastboot-internal.h>
 #include <fb_mmc.h>
+#include <fb_block.h>
 #include <fb_nand.h>
 #include <fs.h>
 #include <part.h>
@@ -114,7 +115,12 @@ static int getvar_get_part_info(const char *part_name, char *response,
 	struct disk_partition disk_part;
 	struct part_info *part_info;
 
-	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC)) {
+	if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_BLOCK)) {
+		r = fastboot_block_get_part_info(part_name, &dev_desc, &disk_part,
+						 response);
+		if (r >= 0 && size)
+			*size = disk_part.size * disk_part.blksz;
+	} else if (IS_ENABLED(CONFIG_FASTBOOT_FLASH_MMC)) {
 		r = fastboot_mmc_get_part_info(part_name, &dev_desc, &disk_part,
 					       response);
 		if (r >= 0 && size)

-- 
2.34.1

Re: [PATCH RFT v2 0/3] fastboot: add support for generic block flashing

[Neil Armstrong]

Hi Mattijs,

On 09/04/2025 09:58, neil.armstrong@linaro.org wrote:
> This serie permits using any block device as target
> for fastboot by moving the generic block logic into
> a common set of helpers and also use them as generic
> backend.
> 
> The erase logic has been extended to support software
> erase since only 2 block drivers exposes the erase
> operation.
> 
> Tests are welcome to make sure this series doesn't
> introduce any regressions on the emmc backend.

Could you get an eye on this v2 ?

Thanks,
Neil

> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Changes in v2:
> - Dropped applied virtio erase patch
> - Reorganize patches, introducing helpers first, using them in mmc afterwards
> - Added soft-erase logic
> - Added move helpers to handle the partitions erase & flash from emmc
> - Fixed const var on last patch
> - Link to v1: https://lore.kernel.org/all/20240306185921.1854109-1-dimorinny@google.com/
> 
> ---
> Dmitrii Merkurev (3):
>        fastboot: blk: introduce fastboot block flashing support
>        fastboot: blk: switch emmc to use the block helpers
>        fastboot: integrate block flashing back-end
> 
>   drivers/fastboot/Kconfig      |  20 ++-
>   drivers/fastboot/Makefile     |   4 +-
>   drivers/fastboot/fb_block.c   | 313 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/fastboot/fb_command.c |   8 ++
>   drivers/fastboot/fb_common.c  |  16 ++-
>   drivers/fastboot/fb_getvar.c  |   8 +-
>   drivers/fastboot/fb_mmc.c     | 210 ++--------------------------
>   include/fb_block.h            | 104 ++++++++++++++
>   8 files changed, 477 insertions(+), 206 deletions(-)
> ---
> base-commit: f892a7f397a66d8d09f418d1e0e06dfb48bac27d
> change-id: 20250408-topic-fastboot-blk-c5e14cd59224
> 
> Best regards,

Re: [PATCH RFT v2 0/3] fastboot: add support for generic block flashing

[Mattijs Korpershoek]

Hey Neil,

On lun., avril 14, 2025 at 11:13, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> Hi Mattijs,
>
> On 09/04/2025 09:58, neil.armstrong@linaro.org wrote:
>> This serie permits using any block device as target
>> for fastboot by moving the generic block logic into
>> a common set of helpers and also use them as generic
>> backend.
>> 
>> The erase logic has been extended to support software
>> erase since only 2 block drivers exposes the erase
>> operation.
>> 
>> Tests are welcome to make sure this series doesn't
>> introduce any regressions on the emmc backend.
>
> Could you get an eye on this v2 ?

Yes, it's on my radar. I plan to test the emmc backend on Khadas VIM3. I
have not gotten to it yet, but will do this week.

Thank you for your patience.
Mattijs

>
> Thanks,
> Neil
>
>> 
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>> Changes in v2:
>> - Dropped applied virtio erase patch
>> - Reorganize patches, introducing helpers first, using them in mmc afterwards
>> - Added soft-erase logic
>> - Added move helpers to handle the partitions erase & flash from emmc
>> - Fixed const var on last patch
>> - Link to v1: https://lore.kernel.org/all/20240306185921.1854109-1-dimorinny@google.com/
>> 
>> ---
>> Dmitrii Merkurev (3):
>>        fastboot: blk: introduce fastboot block flashing support
>>        fastboot: blk: switch emmc to use the block helpers
>>        fastboot: integrate block flashing back-end
>> 
>>   drivers/fastboot/Kconfig      |  20 ++-
>>   drivers/fastboot/Makefile     |   4 +-
>>   drivers/fastboot/fb_block.c   | 313 ++++++++++++++++++++++++++++++++++++++++++
>>   drivers/fastboot/fb_command.c |   8 ++
>>   drivers/fastboot/fb_common.c  |  16 ++-
>>   drivers/fastboot/fb_getvar.c  |   8 +-
>>   drivers/fastboot/fb_mmc.c     | 210 ++--------------------------
>>   include/fb_block.h            | 104 ++++++++++++++
>>   8 files changed, 477 insertions(+), 206 deletions(-)
>> ---
>> base-commit: f892a7f397a66d8d09f418d1e0e06dfb48bac27d
>> change-id: 20250408-topic-fastboot-blk-c5e14cd59224
>> 
>> Best regards,

Re: [PATCH RFT v2 0/3] fastboot: add support for generic block flashing

[Mattijs Korpershoek]

Hey Neil,

Thank you for the series.

On mer., avril 09, 2025 at 09:58, neil.armstrong@linaro.org wrote:

> This serie permits using any block device as target
> for fastboot by moving the generic block logic into
> a common set of helpers and also use them as generic
> backend.
>
> The erase logic has been extended to support software
> erase since only 2 block drivers exposes the erase
> operation.
>
> Tests are welcome to make sure this series doesn't
> introduce any regressions on the emmc backend.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

On top of master (commit 5b4ae0f3f040 ("mailmap: update my name and email")),
Using khadas-vim3_android_defconfig, I did the following:

boot the newly generated U-Boot with boot-g12.py:
$ boot-g12.py ~/work/upstream/yukawa/bootloader/u-boot_kvim3_noab.bin

Run fastboot from U-Boot console:
=> fastboot usb 0
crq->brequest:0x0

Then, on the host, reflash the bootloader:
$ fastboot flash bootloader u-boot_kvim3_noab.bin

In U-Boot, everything seems fine:
** Bad device specification mmc bootloader_a **
** Bad device specification mmc bootloader_a **
Couldn't find partition mmc bootloader_a
Starting download of 1288048 bytes
.........
downloading of 1288048 bytes finished
Flashing Raw Image
........ wrote 1288192 bytes to 'bootloader'
resetting ...
bl31 reboot reason: 0xd
bl31 reboot reason: 0x0
system cmd  1.
G12B:BL:6e7c85:2a3b91;FEAT:E0F83180:402000;POC:D;RCY:0;USB:0;

And after rebooting the board, we are on the newly flashed bootloader.

So, to me:

Tested-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

I do need some more time to review, though.
Thank you for your patience!

> ---
> Changes in v2:
> - Dropped applied virtio erase patch
> - Reorganize patches, introducing helpers first, using them in mmc afterwards
> - Added soft-erase logic
> - Added move helpers to handle the partitions erase & flash from emmc
> - Fixed const var on last patch
> - Link to v1: https://lore.kernel.org/all/20240306185921.1854109-1-dimorinny@google.com/
>
> ---
> Dmitrii Merkurev (3):
>       fastboot: blk: introduce fastboot block flashing support
>       fastboot: blk: switch emmc to use the block helpers
>       fastboot: integrate block flashing back-end
>
>  drivers/fastboot/Kconfig      |  20 ++-
>  drivers/fastboot/Makefile     |   4 +-
>  drivers/fastboot/fb_block.c   | 313 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/fastboot/fb_command.c |   8 ++
>  drivers/fastboot/fb_common.c  |  16 ++-
>  drivers/fastboot/fb_getvar.c  |   8 +-
>  drivers/fastboot/fb_mmc.c     | 210 ++--------------------------
>  include/fb_block.h            | 104 ++++++++++++++
>  8 files changed, 477 insertions(+), 206 deletions(-)
> ---
> base-commit: f892a7f397a66d8d09f418d1e0e06dfb48bac27d
> change-id: 20250408-topic-fastboot-blk-c5e14cd59224
>
> Best regards,
> -- 
> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[Mattijs Korpershoek]

Hi Neil,

Thank you for the patch.

On mer., avril 09, 2025 at 09:58, neil.armstrong@linaro.org wrote:

> From: Dmitrii Merkurev <dimorinny@google.com>
>
> Introduce fastboot block flashing functions and helpers
> to be shared with the MMC implementation.
>
> The write logic comes from the mmc implementation, while
> the partition lookup is much simpler and could be extended.
>
> For the erase logic, allmost no block drivers exposes the
> erase operation, except mmc & virtio, so in order to allow
> erasiong any partition a soft-erase logic has been added
> to write zero-ed buffers in a loop.
>
> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/fastboot/Kconfig    |  20 ++-
>  drivers/fastboot/Makefile   |   4 +-
>  drivers/fastboot/fb_block.c | 313 ++++++++++++++++++++++++++++++++++++++++++++
>  include/fb_block.h          | 104 +++++++++++++++
>  4 files changed, 438 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index 1eb460f5a02a87089d85b252375202d9acaec9fa..76b35d8419e02a7f1d36988fbdac190b10bff35e 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -92,7 +92,7 @@ config FASTBOOT_USB_DEV
>  config FASTBOOT_FLASH
>  	bool "Enable FASTBOOT FLASH command"
>  	default y if ARCH_SUNXI || ARCH_ROCKCHIP
> -	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
> +	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS) || BLK
>  	select IMAGE_SPARSE
>  	help
>  	  The fastboot protocol includes a "flash" command for writing
> @@ -114,12 +114,16 @@ choice
>  
>  config FASTBOOT_FLASH_MMC
>  	bool "FASTBOOT on MMC"
> -	depends on MMC
> +	depends on MMC && BLK
>  
>  config FASTBOOT_FLASH_NAND
>  	bool "FASTBOOT on NAND"
>  	depends on MTD_RAW_NAND && CMD_MTDPARTS
>  
> +config FASTBOOT_FLASH_BLOCK
> +	bool "FASTBOOT on block device"
> +	depends on BLK
> +
>  endchoice
>  
>  config FASTBOOT_FLASH_MMC_DEV
> @@ -194,6 +198,18 @@ config FASTBOOT_MMC_USER_NAME
>  	  defined here.
>  	  The default target name for erasing EMMC_USER is "mmc0".
>  
> +config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
> +	string "Define FASTBOOT block interface name"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	help
> +	  "Fastboot block interface name (mmc, virtio, etc)"

Maybe use another example in the help mmc. for MMC, we have FASTBOOT_FLASH_MMC.
FASTBOOT_FLASH_MMC has more features than FASTBOOT_FLASH_BLOCK_INTERFACE="mmc".

For example, boot partition labels.

When using this diff:

diff --git a/configs/khadas-vim3_android_defconfig b/configs/khadas-vim3_android_defconfig
index b77a44ce859b..bfe4db90963c 100644
--- a/configs/khadas-vim3_android_defconfig
+++ b/configs/khadas-vim3_android_defconfig
@@ -65,8 +65,9 @@ CONFIG_BUTTON_ADC=y
 CONFIG_USB_FUNCTION_FASTBOOT=y
 CONFIG_FASTBOOT_BUF_ADDR=0x6000000
 CONFIG_FASTBOOT_FLASH=y
-CONFIG_FASTBOOT_FLASH_MMC_DEV=2
-CONFIG_FASTBOOT_CMD_OEM_FORMAT=y
+CONFIG_FASTBOOT_FLASH_BLOCK=y
+CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME="mmc"
+CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID=2
 CONFIG_DM_I2C=y
 CONFIG_SYS_I2C_MESON=y
 CONFIG_MMC_MESON_GX=y

We cannot flash the "bootloader" partition:
$ fastboot flash bootloader u-boot_kvim3_noab.bin
Warning: skip copying bootloader image avb footer (bootloader partition size: 0, bootloader image size: 1285488).
Sending 'bootloader' (1255 KB)                     OKAY [  0.054s]
Writing 'bootloader'                               FAILED (remote: 'failed to get partition info')
fastboot: error: Command failed

If we do want to merge both (at some later time) the above should be supported.

> +
> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
> +	int "Define FASTBOOT block device id"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	help
> +	  "Fastboot block device id"

This is similar as CONFIG_FASTBOOT_FLASH_MMC_DEV, no?

To me, it's confusing to have similar KConfig entries based on interface
type.

If we really want to do it this way, then we should add a safeguard in
the code: test if FASTBOOT_FLASH_BLOCK_INTERFACE_NAME is "mmc" and if so, add a
warning to recommend using FASTBOOT_FLASH_MMC instead.

What's your opinion on this?

> +
>  config FASTBOOT_GPT_NAME
>  	string "Target name for updating GPT"
>  	depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
> diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile
> index 048af5aa823436956142a536c5f7dcf1a8948726..91e98763e8eab84ccd9b8e5354ff1419f61ef372 100644
> --- a/drivers/fastboot/Makefile
> +++ b/drivers/fastboot/Makefile
> @@ -3,5 +3,7 @@
>  obj-y += fb_common.o
>  obj-y += fb_getvar.o
>  obj-y += fb_command.o
> -obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o
> +obj-$(CONFIG_FASTBOOT_FLASH_BLOCK) += fb_block.o
> +# MMC reuses block implementation
> +obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_block.o fb_mmc.o
>  obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o
> diff --git a/drivers/fastboot/fb_block.c b/drivers/fastboot/fb_block.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e79429a9732c1da1ce88358747cf1cb9419cd12b
> --- /dev/null
> +++ b/drivers/fastboot/fb_block.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: BSD-2-Clause

Should this be GPL?

See:
https://docs.u-boot.org/en/latest/develop/sending_patches.html#notes

> +/*
> + * Copyright (C) 2024 The Android Open Source Project
> + */
> +
> +#include <blk.h>
> +#include <div64.h>
> +#include <fastboot-internal.h>
> +#include <fastboot.h>
> +#include <fb_block.h>
> +#include <image-sparse.h>
> +#include <part.h>
> +#include <malloc.h>
> +
> +/**
> + * FASTBOOT_MAX_BLOCKS_ERASE - maximum blocks to erase per derase call
> + *
> + * in the ERASE case we can have much larger buffer size since
> + * we're not transferring an actual buffer
> + */
> +#define FASTBOOT_MAX_BLOCKS_ERASE 1048576
> +/**
> + * FASTBOOT_MAX_BLOCKS_SOFT_ERASE - maximum blocks to software erase at once
> + */
> +#define FASTBOOT_MAX_BLOCKS_SOFT_ERASE 4096
> +/**
> + * FASTBOOT_MAX_BLOCKS_WRITE - maximum blocks to write per dwrite call
> + */
> +#define FASTBOOT_MAX_BLOCKS_WRITE 65536
> +
> +struct fb_block_sparse {
> +	struct blk_desc	*dev_desc;
> +};
> +
> +static lbaint_t fb_block_write(struct blk_desc *block_dev, lbaint_t start,
> +			       lbaint_t blkcnt, const void *buffer)
> +{
> +	lbaint_t blk = start;
> +	lbaint_t blks_written = 0;
> +	lbaint_t cur_blkcnt = 0;
> +	lbaint_t blks = 0;
> +	void *erase_buf = NULL;
> +	int erase_buf_blks = 0;
> +	int step = buffer ? FASTBOOT_MAX_BLOCKS_WRITE : FASTBOOT_MAX_BLOCKS_ERASE;
> +	int i;
> +
> +	for (i = 0; i < blkcnt; i += step) {
> +		cur_blkcnt = min((int)blkcnt - i, step);
> +		if (buffer) {
> +			if (fastboot_progress_callback)
> +				fastboot_progress_callback("writing");
> +			blks_written = blk_dwrite(block_dev, blk, cur_blkcnt,
> +						  buffer + (i * block_dev->blksz));
> +		} else {
> +			if (fastboot_progress_callback)
> +				fastboot_progress_callback("erasing");
> +
> +			if (!erase_buf) {
> +				blks_written = blk_derase(block_dev, blk, cur_blkcnt);
> +
> +				/* Allocate erase buffer if erase is not implemented */
> +				if ((long)blks_written == -ENOSYS) {
> +					erase_buf_blks = min_t(long, blkcnt,
> +							       FASTBOOT_MAX_BLOCKS_SOFT_ERASE);
> +					erase_buf = malloc(erase_buf_blks * block_dev->blksz);
> +					memset(erase_buf, 0, erase_buf_blks * block_dev->blksz);
> +
> +					printf("Slowly writing empty buffers due to missing erase operation\n");
> +				}
> +			}
> +
> +			/* Write 0s instead of using erase operation, inefficient but functional */
> +			if (erase_buf) {
> +				int j;
> +
> +				blks_written = 0;
> +				for (j = 0; j < cur_blkcnt; j += erase_buf_blks) {
> +					lbaint_t remain = min_t(lbaint_t, cur_blkcnt - j,
> +								erase_buf_blks);
> +
> +					blks_written += blk_dwrite(block_dev, blk + j,
> +								   remain, erase_buf);
> +					printf(".");

Can we move soft erase to a separate function? Here we already handle
normal block write and normal block erase. Having software erase as well
makes the function a bit too long/nested in my opinion.

I't also avoid writing things like:

    blks_written = blk_derase(block_dev, blk, cur_blkcnt);

Where blks_written actually means "amount of blocks that were erased".

Or maybe split write/erase to make each function do only one thing.

> +				}
> +			}
> +		}
> +		blk += blks_written;
> +		blks += blks_written;
> +	}
> +
> +	if (erase_buf)
> +		free(erase_buf);
> +
> +	return blks;
> +}
> +
> +static lbaint_t fb_block_sparse_write(struct sparse_storage *info,
> +				      lbaint_t blk, lbaint_t blkcnt,
> +				      const void *buffer)
> +{
> +	struct fb_block_sparse *sparse = info->priv;
> +	struct blk_desc *dev_desc = sparse->dev_desc;
> +
> +	return fb_block_write(dev_desc, blk, blkcnt, buffer);
> +}
> +
> +static lbaint_t fb_block_sparse_reserve(struct sparse_storage *info,
> +					lbaint_t blk, lbaint_t blkcnt)
> +{
> +	return blkcnt;
> +}
> +
> +int fastboot_block_get_part_info(const char *part_name,
> +				 struct blk_desc **dev_desc,
> +				 struct disk_partition *part_info,
> +				 char *response)
> +{
> +	int ret;
> +	const char *interface = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
> +						   CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME,
> +						   NULL);
> +	const int device = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
> +					      CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID, -1);
> +
> +	if (!part_name || !strcmp(part_name, "")) {
> +		fastboot_fail("partition not given", response);
> +		return -ENOENT;
> +	}
> +	if (!interface || !strcmp(interface, "")) {
> +		fastboot_fail("block interface isn't provided", response);
> +		return -EINVAL;
> +	}
> +
> +	*dev_desc = blk_get_dev(interface, device);
> +	if (!dev_desc) {
> +		fastboot_fail("no such device", response);
> +		return -ENODEV;
> +	}
> +
> +	ret = part_get_info_by_name(*dev_desc, part_name, part_info);
> +	if (ret < 0)
> +		fastboot_fail("failed to get partition info", response);
> +
> +	return ret;
> +}
> +
> +void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
> +				   char *response)
> +{
> +	lbaint_t written;
> +
> +	debug("Start Erasing %s...\n", disk_name);
> +
> +	written = fb_block_write(dev_desc, 0, dev_desc->lba, NULL);
> +	if (written != dev_desc->lba) {
> +		pr_err("Failed to erase %s\n", disk_name);
> +		fastboot_response("FAIL", response, "Failed to erase %s", disk_name);
> +		return;
> +	}
> +
> +	printf("........ erased " LBAFU " bytes from '%s'\n",
> +	       dev_desc->lba * dev_desc->blksz, disk_name);
> +	fastboot_okay(NULL, response);
> +}
> +
> +void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
> +			      const char *part_name, uint alignment, char *response)
> +{
> +	lbaint_t written, blks_start, blks_size;
> +
> +	if (alignment) {
> +		blks_start = (info->start + alignment - 1) & ~(alignment - 1);
> +		if (info->size >= alignment)
> +			blks_size = (info->size - (blks_start - info->start)) &
> +				(~(alignment - 1));
> +		else
> +			blks_size = 0;
> +
> +		printf("Erasing blocks " LBAFU " to " LBAFU " due to alignment\n",
> +		       blks_start, blks_start + blks_size);
> +	} else {
> +		blks_start = info->start;
> +		blks_size = info->size;
> +	}
> +
> +	written = fb_block_write(dev_desc, blks_start, blks_size, NULL);
> +	if (written != blks_size) {
> +		fastboot_fail("failed to erase partition", response);
> +		return;
> +	}
> +
> +	printf("........ erased " LBAFU " bytes from '%s'\n",
> +	       blks_size * info->blksz, part_name);
> +	fastboot_okay(NULL, response);
> +}
> +
> +void fastboot_block_erase(const char *part_name, char *response)
> +{
> +	struct blk_desc *dev_desc;
> +	struct disk_partition part_info;
> +
> +	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
> +		return;
> +
> +	fastboot_block_raw_erase(dev_desc, &part_info, part_name, 0, response);
> +}
> +
> +void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
> +				   void *buffer, u32 download_bytes, char *response)
> +{
> +	lbaint_t blkcnt;
> +	lbaint_t blks;
> +
> +	/* determine number of blocks to write */
> +	blkcnt = ((download_bytes + (dev_desc->blksz - 1)) & ~(dev_desc->blksz - 1));
> +	blkcnt = lldiv(blkcnt, dev_desc->blksz);
> +
> +	if (blkcnt > dev_desc->lba) {
> +		pr_err("too large for disk: '%s'\n", disk_name);
> +		fastboot_fail("too large for disk", response);
> +		return;
> +	}
> +
> +	puts("Flashing Raw Image\n");

Can't we use printf() here for consistency in the code?

> +
> +	blks = fb_block_write(dev_desc, 0, blkcnt, buffer);
> +
> +	if (blks != blkcnt) {
> +		pr_err("failed writing to %s\n", disk_name);
> +		fastboot_fail("failed writing to device", response);
> +		return;
> +	}
> +
> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * dev_desc->blksz,
> +	       disk_name);
> +	fastboot_okay(NULL, response);
> +}
> +
> +void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
> +				    struct disk_partition *info, const char *part_name,
> +				    void *buffer, u32 download_bytes, char *response)
> +{
> +	lbaint_t blkcnt;
> +	lbaint_t blks;
> +
> +	/* determine number of blocks to write */
> +	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
> +	blkcnt = lldiv(blkcnt, info->blksz);
> +
> +	if (blkcnt > info->size) {
> +		pr_err("too large for partition: '%s'\n", part_name);
> +		fastboot_fail("too large for partition", response);
> +		return;
> +	}
> +
> +	puts("Flashing Raw Image\n");

Can't we use printf() here for consistency in the code?

> +
> +	blks = fb_block_write(dev_desc, info->start, blkcnt, buffer);
> +
> +	if (blks != blkcnt) {
> +		pr_err("failed writing to device %d\n", dev_desc->devnum);
> +		fastboot_fail("failed writing to device", response);
> +		return;
> +	}
> +
> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
> +	       part_name);
> +	fastboot_okay(NULL, response);
> +}
> +
> +void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
> +				       const char *part_name, void *buffer, char *response)
> +{
> +	struct fb_block_sparse sparse_priv;
> +	struct sparse_storage sparse;
> +	int err;
> +
> +	sparse_priv.dev_desc = dev_desc;
> +
> +	sparse.blksz = info->blksz;
> +	sparse.start = info->start;
> +	sparse.size = info->size;
> +	sparse.write = fb_block_sparse_write;
> +	sparse.reserve = fb_block_sparse_reserve;
> +	sparse.mssg = fastboot_fail;
> +
> +	printf("Flashing sparse image at offset " LBAFU "\n",
> +	       sparse.start);
> +
> +	sparse.priv = &sparse_priv;
> +	err = write_sparse_image(&sparse, part_name, buffer,
> +				 response);
> +	if (!err)
> +		fastboot_okay(NULL, response);
> +}
> +
> +void fastboot_block_flash_write(const char *part_name, void *download_buffer,
> +				u32 download_bytes, char *response)
> +{
> +	struct blk_desc *dev_desc;
> +	struct disk_partition part_info;
> +
> +	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
> +		return;
> +
> +	if (is_sparse_image(download_buffer)) {
> +		fastboot_block_write_sparse_image(dev_desc, &part_info, part_name,
> +						  download_buffer, response);
> +	} else {
> +		fastboot_block_write_raw_image(dev_desc, &part_info, part_name,
> +					       download_buffer, download_bytes, response);
> +	}
> +}
> diff --git a/include/fb_block.h b/include/fb_block.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..cfc824214acb515a0cc2f13fd786c606c6141489
> --- /dev/null
> +++ b/include/fb_block.h
> @@ -0,0 +1,104 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */

Same here for the licence.


> +/*
> + * Copyright (C) 2024 The Android Open Source Project
> + */
> +
> +#ifndef _FB_BLOCK_H_
> +#define _FB_BLOCK_H_
> +
> +struct blk_desc;
> +struct disk_partition;
> +
> +/**
> + * fastboot_block_get_part_info() - Lookup block partition by name
> + *
> + * @part_name: Named partition to lookup
> + * @dev_desc: Pointer to returned blk_desc pointer
> + * @part_info: Pointer to returned struct disk_partition
> + * @response: Pointer to fastboot response buffer

The doc is missing a Return: section where we specify the return code.

> + */
> +int fastboot_block_get_part_info(const char *part_name,
> +				 struct blk_desc **dev_desc,
> +				 struct disk_partition *part_info,
> +				 char *response);
> +
> +/**
> + * fastboot_block_raw_erase() - Erase raw block device partition
> + *
> + * @dev_desc: Block device we're going write to
> + * @info: Partition we're going write to
> + * @part_name: Name of partition we're going write to
> + * @alignment: erase start and size alignment, specify 0 to ignore
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
> +			      const char *part_name, uint alignment, char *response);
> +
> +/**
> + * fastboot_block_raw_erase_disk() - Erase raw block device
> + *
> + * @dev_desc: Block device we're going write to
> + * @disk_name: Name of disk we're going write to
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
> +				   char *response);
> +
> +/**
> + * fastboot_block_erase() - Erase partition on block device for fastboot
> + *
> + * @part_name: Named partition to erase
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_erase(const char *part_name, char *response);
> +
> +/**
> + * fastboot_block_write_raw_disk() - Write raw image to block device
> + *
> + * @dev_desc: Block device we're going write to
> + * @disk_name: Name of disk we're going write to
> + * @buffer: Downloaded buffer pointer
> + * @download_bytes: Size of content on downloaded buffer pointer
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
> +				   void *buffer, u32 download_bytes, char *response);
> +
> +/**
> + * fastboot_block_write_raw_image() - Write raw image to block device partition
> + *
> + * @dev_desc: Block device we're going write to
> + * @info: Partition we're going write to
> + * @part_name: Name of partition we're going write to
> + * @buffer: Downloaded buffer pointer
> + * @download_bytes: Size of content on downloaded buffer pointer
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
> +				    struct disk_partition *info, const char *part_name,
> +				    void *buffer, u32 download_bytes, char *response);
> +
> +/**
> + * fastboot_block_write_sparse_image() - Write sparse image to block device partition
> + *
> + * @dev_desc: Block device we're going write to
> + * @info: Partition we're going write to
> + * @part_name: Name of partition we're going write to
> + * @buffer: Downloaded buffer pointer
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
> +				       const char *part_name, void *buffer, char *response);
> +
> +/**
> + * fastboot_block_flash_write() - Write image to block device for fastboot
> + *
> + * @part_name: Named partition to write image to
> + * @download_buffer: Pointer to image data
> + * @download_bytes: Size of image data
> + * @response: Pointer to fastboot response buffer
> + */
> +void fastboot_block_flash_write(const char *part_name, void *download_buffer,
> +				u32 download_bytes, char *response);
> +
> +#endif // _FB_BLOCK_H_
>
> -- 
> 2.34.1

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[Neil Armstrong]

Hi,

On 22/04/2025 15:17, Mattijs Korpershoek wrote:
> Hi Neil,
> 
> Thank you for the patch.

Thx for the review

> 
> On mer., avril 09, 2025 at 09:58, neil.armstrong@linaro.org wrote:
> 
>> From: Dmitrii Merkurev <dimorinny@google.com>
>>
>> Introduce fastboot block flashing functions and helpers
>> to be shared with the MMC implementation.
>>
>> The write logic comes from the mmc implementation, while
>> the partition lookup is much simpler and could be extended.
>>
>> For the erase logic, allmost no block drivers exposes the
>> erase operation, except mmc & virtio, so in order to allow
>> erasiong any partition a soft-erase logic has been added
>> to write zero-ed buffers in a loop.
>>
>> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/fastboot/Kconfig    |  20 ++-
>>   drivers/fastboot/Makefile   |   4 +-
>>   drivers/fastboot/fb_block.c | 313 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/fb_block.h          | 104 +++++++++++++++
>>   4 files changed, 438 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>> index 1eb460f5a02a87089d85b252375202d9acaec9fa..76b35d8419e02a7f1d36988fbdac190b10bff35e 100644
>> --- a/drivers/fastboot/Kconfig
>> +++ b/drivers/fastboot/Kconfig
>> @@ -92,7 +92,7 @@ config FASTBOOT_USB_DEV
>>   config FASTBOOT_FLASH
>>   	bool "Enable FASTBOOT FLASH command"
>>   	default y if ARCH_SUNXI || ARCH_ROCKCHIP
>> -	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
>> +	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS) || BLK
>>   	select IMAGE_SPARSE
>>   	help
>>   	  The fastboot protocol includes a "flash" command for writing
>> @@ -114,12 +114,16 @@ choice
>>   
>>   config FASTBOOT_FLASH_MMC
>>   	bool "FASTBOOT on MMC"
>> -	depends on MMC
>> +	depends on MMC && BLK
>>   
>>   config FASTBOOT_FLASH_NAND
>>   	bool "FASTBOOT on NAND"
>>   	depends on MTD_RAW_NAND && CMD_MTDPARTS
>>   
>> +config FASTBOOT_FLASH_BLOCK
>> +	bool "FASTBOOT on block device"
>> +	depends on BLK
>> +
>>   endchoice
>>   
>>   config FASTBOOT_FLASH_MMC_DEV
>> @@ -194,6 +198,18 @@ config FASTBOOT_MMC_USER_NAME
>>   	  defined here.
>>   	  The default target name for erasing EMMC_USER is "mmc0".
>>   
>> +config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
>> +	string "Define FASTBOOT block interface name"
>> +	depends on FASTBOOT_FLASH_BLOCK
>> +	help
>> +	  "Fastboot block interface name (mmc, virtio, etc)"
> 
> Maybe use another example in the help mmc. for MMC, we have FASTBOOT_FLASH_MMC.
> FASTBOOT_FLASH_MMC has more features than FASTBOOT_FLASH_BLOCK_INTERFACE="mmc".
> 
> For example, boot partition labels.
> 
> When using this diff:
> 
> diff --git a/configs/khadas-vim3_android_defconfig b/configs/khadas-vim3_android_defconfig
> index b77a44ce859b..bfe4db90963c 100644
> --- a/configs/khadas-vim3_android_defconfig
> +++ b/configs/khadas-vim3_android_defconfig
> @@ -65,8 +65,9 @@ CONFIG_BUTTON_ADC=y
>   CONFIG_USB_FUNCTION_FASTBOOT=y
>   CONFIG_FASTBOOT_BUF_ADDR=0x6000000
>   CONFIG_FASTBOOT_FLASH=y
> -CONFIG_FASTBOOT_FLASH_MMC_DEV=2
> -CONFIG_FASTBOOT_CMD_OEM_FORMAT=y
> +CONFIG_FASTBOOT_FLASH_BLOCK=y
> +CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME="mmc"
> +CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID=2
>   CONFIG_DM_I2C=y
>   CONFIG_SYS_I2C_MESON=y
>   CONFIG_MMC_MESON_GX=y
> 
> We cannot flash the "bootloader" partition:
> $ fastboot flash bootloader u-boot_kvim3_noab.bin
> Warning: skip copying bootloader image avb footer (bootloader partition size: 0, bootloader image size: 1285488).
> Sending 'bootloader' (1255 KB)                     OKAY [  0.054s]
> Writing 'bootloader'                               FAILED (remote: 'failed to get partition info')
> fastboot: error: Command failed
> 
> If we do want to merge both (at some later time) the above should be supported.

Right, I don't think we want to merge them, and boot partitions are an MMC specific feature,
I don't think any other storages has those specific hardware boot partitions.

In any case, we can still just name the boot partition "bootloader" and raw flash it.

> 
>> +
>> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
>> +	int "Define FASTBOOT block device id"
>> +	depends on FASTBOOT_FLASH_BLOCK
>> +	help
>> +	  "Fastboot block device id"
> 
> This is similar as CONFIG_FASTBOOT_FLASH_MMC_DEV, no?

No, the MMC one makes the mmc type implicit, while the block one
can target any block device (and mmc)

> 
> To me, it's confusing to have similar KConfig entries based on interface
> type.
> 
> If we really want to do it this way, then we should add a safeguard in
> the code: test if FASTBOOT_FLASH_BLOCK_INTERFACE_NAME is "mmc" and if so, add a
> warning to recommend using FASTBOOT_FLASH_MMC instead.
> 
> What's your opinion on this?

Perhaps, not sure it's really problematic

> 
>> +
>>   config FASTBOOT_GPT_NAME
>>   	string "Target name for updating GPT"
>>   	depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
>> diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile
>> index 048af5aa823436956142a536c5f7dcf1a8948726..91e98763e8eab84ccd9b8e5354ff1419f61ef372 100644
>> --- a/drivers/fastboot/Makefile
>> +++ b/drivers/fastboot/Makefile
>> @@ -3,5 +3,7 @@
>>   obj-y += fb_common.o
>>   obj-y += fb_getvar.o
>>   obj-y += fb_command.o
>> -obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o
>> +obj-$(CONFIG_FASTBOOT_FLASH_BLOCK) += fb_block.o
>> +# MMC reuses block implementation
>> +obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_block.o fb_mmc.o
>>   obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o
>> diff --git a/drivers/fastboot/fb_block.c b/drivers/fastboot/fb_block.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e79429a9732c1da1ce88358747cf1cb9419cd12b
>> --- /dev/null
>> +++ b/drivers/fastboot/fb_block.c
>> @@ -0,0 +1,313 @@
>> +// SPDX-License-Identifier: BSD-2-Clause
> 
> Should this be GPL?

Very good qustion, I'm not the copyright holder, the original author is Dmitrii, not sure about
the legal things here.

Tom can you help ?

> 
> See:
> https://docs.u-boot.org/en/latest/develop/sending_patches.html#notes
> 
>> +/*
>> + * Copyright (C) 2024 The Android Open Source Project
>> + */
>> +
>> +#include <blk.h>
>> +#include <div64.h>
>> +#include <fastboot-internal.h>
>> +#include <fastboot.h>
>> +#include <fb_block.h>
>> +#include <image-sparse.h>
>> +#include <part.h>
>> +#include <malloc.h>
>> +
>> +/**
>> + * FASTBOOT_MAX_BLOCKS_ERASE - maximum blocks to erase per derase call
>> + *
>> + * in the ERASE case we can have much larger buffer size since
>> + * we're not transferring an actual buffer
>> + */
>> +#define FASTBOOT_MAX_BLOCKS_ERASE 1048576
>> +/**
>> + * FASTBOOT_MAX_BLOCKS_SOFT_ERASE - maximum blocks to software erase at once
>> + */
>> +#define FASTBOOT_MAX_BLOCKS_SOFT_ERASE 4096
>> +/**
>> + * FASTBOOT_MAX_BLOCKS_WRITE - maximum blocks to write per dwrite call
>> + */
>> +#define FASTBOOT_MAX_BLOCKS_WRITE 65536
>> +
>> +struct fb_block_sparse {
>> +	struct blk_desc	*dev_desc;
>> +};
>> +
>> +static lbaint_t fb_block_write(struct blk_desc *block_dev, lbaint_t start,
>> +			       lbaint_t blkcnt, const void *buffer)
>> +{
>> +	lbaint_t blk = start;
>> +	lbaint_t blks_written = 0;
>> +	lbaint_t cur_blkcnt = 0;
>> +	lbaint_t blks = 0;
>> +	void *erase_buf = NULL;
>> +	int erase_buf_blks = 0;
>> +	int step = buffer ? FASTBOOT_MAX_BLOCKS_WRITE : FASTBOOT_MAX_BLOCKS_ERASE;
>> +	int i;
>> +
>> +	for (i = 0; i < blkcnt; i += step) {
>> +		cur_blkcnt = min((int)blkcnt - i, step);
>> +		if (buffer) {
>> +			if (fastboot_progress_callback)
>> +				fastboot_progress_callback("writing");
>> +			blks_written = blk_dwrite(block_dev, blk, cur_blkcnt,
>> +						  buffer + (i * block_dev->blksz));
>> +		} else {
>> +			if (fastboot_progress_callback)
>> +				fastboot_progress_callback("erasing");
>> +
>> +			if (!erase_buf) {
>> +				blks_written = blk_derase(block_dev, blk, cur_blkcnt);
>> +
>> +				/* Allocate erase buffer if erase is not implemented */
>> +				if ((long)blks_written == -ENOSYS) {
>> +					erase_buf_blks = min_t(long, blkcnt,
>> +							       FASTBOOT_MAX_BLOCKS_SOFT_ERASE);
>> +					erase_buf = malloc(erase_buf_blks * block_dev->blksz);
>> +					memset(erase_buf, 0, erase_buf_blks * block_dev->blksz);
>> +
>> +					printf("Slowly writing empty buffers due to missing erase operation\n");
>> +				}
>> +			}
>> +
>> +			/* Write 0s instead of using erase operation, inefficient but functional */
>> +			if (erase_buf) {
>> +				int j;
>> +
>> +				blks_written = 0;
>> +				for (j = 0; j < cur_blkcnt; j += erase_buf_blks) {
>> +					lbaint_t remain = min_t(lbaint_t, cur_blkcnt - j,
>> +								erase_buf_blks);
>> +
>> +					blks_written += blk_dwrite(block_dev, blk + j,
>> +								   remain, erase_buf);
>> +					printf(".");
> 
> Can we move soft erase to a separate function? Here we already handle
> normal block write and normal block erase. Having software erase as well
> makes the function a bit too long/nested in my opinion.
> 
> I't also avoid writing things like:
> 
>      blks_written = blk_derase(block_dev, blk, cur_blkcnt);
> 
> Where blks_written actually means "amount of blocks that were erased".
> 
> Or maybe split write/erase to make each function do only one thing.

Sure, I'll move it to a separate function

> 
>> +				}
>> +			}
>> +		}
>> +		blk += blks_written;
>> +		blks += blks_written;
>> +	}
>> +
>> +	if (erase_buf)
>> +		free(erase_buf);
>> +
>> +	return blks;
>> +}
>> +
>> +static lbaint_t fb_block_sparse_write(struct sparse_storage *info,
>> +				      lbaint_t blk, lbaint_t blkcnt,
>> +				      const void *buffer)
>> +{
>> +	struct fb_block_sparse *sparse = info->priv;
>> +	struct blk_desc *dev_desc = sparse->dev_desc;
>> +
>> +	return fb_block_write(dev_desc, blk, blkcnt, buffer);
>> +}
>> +
>> +static lbaint_t fb_block_sparse_reserve(struct sparse_storage *info,
>> +					lbaint_t blk, lbaint_t blkcnt)
>> +{
>> +	return blkcnt;
>> +}
>> +
>> +int fastboot_block_get_part_info(const char *part_name,
>> +				 struct blk_desc **dev_desc,
>> +				 struct disk_partition *part_info,
>> +				 char *response)
>> +{
>> +	int ret;
>> +	const char *interface = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
>> +						   CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME,
>> +						   NULL);
>> +	const int device = config_opt_enabled(CONFIG_FASTBOOT_FLASH_BLOCK,
>> +					      CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID, -1);
>> +
>> +	if (!part_name || !strcmp(part_name, "")) {
>> +		fastboot_fail("partition not given", response);
>> +		return -ENOENT;
>> +	}
>> +	if (!interface || !strcmp(interface, "")) {
>> +		fastboot_fail("block interface isn't provided", response);
>> +		return -EINVAL;
>> +	}
>> +
>> +	*dev_desc = blk_get_dev(interface, device);
>> +	if (!dev_desc) {
>> +		fastboot_fail("no such device", response);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = part_get_info_by_name(*dev_desc, part_name, part_info);
>> +	if (ret < 0)
>> +		fastboot_fail("failed to get partition info", response);
>> +
>> +	return ret;
>> +}
>> +
>> +void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
>> +				   char *response)
>> +{
>> +	lbaint_t written;
>> +
>> +	debug("Start Erasing %s...\n", disk_name);
>> +
>> +	written = fb_block_write(dev_desc, 0, dev_desc->lba, NULL);
>> +	if (written != dev_desc->lba) {
>> +		pr_err("Failed to erase %s\n", disk_name);
>> +		fastboot_response("FAIL", response, "Failed to erase %s", disk_name);
>> +		return;
>> +	}
>> +
>> +	printf("........ erased " LBAFU " bytes from '%s'\n",
>> +	       dev_desc->lba * dev_desc->blksz, disk_name);
>> +	fastboot_okay(NULL, response);
>> +}
>> +
>> +void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
>> +			      const char *part_name, uint alignment, char *response)
>> +{
>> +	lbaint_t written, blks_start, blks_size;
>> +
>> +	if (alignment) {
>> +		blks_start = (info->start + alignment - 1) & ~(alignment - 1);
>> +		if (info->size >= alignment)
>> +			blks_size = (info->size - (blks_start - info->start)) &
>> +				(~(alignment - 1));
>> +		else
>> +			blks_size = 0;
>> +
>> +		printf("Erasing blocks " LBAFU " to " LBAFU " due to alignment\n",
>> +		       blks_start, blks_start + blks_size);
>> +	} else {
>> +		blks_start = info->start;
>> +		blks_size = info->size;
>> +	}
>> +
>> +	written = fb_block_write(dev_desc, blks_start, blks_size, NULL);
>> +	if (written != blks_size) {
>> +		fastboot_fail("failed to erase partition", response);
>> +		return;
>> +	}
>> +
>> +	printf("........ erased " LBAFU " bytes from '%s'\n",
>> +	       blks_size * info->blksz, part_name);
>> +	fastboot_okay(NULL, response);
>> +}
>> +
>> +void fastboot_block_erase(const char *part_name, char *response)
>> +{
>> +	struct blk_desc *dev_desc;
>> +	struct disk_partition part_info;
>> +
>> +	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
>> +		return;
>> +
>> +	fastboot_block_raw_erase(dev_desc, &part_info, part_name, 0, response);
>> +}
>> +
>> +void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
>> +				   void *buffer, u32 download_bytes, char *response)
>> +{
>> +	lbaint_t blkcnt;
>> +	lbaint_t blks;
>> +
>> +	/* determine number of blocks to write */
>> +	blkcnt = ((download_bytes + (dev_desc->blksz - 1)) & ~(dev_desc->blksz - 1));
>> +	blkcnt = lldiv(blkcnt, dev_desc->blksz);
>> +
>> +	if (blkcnt > dev_desc->lba) {
>> +		pr_err("too large for disk: '%s'\n", disk_name);
>> +		fastboot_fail("too large for disk", response);
>> +		return;
>> +	}
>> +
>> +	puts("Flashing Raw Image\n");
> 
> Can't we use printf() here for consistency in the code?

Ack

> 
>> +
>> +	blks = fb_block_write(dev_desc, 0, blkcnt, buffer);
>> +
>> +	if (blks != blkcnt) {
>> +		pr_err("failed writing to %s\n", disk_name);
>> +		fastboot_fail("failed writing to device", response);
>> +		return;
>> +	}
>> +
>> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * dev_desc->blksz,
>> +	       disk_name);
>> +	fastboot_okay(NULL, response);
>> +}
>> +
>> +void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
>> +				    struct disk_partition *info, const char *part_name,
>> +				    void *buffer, u32 download_bytes, char *response)
>> +{
>> +	lbaint_t blkcnt;
>> +	lbaint_t blks;
>> +
>> +	/* determine number of blocks to write */
>> +	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
>> +	blkcnt = lldiv(blkcnt, info->blksz);
>> +
>> +	if (blkcnt > info->size) {
>> +		pr_err("too large for partition: '%s'\n", part_name);
>> +		fastboot_fail("too large for partition", response);
>> +		return;
>> +	}
>> +
>> +	puts("Flashing Raw Image\n");
> 
> Can't we use printf() here for consistency in the code?
> 
>> +
>> +	blks = fb_block_write(dev_desc, info->start, blkcnt, buffer);
>> +
>> +	if (blks != blkcnt) {
>> +		pr_err("failed writing to device %d\n", dev_desc->devnum);
>> +		fastboot_fail("failed writing to device", response);
>> +		return;
>> +	}
>> +
>> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
>> +	       part_name);
>> +	fastboot_okay(NULL, response);
>> +}
>> +
>> +void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
>> +				       const char *part_name, void *buffer, char *response)
>> +{
>> +	struct fb_block_sparse sparse_priv;
>> +	struct sparse_storage sparse;
>> +	int err;
>> +
>> +	sparse_priv.dev_desc = dev_desc;
>> +
>> +	sparse.blksz = info->blksz;
>> +	sparse.start = info->start;
>> +	sparse.size = info->size;
>> +	sparse.write = fb_block_sparse_write;
>> +	sparse.reserve = fb_block_sparse_reserve;
>> +	sparse.mssg = fastboot_fail;
>> +
>> +	printf("Flashing sparse image at offset " LBAFU "\n",
>> +	       sparse.start);
>> +
>> +	sparse.priv = &sparse_priv;
>> +	err = write_sparse_image(&sparse, part_name, buffer,
>> +				 response);
>> +	if (!err)
>> +		fastboot_okay(NULL, response);
>> +}
>> +
>> +void fastboot_block_flash_write(const char *part_name, void *download_buffer,
>> +				u32 download_bytes, char *response)
>> +{
>> +	struct blk_desc *dev_desc;
>> +	struct disk_partition part_info;
>> +
>> +	if (fastboot_block_get_part_info(part_name, &dev_desc, &part_info, response) < 0)
>> +		return;
>> +
>> +	if (is_sparse_image(download_buffer)) {
>> +		fastboot_block_write_sparse_image(dev_desc, &part_info, part_name,
>> +						  download_buffer, response);
>> +	} else {
>> +		fastboot_block_write_raw_image(dev_desc, &part_info, part_name,
>> +					       download_buffer, download_bytes, response);
>> +	}
>> +}
>> diff --git a/include/fb_block.h b/include/fb_block.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..cfc824214acb515a0cc2f13fd786c606c6141489
>> --- /dev/null
>> +++ b/include/fb_block.h
>> @@ -0,0 +1,104 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
> 
> Same here for the licence.
> 
> 
>> +/*
>> + * Copyright (C) 2024 The Android Open Source Project
>> + */
>> +
>> +#ifndef _FB_BLOCK_H_
>> +#define _FB_BLOCK_H_
>> +
>> +struct blk_desc;
>> +struct disk_partition;
>> +
>> +/**
>> + * fastboot_block_get_part_info() - Lookup block partition by name
>> + *
>> + * @part_name: Named partition to lookup
>> + * @dev_desc: Pointer to returned blk_desc pointer
>> + * @part_info: Pointer to returned struct disk_partition
>> + * @response: Pointer to fastboot response buffer
> 
> The doc is missing a Return: section where we specify the return code.


Ack

> 
>> + */
>> +int fastboot_block_get_part_info(const char *part_name,
>> +				 struct blk_desc **dev_desc,
>> +				 struct disk_partition *part_info,
>> +				 char *response);
>> +
>> +/**
>> + * fastboot_block_raw_erase() - Erase raw block device partition
>> + *
>> + * @dev_desc: Block device we're going write to
>> + * @info: Partition we're going write to
>> + * @part_name: Name of partition we're going write to
>> + * @alignment: erase start and size alignment, specify 0 to ignore
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_raw_erase(struct blk_desc *dev_desc, struct disk_partition *info,
>> +			      const char *part_name, uint alignment, char *response);
>> +
>> +/**
>> + * fastboot_block_raw_erase_disk() - Erase raw block device
>> + *
>> + * @dev_desc: Block device we're going write to
>> + * @disk_name: Name of disk we're going write to
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_raw_erase_disk(struct blk_desc *dev_desc, const char *disk_name,
>> +				   char *response);
>> +
>> +/**
>> + * fastboot_block_erase() - Erase partition on block device for fastboot
>> + *
>> + * @part_name: Named partition to erase
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_erase(const char *part_name, char *response);
>> +
>> +/**
>> + * fastboot_block_write_raw_disk() - Write raw image to block device
>> + *
>> + * @dev_desc: Block device we're going write to
>> + * @disk_name: Name of disk we're going write to
>> + * @buffer: Downloaded buffer pointer
>> + * @download_bytes: Size of content on downloaded buffer pointer
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_write_raw_disk(struct blk_desc *dev_desc, const char *disk_name,
>> +				   void *buffer, u32 download_bytes, char *response);
>> +
>> +/**
>> + * fastboot_block_write_raw_image() - Write raw image to block device partition
>> + *
>> + * @dev_desc: Block device we're going write to
>> + * @info: Partition we're going write to
>> + * @part_name: Name of partition we're going write to
>> + * @buffer: Downloaded buffer pointer
>> + * @download_bytes: Size of content on downloaded buffer pointer
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_write_raw_image(struct blk_desc *dev_desc,
>> +				    struct disk_partition *info, const char *part_name,
>> +				    void *buffer, u32 download_bytes, char *response);
>> +
>> +/**
>> + * fastboot_block_write_sparse_image() - Write sparse image to block device partition
>> + *
>> + * @dev_desc: Block device we're going write to
>> + * @info: Partition we're going write to
>> + * @part_name: Name of partition we're going write to
>> + * @buffer: Downloaded buffer pointer
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_write_sparse_image(struct blk_desc *dev_desc, struct disk_partition *info,
>> +				       const char *part_name, void *buffer, char *response);
>> +
>> +/**
>> + * fastboot_block_flash_write() - Write image to block device for fastboot
>> + *
>> + * @part_name: Named partition to write image to
>> + * @download_buffer: Pointer to image data
>> + * @download_bytes: Size of image data
>> + * @response: Pointer to fastboot response buffer
>> + */
>> +void fastboot_block_flash_write(const char *part_name, void *download_buffer,
>> +				u32 download_bytes, char *response);
>> +
>> +#endif // _FB_BLOCK_H_
>>
>> -- 
>> 2.34.1

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[Mattijs Korpershoek]

On mer., avril 23, 2025 at 09:38, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> Hi,
>
> On 22/04/2025 15:17, Mattijs Korpershoek wrote:
>> Hi Neil,
>> 
>> Thank you for the patch.
>
> Thx for the review
>
>> 
>> On mer., avril 09, 2025 at 09:58, neil.armstrong@linaro.org wrote:
>> 
>>> From: Dmitrii Merkurev <dimorinny@google.com>
>>>
>>> Introduce fastboot block flashing functions and helpers
>>> to be shared with the MMC implementation.
>>>
>>> The write logic comes from the mmc implementation, while
>>> the partition lookup is much simpler and could be extended.
>>>
>>> For the erase logic, allmost no block drivers exposes the
>>> erase operation, except mmc & virtio, so in order to allow
>>> erasiong any partition a soft-erase logic has been added
>>> to write zero-ed buffers in a loop.
>>>
>>> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/fastboot/Kconfig    |  20 ++-
>>>   drivers/fastboot/Makefile   |   4 +-
>>>   drivers/fastboot/fb_block.c | 313 ++++++++++++++++++++++++++++++++++++++++++++
>>>   include/fb_block.h          | 104 +++++++++++++++
>>>   4 files changed, 438 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>>> index 1eb460f5a02a87089d85b252375202d9acaec9fa..76b35d8419e02a7f1d36988fbdac190b10bff35e 100644
>>> --- a/drivers/fastboot/Kconfig
>>> +++ b/drivers/fastboot/Kconfig
>>> @@ -92,7 +92,7 @@ config FASTBOOT_USB_DEV
>>>   config FASTBOOT_FLASH
>>>   	bool "Enable FASTBOOT FLASH command"
>>>   	default y if ARCH_SUNXI || ARCH_ROCKCHIP
>>> -	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
>>> +	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS) || BLK
>>>   	select IMAGE_SPARSE
>>>   	help
>>>   	  The fastboot protocol includes a "flash" command for writing
>>> @@ -114,12 +114,16 @@ choice
>>>   
>>>   config FASTBOOT_FLASH_MMC
>>>   	bool "FASTBOOT on MMC"
>>> -	depends on MMC
>>> +	depends on MMC && BLK
>>>   
>>>   config FASTBOOT_FLASH_NAND
>>>   	bool "FASTBOOT on NAND"
>>>   	depends on MTD_RAW_NAND && CMD_MTDPARTS
>>>   
>>> +config FASTBOOT_FLASH_BLOCK
>>> +	bool "FASTBOOT on block device"
>>> +	depends on BLK
>>> +
>>>   endchoice
>>>   
>>>   config FASTBOOT_FLASH_MMC_DEV
>>> @@ -194,6 +198,18 @@ config FASTBOOT_MMC_USER_NAME
>>>   	  defined here.
>>>   	  The default target name for erasing EMMC_USER is "mmc0".
>>>   
>>> +config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
>>> +	string "Define FASTBOOT block interface name"
>>> +	depends on FASTBOOT_FLASH_BLOCK
>>> +	help
>>> +	  "Fastboot block interface name (mmc, virtio, etc)"
>> 
>> Maybe use another example in the help mmc. for MMC, we have FASTBOOT_FLASH_MMC.
>> FASTBOOT_FLASH_MMC has more features than FASTBOOT_FLASH_BLOCK_INTERFACE="mmc".
>> 
>> For example, boot partition labels.
>> 
>> When using this diff:
>> 
>> diff --git a/configs/khadas-vim3_android_defconfig b/configs/khadas-vim3_android_defconfig
>> index b77a44ce859b..bfe4db90963c 100644
>> --- a/configs/khadas-vim3_android_defconfig
>> +++ b/configs/khadas-vim3_android_defconfig
>> @@ -65,8 +65,9 @@ CONFIG_BUTTON_ADC=y
>>   CONFIG_USB_FUNCTION_FASTBOOT=y
>>   CONFIG_FASTBOOT_BUF_ADDR=0x6000000
>>   CONFIG_FASTBOOT_FLASH=y
>> -CONFIG_FASTBOOT_FLASH_MMC_DEV=2
>> -CONFIG_FASTBOOT_CMD_OEM_FORMAT=y
>> +CONFIG_FASTBOOT_FLASH_BLOCK=y
>> +CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME="mmc"
>> +CONFIG_FASTBOOT_FLASH_BLOCK_DEVICE_ID=2
>>   CONFIG_DM_I2C=y
>>   CONFIG_SYS_I2C_MESON=y
>>   CONFIG_MMC_MESON_GX=y
>> 
>> We cannot flash the "bootloader" partition:
>> $ fastboot flash bootloader u-boot_kvim3_noab.bin
>> Warning: skip copying bootloader image avb footer (bootloader partition size: 0, bootloader image size: 1285488).
>> Sending 'bootloader' (1255 KB)                     OKAY [  0.054s]
>> Writing 'bootloader'                               FAILED (remote: 'failed to get partition info')
>> fastboot: error: Command failed
>> 
>> If we do want to merge both (at some later time) the above should be supported.
>
> Right, I don't think we want to merge them, and boot partitions are an MMC specific feature,
> I don't think any other storages has those specific hardware boot partitions.

Yes, indeed. We should not want to merge all the code because we would
end up with missing features for MMC.

>
> In any case, we can still just name the boot partition "bootloader" and raw flash it.

But using raw partition descriptors[1] does not work with the above

[1] https://docs.u-boot.org/en/latest/android/fastboot.html#raw-partition-descriptors

>
>> 
>>> +
>>> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
>>> +	int "Define FASTBOOT block device id"
>>> +	depends on FASTBOOT_FLASH_BLOCK
>>> +	help
>>> +	  "Fastboot block device id"
>> 
>> This is similar as CONFIG_FASTBOOT_FLASH_MMC_DEV, no?
>
> No, the MMC one makes the mmc type implicit, while the block one
> can target any block device (and mmc)

I understand, but when CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME="mmc",
CONFIG_FASTBOOT_FLASH_MMC_DEV and FASTBOOT_FLASH_BLOCK_DEVICE_ID are the
same thing.

>
>> 
>> To me, it's confusing to have similar KConfig entries based on interface
>> type.
>> 
>> If we really want to do it this way, then we should add a safeguard in
>> the code: test if FASTBOOT_FLASH_BLOCK_INTERFACE_NAME is "mmc" and if so, add a
>> warning to recommend using FASTBOOT_FLASH_MMC instead.
>> 
>> What's your opinion on this?
>
> Perhaps, not sure it's really problematic

Maybe not for you, because you have the history on this project. But to
a newcomer, having both options seems a bit strange.

If we look at how the blk-uclass is implemented, it inconsisent with how
it's done with fastboot.

longer term, I'd rather deprecate the CONFIG_FASTBOOT_FLASH_MMC_DEV
entry so that we use FASTBOOT_FLASH_BLOCK_INTERFACE_NAME for everything
(including mmc)

I understand that you might not have the time for doing so, so as a
first step can we add a (printf) warning in the fastboot block layer
when the interface name is mmc ?

>
>> 
>>> +
>>>   config FASTBOOT_GPT_NAME
>>>   	string "Target name for updating GPT"
>>>   	depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
>>> diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile
>>> index 048af5aa823436956142a536c5f7dcf1a8948726..91e98763e8eab84ccd9b8e5354ff1419f61ef372 100644
>>> --- a/drivers/fastboot/Makefile
>>> +++ b/drivers/fastboot/Makefile
>>> @@ -3,5 +3,7 @@
>>>   obj-y += fb_common.o
>>>   obj-y += fb_getvar.o
>>>   obj-y += fb_command.o
>>> -obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o
>>> +obj-$(CONFIG_FASTBOOT_FLASH_BLOCK) += fb_block.o
>>> +# MMC reuses block implementation
>>> +obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_block.o fb_mmc.o
>>>   obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o
>>> diff --git a/drivers/fastboot/fb_block.c b/drivers/fastboot/fb_block.c
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..e79429a9732c1da1ce88358747cf1cb9419cd12b
>>> --- /dev/null
>>> +++ b/drivers/fastboot/fb_block.c
>>> @@ -0,0 +1,313 @@
>>> +// SPDX-License-Identifier: BSD-2-Clause
>> 
>> Should this be GPL?
>
> Very good qustion, I'm not the copyright holder, the original author is Dmitrii, not sure about
> the legal things here.
>
> Tom can you help ?

I think we need Dmitrii to acknowledge publicly (on the list) it's okay
to re-licence this.

Dmitrii, are you okay to re-licence this code under GPL v2 (which is
common licence for this project)

>
>> 
>> See:
>> https://docs.u-boot.org/en/latest/develop/sending_patches.html#notes
>> 
>>> +/*
>>> + * Copyright (C) 2024 The Android Open Source Project
>>> + */
>>> +
>>> +#include <blk.h>
>>> +#include <div64.h>
>>> +#include <fastboot-internal.h>
>>> +#include <fastboot.h>
>>> +#include <fb_block.h>
>>> +#include <image-sparse.h>
>>> +#include <part.h>
>>> +#include <malloc.h>
>>> +
>>> +/**
>>> + * FASTBOOT_MAX_BLOCKS_ERASE - maximum blocks to erase per derase call
>>> + *
>>> + * in the ERASE case we can have much larger buffer size since
>>> + * we're not transferring an actual buffer
>>> + */
>>> +#define FASTBOOT_MAX_BLOCKS_ERASE 1048576
>>> +/**
>>> + * FASTBOOT_MAX_BLOCKS_SOFT_ERASE - maximum blocks to software erase at once
>>> + */
>>> +#define FASTBOOT_MAX_BLOCKS_SOFT_ERASE 4096
>>> +/**
>>> + * FASTBOOT_MAX_BLOCKS_WRITE - maximum blocks to write per dwrite call
>>> + */
>>> +#define FASTBOOT_MAX_BLOCKS_WRITE 65536
>>> +
>>> +struct fb_block_sparse {
>>> +	struct blk_desc	*dev_desc;
>>> +};
>>> +
>>> +static lbaint_t fb_block_write(struct blk_desc *block_dev, lbaint_t start,
>>> +			       lbaint_t blkcnt, const void *buffer)
>>> +{
>>> +	lbaint_t blk = start;
>>> +	lbaint_t blks_written = 0;
>>> +	lbaint_t cur_blkcnt = 0;
>>> +	lbaint_t blks = 0;
>>> +	void *erase_buf = NULL;
>>> +	int erase_buf_blks = 0;
>>> +	int step = buffer ? FASTBOOT_MAX_BLOCKS_WRITE : FASTBOOT_MAX_BLOCKS_ERASE;
>>> +	int i;
>>> +
>>> +	for (i = 0; i < blkcnt; i += step) {
>>> +		cur_blkcnt = min((int)blkcnt - i, step);
>>> +		if (buffer) {
>>> +			if (fastboot_progress_callback)
>>> +				fastboot_progress_callback("writing");
>>> +			blks_written = blk_dwrite(block_dev, blk, cur_blkcnt,
>>> +						  buffer + (i * block_dev->blksz));
>>> +		} else {
>>> +			if (fastboot_progress_callback)
>>> +				fastboot_progress_callback("erasing");
>>> +
>>> +			if (!erase_buf) {
>>> +				blks_written = blk_derase(block_dev, blk, cur_blkcnt);
>>> +
>>> +				/* Allocate erase buffer if erase is not implemented */
>>> +				if ((long)blks_written == -ENOSYS) {
>>> +					erase_buf_blks = min_t(long, blkcnt,
>>> +							       FASTBOOT_MAX_BLOCKS_SOFT_ERASE);
>>> +					erase_buf = malloc(erase_buf_blks * block_dev->blksz);
>>> +					memset(erase_buf, 0, erase_buf_blks * block_dev->blksz);
>>> +
>>> +					printf("Slowly writing empty buffers due to missing erase operation\n");
>>> +				}
>>> +			}
>>> +
>>> +			/* Write 0s instead of using erase operation, inefficient but functional */
>>> +			if (erase_buf) {
>>> +				int j;
>>> +
>>> +				blks_written = 0;
>>> +				for (j = 0; j < cur_blkcnt; j += erase_buf_blks) {
>>> +					lbaint_t remain = min_t(lbaint_t, cur_blkcnt - j,
>>> +								erase_buf_blks);
>>> +
>>> +					blks_written += blk_dwrite(block_dev, blk + j,
>>> +								   remain, erase_buf);
>>> +					printf(".");
>> 
>> Can we move soft erase to a separate function? Here we already handle
>> normal block write and normal block erase. Having software erase as well
>> makes the function a bit too long/nested in my opinion.
>> 
>> I't also avoid writing things like:
>> 
>>      blks_written = blk_derase(block_dev, blk, cur_blkcnt);
>> 
>> Where blks_written actually means "amount of blocks that were erased".
>> 
>> Or maybe split write/erase to make each function do only one thing.
>
> Sure, I'll move it to a separate function

Great, thanks.

[...]

>
>>>
>>> -- 
>>> 2.34.1

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[Dmitrii Merkurev]

>
> Dmitrii, are you okay to re-licence this code under GPL v2 (which is
> common licence for this project)
>

Yes, I'm ok with that.

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[Mattijs Korpershoek]

On mar., avril 29, 2025 at 11:45, Dmitrii Merkurev <dimorinny@google.com> wrote:

>>
>> Dmitrii, are you okay to re-licence this code under GPL v2 (which is
>> common licence for this project)
>>
>
> Yes, I'm ok with that.

Fantastic, thanks a lot for answering so quickly!

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[neil.armstrong]

On 29/04/2025 14:32, Mattijs Korpershoek wrote:
> On mar., avril 29, 2025 at 11:45, Dmitrii Merkurev <dimorinny@google.com> wrote:
> 
>>>
>>> Dmitrii, are you okay to re-licence this code under GPL v2 (which is
>>> common licence for this project)
>>>
>>
>> Yes, I'm ok with that.
> 
> Fantastic, thanks a lot for answering so quickly!

Thanks for all the comments, I'll prepare a v3.

Thanks,
Neil

Re: [PATCH RFT v2 1/3] fastboot: blk: introduce fastboot block flashing support

[neil.armstrong]

Hi,


<snip>
> But using raw partition descriptors[1] does not work with the above
> 
> [1] https://docs.u-boot.org/en/latest/android/fastboot.html#raw-partition-descriptors

Right, not all features will work with generic block backend, but it's a start!

> 
>>
>>>
>>>> +
>>>> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
>>>> +	int "Define FASTBOOT block device id"
>>>> +	depends on FASTBOOT_FLASH_BLOCK
>>>> +	help
>>>> +	  "Fastboot block device id"
>>>
>>> This is similar as CONFIG_FASTBOOT_FLASH_MMC_DEV, no?
>>
>> No, the MMC one makes the mmc type implicit, while the block one
>> can target any block device (and mmc)
> 
> I understand, but when CONFIG_FASTBOOT_FLASH_BLOCK_INTERFACE_NAME="mmc",
> CONFIG_FASTBOOT_FLASH_MMC_DEV and FASTBOOT_FLASH_BLOCK_DEVICE_ID are the
> same thing.
> 
>>
>>>
>>> To me, it's confusing to have similar KConfig entries based on interface
>>> type.
>>>
>>> If we really want to do it this way, then we should add a safeguard in
>>> the code: test if FASTBOOT_FLASH_BLOCK_INTERFACE_NAME is "mmc" and if so, add a
>>> warning to recommend using FASTBOOT_FLASH_MMC instead.
>>>
>>> What's your opinion on this?
>>
>> Perhaps, not sure it's really problematic
> 
> Maybe not for you, because you have the history on this project. But to
> a newcomer, having both options seems a bit strange.
> 
> If we look at how the blk-uclass is implemented, it inconsisent with how
> it's done with fastboot.
> 
> longer term, I'd rather deprecate the CONFIG_FASTBOOT_FLASH_MMC_DEV
> entry so that we use FASTBOOT_FLASH_BLOCK_INTERFACE_NAME for everything
> (including mmc)
> 
> I understand that you might not have the time for doing so, so as a
> first step can we add a (printf) warning in the fastboot block layer
> when the interface name is mmc ?
> 

Yeah my goal is to first have a support for generic block, merging
both makes sense.

Ok I'll add a warning

Thanks,
Neil