On Fri, Dec 05, 2025 at 09:40:33AM +0530, Anshuman Khandual wrote:
On 01/12/25 4:51 PM, Leo Yan wrote:
It gives priority to TRBSR_EL1.EA (external abort); an external abort will immediately bail out and return an error.
This is an abrupt starting for a commit message without giving context. The rationale for the change needs to be explained rather than details of implementation.
Agreed. I will refine the commit log to state that we need to diagnose the error code based on the hierarchy information (EC and BSC).
Next, the syndrome decoding is refactored based on two levels of information: the EC (Event Class) bits and the BSC (Trace Buffer Status Code) bits.
If TRBSR_EL1.EC==0b000000, the driver continues parsing TRBSR_EL1.BSC to identify the specific trace buffer event. Otherwise, any non-zero TRBSR_EL1.EC is treated as an error.
For error cases, the driver prints an error string and dumps registers for debugging.
No additional checks are required for wrap mode beyond verifying the TRBSR_EL1.WRAP bit, even on units with overwrite errata, as this bit reliably indicates a buffer wrap.
Should the errata related changes be done in a separate patch instead ?
Good point. I will consider extracting the change; this would make the review clearer.
[...]
static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *handle, u64 trbsr) {
- const char *err_str; int ec = get_trbe_ec(trbsr); int bsc = get_trbe_bsc(trbsr);
- struct trbe_buf *buf = etm_perf_sink_config(handle);
- struct trbe_cpudata *cpudata = buf->cpudata;
WARN_ON(is_trbe_running(trbsr));
- if (is_trbe_trg(trbsr) || is_trbe_abort(trbsr))
return TRBE_FAULT_ACT_FATAL;
- if ((ec == TRBE_EC_STAGE1_ABORT) || (ec == TRBE_EC_STAGE2_ABORT))
return TRBE_FAULT_ACT_FATAL;
- if (is_trbe_abort(trbsr)) {
err_str = "External abort";goto out_fatal;- }
- /*
* If the trbe is affected by TRBE_WORKAROUND_OVERWRITE_FILL_MODE,* it might write data after a WRAP event in the fill mode.* Thus the check TRBPTR == TRBBASER will not be honored.*/- if ((is_trbe_wrap(trbsr) && (ec == TRBE_EC_OTHERS) && (bsc == TRBE_BSC_FILLED)) &&
(trbe_may_overwrite_in_fill_mode(cpudata) ||get_trbe_write_pointer() == get_trbe_base_pointer()))
- switch (ec) {
- case TRBE_EC_OTHERS:
break;No message for this ?
No. EC_OTHERS means "this is a normal maintenance interrupt".
- case TRBE_EC_BUF_MGMT_IMPL:
err_str = "Unexpected implemented management";goto out_fatal;- case TRBE_EC_GP_CHECK_FAULT:
err_str = "Granule Protection Check fault";goto out_fatal;- case TRBE_EC_STAGE1_ABORT:
err_str = "Stage 1 data abort";goto out_fatal;- case TRBE_EC_STAGE2_ABORT:
err_str = "Stage 2 data abort";goto out_fatal;- default:
err_str = "Unknown error code";goto out_fatal;- }
- switch (bsc) {
- case TRBE_BSC_NOT_STOPPED:
break;- case TRBE_BSC_FILLED:
break;- case TRBE_BSC_TRIGGERED:
err_str = "Unexpected trigger status";goto out_fatal;- default:
err_str = "Unexpected buffer status code";goto out_fatal;- }
Just wondering if it would be cleaner to add a const char * based static array mapping these EC/BSC codes with above error messages.
Now we have two level's (EC/BSC) error codes, it would be complex to maintain a string array and map to two level's error codes.
- if (is_trbe_wrap(trbsr))
But what about TRBE affected with trbe_may_overwrite_in_fill_mode() is that still being taken care of some how ?
The TRBSR_EL1.WRAP bit reliably indicates that the pointer has wrapped. I don't think we need any special handling for the overwrite erratum here; we can simply return TRBE_FAULT_ACT_WRAP.
Afterward, trbe_get_trace_size() consumes the wrap flag and will compute the trace size appropriately for the overwrite erratum when the wrap flag is set.
[...]
-#define TRBE_EC_OTHERS 0 -#define TRBE_EC_STAGE1_ABORT 36 -#define TRBE_EC_STAGE2_ABORT 37 +#define TRBE_EC_OTHERS 0x0 +#define TRBE_EC_GP_CHECK_FAULT 0X1e +#define TRBE_EC_BUF_MGMT_IMPL 0x1f +#define TRBE_EC_STAGE1_ABORT 0x24 +#define TRBE_EC_STAGE2_ABORT 0x25
Please do mention the document source for these new EC codes in the commit message.
As James suggested in another reply, I will consider to add sysreg enum.
Thanks, Leo