Reviewing someone else's plan is a principal engineer's most frequent planning job: the plan exists, the team is confident, and the review is where its blind spots are found or not. The plan below is for a DMA engine with four channels, descriptor chains in memory, an AXI master port and an interrupt per channel. It is plausible, tidy and wrong in eleven places.
Your task: find the flaws, rank them by the risk they leave, and write the comment you would give the author. Aim for at least eight before opening the answers.
Section | Content |
|---|---|
Scope | DMA engine RTL, shipped as configurable IP with 1 to 8 channels; verified in its default configuration of 4 channels; AXI4 master; APB registers |
Features | F1 register access; F2 single-descriptor copy; F3 descriptor chains (lengths 1, 2, 4); F4 four channels active; F5 interrupts on completion; F6 error response from memory; F7 channel priority |
Checks | A scoreboard compares memory contents after each test with the reference model's |
Reference model | C++ model written by the RTL designer from the RTL, to save time |
Stimulus | Constrained-random descriptors, 50 seeds per feature; memory model responds in 2 cycles |
Coverage | 100% code coverage (line, branch, toggle) |
Formal | Not planned: simulation is sufficient |
Effort | Two engineers, eight weeks, split evenly across the seven features |
Sign-off | All tests pass and coverage is 100% |
Write your list, then rank it: which three flaws, if left, are most likely to let a serious bug escape?
# | Flaw | Why it matters |
|---|---|---|
1 | "100% code coverage (line, branch, toggle)" as the main completeness measure | Code coverage shows what ran, not what was checked or what the specification requires (lessons 6.1, 6.2); there is no functional coverage model. |
2 | "A scoreboard compares memory contents after each test" is the only check | One check for everything: interrupt timing, error status and ordering between channels have no checks (lesson 5.2). |
3 | The reference model is written by the RTL designer from the RTL | Not independent: it shares the designer's reading of the specification (lessons 2.3, 5.5). |
4 | "Constrained-random descriptors, 50 seeds per feature", including the error responses | Error injection needs deliberate stimulus; random legal traffic never produces bus errors (lesson 4.7). |
5 | "Memory model responds in 2 cycles" | No timing variation, so stalls, reordering and full queues are never exercised (lesson 4.8). |
6 | No features for reset in the middle of a transfer or for channel abort | Interaction features missing; escapes cluster here (lessons 1.8, 9.2). |
7 | Descriptor-chain length tested only with lengths 1, 2 and 4 | Boundaries missing: 0, maximum, and lengths that cross a burst or page boundary (lesson 1.3). |
8 | Formal: "not planned, simulation is sufficient" | The four-channel arbiter and the descriptor state machine are small control logic where formal wins (lesson 7.5); the decision needs a reason. |
9 | Sign-off: "all tests pass and coverage is 100%" | No planted-bug or mutation evidence, no bug-trend criterion, no open-bug decision, no residual-risk statement (lessons 14.2, 6.8). |
10 | Features have no risk column; effort is spread evenly | No prioritisation: low-risk register tests get the same effort as the arbiter (lesson 9.3). |
11 | "Verified in its default configuration of 4 channels" for IP shipped with 1 to 8 channels | Configurations untested: extremes and customer builds missing (lesson 15.7). |
How many did you find? Reviews rarely find everything on the first pass; rank before you count.
Most dangerous: (3) the reference model copied from the RTL, because it makes the only check blind to every misreading of the specification; (2) a single end-of-test memory check, because timing, interrupts, errors and ordering are unchecked; (9) the sign-off criteria, because nothing shows the checks can detect a bug. Next: (4) and (5), because error and stall paths are never exercised; (6) and (7), missing interactions and boundaries; (8) formal for the arbiter; (11) configurations; (1) and (10) are symptoms of the same planning gap. Comment to the author: "The plan measures activity, not detection. Before effort is committed: an independent reference model from the specification; checks per interface (AXI protocol, interrupt timing, error status, per-channel ordering); error injection and variable memory latency; interaction and boundary features with risk ranking; formal on the arbiter and descriptor FSM, or a written reason; a functional coverage model; sign-off with planted bugs, bug trend and residual risk."
It reproduces the RTL's interpretation of the specification, so the scoreboard agrees with every misreading; the only check becomes blind to specification bugs.
It shows which lines ran, not whether their results were checked or whether the specified features and combinations were exercised.
Review a plan by asking what bug would get past each section.
The most dangerous flaws blind the checks: copied oracles, single checks, no detection evidence.
Rank review comments by escape risk, and write them as actions.