agent: scan NOR through flash_read, not the memory window that wraps at 1 MiB by widgetii · Pull Request #153 · OpenIPC/defib

widgetii · 2026-10-16T09:34:40Z

On hi3536dv100 (Xiongmai NBD8010S-KL-V2, W25Q128) agent scan reported
256/256 GOOD with per-sector CRCs repeating every 16 sectors: sector 16
(0x100000, kernel) carried sector 0's CRC (U-Boot). handle_scan() read
NOR through the memory-mapped boot-mode window at FLASH_MEM. That window
wraps at 1 MiB on some SoCs, which is why CMD_READ and CMD_CRC32 already
go through the register path ("boot mode memory window wraps at 1MB on
some SoCs", handle_crc32_cmd). So every sector past the first MiB was
scanned as a copy of one inside it, and empty sectors were missed.

Both passes now read the chip as CMD_READ does:

Pass 1 (CRC32 and the uniformity check) reads each sector with
flash_read() into scan_buf. NOR sectors larger than scan_buf (128 KiB)
are read in chunks, with the CRC chained across them. NAND keeps its
one read per block.
Pass 2, NOR's stability check, is an independent flash_crc32() of the
sector, a second read through the same path. Before, it was a second
pass over the same wrapped window.

On the board (agent built from this commit, flash = OpenIPC 16M image):
before, 256 GOOD and 16 distinct CRCs; now 133 GOOD and 123 EMPTY. The
123 EMPTY are the all-0xFF sectors FLASH_STREAM skips for this image.
There are 93 distinct CRCs, and every non-empty sector's scan CRC equals
CMD_CRC32 for it. The scan takes 20 s (27 s before).

make -C agent test passes. All ten SoC builds compile with the same
warnings as master (compared line by line on hi3536dv100/FMC100 and
hi3520dv200/HISFC350).

…at 1 MiB On hi3536dv100 (Xiongmai NBD8010S-KL-V2, W25Q128) `agent scan` reported 256/256 GOOD with per-sector CRCs repeating every 16 sectors: sector 16 (0x100000, kernel) carried sector 0's CRC (U-Boot). handle_scan() read NOR through the memory-mapped boot-mode window at FLASH_MEM. That window wraps at 1 MiB on some SoCs, which is why CMD_READ and CMD_CRC32 already go through the register path ("boot mode memory window wraps at 1MB on some SoCs", handle_crc32_cmd). So every sector past the first MiB was scanned as a copy of one inside it, and empty sectors were missed. Both passes now read the chip as CMD_READ does: - Pass 1 (CRC32 and the uniformity check) reads each sector with flash_read() into scan_buf. NOR sectors larger than scan_buf (128 KiB) are read in chunks, with the CRC chained across them. NAND keeps its one read per block. - Pass 2, NOR's stability check, is an independent flash_crc32() of the sector, a second read through the same path. Before, it was a second pass over the same wrapped window. On the board (agent built from this commit, flash = OpenIPC 16M image): before, 256 GOOD and 16 distinct CRCs; now 133 GOOD and 123 EMPTY. The 123 EMPTY are the all-0xFF sectors FLASH_STREAM skips for this image. There are 93 distinct CRCs, and every non-empty sector's scan CRC equals CMD_CRC32 for it. The scan takes 20 s (27 s before). make -C agent test passes. All ten SoC builds compile with the same warnings as master (compared line by line on hi3536dv100/FMC100 and hi3520dv200/HISFC350).

qodo-free-for-open-source-projects · 2026-10-16T09:35:15Z

PR Summary by Qodo

Fix NOR sector scans beyond the 1 MiB boot window

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Read NOR sectors through flash_read to prevent wrapped boot-window data from corrupting scan
 results.
• Chunk large sectors and chain CRCs while preserving NAND bad-block handling.
• Re-read nonuniform NOR sectors through flash_crc32 for an independent stability check.

Diagram

graph TD
  A["Scan command"] --> B{"NAND bad block?"} -->|No| C["flash_read chunks"] --> D["CRC and pattern"] -->|Nonuniform NOR| E["flash_crc32 reread"] --> F["Scan results"]
  B -->|Yes| F
  D -->|Empty, stuck or NAND| F

High-Level Assessment

Using the existing flash_read and flash_crc32 paths is the best fit: it removes the faulty direct boot-window access without adding a new flash-reading mechanism or changing the scan protocol. Chunking keeps reads within the existing buffer.

Files changed (1) +32 / -29

Bug fix (1) +32 / -29

main.cRoute sector scans through flash read APIs +32/-29

Route sector scans through flash read APIs

• Replaces direct NOR boot-window access with chunked flash_read calls for the first-pass CRC and uniformity check, while retaining NAND bad-block handling. Uses flash_crc32 for an independent second read of nonuniform NOR sectors, preventing wrapped-window data from misclassifying sectors beyond 1 MiB.

agent/main.c

qodo-free-for-open-source-projects · 2026-10-16T09:39:03Z

Code Review by Qodo

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

1. ~~Scans repeat data beyond 16 MiB~~ ✓ Resolved

Description

handle_scan() now reads every NOR sector through flash_read(), but the FMC100 driver sends those
reads with only three address bytes and has no bank-switch path. On a detected 32 or 64 MiB chip,
sectors at or above 16 MiB are read from lower addresses and receive misleading scan results.

Code

agent/main.c[1448]

+            flash_read(s * sector_sz + off, scan_buf, n);

Evidence
The scan now supplies full sector offsets to flash_read(). FMC100 detects chips larger than 16 MiB
but programs each NOR read with OP_CFG_ADDR_NUM(3); its CRC read uses the same three-byte
addressing.
agent/main.c[1445-1449]
agent/spi_flash.c[145-148]
agent/spi_flash.c[668-679]
agent/spi_flash.c[883-890]

Agent prompt

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

## Issue description
FMC100 scan reads above 16 MiB use three-byte addresses, so they cannot reach the requested sectors.
## Fix Focus Areas
- agent/main.c[1445-1449]
- agent/spi_flash.c[668-679]
- agent/spi_flash.c[145-148]
## Recommended Fix
Implement and use a supported bank-switch or four-byte addressing path for FMC100 NOR reads above 16 MiB. Ensure both scan passes address the same requested sector.

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

2. ~~Read timeouts can look like valid scans~~ ✓ Resolved

Description

handle_scan() immediately checksums the buffer after flash_read(), while FMC100 flash_read()
ignores a failed fmc_wait_ready() and copies the I/O buffer anyway. If a NOR register operation
times out, the newly added scan path can classify stale bytes as valid flash contents, including
reporting a misleading empty sector.

Code

agent/main.c[R1448-1449]

+            flash_read(s * sector_sz + off, scan_buf, n);
+            crc1 = crc32(crc1, scan_buf, n);

Evidence
The new pass consumes scan_buf without a read status. The FMC100 wait explicitly returns -1 on
timeout, but NOR flash_read() ignores that return and copies the I/O-buffer contents.
agent/main.c[1445-1454]
agent/spi_flash.c[223-229]
agent/spi_flash.c[668-686]

Agent prompt

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

## Issue description
A timed-out FMC100 register read can supply stale bytes to the scan without any error indication.
## Fix Focus Areas
- agent/main.c[1445-1450]
- agent/spi_flash.c[223-229]
- agent/spi_flash.c[668-686]
## Recommended Fix
Provide a checked flash-read path that propagates FMC operation failures. Have the scan report a read error rather than calculate a CRC or uniformity status from the affected buffer.

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

Tip of the day

💡 Did you know, you can pick a display mode instead of tweaking every Display option by hand

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

…d reads Qodo review on this branch. 1. Reads above 16 MiB. FMC100's register reads (flash_read, flash_crc32) send three address bytes, so on a 32/64 MiB FMC100 chip they cannot reach a sector at or above 16 MiB. CMD_READ and CMD_CRC32 share that limit today. The scan, though, read those sectors through the memory window before this branch, so it would have regressed there. scan_read() and scan_crc() now keep the window for FMC100 NOR offsets >= 16 MiB, exactly as before, and use the register path below that, where the 1 MiB wrap is. HISFC350's flash_read banks across the chip (hi3520dv200's 32 MiB MX25L25635E), so it is used everywhere there. SPI_DRIVER_FMC100 now tells main.c which driver it was built with. 2. Stalled reads. FMC100's NOR flash_read() ignored a timed-out fmc_wait_ready() and copied the I/O buffer anyway, and its NAND path ignored nand_read()'s 0xFF "never finished". flash_read() now returns -1 for either (HISFC350's memory-mapped reads return 0). The scan marks such a sector SCAN_READ_ERROR (0x05, already in the host's SectorStatus) with CRC 0, instead of judging stale bytes, which could pass as EMPTY. Other flash_read() callers keep ignoring the result. On the NBD8010S-KL-V2 (16 MiB W25Q128, so every sector takes the register path): 133 GOOD, 123 EMPTY, no READ_ERROR, and every GOOD sector's CRC equals CMD_CRC32's, in 20 s. The >= 16 MiB window branch is unchanged behaviour, not exercised here. make -C agent test passes. hi3536dv100 (FMC100) and hi3520dv200 (HISFC350) build with the same warnings as master, and four more SoCs build.

qodo-free-for-open-source-projects Bot reviewed Oct 16, 2026

View reviewed changes

Comment thread agent/main.c Outdated

Comment thread agent/main.c Outdated

widgetii merged commit 664d9df into master Oct 16, 2026
13 checks passed

widgetii deleted the agent-scan-nor-register-reads branch October 16, 2026 09:49

Navigation Menu

Repository navigation

agent: scan NOR through flash_read, not the memory window that wraps at 1 MiB - #153

agent: scan NOR through flash_read, not the memory window that wraps at 1 MiB#153
widgetii merged 2 commits into
masterfrom
agent-scan-nor-register-reads

widgetii commented Oct 16, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 16, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

widgetii commented Oct 16, 2026

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 16, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-free-for-open-source-projects Bot commented Oct 16, 2026 •

edited

Loading