Re: [PATCH v3] coresight: tpdm: add traceid_show for checking traceid

[Jie Gan <jie.gan@oss.qualcomm.com>]

Hi James,

On 3/30/2026 10:55 PM, James Clark wrote:
> 
> 
> On 25/03/2026 3:10 am, Jie Gan wrote:
>> Save the trace ID in drvdata during TPDM enablement and expose it
>> to userspace to support trace data parsing.
>>
>> The TPDM device’s trace ID corresponds to the trace ID allocated
>> to the connected TPDA device.
>>
>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
>> ---
>> Changes in v3:
>> 1. Only allow user to read the traceid while the TPDM device is enabled.
>> - Link to v2: https://lore.kernel.org/r/20260316-add-traceid-show-for- 
>> tpdm-v2-1-1dec2a67e4ed@oss.qualcomm.com
>>
>> Changes in V2:
>> 1. Use sysfs_emit instead of sprintf.
>> Link to V1 - https://lore.kernel.org/all/20260306-add-traceid-show- 
>> for-tpdm-v1-1-0658a8edb972@oss.qualcomm.com/
>> ---
>>   drivers/hwtracing/coresight/coresight-tpdm.c | 34 ++++++++++++++++++ 
>> +++++++++-
>>   drivers/hwtracing/coresight/coresight-tpdm.h |  2 ++
>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/ 
>> hwtracing/coresight/coresight-tpdm.c
>> index da77bdaad0a4..c8339b973bfc 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.c
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.c
>> @@ -481,7 +481,7 @@ static void __tpdm_enable(struct tpdm_drvdata 
>> *drvdata)
>>   static int tpdm_enable(struct coresight_device *csdev, struct 
>> perf_event *event,
>>                  enum cs_mode mode,
>> -               __maybe_unused struct coresight_path *path)
>> +               struct coresight_path *path)
>>   {
>>       struct tpdm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> @@ -497,6 +497,7 @@ static int tpdm_enable(struct coresight_device 
>> *csdev, struct perf_event *event,
>>       }
>>       __tpdm_enable(drvdata);
>> +    drvdata->traceid = path->trace_id;
>>       drvdata->enable = true;
>>       spin_unlock(&drvdata->spinlock);
>> @@ -693,6 +694,29 @@ static struct attribute_group tpdm_attr_grp = {
>>       .attrs = tpdm_attrs,
>>   };
>> +static ssize_t traceid_show(struct device *dev,
>> +                struct device_attribute *attr, char *buf)
>> +{
>> +    unsigned long val;
>> +    struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
>> +
>> +    if (coresight_get_mode(drvdata->csdev) == CS_MODE_DISABLED)
>> +        return -EINVAL;
>> +
>> +    val = drvdata->traceid;
> 
> You probably need to take the coresight_mutex here otherwise you could 
> still return an invalid or stale value despite checking the mode.
> 

Acked. I have missed this potential race condition.

> There might also be some value in it returning the last used trace ID 
> even if the mode isn't enabled anymore. Because you can still read out 
> of the sink after disabling, so it makes more sense for a script to read 
> it at that point rather than when it's enabled. Also, you probably don't 
> want to be doing other things in your script in the point between 
> enabling and disabling.

That's making sense. I shouldnt add such restriction for the read process.

Scenarios for reading:
1. device is enabled -> trace ID is valid
2. device is enabled then disabled -> trace ID is valid for the last 
trace event
3. device is never enabled -> invalid trace ID (value 0)

we only need to check the validation of the trace ID.

mutex_lock(&coresight_mutex);
val = drvdata->traceid;
mutex_unlock(&coresight_mutex);

if (!val)
     return -EINVAL;

return sysfs_emit(buf, "%#lx\n", val);

Thanks,
Jie

> 
>> +    return sysfs_emit(buf, "%#lx\n", val);
>> +}
>> +static DEVICE_ATTR_RO(traceid);
>> +
>> +static struct attribute *traceid_attrs[] = {
>> +    &dev_attr_traceid.attr,
>> +    NULL,
>> +};
>> +
>> +static struct attribute_group traceid_attr_grp = {
>> +    .attrs = traceid_attrs,
>> +};
>> +
>>   static ssize_t dsb_mode_show(struct device *dev,
>>                    struct device_attribute *attr,
>>                    char *buf)
>> @@ -1367,6 +1391,12 @@ static const struct attribute_group 
>> *tpdm_attr_grps[] = {
>>       &tpdm_cmb_patt_grp,
>>       &tpdm_cmb_msr_grp,
>>       &tpdm_mcmb_attr_grp,
>> +    &traceid_attr_grp,
>> +    NULL,
>> +};
>> +
>> +static const struct attribute_group *static_tpdm_attr_grps[] = {
>> +    &traceid_attr_grp,
>>       NULL,
>>   };
>> @@ -1425,6 +1455,8 @@ static int tpdm_probe(struct device *dev, struct 
>> resource *res)
>>       desc.access = CSDEV_ACCESS_IOMEM(base);
>>       if (res)
>>           desc.groups = tpdm_attr_grps;
>> +    else
>> +        desc.groups = static_tpdm_attr_grps;
>>       drvdata->csdev = coresight_register(&desc);
>>       if (IS_ERR(drvdata->csdev))
>>           return PTR_ERR(drvdata->csdev);
>> diff --git a/drivers/hwtracing/coresight/coresight-tpdm.h b/drivers/ 
>> hwtracing/coresight/coresight-tpdm.h
>> index 2867f3ab8186..11da64e1ade8 100644
>> --- a/drivers/hwtracing/coresight/coresight-tpdm.h
>> +++ b/drivers/hwtracing/coresight/coresight-tpdm.h
>> @@ -300,6 +300,7 @@ struct cmb_dataset {
>>    * @cmb         Specifics associated to TPDM CMB.
>>    * @dsb_msr_num Number of MSR supported by DSB TPDM
>>    * @cmb_msr_num Number of MSR supported by CMB TPDM
>> + * @traceid    Trace ID of the path.
>>    */
>>   struct tpdm_drvdata {
>> @@ -313,6 +314,7 @@ struct tpdm_drvdata {
>>       struct cmb_dataset    *cmb;
>>       u32            dsb_msr_num;
>>       u32            cmb_msr_num;
>> +    u8            traceid;
>>   };
>>   /* Enumerate members of various datasets */
>>
>> ---
>> base-commit: b84a0ebe421ca56995ff78b66307667b62b3a900
>> change-id: 20260316-add-traceid-show-for-tpdm-88d040651f00
>>
>> Best regards,
>