From 2fa97f38727ecda4d01a81d185464055eea4af9b Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 11 Apr 2024 14:00:37 -0400 Subject: [PATCH 1/2] fix flash_cache_table allocation failures; clean up code --- .../shared/external_flash/external_flash.c | 117 +++++++++--------- 1 file changed, 58 insertions(+), 59 deletions(-) diff --git a/supervisor/shared/external_flash/external_flash.c b/supervisor/shared/external_flash/external_flash.c index 4cd20ada3879..780737fdb99b 100644 --- a/supervisor/shared/external_flash/external_flash.c +++ b/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. @@ -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 (uint8_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 (uint8_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 (uint8_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 (uint8_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 (uint8_t i = 0; i < BLOCKS_PER_SECTOR; 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], 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 block_index = (address / FILESYSTEM_BLOCK_SIZE) % BLOCKS_PER_SECTOR; uint8_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,7 +527,7 @@ 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 block_index = (address / FILESYSTEM_BLOCK_SIZE) % BLOCKS_PER_SECTOR; uint8_t mask = 1 << (block_index); // Flush the cache if we're moving onto a sector or we're writing the // same block again. @@ -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); } From 98957cccd844ea724f65bc90ed345df60b0892d0 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 11 Apr 2024 14:15:41 -0400 Subject: [PATCH 2/2] change unneeded uint8_t decls to size_t or uint32_t --- .../shared/external_flash/external_flash.c | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/supervisor/shared/external_flash/external_flash.c b/supervisor/shared/external_flash/external_flash.c index 780737fdb99b..229fd629e0fe 100644 --- a/supervisor/shared/external_flash/external_flash.c +++ b/supervisor/shared/external_flash/external_flash.c @@ -211,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; } @@ -223,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; @@ -238,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 && @@ -327,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 < BLOCKS_PER_SECTOR; 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, @@ -342,7 +342,7 @@ 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 < BLOCKS_PER_SECTOR; 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); } @@ -416,9 +416,9 @@ 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; - for (uint8_t i = 0; i < BLOCKS_PER_SECTOR; 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], @@ -439,8 +439,8 @@ 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 < BLOCKS_PER_SECTOR; i++) { - for (uint8_t j = 0; j < 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); @@ -497,8 +497,8 @@ 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) % BLOCKS_PER_SECTOR; - 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) { @@ -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) % BLOCKS_PER_SECTOR; - 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) {