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

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

On 3/31/2026 5:26 PM, James Clark wrote:
> 
> 
> On 31/03/2026 4:18 am, Jie Gan wrote:
>> 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
>>
> 
> If the plan is to only check for non-zero trace ID and not check the 
> mode any more then you don't need to lock. Maybe lets see what Suzuki 
> thinks about returning the last trace ID though as it was his idea to 
> add -EINVAL.
> 

Sure. The trace ID is allocated during TPDA device probe and remains 
unchanged for the entire lifetime of the device.

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