storage/stream: fix possible unaligned write on buffer flush request #25584

nvlsianpu · 2020-05-25T11:05:44Z

On buffer flush request it is very probably that write buffer
contains amount of data which is non write-block-size aligned.
Flash memory need to be write at minimal by write-block-size chunks.
This patch addresses mechanism which ensure such behavior by adding
missing bytes.

fixes #25471

Signed-off-by: Andrzej Puzdrowski andrzej.puzdrowski@nordicsemi.no

subsys/storage/stream/stream_flash.c

nandojve · 2020-05-25T12:31:20Z

I was wondering if a regression test for this would not be good.

zephyrbot · 2020-05-25T12:36:53Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:61: WARNING:LONG_LINE_STRING: line over 80 characters
#61: FILE: subsys/storage/stream/stream_flash.c:195:
+		LOG_ERR("Buffer size is not aligned to minimal write-block-size");

- total: 0 errors, 1 warnings, 116 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

nvlsianpu · 2020-05-25T14:04:33Z

I was wondering if a regression test for this would not be good.

Yes it is possible.

subsys/storage/stream/stream_flash.c

de-nordic · 2020-05-25T14:22:16Z

subsys/storage/stream/stream_flash.c

+			 * should be erased in order to get the erased
+			 * byte-value.
+			 */
+			rc = flash_read(ctx->fdev,


This is side note: We should consider adding some API call for returning the 'erased' byte value for the device. It is at least second time when I see that the value is needed and is either read from device (here) or predefined in code, e.g. in mcuboot.

have this on my API request change list

de-nordic

Notes left. otherwise I think that the solution is fine.

nvlsianpu · 2020-05-25T15:30:00Z

@nandojve Test added

tests/subsys/storage/stream/stream_flash/testcase.yaml

hakonfam

There is a typo in the last commit "steram" -> "stream"

nandojve

After apply changes, the board disco_l475_iot1 started to work properly.

nvlsianpu · 2020-05-26T08:20:23Z

In the last commit added note on requirements for size of the stream buffer.

On buffer flush request it is very probably that write buffer contains amount of data which is non write-block-size aligned. Flash memory need to be write at minimal by write-block-size chunks. This patch addresses mechanism which ensure such behavior by adding missing bytes. fixes zephyrproject-rtos#25471 streamer buffer size should be multiple write-block-size of the flash device in order to avoid unaligned flash write request. Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

Added test for check proper service of flush-write when buffer contains unaligned amount of data. Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

Implementation requires that streamer buffer size must be multiple of the flash device wbs. This patch reflects this in API documentation Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

… flush On buffer flush request it is very probably that write buffer contains amount of data which is non write-block-size aligned. Flash memory need to be write at minimal by write-block-size chunks. This patch addresses mechanism which ensure such behavior by adding missing bytes. fixes #25471 streamer buffer size should be multiple write-block-size of the flash device in order to avoid unaligned flash write request. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

… test Added test for check proper service of flush-write when buffer contains unaligned amount of data. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

requirements Implementation requires that streamer buffer size must be multiple of the flash device wbs. This patch reflects this in API documentation upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

… flush On buffer flush request it is very probably that write buffer contains amount of data which is non write-block-size aligned. Flash memory need to be write at minimal by write-block-size chunks. This patch addresses mechanism which ensure such behavior by adding missing bytes. fixes #25471 streamer buffer size should be multiple write-block-size of the flash device in order to avoid unaligned flash write request. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit 642dc37)

… test Added test for check proper service of flush-write when buffer contains unaligned amount of data. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit 911bc79)

…rements Implementation requires that streamer buffer size must be multiple of the flash device wbs. This patch reflects this in API documentation upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit b7e535c)

… flush On buffer flush request it is very probably that write buffer contains amount of data which is non write-block-size aligned. Flash memory need to be write at minimal by write-block-size chunks. This patch addresses mechanism which ensure such behavior by adding missing bytes. fixes #25471 streamer buffer size should be multiple write-block-size of the flash device in order to avoid unaligned flash write request. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit 642dc37)

… test Added test for check proper service of flush-write when buffer contains unaligned amount of data. upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit 911bc79)

…rements Implementation requires that streamer buffer size must be multiple of the flash device wbs. This patch reflects this in API documentation upstream: zephyrproject-rtos/zephyr#25584 Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no> (cherry picked from commit b7e535c)

nvlsianpu added bug The issue is a bug, or the PR is fixing a bug area: Storage Storage subsystem labels May 25, 2020

nvlsianpu requested review from hakonfam, nandojve and de-nordic May 25, 2020 11:05

pabigot requested changes May 25, 2020

View reviewed changes

