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 two issues in ExhaustibleBlockDevice #7058

Merged
merged 1 commit into from Jun 15, 2018

Conversation

Projects
None yet
6 participants
@davidsaada
Contributor

davidsaada commented May 30, 2018

Description

This PR fixes two issues in the exhaustible block device (block device simulating wearing of flash sectors):
1. Return errors when attempting to erase a worn out erase unit or write to it. Current code simply returns 0, without performing the erase/program action, meaning that the failure will be postponed to the time someone attempts to read these values (if at all).
2. Support erasing of more than one erase unit, in one call (as current erase API assumes we only erase one erase unit).

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-storage May 30, 2018

@@ -24,6 +24,10 @@
#include "BlockDevice.h"
enum {
BD_ERROR_ERASE_UNIT_WORN_OUT = -3301,

This comment has been minimized.

@cmonr

cmonr May 31, 2018

Contributor

I'm confused. Why is this an anonymous enum that only has one entry?

This comment has been minimized.

@davidsaada

davidsaada May 31, 2018

Contributor

This is an error code specific to exhaustible block device. Followed the convention dictated by existing block devices, like MBRBlockDevice. My understanding is that these error codes are additions to the ones defined in BlockDevice base class.

This comment has been minimized.

@cmonr

cmonr May 31, 2018

Contributor

👍 I figured as much, but wanted to be sure.

This comment has been minimized.

@dannybenor

dannybenor Jun 3, 2018

I think the error should be removed. Real flash will not identify the error.

This comment has been minimized.

@davidsaada

davidsaada Jun 3, 2018

Contributor

Right. The error will be removed as it also breaks the LittleFS wear level test.

@davidsaada davidsaada force-pushed the davidsaada:david_exhaustible_bd_fix branch from 831c36d to 79a8f3e Jun 3, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 3, 2018

Removed the "worn out" error addition (and fixed the commit message).

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 11, 2018

@mbed-os-storage please review

// TODO possibly something more destructive here
return 0;
if (_erase_array[addr / eu_size] == 0) {
return 0;

This comment has been minimized.

@offirko

offirko Jun 11, 2018

Contributor

if the erase expands over several sectors and only one of them was exhausted do you stop in the middle of the erase while and not erase the other sectors? Also, is there no error indication for cycles exhausted?

This comment has been minimized.

@davidsaada

davidsaada Jun 11, 2018

Contributor

Regarding the loop exit: Good point, will fix.
Regarding the error indication: This was originally included in the PR (see the strikethrough text in the description) and then I reverted it. Reason was the fact that flash components don't give you an indication for it (you won't get an error, just read the wrong data). In addition, This broke the LittleFS resilience test.

return err;
}
addr += eu_size;
size -= eu_size;

This comment has been minimized.

@offirko

offirko Jun 11, 2018

Contributor

can size become negative and then the while loop wont exit?

This comment has been minimized.

@davidsaada

davidsaada Jun 11, 2018

Contributor

No, this can't happen. The function checks that the erase is valid (at its start), meaning that the erase size is a multiple of the erase unit size, so the scenario you describe can't happen.

This comment has been minimized.

@offirko

offirko Jun 11, 2018

Contributor

I saw the assert now, thanks

@davidsaada davidsaada force-pushed the davidsaada:david_exhaustible_bd_fix branch from 79a8f3e to 4af2c9d Jun 11, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Jun 11, 2018

Pushed a fix with @offirko 's comment (not returning when reaching a worn sector).

@cmonr cmonr added needs: CI and removed needs: review labels Jun 13, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 13, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 15, 2018

@cmonr cmonr merged commit 74a75d5 into ARMmbed:master Jun 15, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 921 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9594 cycles (+11 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@davidsaada davidsaada deleted the davidsaada:david_exhaustible_bd_fix branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment