feat(spi_nand_flash): Add block device support#606
Conversation
5e70c2f to
afd1301
Compare
69e8bbd to
422413c
Compare
b05a6d5 to
2d1b2a3
Compare
2d1b2a3 to
19bd12c
Compare
c335750 to
cc21a22
Compare
0f79e0e to
d773d82
Compare
cdb519e to
2f78ca0
Compare
|
Outcome of the final review: 1. Flash BDL erase: missing out-of-bounds checkSeverity: High
Both Location: Fix — add after the if (start_addr + erase_len > handle->geometry.disk_size) {
ESP_LOGE(TAG, "Erase range exceeds device bounds");
return ESP_ERR_INVALID_SIZE;
}2.
|
| File | Function |
|---|---|
src/nand_flash_blockdev.c |
is_ecc_exceed_threshold() |
src/nand_diag_api.c |
is_ecc_exceed_threshold() |
src/nand.c |
s_need_data_refresh() |
All three are identical: compare ecc_corrected_bits_status against ecc_data_refresh_threshold using the same 1/4/7 mapping.
Fix: Extract a single shared helper (e.g. static inline in a private header) and call it from all three sites.
5. chip_name stores only manufacturer name
Severity: Low
Device init files (nand_gigadevice.c, nand_micron.c, nand_winbond.c, etc.) set chip_name to just the manufacturer string:
char *name = "gigadevice";
strncpy(dev->device_info.chip_name, name, strlen(name) + 1);The GET_NAND_FLASH_INFO ioctl exposes this to users. For diagnostics, a model-specific name (e.g. "GD5F1GQ5UExxG" or at least "gigadevice-0x51") would be more useful. The 32-byte chip_name field is underutilized.
6. size_t → uint32_t truncation in erase log
Severity: Low
In nand_flash_blockdev_erase():
ESP_LOGV(TAG, "erase - ... size=0x%08" PRIx32 " ...", ..., (uint32_t)erase_len, ret);erase_len is size_t (64-bit on Linux host). The (uint32_t) cast silently truncates. The WL erase log has the same pattern. Use %zx or cast to uint64_t with PRIx64.
Summary
| # | Severity | Issue |
|---|---|---|
| 1 | High | Flash BDL erase: missing out-of-bounds check |
| 2 | Medium | dhara_nand_read BDL path: dst_buf_size doesn't match actual buffer |
| 3 | Medium | spi_nand_flash_init_device() doxygen/Kconfig not updated for BDL incompatibility |
| 4 | Low | Triplicated ECC threshold check logic |
| 5 | Low | chip_name only stores manufacturer, not model |
| 6 | Low | size_t → uint32_t truncation in erase log |
After fixing those points (1,2,3 and 6 at least), the PR can be merged.
Thank you
2f78ca0 to
14991e0
Compare
554f473 to
14991e0
Compare
pacucha42
left a comment
There was a problem hiding this comment.
Great! Thank you for addressing all the comments. Approving
|
need to revert the deletion of the workaround: https://github.com/espressif/idf-extra-components/compare/554f4734e8927e1308bfca81d213f39e512efdba..14991e0849cb7d6e2a7c41067d460657a54135d9 the latest docker images still requires hours to be created. |
the latest docker image has been updated! |
14991e0 to
8e079e9
Compare
154e464 to
6178b51
Compare
- add block device support - refactor the component for improved structure and maintainability - remove the fatfs/vfs component dependency from the driver
…and_flash into new component
pacucha42
left a comment
There was a problem hiding this comment.
All looks good, approviing. Thank you @RathiSonika
Change description
Add block device support (nand_flash_bdl and nand_wl_bdl)
Please refer spi_nand_flash/layered_architecture.md for the details and migration guide
Refactor the component for improved structure, maintainability and readability
- Added page-based API terminology (
read_page,write_page,get_page_count,get_page_size) with backward-compatible sector aliases.Removed the fatfs/vfs component dependency; it will be relocated (along with examples) to another component in a different PR.
Add Kconfig option NAND_FLASH_ENABLE_BDL to enable/disable BDL support
Updated test_apps and host_tests to verify the same
OTP and OOB area handling are still missing. I’ll address them in a separate PR, as the scope of this one is already growing.