Hi Lothar,
Thanks for participating in review. I gave you some wording feedback
off-list but since I did not get any response I'm sending it to everyone
as well.
On Wed, Jul 02, 2025 at 04:59, Lothar Waßmann <l...@karo-electronics.de> wrote:
> Hi,
>
> On Mon, 30 Jun 2025 17:19:57 +0200 Neil Armstrong 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>
>> Reviewed-by: Mattijs Korpershoek <mkorpersh...@kernel.org>
>> Tested-by: Mattijs Korpershoek <mkorpersh...@kernel.org>
>> Signed-off-by: Neil Armstrong <neil.armstr...@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... ;-)
This might be intended as a joke, but it could also be taken as an insult
for the submitter. ("the submitter is so dumb he could not even sort
alphabetically his includes").
Next time, please consider using something more neutral like:
"Make sure to sort alphabetically your includes".
No ill intention here. Thanks again for reviewing patches submitted on
the U-Boot mailing list.
Mattijs
>
>> +/**
>> + * 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
