[PATCH] lib: sparse: Make CHUNK_TYPE_RAW buffer aligned

[Sean Anderson]

Hi quanfan,

Thanks for the patch. I had something similar in mind.

On 11/5/21 1:46 AM, qianfanguijin at qq.com wrote:
> From: qianfan Zhao <qianfanguijin at 163.com>
>
> CHUNK_TYPE_RAW buffer is not aligned, and flash sparse images by
> fastboot will report "Misaligned operation" if DCACHE is enabled.
>
> Flashing Sparse Image
> CACHE: Misaligned operation at range [84000028, 84001028]
> CACHE: Misaligned operation at range [84001034, 84002034]
> CACHE: Misaligned operation at range [8401104c, 8401304c]
>
> Fix it
>
> Signed-off-by: qianfan Zhao <qianfanguijin at 163.com>
> ---
>   lib/image-sparse.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
> index d80fdbbf58..1c621cd685 100644
> --- a/lib/image-sparse.c
> +++ b/lib/image-sparse.c
> @@ -49,6 +49,48 @@
>
>   static void default_log(const char *ignored, char *response) {}
>
> +static lbaint_t write_sparse_chunk_raw(struct sparse_storage *info,
> +				       lbaint_t blk, lbaint_t blkcnt,
> +				       void *data,
> +				       char *response)
> +{
> +#if !CONFIG_IS_ENABLED(SYS_DCACHE_OFF)

Please rewrite this like

	if (CONFIG_IS_ENABLED(SYS_DCACHE_OFF)
		return info->write(info, blk, blkcnt, data);

> +	lbaint_t n, blks = 0, aligned_buf_blks = 100;

Can we try allocating info->blksz * blkcnt up front?

> +	uint32_t *aligned_buf = NULL;
> +
> +	while (blkcnt > 0) {
> +		aligned_buf = (uint32_t *)
> +				   memalign(ARCH_DMA_MINALIGN,
> +					    ROUNDUP(
> +						info->blksz * aligned_buf_blks,
> +						ARCH_DMA_MINALIGN));

I don't think we need this ROUNDUP. info->blksz is the block size of the
underlying storage, which is at least 512. And void pointers don't need
to be casted.

> +		if (!aligned_buf) {
> +			info->mssg("Malloc failed for: CHUNK_TYPE_RAW",
> +				   response);
> +			return -1;

return -ENOMEM;

Yes, the rest of the function returns -1, but there is no reason to
perpetuate that.

> +		}
> +
> +		n = min(aligned_buf_blks, blkcnt);
> +		memcpy(aligned_buf, data, n * info->blksz);
> +
> +		if (info->write(info, blk + blks, n, aligned_buf) != n) {
> +			free(aligned_buf);

Can we reuse the buffer instead of allocating/freeing it every loop?

> +			return n + blks;
> +		}
> +
> +		free(aligned_buf);
> +
> +		data += n * info->blksz;
> +		blkcnt -= n;
> +		blks += n;
> +	}
> +
> +	return blks;
> +#else
> +	return info->write(info, blk, blkcnt, data);
> +#endif
> +}
> +
>   int write_sparse_image(struct sparse_storage *info,
>   		       const char *part_name, void *data, char *response)
>   {
> @@ -152,7 +194,9 @@ int write_sparse_image(struct sparse_storage *info,
>   				return -1;
>   			}
>
> -			blks = info->write(info, blk, blkcnt, data);
> +			blks = write_sparse_chunk_raw(info, blk, blkcnt,
> +						      data, response);
> +
>   			/* blks might be > blkcnt (eg. NAND bad-blocks) */
>   			if (blks < blkcnt) {
>   				printf("%s: %s" LBAFU " [" LBAFU "]\n",

You also need to fix the error handling here. Otherwise you will get
tuings like "Write failed, block #4294967295" on out of memory.

--Sean