subsys/storage/stream/stream_flash.c Outdated Show resolved Hide resolved

subsys/storage/stream/stream_flash.c Outdated Show resolved Hide resolved

nvlsianpu force-pushed the bugfix/issue_25471 branch from 4aa4b5e to bb5dc9f Compare May 25, 2020 12:12

nvlsianpu added this to the v2.3.0 milestone May 25, 2020

carlescufi requested a review from pabigot May 25, 2020 12:27

hakonfam reviewed May 25, 2020

View reviewed changes

subsys/storage/stream/stream_flash.c Outdated Show resolved Hide resolved

subsys/storage/stream/stream_flash.c Outdated Show resolved Hide resolved

subsys/storage/stream/stream_flash.c Show resolved Hide resolved

nvlsianpu force-pushed the bugfix/issue_25471 branch from bb5dc9f to 1885501 Compare May 25, 2020 12:34

nvlsianpu force-pushed the bugfix/issue_25471 branch from 1885501 to 514b6b8 Compare May 25, 2020 12:48

de-nordic reviewed May 25, 2020

View reviewed changes

subsys/storage/stream/stream_flash.c Outdated Show resolved Hide resolved

de-nordic requested changes May 25, 2020

View reviewed changes

de-nordic approved these changes May 25, 2020

View reviewed changes

pabigot approved these changes May 25, 2020

View reviewed changes

nvlsianpu force-pushed the bugfix/issue_25471 branch from 92ba864 to 69d9656 Compare May 25, 2020 15:27

nvlsianpu requested a review from nashif as a code owner May 25, 2020 15:27

pabigot reviewed May 25, 2020

View reviewed changes

tests/subsys/storage/stream/stream_flash/testcase.yaml Outdated Show resolved Hide resolved

zephyrbot added the area: Tests Issues related to a particular existing or missing test label May 25, 2020

nvlsianpu force-pushed the bugfix/issue_25471 branch 2 times, most recently from 7d120b3 to 64d46a4 Compare May 25, 2020 15:44

hakonfam approved these changes May 25, 2020

View reviewed changes

nvlsianpu force-pushed the bugfix/issue_25471 branch 2 times, most recently from 4e0b926 to 46b5abd Compare May 25, 2020 19:26

nandojve approved these changes May 26, 2020

View reviewed changes

nvlsianpu requested review from carlescufi, galak and MaureenHelm as code owners May 26, 2020 08:19

zephyrbot added the area: API Changes to public APIs label May 26, 2020

nvlsianpu added 3 commits May 26, 2020 11:32

tests/subsys/storage/stream_flash: add unaligned flash test

1cbf611

Added test for check proper service of flush-write when buffer contains unaligned amount of data. Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

include/storage/steram_flash: update buffer size requirements

d8956a3

Implementation requires that streamer buffer size must be multiple of the flash device wbs. This patch reflects this in API documentation Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>

nvlsianpu force-pushed the bugfix/issue_25471 branch from d7ab2dc to d8956a3 Compare May 26, 2020 09:33

nvlsianpu mentioned this pull request May 26, 2020

[nrf fromlist] storage/stream: fix possible unaligned write on buffer flush request joerchan/sdk-zephyr#2

Merged

carlescufi merged commit 50ecfd2 into zephyrproject-rtos:master May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage/stream: fix possible unaligned write on buffer flush request #25584

storage/stream: fix possible unaligned write on buffer flush request #25584

nvlsianpu commented May 25, 2020

nandojve commented May 25, 2020

zephyrbot commented May 25, 2020 •

edited

Loading

nvlsianpu commented May 25, 2020

de-nordic May 25, 2020

nvlsianpu May 25, 2020

de-nordic left a comment

nvlsianpu commented May 25, 2020

hakonfam left a comment

nandojve left a comment •

edited

Loading

nvlsianpu commented May 26, 2020

storage/stream: fix possible unaligned write on buffer flush request #25584

storage/stream: fix possible unaligned write on buffer flush request #25584

Conversation

nvlsianpu commented May 25, 2020

nandojve commented May 25, 2020

zephyrbot commented May 25, 2020 • edited Loading

checkpatch (informational only, not a failure)

nvlsianpu commented May 25, 2020

de-nordic May 25, 2020

Choose a reason for hiding this comment

nvlsianpu May 25, 2020

Choose a reason for hiding this comment

de-nordic left a comment

Choose a reason for hiding this comment

nvlsianpu commented May 25, 2020

hakonfam left a comment

Choose a reason for hiding this comment

nandojve left a comment • edited Loading

Choose a reason for hiding this comment

nvlsianpu commented May 26, 2020

zephyrbot commented May 25, 2020 •

edited

Loading

nandojve left a comment •

edited

Loading