From 9b0d3b81739b5bf01f153447704efcbb7fb35bb3 Mon Sep 17 00:00:00 2001
From: Flavien Solt <flavien.solt97@gmail.com>
Date: Fri, 8 May 2026 22:21:41 +0800
Subject: [PATCH] soc_event_generator: preserve r_err set on concurrent APB
 read race

The single always_ff at line 197 mixes blocking (=) and non-blocking
(<=) assignments and writes r_err on two paths:

  - the per-source set loop:
        for (int i=0;i<EVNT_NUM;i++)
            if(s_err[i])
                r_err[i] = 1'b1;     // blocking
  - the APB read-clear branch (PSEL && PENABLE && ~PWRITE):
        case (s_apb_addr)
            REG_ERR_0: r_err[31:0]  = 'h0;   // blocking
            ...

Both use blocking assignments and execute in lexical order. The set
loop runs first, then the read-clear runs and overwrites the bits
just set. If a queue overflow at index i in [0..31] fires (s_err[i]
high) on the same HCLK edge as the data phase of a SW read of
REG_ERR_0, the cycle plays out as:

  step 1: r_err[i]    = 1'b1;
  step 2: r_err[31:0] = 'h0;   <-- nukes step 1 for i in [0,31]

The new error is silently dropped. Software does not see it via
PRDATA either, because the read multiplexer captures the *pre-cycle*
value of r_err[31:0] in a separate always_comb. The overflow is
invisible to software both in the read result and in subsequent
reads.

The race window is the heavily-loaded scenario in which SW would
*want* to drain errors: soc_event_queue.err_o is high precisely when
its 2-bit counter is at max and a new event_i arrives.

Reproducer (Verilator 5.046):
  - APB-write REG_FC_MASK_0 = 0 so per_events_i[0] gets routed and
    the testbench's fc_event_ready_i = 0 prevents the arbiter from
    draining the queue.
  - Pulse per_events_i[0] until soc_event_queue's counter saturates
    and r_err[0] sticky-sets to 1.
  - APB-read REG_ERR_0 while pulsing per_events_i[0] in the data
    phase. Pre-read r_err[0] = 1; post-read r_err[0] = 0 (the
    in-flight overflow event was lost).
  - With this commit applied, post-read r_err[0] = 1 (sticky).

Fix: move the per-source set loop to *after* the APB read-clear
branch within the else block. Same lexical ordering rule, but with
set after clear so the sticky-set wins on a same-cycle race. Other
behaviour (mask register writes, r_apb_events default) is untouched.
---
 rtl/pulp_soc/soc_event_generator.sv | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/rtl/pulp_soc/soc_event_generator.sv b/rtl/pulp_soc/soc_event_generator.sv
index 7dc0c207..615d86d4 100644
--- a/rtl/pulp_soc/soc_event_generator.sv
+++ b/rtl/pulp_soc/soc_event_generator.sv
@@ -206,9 +206,6 @@ module soc_event_generator #(
       r_err             =  'h0;
     end
     else begin
-      for (int i=0;i<EVNT_NUM;i++)
-        if(s_err[i])
-          r_err[i] = 1'b1;
       r_apb_events     =  'h0;
       if (PSEL && PENABLE && PWRITE) begin
         case (s_apb_addr)
@@ -341,6 +338,12 @@ module soc_event_generator #(
               r_err[255:224] = 'h0;
         endcase // s_apb_addr
       end
+      // Sticky-set per-source error bits AFTER any APB read-clear so
+      // that an event firing on the same cycle as the read survives.
+      // Set wins over clear in the same-cycle race.
+      for (int i=0;i<EVNT_NUM;i++)
+        if(s_err[i])
+          r_err[i] = 1'b1;
     end
   end //always