The question

“Here is a block you have never seen. Review it in 30 minutes and tell us what you find.”

The question tests triage, not completeness. The signal is a fixed pass order that finds load-bearing defects first and says what was skipped.

The pass order

Work from the outside in. Interfaces first, then control, then reset and clocks, then datapath timing, then verification hooks. Each pass has one question, one typical defect and a fixed slice of the 30 minutes.

Read it top to bottom; when a box's minutes run out, note what is unchecked and move on.

• The boundary passes come first because their defects cost the most: widths, protocols, resets and crossings.

• The report has its own 3 minutes. A finding that is never written down does not exist.

Timebox each pass. A review that spends twenty minutes on the ALU and never checks the interfaces has failed the exercise.

A worked review

The block below counts bytes on a valid/ready packet stream. It is about 20 lines long and has four seeded bugs, one for each of the first four passes. Try the passes yourself before reading the findings.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

// yosys: latch
// Review target: a byte counter for a valid/ready packet stream. Four bugs are seeded.
module pkt_stat (
  input  logic       clk, rst_n,
  input  logic       in_valid,
  input  logic [7:0] in_len,
  output logic       in_ready,
  input  logic       clr,          // from the CPU clock domain
  output logic [7:0] total         // bytes seen since the last clear
);
  typedef enum logic [1:0] {IDLE, ACC, HOLD} st_t;
  st_t st;
  logic [7:0] len_q;
  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) begin st <= IDLE; total <= '0; end
    else case (st)
      IDLE: if (in_valid) begin len_q <= in_len; st <= ACC; end
      ACC:  begin total <= total + len_q; st <= HOLD; end
      HOLD: if (clr) begin total <= '0; st <= IDLE; end
    endcase
  always_comb
    if (st == IDLE) in_ready = 1'b1;
endmodule

Each finding is written the way it would go in the report: location, trigger, consequence, then the fix.

Read down the first column: each pass found the bug it is designed to find, and the fifth pass found a gap rather than a bug. Two things were looked at and deliberately not reported.

• len_q has no reset value. It is written in IDLE before ACC reads it, so the value is never observed. Not a finding.

• The state encoding and the names are style. Skimmed and said so.

The same block after the review, with a bench that checks each fix:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33

// The same block after the review: each fix is marked with its finding number.
module pkt_stat (
  input  logic        clk, rst_n,
  input  logic        in_valid,
  input  logic [7:0]  in_len,
  output logic        in_ready,
  input  logic        clr_async,     // level from the CPU clock domain, held for at least 3 clk cycles
  output logic [15:0] total          // fix 1: width from the spec (here 16 bits), not from in_len
);
  typedef enum logic [1:0] {IDLE, ACC} st_t;
  st_t st;
  logic [7:0] len_q;
  logic [1:0] clr_ff;                // fix 3: two-flop synchroniser on the crossing
  logic       clr;
  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) clr_ff <= 2'b00;
    else        clr_ff <= {clr_ff[0], clr_async};
  assign clr = clr_ff[1];

  always_ff @(posedge clk or negedge rst_n)
    if (!rst_n) begin
      st <= IDLE; total <= '0; len_q <= '0;
    end else if (clr) begin
      st <= IDLE; total <= '0;
    end else begin
      case (st)
        IDLE:    if (in_valid) begin len_q <= in_len; st <= ACC; end
        ACC:     begin total <= total + len_q; st <= IDLE; end   // fix 2: back to IDLE, no stall
        default: st <= IDLE;
      endcase
    end
  assign in_ready = (st == IDLE) && !clr;   // fix 4: continuous assignment, no latch
endmodule

Checklist

• Interfaces: every output driven; widths match the spec at both ends; the protocol and its clock named for each port.

• Control: every state reachable; every wait has an exit that does not depend on software; a default arm.

• Reset and clocks: every input from another clock synchronised or handshaken; reset release synchronised per domain.

• Datapath and timing: no latch (every combinational output assigned on every path); the deepest unregistered path named.

• Hooks: an assertion or status flag on the key invariant, such as overflow or a handshake in the wrong state.

• Report: one line per finding with location, trigger and consequence; a list of what was skimmed.

Follow-ups

“What do you skip?” Transistor-level sizing and style nits. Flag only what changes function, timing, or debuggability, and say the rest was skimmed.

“How do you report a finding?” One line per defect: location, trigger condition, and consequence. Then the fix and the check that would close it.

“The designer is in the room. How do you start?” Restate the block contract in one sentence and ask for correction. A wrong mental model reviewed confidently is worse than a slow review.

Common wrong answers

A weaker answer narrates the code top to bottom with no pass order. That tour treats a misplaced comment and a missing reset as equals.

Another weak answer lists suspicions with no trigger. Every finding needs the condition that exposes it, or it is not a finding.

Related topics

• Reset strategies and release timing