diff --git a/Bdd/Targets/FreeRtosLwip/README.md b/Bdd/Targets/FreeRtosLwip/README.md index 33420c60..e4ec475e 100644 --- a/Bdd/Targets/FreeRtosLwip/README.md +++ b/Bdd/Targets/FreeRtosLwip/README.md @@ -25,10 +25,14 @@ oracle. LAN9118 (SMSC9220) low-level driver. RX is driven by the IRQ-13 `EthernetISR` through a task notification; TX sends pbufs via `smsc9220_send_by_chunks`. - `netif/smsc9220/smsc9220_eth_drv.{c,h}`, `netif/smsc9220/smsc9220_emac_config.h`: - the Arm low-level driver, vendored verbatim from FreeRTOS-Plus-TCP's MPS2_AN385 - network interface (Apache-2.0; copyright and license headers preserved). Kept in - its own `smsc9220/` subdirectory with a `DisableFormat` `.clang-format` so - `analyze-format` leaves the third-party source untouched. + the Arm low-level driver, vendored from FreeRTOS-Plus-TCP's MPS2_AN385 network + interface (Apache-2.0; copyright and license headers preserved). Kept in its own + `smsc9220/` subdirectory with a `DisableFormat` `.clang-format` so `analyze-format` + leaves the third-party source untouched. Our snapshot predates upstream's + `-Wconversion` cleanup (PR #1245) and carries three known defects that are benign + on QEMU but would bite on real silicon — see + [`netif/smsc9220/KNOWN-LIMITATIONS.md`](netif/smsc9220/KNOWN-LIMITATIONS.md) for the + provenance snapshot and the defect/fix details. ## Scope diff --git a/Bdd/Targets/FreeRtosLwip/netif/smsc9220/.clang-format b/Bdd/Targets/FreeRtosLwip/netif/smsc9220/.clang-format index 9f525c39..00d9e92b 100644 --- a/Bdd/Targets/FreeRtosLwip/netif/smsc9220/.clang-format +++ b/Bdd/Targets/FreeRtosLwip/netif/smsc9220/.clang-format @@ -1,5 +1,6 @@ -# Vendored third-party code (Arm SMSC9220 LAN9118 driver, Apache-2.0). Keep it -# byte-for-byte as upstream ships it — DisableFormat makes clang-format a no-op -# here so the analyze-format lane does not try to reflow it to project style. +# Vendored third-party code (Arm SMSC9220 LAN9118 driver, Apache-2.0). Keep it as +# our vendored snapshot ships it — DisableFormat makes clang-format a no-op here so +# the analyze-format lane does not try to reflow it to project style. See +# KNOWN-LIMITATIONS.md for the upstream snapshot basis and known defects. DisableFormat: true SortIncludes: Never diff --git a/Bdd/Targets/FreeRtosLwip/netif/smsc9220/KNOWN-LIMITATIONS.md b/Bdd/Targets/FreeRtosLwip/netif/smsc9220/KNOWN-LIMITATIONS.md new file mode 100644 index 00000000..aabfcfd5 --- /dev/null +++ b/Bdd/Targets/FreeRtosLwip/netif/smsc9220/KNOWN-LIMITATIONS.md @@ -0,0 +1,124 @@ +# Vendored SMSC9220 / LAN9118 driver — provenance and known limitations + +`smsc9220_eth_drv.{c,h}` and `smsc9220_emac_config.h` are the Arm low-level +LAN9118 (SMSC9220) Ethernet driver, vendored from FreeRTOS-Plus-TCP's +`source/portable/NetworkInterface/MPS2_AN385/ether_lan9118/` (Apache-2.0; +copyright and license headers preserved). They are kept in this `smsc9220/` +subdirectory with a `DisableFormat` `.clang-format` so `analyze-format` leaves +the third-party source untouched. + +## Provenance / snapshot + +Our snapshot predates upstream commit +[`861750b9` (2025-05-22), "smsc9220_eth_drv.c; fix warnings from gcc +-Wconversion" (#1245)](https://github.com/FreeRTOS/FreeRTOS-Plus-TCP/pull/1245). +Relative to current upstream `main` we are missing only that commit's narrowing +casts (the `(char)` casts on the MAC bytes near line 958 and the `(uint32_t) -1` +sentinel near line 1226). We do not need them because this translation unit is +compiled with `-Wno-conversion -Wno-sign-conversion` (see the target +`CMakeLists.txt`). The driver is therefore **not** byte-for-byte identical to +upstream `main`; it is a known, slightly stale verbatim snapshot. + +## Known defects (real silicon only — benign on QEMU) + +Three defects exist in this driver, **unchanged in upstream `main`** as of +2026-05. They are real bugs against actual LAN9118 silicon but **cannot fire on +QEMU's MPS2 LAN9118 model**, which is deterministic and never enters the +divergent code path — which is why the S28.09/S28.10/S28.11 oracle lanes are +green. We run only under QEMU and have no real-hardware test rig, so these are +documented rather than patched (see "Why not patched" below). + +### 1. (Critical) MAC CSR busy-bit poll in `smsc9220_mac_regwrite` + +The busy-wait loop condition (≈line 467) is: + +```c +} while( time_out && + ( register_map->mac_csr_cmd & + GET_BIT( register_map->mac_csr_cmd, MAC_CSR_CMD_BUSY_INDEX ) ) ); +``` + +`GET_BIT` already returns `0`/`1`, so this ANDs the whole register with bit 0 +(the LSB of the MAC register address) instead of testing the BUSY bit. The +write can return before the operation completes and race the next MAC/PHY +access. The sibling `smsc9220_mac_regread` (≈line 416) shows the correct form: +condition on `GET_BIT( ... )` alone. + +**Correct fix:** condition on `GET_BIT( register_map->mac_csr_cmd, +MAC_CSR_CMD_BUSY_INDEX )` alone, matching `smsc9220_mac_regread`. + +**Benign on QEMU because:** QEMU processes a MAC-CSR command synchronously +inside the MMIO write handler, so the readback never shows BUSY set. The early +exit lands on an already-completed operation and reads correct data. + +### 2. (Major) TX filler DWORD on word-aligned chunks + +`fill_tx_fifo` (≈line 320) computes: + +```c +uint32_t remainder_bytes = ( size_bytes % 4 ); +uint32_t filler_bytes = ( 4 - remainder_bytes ); +``` + +For a word-aligned chunk (`remainder_bytes == 0`) this is `4` — a full zero +DWORD is prepended and `smsc9220_send_by_chunks` sets `data_start_offset = 4` +(≈line 1150, same `4 - (current_size % 4)` shape). The emitted packet is still +correct (the offset tells the hardware to skip the filler), but a whole FIFO +DWORD is wasted, and the FIFO free-space check (≈line 1126) does not account for +it, so a chunk whose footprint is within 4 bytes of TX-FIFO capacity could +overrun. + +**Correct fix:** use `(4 - remainder_bytes) % 4` at **both** `fill_tx_fifo` +(filler bytes) and `smsc9220_send_by_chunks` (`data_start_offset_bytes`) so the +two stay consistent — word-aligned chunks then prepend zero filler and set +offset 0. + +**Benign on QEMU because:** QEMU honours `data_start_offset`, so the packet is +byte-correct; the off-by-4 free-space error is per-packet and size-deterministic +(never accumulates — the FIFO drains between packets), and syslog records are +far smaller than the TX FIFO, so the overrun condition is never reached. + +### 3. (Major) `smsc9220_check_id` return type + +`smsc9220_check_id` (≈line 855) is declared returning `int` and returns `1` on a +chip-ID mismatch: + +```c +return( ( GET_BIT_FIELD( id, CHIP_ID_MASK, CHIP_ID_POS ) == CHIP_ID ) ? 0 : 1 ); +``` + +Its only caller stores the result into an `enum smsc9220_error_t` (≈line 996), +where `1 == SMSC9220_ERROR_TIMEOUT` — so a genuine ID mismatch is misreported as +a timeout. Initialisation still aborts (non-zero), so this is a misleading +diagnostic rather than a functional failure. + +**Correct fix:** change the signature to `enum smsc9220_error_t` and return +`SMSC9220_ERROR_NONE` / `SMSC9220_ERROR_INTERNAL`. + +**Benign on QEMU because:** the model's chip ID always matches, so the mismatch +path is never taken. + +## Why not patched + +We run this driver only under QEMU's deterministic LAN9118 model, which cannot +exercise any of the three divergences (see each "Benign on QEMU" note). A fix +would therefore be unverifiable on the only axis that matters — real silicon — +and an untested change to hardware-facing code is a liability, not an +improvement, especially as it would *look* correct. This mirrors the project's +"no untested production code" discipline applied to vendored code. + +**Do not "fix" these without a real LAN9118 hardware test rig.** QEMU cannot +show the difference; a plausible-looking change could regress real silicon with +no signal here. + +## Upstream + +All three are present and unchanged in upstream FreeRTOS-Plus-TCP `main`, and a +search (2026-05) found no public report, fix, fork, CVE, or erratum. They have +been raised with the upstream community via the FreeRTOS forum (Libraries +category) as a single good-citizen report; the driver is Arm-origin, so a fix +may land either in the FreeRTOS-Plus-TCP copy or at the Arm source. If upstream +lands hardware-validated fixes, prefer re-syncing to the upstream commit over +carrying local patches. + +Tracked locally as story S28.12 (#479), closed as documented known-limitations. diff --git a/CLAUDE.md b/CLAUDE.md index 542c6df0..bc055219 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,7 +30,7 @@ what matters — it becomes the permanent commit message on `main` on squash mer **Branch protection rules (configured on GitHub):** - Direct pushes to `main` are blocked -- PRs require all status checks to pass before merging: build-linux-gcc, build-linux-clang, build-windows-msvc, sanitize-linux-gcc, coverage-linux-gcc, analyze-tidy, analyze-cppcheck, analyze-format, analyze-iwyu, integration-linux-openssl, integration-windows-openssl, bdd-linux-syslog-ng, bdd-windows-otel, bdd-freertos-qemu-plustcp, build-freertos-host-tdd-plustcp, build-freertos-target-plustcp, summary +- PRs require all status checks to pass before merging: build-linux-gcc, build-linux-clang, build-windows-msvc, sanitize-linux-gcc, coverage-linux-gcc, analyze-tidy, analyze-cppcheck, analyze-format, analyze-iwyu, integration-linux-openssl, integration-linux-mbedtls, integration-windows-openssl, bdd-linux-syslog-ng, bdd-windows-otel, bdd-freertos-qemu-plustcp, build-freertos-host-tdd-plustcp, build-freertos-target-plustcp, analyze-tidy-freertos-plustcp, analyze-iwyu-freertos-plustcp, bdd-freertos-qemu-lwip, build-freertos-target-lwip, analyze-tidy-freertos-lwip, analyze-iwyu-freertos-lwip, summary - Squash merge only — other merge strategies are disabled - Branches are deleted automatically after merge diff --git a/DEVLOG.md b/DEVLOG.md index e5aa063e..fb7b7ad5 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -13167,3 +13167,59 @@ MISRA rule — different category, doesn't set precedent. every untriaged style finding becomes a hard gate. Worth E24 discussion. + +## 2026-05-30 — S28.12: vendored smsc9220 driver — document, report, no patch + +### Decisions + +- **Closed S28.12 (#479) as documented known-limitations, not a code + patch.** The three CodeRabbit findings in the vendored Arm LAN9118 + driver (busy-bit poll in `smsc9220_mac_regwrite`, TX filler DWORD on + word-aligned chunks, `smsc9220_check_id` returning `int` into an + `enum`) are real against silicon but provably inert on QEMU's + deterministic MPS2 LAN9118 model — each only diverges on a path the + model never enters (BUSY never held, FIFO never near-full with small + syslog records, chip ID always matches). We run only under QEMU and + have no real-hardware rig, so a fix would be unverifiable on the only + axis that matters; an untested change to hardware-facing code is a + liability. Applied the project's "no untested production code" + discipline to vendored code: document, don't patch. + +- **Confirmed they are not a BDD-flakiness source.** Flakiness needs a + non-deterministic source; all three are dead code on the deterministic + model and produce bit-identical results every run. Real flake suspects + (if any appear) live in the async/timing layer — tcpip thread, RX + IRQ→task-notify, ARP timing, syslog-ng readiness, Nagle, harness log + handling — not in this driver's logic. + +- **Provenance honesty fix.** Established our snapshot predates upstream + PR #1245 (2025-05-22 `-Wconversion` cleanup): we're missing only its + narrowing casts, which we don't need because the TU is built with + `-Wno-conversion -Wno-sign-conversion`. Dropped the "byte-for-byte + verbatim" wording in the README and the driver's `.clang-format`; + added `netif/smsc9220/KNOWN-LIMITATIONS.md` with the snapshot basis, + the three defects (location + correct fix + why benign on QEMU), and a + "do not fix without a real-hardware rig" warning. + +- **Report upstream as one forum post, not issues or a PR.** Searched + (via Claude AI) for prior reports across FreeRTOS-Plus-TCP, TF-M, + mbed-os, CMSIS, Zephyr, CVEs — none found. FreeRTOS's CONTRIBUTING + points to the forum first; settled on a single good-citizen post under + the FreeRTOS forum **Libraries** category (no per-library +TCP + sub-category) covering all three, with a provenance note that the + driver is Arm-origin (also in TF-M/mbed-os/CMSIS) so a fix may belong + at the Arm source too. Not filing issues or a merge-ready PR — we + can't validate on real HW, and the aim is to put it on the record + without committing to chase follow-ups. + +### Deferred + +- **Re-sync to current upstream `main`.** Not done — out of scope for a + document-and-close story, and re-pinning is itself an untested driver + change. If upstream ever lands hardware-validated fixes for the three + defects, prefer re-syncing to that commit over carrying local patches. + +### Open questions + +- None outstanding. Forum post drafted for David to post under the + Libraries category at his discretion; in-repo docs land via this PR.