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

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

On Mon, 30 Jun 2025 17:19:57 +0200 Neil Armstrong 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>
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> Tested-by: Mattijs Korpershoek <mkorpershoek@kernel.org>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/fastboot/fb_block.c | 323 ++++++++++++++++++++++++++++++++++++++++++++
>  include/fb_block.h          | 105 ++++++++++++++
>  2 files changed, 428 insertions(+)
> 
> diff --git a/drivers/fastboot/fb_block.c b/drivers/fastboot/fb_block.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b725397c91af2717812e69e2b624076eb30f781d
> --- /dev/null
> +++ b/drivers/fastboot/fb_block.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * 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>
>
The alphabet seems to be a difficult concept for programmers to
grasp... ;-)

> +/**
> + * 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
> +
Why invent an arbitrary number here, when there is already
CONFIG_FASTBOOT_BUF_SIZE which could be used for this purpose?

> +struct fb_block_sparse {
> +	struct blk_desc	*dev_desc;
> +};
> +
> +/* Write 0s instead of using erase operation, inefficient but functional */
> +static lbaint_t fb_block_soft_erase(struct blk_desc *block_dev, lbaint_t blk,
> +				    lbaint_t cur_blkcnt, lbaint_t erase_buf_blks,
> +				    void *erase_buffer)
> +{
> +	lbaint_t blks_written = 0;
> +	int j;
>
lbaint_t to match the type of cur_blkcnt and prevent integer overflow
on 32bit systems!

> +
> +	memset(erase_buffer, 0, erase_buf_blks * block_dev->blksz);
> +
> +	for (j = 0; j < cur_blkcnt; j += erase_buf_blks) {
> +		lbaint_t remain = min_t(lbaint_t, cur_blkcnt - j,
> +				erase_buf_blks);
> +
No need for min_t() here (see above).

> +		blks_written += blk_dwrite(block_dev, blk + j,
> +				remain, erase_buffer);
> +		printf(".");
> +	}
> +
> +	return blks_written;
> +}
> +
> +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;
>
useless initialization of cur_blkcnt; definition could be moved inside
the for loop.
> +	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;
>
lbaint_t for 'step' an 'i' to match the type of blkcnt!

> +
> +	for (i = 0; i < blkcnt; i += step) {
> +		cur_blkcnt = min((int)blkcnt - i, step);
>
No type cast needed.


Lothar Waßmann