Skip to content

Prevent sector-unaligned erase #7952

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 3, 2018
Merged

Prevent sector-unaligned erase #7952

merged 2 commits into from
Sep 3, 2018

Conversation

offirko
Copy link
Contributor

@offirko offirko commented Sep 2, 2018 •
edited
Loading

Description

Pull request type

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

@offirko
Copy link
Contributor Author

offirko commented Sep 2, 2018

@bulislaw , @donatieng , @ARMmbed/mbed-os-maintainers , @dannybenor , @davidsaada .
I'd appreciate your review of this fix for SPIFBlockDevice

@offirko
Copy link
Contributor Author

offirko commented Sep 2, 2018

Please note that this fix should be entered before code freeze!
Sorry for the post-last-minute PR, due to its dependency with #7774 it wasn't possible to issue this PR earlier.

@offirko
Copy link
Contributor Author

offirko commented Sep 2, 2018

davidsaada
davidsaada suggested changes Sep 2, 2018
View reviewed changes
components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.h Outdated
SPIF_BD_ERROR_OK = 0, /*!< no error */
SPIF_BD_ERROR_DEVICE_ERROR = BD_ERROR_DEVICE_ERROR, /*!< device specific error -4001 */
SPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */
SPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */
Copy link
Contributor

Choose a reason for hiding this comment

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

Rogue spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I caught the rouge tab (-:

components/storage/blockdevice/COMPONENT_SPIF/TESTS/block_device/spif/main.cpp Outdated
{
utest_printf("\nTest Unaligned Program Starts..\n");
utest_printf("\nTest Unaligned Erase Starts..\n");

SPIFBlockDevice blockD(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO, MBED_CONF_SPIF_DRIVER_SPI_CLK,
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not use a camel case name (blockD) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

components/storage/blockdevice/COMPONENT_SPIF/SPIFBlockDevice.cpp
return SPIF_BD_ERROR_INVALID_ERASE_PARAMS;
}

if ( ((addr % get_erase_size(addr)) != 0 ) || (((addr + in_size) % get_erase_size(addr + in_size - 1)) != 0 ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't the function is_valid_erase work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is_valid_erase wont work because it relies on a single erase-sector size.
This supports variable erase-sector size according to address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. This means that it has a bug, which should be fixed (but not in this PR of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidsaada - I think not all Block Devices support variable sector-erase size. You're right, it will require a separate PR, probably updating BlockDevice.h base class and maybe some inheriting block devices.

davidsaada
davidsaada approved these changes Sep 2, 2018
View reviewed changes
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.

👍

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 3, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Sep 3, 2018

Build : SUCCESS

Build number : 2992
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7952/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Sep 3, 2018

Test : SUCCESS

Build number : 2755
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7952/2755

@mbed-ci
Copy link

mbed-ci commented Sep 3, 2018

Exporter Build : SUCCESS

Build number : 2603
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7952/

@adbridge adbridge merged commit 2efa4e4 into ARMmbed:master Sep 3, 2018
@0xc0170 0xc0170 removed the needs: CI label Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@davidsaada davidsaada davidsaada approved these changes

Assignees
No one assigned
Labels
release-version: 5.10.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@offirko @0xc0170 @mbed-ci @davidsaada @adbridge @NirSonnenschein