[PATCH] coresight: etm4x: Fix timestamp bit field handling

[Suzuki K Poulose]

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 at arm.com>
>> Fixes: c382ee674c8b ("arm64/sysreg/tools: Move TRFCR definitions to 
>> sysreg")
>> Signed-off-by: Leo Yan <leo.yan at 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 at linaro.org>
>