avoid Read-While-Write errors #2117
Conversation
Refer to https://github.com/ARMmbed/configuration-store/issues/21 for more background information. Read-while-write errors may occur when there is a read/fetch on a bank of internal flash while an erase/program operation to the same bank is active. The possibility of read-while-write errors depends on the application's span of internal-flash. In the trivial (and also the default) case when this boundary is set to lie at the junction between the two banks of internal flash (i.e. BLOCK1_START_ADDR), there is *no* possibility of read-while-write. In the more common case, when the system doesn't reserve all of BLOCK1 for the flash driver, there exists the possibility of read-while-write. To avoid this problem for the common case (above), we enforce the following: - Force synchronous mode of execution in the storage_driver. - Disable interrupts during erase/program operations. This prevents the execution of arbitrary user-code which might access flash. - Ensure all code and data structures used in the storage driver execute out of RAM. Refer to __RAMFUNC which allows for this. We assume that CONFIG_HARDWARE_MTD_START_ADDR is the boundary between application and this driver.
| /* define __RAMFUNC to put a declaration in the .data section--i.e. the | ||
| * initialized data section. This will be copied into RAM automatically by the | ||
| * startup sequence. */ | ||
| #define __RAMFUNC __attribute__ ((section (".data#"))) /* The '#' following ".data" needs a bit of |
There was a problem hiding this comment.
Can you not just make it ".data.ramfunc", rather than putting in ".data"? GCC linker script puts all ".data*" sections in the relevant area.
Actually does this even work for ARMCC? The comment hack certainly won't. Naming a separate area also won't, as it will just be a read-only one. You might need to change the linker map, so a specific named read-only section gets included in the RW_m_data map area.
There was a problem hiding this comment.
using .data.ramfunc produces the following warning:
/tmp/cc0IYu2K.s: Assembler messages:
/tmp/cc0IYu2K.s:19: Warning: setting incorrect section attributes for .data.ramfunc
There was a problem hiding this comment.
using .data.ramfunc # doesn't do any better:
/tmp/ccyw1VRL.s: Assembler messages:
/tmp/ccyw1VRL.s:19: Warning: setting incorrect section attributes for .data.ramfunc#
There was a problem hiding this comment.
Well, it's kind of right. It's not really data. It may be you should do what I already think you need to do for ARMCC, which is have a custom area name, which will then need to be added to the linker map. (eg ".ramfuncs" or something).
What is the ARMCC status?
There was a problem hiding this comment.
yes, I need to add a section like .ramfuncs to support ARMCC. Working on that.
|
Patch seems to do what would be necessary. Not convinced that the RAM relocation on ARMCC is functional, or that it's done as neatly as it could be on GCC. Only real question is - do we want to do this at all? Do we allow this routine to disable interrupts for however long it is? (4ms by Rohit's measurements, 15ms typical according to datasheet, increasing to max 115ms with age) My vote is no. We should not be attempting to share BLOCK1 for program and cfstore. The stability cost from the interrupt disabling is too high. I'd only find it acceptable if we knew "flush" was only called on system shutdown/reboot, not in normal operation. |
|
@kjbracey-arm please review the updates |
| @@ -202,6 +312,14 @@ static const ARM_DRIVER_VERSION version = { | |||
| .drv = ARM_DRIVER_VERSION_MAJOR_MINOR(1,00) | |||
| }; | |||
|
|
|||
|
|
|||
| #if ((!defined(CONFIG_HARDWARE_MTD_ASYNC_OPS) || CONFIG_HARDWARE_MTD_ASYNC_OPS) && \ | |||
There was a problem hiding this comment.
configuration_store.c uses the following symbol for enabling async ops: STORAGE_DRIVER_CONFIG_HARDWARE_MTD_ASYNC_OPS
We should adopt a common symbol that makes sense across both mbed-os configuration-store and mbedmicro/mbed. I suggest :
- prepending STORAGE_ to make it clear this is part of the storage feature
- prepending DRIVER_ to make it clear this is enabling a feature of the STORAGE_DRIVER, which is an agreed name.
- adopting this symbols as I've already communicated to developers this symbol needs to be on the mbed compile command line for building sync mode CFSTORE/Flash Journal
There was a problem hiding this comment.
Thanks @simonqhughes. I'll switch to using this naming scheme for configs
|
Hi I ran the CFSTORE & flash-journal basicAPI test cases and these are the results: Please can we fix basicAPI test? There also used to be a flash journal stress test in the tree but I'm unable to find it now. Has it been renamed? |
|
This follows from my memcpy note - the overall change set is massively overdone. All the context modifications are unnecessary. No context info needs to be accessed during the actual flash operation. The flash operation only lasts from launchCommand to "not busy". A hand-coded RAM-resident "setAndWaitForReadHigh" routine to play with the CCIF bit is all you need, as far as I can tell. |
|
@kjbracey-arm the context related changes are orthogonal. They would have gone into this driver independently of this issue. As stated in the original review request:
|
|
I feel that there is benefit in splitting away from this pull request changes which are not directly relevant to the original read-while-write issue. I am considering re-submitting this as separate pull requests. |
|
I'll resubmit an updated version of this work in separate pull requests. At the very least, changes unrelated to the read-while-write issue can be reviewed independently. |
|
#2121: I've submitted an initial PR to update the K64F Storage driver. This brings in improvements which have been collected over the recent weeks. |
|
the second (and final) PR from this work: #2150 |
…..3c7d50e 3c7d50e Remove test files d5f5bee Merge branch 'release_internal' into release_external 72b065b Merge pull request ARMmbed#2133 from ARMmbed/few_updates 1da0b9f Fix unittest cleanup d173249 Adjust GC cleanup threshold and traces 26166d1 Remove ns_sw_mac_packet_ingress_rate_limit_by_mem (ARMmbed#2132) 5305754 Merge pull request ARMmbed#2130 from ARMmbed/IOTTHD-3419 038bc2e Copy ingress rate limiting API to ns_conf 940b516 Fix compiler warnings 64e6ff3 Review corrections 01e7d84 Update GC thresholds, init and traces 1acd1cc Add unit tests for monitor 64c969e Nanostack heap garbage collection d3330b2 Fix errors found by Coverity (ARMmbed#2131) da5d2c7 MAC, RPL and ETX trace clean. eaf8907 Limit amount of incoming packet based on memory (ARMmbed#2128) 0c2b383 Fixed unitest header size compare value for support brodacst shedule. 6c70262 Wi-sun LLC update 7c57343 WS PAN Config handler update 161421b Wi-sun PAN_VERSION lifetime and timeout update 43083ed Fixed broken wi-sun neigbour black list filtering. 0992ee2 Update Pan information data from all selected parent. cafc142 Fixed Pan advertisment route cost comapre for consistent and incosistent db81d02 SW MAC timestamp read update eddf91b Wi-sun neighbor generate limitation 4d6abb3 Added 15 second guarantee time for packet handler before remove link. 9ed97eb SW MAC new API for read current timestamp 6a44829 Stop advertisment RPL prefix if we not have a address and it is not 'A-flag' d37ce6a Revert "Wi-sun dublicate MPX ID filter support" d80ebf8 Fix debug trace format. b1ef0f6 Removed Address reg pending and rady mask from address. ce672ba Added trace for debugging DHCPv6 failure reason. aaf2b39 Wi-sun dublicate MPX ID filter support bd51f9f Merge pull request ARMmbed#2117 from ARMmbed/IOTTHD-3587 b016d52 Fixed DHCPv6 client delete when address was removed to new network discovery. 69fb24b Wi-sun address registration and RPL update 92d3a92 RPL: trace new preferred parent 9a6e4e0 disable multicast NS for wisun 6e13d81 Merge pull request ARMmbed#2113 from ARMmbed/IOTTHD-3577 85aaae7 Refactored the Wi-SUN BBR logic according to design 61f6f5b WS: Added ws stats empty functions ac191c3 Removed link local address verifycation and dead code. 77076d1 Negative ARO timeout use same timeout than not trusted device. c4e8735 Wi-sun DHCP solicit max to 15min from 60min. 66615e6 Updated wi-sun BBR min hop rank increase to 196. 9ce41f1 Pendig address registration will move DAO send. f54ea6b Merge pull request ARMmbed#2110 from ARMmbed/IOTTHD-3577 18dbac2 WS: Implemented ws statistics 3ce95fa FHSS WS: Fixed drift compensation stats b878bd9 KMP address update 7f18afe PAE controller and NVM update f692eb8 Wi-SUN border router configure update 386e5ff Wi-sun Update 05b1fe8 do not send periodic DIO messages if DAO registration is not done 6acee47 modified RSL calculation and value in messaging ee7f218 Modify discovery start timing 1ba806d Wi-SUN NUD send fix 31fb8cd Moved counter config to config.h cf18063 Added storing of MAC frame counter to NVM 68adb36 Neighbor cache update 8cdd961 Added possibility for Update DHCPv6 client server address. 2dfa536 Merge pull request ARMmbed#2099 from ARMmbed/IOTTHD-3231 8dc200b FHSS: Created statistics for WS 5e67f7c RPL update bbae493 wi-sun RPL param update e8567d7 MAC neighbour remove and add trace simplify. 8bb4ab5 Cleaned unnessary debug trace. 3608153 DIO prefix handler update 332735b Disabled Version number periodic update if Dodag max rank inrease is not 0. 940de0b Wi-SUN setup update: c1a89e5 Merge pull request ARMmbed#2095 from ARMmbed/IOTTHD-3474 f6d81b5 Review corrections 7487ca1 Fix clang-6.0 build error and warnings cce3fc7 Security protocols are no longer started second time on authenticator git-subtree-dir: features/nanostack/sal-stack-nanostack git-subtree-split: 3c7d50e
fixes configuration-store/#21: avoid Read-While-Write errors
Refer to https://github.com/ARMmbed/configuration-store/issues/21 for more
background information.
Read-while-write errors may occur when there is a read/fetch on a bank of
internal flash while an erase/program operation to the same bank is active.
A read/fetch against internal flash can arise arbitrarily from program activity.
The possibility of read-while-write collision errors depends on the application's span
of internal-flash.
In the trivial (and also the default) case when this boundary is set to lie at
the junction between the two banks of internal flash (i.e. BLOCK1_START_ADDR),
there is no possibility of read-while-write. In the more common case, when
the system doesn't reserve all of BLOCK1 for the flash driver, there exists
the possibility of read-while-write.
To avoid this problem for the common case (above), we enforce the following:
execution of arbitrary user-code which might access flash.
out of RAM. Refer to __RAMFUNC which allows for this.
We assume that the build-time parameter
CONFIG_HARDWARE_MTD_START_ADDRis the boundary between application and this driver. User needs to define this parameter to a value that lies after the end of their application's use of internal flash. This is important.CONFIG_HARDWARE_MTD_SIZEmay also need to be updated ifCONFIG_HARDWARE_MTD_START_ADDRis set.please review: @kjbracey-arm [you need to review only the commit 7810829; other commits are generic]
cc: @simonqhughes @meriac
notify: @marcuschangarm @LiyouZhou