Skip to content
New issue

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

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

Already on GitHub? # to your account

lfs: correct alignment restriction on lookahead buffer #253

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

pabigot
Copy link
Contributor

@pabigot pabigot commented Jul 31, 2019

The buffer need only be 32-bit aligned.

Signed-off-by: Peter A. Bigot pab@pabigot.com

The buffer need only be 32-bit aligned.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
pabigot added a commit to pabigot/zephyr-littlefs that referenced this pull request Aug 2, 2019
Zephyr wrapper uses 32-bit alignment which is all that's needed.  Remove
the outdated upstream assert.

Upstream: littlefs-project/littlefs#253
Signed-off-by: Peter A. Bigot <pab@pabigot.com>
pabigot added a commit to pabigot/zephyr-littlefs that referenced this pull request Aug 2, 2019
Zephyr wrapper uses 32-bit alignment which is all that's needed.  Remove
the outdated upstream assert.

Upstream: littlefs-project/littlefs#253
Signed-off-by: Peter A. Bigot <pab@pabigot.com>
@geky
Copy link
Member

geky commented Aug 5, 2019

Hi @pabigot, thanks for creating a pr!

This was discussed over here: #134, though I realize it's not easy to find and should be documented in lfs.h.

One of the possible solutions for #75 involves storing pairs of pointers (address+size pairs). The lookahead buffer would then need to be a multiple of 64-bits.

Well, it doesn't need to be, but any unaligned size would be wasted. It's cheaper and more transparent to the user to present this as a strict requirement to the user.

If we don't adopt this solution for #75, we can just remove the 64-bit requirement. But adopting it early makes it possible to introduce without breaking compatibility.

It's not needed yet but the alignment is reserved as it is likely it will be needed to solve #75. If it isn't we can remove the assert.

@pabigot
Copy link
Contributor Author

pabigot commented Aug 5, 2019

@geky I'm confused again. I thought with #239 you agreed: yes, the length of the lookahead array needs to be a multiple of 8 bytes, but even with the proposed solution of pairs of address and size the alignment of the array is to 4-byte values.

So it is documented in lfs.h. I hadn't realized the code enforced the too-strong alignment until I finally ran into the situation.

@geky
Copy link
Member

geky commented Aug 5, 2019

Ah! You're right, I'm getting confused again.

After some more coffee this looks great 👍

pabigot added a commit to zephyrproject-rtos/littlefs that referenced this pull request Aug 6, 2019
The buffer need only be 32-bit aligned.

Upstream-Status: Submitted [littlefs-project/littlefs#253]
Signed-off-by: Peter A. Bigot <pab@pabigot.com>
@geky geky merged commit 9d1f121 into littlefs-project:master Aug 8, 2019
@pabigot pabigot deleted the pr/20190730a branch August 11, 2019 15:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants