Merge pull request #9169 from dhalbert/flash_cache_table-fix

Author: dhalbert

supervisor/shared/external_flash/external_flash.c

@@ -54,7 +54,11 @@ static const external_flash_device *flash_device = NULL;
 // cache.
 static uint32_t dirty_mask;
 
-// Table of pointers to each cached block
+// Table of pointers to each cached block. Should be zero'd after allocation.
+#define BLOCKS_PER_SECTOR (SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE)
+#define PAGES_PER_BLOCK (FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE)
+#define FLASH_CACHE_TABLE_NUM_ENTRIES (BLOCKS_PER_SECTOR * PAGES_PER_BLOCK)
+#define FLASH_CACHE_TABLE_SIZE (FLASH_CACHE_TABLE_NUM_ENTRIES * sizeof (uint8_t *))
 static uint8_t **flash_cache_table = NULL;
 
 // Wait until both the write enable and write in progress bits have cleared.
@@ -207,7 +211,7 @@ void supervisor_flash_init(void) {
     // Delay to give the SPI Flash time to get going.
     // TODO(tannewt): Only do this when we know power was applied vs a reset.
     uint16_t max_start_up_delay_us = 0;
-    for (uint8_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
+    for (size_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
         if (possible_devices[i].start_up_time_us > max_start_up_delay_us) {
             max_start_up_delay_us = possible_devices[i].start_up_time_us;
         }
@@ -219,7 +223,7 @@ void supervisor_flash_init(void) {
     #ifdef EXTERNAL_FLASH_NO_JEDEC
     // For NVM that don't have JEDEC response
     spi_flash_command(CMD_WAKE);
-    for (uint8_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
+    for (size_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
         const external_flash_device *possible_device = &possible_devices[i];
         flash_device = possible_device;
         break;
@@ -234,7 +238,7 @@ void supervisor_flash_init(void) {
     while ((count-- > 0) && (jedec_id_response[0] == 0xff || jedec_id_response[2] == 0x00)) {
         spi_flash_read_command(CMD_READ_JEDEC_ID, jedec_id_response, 3);
     }
-    for (uint8_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
+    for (size_t i = 0; i < EXTERNAL_FLASH_DEVICE_COUNT; i++) {
         const external_flash_device *possible_device = &possible_devices[i];
         if (jedec_id_response[0] == possible_device->manufacturer_id &&
             jedec_id_response[1] == possible_device->memory_type &&
@@ -323,7 +327,7 @@ static bool flush_scratch_flash(void) {
     // cached.
     bool copy_to_scratch_ok = true;
     uint32_t scratch_sector = flash_device->total_size - SPI_FLASH_ERASE_SIZE;
-    for (uint8_t i = 0; i < SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE; i++) {
+    for (size_t i = 0; i < BLOCKS_PER_SECTOR; i++) {
         if ((dirty_mask & (1 << i)) == 0) {
             copy_to_scratch_ok = copy_to_scratch_ok &&
                 copy_block(current_sector + i * FILESYSTEM_BLOCK_SIZE,
@@ -338,72 +342,70 @@ static bool flush_scratch_flash(void) {
     // Second, erase the current sector.
     erase_sector(current_sector);
     // Finally, copy the new version into it.
-    for (uint8_t i = 0; i < SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE; i++) {
+    for (size_t i = 0; i < BLOCKS_PER_SECTOR; i++) {
         copy_block(scratch_sector + i * FILESYSTEM_BLOCK_SIZE,
             current_sector + i * FILESYSTEM_BLOCK_SIZE);
     }
     return true;
 }
 
+// Free all entries in the partially or completely filled flash_cache_table, and then free the table itself.
+static void release_ram_cache(void) {
+    if (flash_cache_table == NULL) {
+        return;
+    }
+
+    for (size_t i = 0; i < FLASH_CACHE_TABLE_NUM_ENTRIES; i++) {
+        // Table may not be completely full. Stop at first NULL entry.
+        if (flash_cache_table[i] == NULL) {
+            break;
+        }
+        port_free(flash_cache_table[i]);
+    }
+    port_free(flash_cache_table);
+    flash_cache_table = NULL;
+}
+
 // Attempts to allocate a new set of page buffers for caching a full sector in
 // ram. Each page is allocated separately so that the GC doesn't need to provide
 // one huge block. We can free it as we write if we want to also.
 static bool allocate_ram_cache(void) {
-    uint8_t blocks_per_sector = SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE;
-    uint8_t pages_per_block = FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE;
-
-    uint32_t table_size = blocks_per_sector * pages_per_block * sizeof(size_t);
-    // Attempt to allocate outside the heap first.
-    flash_cache_table = port_malloc(table_size, false);
-
-    // Declare i and j outside the loops in case we fail to allocate everything
-    // we need. In that case we'll give it back.
-    uint8_t i = 0;
-    uint8_t j = 0;
-    bool success = flash_cache_table != NULL;
-    for (i = 0; i < blocks_per_sector && success; i++) {
-        for (j = 0; j < pages_per_block && success; j++) {
+    flash_cache_table = port_malloc(FLASH_CACHE_TABLE_SIZE, false);
+    if (flash_cache_table == NULL) {
+        // Not enough space even for the cache table.
+        return false;
+    }
+
+    // Clear all the entries so it's easy to find the last entry.
+    memset(flash_cache_table, 0, FLASH_CACHE_TABLE_SIZE);
+
+    bool success = true;
+    for (size_t i = 0; i < BLOCKS_PER_SECTOR && success; i++) {
+        for (size_t j = 0; j < PAGES_PER_BLOCK && success; j++) {
             uint8_t *page_cache = port_malloc(SPI_FLASH_PAGE_SIZE, false);
             if (page_cache == NULL) {
                 success = false;
                 break;
             }
-            flash_cache_table[i * pages_per_block + j] = page_cache;
+            flash_cache_table[i * PAGES_PER_BLOCK + j] = page_cache;
         }
     }
+
     // We couldn't allocate enough so give back what we got.
     if (!success) {
-        // We add 1 so that we delete 0 when i is 1. Going to zero (i >= 0)
-        // would never stop because i is unsigned.
-        i++;
-        for (; i > 0; i--) {
-            for (; j > 0; j--) {
-                port_free(flash_cache_table[(i - 1) * pages_per_block + (j - 1)]);
-            }
-            j = pages_per_block;
-        }
-        port_free(flash_cache_table);
-        flash_cache_table = NULL;
+        release_ram_cache();
     }
     return success;
 }
 
-static void release_ram_cache(void) {
-    uint8_t blocks_per_sector = SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE;
-    uint8_t pages_per_block = FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE;
-    for (uint8_t i = 0; i < blocks_per_sector; i++) {
-        for (uint8_t j = 0; j < pages_per_block; j++) {
-            uint32_t offset = i * pages_per_block + j;
-            port_free(flash_cache_table[offset]);
-        }
-    }
-    port_free(flash_cache_table);
-    flash_cache_table = NULL;
-}
-
 // Flush the cached sector from ram onto the flash. We'll free the cache unless
 // keep_cache is true.
 static bool flush_ram_cache(bool keep_cache) {
+    if (flash_cache_table == NULL) {
+        // Nothing to flush because there is no cache.
+        return true;
+    }
+
     if (current_sector == NO_SECTOR_LOADED) {
         if (!keep_cache) {
             release_ram_cache();
@@ -414,13 +416,12 @@ static bool flush_ram_cache(bool keep_cache) {
     // we've cached. If we don't do this we'll erase the data during the sector
     // erase below.
     bool copy_to_ram_ok = true;
-    uint8_t pages_per_block = FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE;
-    for (uint8_t i = 0; i < SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE; i++) {
+    for (size_t i = 0; i < BLOCKS_PER_SECTOR; i++) {
         if ((dirty_mask & (1 << i)) == 0) {
-            for (uint8_t j = 0; j < pages_per_block; j++) {
+            for (size_t j = 0; j < PAGES_PER_BLOCK; j++) {
                 copy_to_ram_ok = read_flash(
-                    current_sector + (i * pages_per_block + j) * SPI_FLASH_PAGE_SIZE,
-                    flash_cache_table[i * pages_per_block + j],
+                    current_sector + (i * PAGES_PER_BLOCK + j) * SPI_FLASH_PAGE_SIZE,
+                    flash_cache_table[i * PAGES_PER_BLOCK + j],
                     SPI_FLASH_PAGE_SIZE);
                 if (!copy_to_ram_ok) {
                     break;
@@ -438,10 +439,10 @@ static bool flush_ram_cache(bool keep_cache) {
     // Second, erase the current sector.
     erase_sector(current_sector);
     // Lastly, write all the data in ram that we've cached.
-    for (uint8_t i = 0; i < SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE; i++) {
-        for (uint8_t j = 0; j < pages_per_block; j++) {
-            write_flash(current_sector + (i * pages_per_block + j) * SPI_FLASH_PAGE_SIZE,
-                flash_cache_table[i * pages_per_block + j],
+    for (size_t i = 0; i < BLOCKS_PER_SECTOR; i++) {
+        for (size_t j = 0; j < PAGES_PER_BLOCK; j++) {
+            write_flash(current_sector + (i * PAGES_PER_BLOCK + j) * SPI_FLASH_PAGE_SIZE,
+                flash_cache_table[i * PAGES_PER_BLOCK + j],
                 SPI_FLASH_PAGE_SIZE);
         }
     }
@@ -496,15 +497,14 @@ static bool external_flash_read_block(uint8_t *dest, uint32_t block) {
 
     // Mask out the lower bits that designate the address within the sector.
     uint32_t this_sector = address & (~(SPI_FLASH_ERASE_SIZE - 1));
-    uint8_t block_index = (address / FILESYSTEM_BLOCK_SIZE) % (SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE);
-    uint8_t mask = 1 << (block_index);
+    size_t block_index = (address / FILESYSTEM_BLOCK_SIZE) % BLOCKS_PER_SECTOR;
+    uint32_t mask = 1 << (block_index);
     // We're reading from the currently cached sector.
     if (current_sector == this_sector && (mask & dirty_mask) > 0) {
         if (flash_cache_table != NULL) {
-            uint8_t pages_per_block = FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE;
-            for (int i = 0; i < pages_per_block; i++) {
+            for (int i = 0; i < PAGES_PER_BLOCK; i++) {
                 memcpy(dest + i * SPI_FLASH_PAGE_SIZE,
-                    flash_cache_table[block_index * pages_per_block + i],
+                    flash_cache_table[block_index * PAGES_PER_BLOCK + i],
                     SPI_FLASH_PAGE_SIZE);
             }
             return true;
@@ -527,8 +527,8 @@ static bool external_flash_write_block(const uint8_t *data, uint32_t block) {
     wait_for_flash_ready();
     // Mask out the lower bits that designate the address within the sector.
     uint32_t this_sector = address & (~(SPI_FLASH_ERASE_SIZE - 1));
-    uint8_t block_index = (address / FILESYSTEM_BLOCK_SIZE) % (SPI_FLASH_ERASE_SIZE / FILESYSTEM_BLOCK_SIZE);
-    uint8_t mask = 1 << (block_index);
+    size_t block_index = (address / FILESYSTEM_BLOCK_SIZE) % BLOCKS_PER_SECTOR;
+    uint32_t mask = 1 << (block_index);
     // Flush the cache if we're moving onto a sector or we're writing the
     // same block again.
     if (current_sector != this_sector || (mask & dirty_mask) > 0) {
@@ -550,9 +550,8 @@ static bool external_flash_write_block(const uint8_t *data, uint32_t block) {
     dirty_mask |= mask;
     // Copy the block to the appropriate cache.
     if (flash_cache_table != NULL) {
-        uint8_t pages_per_block = FILESYSTEM_BLOCK_SIZE / SPI_FLASH_PAGE_SIZE;
-        for (int i = 0; i < pages_per_block; i++) {
-            memcpy(flash_cache_table[block_index * pages_per_block + i],
+        for (int i = 0; i < PAGES_PER_BLOCK; i++) {
+            memcpy(flash_cache_table[block_index * PAGES_PER_BLOCK + i],
                 data + i * SPI_FLASH_PAGE_SIZE,
                 SPI_FLASH_PAGE_SIZE);
         }