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.

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

pkg/{littlefs,littlefs2}: fix unaligned memory accesses #18473

Merged
merged 2 commits into from
Aug 19, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Aug 18, 2022

Contribution description

This aligns the buffers in littlefs_desc_t / littlefs2_desc_t to fix hard faults on Cortex M0+ MCUs such as the samr21-xpro.

Testing procedure

  • make BOARD=samr21-xpro -C tests/pkg_littlefs
  • make BOARD=samr21-xpro -C tests/pkg_littlefs2

Issues/PRs references

Prior to #18141 the alignment requirement was met but luck. With this, the alignment requirement is now communicated to the compiler, which should fix future issues.

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 18, 2022
@maribu maribu requested a review from MrKevinWeiss August 18, 2022 16:40
@github-actions github-actions bot added the Area: sys Area: System label Aug 18, 2022
maribu added 2 commits August 18, 2022 18:53
Previously `tests/pkg_littlefs` crashed on the `samr21-xpro`. This
now aligns the buffers in `littlefs_desc_t` to the alignment
requirement of `uint32_t`.

Specifically the issue causing the crash at hand was that
`lfs_free_t::buffer` is of type `uint32_t *`, so access are expected
to be aligned to `uint32_t`. After this commit, this assumption is
fulfilled.
Previously `tests/pkg_littlefs2` crashed on the `samr21-xpro`. This
now aligns the buffers in `littlefs2_desc_t` to the alignment
requirement of `uint32_t`.

Specifically the issue causing the crash at hand was that
`struct lfs_free::buffer` is of type `uint32_t *`, so access are
expected to be aligned to `uint32_t`. After this commit, this
assumption is fulfilled.
@kaspar030
Copy link
Contributor

If it's an unaligned access, does it read/write whole words (or u16/u32)? In that case, wouldn't also a proper multiple be required, and thus probably a word (or u16/u32) typed buffer?

@kaspar030
Copy link
Contributor

Anyhow, nice catch!

@maribu
Copy link
Member Author

maribu commented Aug 18, 2022

The uint8_t * buffer is casted to an uint32_t * pointer somewhere in the littlefs code, which is than accessed word-wise. Since all casts to pointers with higher alignment requirements have been removed from RIOT (or properly documented and other means of enforcement of the alignment employed), I believe the culprit is the littlefs API is asking for uint8_t * in their APIs. I haven't really looked into it, though.

Likely it is worth to use just uint32_t buffers and update all APIs in the way. But let's have this quick fix merged in the meantime. Green nightlies would be nice again :)

@maribu
Copy link
Member Author

maribu commented Aug 19, 2022

The sole failure is a CI glitch:

-- running on worker mobi7 thread 8, build number 3118.
make: Entering directory '/tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/tests/cpp_exclude'
make: Leaving directory '/tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/tests/cpp_exclude'
make: Entering directory '/tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/tests/cpp_exclude'
Building application "tests_cpp_exclude" for "arduino-mkrzero" with MCU "samd21".

sha1sum /tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/tests/cpp_exclude/tests/01-run.py /tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/build/tests_cpp_exclude.bin > /tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/build/test-input-hash.sha1
   text	   data	    bss	    dec	    hex	filename
  24368	    140	   4752	  29260	   724c	/tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/build/tests_cpp_exclude.elf
make: Leaving directory '/tmp/dwq.0.6613085980014425/17075f86ebbd7c6850b7f011c9f4afb8/tests/cpp_exclude'
0a1,3
> [INFO] Kconfiglib not found - getting it
> git-cache: cloning from cache. tag=commit061e71f7d78cb057762d88de088055361863deff-1619566
> [INFO] Kconfiglib downloaded
{"build/": 1864}

@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2022
@MrKevinWeiss MrKevinWeiss merged commit 848911e into RIOT-OS:master Aug 19, 2022
@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Aug 19, 2022

Darn. Ya, if someone who knows make better than me wants to take a look at #18470 we can fix those glitches. I want to just silence those commands but still fetch the package.

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@maribu maribu deleted the pkg/littlefs branch January 21, 2023 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants