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 flash wear leveling sector calculation #24776

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Diff-fusion
Copy link

Description

Currently the calculation of the last usable flash sector doesn't work properly if last_sector starts beyond the end of the actual flash size:

for (flash_sector_t i = 0; i < last_sector; ++i) {
first_sector = last_sector - i - 1;
if (flashGetSectorOffset(flash, first_sector) >= flash_size) {
last_sector = first_sector;
continue;
}

Here the last_sector is calculated from the first_sector but also i is included in the calculation. But i is incremented in each loop iteration and thus an increasing offset is added.
The solution here is to first calculate the number of the last usable sector and then use another loop to calculate the first sector.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Jan 2, 2025
@drashna drashna requested review from tzarc and a team January 2, 2025 15:57
Copy link
Contributor

@sigprof sigprof left a comment

Choose a reason for hiding this comment

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

Looks like this code won't work on STM32F4xx (and any other chips with non-uniform flash sectors), because flashGetSectorOffset(flash, desc->sectors_count) would read beyond the end of the desc->sectors array. So the code which finds the actual number of available flash sectors needs to be careful to avoid passing invalid sector numbers to ChibiOS functions.

Comment on lines 86 to 91
flash_sector_t last_sector = desc->sectors_count - WEAR_LEVELING_EFL_OMIT_LAST_SECTOR_COUNT;

// skip sectors that are past the actual flash size
while (flashGetSectorOffset(flash, last_sector) >= flash_size) {
last_sector--;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Applying WEAR_LEVELING_EFL_OMIT_LAST_SECTOR_COUNT first looks wrong (probably whoever added this code assumed that desc->sectors_count should be correct, but this might not be the case). There are no real users of that option in the repo though.

@@ -84,12 +84,14 @@ bool backing_store_init(void) {

// Work out how many sectors we want to use, working backwards from the end of the flash
flash_sector_t last_sector = desc->sectors_count - WEAR_LEVELING_EFL_OMIT_LAST_SECTOR_COUNT;

// skip sectors that are past the actual flash size
while (flashGetSectorOffset(flash, last_sector) >= flash_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code would trigger undefined behavior on the first iteration if EFL uses the desc->sectors array (e.g., on STM32F4xx), because the valid range of sector numbers is [0…desc->sectors_count-1]. The original code (before it had been broken by the change which added WEAR_LEVELING_EFL_OMIT_LAST_SECTOR_COUNT) did not have this problem, because it subtracted at least 1 from desc->sectors_count.

Also should we guard against more pathological cases, like no free sectors, or even a sector being partially out of range of the reported flash size? Although with the current error handling the only option is apparently to halt with a message that nobody would see.

* subtract WEAR_LEVELING_EFL_OMIT_LAST_SECTOR_COUNT after getting
  last_sector
* don't pass value beyond desc->sectors_count to flashGetSectorOffset
* add checks if not enough flash sectors are available
@Diff-fusion
Copy link
Author

Thanks for the review. I fixed the issues you mentioned and added some additional checks.
One remaining issue is that flash sectors that are used for the firmware code could be used by the wear_leveling driver. I don't see any checks to prevent that. But I'm not that experienced with QMK and don't know what would be the best way to do this.
Error handling is a bit problematic. Exiting with chSysHalt is better than just crashing. But maybe compile time checks could be used to prevent some of the error cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants