[compot-dev]

A failed USB transfer in the SPI NAND layer went unnoticed: the result of a read from the cache and of a status read was not looked at.

• A read ended with Status: OK and wrong data in the dump.
• A write reads the page first, so it could program a page over data it had not read.
• Page data that did not reach the chip still ended in a program command and Status: OK.
• With a status that did not come back, an operation went on from an uninitialized value.

Now spi_nand_protocol_read_from_cache and spi_nand_protocol_get_feature report the failure, spi_nand_wait_ready passes it on, and read, write and erase stop with a message (SPI_NAND_FLASH_RTN_IO_ERROR). A command that failed earlier is caught too: the failure of a held transfer comes with the next read. The failed page is not kept in the buffers, and with -k a USB failure stops the operation instead of counting as a BAD page. The detection fails too when the ECC setup does not get through: its four identical branches are one function now.

Version 1.20.2.

Tests: nandsim gets USB failures of a cache read and of a status read: read, -k read, write (nothing is programmed) and erase fail, and the next read brings the right data; the detection fails on a failed ECC setup through both registers. Five of the new checks failed on the code before this change.

Hardware, normal paths only (a real USB failure was not provoked): CH347T + W25N01G and CH341A + F35SQA001G, 1 MB read twice with the same md5 and -t OK on both, 1 MB in 2.4 s and 9.7 s.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Stop SPI NAND operations when USB reads fail

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Fail SPI NAND reads, writes, and erases when cache or status USB reads fail.
• Invalidate failed page reads and keep USB errors distinct from BAD pages under -k.
• Add simulated failure checks and release version 1.20.2.

Diagram

graph TD
 A["USB controller"] --> B["Feature or cache"] --> C{"Read succeeds?"}
 C -- "yes" --> D["Continue operation"]
 C -- "no" --> E{"Cache read?"}
 E -- "yes" --> F["Invalidate page"] --> G["Stop operation"]
 E -- "no" --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Checked transaction API

• ➕ Could enforce error handling consistently across all SPI NAND commands, including writes.
• ➖ Requires a broader protocol-layer redesign and expands the risk and scope of this fix.

Recommendation: Keep the targeted propagation for this bug: it stops operations where missing cache or status data would make their results unsafe. Consider a checked transaction API separately if comprehensive transfer-error handling is needed.

Files changed (5) +109 / -23

Bug fix (2) +50 / -22

spi_nand_flash.cPropagate SPI NAND USB read failures +49/-22

Propagate SPI NAND USB read failures

• Checks cache and feature-read transfers, returns I/O errors from status polling, and stops read, write, and erase operations when the chip state is unknown. Invalidates a page after its cache read fails so a subsequent read fetches it again; USB failures are not treated as skippable BAD pages.

src/spi_nand_flash.c

spi_nand_flash.hDefine a SPI NAND I/O error result +1/-0

Define a SPI NAND I/O error result

• Adds 'SPI_NAND_FLASH_RTN_IO_ERROR' to distinguish USB transfer failures from chip timeouts and flash failures.

src/spi_nand_flash.h

Tests (1) +51 / -0

nandsim.cExercise failed cache and status transfers +51/-0

Exercise failed cache and status transfers

• Adds simulator fault injection for page-cache and status reads. Checks failure propagation through reads, '-k' reads, writes, and erases, including page refetch and prevention of programming after a failed preread.

tests/nandsim.c

Documentation (1) +7 / -0

CHANGELOG.mdDocument the 1.20.2 USB failure fix +7/-0

Document the 1.20.2 USB failure fix

• Adds a release entry explaining that failed SPI NAND USB reads now stop operations and do not retain the failed page.

CHANGELOG.md

Other (1) +1 / -1

main.cBump the version to 1.20.2 +1/-1

Bump the version to 1.20.2

• Changes the version string reported by the application from 1.20.1 to 1.20.2.

src/main.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Reads can return data from a failed write ✓ Resolved

Description

spi_nand_write_page() returns immediately when its program-status poll reports an I/O error,
before invalidating the page buffer it modified for the write. If the caller retries by reading that
page, _current_page_num still matches and the read can return the attempted write data without
fetching what is actually on the chip.

---

2. A failed status read can report a good write ✓ Resolved

Resolving commit: 48f576d

Resolution reason

The commit makes spi_nand_write_page() check the results of the program-load and the program-ready
path, returning SPI_NAND_FLASH_RTN_IO_ERROR and clearing the read cache on failure. The status read
after programming is still handled by spi_nand_wait_ready, which now propagates the error via
rtn_status. Also, the test nandsim adds a case where the status read after program execute fails on
USB (fail_after_program / fail_feature=0xc0) and asserts the write returns an error. The fix is a
different implementation: it checks the status result via the wait path and the program load instead
of adding a check on a separate status read, but the finding's premise (a USB failure after program
is reported as success) is addressed.

Description

spi_nand_protocol_get_feature() now returns an I/O error for a failed transfer, but
spi_nand_write_page() ignores that result when it reads status register 1 after programming. If
that transfer fails after the program-ready poll succeeds, the write continues to its success return
rather than reporting the USB failure.

---

3. A NAND whose ECC setup fails is re-probed as NOR 🐞 Bug ☼ Reliability

Description

SPI_NAND_Flash_Init now returns SPI_NAND_FLASH_RTN_IO_ERROR after the chip has been identified
as SPI NAND, so snand_init returns -1. flash_cmd_init then falls through to snor_init() and
sends NOR probe commands to a chip it already knows is NAND. The run then ends with "Flash not
found!!!!", or the NAND's ID may be accepted by the NOR table, instead of stopping on the USB error.
The NOR probe can also be hit by the leftover held_failed flag from a set_feature CS high that
failed.

Code

src/spi_nand_flash.c[R5309-5313]

+		if( SPI_NAND_Flash_Enable_OnDie_ECC() != SPI_NAND_FLASH_RTN_NO_ERROR )
+		{
+			_SPI_NAND_PRINTF("SPI NAND Flash: the USB transfer failed while setting ECC\n");
+			return SPI_NAND_FLASH_RTN_IO_ERROR;
+		}

Evidence

flash_cmd_init calls snor_init() whenever snand_init() returns <= 0, and snand_init returns
-1 when nandflash_init fails, which now includes the new ECC-setup error return. In
spi_controller.c, SPI_CONTROLLER_Chip_Select_High sets held_failed = 1 when a held flush
fails, and only the next read or long write clears it. spi_nand_set_ondie_ecc returns right after
a failed set_feature, so the flag is still set when the NOR probe reads.

src/flashcmd_api.c[45-58]
src/spi_nand_flash.c[5712-5722]
src/spi_nand_flash.c[5625-5635]
src/spi_controller.c[94-123]
src/spi_nand_flash.c[5562-5581]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When a chip is identified as SPI NAND but the ECC setup's USB transfer fails, `SPI_NAND_Flash_Init` returns `SPI_NAND_FLASH_RTN_IO_ERROR`, `snand_init` returns -1, and `flash_cmd_init` then probes the same chip as SPI NOR. The result is a misleading "Flash not found" or a NOR misdetection, and a stale `held_failed` flag can also be consumed by the NOR probe.
## Fix Focus Areas
- src/spi_nand_flash.c[5309-5313]
- src/spi_nand_flash.c[5712-5722]
- src/flashcmd_api.c[45-58]
## Recommended Fix
Let `snand_init` tell "no NAND found" apart from "NAND found but its setup failed", for example by returning a distinct negative value (-2) when `SPI_NAND_Flash_Init` returns `SPI_NAND_FLASH_RTN_IO_ERROR`. In `flash_cmd_init`, return that error immediately without calling `snor_init()`. Also make sure a pending held-transfer failure is cleared once it has been reported.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. A failed page upload can still report a good write ✓ Resolved

Description

spi_nand_protocol_program_load sends the page data (more than 4 bytes) straight away through
SPI_CONTROLLER_Write_NByte. It throws away the result. A failure on that direct send is returned
to the caller only: held_failed is set only when a held transfer fails at CS high, so the
following spi_nand_wait_ready poll does not see it. If the USB transfer of the page data fails,
spi_nand_write_page still runs program execute, so the chip programs whatever its cache holds (the
page it read earlier, or part of the new data). The status poll then succeeds and the write ends
with Status: OK. That is the failure this PR says it fixes, and the new spi_nand_wait_ready
comment says an earlier failed command shows up at the next read, which is not true for this
transfer.

---

View review recommended (2)

5. Failed ECC writes can pass detection ✓ Resolved

Description

spi_nand_set_ondie_ecc() discards the feature-write result, while
spi_nand_protocol_set_feature() itself always returns success without checking chip-select
deassertion. On a device where that deassertion fails but the following feature read succeeds,
initialization accepts the ECC setup even though its write transfer failed.

---

6. Initialization masks failed ECC reads ✓ Resolved

Description

spi_nand_protocol_get_feature() now reports a failed feature read, but
SPI_NAND_Flash_Enable_OnDie_ECC() discards that result and always returns success. If the USB
transfer fails while ECC is being configured, the code can use a feature byte that was not read from
the chip, and SPI_NAND_Flash_Init() still reports successful initialization.

---

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[compot-dev]

Qodo finding 1 (ECC setup masks a failed read) is fixed in 46fda5d: the ECC setup checks its reads, the detection fails on a failure. Checked with the build and all tests in the container (new checks through registers 0xB0 and 0x90) and on CH347T + W25N01G and CH341A + F35SQA001G, with and without -d.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 46fda5d

[compot-dev]

Qodo findings 1 and 2 of the last review are fixed in 1e96da2: a write stopped at the status poll clears the page buffers, and spi_nand_protocol_set_feature reports a failed CS high, which the ECC setup checks. Both new checks failed on the code before this commit; build, all tests and -t on CH347T + W25N01G and CH341A + F35SQA001G pass.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Code Review by Qodo

New Review Started

This review has been superseded by a new analysis

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 1e96da2

[compot-dev]

Qodo finding "A failed page upload can still report a good write" is fixed in 48f576d: spi_nand_protocol_program_load reports the failure and the write stops before the program command. Both new checks failed on the code before this commit; build, all tests and -t on CH347T + W25N01G and CH341A + F35SQA001G pass.

[compot-dev]

/review

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 48f576d

[compot-dev]

Two Qodo findings on 48f576d are left as they are, with these reasons:

• A failed status read can report a good write: status register 1 after the program goes only to the debug output. The program result comes from the status poll, which is checked, so a failed read of register 1 does not hide a failed program.
• A NAND whose ECC setup fails is re-probed as NOR: the fallback to the NOR probe is how a failed NAND init has always worked. The run ends with an error, not with Status: OK. Stopping on the USB error at that point is a separate change to the detection.