[PATCH v4] lib: sparse: Make CHUNK_TYPE_RAW buffer aligned

[Sean Anderson]

On 11/17/21 3:41 AM, qianfan wrote:
> 
> 在 2021/11/16 23:05, Sean Anderson 写道:
>>
>>
>> On 11/15/21 8:35 PM, 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 | 69 ++++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 61 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/image-sparse.c b/lib/image-sparse.c
>>> index d80fdbbf58..5ec0f94ab3 100644
>>> --- a/lib/image-sparse.c
>>> +++ b/lib/image-sparse.c
>>> @@ -46,9 +46,66 @@
>>>   #include <asm/cache.h>
>>>     #include <linux/math64.h>
>>> +#include <linux/err.h>
>>>     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)
>>> +{
>>> +    lbaint_t n = blkcnt, write_blks, blks = 0, aligned_buf_blks = 100;
>>> +    uint32_t *aligned_buf = NULL;
>>> +
>>> +    if (CONFIG_IS_ENABLED(SYS_DCACHE_OFF)) {
>>> +        write_blks = info->write(info, blk, n, data);
>>> +        if (write_blks < n)
>>> +            goto write_fail;
>>> +
>>> +        return write_blks;
>>> +    }
>>> +
>>> +    aligned_buf = memalign(ARCH_DMA_MINALIGN, info->blksz * aligned_buf_blks);
>>> +    if (!aligned_buf) {
>>> +        info->mssg("Malloc failed for: CHUNK_TYPE_RAW", response);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    while (blkcnt > 0) {
>>> +        n = min(aligned_buf_blks, blkcnt);
>>> +        memcpy(aligned_buf, data, n * info->blksz);
>>> +
>>> +        /* write_blks might be > n due to NAND bad-blocks */
>>
>> nit: <
> 
> This comment is misleading here, but it is actually correct. It is correct to write more that the specified number when nand flash bad blocks occur, so (write_blks >= n) is write successful. maybe rewrite the judge conditions like this is better?

Ah, you are right.

> if (!(write_blks >= n))
> 
>>
>>> +        write_blks = info->write(info, blk + blks, n, aligned_buf);
>>> +        if (write_blks < n) {
>>> +            free(aligned_buf);
>>> +            goto write_fail;
>>> +        }
>>> +
>>> +        blks += write_blks;
>>> +        data += n * info->blksz;
>>> +        blkcnt -= n;
>>> +    }
>>> +
>>> +    free(aligned_buf);
>>> +    return blks;
>>> +
>>> +write_fail:
>>
>> I think this label can be lower, but it does not affect correctness.
> sorry, could you please explain this clear, I can't understand.
>>
>>> +    if (IS_ERR_VALUE(write_blks)) {
>>> +        printf("%s: Write failed, block #" LBAFU " [" LBAFU "] (%lld)\n",
>>> +               __func__, blk + blks, n, (long long)write_blks);
>>> +        info->mssg("flash write failure", response);
>>> +        return write_blks;
>>> +    }

The above label could be placed here, since you check the value of write_blks before jumping.

--Sean

>>> +    /* write_blks < n */
>>> +    printf("%s: Write failed, block #" LBAFU " [" LBAFU "]\n",
>>> +           __func__, blk + blks, n);
>>> +    info->mssg("flash write failure(incomplete)", response);
>>> +    return -1;
>>
>> -EIO?
> It's OK
>>
>>> +}
>>> +
>>>   int write_sparse_image(struct sparse_storage *info,
>>>                  const char *part_name, void *data, char *response)
>>>   {
>>> @@ -152,15 +209,11 @@ int write_sparse_image(struct sparse_storage *info,
>>>                   return -1;
>>>               }
>>>   -            blks = info->write(info, blk, blkcnt, data);
>>> -            /* blks might be > blkcnt (eg. NAND bad-blocks) */
>>> -            if (blks < blkcnt) {
>>> -                printf("%s: %s" LBAFU " [" LBAFU "]\n",
>>> -                       __func__, "Write failed, block #",
>>> -                       blk, blks);
>>> -                info->mssg("flash write failure", response);
>>> +            blks = write_sparse_chunk_raw(info, blk, blkcnt,
>>> +                              data, response);
>>> +            if (blks < 0)
>>>                   return -1;
>>
>> ditto
> It's OK
>>
>>> -            }
>>> +
>>>               blk += blks;
>>>               bytes_written += ((u64)blkcnt) * info->blksz;
>>>               total_blocks += chunk_header->chunk_sz;
>>>
>>
>> Reviewed-by: Sean Anderson <sean.anderson at seco.com>