[PATCH] lib: sparse: Make CHUNK_TYPE_RAW buffer aligned

[qianfan]

在 2021/11/5 23:06, 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.

thanks for your's review. I had changed based on yours guide and has a 
question about this error message.

What is your's test platform, nand flash? I had tested on emmc platform 
and no such messages.

>
> --Sean