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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

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