Hi Neil, Thank you for the patch.
On mer., avril 09, 2025 at 09:58, neil.armstr...@linaro.org wrote: > From: Dmitrii Merkurev <dimori...@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 <dimori...@google.com> > Signed-off-by: Neil Armstrong <neil.armstr...@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
