[PATCH] lib: sparse: Make CHUNK_TYPE_RAW buffer aligned

[Sean Anderson]

On 11/7/21 9:12 PM, qianfan wrote:
> 
> 在 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.

write_sparse_chunk_raw has two error conditions: blks < blkcnt, and blks < 0. The latter case will be handled incorrectly.

--Sean