From da1cf494796d68bb5c02ba70cf78a904db08a170 Mon Sep 17 00:00:00 2001 From: Deomid Ryabkov Date: Fri, 22 Sep 2017 18:35:43 +0100 Subject: [PATCH] Be careful resetting flag bits Only reset those that are set, do not assume that 0 -> 1 writes are ignored. (hopefully) fixes https://github.com/pellepl/spiffs/issues/172 --- .travis.yml | 5 ++++- src/default/spiffs_config.h | 11 +++++++++++ src/spiffs_check.c | 23 ++++++++++++++++++----- src/spiffs_nucleus.c | 13 +++++++++---- src/test/test_spiffs.c | 19 +++++++++++-------- 5 files changed, 53 insertions(+), 18 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8c36dc5..3b3829c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,4 +5,7 @@ compiler: before_script: -script: make all && make clean && make test && make build-all && make clean test FLAGS=-DSPIFFS_OBJ_META_LEN=8 +script: > + make all && make clean && make test && make build-all && + make clean test FLAGS=-DSPIFFS_OBJ_META_LEN=8 && + make clean test FLAGS=-DSPIFFS_NO_BLIND_WRITES=1 diff --git a/src/default/spiffs_config.h b/src/default/spiffs_config.h index ce562bf..376d082 100644 --- a/src/default/spiffs_config.h +++ b/src/default/spiffs_config.h @@ -311,6 +311,17 @@ #define SPIFFS_IX_MAP 1 #endif +// By default SPIFFS in some cases relies on the property of NOR flash that bits +// cannot be set from 0 to 1 by writing and that controllers will ignore such +// bit changes. This results in fewer reads as SPIFFS can in some cases perform +// blind writes, with all bits set to 1 and only those it needs reset set to 0. +// Most of the chips and controllers allow this behavior, so the default is to +// use this technique. If your controller is one of the rare ones that don't, +// turn this option on and SPIFFS will perform a read-modify-write instead. +#ifndef SPIFFS_NO_BLIND_WRITES +#define SPIFFS_NO_BLIND_WRITES 0 +#endif + // Set SPIFFS_TEST_VISUALISATION to non-zero to enable SPIFFS_vis function // in the api. This function will visualize all filesystem using given printf // function. diff --git a/src/spiffs_check.c b/src/spiffs_check.c index dde85ef..0331fb1 100644 --- a/src/spiffs_check.c +++ b/src/spiffs_check.c @@ -161,11 +161,17 @@ static s32_t spiffs_delete_obj_lazy(spiffs *fs, spiffs_obj_id obj_id) { return SPIFFS_OK; } SPIFFS_CHECK_RES(res); - u8_t flags = 0xff & ~SPIFFS_PH_FLAG_IXDELE; + u8_t flags = 0xff; +#if SPIFFS_NO_BLIND_WRITES + res = _spiffs_rd(fs, SPIFFS_OP_T_OBJ_LU | SPIFFS_OP_C_READ, + 0, SPIFFS_PAGE_TO_PADDR(fs, objix_hdr_pix) + offsetof(spiffs_page_header, flags), + sizeof(flags), &flags); + SPIFFS_CHECK_RES(res); +#endif + flags &= ~SPIFFS_PH_FLAG_IXDELE; res = _spiffs_wr(fs, SPIFFS_OP_T_OBJ_LU | SPIFFS_OP_C_UPDT, 0, SPIFFS_PAGE_TO_PADDR(fs, objix_hdr_pix) + offsetof(spiffs_page_header, flags), - sizeof(u8_t), - (u8_t *)&flags); + sizeof(flags), &flags); return res; } @@ -423,10 +429,17 @@ static s32_t spiffs_lookup_check_validate(spiffs *fs, spiffs_obj_id lu_obj_id, s // just finalize SPIFFS_CHECK_DBG("LU: FIXUP: unfinalized page is referred, finalizing\n"); CHECK_CB(fs, SPIFFS_CHECK_LOOKUP, SPIFFS_CHECK_FIX_LOOKUP, p_hdr->obj_id, p_hdr->span_ix); - u8_t flags = 0xff & ~SPIFFS_PH_FLAG_FINAL; + u8_t flags = 0xff; +#if SPIFFS_NO_BLIND_WRITES + res = _spiffs_rd(fs, SPIFFS_OP_T_OBJ_DA | SPIFFS_OP_C_READ, + 0, SPIFFS_PAGE_TO_PADDR(fs, cur_pix) + offsetof(spiffs_page_header, flags), + sizeof(flags), &flags); + SPIFFS_CHECK_RES(res); +#endif + flags &= ~SPIFFS_PH_FLAG_FINAL; res = _spiffs_wr(fs, SPIFFS_OP_T_OBJ_DA | SPIFFS_OP_C_UPDT, 0, SPIFFS_PAGE_TO_PADDR(fs, cur_pix) + offsetof(spiffs_page_header, flags), - sizeof(u8_t), (u8_t*)&flags); + sizeof(flags), &flags); } } } diff --git a/src/spiffs_nucleus.c b/src/spiffs_nucleus.c index 27ecdff..6b052b0 100644 --- a/src/spiffs_nucleus.c +++ b/src/spiffs_nucleus.c @@ -877,8 +877,6 @@ s32_t spiffs_page_delete( spiffs *fs, spiffs_page_ix pix) { s32_t res; - spiffs_page_header hdr; - hdr.flags = 0xff & ~(SPIFFS_PH_FLAG_DELET | SPIFFS_PH_FLAG_USED); // mark deleted entry in source object lookup spiffs_obj_id d_obj_id = SPIFFS_OBJ_ID_DELETED; res = _spiffs_wr(fs, SPIFFS_OP_T_OBJ_LU | SPIFFS_OP_C_DELE, @@ -892,11 +890,18 @@ s32_t spiffs_page_delete( fs->stats_p_allocated--; // mark deleted in source page + u8_t flags = 0xff; +#if SPIFFS_NO_BLIND_WRITES + res = _spiffs_rd(fs, SPIFFS_OP_T_OBJ_DA | SPIFFS_OP_C_READ, + 0, SPIFFS_PAGE_TO_PADDR(fs, pix) + offsetof(spiffs_page_header, flags), + sizeof(flags), &flags); + SPIFFS_CHECK_RES(res); +#endif + flags &= ~(SPIFFS_PH_FLAG_DELET | SPIFFS_PH_FLAG_USED); res = _spiffs_wr(fs, SPIFFS_OP_T_OBJ_DA | SPIFFS_OP_C_DELE, 0, SPIFFS_PAGE_TO_PADDR(fs, pix) + offsetof(spiffs_page_header, flags), - sizeof(u8_t), - (u8_t *)&hdr.flags); + sizeof(flags), &flags); return res; } diff --git a/src/test/test_spiffs.c b/src/test/test_spiffs.c index aa60866..78ca447 100644 --- a/src/test/test_spiffs.c +++ b/src/test/test_spiffs.c @@ -200,14 +200,17 @@ static s32_t _write( } for (i = 0; i < size; i++) { - if (((addr + i) & (SPIFFS_CFG_LOG_PAGE_SZ(&__fs)-1)) != offsetof(spiffs_page_header, flags)) { - if (check_valid_flash && ((AREA(addr + i) ^ src[i]) & src[i])) { - printf("trying to write %02x to %02x at addr %08x (as part of writing %d bytes to addr %08x)\n", src[i], AREA(addr + i), addr+i, size, addr); - spiffs_page_ix pix = (addr + i) / SPIFFS_CFG_LOG_PAGE_SZ(&__fs); - dump_page(&__fs, pix); - ERREXIT(); - return -1; - } +#if !SPIFFS_NO_BLIND_WRITES + if (((addr + i) & (SPIFFS_CFG_LOG_PAGE_SZ(&__fs)-1)) == offsetof(spiffs_page_header, flags)) { + /* Blind flag writes are allowed. */ + } else +#endif + if (check_valid_flash && ((AREA(addr + i) ^ src[i]) & src[i])) { + printf("trying to write %02x to %02x at addr %08x (as part of writing %d bytes to addr %08x)\n", src[i], AREA(addr + i), addr+i, size, addr); + spiffs_page_ix pix = (addr + i) / SPIFFS_CFG_LOG_PAGE_SZ(&__fs); + dump_page(&__fs, pix); + ERREXIT(); + return -1; } AREA(addr + i) &= src[i]; }