-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Direct access to device key #9215
Direct access to device key #9215
Conversation
@offirko, thank you for your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also git log might be cleaned up (reduced to few c ommits for 3 new files).
#include "FlashIAP.h" | ||
#include <stdio.h> | ||
|
||
int direct_access_to_devicekey(uint32_t tdb_start_offset, uint32_t tdb_end_offset, void *data_buf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this missing doxygen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I added doxygen info - and squashed the log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks better. But still can be improved: "inital version" - what initial version is it? what does it bring? or successfully compiled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xc0170 - I'm not sure I follow.. git is source control. Part of its use is to track the different development stages (e.g. enabling quick recovery milestones..) . I agree that sometimes there are too many unimportant stages that can be squashed .. but in general I don't think we should rewrite git-log history prior to delivery or make it look better)..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I partly agree but my point was that this feels like development branch rather than ready for the release. Don't need to squash but rather add details important for users.
Two commits msg have zero value without studying changes and this is functionality change, we should introduce it - why are we bringing this functionality change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think I now understand what you meant - I'll add the required information.
d12cab6
to
6bd13b6
Compare
Fixes #9196 |
6bd13b6
to
e14b6b7
Compare
Pending on #9237 |
Does it need change to design docs and storage documentation? It's hard to see how it fits in the architecture. |
This code will not be documented. Is for the use of the PDMC bootloader, to reduce the size and avoid linking the full TDBStore. In 2 versions, the bootloader will completely change to a PSA oriented, and this code will not be relevant. |
Fyi @ARMmbed/mbed-docs @ARMmbed/mbed-os-maintainers |
Version based on prceeding PR |
@davidsaada - can you please review - Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for a few minor issues.
exit_point: | ||
if (true == is_flash_init) { | ||
flash.deinit(); | ||
is_flash_init = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this line.
*out_tdb_start_offset = MBED_CONF_STORAGE_TDB_EXTERNAL_INTERNAL_BASE_ADDRESS; | ||
tdb_size = MBED_CONF_STORAGE_TDB_EXTERNAL_RBP_INTERNAL_SIZE; | ||
} else { | ||
return MBED_ERROR_UNSUPPORTED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should deinit flash here and on next failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad, some of the ifs are later addition to the code. I will move flash init below them.
static int reserved_data_get(FlashIAP *flash, tdbstore_area_data_t *area_params, void *reserved_data_buf, | ||
size_t reserved_data_buf_size, size_t *actual_data_size_ptr) | ||
{ | ||
int status = MBED_SUCCESS;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant semicolon.
9f73cae
to
3c5c098
Compare
Ready for CI as soon as reviewers approve (@davidsaada ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now.
CI started |
Test run: FAILEDSummary: 3 of 7 test jobs failed Failed test jobs:
|
I have changed the test main file to skip compiling if there is no component flashiap set for the target. I hope this will solve all the build failure. |
Hi @offirko, |
This enables application with code size restrictions to access devicekey directly based on address in internal flash without kvconfig overhead
…e position. (Supporting FILESYSTEM and TDB_EXTERNAL configuration only)
…ent flashiap block device in the target
e0eea8e
to
423c4fb
Compare
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
Low code size tool that provides direct access to ROT Device Key
(to be used by BootLoader).
Pending on #9237
Pull request type