Re: [PATCH] fastboot: scale default sparse max_download_size with available memory

[Johannes Zink <j.zink@pengutronix.de>]

On 8/25/23 19:22, Ahmad Fatoum wrote:
> The fastboot host tool chokes when asked to sparse images that are not
> 4K-aligned:
> 
>    error: write_sparse_skip_chunk: don't care size 4280912 is not a multiple
>    of the block size 4096
> 
> This is often not a problem, because disk images tend to be 4K-aligned
> and are best converted to sparse images properly explicitly anyway, so
> don't care holes are preserved.
> 
> Bootloader binaries on the other hand are smaller than the default
> fastboot.max_download_size of 8M, so they are downloaded as a single
> chunk without problems.
> 
> Situation is different for kernel and FIT images, which aren't
> necessarily 4K aligned, usually bigger than 8M, and are usually
> compressed, so explicitly sparsing them doesn't save time and thus is
> usually not done.
> 
> Short of fixing the fastboot host tool, users can skip sparsing kernel
> images as well by choosing a suitable larger fastboot.max_download_size
> than the 8M default.
> 
> To improve user experience, let's scale max_download_size with the
> available memory. The minimum default value remains 8M, but the value
> can go up to 128M now and is computed by dividing available malloc
> area by 8 (roughly 1/16th of total SDRAM). For ARM systems, buffer
> sizes would now look like this:
> 
>    SDRAM Size	Fastboot Buffer size
> 
>    1024M		64M
>     512M		32M
>     256M		16M
>     128M		8M
> 
> To err on the side of caution, we only generate powers-of-two sizes.
> This only affects the default value and users can as before override
> it in the environment.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

LGTM

Reviewed-by: Johannes Zink <j.zink@pengutronix.de>

> ---
>   common/fastboot.c | 9 ++++++++-
>   include/memory.h  | 5 +++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fastboot.c b/common/fastboot.c
> index f91f398d7a95..645aaa6a9f3f 100644
> --- a/common/fastboot.c
> +++ b/common/fastboot.c
> @@ -30,7 +30,9 @@
>   #include <ubiformat.h>
>   #include <unistd.h>
>   #include <magicvar.h>
> +#include <linux/log2.h>
>   #include <linux/sizes.h>
> +#include <memory.h>
>   #include <progress.h>
>   #include <environment.h>
>   #include <globalvar.h>
> @@ -940,9 +942,14 @@ struct file_list *get_fastboot_partitions(void)
>   
>   static int fastboot_globalvars_init(void)
>   {
> -	if (IS_ENABLED(CONFIG_FASTBOOT_SPARSE))
> +	if (IS_ENABLED(CONFIG_FASTBOOT_SPARSE)) {
> +		fastboot_max_download_size
> +			= roundup_pow_of_two(clamp_t(ulong, mem_malloc_size() / 8,
> +						     SZ_8M, SZ_128M));
>   		globalvar_add_simple_int("fastboot.max_download_size",
>   				 &fastboot_max_download_size, "%u");
> +	}
> +
>   	globalvar_add_simple_bool("fastboot.bbu", &fastboot_bbu);
>   	globalvar_add_simple_string("fastboot.partitions",
>   				    &fastboot_partitions);
> diff --git a/include/memory.h b/include/memory.h
> index ffd66db02ba0..73642dd5aaa5 100644
> --- a/include/memory.h
> +++ b/include/memory.h
> @@ -10,6 +10,11 @@ void mem_malloc_init(void *start, void *end);
>   ulong mem_malloc_start(void);
>   ulong mem_malloc_end(void);
>   
> +static inline ulong mem_malloc_size(void)
> +{
> +	return mem_malloc_end() - mem_malloc_start();
> +}
> +
>   struct memory_bank {
>   	struct list_head list;
>   	unsigned long start;

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |