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

[Neil Armstrong]

Hi,

On 02/07/2025 04:59, Lothar Waßmann wrote:
> Hi,
> 
> On Mon, 30 Jun 2025 17:19:57 +0200 Neil Armstrong wrote:
>> From: Dmitrii Merkurev <dimorinny at 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 at google.com>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek at kernel.org>
>> Tested-by: Mattijs Korpershoek <mkorpershoek at kernel.org>
>> Signed-off-by: Neil Armstrong <neil.armstrong at 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?

Those are completely different purposes, this define is related to the
image-sparse.h FASTBOOT_MAX_BLK_WRITE, but for a generic block write.

Note this is an initial implementation, any optimization could be done later.

> 
>> +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!

Ack, will fix

> 
>> +
>> +	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).

Ok

> 
>> +		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.

OK

>> +	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!

Indeed

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

OK will fix those.

Thank,
Neil


> 
> Lothar Waßmann