On 19/05/2025 15:11, James Clark wrote:
On 19/05/2025 2:50 pm, Leo Yan wrote:
Timestamps in the trace data appear as all zeros on recent kernels, although the feature works correctly on old kernels (e.g., v6.12).
Since commit c382ee674c8b ("arm64/sysreg/tools: Move TRFCR definitions to sysreg"), the TRFCR_ELx_TS_{VIRTUAL|GUEST_PHYSICAL|PHYSICAL} macros were updated to remove the bit shift. As a result, the driver no longer shifts bits when operates the timestamp field.
Fix this by using the FIELD_PREP() and FIELD_GET() helpers. Simplify the ts_source_show() function: return -1 when the value is zero, as this indciates an invalid value; otherwise return the decoded TS value directly.
Reported-by: Tamas Zsoldos tamas.zsoldos@arm.com Fixes: c382ee674c8b ("arm64/sysreg/tools: Move TRFCR definitions to sysreg") Signed-off-by: Leo Yan leo.yan@arm.com
.../hwtracing/coresight/coresight-etm4x-core.c | 2 +- .../hwtracing/coresight/coresight-etm4x-sysfs.c | 17 ++--------------- 2 files changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/ drivers/hwtracing/coresight/coresight-etm4x-core.c index 6a5898355a83..acb4a58e4bb9 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -1237,7 +1237,7 @@ static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata) * tracing at the kernel EL and EL0, forcing to use the * virtual time as the timestamp. */ - trfcr = (TRFCR_EL1_TS_VIRTUAL | + trfcr = (FIELD_PREP(TRFCR_EL1_TS_MASK, TRFCR_EL1_TS_VIRTUAL) | TRFCR_EL1_ExTRE | TRFCR_EL1_E0TRE); diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/ drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index 49d5fb87a74b..8a2749eeb9a5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -2315,23 +2315,10 @@ static ssize_t ts_source_show(struct device *dev, int val; struct etmv4_drvdata *drvdata = dev_get_drvdata(dev->parent); - if (!drvdata->trfcr) { + val = FIELD_GET(TRFCR_EL1_TS_MASK, drvdata->trfcr); + if (!val)
I think this might be problematic. TS==0 is a reserved value for software, and doesn't imply a TS is not in effect. Thus I think we should retain the older check as before to ensure TRFCR is effective.
The rest looks good to me.
Suzuki
val = -1; - goto out; - }
- switch (drvdata->trfcr & TRFCR_EL1_TS_MASK) { - case TRFCR_EL1_TS_VIRTUAL: - case TRFCR_EL1_TS_GUEST_PHYSICAL: - case TRFCR_EL1_TS_PHYSICAL: - val = FIELD_GET(TRFCR_EL1_TS_MASK, drvdata->trfcr); - break; - default: - val = -1; - break; - } -out: return sysfs_emit(buf, "%d\n", val); } static DEVICE_ATTR_RO(ts_source);
Reviewed-by: James Clark james.clark@linaro.org