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

Fix functionality for FlashIAPBD & SlicingBD #10108

Merged
merged 2 commits into from
Mar 19, 2019

Conversation

theamirocohen
Copy link
Contributor

Description

Due to discovery of inconsistent sector sizes in devices storage the is_valid_erase function was adjusted,
For FlashIAPBD the 'code size' was included to the calculation, preventing faulty "virtual" addresses calculation.
For SlicingBD the same error was fixed and in all 3 validation functions that sent addresses for validation and program/read/erase different addresses.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-storage
@geky

Due to discovery of inconsistent sector sizes in devices storage the is_valid_erase function was adjusted,
For FlashIAPBD the 'code size' was included to the calculation, preventing faulty "virtual" addresses calculation.
For SlicingBD the same error was fixed and in all 3 validation functions that sent addresses for validation and program/read/erase
different addresses.
@ciarmcom
Copy link
Member

@theamirocohen, thank you for your changes.
@geky @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 15, 2019

This is a fix adding new functionality, marked for 5.13rc1

@theamirocohen
Copy link
Contributor Author

This is a fix adding new functionality, marked for 5.13rc1

Hi @0xc0170, this pull request doesn't add new functionality, it just implement class functions.
We'll be happy if it will be merged to 5.12.1

@davidsaada
Copy link
Contributor

@0xc0170, this is definitely a bug fix. The FlashIAP and Slicing block devices currently use wrong logic to decide whether the input parameters for erase/read/write APIs are valid or not. This PR fixes that logic.

@0xc0170 0xc0170 requested a review from dannybenor March 18, 2019 10:10
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 18, 2019

Thanks for the details!

@dannybenor Please approve and confirm the intention having this in the patch release.

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for @dannybenor approval

Copy link

@dannybenor dannybenor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should go to a patch of 5.12

@@ -256,4 +256,13 @@ const char *FlashIAPBlockDevice::get_type() const
return "FLASHIAP";
}

bool FlashIAPBlockDevice::is_valid_erase(bd_addr_t addr, bd_size_t size) const
{
bd_addr_t base_addr = addr + (_base - _flash.get_flash_start());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment saying what is base_addr and that it is related to the full flash, not only the addresses inside the blockdevice

@cmonr
Copy link
Contributor

cmonr commented Mar 18, 2019

CI started.

Would like to see this come into RC3 is possible (@ARmmbed/mbed-os-maintainers)

@mbed-ci
Copy link

mbed-ci commented Mar 19, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr removed the needs: CI label Mar 19, 2019
@0xc0170 0xc0170 merged commit 54602f5 into ARMmbed:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants