IOTSTOR-953: Fix address calculations from SlicingBlockDevice #11797

SeppoTakalo · 2019-11-01T13:11:09Z

Description (required)

IOTSTOR-953: Fix address calculations from SlicingBlockDevice

Change MBED_ASSERTS() to return valid error code, so that
checks are not bypassed on release builds.
Fix starting address calculations so that "addr" parameter is always
relative to SlicingDevice and "_start" is only added when calls to
underlying storage block is made.
Bypass BlockDevice:is_valid_*() to underlying block device.
Slicingblockdevice was just verifying addresses independently, without
verifying those from underlying block storage.
Add module tests for SlicingBlockDevice to verify the problem.

Summary of change (What the change is for and why)

Fix address calculations from SlicingBlockDevice.

This fixed #11781

Documentation (Details of any document updates required)

No documentation update.
No API changes.

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

Reviewers (optional)

@VeijoPesonen

Release Notes (required for feature/major PRs)

Summary of changes

Impact of changes

Migration actions required

SeppoTakalo · 2019-11-01T13:12:35Z

Test results before the last commit:

49: Test command: /Users/septak01/src/mbed-os/BUILD/moduletests-storage-blockdevice-SlicingBlockDevice
49: Test timeout computed to be: 1500
49: Running main() from gmock_main.cc
49: [==========] Running 4 tests from 1 test case.
49: [----------] Global test environment set-up.
49: [----------] 4 tests from SlicingBlockModuleTest
49: [ RUN      ] SlicingBlockModuleTest.constructor
49: [       OK ] SlicingBlockModuleTest.constructor (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.slice_in_middle
49: [       OK ] SlicingBlockModuleTest.slice_in_middle (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.slice_at_the_end
49: [       OK ] SlicingBlockModuleTest.slice_at_the_end (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.over_write
49: mbed assertation failed: is_valid_program(addr + _start, size), file: /Users/septak01/src/mbed-os/features/storage/blockdevice/SlicingBlockDevice.cpp, line 84
49: /Users/septak01/src/mbed-os/UNITTESTS/moduletests/storage/blockdevice/SlicingBlockDevice/moduletest.cpp:142: Failure
49: Expected equality of these values:
49:   slice.program(program, 2*(512), (512))
49:     Which is: 0
49:   BD_ERROR_DEVICE_ERROR
49:     Which is: -4001
49: [  FAILED  ] SlicingBlockModuleTest.over_write (0 ms)
49: [----------] 4 tests from SlicingBlockModuleTest (1 ms total)
49:
49: [----------] Global test environment tear-down
49: [==========] 4 tests from 1 test case ran. (1 ms total)
49: [  PASSED  ] 3 tests.
49: [  FAILED  ] 1 test, listed below:
49: [  FAILED  ] SlicingBlockModuleTest.over_write
49:
49:  1 FAILED TEST
2/2 Test #49: moduletests-storage-blockdevice-SlicingBlockDevice ...***Failed    0.21 sec

The following tests passed:
    features-storage-blockdevice-HeapBlockDevice

50% tests passed, 1 tests failed out of 2

Total Test time (real) =   0.63 sec

The following tests FAILED:
     49 - moduletests-storage-blockdevice-SlicingBlockDevice (Failed)
Errors while running CTest
make: *** [test] Error 8

SeppoTakalo · 2019-11-01T13:13:36Z

After the last commit

49: Test command: /Users/septak01/src/mbed-os/BUILD/moduletests-storage-blockdevice-SlicingBlockDevice
49: Test timeout computed to be: 1500
49: Running main() from gmock_main.cc
49: [==========] Running 4 tests from 1 test case.
49: [----------] Global test environment set-up.
49: [----------] 4 tests from SlicingBlockModuleTest
49: [ RUN      ] SlicingBlockModuleTest.constructor
49: [       OK ] SlicingBlockModuleTest.constructor (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.slice_in_middle
49: [       OK ] SlicingBlockModuleTest.slice_in_middle (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.slice_at_the_end
49: [       OK ] SlicingBlockModuleTest.slice_at_the_end (0 ms)
49: [ RUN      ] SlicingBlockModuleTest.over_write
49: [       OK ] SlicingBlockModuleTest.over_write (0 ms)
49: [----------] 4 tests from SlicingBlockModuleTest (0 ms total)
49:
49: [----------] Global test environment tear-down
49: [==========] 4 tests from 1 test case ran. (1 ms total)
49: [  PASSED  ] 4 tests.
49/50 Test #49: moduletests-storage-blockdevice-SlicingBlockDevice ........   Passed    0.23 sec

ciarmcom · 2019-11-01T14:00:27Z

@SeppoTakalo, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

UNITTESTS/features/storage/blockdevice/HeapBlockDevice/test.cpp

UNITTESTS/moduletests/storage/blockdevice/SlicingBlockDevice/moduletest.cpp

SeppoTakalo · 2019-11-04T08:57:49Z

Just realized that I have used wrong Jira ticket number.. This is IOTSTOR-953, not 941.

VeijoPesonen

Looks good to me

0xc0170

Mainly cosmetic changes requested around licenses

UNITTESTS/empty_baseline/empty_baseline.cpp

UNITTESTS/target_h/rtos/Semaphore.h

components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp

kjbracey · 2019-11-04T11:32:05Z

components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp

@@ -874,7 +873,7 @@ int SPIFBlockDevice::_sfdp_detect_best_bus_read_mode(uint8_t *basic_param_table_
                read_inst = basic_param_table_ptr[SPIF_BASIC_PARAM_TABLE_222_READ_INST_BYTE];
                _read_dummy_and_mode_cycles = (basic_param_table_ptr[SPIF_BASIC_PARAM_TABLE_222_READ_INST_BYTE - 1] >> 5)
                                         + (basic_param_table_ptr[SPIF_BASIC_PARAM_TABLE_222_READ_INST_BYTE - 1] & 0x1F);
-                debug_if(MBED_CONF_SPIF_DRIVER_DEBUG, "\nDEBUG: Read Bus Mode set to 2-2-2, Instruction: 0x%xh", read_inst);
+                tr_debug("\nRead Bus Mode set to 2-2-2, Instruction: 0x%xh", read_inst);


Can search-and-replace a bit harder for \n. (Can't immediately see why this was ever at the start of line.)

You are commenting a line, that has been commented out from the code..

I should probably drop the whole block of commented out code. I did a search-and-replace but only for active lines.

mbed-ci · 2019-11-04T12:49:23Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

"--coverage" is synonym to "-fprofile-arcs -ftest-coverage" https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html This was causing extra compilation warnings on CLANG.

* Refactor some headers to use relative path from Mbed OS root. * Refactor some data types to compile on 64bit machines. * Refactor some debug traces to use mbed_trace.

This uses HeapBlockDevice for providing the underlying storage block. Check boundaries that slicingblockdevice do not overlow over to unassigned blocks.

* Change MBED_ASSERTS() to return valid error code, so that checks are not bypassed on release builds. * Fix starting address calculations so that "addr" parameter is always relative to SlicingDevice and "_start" is only added when calls to underlying storage block is made. * Bypass BlockDevice:is_valid_*() to underlying block device. Slicingblockdevice was just verifying addresses independently, without verifying those from underlying block storage.

DeviceKey was using mbedtls_ssl_safer_memcmp() to compare result against buffer of equal length, but zero content. This comparison makes no sense as the entropy function is already returning proper error, if it fails.

SeppoTakalo · 2019-11-04T14:28:19Z

Rebased on top of master, should fix the conflicts.

0xc0170 · 2019-11-04T14:51:12Z

Ci restarted

mbed-ci · 2019-11-04T16:05:23Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

SeppoTakalo requested a review from VeijoPesonen November 1, 2019 13:11

ciarmcom requested review from a team November 1, 2019 14:00

ciarmcom added the needs: review label Nov 1, 2019

VeijoPesonen suggested changes Nov 1, 2019

View reviewed changes

SeppoTakalo force-pushed the IOTSTOR-941 branch 2 times, most recently from 5f437d6 to ab538f5 Compare November 1, 2019 17:07

VeijoPesonen approved these changes Nov 2, 2019

View reviewed changes

SeppoTakalo force-pushed the IOTSTOR-941 branch from ab538f5 to 6f126f6 Compare November 4, 2019 08:57

SeppoTakalo changed the title ~~IOTSTOR-941: Fix address calculations from SlicingBlockDevice~~ IOTSTOR-953: Fix address calculations from SlicingBlockDevice Nov 4, 2019

SeppoTakalo removed the request for review from a team November 4, 2019 09:01

VeijoPesonen approved these changes Nov 4, 2019

View reviewed changes

SeppoTakalo added needs: CI and removed needs: review labels Nov 4, 2019

0xc0170 requested changes Nov 4, 2019

View reviewed changes

UNITTESTS/empty_baseline/empty_baseline.cpp Show resolved Hide resolved

UNITTESTS/target_h/rtos/Semaphore.h Outdated Show resolved Hide resolved

0xc0170 added needs: work release-version: 5.14.2 and removed needs: CI labels Nov 4, 2019

kjbracey reviewed Nov 4, 2019

View reviewed changes

components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp Show resolved Hide resolved

SeppoTakalo force-pushed the IOTSTOR-941 branch from 6f126f6 to a2ffa45 Compare November 4, 2019 11:17

SeppoTakalo added needs: review and removed needs: work labels Nov 4, 2019

kjbracey reviewed Nov 4, 2019

View reviewed changes

Remove extra GCC arquements.

82045b0

"--coverage" is synonym to "-fprofile-arcs -ftest-coverage" https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html This was causing extra compilation warnings on CLANG.

Seppo Takalo added 10 commits November 4, 2019 16:12

Add enough stubs to allow drivers/* to compile.

376bbfc

Add empty unittest to generate zero baseline for untested files.

7f089ac

Add storage related files to baseline unittest.

168b51c

* Refactor some headers to use relative path from Mbed OS root. * Refactor some data types to compile on 64bit machines. * Refactor some debug traces to use mbed_trace.

Add some functionality to atomic stubs

31fbed9

Add unittest for HeapBlockDevice and change some MBED_ASSERTS to errors.

a6d8f74

Add moduletest for SlicingBlockDevice

333b281

This uses HeapBlockDevice for providing the underlying storage block. Check boundaries that slicingblockdevice do not overlow over to unassigned blocks.

Remove usage of Mbed TLS internal function from DeviceKey.

f0790cd

DeviceKey was using mbedtls_ssl_safer_memcmp() to compare result against buffer of equal length, but zero content. This comparison makes no sense as the entropy function is already returning proper error, if it fails.

Remove linefeeds from debug prints

ccb7738

Remove commented-out code

21987db

0xc0170 self-requested a review November 4, 2019 14:23

SeppoTakalo force-pushed the IOTSTOR-941 branch from a734d9a to 21987db Compare November 4, 2019 14:27

0xc0170 approved these changes Nov 4, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Nov 4, 2019

VeijoPesonen approved these changes Nov 5, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Nov 5, 2019

0xc0170 merged commit 355336c into master Nov 5, 2019

0xc0170 removed the ready for merge label Nov 5, 2019

SeppoTakalo deleted the IOTSTOR-941 branch November 5, 2019 11:15

kjbracey mentioned this pull request Nov 13, 2019

MbedCRC and CRC HAL revisions #11559

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IOTSTOR-953: Fix address calculations from SlicingBlockDevice #11797

IOTSTOR-953: Fix address calculations from SlicingBlockDevice #11797

SeppoTakalo commented Nov 1, 2019 •

edited

SeppoTakalo commented Nov 1, 2019

SeppoTakalo commented Nov 1, 2019

ciarmcom commented Nov 1, 2019

SeppoTakalo commented Nov 4, 2019

VeijoPesonen left a comment

0xc0170 left a comment

kjbracey Nov 4, 2019

SeppoTakalo Nov 4, 2019

mbed-ci commented Nov 4, 2019

SeppoTakalo commented Nov 4, 2019

0xc0170 commented Nov 4, 2019

mbed-ci commented Nov 4, 2019

IOTSTOR-953: Fix address calculations from SlicingBlockDevice #11797

IOTSTOR-953: Fix address calculations from SlicingBlockDevice #11797

Conversation

SeppoTakalo commented Nov 1, 2019 • edited

Description (required)

Summary of change (What the change is for and why)

Documentation (Details of any document updates required)

Pull request type (required)

Test results (required)

Reviewers (optional)

@VeijoPesonen

Release Notes (required for feature/major PRs)

Summary of changes

Impact of changes

Migration actions required

SeppoTakalo commented Nov 1, 2019

SeppoTakalo commented Nov 1, 2019

ciarmcom commented Nov 1, 2019

SeppoTakalo commented Nov 4, 2019

VeijoPesonen left a comment

Choose a reason for hiding this comment

0xc0170 left a comment

Choose a reason for hiding this comment

kjbracey Nov 4, 2019

Choose a reason for hiding this comment

SeppoTakalo Nov 4, 2019

Choose a reason for hiding this comment

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

SeppoTakalo commented Nov 4, 2019

0xc0170 commented Nov 4, 2019

mbed-ci commented Nov 4, 2019

Test run: SUCCESS

SeppoTakalo commented Nov 1, 2019 •

edited