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

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

Hi James,

On 3/31/2026 9:29 AM, Jie Gan wrote:
> 
> 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.
> 

I missed one point in last message.

Is that acceptable to export the coresight_mutex from the core module?
Currently, the coresight_mutex is used within the module only.

Thanks,
Jie

> 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,
>>
>