From aacfe19a4e70dbbe194f10428a47ce0481209f04 Mon Sep 17 00:00:00 2001
From: Mahavir Jain <mahavir@espressif.com>
Date: Wed, 10 Jul 2024 15:51:01 +0530
Subject: [PATCH] fix(esp_mm): for existing mmap region, consider new offset
 for virtual addr

While returning virtual address for existing memory mapped region, newly
supplied offset from the physical address was not getting considered.

This was a regression present from ESP-IDF 5.1 release.

Added test case in spi_flash component that fails without this fix.

Closes https://github.com/espressif/esp-idf/issues/13929
---
 components/esp_mm/esp_mmu_map.c               | 10 ++++++-
 components/esp_mm/include/esp_mmu_map.h       |  2 +-
 .../flash_mmap/main/test_flash_mmap.c         | 27 +++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/components/esp_mm/esp_mmu_map.c b/components/esp_mm/esp_mmu_map.c
index 226ec38e0c..8b00d3402a 100644
--- a/components/esp_mm/esp_mmu_map.c
+++ b/components/esp_mm/esp_mmu_map.c
@@ -509,7 +509,15 @@ esp_err_t esp_mmu_map(esp_paddr_t paddr_start, size_t size, mmu_target_t target,
 
     if (is_enclosed) {
         ESP_LOGW(TAG, "paddr block is mapped already, vaddr_start: %p, size: 0x%x", (void *)mem_block->vaddr_start, mem_block->size);
-        *out_ptr = (void *)mem_block->vaddr_start;
+        /*
+         * This condition is triggered when `s_is_enclosed` is true and hence
+         * we are sure that `paddr_start` >= `mem_block->paddr_start`.
+         *
+         * Add the offset of new physical address while returning the virtual
+         * address.
+         */
+        const uint32_t new_paddr_offset = paddr_start - mem_block->paddr_start;
+        *out_ptr = (void *)mem_block->vaddr_start + new_paddr_offset;
         return ESP_ERR_INVALID_STATE;
     }
 
diff --git a/components/esp_mm/include/esp_mmu_map.h b/components/esp_mm/include/esp_mmu_map.h
index e4a42e483d..2e64a0d842 100644
--- a/components/esp_mm/include/esp_mmu_map.h
+++ b/components/esp_mm/include/esp_mmu_map.h
@@ -75,7 +75,7 @@ typedef uint32_t esp_paddr_t;
  *        - ESP_ERR_NOT_SUPPORTED: Only on ESP32, PSRAM is not a supported physical memory target
  *        - ESP_ERR_NOT_FOUND:     No enough size free block to use
  *        - ESP_ERR_NO_MEM:        Out of memory, this API will allocate some heap memory for internal usage
- *        - ESP_ERR_INVALID_STATE: Paddr is mapped already, this API will return corresponding vaddr_start of the previously mapped block.
+ *        - ESP_ERR_INVALID_STATE: Paddr is mapped already, this API will return corresponding `vaddr_start + new_block_offset` as per the previously mapped block.
  *                                 Only to-be-mapped paddr block is totally enclosed by a previously mapped block will lead to this error. (Identical scenario will behave similarly)
  *                                 new_block_start               new_block_end
  *                                              |-------- New Block --------|
diff --git a/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c b/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c
index 83b0453bcb..3f36d6bf34 100644
--- a/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c
+++ b/components/spi_flash/test_apps/flash_mmap/main/test_flash_mmap.c
@@ -101,6 +101,33 @@ static void setup_mmap_tests(void)
     }
 }
 
+TEST_CASE("Can get correct data in existing mapped region", "[spi_flash][mmap]")
+{
+    setup_mmap_tests();
+
+    printf("Mapping %"PRIx32" (+%"PRIx32")\n", start, end - start);
+    const void *ptr1;
+    TEST_ESP_OK( spi_flash_mmap(start, end - start, SPI_FLASH_MMAP_DATA, &ptr1, &handle1) );
+    printf("mmap_res: handle=%"PRIx32" ptr=%p\n", (uint32_t)handle1, ptr1);
+
+    /* Remap in the previously mapped region itself */
+    uint32_t new_start = start + CONFIG_MMU_PAGE_SIZE;
+    printf("Mapping %"PRIx32" (+%"PRIx32")\n", new_start, end - new_start);
+    const void *ptr2;
+    TEST_ESP_OK( spi_flash_mmap(new_start, end - new_start, SPI_FLASH_MMAP_DATA, &ptr2, &handle2) );
+    printf("mmap_res: handle=%"PRIx32" ptr=%p\n", (uint32_t)handle2, ptr2);
+
+    const void *src1 = (void *) ((uint32_t) ptr1 + CONFIG_MMU_PAGE_SIZE);
+    const void *src2 = ptr2;
+    /* Memory contents should be identical - as the region is same */
+    TEST_ASSERT_EQUAL(0, memcmp(src1, src2, end - new_start));
+
+    spi_flash_munmap(handle1);
+    handle1 = 0;
+    spi_flash_munmap(handle2);
+    handle2 = 0;
+    TEST_ASSERT_EQUAL_PTR(NULL, spi_flash_phys2cache(start, SPI_FLASH_MMAP_DATA));
+}
 
 TEST_CASE("Can mmap into data address space", "[spi_flash][mmap]")
 {