Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent sector-unaligned erase #7952

Merged
merged 2 commits into from
Sep 3, 2018
Merged

Prevent sector-unaligned erase #7952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b71db49
Prevent sector-unaligned erase
Sep 2, 2018
df36f0e
Fixed PR review remarks
Sep 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,12 +339,22 @@ int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
int size = (int)in_size;
bool erase_failed = false;
int status = SPIF_BD_ERROR_OK;
// Find region of erased address
// Find region of erased address
int region = _utils_find_addr_region(addr);
// Erase Types of selected region
uint8_t bitfield = _region_erase_types_bitfield[region];

tr_error("DEBUG: erase - addr: %llu, in_size: %llu", addr, in_size);
tr_info("DEBUG: erase - addr: %llu, in_size: %llu", addr, in_size);

if ((addr + in_size) > _device_size_bytes) {
tr_error("ERROR: erase exceeds flash device size");
return SPIF_BD_ERROR_INVALID_ERASE_PARAMS;
}

if ( ((addr % get_erase_size(addr)) != 0 ) || (((addr + in_size) % get_erase_size(addr + in_size - 1)) != 0 ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the function is_valid_erase work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_valid_erase wont work because it relies on a single erase-sector size.
This supports variable erase-sector size according to address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This means that it has a bug, which should be fixed (but not in this PR of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidsaada - I think not all Block Devices support variable sector-erase size. You're right, it will require a separate PR, probably updating BlockDevice.h base class and maybe some inheriting block devices.

tr_error("ERROR: invalid erase - unaligned address and size");
return SPIF_BD_ERROR_INVALID_ERASE_PARAMS;
}

// For each iteration erase the largest section supported by current region
while (size > 0) {
Expand All @@ -353,13 +363,12 @@ int SPIFBlockDevice::erase(bd_addr_t addr, bd_size_t in_size)
// find the matching instruction and erase size chunk for that type.
type = _utils_iterate_next_largest_erase_type(bitfield, size, (unsigned int)addr, _region_high_boundary[region]);
cur_erase_inst = _erase_type_inst_arr[type];
//chunk = _erase_type_size_arr[type];
offset = addr % _erase_type_size_arr[type];
chunk = ( (offset + size) < _erase_type_size_arr[type]) ? size : (_erase_type_size_arr[type] - offset);

tr_error("DEBUG: erase - addr: %llu, size:%d, Inst: 0x%xh, chunk: %lu , ",
tr_debug("DEBUG: erase - addr: %llu, size:%d, Inst: 0x%xh, chunk: %lu , ",
addr, size, cur_erase_inst, chunk);
tr_error("DEBUG: erase - Region: %d, Type:%d",
tr_debug("DEBUG: erase - Region: %d, Type:%d",
region, type);

_mutex->lock();
Expand Down
12 changes: 7 additions & 5 deletions components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@
* @enum spif_bd_error
*/
enum spif_bd_error {
SPIF_BD_ERROR_OK = 0, /*!< no error */
SPIF_BD_ERROR_DEVICE_ERROR = BD_ERROR_DEVICE_ERROR, /*!< device specific error -4001 */
SPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */
SPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */
SPIF_BD_ERROR_WREN_FAILED = -4004, /* Write Enable Failed */
SPIF_BD_ERROR_OK = 0, /*!< no error */
SPIF_BD_ERROR_DEVICE_ERROR = BD_ERROR_DEVICE_ERROR, /*!< device specific error -4001 */
SPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */
SPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */
SPIF_BD_ERROR_WREN_FAILED = -4004, /* Write Enable Failed */
SPIF_BD_ERROR_INVALID_ERASE_PARAMS = -4005, /* Erase command not on sector aligned addresses or exceeds device size */
};


Expand Down Expand Up @@ -136,6 +137,7 @@ class SPIFBlockDevice : public BlockDevice {
* SPIF_BD_ERROR_DEVICE_ERROR - device driver transaction failed
* SPIF_BD_ERROR_READY_FAILED - Waiting for Memory ready failed or timed out
* SPIF_BD_ERROR_WREN_FAILED - Write Enable failed
* SPIF_BD_ERROR_INVALID_ERASE_PARAMS - Trying to erase unaligned address or size
*/
virtual int erase(bd_addr_t addr, bd_size_t size);

Expand Down
132 changes: 60 additions & 72 deletions components/storage/blockdevice/COMPONENT_SPIF/TESTS/block_device/spif/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@ static SingletonPtr<PlatformMutex> _mutex;
// Mutex is protecting rand() per srand for buffer writing and verification.
// Mutex is also protecting printouts for clear logs.
// Mutex is NOT protecting Block Device actions: erase/program/read - which is the purpose of the multithreaded test!
void basic_erase_program_read_test(SPIFBlockDevice& blockD, bd_size_t block_size, uint8_t *write_block,
void basic_erase_program_read_test(SPIFBlockDevice& block_device, bd_size_t block_size, uint8_t *write_block,
uint8_t *read_block, unsigned addrwidth)
{
int err = 0;
_mutex->lock();
// Find a random block
bd_addr_t block = (rand() * block_size) % blockD.size();
bd_addr_t block = (rand() * block_size) % block_device.size();

// Use next random number as temporary seed to keep
// the address progressing in the pseudorandom sequence
Expand All @@ -62,13 +62,13 @@ void basic_erase_program_read_test(SPIFBlockDevice& blockD, bd_size_t block_size
utest_printf("\ntest %0*llx:%llu...", addrwidth, block, block_size);
_mutex->unlock();

err = blockD.erase(block, block_size);
err = block_device.erase(block, block_size);
TEST_ASSERT_EQUAL(0, err);

err = blockD.program(write_block, block, block_size);
err = block_device.program(write_block, block, block_size);
TEST_ASSERT_EQUAL(0, err);

err = blockD.read(read_block, block, block_size);
err = block_device.read(read_block, block, block_size);
TEST_ASSERT_EQUAL(0, err);

_mutex->lock();
Expand All @@ -91,16 +91,17 @@ void test_spif_random_program_read_erase()
{
utest_printf("\nTest Random Program Read Erase Starts..\n");

SPIFBlockDevice blockD(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO, MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);
SPIFBlockDevice block_device(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO,
MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);

int err = blockD.init();
int err = block_device.init();
TEST_ASSERT_EQUAL(0, err);

for (unsigned atr = 0; atr < sizeof(ATTRS) / sizeof(ATTRS[0]); atr++) {
static const char *prefixes[] = {"", "k", "M", "G"};
for (int i_ind = 3; i_ind >= 0; i_ind--) {
bd_size_t size = (blockD.*ATTRS[atr].method)();
bd_size_t size = (block_device.*ATTRS[atr].method)();
if (size >= (1ULL << 10 * i_ind)) {
utest_printf("%s: %llu%sbytes (%llubytes)\n",
ATTRS[atr].name, size >> 10 * i_ind, prefixes[i_ind], size);
Expand All @@ -109,8 +110,8 @@ void test_spif_random_program_read_erase()
}
}

bd_size_t block_size = blockD.get_erase_size();
unsigned addrwidth = ceil(log(float(blockD.size() - 1)) / log(float(16))) + 1;
bd_size_t block_size = block_device.get_erase_size();
unsigned addrwidth = ceil(log(float(block_device.size() - 1)) / log(float(16))) + 1;

uint8_t *write_block = new (std::nothrow) uint8_t[block_size];
uint8_t *read_block = new (std::nothrow) uint8_t[block_size];
Expand All @@ -120,31 +121,32 @@ void test_spif_random_program_read_erase()
}

for (int b = 0; b < TEST_BLOCK_COUNT; b++) {
basic_erase_program_read_test(blockD, block_size, write_block, read_block, addrwidth);
basic_erase_program_read_test(block_device, block_size, write_block, read_block, addrwidth);
}

err = blockD.deinit();
err = block_device.deinit();
TEST_ASSERT_EQUAL(0, err);

end:
delete[] write_block;
delete[] read_block;
}

void test_spif_unaligned_program()
void test_spif_unaligned_erase()
{
utest_printf("\nTest Unaligned Program Starts..\n");
utest_printf("\nTest Unaligned Erase Starts..\n");

SPIFBlockDevice blockD(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO, MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);
SPIFBlockDevice block_device(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO,
MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);

int err = blockD.init();
int err = block_device.init();
TEST_ASSERT_EQUAL(0, err);

for (unsigned atr = 0; atr < sizeof(ATTRS) / sizeof(ATTRS[0]); atr++) {
static const char *prefixes[] = {"", "k", "M", "G"};
for (int i_ind = 3; i_ind >= 0; i_ind--) {
bd_size_t size = (blockD.*ATTRS[atr].method)();
bd_size_t size = (block_device.*ATTRS[atr].method)();
if (size >= (1ULL << 10 * i_ind)) {
utest_printf("%s: %llu%sbytes (%llubytes)\n",
ATTRS[atr].name, size >> 10 * i_ind, prefixes[i_ind], size);
Expand All @@ -153,68 +155,53 @@ void test_spif_unaligned_program()
}
}

bd_size_t block_size = blockD.get_erase_size();
unsigned addrwidth = ceil(log(float(blockD.size() - 1)) / log(float(16))) + 1;
bd_addr_t addr = 0;
bd_size_t sector_erase_size = block_device.get_erase_size(addr);
unsigned addrwidth = ceil(log(float(block_device.size() - 1)) / log(float(16))) + 1;

uint8_t *write_block = new (std::nothrow) uint8_t[block_size];
uint8_t *read_block = new (std::nothrow) uint8_t[block_size];
if (!write_block || !read_block ) {
utest_printf("\n Not enough memory for test");
goto end;
}

{
bd_addr_t block = (rand() * block_size) % blockD.size() + 1;
utest_printf("\ntest %0*llx:%llu...", addrwidth, addr, sector_erase_size);

// Use next random number as temporary seed to keep
// the address progressing in the pseudorandom sequence
unsigned seed = rand();
//unaligned start address
addr += 1;
err = block_device.erase(addr, sector_erase_size - 1);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

// Fill with random sequence
srand(seed);
for (bd_size_t i_ind = 0; i_ind < block_size; i_ind++) {
write_block[i_ind] = 0xff & rand();
}
err = block_device.erase(addr, sector_erase_size);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

// Write, sync, and read the block
utest_printf("\ntest %0*llx:%llu...", addrwidth, block, block_size);
err = block_device.erase(addr, 1);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

err = blockD.erase(block, block_size);
TEST_ASSERT_EQUAL(0, err);
//unaligned end address
addr = 0;

//err = blockD.erase(block+4096, block_size);
//TEST_ASSERT_EQUAL(0, err);
err = block_device.erase(addr, 1);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

err = blockD.program(write_block, block, block_size);
TEST_ASSERT_EQUAL(0, err);
err = block_device.erase(addr, sector_erase_size + 1);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

err = blockD.read(read_block, block, block_size);
TEST_ASSERT_EQUAL(0, err);
//erase size exceeds flash device size
err = block_device.erase(addr, block_device.size() + 1);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_INVALID_ERASE_PARAMS, err);

// Check that the data was unmodified
srand(seed);
for (bd_size_t i_ind = 0; i_ind < block_size; i_ind++) {
//printf("index %d\n", i_ind);
TEST_ASSERT_EQUAL(0xff & rand(), read_block[i_ind]);
}
// Valid erase
err = block_device.erase(addr, sector_erase_size);
TEST_ASSERT_EQUAL(SPIF_BD_ERROR_OK, err);

err = blockD.deinit();
TEST_ASSERT_EQUAL(0, err);
}
end:
delete[] write_block;
delete[] read_block;
err = block_device.deinit();
TEST_ASSERT_EQUAL(0, err);
}

static void test_spif_thread_job(void *vBlockD/*, int thread_num*/)
static void test_spif_thread_job(void *block_device_ptr/*, int thread_num*/)
{
static int thread_num = 0;
thread_num++;
SPIFBlockDevice *blockD = (SPIFBlockDevice *)vBlockD;
SPIFBlockDevice *block_device = (SPIFBlockDevice *)block_device_ptr;
utest_printf("\n Thread %d Started \n", thread_num);

bd_size_t block_size = blockD->get_erase_size();
unsigned addrwidth = ceil(log(float(blockD->size() - 1)) / log(float(16))) + 1;
bd_size_t block_size = block_device->get_erase_size();
unsigned addrwidth = ceil(log(float(block_device->size() - 1)) / log(float(16))) + 1;

uint8_t *write_block = new (std::nothrow) uint8_t[block_size];
uint8_t *read_block = new (std::nothrow) uint8_t[block_size];
Expand All @@ -224,7 +211,7 @@ static void test_spif_thread_job(void *vBlockD/*, int thread_num*/)
}

for (int b = 0; b < TEST_BLOCK_COUNT; b++) {
basic_erase_program_read_test((*blockD), block_size, write_block, read_block, addrwidth);
basic_erase_program_read_test((*block_device), block_size, write_block, read_block, addrwidth);
}

end:
Expand All @@ -236,16 +223,17 @@ void test_spif_multi_threads()
{
utest_printf("\nTest Multi Threaded Erase/Program/Read Starts..\n");

SPIFBlockDevice blockD(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO, MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);
SPIFBlockDevice block_device(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO,
MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS);

int err = blockD.init();
int err = block_device.init();
TEST_ASSERT_EQUAL(0, err);

for (unsigned atr = 0; atr < sizeof(ATTRS) / sizeof(ATTRS[0]); atr++) {
static const char *prefixes[] = {"", "k", "M", "G"};
for (int i_ind = 3; i_ind >= 0; i_ind--) {
bd_size_t size = (blockD.*ATTRS[atr].method)();
bd_size_t size = (block_device.*ATTRS[atr].method)();
if (size >= (1ULL << 10 * i_ind)) {
utest_printf("%s: %llu%sbytes (%llubytes)\n",
ATTRS[atr].name, size >> 10 * i_ind, prefixes[i_ind], size);
Expand All @@ -260,7 +248,7 @@ void test_spif_multi_threads()
int i_ind;

for (i_ind = 0; i_ind < SPIF_TEST_NUM_OF_THREADS; i_ind++) {
threadStatus = spif_bd_thread[i_ind].start(test_spif_thread_job, (void *)&blockD);
threadStatus = spif_bd_thread[i_ind].start(test_spif_thread_job, (void *)&block_device);
if (threadStatus != 0) {
utest_printf("\n Thread %d Start Failed!", i_ind + 1);
}
Expand All @@ -270,7 +258,7 @@ void test_spif_multi_threads()
spif_bd_thread[i_ind].join();
}

err = blockD.deinit();
err = block_device.deinit();
TEST_ASSERT_EQUAL(0, err);
}

Expand All @@ -282,7 +270,7 @@ utest::v1::status_t test_setup(const size_t number_of_cases)
}

Case cases[] = {
Case("Testing unaligned program blocks", test_spif_unaligned_program),
Case("Testing unaligned erase blocks", test_spif_unaligned_erase),
Case("Testing read write random blocks", test_spif_random_program_read_erase),
Case("Testing Multi Threads Erase Program Read", test_spif_multi_threads)
};
Expand Down