[nrf noup] boot: Introduce retry mechanism for image verification #467
Conversation
adsz-nordic
changed the title
boot/zephyr/Kconfig
Outdated
| @@ -1213,4 +1213,22 @@ config MCUBOOT_VERIFY_IMG_ADDRESS | |||
| also be useful when BOOT_DIRECT_XIP is enabled, to ensure that the image | |||
| linked at the correct address is loaded. | |||
|
|
|||
|
|
|||
| config MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT | |||
There was a problem hiding this comment.
Can we standardize to use NRF_ prefix on MCUboot options that are added as NRF noup things?
Here it would be NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT. This way we can avoid any collisions with upstream added features.
There was a problem hiding this comment.
My initial thought was that the PR was going to be "noup" but that has changed and PR needs to go to upstream. Sorry for confusion - I am going to update the tag and description.
I presume the config name will not need a change in such case, right?
There was a problem hiding this comment.
It turns out that after all, this PR is going to be downstream-only.
I applied the suggested changes.
Please review again.
|
It turns out that this feature will be added only for downstream. |
| BOOT_LOG_DBG("Image validation attempt %d/%d", i, CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT); | ||
| #endif /* CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1 */ | ||
|
|
||
| #if defined(MCUBOOT_SWAP_USING_OFFSET) && defined(MCUBOOT_SERIAL_RECOVERY) | ||
| FIH_CALL(bootutil_img_validate, fih_rc, state, hdr, fap, tmpbuf, BOOT_TMPBUF_SZ, | ||
| NULL, 0, NULL, 0); | ||
| FIH_CALL(bootutil_img_validate, fih_rc, state, hdr, fap, tmpbuf, BOOT_TMPBUF_SZ, | ||
| NULL, 0, NULL, 0); | ||
| #else | ||
| FIH_CALL(bootutil_img_validate, fih_rc, state, hdr, fap, tmpbuf, BOOT_TMPBUF_SZ, | ||
| NULL, 0, NULL); | ||
| FIH_CALL(bootutil_img_validate, fih_rc, state, hdr, fap, tmpbuf, BOOT_TMPBUF_SZ, | ||
| NULL, 0, NULL); | ||
| #endif | ||
|
|
||
| if (FIH_EQ(fih_rc, FIH_SUCCESS)) { | ||
| #if CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1 | ||
| BOOT_LOG_DBG("Image validation attempt %d/%d success", i, CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT); | ||
| #endif /* CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1 */ | ||
| break; | ||
| } else { | ||
| #if CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1 |
There was a problem hiding this comment.
As per upstream comments, only show messages if NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1
There was a problem hiding this comment.
I thought you only meant messages showing attempt numbers, not all the messages.
Now each new log will be wrapped with #if, although this reduces readability of code.
There was a problem hiding this comment.
It's already output by other functions if this function fails so doesn't make sense to duplicate it again here
boot/bootutil/src/loader.c
Outdated
| break; | ||
| } else { | ||
| #if CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT > 1 | ||
| BOOT_LOG_WRN("Image validation attempt %d/%d failure: %d", i, CONFIG_NRF_MCUBOOT_IMG_VALIDATE_ATTEMPT_COUNT, fih_rc); |
There was a problem hiding this comment.
Changed. Are there no automatic CI jobs for checks like this for mcuboot?
There was a problem hiding this comment.
there isn't since checkpatch which zepyhyr uses is a linux kernel script (and has so many issues)
Intended mainly for direct-xip mode. Allows to control: - number of image validation attempts performed before considering the image invalid - time before next attempt is made Signed-off-by: Adam Szczygieł <adam.szczygiel@nordicsemi.no>
|
Intended mainly for direct-xip mode.
Allows to control:
This is requested by customer who doesn't like the fact that a single, temporary bit flip may lead to deletion of an image.