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, qianfangui...@qq.com wrote:
From: qianfan Zhao <qianfangui...@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 <qianfangui...@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

Reply via email to